Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH 00/11] JZ4740 SoC cleanup
From: Richard Weinberger @ 2019-07-29 18:08 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Richard Weinberger, Ralf Baechle, Paul Burton, James Hogan,
	Rob Herring, Mark Rutland, Vinod Koul, Jean Delvare,
	Guenter Roeck, Lee Jones, Miquel Raynal, Sebastian Reichel,
	Bartlomiej Zolnierkiewicz, Liam Girdwood, Mark Brown, linux-hwmon,
	devicetree, linux-fbdev, alsa-devel, linux-pm, linux-mips, DRI
In-Reply-To: <1564419921.1759.1@crapouillou.net>

----- Ursprüngliche Mail -----
>> Was this series tested with the Ben Nanonote device?
>> I have one of these and from time to time I upgrade the kernel on it.
> 
> Yes! Artur (Cc'd) tested it.
> 
> You can test it yourself, after merging this patchset with:
> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git
> branch next,
> git://git.freedesktop.org/git/drm-misc branch drm-misc-next.
> 
> These will be in 5.4-rc1.

Awesome! Thanks a lot for cleaning this up.

Thanks,
//richard

^ permalink raw reply

* Re: [PATCH v3 00/10] implement poweroff for mt6323 / bpi-r2
From: Frank Wunderlich @ 2019-07-29 18:16 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Allison Randal,
	David S. Miller, devicetree, Eddie Huang, Greg Kroah-Hartman,
	Jonathan Cameron, Kate Stewart, Lee Jones, linux-arm-kernel,
	linux-kernel, linux-mediatek, linux-pm, linux-rtc, Mark Rutland,
	Matthias Brugger, Mauro Carvalho Chehab, Nicolas Ferre,
	Richard Fontana, Rob Herring, Sean Wang,
	Sebastian Reichel <sre>
In-Reply-To: <20190729174154.4335-1-frank-w@public-files.de>

Hi,

sorry about missing part 9+10, my mail-provider blocks the mails, 
currently i cannot send with this account

i try sending with another

regards Frank

^ permalink raw reply

* Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line
From: Uwe Kleine-König @ 2019-07-29 18:20 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Chen-Yu Tsai, Jernej Skrabec, Thierry Reding, Rob Herring,
	Mark Rutland, linux-pwm-u79uwXL29TY76Z2rM5mHXA, devicetree,
	linux-arm-kernel, linux-kernel, linux-sunxi, Philipp Zabel
In-Reply-To: <20190729163715.vtv7lkrywewomxga-YififvaboMKzQB+pC5nmwQ@public.gmane.org>

On Mon, Jul 29, 2019 at 06:37:15PM +0200, Maxime Ripard wrote:
> On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> > > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > > > >       if (IS_ERR(pwm->clk))
> > > > >               return PTR_ERR(pwm->clk);
> > > > >
> > > > > +     if (pwm->data->has_reset) {
> > > > > +             pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > > +             if (IS_ERR(pwm->rst))
> > > > > +                     return PTR_ERR(pwm->rst);
> > > > > +
> > > > > +             reset_control_deassert(pwm->rst);
> > > > > +     }
> > > > > +
> > > >
> > > > I wonder why there is a need to track if a given chip needs a reset
> > > > line. I'd just use devm_reset_control_get_optional() and drop the
> > > > .has_reset member in struct sun4i_pwm_data.
> > >
> > > Because it's not optional for this platform, i.e. it won't work if
> > > the reset control (or clk, in the next patch) is somehow missing from
> > > the device tree.
> >
> > If the device tree is wrong it is considered ok that the driver doesn't
> > behave correctly. So this is not a problem you need (or should) care
> > about.
> 
> To some extent that's true, but if we can make the life easier for
> everyone by reporting an error and bailing out instead of silently
> ignoring that, continuing to probe and just ending up with a
> completely broken system for no apparent reason, then why not just do
> that?
> 
> I mean, all it takes is three lines of code.

<pedantic>Actually it's more because you need to track for each variant
if it needs the clock (or reset stuff) or not.</pedantic>

It's a weighing between "simpler driver" and "catch unlikely errors".
(If the SoC's .dtsi includes the needed stuff, an error here is really
unlikely, isn't it?)

So to some degree it's subjective which one is better. I tend to prefer
"simpler driver". Also when catching this type of error you have to do
it right twice (in the .dtsi and the driver) while with my approach
there is only a single place that defines what is right, which is a good
design according to what I learned during my studies.

> It's no different than just calling clk_get, and testing the return
> code to see if it's there or not. I wouldn't call that check when you
> depend on a clock "validating the DT". It's just making sure that all
> the resources needed for you to operate properly are there, which is a
> pretty common thing to do.

This is different. As a driver author you are allowed (or even supposed)
to assume that the device tree (or ACPI or platform data ...) is right
and completely defines the stuff for the driven hardware to work
correctly. You must not assume that clk_get() succeeds unconditionally.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729182046.6bwrterbxoceulrx%40pengutronix.de.

^ permalink raw reply

* [PATCH v3 09/10] MAINTAINERS: add Mediatek shutdown drivers
From: Frank Wunderlich @ 2019-07-29 18:24 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Allison Randal,
	David S. Miller, devicetree, Eddie Huang, Greg Kroah-Hartman,
	Jonathan Cameron, Kate Stewart, Lee Jones, linux-arm-kernel,
	linux-kernel, linux-mediatek, linux-pm, linux-rtc, Mark Rutland,
	Matthias Brugger, Mauro Carvalho Chehab, Nicolas Ferre,
	Richard Fontana, Rob Herring, Sean Wang,
	Sebastian Reichel <sre>
  Cc: Josef Friedl, Frank Wunderlich

From: Josef Friedl <josef.friedl@speed.at>

add Section in MAINTAINERS file for poweroff driver

changes since v2: none (=v2 part 6)

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Josef Friedl <josef.friedl@speed.at>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6426db5198f0..4172a3177633 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10128,6 +10128,13 @@ S:	Maintained
 F:	drivers/net/dsa/mt7530.*
 F:	net/dsa/tag_mtk.c
 
+MEDIATEK BOARD LEVEL SHUTDOWN DRIVERS
+M:	Sean Wang <sean.wang@mediatek.com>
+L:	linux-pm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/power/reset/mt6323-poweroff.txt
+F:	drivers/power/reset/mt6323-poweroff.c
+
 MEDIATEK JPEG DRIVER
 M:	Rick Chang <rick.chang@mediatek.com>
 M:	Bin Liu <bin.liu@mediatek.com>
-- 
2.17.1

^ permalink raw reply related

* [PATCH v3 10/10] arm: dts: mt6323: add keys, power-controller, rtc and codec
From: Frank Wunderlich @ 2019-07-29 18:24 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Allison Randal,
	David S. Miller, devicetree, Eddie Huang, Greg Kroah-Hartman,
	Jonathan Cameron, Kate Stewart, Lee Jones, linux-arm-kernel,
	linux-kernel, linux-mediatek, linux-pm, linux-rtc, Mark Rutland,
	Matthias Brugger, Mauro Carvalho Chehab, Nicolas Ferre,
	Richard Fontana, Rob Herring, Sean Wang,
	Sebastian Reichel <sre>
  Cc: Josef Friedl, Frank Wunderlich

From: Josef Friedl <josef.friedl@speed.at>

support poweroff and power-related keys on bpi-r2

changes since v2: none (=v2 part 7)

Suggested-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Josef Friedl <josef.friedl@speed.at>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 arch/arm/boot/dts/mt6323.dtsi | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/mt6323.dtsi b/arch/arm/boot/dts/mt6323.dtsi
index ba397407c1dd..7fda40ab5fe8 100644
--- a/arch/arm/boot/dts/mt6323.dtsi
+++ b/arch/arm/boot/dts/mt6323.dtsi
@@ -238,5 +238,32 @@
 				regulator-enable-ramp-delay = <216>;
 			};
 		};
+
+		mt6323keys: mt6323keys {
+			compatible = "mediatek,mt6323-keys";
+			mediatek,long-press-mode = <1>;
+			power-off-time-sec = <0>;
+
+			power {
+				linux,keycodes = <116>;
+				wakeup-source;
+			};
+
+			home {
+				linux,keycodes = <114>;
+			};
+		};
+
+		codec: mt6397codec {
+			compatible = "mediatek,mt6397-codec";
+		};
+
+		power-controller {
+			compatible = "mediatek,mt6323-pwrc";
+		};
+
+		rtc {
+			compatible = "mediatek,mt6323-rtc";
+		};
 	};
 };
-- 
2.17.1

^ permalink raw reply related

* [PATCH v2 1/3] dt-bindings: Document JZ47xx VPU auxiliary processor
From: Paul Cercueil @ 2019-07-29 18:31 UTC (permalink / raw)
  To: Ohad Ben-Cohen, Bjorn Andersson, Rob Herring, Mark Rutland
  Cc: Paul Burton, od, linux-remoteproc, devicetree, linux-kernel,
	Paul Cercueil

Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from
Ingenic is a second Xburst MIPS CPU very similar to the main core.
This document describes the devicetree bindings for this auxiliary
processor.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: Update TCSM0 address in example

 .../bindings/remoteproc/ingenic,vpu.txt       | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.txt

diff --git a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.txt b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.txt
new file mode 100644
index 000000000000..576f9e582780
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.txt
@@ -0,0 +1,36 @@
+* Ingenic JZ47xx auxiliary processor
+
+Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from Ingenic
+is a second Xburst MIPS CPU very similar to the main core.
+This document describes the devicetree bindings for this auxiliary processor.
+
+Required properties:
+- compatible: Should be "ingenic,jz4770-vpu-rproc"
+- reg: Must contain the registers location and length for:
+  * the auxiliary processor,
+  * the Tightly Coupled Shared Memory 0 (TCSM0),
+  * the Tightly Coupled Shared Memory 1 (TCSM1),
+  * the shared SRAM.
+- reg-names: Must contain "aux", "tcsm0", "tcsm1", "sram".
+- clocks: Clock specifier for the AUX and VPU clocks.
+- clock-names: Must contain "aux", "vpu".
+- interrupts: Interrupt specifier for the VPU hardware block.
+
+Example:
+
+vpu: cpu@132a0000 {
+	compatible = "ingenic,jz4770-vpu-rproc";
+
+	reg = <0x132a0000 0x20 /* AUX */
+		   0x132b0000 0x4000 /* TCSM0 */
+		   0x132c0000 0xc000 /* TCSM1 */
+		   0x132f0000 0x7000 /* SRAM */
+	>;
+	reg-names = "aux", "tcsm0", "tcsm1", "sram";
+
+	clocks = <&cgu JZ4770_CLK_AUX>, <&cgu JZ4770_CLK_VPU>;
+	clock-names = "aux", "vpu";
+
+	interrupt-parent = <&cpuintc>;
+	interrupts = <3>;
+};
-- 
2.21.0.593.g511ec345e18

^ permalink raw reply related

* [PATCH v2 2/3] remoteproc: Add prepare/unprepare callbacks
From: Paul Cercueil @ 2019-07-29 18:31 UTC (permalink / raw)
  To: Ohad Ben-Cohen, Bjorn Andersson, Rob Herring, Mark Rutland
  Cc: Paul Burton, od, linux-remoteproc, devicetree, linux-kernel,
	Paul Cercueil
In-Reply-To: <20190729183109.18283-1-paul@crapouillou.net>

The .prepare() callback is called before the firmware is loaded to
memory. This is useful for instance in the case where some setup is
required for the memory to be accessible.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: No change

 drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
 include/linux/remoteproc.h           |  4 ++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3c5fbbbfb0f1..68faad3ef0c1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1300,11 +1300,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	struct device *dev = &rproc->dev;
 	int ret;
 
+	if (rproc->ops->prepare) {
+		ret = rproc->ops->prepare(rproc);
+		if (ret) {
+			dev_err(dev, "Failed to prepare rproc: %d\n", ret);
+			return ret;
+		}
+	}
+
 	/* load the ELF segments to memory */
 	ret = rproc_load_segments(rproc, fw);
 	if (ret) {
 		dev_err(dev, "Failed to load program segments: %d\n", ret);
-		return ret;
+		goto unprepare_rproc;
 	}
 
 	/*
@@ -1355,6 +1363,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	rproc_unprepare_subdevices(rproc);
 reset_table_ptr:
 	rproc->table_ptr = rproc->cached_table;
+unprepare_rproc:
+	if (rproc->ops->unprepare)
+		rproc->ops->unprepare(rproc);
 
 	return ret;
 }
@@ -1484,6 +1495,9 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 
 	rproc->state = RPROC_OFFLINE;
 
+	if (rproc->ops->unprepare)
+		rproc->ops->unprepare(rproc);
+
 	dev_info(dev, "stopped remote processor %s\n", rproc->name);
 
 	return 0;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 16ad66683ad0..be65c2a92c58 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -355,6 +355,8 @@ enum rsc_handling_status {
 
 /**
  * struct rproc_ops - platform-specific device handlers
+ * @prepare:	prepare the device for power up (before the firmware is loaded)
+ * @unprepare:	unprepare the device after it is stopped
  * @start:	power on the device and boot it
  * @stop:	power off the device
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
@@ -371,6 +373,8 @@ enum rsc_handling_status {
  * @get_boot_addr:	get boot address to entry point specified in firmware
  */
 struct rproc_ops {
+	int (*prepare)(struct rproc *rproc);
+	void (*unprepare)(struct rproc *rproc);
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
-- 
2.21.0.593.g511ec345e18

^ permalink raw reply related

* [PATCH v2 3/3] remoteproc: ingenic: Added remoteproc driver
From: Paul Cercueil @ 2019-07-29 18:31 UTC (permalink / raw)
  To: Ohad Ben-Cohen, Bjorn Andersson, Rob Herring, Mark Rutland
  Cc: Paul Burton, od, linux-remoteproc, devicetree, linux-kernel,
	Paul Cercueil
In-Reply-To: <20190729183109.18283-1-paul@crapouillou.net>

This driver is used to boot, communicate with and load firmwares to the
MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
Ingenic.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: Remove exception for always-mapped memories

 drivers/remoteproc/Kconfig         |   8 +
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/ingenic_rproc.c | 285 +++++++++++++++++++++++++++++
 3 files changed, 294 insertions(+)
 create mode 100644 drivers/remoteproc/ingenic_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 28ed306982f7..a0be40e2098d 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -214,6 +214,14 @@ config STM32_RPROC
 
 	  This can be either built-in or a loadable module.
 
+config INGENIC_RPROC
+	tristate "Ingenic JZ47xx VPU remoteproc support"
+	depends on MIPS || COMPILE_TEST
+	help
+	  Say y or m here to support the VPU in the JZ47xx SoCs from Ingenic.
+	  This can be either built-in or a loadable module.
+	  If unsure say N.
+
 endif # REMOTEPROC
 
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 00f09e658cb3..6eb0137abbc7 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -10,6 +10,7 @@ remoteproc-y				+= remoteproc_sysfs.o
 remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
+obj-$(CONFIG_INGENIC_RPROC)			+= ingenic_rproc.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
new file mode 100644
index 000000000000..6fe0530c83a6
--- /dev/null
+++ b/drivers/remoteproc/ingenic_rproc.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Ingenic JZ47xx remoteproc driver
+ * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+#define REG_AUX_CTRL		0x0
+#define REG_AUX_MSG_ACK		0x10
+#define REG_AUX_MSG		0x14
+#define REG_CORE_MSG_ACK	0x18
+#define REG_CORE_MSG		0x1C
+
+#define AUX_CTRL_SLEEP		BIT(31)
+#define AUX_CTRL_MSG_IRQ_EN	BIT(3)
+#define AUX_CTRL_NMI_RESETS	BIT(2)
+#define AUX_CTRL_NMI		BIT(1)
+#define AUX_CTRL_SW_RESET	BIT(0)
+
+struct vpu_mem_map {
+	const char *name;
+	unsigned int da;
+};
+
+struct vpu_mem_info {
+	const struct vpu_mem_map *map;
+	unsigned long len;
+	void __iomem *base;
+};
+
+static const struct vpu_mem_map vpu_mem_map[] = {
+	{ "tcsm0", 0x132b0000 },
+	{ "tcsm1", 0xf4000000 },
+	{ "sram",  0x132f0000 },
+};
+
+/* Device data */
+struct vpu {
+	int irq;
+	struct clk *vpu_clk;
+	struct clk *aux_clk;
+	void __iomem *aux_base;
+	struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
+	struct device *dev;
+};
+
+static int ingenic_rproc_prepare(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+	int ret;
+
+	ret = clk_prepare_enable(vpu->vpu_clk);
+	if (ret) {
+		dev_err(vpu->dev, "Unable to start VPU clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(vpu->aux_clk);
+	if (ret) {
+		dev_err(vpu->dev, "Unable to start AUX clock: %d\n", ret);
+		goto err_disable_vpu_clk;
+	}
+
+	return 0;
+
+err_disable_vpu_clk:
+	clk_disable_unprepare(vpu->vpu_clk);
+	return ret;
+}
+
+static void ingenic_rproc_unprepare(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+
+	clk_disable_unprepare(vpu->aux_clk);
+	clk_disable_unprepare(vpu->vpu_clk);
+}
+
+static int ingenic_rproc_start(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+	u32 ctrl;
+
+	enable_irq(vpu->irq);
+
+	/* Reset the AUX and enable message IRQ */
+	ctrl = AUX_CTRL_NMI_RESETS | AUX_CTRL_NMI | AUX_CTRL_MSG_IRQ_EN;
+	writel(ctrl, vpu->aux_base + REG_AUX_CTRL);
+
+	return 0;
+}
+
+static int ingenic_rproc_stop(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+
+	/* Keep AUX in reset mode */
+	writel(AUX_CTRL_SW_RESET, vpu->aux_base + REG_AUX_CTRL);
+
+	disable_irq_nosync(vpu->irq);
+
+	return 0;
+}
+
+static void ingenic_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct vpu *vpu = rproc->priv;
+
+	writel(vqid, vpu->aux_base + REG_CORE_MSG);
+}
+
+static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+	struct vpu *vpu = rproc->priv;
+	void __iomem *va = NULL;
+	unsigned int i;
+
+	if (len <= 0)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
+		const struct vpu_mem_info *info = &vpu->mem_info[i];
+		const struct vpu_mem_map *map = info->map;
+
+		if (da >= map->da && (da + len) < (map->da + info->len)) {
+			va = info->base + (da - map->da);
+			break;
+		}
+	}
+
+	return (__force void *)va;
+}
+
+static struct rproc_ops ingenic_rproc_ops = {
+	.prepare = ingenic_rproc_prepare,
+	.unprepare = ingenic_rproc_unprepare,
+	.start = ingenic_rproc_start,
+	.stop = ingenic_rproc_stop,
+	.kick = ingenic_rproc_kick,
+	.da_to_va = ingenic_rproc_da_to_va,
+};
+
+static irqreturn_t vpu_interrupt(int irq, void *data)
+{
+	struct rproc *rproc = data;
+	struct vpu *vpu = rproc->priv;
+	u32 vring;
+
+	vring = readl(vpu->aux_base + REG_AUX_MSG);
+
+	/* Ack the interrupt */
+	writel(0, vpu->aux_base + REG_AUX_MSG_ACK);
+
+	return rproc_vq_interrupt(rproc, vring);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id ingenic_rproc_of_matches[] = {
+	{ .compatible = "ingenic,jz4770-vpu-rproc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
+#endif
+
+static void ingenic_rproc_free(void *rproc)
+{
+	rproc_free(rproc);
+}
+
+static void ingenic_rproc_unregister(void *rproc)
+{
+	rproc_del(rproc);
+	rproc_shutdown(rproc);
+}
+
+static int ingenic_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *mem;
+	struct rproc *rproc;
+	struct vpu *vpu;
+	unsigned int i;
+	int ret;
+
+	rproc = rproc_alloc(dev, "ingenic-vpu",
+			    &ingenic_rproc_ops, NULL, sizeof(*vpu));
+	if (!rproc)
+		return -ENOMEM;
+
+	ret = devm_add_action_or_reset(dev, ingenic_rproc_free, rproc);
+	if (ret) {
+		dev_err(dev, "Unable to add action");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, rproc);
+	vpu = rproc->priv;
+	vpu->dev = &pdev->dev;
+
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
+	vpu->aux_base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(vpu->aux_base)) {
+		dev_err(dev, "Failed to ioremap");
+		return PTR_ERR(vpu->aux_base);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
+		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   vpu_mem_map[i].name);
+
+		vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
+		if (IS_ERR(vpu->mem_info[i].base)) {
+			ret = PTR_ERR(vpu->mem_info[i].base);
+			dev_err(dev, "Failed to ioremap");
+			return ret;
+		}
+
+		vpu->mem_info[i].len = resource_size(mem);
+		vpu->mem_info[i].map = &vpu_mem_map[i];
+	}
+
+	vpu->vpu_clk = devm_clk_get(dev, "vpu");
+	if (IS_ERR(vpu->vpu_clk)) {
+		dev_err(dev, "Failed to get VPU clock");
+		return PTR_ERR(vpu->vpu_clk);
+	}
+
+	vpu->aux_clk = devm_clk_get(dev, "aux");
+	if (IS_ERR(vpu->aux_clk)) {
+		dev_err(dev, "Failed to get AUX clock");
+		return PTR_ERR(vpu->aux_clk);
+	}
+
+	vpu->irq = platform_get_irq(pdev, 0);
+	if (vpu->irq < 0) {
+		dev_err(dev, "Failed to get platform IRQ");
+		return vpu->irq;
+	}
+
+	ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, "VPU", rproc);
+	if (ret < 0) {
+		dev_err(dev, "Failed to request IRQ");
+		return ret;
+	}
+
+	disable_irq_nosync(vpu->irq);
+
+	ret = rproc_add(rproc);
+	if (ret) {
+		dev_err(dev, "Failed to register remote processor");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, ingenic_rproc_unregister, rproc);
+	if (ret) {
+		dev_err(dev, "Unable to add action");
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver ingenic_rproc_driver = {
+	.probe = ingenic_rproc_probe,
+	.driver = {
+		.name = "ingenic-vpu",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),
+	},
+};
+module_platform_driver(ingenic_rproc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
+MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control driver");
-- 
2.21.0.593.g511ec345e18

^ permalink raw reply related

* [PATCH 0/5] Rework secure-monitor driver
From: Carlo Caione @ 2019-07-29 18:39 UTC (permalink / raw)
  To: srinivas.kandagatla, khilman, narmstrong, robh+dt, tglx, jbrunet,
	linux-arm-kernel, linux-amlogic, devicetree
  Cc: Carlo Caione

The secure-monitor driver is currently in really bad shape, not my 
proudest piece of code (thanks Jerome for pointing that out ;). I tried 
to rework it a bit to make it a bit more tolerable.

I needed to change a bit the APIs and consequently adapt the only user 
we have, that is the nvmem/efuses driver. To not break bisectability I 
added one single commit to change both the drivers.

The remaining commits are cosmetic and DTS/docs fixes.

Carlo Caione (5):
  nvmem: meson-efuse: Move data to a container struct
  firmware: meson_sm: Mark chip struct as static const
  nvmem: meson-efuse: bindings: Add secure-monitor phandle
  firmware: meson_sm: Rework driver as a proper platform driver
  arm64: dts: meson: Link nvmem and secure-monitor nodes

 .../bindings/nvmem/amlogic-efuse.txt          |  6 ++
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi    |  1 +
 arch/arm64/boot/dts/amlogic/meson-g12a.dtsi   |  1 +
 drivers/firmware/meson/meson_sm.c             | 96 +++++++++++++------
 drivers/nvmem/meson-efuse.c                   | 70 +++++++++-----
 include/linux/firmware/meson/meson_sm.h       | 15 +--
 6 files changed, 128 insertions(+), 61 deletions(-)

-- 
2.20.1

^ permalink raw reply

* [PATCH 1/5] nvmem: meson-efuse: Move data to a container struct
From: Carlo Caione @ 2019-07-29 18:39 UTC (permalink / raw)
  To: srinivas.kandagatla, khilman, narmstrong, robh+dt, tglx, jbrunet,
	linux-arm-kernel, linux-amlogic, devicetree
  Cc: Carlo Caione
In-Reply-To: <20190729183941.18164-1-ccaione@baylibre.com>

No functional changes, just a cleanup commit to tidy up a bit. Move the
nvmem_* and clk structures in a container struct.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/nvmem/meson-efuse.c | 47 ++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/nvmem/meson-efuse.c b/drivers/nvmem/meson-efuse.c
index 39bd76306033..9924b98db772 100644
--- a/drivers/nvmem/meson-efuse.c
+++ b/drivers/nvmem/meson-efuse.c
@@ -14,6 +14,12 @@
 
 #include <linux/firmware/meson/meson_sm.h>
 
+struct meson_efuse {
+	struct nvmem_device *nvmem;
+	struct nvmem_config config;
+	struct clk *clk;
+};
+
 static int meson_efuse_read(void *context, unsigned int offset,
 			    void *val, size_t bytes)
 {
@@ -37,21 +43,23 @@ MODULE_DEVICE_TABLE(of, meson_efuse_match);
 static int meson_efuse_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct nvmem_device *nvmem;
-	struct nvmem_config *econfig;
-	struct clk *clk;
+	struct meson_efuse *efuse;
 	unsigned int size;
 	int ret;
 
-	clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
+	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
+	if (!efuse)
+		return -ENOMEM;
+
+	efuse->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(efuse->clk)) {
+		ret = PTR_ERR(efuse->clk);
 		if (ret != -EPROBE_DEFER)
 			dev_err(dev, "failed to get efuse gate");
 		return ret;
 	}
 
-	ret = clk_prepare_enable(clk);
+	ret = clk_prepare_enable(efuse->clk);
 	if (ret) {
 		dev_err(dev, "failed to enable gate");
 		return ret;
@@ -59,7 +67,7 @@ static int meson_efuse_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(dev,
 				       (void(*)(void *))clk_disable_unprepare,
-				       clk);
+				       efuse->clk);
 	if (ret) {
 		dev_err(dev, "failed to add disable callback");
 		return ret;
@@ -70,21 +78,18 @@ static int meson_efuse_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
-	if (!econfig)
-		return -ENOMEM;
-
-	econfig->dev = dev;
-	econfig->name = dev_name(dev);
-	econfig->stride = 1;
-	econfig->word_size = 1;
-	econfig->reg_read = meson_efuse_read;
-	econfig->reg_write = meson_efuse_write;
-	econfig->size = size;
+	efuse->config.dev = dev;
+	efuse->config.name = dev_name(dev);
+	efuse->config.stride = 1;
+	efuse->config.word_size = 1;
+	efuse->config.reg_read = meson_efuse_read;
+	efuse->config.reg_write = meson_efuse_write;
+	efuse->config.size = size;
+	efuse->config.priv = efuse;
 
-	nvmem = devm_nvmem_register(&pdev->dev, econfig);
+	efuse->nvmem = devm_nvmem_register(&pdev->dev, &efuse->config);
 
-	return PTR_ERR_OR_ZERO(nvmem);
+	return PTR_ERR_OR_ZERO(efuse->nvmem);
 }
 
 static struct platform_driver meson_efuse_driver = {
-- 
2.20.1

^ permalink raw reply related

* [PATCH 2/5] firmware: meson_sm: Mark chip struct as static const
From: Carlo Caione @ 2019-07-29 18:39 UTC (permalink / raw)
  To: srinivas.kandagatla, khilman, narmstrong, robh+dt, tglx, jbrunet,
	linux-arm-kernel, linux-amlogic, devicetree
  Cc: Carlo Caione
In-Reply-To: <20190729183941.18164-1-ccaione@baylibre.com>

No need to be a global struct.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/firmware/meson/meson_sm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index 8d908a8e0d20..772ca6726e7b 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -35,7 +35,7 @@ struct meson_sm_chip {
 	struct meson_sm_cmd cmd[];
 };
 
-struct meson_sm_chip gxbb_chip = {
+static const struct meson_sm_chip gxbb_chip = {
 	.shmem_size		= SZ_4K,
 	.cmd_shmem_in_base	= 0x82000020,
 	.cmd_shmem_out_base	= 0x82000021,
-- 
2.20.1

^ permalink raw reply related

* [PATCH 3/5] nvmem: meson-efuse: bindings: Add secure-monitor phandle
From: Carlo Caione @ 2019-07-29 18:39 UTC (permalink / raw)
  To: srinivas.kandagatla, khilman, narmstrong, robh+dt, tglx, jbrunet,
	linux-arm-kernel, linux-amlogic, devicetree
  Cc: Carlo Caione
In-Reply-To: <20190729183941.18164-1-ccaione@baylibre.com>

Add a new property to link the nvmem driver to the secure-monitor. The
nvmem driver needs to access the secure-monitor to be able to access the
fuses.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt
index 2e0723ab3384..f7b3ed74db54 100644
--- a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt
+++ b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt
@@ -4,6 +4,7 @@ Required properties:
 - compatible: should be "amlogic,meson-gxbb-efuse"
 - clocks: phandle to the efuse peripheral clock provided by the
 	  clock controller.
+- secure-monitor: phandle to the secure-monitor node
 
 = Data cells =
 Are child nodes of eFuse, bindings of which as described in
@@ -16,6 +17,7 @@ Example:
 		clocks = <&clkc CLKID_EFUSE>;
 		#address-cells = <1>;
 		#size-cells = <1>;
+		secure-monitor = <&sm>;
 
 		sn: sn@14 {
 			reg = <0x14 0x10>;
@@ -30,6 +32,10 @@ Example:
 		};
 	};
 
+	sm: secure-monitor {
+		compatible = "amlogic,meson-gxbb-sm";
+	};
+
 = Data consumers =
 Are device nodes which consume nvmem data cells.
 
-- 
2.20.1

^ permalink raw reply related

* [PATCH 4/5] firmware: meson_sm: Rework driver as a proper platform driver
From: Carlo Caione @ 2019-07-29 18:39 UTC (permalink / raw)
  To: srinivas.kandagatla, khilman, narmstrong, robh+dt, tglx, jbrunet,
	linux-arm-kernel, linux-amlogic, devicetree
  Cc: Carlo Caione
In-Reply-To: <20190729183941.18164-1-ccaione@baylibre.com>

The secure monitor driver is currently a frankenstein driver which is
registered as a platform driver but its functionality goes through a
global struct accessed by the consumer drivers using exported helper
functions.

Try to tidy up the driver moving the firmware struct into the driver
data and make the consumer drivers referencing the secure-monitor using
a new property in the DT.

Currently only the nvmem driver is using this API so we can fix it in
the same commit.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/firmware/meson/meson_sm.c       | 94 +++++++++++++++++--------
 drivers/nvmem/meson-efuse.c             | 23 +++++-
 include/linux/firmware/meson/meson_sm.h | 15 ++--
 3 files changed, 93 insertions(+), 39 deletions(-)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index 772ca6726e7b..2e36a2aa274c 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -54,8 +54,6 @@ struct meson_sm_firmware {
 	void __iomem *sm_shmem_out_base;
 };
 
-static struct meson_sm_firmware fw;
-
 static u32 meson_sm_get_cmd(const struct meson_sm_chip *chip,
 			    unsigned int cmd_index)
 {
@@ -90,6 +88,7 @@ static void __iomem *meson_sm_map_shmem(u32 cmd_shmem, unsigned int size)
 /**
  * meson_sm_call - generic SMC32 call to the secure-monitor
  *
+ * @fw:		Pointer to secure-monitor firmware
  * @cmd_index:	Index of the SMC32 function ID
  * @ret:	Returned value
  * @arg0:	SMC32 Argument 0
@@ -100,15 +99,15 @@ static void __iomem *meson_sm_map_shmem(u32 cmd_shmem, unsigned int size)
  *
  * Return:	0 on success, a negative value on error
  */
-int meson_sm_call(unsigned int cmd_index, u32 *ret, u32 arg0,
-		  u32 arg1, u32 arg2, u32 arg3, u32 arg4)
+int meson_sm_call(struct meson_sm_firmware *fw, unsigned int cmd_index,
+		  u32 *ret, u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4)
 {
 	u32 cmd, lret;
 
-	if (!fw.chip)
+	if (!fw->chip)
 		return -ENOENT;
 
-	cmd = meson_sm_get_cmd(fw.chip, cmd_index);
+	cmd = meson_sm_get_cmd(fw->chip, cmd_index);
 	if (!cmd)
 		return -EINVAL;
 
@@ -124,6 +123,7 @@ EXPORT_SYMBOL(meson_sm_call);
 /**
  * meson_sm_call_read - retrieve data from secure-monitor
  *
+ * @fw:		Pointer to secure-monitor firmware
  * @buffer:	Buffer to store the retrieved data
  * @bsize:	Size of the buffer
  * @cmd_index:	Index of the SMC32 function ID
@@ -137,22 +137,23 @@ EXPORT_SYMBOL(meson_sm_call);
  *		When 0 is returned there is no guarantee about the amount of
  *		data read and bsize bytes are copied in buffer.
  */
-int meson_sm_call_read(void *buffer, unsigned int bsize, unsigned int cmd_index,
-		       u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4)
+int meson_sm_call_read(struct meson_sm_firmware *fw, void *buffer,
+		       unsigned int bsize, unsigned int cmd_index, u32 arg0,
+		       u32 arg1, u32 arg2, u32 arg3, u32 arg4)
 {
 	u32 size;
 	int ret;
 
-	if (!fw.chip)
+	if (!fw->chip)
 		return -ENOENT;
 
-	if (!fw.chip->cmd_shmem_out_base)
+	if (!fw->chip->cmd_shmem_out_base)
 		return -EINVAL;
 
-	if (bsize > fw.chip->shmem_size)
+	if (bsize > fw->chip->shmem_size)
 		return -EINVAL;
 
-	if (meson_sm_call(cmd_index, &size, arg0, arg1, arg2, arg3, arg4) < 0)
+	if (meson_sm_call(fw, cmd_index, &size, arg0, arg1, arg2, arg3, arg4) < 0)
 		return -EINVAL;
 
 	if (size > bsize)
@@ -164,7 +165,7 @@ int meson_sm_call_read(void *buffer, unsigned int bsize, unsigned int cmd_index,
 		size = bsize;
 
 	if (buffer)
-		memcpy(buffer, fw.sm_shmem_out_base, size);
+		memcpy(buffer, fw->sm_shmem_out_base, size);
 
 	return ret;
 }
@@ -173,6 +174,7 @@ EXPORT_SYMBOL(meson_sm_call_read);
 /**
  * meson_sm_call_write - send data to secure-monitor
  *
+ * @fw:		Pointer to secure-monitor firmware
  * @buffer:	Buffer containing data to send
  * @size:	Size of the data to send
  * @cmd_index:	Index of the SMC32 function ID
@@ -184,23 +186,24 @@ EXPORT_SYMBOL(meson_sm_call_read);
  *
  * Return:	size of sent data on success, a negative value on error
  */
-int meson_sm_call_write(void *buffer, unsigned int size, unsigned int cmd_index,
-			u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4)
+int meson_sm_call_write(struct meson_sm_firmware *fw, void *buffer,
+			unsigned int size, unsigned int cmd_index, u32 arg0,
+			u32 arg1, u32 arg2, u32 arg3, u32 arg4)
 {
 	u32 written;
 
-	if (!fw.chip)
+	if (!fw->chip)
 		return -ENOENT;
 
-	if (size > fw.chip->shmem_size)
+	if (size > fw->chip->shmem_size)
 		return -EINVAL;
 
-	if (!fw.chip->cmd_shmem_in_base)
+	if (!fw->chip->cmd_shmem_in_base)
 		return -EINVAL;
 
-	memcpy(fw.sm_shmem_in_base, buffer, size);
+	memcpy(fw->sm_shmem_in_base, buffer, size);
 
-	if (meson_sm_call(cmd_index, &written, arg0, arg1, arg2, arg3, arg4) < 0)
+	if (meson_sm_call(fw, cmd_index, &written, arg0, arg1, arg2, arg3, arg4) < 0)
 		return -EINVAL;
 
 	if (!written)
@@ -210,6 +213,24 @@ int meson_sm_call_write(void *buffer, unsigned int size, unsigned int cmd_index,
 }
 EXPORT_SYMBOL(meson_sm_call_write);
 
+/**
+ * meson_sm_get - get pointer to meson_sm_firmware structure.
+ *
+ * @sm_node:		Pointer to the secure-monitor Device Tree node.
+ *
+ * Return:		NULL is the secure-monitor device is not ready.
+ */
+struct meson_sm_firmware *meson_sm_get(struct device_node *sm_node)
+{
+	struct platform_device *pdev = of_find_device_by_node(sm_node);
+
+	if (!pdev)
+		return NULL;
+
+	return platform_get_drvdata(pdev);
+}
+EXPORT_SYMBOL_GPL(meson_sm_get);
+
 #define SM_CHIP_ID_LENGTH	119
 #define SM_CHIP_ID_OFFSET	4
 #define SM_CHIP_ID_SIZE		12
@@ -217,14 +238,18 @@ EXPORT_SYMBOL(meson_sm_call_write);
 static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
+	struct platform_device *pdev = to_platform_device(dev);
+	struct meson_sm_firmware *fw;
 	uint8_t *id_buf;
 	int ret;
 
+	fw = platform_get_drvdata(pdev);
+
 	id_buf = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
 	if (!id_buf)
 		return -ENOMEM;
 
-	ret = meson_sm_call_read(id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
+	ret = meson_sm_call_read(fw, id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
 				 0, 0, 0, 0, 0);
 	if (ret < 0) {
 		kfree(id_buf);
@@ -268,25 +293,34 @@ static const struct of_device_id meson_sm_ids[] = {
 
 static int __init meson_sm_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	const struct meson_sm_chip *chip;
+	struct meson_sm_firmware *fw;
+
+	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
+	if (!fw)
+		return -ENOMEM;
 
-	chip = of_match_device(meson_sm_ids, &pdev->dev)->data;
+	chip = of_match_device(meson_sm_ids, dev)->data;
 
 	if (chip->cmd_shmem_in_base) {
-		fw.sm_shmem_in_base = meson_sm_map_shmem(chip->cmd_shmem_in_base,
-							 chip->shmem_size);
-		if (WARN_ON(!fw.sm_shmem_in_base))
+		fw->sm_shmem_in_base = meson_sm_map_shmem(chip->cmd_shmem_in_base,
+							  chip->shmem_size);
+		if (WARN_ON(!fw->sm_shmem_in_base))
 			goto out;
 	}
 
 	if (chip->cmd_shmem_out_base) {
-		fw.sm_shmem_out_base = meson_sm_map_shmem(chip->cmd_shmem_out_base,
-							  chip->shmem_size);
-		if (WARN_ON(!fw.sm_shmem_out_base))
+		fw->sm_shmem_out_base = meson_sm_map_shmem(chip->cmd_shmem_out_base,
+							   chip->shmem_size);
+		if (WARN_ON(!fw->sm_shmem_out_base))
 			goto out_in_base;
 	}
 
-	fw.chip = chip;
+	fw->chip = chip;
+
+	platform_set_drvdata(pdev, fw);
+
 	pr_info("secure-monitor enabled\n");
 
 	if (sysfs_create_group(&pdev->dev.kobj, &meson_sm_sysfs_attr_group))
@@ -295,7 +329,7 @@ static int __init meson_sm_probe(struct platform_device *pdev)
 	return 0;
 
 out_in_base:
-	iounmap(fw.sm_shmem_in_base);
+	iounmap(fw->sm_shmem_in_base);
 out:
 	return -EINVAL;
 }
diff --git a/drivers/nvmem/meson-efuse.c b/drivers/nvmem/meson-efuse.c
index 9924b98db772..669d20d73877 100644
--- a/drivers/nvmem/meson-efuse.c
+++ b/drivers/nvmem/meson-efuse.c
@@ -17,20 +17,25 @@
 struct meson_efuse {
 	struct nvmem_device *nvmem;
 	struct nvmem_config config;
+	struct meson_sm_firmware *fw;
 	struct clk *clk;
 };
 
 static int meson_efuse_read(void *context, unsigned int offset,
 			    void *val, size_t bytes)
 {
-	return meson_sm_call_read((u8 *)val, bytes, SM_EFUSE_READ, offset,
+	struct meson_efuse *efuse = context;
+
+	return meson_sm_call_read(efuse->fw, (u8 *)val, bytes, SM_EFUSE_READ, offset,
 				  bytes, 0, 0, 0);
 }
 
 static int meson_efuse_write(void *context, unsigned int offset,
 			     void *val, size_t bytes)
 {
-	return meson_sm_call_write((u8 *)val, bytes, SM_EFUSE_WRITE, offset,
+	struct meson_efuse *efuse = context;
+
+	return meson_sm_call_write(efuse->fw, (u8 *)val, bytes, SM_EFUSE_WRITE, offset,
 				   bytes, 0, 0, 0);
 }
 
@@ -43,6 +48,7 @@ MODULE_DEVICE_TABLE(of, meson_efuse_match);
 static int meson_efuse_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *sm_np;
 	struct meson_efuse *efuse;
 	unsigned int size;
 	int ret;
@@ -51,6 +57,17 @@ static int meson_efuse_probe(struct platform_device *pdev)
 	if (!efuse)
 		return -ENOMEM;
 
+	sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0);
+	if (!sm_np) {
+		dev_err(&pdev->dev, "no secure-monitor node\n");
+		return -ENODEV;
+	}
+
+	efuse->fw = meson_sm_get(sm_np);
+	of_node_put(sm_np);
+	if (!efuse->fw)
+		return -EPROBE_DEFER;
+
 	efuse->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(efuse->clk)) {
 		ret = PTR_ERR(efuse->clk);
@@ -73,7 +90,7 @@ static int meson_efuse_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	if (meson_sm_call(SM_EFUSE_USER_MAX, &size, 0, 0, 0, 0, 0) < 0) {
+	if (meson_sm_call(efuse->fw, SM_EFUSE_USER_MAX, &size, 0, 0, 0, 0, 0) < 0) {
 		dev_err(dev, "failed to get max user");
 		return -EINVAL;
 	}
diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
index 7613bf7c9442..6669e2a1d5fd 100644
--- a/include/linux/firmware/meson/meson_sm.h
+++ b/include/linux/firmware/meson/meson_sm.h
@@ -16,11 +16,14 @@ enum {
 
 struct meson_sm_firmware;
 
-int meson_sm_call(unsigned int cmd_index, u32 *ret, u32 arg0, u32 arg1,
-		  u32 arg2, u32 arg3, u32 arg4);
-int meson_sm_call_write(void *buffer, unsigned int b_size, unsigned int cmd_index,
-			u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4);
-int meson_sm_call_read(void *buffer, unsigned int bsize, unsigned int cmd_index,
-		       u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4);
+int meson_sm_call(struct meson_sm_firmware *fw, unsigned int cmd_index,
+		  u32 *ret, u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4);
+int meson_sm_call_write(struct meson_sm_firmware *fw, void *buffer,
+			unsigned int b_size, unsigned int cmd_index, u32 arg0,
+			u32 arg1, u32 arg2, u32 arg3, u32 arg4);
+int meson_sm_call_read(struct meson_sm_firmware *fw, void *buffer,
+		       unsigned int bsize, unsigned int cmd_index, u32 arg0,
+		       u32 arg1, u32 arg2, u32 arg3, u32 arg4);
+struct meson_sm_firmware *meson_sm_get(struct device_node *firmware_node);
 
 #endif /* _MESON_SM_FW_H_ */
-- 
2.20.1

^ permalink raw reply related

* [PATCH 5/5] arm64: dts: meson: Link nvmem and secure-monitor nodes
From: Carlo Caione @ 2019-07-29 18:39 UTC (permalink / raw)
  To: srinivas.kandagatla, khilman, narmstrong, robh+dt, tglx, jbrunet,
	linux-arm-kernel, linux-amlogic, devicetree
  Cc: Carlo Caione
In-Reply-To: <20190729183941.18164-1-ccaione@baylibre.com>

The former is going to use the latter to retrieve the efuses data.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi  | 1 +
 arch/arm64/boot/dts/amlogic/meson-g12a.dtsi | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
index 6219337033a0..b8244efb85fa 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -117,6 +117,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		read-only;
+		secure-monitor = <&sm>;
 	};
 
 	psci {
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
index f8d43e3dcf20..2b07752e034f 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
@@ -100,6 +100,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		read-only;
+		secure-monitor = <&sm>;
 	};
 
 	psci {
-- 
2.20.1

^ permalink raw reply related

* Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
From: Uwe Kleine-König @ 2019-07-29 18:40 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai, Mark Rutland,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA, devicetree, linux-kernel,
	Maxime Ripard, Rob Herring, Thierry Reding,
	kernel-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel
In-Reply-To: <2346193.MplWYqIveT@jernej-laptop>

On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König 
> napisal(a):
> > Hello,
> > 
> > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > 
> > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > > 
> > > > > napisal(a):
> > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > 
> > > > > > >   .npwm = 1,
> > > > > > >  
> > > > > > >  };
> > > > > > > 
> > > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst
> > > > > > > = {
> > > > > > > + .has_bus_clock = true,
> > > > > > > + .has_prescaler_bypass = true,
> > > > > > > + .has_reset = true,
> > > > > > > + .npwm = 2,
> > > > > > > +};
> > > > > > > +
> > > > > > > 
> > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > >  
> > > > > > >   {
> > > > > > >   
> > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > 
> > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > {
> > > > > > > 
> > > > > > >   }, {
> > > > > > >   
> > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > 
> > > > > > > + }, {
> > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > 
> > > > > > If you follow my suggestion for the two previous patches, you can
> > > > > > just
> > > > > > 
> > > > > > use:
> > > > > >     compatible = "allwinner,sun50i-h6-pwm",
> > > > > >     "allwinner,sun5i-a10s-pwm";
> > > > > > 
> > > > > > and drop this patch.
> > > > > 
> > > > > Maxime found out that it's not compatible with A10s due to difference
> > > > > in bypass bit, but yes, I know what you mean.
> > > > > 
> > > > > Since H6 requires reset line and bus clock to be specified, it's not
> > > > > compatible from DT binding side. New yaml based binding must somehow
> > > > > know that in order to be able to validate DT node, so it needs
> > > > > standalone compatible. However, depending on conclusions of other
> > > > > discussions, this new compatible can be associated with already
> > > > > available quirks structure or have it's own.> > 
> > > > I cannot follow. You should be able to specify in the binding that the
> > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > without a reset line and bus clock also verifies, but this doesn't
> > > > really hurt (and who knows, maybe the next allwinner chip needs exactly
> > > > this).
> > > 
> > > It is not optional. It will not work if either the clocks or reset
> > > controls
> > > are missing. How would these be optional anyway? Either it's connected and
> > > thus required, or it's not and therefore should be omitted from the
> > > description.
> > 
> > [Just arguing about the clock here, the argumentation is analogous for
> > the reset control.]
> > 
> > From the driver's perspective it's optional: There are devices with and
> > without a bus clock. This doesn't mean that you can just ignore this
> > clock if it's specified. It's optional in the sense "If dt doesn't
> > specify it, then assume this is a device that doesn't have it and so you
> > don't need to handle it." but not in the sense "it doesn't matter if
> > you handle it or not.".
> > 
> > Other than that I'm on your side. So for example I think it's not
> > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > because this hides exactly the kind of problem you point out here.
> >
> 
> I think there's misunderstanding. I only argued that we can't use
> 
> compatible = "allwinner,sun50i-h6-pwm",
> 	 "allwinner,sun5i-a10s-pwm";
> 
> as you suggested and only 
> 
> compatible = "allwinner,sun50i-h6-pwm"; 
> 
> will work. Not because of driver itself (it can still use _optional() 
> variants), but because of DT binding, which should be able to validate H6 PWM 
> node - reset and bus clock references are required in this case.

I think I understood. In my eyes there is no need to let validation of
the DT bindings catch a missing "optional" property that is needed on
H6.

You have to draw the line somewhere which information the driver has
hard-coded and what is only provided by the device tree and just assumed
to be correct by the driver. You argue the driver should know that if it
cares for a "allwinner,sun50i-h6-pwm" device it should know (and check)
that there is a clock named "bus" and a resets property that links to a
reset controller. How is that different from checking that the base
address is 0x0300a000 or that the "pwm" clock is the osc24M clock
running at 24 MHz? This isn't checked in the driver or the dt schema.
Still if the device tree got one of them wrong this yields an
non-working pwm device that isn't catched in the driver.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729184041.vlvfz3vz3ykhufdk%40pengutronix.de.

^ permalink raw reply

* Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
From: Jernej Škrabec @ 2019-07-29 18:46 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai, Mark Rutland,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA, devicetree, linux-kernel,
	Maxime Ripard, Rob Herring, Thierry Reding,
	kernel-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel
In-Reply-To: <20190729184041.vlvfz3vz3ykhufdk-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König 
napisal(a):
> On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > 
> > napisal(a):
> > > Hello,
> > > 
> > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > 
> > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > Kleine-König
> > > > > > 
> > > > > > napisal(a):
> > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > 
> > > > > > > >   .npwm = 1,
> > > > > > > >  
> > > > > > > >  };
> > > > > > > > 
> > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > = {
> > > > > > > > + .has_bus_clock = true,
> > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > + .has_reset = true,
> > > > > > > > + .npwm = 2,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > 
> > > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > >  
> > > > > > > >   {
> > > > > > > >   
> > > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > 
> > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > {
> > > > > > > > 
> > > > > > > >   }, {
> > > > > > > >   
> > > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > > 
> > > > > > > > + }, {
> > > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > 
> > > > > > > If you follow my suggestion for the two previous patches, you
> > > > > > > can
> > > > > > > just
> > > > > > > 
> > > > > > > use:
> > > > > > >     compatible = "allwinner,sun50i-h6-pwm",
> > > > > > >     "allwinner,sun5i-a10s-pwm";
> > > > > > > 
> > > > > > > and drop this patch.
> > > > > > 
> > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > difference
> > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > 
> > > > > > Since H6 requires reset line and bus clock to be specified, it's
> > > > > > not
> > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > somehow
> > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > standalone compatible. However, depending on conclusions of other
> > > > > > discussions, this new compatible can be associated with already
> > > > > > available quirks structure or have it's own.> >
> > > > > 
> > > > > I cannot follow. You should be able to specify in the binding that
> > > > > the
> > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > > without a reset line and bus clock also verifies, but this doesn't
> > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > exactly
> > > > > this).
> > > > 
> > > > It is not optional. It will not work if either the clocks or reset
> > > > controls
> > > > are missing. How would these be optional anyway? Either it's connected
> > > > and
> > > > thus required, or it's not and therefore should be omitted from the
> > > > description.
> > > 
> > > [Just arguing about the clock here, the argumentation is analogous for
> > > the reset control.]
> > > 
> > > From the driver's perspective it's optional: There are devices with and
> > > without a bus clock. This doesn't mean that you can just ignore this
> > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > specify it, then assume this is a device that doesn't have it and so you
> > > don't need to handle it." but not in the sense "it doesn't matter if
> > > you handle it or not.".
> > > 
> > > Other than that I'm on your side. So for example I think it's not
> > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > because this hides exactly the kind of problem you point out here.
> > 
> > I think there's misunderstanding. I only argued that we can't use
> > 
> > compatible = "allwinner,sun50i-h6-pwm",
> > 
> > 	 "allwinner,sun5i-a10s-pwm";
> > 
> > as you suggested and only
> > 
> > compatible = "allwinner,sun50i-h6-pwm";
> > 
> > will work. Not because of driver itself (it can still use _optional()
> > variants), but because of DT binding, which should be able to validate H6
> > PWM node - reset and bus clock references are required in this case.
> 
> I think I understood. In my eyes there is no need to let validation of
> the DT bindings catch a missing "optional" property that is needed on
> H6.
> 
> You have to draw the line somewhere which information the driver has
> hard-coded and what is only provided by the device tree and just assumed
> to be correct by the driver. You argue the driver should know that 

No, in this thread I argue that DT validation tool, executed by

make ARCH=arm64 dtbs_check

should catch that. This is not a driver, but DT binding described in YAML.

Best regards,
Jernej

> if it
> cares for a "allwinner,sun50i-h6-pwm" device it should know (and check)
> that there is a clock named "bus" and a resets property that links to a
> reset controller. How is that different from checking that the base
> address is 0x0300a000 or that the "pwm" clock is the osc24M clock
> running at 24 MHz? This isn't checked in the driver or the dt schema.
> Still if the device tree got one of them wrong this yields an
> non-working pwm device that isn't catched in the driver.
> 
> Best regards
> Uwe




-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/173825848.1FZsmuHfpq%40jernej-laptop.

^ permalink raw reply

* Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM
From: Uwe Kleine-König @ 2019-07-29 18:51 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai, Mark Rutland,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA, devicetree, linux-kernel,
	Maxime Ripard, Rob Herring, Thierry Reding,
	kernel-bIcnvbaLZ9MEGnE8C9+IrQ, linux-arm-kernel
In-Reply-To: <173825848.1FZsmuHfpq@jernej-laptop>

On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König 
> napisal(a):
> > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > > 
> > > napisal(a):
> > > > Hello,
> > > > 
> > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > 
> > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > > Kleine-König
> > > > > > > 
> > > > > > > napisal(a):
> > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > > 
> > > > > > > > >   .npwm = 1,
> > > > > > > > >  
> > > > > > > > >  };
> > > > > > > > > 
> > > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > > = {
> > > > > > > > > + .has_bus_clock = true,
> > > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > > + .has_reset = true,
> > > > > > > > > + .npwm = 2,
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > 
> > > > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > > >  
> > > > > > > > >   {
> > > > > > > > >   
> > > > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > > 
> > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > > {
> > > > > > > > > 
> > > > > > > > >   }, {
> > > > > > > > >   
> > > > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > > > 
> > > > > > > > > + }, {
> > > > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > > 
> > > > > > > > If you follow my suggestion for the two previous patches, you
> > > > > > > > can
> > > > > > > > just
> > > > > > > > 
> > > > > > > > use:
> > > > > > > >     compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > >     "allwinner,sun5i-a10s-pwm";
> > > > > > > > 
> > > > > > > > and drop this patch.
> > > > > > > 
> > > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > > difference
> > > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > > 
> > > > > > > Since H6 requires reset line and bus clock to be specified, it's
> > > > > > > not
> > > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > > somehow
> > > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > > standalone compatible. However, depending on conclusions of other
> > > > > > > discussions, this new compatible can be associated with already
> > > > > > > available quirks structure or have it's own.> >
> > > > > > 
> > > > > > I cannot follow. You should be able to specify in the binding that
> > > > > > the
> > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > > > without a reset line and bus clock also verifies, but this doesn't
> > > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > > exactly
> > > > > > this).
> > > > > 
> > > > > It is not optional. It will not work if either the clocks or reset
> > > > > controls
> > > > > are missing. How would these be optional anyway? Either it's connected
> > > > > and
> > > > > thus required, or it's not and therefore should be omitted from the
> > > > > description.
> > > > 
> > > > [Just arguing about the clock here, the argumentation is analogous for
> > > > the reset control.]
> > > > 
> > > > From the driver's perspective it's optional: There are devices with and
> > > > without a bus clock. This doesn't mean that you can just ignore this
> > > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > > specify it, then assume this is a device that doesn't have it and so you
> > > > don't need to handle it." but not in the sense "it doesn't matter if
> > > > you handle it or not.".
> > > > 
> > > > Other than that I'm on your side. So for example I think it's not
> > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > > because this hides exactly the kind of problem you point out here.
> > > 
> > > I think there's misunderstanding. I only argued that we can't use
> > > 
> > > compatible = "allwinner,sun50i-h6-pwm",
> > > 
> > > 	 "allwinner,sun5i-a10s-pwm";
> > > 
> > > as you suggested and only
> > > 
> > > compatible = "allwinner,sun50i-h6-pwm";
> > > 
> > > will work. Not because of driver itself (it can still use _optional()
> > > variants), but because of DT binding, which should be able to validate H6
> > > PWM node - reset and bus clock references are required in this case.
> > 
> > I think I understood. In my eyes there is no need to let validation of
> > the DT bindings catch a missing "optional" property that is needed on
> > H6.
> > 
> > You have to draw the line somewhere which information the driver has
> > hard-coded and what is only provided by the device tree and just assumed
> > to be correct by the driver. You argue the driver should know that 
> 
> No, in this thread I argue that DT validation tool, executed by
> 
> make ARCH=arm64 dtbs_check
> 
> should catch that. This is not a driver, but DT binding described in YAML.

The argumentation is the same. dtbs_check doesn't notice if the base
address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should
it catch a missing reset controller phandle?

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729185108.tpilwoooxvi2z72e%40pengutronix.de.

^ permalink raw reply

* Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock
From: Uwe Kleine-König @ 2019-07-29 19:04 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Jernej Škrabec, mark.rutland-5wv7dgnIgG8,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, wens-jdAy2FN1RRM,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20190729164516.yxfgj2zd3d5ii4c4-YififvaboMKzQB+pC5nmwQ@public.gmane.org>

Hello Maxime,

On Mon, Jul 29, 2019 at 06:45:16PM +0200, Maxime Ripard wrote:
> On Mon, Jul 29, 2019 at 06:14:35PM +0200, Uwe Kleine-König wrote:
> > Then maybe something like the following?:
> >
> > 	busclk = devm_clk_get_optional(..., "bus");
> > 	modclk = devm_clk_get_optional(..., "mod");
> >
> > 	/*
> > 	 * old dtbs might have a single clock but no clock names. Fall
> > 	 * back to this for compatibility reasons.
> > 	 */
> > 	if (!modclk) {
> > 		modclk = devm_clk_get(..., NULL);
> > 	}
> 
> Again, there's nothing optional about these clocks. You need a
> particular set of clocks for a given generation, and a separate set of
> them on another generation of SoCs.

It depends on the way how "optional" is understood. The semantic of
"optional" as it is used and implemented by devm_clk_get_optional (and
gpiod_get_optional and devm_reset_control_get_optional) is different
than yours when saying "on H6 the clock is not optional". If it was
about the "it doesn't matter if it's taken care of or not" semantic you
seem to mean the function would be useless and no driver would need to
actually use it. In the sense of the functions listed above "optional"
means: Some devices need it, others don't. Using this semantic the "bus"
clock is optional.

> It really isn't about DT validation. We're really making sure that the
> device can be operational. It's as much of a validation step than
> making sure we have mapped registers (reg), or an interrupt if we had
> any.

Do you agree with Jernej in the other end of this thread? If so I don't
think that repeating the same arguments here is sensible. Please read
what I wrote there.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729190404.rnxw2aihmciafy6c%40pengutronix.de.

^ permalink raw reply

* Re: [PATCH v2 1/7] ASoC: fsl_sai: Add registers definition for multiple datalines
From: Nicolin Chen @ 2019-07-29 19:42 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: devicetree, alsa-devel, kernel, timur, robh, shengjiu.wang, angus,
	tiwai, linux-kernel, broonie, linux-imx, viorel.suman, festevam,
	mihai.serban, l.stach
In-Reply-To: <20190728192429.1514-2-daniel.baluta@nxp.com>

On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:
> SAI IP supports up to 8 data lines. The configuration of
> supported number of data lines is decided at SoC integration
> time.
> 
> This patch adds definitions for all related data TX/RX registers:
> 	* TDR0..7, Transmit data register
> 	* TFR0..7, Transmit FIFO register
> 	* RDR0..7, Receive data register
> 	* RFR0..7, Receive FIFO register
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  sound/soc/fsl/fsl_sai.c | 76 +++++++++++++++++++++++++++++++++++------
>  sound/soc/fsl/fsl_sai.h | 36 ++++++++++++++++---
>  2 files changed, 98 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 6d3c6c8d50ce..17b0aff4ee8b 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c

> @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
>  	case FSL_SAI_TCR3:
>  	case FSL_SAI_TCR4:
>  	case FSL_SAI_TCR5:
> -	case FSL_SAI_TFR:
> +	case FSL_SAI_TFR0:
> +	case FSL_SAI_TFR1:
> +	case FSL_SAI_TFR2:
> +	case FSL_SAI_TFR3:
> +	case FSL_SAI_TFR4:
> +	case FSL_SAI_TFR5:
> +	case FSL_SAI_TFR6:
> +	case FSL_SAI_TFR7:
>  	case FSL_SAI_TMR:
>  	case FSL_SAI_RCSR:
>  	case FSL_SAI_RCR1:

A tricky thing here is that those SAI instances on older SoC don't
support multi data lines physically, while seemly having registers
pre-defined. So your change doesn't sound doing anything wrong to
them at all, I am still wondering if it is necessary to apply them
to newer compatible only though, as for older compatibles of SAI,
these registers would be useless and confusing if being exposed.

What do you think?

^ permalink raw reply

* Re: [PATCH v2] dt-bindings: rcar-{csi2,vin}: Rename bindings documentation files
From: Niklas Söderlund @ 2019-07-29 19:55 UTC (permalink / raw)
  To: Simon Horman
  Cc: Rob Herring, devicetree, linux-media, linux-renesas-soc,
	Geert Uytterhoeven, Ulrich Hecht
In-Reply-To: <20190729074355.urlf77ihukgnk6ov@verge.net.au>

Hi,

On 2019-07-29 09:43:55 +0200, Simon Horman wrote:
> On Fri, Jul 12, 2019 at 04:17:03PM +0900, Niklas S�derlund wrote:
> > Hi,
> > 
> > A gentle ping on this patch. I assume if it could get a ack from Rob it 
> > could be taken in thru the media-tree?
> 
> Given the lack of interest I think it could also be taken through
> Geert's tree.

I would be happy if that could happen thru Geert's tree.

> 
> > 
> > On 2019-06-12 23:12:41 +0200, Niklas S�derlund wrote:
> > > Renesas media binding documentation files uses a naming schema of
> > > 'renesas,<module>.txt'. Rename VIN and CSI-2 files to match this
> > > pattern.
> > > 
> > > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > > ---
> > >  .../media/{renesas,rcar-csi2.txt => renesas,csi2.txt}         | 0
> > >  .../bindings/media/{rcar_vin.txt => renesas,vin.txt}          | 0
> > >  MAINTAINERS                                                   | 4 ++--
> > >  3 files changed, 2 insertions(+), 2 deletions(-)
> > >  rename Documentation/devicetree/bindings/media/{renesas,rcar-csi2.txt => renesas,csi2.txt} (100%)
> > >  rename Documentation/devicetree/bindings/media/{rcar_vin.txt => renesas,vin.txt} (100%)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt b/Documentation/devicetree/bindings/media/renesas,csi2.txt
> > > similarity index 100%
> > > rename from Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
> > > rename to Documentation/devicetree/bindings/media/renesas,csi2.txt
> > > diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/renesas,vin.txt
> > > similarity index 100%
> > > rename from Documentation/devicetree/bindings/media/rcar_vin.txt
> > > rename to Documentation/devicetree/bindings/media/renesas,vin.txt
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 6a3bac28ebb47830..a0d21ff13c8e8989 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -9807,8 +9807,8 @@ L:	linux-media@vger.kernel.org
> > >  L:	linux-renesas-soc@vger.kernel.org
> > >  T:	git git://linuxtv.org/media_tree.git
> > >  S:	Supported
> > > -F:	Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
> > > -F:	Documentation/devicetree/bindings/media/rcar_vin.txt
> > > +F:	Documentation/devicetree/bindings/media/renesas,csi2.txt
> > > +F:	Documentation/devicetree/bindings/media/renesas,vin.txt
> > >  F:	drivers/media/platform/rcar-vin/
> > >  
> > >  MEDIA DRIVERS FOR RENESAS - VSP1
> > > -- 
> > > 2.21.0
> > > 
> > 
> > -- 
> > Regards,
> > Niklas S�derlund
> > 

-- 
Regards,
Niklas S�derlund

^ permalink raw reply

* Re: [PATCH] dt-bindings: rcar-imr: Rename bindings documentation file
From: Niklas Söderlund @ 2019-07-29 19:57 UTC (permalink / raw)
  To: Simon Horman
  Cc: Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Magnus Damm,
	Geert Uytterhoeven, linux-media, devicetree, linux-renesas-soc
In-Reply-To: <20190729074757.9581-1-horms+renesas@verge.net.au>

Hi Simon,

Thanks for your work.

On 2019-07-29 09:47:57 +0200, Simon Horman wrote:
> Renesas media binding documentation files uses a naming schema of
> 'renesas,<module>.txt'. Rename IMR file to match this pattern.
> 
> Cc: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

Reviewed-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  Documentation/devicetree/bindings/media/{rcar_imr.txt => renesas,imr.txt} | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename Documentation/devicetree/bindings/media/{rcar_imr.txt => renesas,imr.txt} (100%)
> 
> diff --git a/Documentation/devicetree/bindings/media/rcar_imr.txt b/Documentation/devicetree/bindings/media/renesas,imr.txt
> similarity index 100%
> rename from Documentation/devicetree/bindings/media/rcar_imr.txt
> rename to Documentation/devicetree/bindings/media/renesas,imr.txt
> -- 
> 2.11.0
> 

-- 
Regards,
Niklas S�derlund

^ permalink raw reply

* Re: [alsa-devel] [PATCH v2 1/7] ASoC: fsl_sai: Add registers definition for multiple datalines
From: Daniel Baluta @ 2019-07-29 19:57 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: Daniel Baluta, Devicetree List, Linux-ALSA,
	Pengutronix Kernel Team, Timur Tabi, Rob Herring, S.j. Wang,
	Angus Ainslie (Purism), Takashi Iwai, Linux Kernel Mailing List,
	Mark Brown, dl-linux-imx, Viorel Suman, Fabio Estevam,
	Mihai Serban, Lucas Stach
In-Reply-To: <20190729194214.GA20594@Asurada-Nvidia.nvidia.com>

On Mon, Jul 29, 2019 at 10:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Sun, Jul 28, 2019 at 10:24:23PM +0300, Daniel Baluta wrote:
> > SAI IP supports up to 8 data lines. The configuration of
> > supported number of data lines is decided at SoC integration
> > time.
> >
> > This patch adds definitions for all related data TX/RX registers:
> >       * TDR0..7, Transmit data register
> >       * TFR0..7, Transmit FIFO register
> >       * RDR0..7, Receive data register
> >       * RFR0..7, Receive FIFO register
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  sound/soc/fsl/fsl_sai.c | 76 +++++++++++++++++++++++++++++++++++------
> >  sound/soc/fsl/fsl_sai.h | 36 ++++++++++++++++---
> >  2 files changed, 98 insertions(+), 14 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > index 6d3c6c8d50ce..17b0aff4ee8b 100644
> > --- a/sound/soc/fsl/fsl_sai.c
> > +++ b/sound/soc/fsl/fsl_sai.c
>
> > @@ -704,7 +711,14 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg)
> >       case FSL_SAI_TCR3:
> >       case FSL_SAI_TCR4:
> >       case FSL_SAI_TCR5:
> > -     case FSL_SAI_TFR:
> > +     case FSL_SAI_TFR0:
> > +     case FSL_SAI_TFR1:
> > +     case FSL_SAI_TFR2:
> > +     case FSL_SAI_TFR3:
> > +     case FSL_SAI_TFR4:
> > +     case FSL_SAI_TFR5:
> > +     case FSL_SAI_TFR6:
> > +     case FSL_SAI_TFR7:
> >       case FSL_SAI_TMR:
> >       case FSL_SAI_RCSR:
> >       case FSL_SAI_RCR1:
>
> A tricky thing here is that those SAI instances on older SoC don't
> support multi data lines physically, while seemly having registers
> pre-defined. So your change doesn't sound doing anything wrong to
> them at all, I am still wondering if it is necessary to apply them
> to newer compatible only though, as for older compatibles of SAI,
> these registers would be useless and confusing if being exposed.
>
> What do you think?

Yes, I thought about this too. But, I tried to keep the code as short
as possible and technically it is not wrong. When 1 data line is supported
for example application will only care about TDR0, TFR0, etc.

^ permalink raw reply

* Re: [PATCH v3 0/6] Introduce Bandwidth OPPs for interconnect paths
From: Saravana Kannan @ 2019-07-29 20:12 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
	Sweeney, Sean, David Dai, Rajendra Nayak, Sibi Sankar,
	Bjorn Andersson, Evan Green, Android Kernel Team, Linux PM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <20190729092454.6lfqzmhkvrhpimsp@vireshk-i7>

On Mon, Jul 29, 2019 at 2:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-07-19, 21:12, Saravana Kannan wrote:
> > On Wed, Jul 17, 2019 at 10:37 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > I would like
> > > to put this data in the GPU OPP table only. What about putting a
> > > range in the GPU OPP table for the Bandwidth if it can change so much
> > > for the same frequency.
> >
> > I don't think the range is going to work.
>
> Any specific reason for that ?

The next sentence was literally explaining this :) Fine to debate
that, but ignoring that and asking this question is kinda funny.

> > If a GPU is doing purely
> > computational work, it's not unreasonable for it to vote for the
> > lowest bandwidth for any GPU frequency.
>
> I think that is fine, but if the GPU is able to find how much
> bandwidth it needs why can't it just pass that value without needing
> to have another OPP table for the path ?

You were asking this question in the context of "can the GPU OPP just
list all the range of bandwidth it might use per GPU frequency". My point
is that the range would be useless because it would the entire
available bandwidth range (because purely compute work might not need
any bandwidth).

Whereas, what the GPU's algorithm actually needs might be the list of
"useful" bandwidth levels to use.

Also, as we add more ICC request properties, this range idea will not scale.

-Saravana

^ permalink raw reply

* Re: [PATCH v2 4/7] ASoC: dt-bindings: Document dl-mask property
From: Nicolin Chen @ 2019-07-29 20:15 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: broonie, l.stach, mihai.serban, alsa-devel, viorel.suman, timur,
	shengjiu.wang, angus, tiwai, linux-imx, kernel, festevam,
	linux-kernel, devicetree, robh
In-Reply-To: <20190728192429.1514-5-daniel.baluta@nxp.com>

On Sun, Jul 28, 2019 at 10:24:26PM +0300, Daniel Baluta wrote:
> SAI supports up to 8 data lines. This property let the user
> configure how many data lines should be used per transfer
> direction (Tx/Rx).

This sounds a bit less persuasive to me as we are adding a
DT property that's used to describe a hardware connections
and it would be probably better to mention that the mapping
between the mask and the data lines could be more flexible
than consecutive active data lines as you said previously.

> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  Documentation/devicetree/bindings/sound/fsl-sai.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> index 2e726b983845..2b38036a4883 100644
> --- a/Documentation/devicetree/bindings/sound/fsl-sai.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
> @@ -49,6 +49,13 @@ Optional properties:
>  
>    - big-endian		: Boolean property, required if all the SAI
>  			  registers are big-endian rather than little-endian.
> +  - fsl,dl-mask		: list of two integers (bitmask, first for RX, second

I am leaving this naming to DT maintainer.

> +			  for TX) representing enabled datalines. Bit 0
> +			  represents first data line, bit 1 represents second
> +			  data line and so on. Data line is enabled if
> +			  corresponding bit is set to 1. By default, if property
> +			  not present, only dataline 0 is enabled for both
> +			  directions.

To make this patch more convincing, could we add an example
as well in the Example section of this binding file? Like:
	/* RX data lines 0/1 and TX data lines 0/2 are connected */
	fsl,dl-mask = <0x3 0x5>;

^ permalink raw reply

* Re: [PATCH v4 0/3] Introduce Bandwidth OPPs for interconnects
From: Saravana Kannan @ 2019-07-29 20:16 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rob Herring, Mark Rutland, Viresh Kumar, Nishanth Menon,
	Stephen Boyd, Rafael J. Wysocki, Georgi Djakov, Vincent Guittot,
	Sweeney, Sean, David Dai, adharmap, Rajendra Nayak, Sibi Sankar,
	Bjorn Andersson, Evan Green, Android Kernel Team, Linux PM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <20190729093545.kvnqxjkyx4nogddk@vireshk-i7>

On Mon, Jul 29, 2019 at 2:35 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 26-07-19, 16:15, Saravana Kannan wrote:
> > Interconnects and interconnect paths quantify their performance levels in
> > terms of bandwidth and not in terms of frequency. So similar to how we have
> > frequency based OPP tables in DT and in the OPP framework, we need
> > bandwidth OPP table support in DT and in the OPP framework.
> >
> > So with the DT bindings added in this patch series, the DT for a GPU
> > that does bandwidth voting from GPU to Cache and GPU to DDR would look
> > something like this:
> >
> > gpu_cache_opp_table: gpu_cache_opp_table {
> >       compatible = "operating-points-v2";
> >
> >       gpu_cache_3000: opp-3000 {
> >               opp-peak-KBps = <3000000>;
> >               opp-avg-KBps = <1000000>;
> >       };
> >       gpu_cache_6000: opp-6000 {
> >               opp-peak-KBps = <6000000>;
> >               opp-avg-KBps = <2000000>;
> >       };
> >       gpu_cache_9000: opp-9000 {
> >               opp-peak-KBps = <9000000>;
> >               opp-avg-KBps = <9000000>;
> >       };
> > };
> >
> > gpu_ddr_opp_table: gpu_ddr_opp_table {
> >       compatible = "operating-points-v2";
> >
> >       gpu_ddr_1525: opp-1525 {
> >               opp-peak-KBps = <1525000>;
> >               opp-avg-KBps = <452000>;
> >       };
> >       gpu_ddr_3051: opp-3051 {
> >               opp-peak-KBps = <3051000>;
> >               opp-avg-KBps = <915000>;
> >       };
> >       gpu_ddr_7500: opp-7500 {
> >               opp-peak-KBps = <7500000>;
> >               opp-avg-KBps = <3000000>;
> >       };
> > };
> >
> > gpu_opp_table: gpu_opp_table {
> >       compatible = "operating-points-v2";
> >       opp-shared;
> >
> >       opp-200000000 {
> >               opp-hz = /bits/ 64 <200000000>;
> >       };
> >       opp-400000000 {
> >               opp-hz = /bits/ 64 <400000000>;
> >       };
> > };
> >
> > gpu@7864000 {
> >       ...
> >       operating-points-v2 = <&gpu_opp_table>, <&gpu_cache_opp_table>, <&gpu_ddr_opp_table>;
> >       ...
> > };
>
> One feedback I missed giving earlier. Will it be possible to get some
> user code merged along with this ? I want to make sure anything we add
> ends up getting used.

Sibi might be working on doing that for the SDM845 CPUfreq driver.
Georgi could also change his GPU driver use case to use this BW OPP
table and required-opps.

The problem is that people don't want to start using this until we
decide on the DT representation. So it's like a chicken and egg
situation.

-Saravana

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox