[PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

Instead of hard coding the product register in rcar_du_crtc.c,
read it based on the device tree.

This patch series is based on

https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/gen3-latest
renesas-drivers-2016-05-17-v4.6
4717ef6d59c3204e385c
Revert "ASoC: simple-card: Add pm callbacks to platform driver"

It's boot tested on r8a7795 Salvator-X and checked that the same
product register value is read without and with this patch series.

Dirk

[PATCH 1/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

The product register of the Renesas R-Car3 isn't part of any SoC
module, but "just" a register at address 0xFFF00044.

It provides some configuration information about the SoC itself, like
the number of the CA57, CA53 and CR7 cores, the product description
(R-Car H3, R-Car M3-W etc) and the sample revision.

This information is needed by some drivers to enable or disable
specific features which are present or not in the SoC.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 drivers/soc/renesas/Makefile    |  4 ++--
 drivers/soc/renesas/rcar3-prr.c | 49 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 drivers/soc/renesas/rcar3-prr.c

diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
index cd85cd5..d39990a 100644
--- a/drivers/soc/renesas/Makefile
+++ b/drivers/soc/renesas/Makefile
@@ -4,5 +4,5 @@ obj-$(CONFIG_ARCH_R8A7791)	+= rcar-sysc.o r8a7791-sysc.o
 # R-Car M2-N is identical to R-Car M2-W w.r.t. power domains.
 obj-$(CONFIG_ARCH_R8A7793)	+= rcar-sysc.o r8a7791-sysc.o
 obj-$(CONFIG_ARCH_R8A7794)	+= rcar-sysc.o r8a7794-sysc.o
-obj-$(CONFIG_ARCH_R8A7795)	+= rcar-sysc.o r8a7795-sysc.o
-obj-$(CONFIG_ARCH_R8A7796)	+= rcar-sysc.o r8a7796-sysc.o
+obj-$(CONFIG_ARCH_R8A7795)	+= rcar-sysc.o r8a7795-sysc.o rcar3-prr.o
+obj-$(CONFIG_ARCH_R8A7796)	+= rcar-sysc.o r8a7796-sysc.o rcar3-prr.o
diff --git a/drivers/soc/renesas/rcar3-prr.c b/drivers/soc/renesas/rcar3-prr.c
new file mode 100644
index 0000000..8fda1c2
--- /dev/null
+++ b/drivers/soc/renesas/rcar3-prr.c
@@ -0,0 +1,49 @@
+/*
+ * R-Car3 Product Register (PRR) support
+ *
+ * Copyright (C) 2016 Robert Bosch Car Multimedia GmbH
+ *               Dirk Behme <dirk.behme@de.bosch.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+unsigned int __rcar3_prr;
+EXPORT_SYMBOL(__rcar3_prr);
+
+static const struct of_device_id rcar3_prr_ids[] __initconst = {
+	{ .compatible = "renesas,rcar-gen3-prr" },
+	{}
+};
+
+static int __init rcar3_read_prr(void)
+{
+	struct device_node *np;
+	void __iomem *prr;
+
+	np = of_find_matching_node(NULL, rcar3_prr_ids);
+	if (!np) {
+		pr_err("can't find renesas,rcar-gen3-prr device node");
+		return -ENOENT;
+	}
+
+	prr = of_iomap(np, 0);
+	if (!prr) {
+		pr_err("failed to map product register");
+		of_node_put(np);
+		return -ENOMEM;
+	}
+
+	__rcar3_prr = ioread32(prr);
+
+	iounmap(prr);
+	of_node_put(np);
+
+	return 0;
+}
+early_initcall(rcar3_read_prr);
-- 
2.8.0

[PATCH 2/4] arm64: dts: Renesas: R-Car3: Add product register

[Dirk Behme]

The product register of the Renesas R-Car3 isn't part of any SoC
module, but "just" a register at address 0xFFF00044.

It provides some configuration information about the SoC itself, like
the number of the CA57, CA53 and CR7 cores, the product description
(R-Car H3, R-Car M3-W etc) and the sample revision.

This information is needed by some drivers to enable or disable
specific features which are present or not in the SoC.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 .../bindings/soc/renesas/renesas,rcar-gen3-prr.txt  | 21 +++++++++++++++++++++
 arch/arm64/boot/dts/renesas/r8a7795.dtsi            |  6 ++++++
 arch/arm64/boot/dts/renesas/r8a7796.dtsi            |  6 ++++++
 3 files changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/renesas/renesas,rcar-gen3-prr.txt

diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rcar-gen3-prr.txt b/Documentation/devicetree/bindings/soc/renesas/renesas,rcar-gen3-prr.txt
new file mode 100644
index 0000000..23101cd
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rcar-gen3-prr.txt
@@ -0,0 +1,21 @@
+Renesas R-Car3 product register binding
+---------------------------------------
+
+The product register of the Renesas R-Car3 provides some configuration
+information about the SoC itself, like the number of the CA57, CA53 and
+CR7 cores, the product description (R-Car H3, R-Car M3-W etc) and the
+sample revision. This information is needed by some drivers to enable
+or disable specific features which are present or not in the SoC.
+
+Required properties:
+- compatible : "renesas,rcar-gen3-prr"
+	       and additionally a SoC specific property like
+	       "renesas,r8a7795-prr" or
+	       "renesas,r8a7796-prr"
+- reg : Location and size of the product register
+
+Example:
+	prr: product-register@fff00044 {
+		compatible = "renesas,rcar-gen3-prr", "renesas,r8a7795-prr";
+		reg = <0 0xfff00044 0 0x4>;
+	};
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 26df300..0ae4687 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1610,5 +1610,11 @@
 			interrupts = <0 436 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&cpg CPG_MOD 728>;
 		};
+
+		prr: product-register@fff00044 {
+			compatible = "renesas,rcar-gen3-prr",
+				     "renesas,r8a7795-prr";
+			reg = <0 0xfff00044 0 0x4>;
+		};
 	};
 };
diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
index 1389528..100b531 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
@@ -141,5 +141,11 @@
 			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
 			status = "disabled";
 		};
+
+		prr: product-register@fff00044 {
+			compatible = "renesas,rcar-gen3-prr",
+				     "renesas,r8a7796-prr";
+			reg = <0 0xfff00044 0 0x4>;
+		};
 	};
 };
-- 
2.8.0

[PATCH 3/4] ARM: Renesas: R-Car3: Add cpu and revision handlers

[Dirk Behme]

The Renesas R-Car3 product register (PRR) helps us to identify the
SoC and revision we are running on. Add helper functions to check
for this easily in code where it's needed.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 include/linux/soc/renesas/rcar3-prr.h | 51 +++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 include/linux/soc/renesas/rcar3-prr.h

diff --git a/include/linux/soc/renesas/rcar3-prr.h b/include/linux/soc/renesas/rcar3-prr.h
new file mode 100644
index 0000000..9bbe369
--- /dev/null
+++ b/include/linux/soc/renesas/rcar3-prr.h
@@ -0,0 +1,51 @@
+/*
+ * R-Car3 Product Register (PRR) support
+ *
+ * Copyright (C) 2016 Robert Bosch Car Multimedia GmbH
+ *               Dirk Behme <dirk.behme@de.bosch.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#ifndef __SOC_RENESAS_RCAR3_PRR_H__
+#define __SOC_RENESAS_RCAR3_PRR_H__
+
+#define PRODUCT_MASK	(0x7f << 8)
+#define PRODUCT_H3	(0x4f << 8)
+#define PRODUCT_M3W	(0x52 << 8)
+
+#define CUT_MASK	(0x000000ff)
+#define CUT_ES1		(0x0 << 0)
+#define CUT_ES2		(0x1 << 4)
+#define CUT_ES3		(0x1 << 5)
+
+extern unsigned int __rcar3_prr;
+
+static inline bool cpu_is_rcar3_h3(void)
+{
+	return (__rcar3_prr & PRODUCT_MASK) == PRODUCT_H3;
+}
+
+static inline bool cpu_is_rcar3_m3w(void)
+{
+	return (__rcar3_prr & PRODUCT_MASK) == PRODUCT_M3W;
+}
+
+static inline bool revision_is_rcar3_es1(void)
+{
+	return (__rcar3_prr & CUT_MASK) == CUT_ES1;
+}
+
+static inline bool revision_is_rcar3_es2(void)
+{
+	return (__rcar3_prr & CUT_MASK) == CUT_ES2;
+}
+
+static inline bool revision_is_rcar3_es3(void)
+{
+	return (__rcar3_prr & CUT_MASK) == CUT_ES3;
+}
+
+#endif /* __SOC_RENESAS_RCAR3_PRR_H__ */
-- 
2.8.0

[PATCH 4/4] drm: rcar-du: Use product register framework

[Dirk Behme]

Instead of hard coding the product register in the rcar-du, use
the framework for it to get the SoC version and the revision needed
for the enabling the workaround.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index e10943b..ee639a6 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -13,6 +13,7 @@
 
 #include <linux/clk.h>
 #include <linux/mutex.h>
+#include <linux/soc/renesas/rcar3-prr.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic.h>
@@ -30,12 +31,6 @@
 #include "rcar_du_regs.h"
 #include "rcar_du_vsp.h"
 
-#define PRODUCT_REG	0xfff00044
-#define PRODUCT_H3_BIT	(0x4f << 8)
-#define PRODUCT_MASK	(0x7f << 8)
-#define CUT_ES1		(0x00)
-#define CUT_ES1_MASK	(0x000000ff)
-
 static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
 {
 	struct rcar_du_device *rcdu = rcrtc->group->dev;
@@ -167,7 +162,6 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	u32 div;
 	u32 dpll_reg = 0;
 	struct dpll_info *dpll;
-	void __iomem *product_reg;
 	bool h3_es1_workaround = false;
 
 	dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
@@ -175,11 +169,8 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 		return;
 
 	/* DU2 DPLL Clock Select bit workaround in R-Car H3(ES1.0) */
-	product_reg = ioremap(PRODUCT_REG, 0x04);
-	if (((readl(product_reg) & PRODUCT_MASK) == PRODUCT_H3_BIT)
-		&& ((readl(product_reg) & CUT_ES1_MASK) == CUT_ES1))
+	if (cpu_is_rcar3_h3() && revision_is_rcar3_es1())
 		h3_es1_workaround = true;
-	iounmap(product_reg);
 
 	/* Compute the clock divisor and select the internal or external dot
 	 * clock based on the requested frequency.
-- 
2.8.0

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Geert Uytterhoeven]

Hi Dirk,

On Wed, May 25, 2016 at 10:58 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Instead of hard coding the product register in rcar_du_crtc.c,
> read it based on the device tree.
>
> This patch series is based on
>
> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/gen3-latest
> renesas-drivers-2016-05-17-v4.6
> 4717ef6d59c3204e385c
> Revert "ASoC: simple-card: Add pm callbacks to platform driver"
>
> It's boot tested on r8a7795 Salvator-X and checked that the same
> product register value is read without and with this patch series.

Thanks for your series!

Given the use of PRR you saw is in a patch that's definitely not ready for
upstream, I think adding a full-fledged PRR driver is a bit premature.

So far we've always planned to handle differences in ES versions using the
compatible value, cfr. "renesas,sata-r8a7790-es1".
But in general we target the latest (production) version in upstream.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

Hi Geert,

On 26.05.2016 09:14, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Wed, May 25, 2016 at 10:58 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> Instead of hard coding the product register in rcar_du_crtc.c,
>> read it based on the device tree.
>>
>> This patch series is based on
>>
>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/gen3-latest
>> renesas-drivers-2016-05-17-v4.6
>> 4717ef6d59c3204e385c
>> Revert "ASoC: simple-card: Add pm callbacks to platform driver"
>>
>> It's boot tested on r8a7795 Salvator-X and checked that the same
>> product register value is read without and with this patch series.
>
> Thanks for your series!
>
> Given the use of PRR you saw is in a patch that's definitely not ready for
> upstream, I think adding a full-fledged PRR driver is a bit premature.
>
> So far we've always planned to handle differences in ES versions using the
> compatible value, cfr. "renesas,sata-r8a7790-es1".
> But in general we target the latest (production) version in upstream.


Well, there are several issues ;)


First, regarding "handle differences in ES versions using the compatible 
value, cfr. "renesas,sata-r8a7790-es1": I can't talk about r8a7790. But 
for r8a7795 & r8a7796 we are able to auto-detect this by the PRR. And 
I'd think what can be auto detected, should be auto detected. And not 
handled via the device tree.


Second: Other SoCs families show us that we definitely need such kind of 
cpu_is() and revision_is() logic.


Third: Regarding "But in general we target the latest (production) 
version in upstream": Please keep in mind how Renesas is doing their 
BSPs. For the Renesas BSPs, Renesas picks your renesas-drivers (and not 
upstream!). I.e. what ever is in your renesas-drivers might end up in a 
customer BSP. And this is what really hurts us (as a customer) here.

Now, we have a Renesas BSP, based on your renesas-drivers, which has a 
hard coded MODEMR and PRODUCT_REG (and I think ADVFS, but that seems to 
be from Renesas and not from renesas-drivers). And this really hurts us. 
Therefore I try to fix this in your renesas-drivers, to hopefully have 
it fixed in the next Renesas BSP. *And* discuss with the community how 
to further improve it.

I.e. in a frist step, I'm mainly thinking about getting renesas-drivers 
"fixed" and with this the next Renesas BSP.

Best regards

Dirk

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Magnus Damm]

Hi Dirk,

On Thu, May 26, 2016 at 4:48 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Hi Geert,
>
> On 26.05.2016 09:14, Geert Uytterhoeven wrote:
>>
>> Hi Dirk,
>>
>> On Wed, May 25, 2016 at 10:58 AM, Dirk Behme <dirk.behme@de.bosch.com>
>> wrote:
>>>
>>> Instead of hard coding the product register in rcar_du_crtc.c,
>>> read it based on the device tree.
>>>
>>> This patch series is based on
>>>
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/gen3-latest
>>> renesas-drivers-2016-05-17-v4.6
>>> 4717ef6d59c3204e385c
>>> Revert "ASoC: simple-card: Add pm callbacks to platform driver"
>>>
>>> It's boot tested on r8a7795 Salvator-X and checked that the same
>>> product register value is read without and with this patch series.
>>
>>
>> Thanks for your series!
>>
>> Given the use of PRR you saw is in a patch that's definitely not ready for
>> upstream, I think adding a full-fledged PRR driver is a bit premature.
>>
>> So far we've always planned to handle differences in ES versions using the
>> compatible value, cfr. "renesas,sata-r8a7790-es1".
>> But in general we target the latest (production) version in upstream.
>
> Well, there are several issues ;)
>
> First, regarding "handle differences in ES versions using the compatible
> value, cfr. "renesas,sata-r8a7790-es1": I can't talk about r8a7790. But for
> r8a7795 & r8a7796 we are able to auto-detect this by the PRR. And I'd think
> what can be auto detected, should be auto detected. And not handled via the
> device tree.
>
> Second: Other SoCs families show us that we definitely need such kind of
> cpu_is() and revision_is() logic.

I don't think anyone disagrees that it makes sense to be able to
determine ES version during runtime. The questions in my mind are how
to do it and the urgency.

So far we have decided to use the compatible string since it is a
common driver model abstraction already used by device drivers and
hardware description in DT. Using DT compat string matching we can
have logic in the drivers to handle ES differences if needed. As for
how to enable the workaround, my opinion is that the missing piece
consists of ES workaround code that appends ES suffix information to
the DT compat strings automatically during boot.

I suppose the workaround is not yet implemented because no one has
deemed this topic as particularly urgent. Until it becomes urgent or a
new ES version appears the affected users can simply adjust the DT
binding themselves. This is what happened to the "sata-r8a7790-es1"
case above.

Thanks,

/ magnus

Re: [PATCH 4/4] drm: rcar-du: Use product register framework

[Magnus Damm]

Hi Dirk,

On Wed, May 25, 2016 at 5:58 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Instead of hard coding the product register in the rcar-du, use
> the framework for it to get the SoC version and the revision needed
> for the enabling the workaround.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index e10943b..ee639a6 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -13,6 +13,7 @@
>
>  #include <linux/clk.h>
>  #include <linux/mutex.h>
> +#include <linux/soc/renesas/rcar3-prr.h>
>
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic.h>
> @@ -30,12 +31,6 @@
>  #include "rcar_du_regs.h"
>  #include "rcar_du_vsp.h"
>
> -#define PRODUCT_REG    0xfff00044
> -#define PRODUCT_H3_BIT (0x4f << 8)
> -#define PRODUCT_MASK   (0x7f << 8)
> -#define CUT_ES1                (0x00)
> -#define CUT_ES1_MASK   (0x000000ff)
> -
>  static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
>  {
>         struct rcar_du_device *rcdu = rcrtc->group->dev;
> @@ -167,7 +162,6 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>         u32 div;
>         u32 dpll_reg = 0;
>         struct dpll_info *dpll;
> -       void __iomem *product_reg;
>         bool h3_es1_workaround = false;
>
>         dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
> @@ -175,11 +169,8 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>                 return;
>
>         /* DU2 DPLL Clock Select bit workaround in R-Car H3(ES1.0) */
> -       product_reg = ioremap(PRODUCT_REG, 0x04);
> -       if (((readl(product_reg) & PRODUCT_MASK) == PRODUCT_H3_BIT)
> -               && ((readl(product_reg) & CUT_ES1_MASK) == CUT_ES1))
> +       if (cpu_is_rcar3_h3() && revision_is_rcar3_es1())
>                 h3_es1_workaround = true;
> -       iounmap(product_reg);

Thanks for your efforts!

I agree that doing ioremap() with a hard coded address and reading out
an unrelated register looks like a short term workaround. Replacing
that with something cleaner makes sense. The question in my mind is
how to make it cleaner.

I can see that in this series you have introduced some specialized
functions to be able to check ES version. This may be slightly
cleaner, but the design comes with some drawbacks. One major drawback
is that specialized functions makes it difficult to move the code
between several architectures. So if we should clean up the code then
we should go all the way and do it in a reusable way.

Over the years many drivers have been initially written on the SH
arch, then moved to 32-bit ARM and recently also be used for ARM v8.
Not sure about the DU driver, it may only be shared between 32-bit ARM
and 64-bit ARM instead of all 3 architectures. I think the SCIF driver
has been used on even more architectures like H8. One major reason why
we have been able to reuse code over several architectures is that
we've focus on using standard functions and stayed away from
architecture-specific extensions. In case a short term workaround is
needed then it should not break the code on other architectures. The
initial short term code above would work on any of the supported SoCs
and it would also compile on a whole bunch of other architectures for
compile testing purpose.

Anyway, please use something standard that can be used on several
architectures without causing breakage. You should also discuss this
with the initial author of the DU driver. He may have a different
opinion than me, but I'm sure he agrees on that the driver should keep
on working on several architectures.

Thanks,

/ magnus

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

Hi Magnus,

On 27.05.2016 05:13, Magnus Damm wrote:
> Hi Dirk,
>
> On Thu, May 26, 2016 at 4:48 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> Hi Geert,
>>
>> On 26.05.2016 09:14, Geert Uytterhoeven wrote:
>>>
>>> Hi Dirk,
>>>
>>> On Wed, May 25, 2016 at 10:58 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>>
>>>> Instead of hard coding the product register in rcar_du_crtc.c,
>>>> read it based on the device tree.
>>>>
>>>> This patch series is based on
>>>>
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/gen3-latest
>>>> renesas-drivers-2016-05-17-v4.6
>>>> 4717ef6d59c3204e385c
>>>> Revert "ASoC: simple-card: Add pm callbacks to platform driver"
>>>>
>>>> It's boot tested on r8a7795 Salvator-X and checked that the same
>>>> product register value is read without and with this patch series.
>>>
>>>
>>> Thanks for your series!
>>>
>>> Given the use of PRR you saw is in a patch that's definitely not ready for
>>> upstream, I think adding a full-fledged PRR driver is a bit premature.
>>>
>>> So far we've always planned to handle differences in ES versions using the
>>> compatible value, cfr. "renesas,sata-r8a7790-es1".
>>> But in general we target the latest (production) version in upstream.
>>
>> Well, there are several issues ;)
>>
>> First, regarding "handle differences in ES versions using the compatible
>> value, cfr. "renesas,sata-r8a7790-es1": I can't talk about r8a7790. But for
>> r8a7795 & r8a7796 we are able to auto-detect this by the PRR. And I'd think
>> what can be auto detected, should be auto detected. And not handled via the
>> device tree.
>>
>> Second: Other SoCs families show us that we definitely need such kind of
>> cpu_is() and revision_is() logic.
>
> I don't think anyone disagrees that it makes sense to be able to
> determine ES version during runtime. The questions in my mind are how
> to do it


I've made a proposal ;) And I'm happy to discuss technically about it.


> and the urgency.


Regarding the urgency: Someone has accepted the hard coded PRODUCT 
register (and MODEMR) being in renesas-drivers, now:

https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177

This does really hurts us.

Therefore, I try to get this removed as soon as possible to hopefully 
get a Renesas BSP without this, the next time.

If anybody finds an other way to remove that, that would be fine, too.


> So far we have decided to use the compatible string since it is a
> common driver model abstraction already used by device drivers and
> hardware description in DT. Using DT compat string matching we can
> have logic in the drivers to handle ES differences if needed. As for
> how to enable the workaround, my opinion is that the missing piece
> consists of ES workaround code that appends ES suffix information to
> the DT compat strings automatically during boot.


Technically this sounds slightly too complicated to me. As I have to 
advocate my proposal (inspired from an other mainline SoC family) I'd 
think that my proposal is easier.


> I suppose the workaround is not yet implemented because no one has
> deemed this topic as particularly urgent. Until it becomes urgent or a
> new ES version appears the affected users can simply adjust the DT
> binding themselves. This is what happened to the "sata-r8a7790-es1"
> case above.

Best regards

Dirk

Re: [PATCH 4/4] drm: rcar-du: Use product register framework

[Dirk Behme]

Hi Magnus,

On 27.05.2016 05:40, Magnus Damm wrote:
> Hi Dirk,
>
> On Wed, May 25, 2016 at 5:58 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> Instead of hard coding the product register in the rcar-du, use
>> the framework for it to get the SoC version and the revision needed
>> for the enabling the workaround.
>>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 13 ++-----------
>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> index e10943b..ee639a6 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -13,6 +13,7 @@
>>
>>  #include <linux/clk.h>
>>  #include <linux/mutex.h>
>> +#include <linux/soc/renesas/rcar3-prr.h>
>>
>>  #include <drm/drmP.h>
>>  #include <drm/drm_atomic.h>
>> @@ -30,12 +31,6 @@
>>  #include "rcar_du_regs.h"
>>  #include "rcar_du_vsp.h"
>>
>> -#define PRODUCT_REG    0xfff00044
>> -#define PRODUCT_H3_BIT (0x4f << 8)
>> -#define PRODUCT_MASK   (0x7f << 8)
>> -#define CUT_ES1                (0x00)
>> -#define CUT_ES1_MASK   (0x000000ff)
>> -
>>  static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
>>  {
>>         struct rcar_du_device *rcdu = rcrtc->group->dev;
>> @@ -167,7 +162,6 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>>         u32 div;
>>         u32 dpll_reg = 0;
>>         struct dpll_info *dpll;
>> -       void __iomem *product_reg;
>>         bool h3_es1_workaround = false;
>>
>>         dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
>> @@ -175,11 +169,8 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>>                 return;
>>
>>         /* DU2 DPLL Clock Select bit workaround in R-Car H3(ES1.0) */
>> -       product_reg = ioremap(PRODUCT_REG, 0x04);
>> -       if (((readl(product_reg) & PRODUCT_MASK) == PRODUCT_H3_BIT)
>> -               && ((readl(product_reg) & CUT_ES1_MASK) == CUT_ES1))
>> +       if (cpu_is_rcar3_h3() && revision_is_rcar3_es1())
>>                 h3_es1_workaround = true;
>> -       iounmap(product_reg);
>
> Thanks for your efforts!
>
> I agree that doing ioremap() with a hard coded address and reading out
> an unrelated register looks like a short term workaround. Replacing
> that with something cleaner makes sense. The question in my mind is
> how to make it cleaner.
>
> I can see that in this series you have introduced some specialized
> functions to be able to check ES version. This may be slightly
> cleaner, but the design comes with some drawbacks. One major drawback
> is that specialized functions makes it difficult to move the code
> between several architectures. So if we should clean up the code then
> we should go all the way and do it in a reusable way.
>
> Over the years many drivers have been initially written on the SH
> arch, then moved to 32-bit ARM and recently also be used for ARM v8.
> Not sure about the DU driver, it may only be shared between 32-bit ARM
> and 64-bit ARM instead of all 3 architectures. I think the SCIF driver
> has been used on even more architectures like H8. One major reason why
> we have been able to reuse code over several architectures is that
> we've focus on using standard functions and stayed away from
> architecture-specific extensions. In case a short term workaround is
> needed then it should not break the code on other architectures. The
> initial short term code above would work on any of the supported SoCs
> and it would also compile on a whole bunch of other architectures for
> compile testing purpose.


Please correct me if I'm wrong, but the initial short term code does 
work on all SoCs having a PRODUCT_REG @ 0xfff00044. Correct?

And my code should work identically, no?

Or where do you see the difference?

Going even one step further, running the initial short term code on SoCs 
*not* having a PRODUCT_REG @ 0xfff00044 will fail randomly, depending on 
the result read from that address, then.

Instead, my proposal, will give at least a valid error message (assuming 
the device tree entries are set correctly).


Regarding compilation, you are correct. Most probably I should add some 
empty stub functions for the cpu_is_xx() and revision_is_xx() functions 
used on unsupported platforms in a v2.


> Anyway, please use something standard that can be used on several
> architectures without causing breakage. You should also discuss this
> with the initial author of the DU driver. He may have a different
> opinion than me, but I'm sure he agrees on that the driver should keep
> on working on several architectures.


I agree here and I'm happy do discuss this. I tried this with adding 
Koji Matsuoka into all discussions CC. Do you like to propose anybody else?


Best regards

Dirk

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Geert Uytterhoeven]

On Fri, May 27, 2016 at 9:56 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>>>> Given the use of PRR you saw is in a patch that's definitely not ready for
>>>> upstream, I think adding a full-fledged PRR driver is a bit premature.

>> I don't think anyone disagrees that it makes sense to be able to
>> determine ES version during runtime. The questions in my mind are how
>> to do it
>
> I've made a proposal ;) And I'm happy to discuss technically about it.
>
>> and the urgency.
>
> Regarding the urgency: Someone has accepted the hard coded PRODUCT register
> (and MODEMR) being in renesas-drivers, now:
>
> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177

I will drop the topic branch topic/salvator-x-hdmi-prototype-v2-rebased1
from next renesas-drivers release, as it's definitely not ready.

People who want to use that branch can still merge it themselves.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

On 27.05.2016 10:39, Geert Uytterhoeven wrote:
> On Fri, May 27, 2016 at 9:56 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>>>>> Given the use of PRR you saw is in a patch that's definitely not ready for
>>>>> upstream, I think adding a full-fledged PRR driver is a bit premature.
>
>>> I don't think anyone disagrees that it makes sense to be able to
>>> determine ES version during runtime. The questions in my mind are how
>>> to do it
>>
>> I've made a proposal ;) And I'm happy to discuss technically about it.
>>
>>> and the urgency.
>>
>> Regarding the urgency: Someone has accepted the hard coded PRODUCT register
>> (and MODEMR) being in renesas-drivers, now:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177
>
> I will drop the topic branch topic/salvator-x-hdmi-prototype-v2-rebased1
> from next renesas-drivers release, as it's definitely not ready.
>
> People who want to use that branch can still merge it themselves.


Ok, fine, thanks. But maybe it makes sense to continue to discuss about 
the SoC/revision detection independently, then. Maybe with less urgency ;)


Btw, what's about the MODEMR patch series? As it's from you and there 
have been no further comments with v3, I'm somehow hoping to get it 
integrated.


Best regards

Dirk

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Geert Uytterhoeven]

On Fri, May 27, 2016 at 10:44 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Btw, what's about the MODEMR patch series? As it's from you and there have
> been no further comments with v3, I'm somehow hoping to get it integrated.

I'll be working on it soon.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] drm: rcar-du: Use product register framework

[Magnus Damm]

Hi Dirk,

On Fri, May 27, 2016 at 5:16 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Hi Magnus,
>
>
> On 27.05.2016 05:40, Magnus Damm wrote:
>>
>> Hi Dirk,
>>
>> On Wed, May 25, 2016 at 5:58 PM, Dirk Behme <dirk.behme@de.bosch.com>
>> wrote:
>>>
>>> Instead of hard coding the product register in the rcar-du, use
>>> the framework for it to get the SoC version and the revision needed
>>> for the enabling the workaround.
>>>
>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>> ---
>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 13 ++-----------
>>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> index e10943b..ee639a6 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> @@ -13,6 +13,7 @@
>>>
>>>  #include <linux/clk.h>
>>>  #include <linux/mutex.h>
>>> +#include <linux/soc/renesas/rcar3-prr.h>
>>>
>>>  #include <drm/drmP.h>
>>>  #include <drm/drm_atomic.h>
>>> @@ -30,12 +31,6 @@
>>>  #include "rcar_du_regs.h"
>>>  #include "rcar_du_vsp.h"
>>>
>>> -#define PRODUCT_REG    0xfff00044
>>> -#define PRODUCT_H3_BIT (0x4f << 8)
>>> -#define PRODUCT_MASK   (0x7f << 8)
>>> -#define CUT_ES1                (0x00)
>>> -#define CUT_ES1_MASK   (0x000000ff)
>>> -
>>>  static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
>>>  {
>>>         struct rcar_du_device *rcdu = rcrtc->group->dev;
>>> @@ -167,7 +162,6 @@ static void rcar_du_crtc_set_display_timing(struct
>>> rcar_du_crtc *rcrtc)
>>>         u32 div;
>>>         u32 dpll_reg = 0;
>>>         struct dpll_info *dpll;
>>> -       void __iomem *product_reg;
>>>         bool h3_es1_workaround = false;
>>>
>>>         dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
>>> @@ -175,11 +169,8 @@ static void rcar_du_crtc_set_display_timing(struct
>>> rcar_du_crtc *rcrtc)
>>>                 return;
>>>
>>>         /* DU2 DPLL Clock Select bit workaround in R-Car H3(ES1.0) */
>>> -       product_reg = ioremap(PRODUCT_REG, 0x04);
>>> -       if (((readl(product_reg) & PRODUCT_MASK) == PRODUCT_H3_BIT)
>>> -               && ((readl(product_reg) & CUT_ES1_MASK) == CUT_ES1))
>>> +       if (cpu_is_rcar3_h3() && revision_is_rcar3_es1())
>>>                 h3_es1_workaround = true;
>>> -       iounmap(product_reg);
>>
>>
>> Thanks for your efforts!
>>
>> I agree that doing ioremap() with a hard coded address and reading out
>> an unrelated register looks like a short term workaround. Replacing
>> that with something cleaner makes sense. The question in my mind is
>> how to make it cleaner.
>>
>> I can see that in this series you have introduced some specialized
>> functions to be able to check ES version. This may be slightly
>> cleaner, but the design comes with some drawbacks. One major drawback
>> is that specialized functions makes it difficult to move the code
>> between several architectures. So if we should clean up the code then
>> we should go all the way and do it in a reusable way.
>>
>> Over the years many drivers have been initially written on the SH
>> arch, then moved to 32-bit ARM and recently also be used for ARM v8.
>> Not sure about the DU driver, it may only be shared between 32-bit ARM
>> and 64-bit ARM instead of all 3 architectures. I think the SCIF driver
>> has been used on even more architectures like H8. One major reason why
>> we have been able to reuse code over several architectures is that
>> we've focus on using standard functions and stayed away from
>> architecture-specific extensions. In case a short term workaround is
>> needed then it should not break the code on other architectures. The
>> initial short term code above would work on any of the supported SoCs
>> and it would also compile on a whole bunch of other architectures for
>> compile testing purpose.
>
>
>
> Please correct me if I'm wrong, but the initial short term code does work on
> all SoCs having a PRODUCT_REG @ 0xfff00044. Correct?
>
> And my code should work identically, no?
>
> Or where do you see the difference?
>
> Going even one step further, running the initial short term code on SoCs
> *not* having a PRODUCT_REG @ 0xfff00044 will fail randomly, depending on the
> result read from that address, then.
>
> Instead, my proposal, will give at least a valid error message (assuming the
> device tree entries are set correctly).

Your code is an improvement to the original one for sure, and I think
it should work identically but with better error reporting...

> Regarding compilation, you are correct. Most probably I should add some
> empty stub functions for the cpu_is_xx() and revision_is_xx() functions used
> on unsupported platforms in a v2.

.. however adding a specialized API like this seems like the wrong
direction IMO. As you probably know, the hardware IP included in SoCs
come from various vendors with potentially shared device drivers.
Think USB host controller drivers or serial ports for instance. With
your approach you would potentially force shared drivers to include
<linux/soc/renesas/rcar3-prr.h> and similar per-SoC-vendor includes
just to be able to check version of a specific IP. This does not scale
very well IMO.

Because of this I prefer to go with some kind of shared standard API
instead of per-vendor solutions.

>> Anyway, please use something standard that can be used on several
>> architectures without causing breakage. You should also discuss this
>> with the initial author of the DU driver. He may have a different
>> opinion than me, but I'm sure he agrees on that the driver should keep
>> on working on several architectures.
>
> I agree here and I'm happy do discuss this. I tried this with adding Koji
> Matsuoka into all discussions CC. Do you like to propose anybody else?

I suspect Matsuoka-san wrote this code with the BSP target in mind.
Then this was broken out into a series when trying to enable HDMI
support on top of mainline. Like Geert wrote in his email, the code
seems a bit immature at this point. So simply dropping the HDMI series
for now makes sense to me.

Thanks,

/ magnus

Re: [PATCH 4/4] drm: rcar-du: Use product register framework

[Dirk Behme]

Hi Magnus,

On 27.05.2016 11:56, Magnus Damm wrote:
> Hi Dirk,
>
> On Fri, May 27, 2016 at 5:16 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> Hi Magnus,
>>
>>
>> On 27.05.2016 05:40, Magnus Damm wrote:
>>>
>>> Hi Dirk,
>>>
>>> On Wed, May 25, 2016 at 5:58 PM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>>
>>>> Instead of hard coding the product register in the rcar-du, use
>>>> the framework for it to get the SoC version and the revision needed
>>>> for the enabling the workaround.
>>>>
>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>> ---
>>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 13 ++-----------
>>>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> index e10943b..ee639a6 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> @@ -13,6 +13,7 @@
>>>>
>>>>  #include <linux/clk.h>
>>>>  #include <linux/mutex.h>
>>>> +#include <linux/soc/renesas/rcar3-prr.h>
>>>>
>>>>  #include <drm/drmP.h>
>>>>  #include <drm/drm_atomic.h>
>>>> @@ -30,12 +31,6 @@
>>>>  #include "rcar_du_regs.h"
>>>>  #include "rcar_du_vsp.h"
>>>>
>>>> -#define PRODUCT_REG    0xfff00044
>>>> -#define PRODUCT_H3_BIT (0x4f << 8)
>>>> -#define PRODUCT_MASK   (0x7f << 8)
>>>> -#define CUT_ES1                (0x00)
>>>> -#define CUT_ES1_MASK   (0x000000ff)
>>>> -
>>>>  static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
>>>>  {
>>>>         struct rcar_du_device *rcdu = rcrtc->group->dev;
>>>> @@ -167,7 +162,6 @@ static void rcar_du_crtc_set_display_timing(struct
>>>> rcar_du_crtc *rcrtc)
>>>>         u32 div;
>>>>         u32 dpll_reg = 0;
>>>>         struct dpll_info *dpll;
>>>> -       void __iomem *product_reg;
>>>>         bool h3_es1_workaround = false;
>>>>
>>>>         dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
>>>> @@ -175,11 +169,8 @@ static void rcar_du_crtc_set_display_timing(struct
>>>> rcar_du_crtc *rcrtc)
>>>>                 return;
>>>>
>>>>         /* DU2 DPLL Clock Select bit workaround in R-Car H3(ES1.0) */
>>>> -       product_reg = ioremap(PRODUCT_REG, 0x04);
>>>> -       if (((readl(product_reg) & PRODUCT_MASK) == PRODUCT_H3_BIT)
>>>> -               && ((readl(product_reg) & CUT_ES1_MASK) == CUT_ES1))
>>>> +       if (cpu_is_rcar3_h3() && revision_is_rcar3_es1())
>>>>                 h3_es1_workaround = true;
>>>> -       iounmap(product_reg);
>>>
>>>
>>> Thanks for your efforts!
>>>
>>> I agree that doing ioremap() with a hard coded address and reading out
>>> an unrelated register looks like a short term workaround. Replacing
>>> that with something cleaner makes sense. The question in my mind is
>>> how to make it cleaner.
>>>
>>> I can see that in this series you have introduced some specialized
>>> functions to be able to check ES version. This may be slightly
>>> cleaner, but the design comes with some drawbacks. One major drawback
>>> is that specialized functions makes it difficult to move the code
>>> between several architectures. So if we should clean up the code then
>>> we should go all the way and do it in a reusable way.
>>>
>>> Over the years many drivers have been initially written on the SH
>>> arch, then moved to 32-bit ARM and recently also be used for ARM v8.
>>> Not sure about the DU driver, it may only be shared between 32-bit ARM
>>> and 64-bit ARM instead of all 3 architectures. I think the SCIF driver
>>> has been used on even more architectures like H8. One major reason why
>>> we have been able to reuse code over several architectures is that
>>> we've focus on using standard functions and stayed away from
>>> architecture-specific extensions. In case a short term workaround is
>>> needed then it should not break the code on other architectures. The
>>> initial short term code above would work on any of the supported SoCs
>>> and it would also compile on a whole bunch of other architectures for
>>> compile testing purpose.
>>
>>
>>
>> Please correct me if I'm wrong, but the initial short term code does work on
>> all SoCs having a PRODUCT_REG @ 0xfff00044. Correct?
>>
>> And my code should work identically, no?
>>
>> Or where do you see the difference?
>>
>> Going even one step further, running the initial short term code on SoCs
>> *not* having a PRODUCT_REG @ 0xfff00044 will fail randomly, depending on the
>> result read from that address, then.
>>
>> Instead, my proposal, will give at least a valid error message (assuming the
>> device tree entries are set correctly).
>
> Your code is an improvement to the original one for sure, and I think
> it should work identically but with better error reporting...
>
>> Regarding compilation, you are correct. Most probably I should add some
>> empty stub functions for the cpu_is_xx() and revision_is_xx() functions used
>> on unsupported platforms in a v2.
>
> .. however adding a specialized API like this seems like the wrong
> direction IMO. As you probably know, the hardware IP included in SoCs
> come from various vendors with potentially shared device drivers.
> Think USB host controller drivers or serial ports for instance. With
> your approach you would potentially force shared drivers to include
> <linux/soc/renesas/rcar3-prr.h> and similar per-SoC-vendor includes
> just to be able to check version of a specific IP. This does not scale
> very well IMO.


If a specific IP on a specific SoC needs a SoC specific (revision-) 
check, I have difficulties to imagine how this could look like better.

You won't put Renesas specific registers/functions/macros in e.g.

<linux/soc-revision.h>

No?

What you could do is create a

<linux/soc-revision.h>

include this in your source code, and this <linux/soc-revision.h> 
includes the <linux/soc/renesas/rcar3-prr.h> based on some #ifdef. Hmm, 
would that be better?


> Because of this I prefer to go with some kind of shared standard API
> instead of per-vendor solutions.


Any proposals?

Several SoCs seem to do this cpu_is_xxx() stuff in mainline.


>>> Anyway, please use something standard that can be used on several
>>> architectures without causing breakage. You should also discuss this
>>> with the initial author of the DU driver. He may have a different
>>> opinion than me, but I'm sure he agrees on that the driver should keep
>>> on working on several architectures.
>>
>> I agree here and I'm happy do discuss this. I tried this with adding Koji
>> Matsuoka into all discussions CC. Do you like to propose anybody else?
>
> I suspect Matsuoka-san wrote this code with the BSP target in mind.
> Then this was broken out into a series when trying to enable HDMI
> support on top of mainline. Like Geert wrote in his email, the code
> seems a bit immature at this point. So simply dropping the HDMI series
> for now makes sense to me.


This does help for the issue with the hard coded PRODUCT register for 
the moment. But please note that it don't solve the problem in long 
term. Once the code should be put back, you'll need that revision check, 
again. Just because there is different hardware out there.


Best regards

Dirk

Re: [PATCH 4/4] drm: rcar-du: Use product register framework

[Magnus Damm]

Hi Dirk,

On Fri, May 27, 2016 at 7:18 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Hi Magnus,
>
>
> On 27.05.2016 11:56, Magnus Damm wrote:
>>
>> Hi Dirk,
>>
>> On Fri, May 27, 2016 at 5:16 PM, Dirk Behme <dirk.behme@de.bosch.com>
>> wrote:
>>>
>>> Hi Magnus,
>>>
>>>
>>> On 27.05.2016 05:40, Magnus Damm wrote:
>>>>
>>>>
>>>> Hi Dirk,
>>>>
>>>> On Wed, May 25, 2016 at 5:58 PM, Dirk Behme <dirk.behme@de.bosch.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Instead of hard coding the product register in the rcar-du, use
>>>>> the framework for it to get the SoC version and the revision needed
>>>>> for the enabling the workaround.
>>>>>
>>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>>> ---
>>>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 13 ++-----------
>>>>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>>> index e10943b..ee639a6 100644
>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>>> @@ -13,6 +13,7 @@
>>>>>
>>>>>  #include <linux/clk.h>
>>>>>  #include <linux/mutex.h>
>>>>> +#include <linux/soc/renesas/rcar3-prr.h>
>>>>>
>>>>>  #include <drm/drmP.h>
>>>>>  #include <drm/drm_atomic.h>
>>>>> @@ -30,12 +31,6 @@
>>>>>  #include "rcar_du_regs.h"
>>>>>  #include "rcar_du_vsp.h"
>>>>>
>>>>> -#define PRODUCT_REG    0xfff00044
>>>>> -#define PRODUCT_H3_BIT (0x4f << 8)
>>>>> -#define PRODUCT_MASK   (0x7f << 8)
>>>>> -#define CUT_ES1                (0x00)
>>>>> -#define CUT_ES1_MASK   (0x000000ff)
>>>>> -
>>>>>  static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
>>>>>  {
>>>>>         struct rcar_du_device *rcdu = rcrtc->group->dev;
>>>>> @@ -167,7 +162,6 @@ static void rcar_du_crtc_set_display_timing(struct
>>>>> rcar_du_crtc *rcrtc)
>>>>>         u32 div;
>>>>>         u32 dpll_reg = 0;
>>>>>         struct dpll_info *dpll;
>>>>> -       void __iomem *product_reg;
>>>>>         bool h3_es1_workaround = false;
>>>>>
>>>>>         dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
>>>>> @@ -175,11 +169,8 @@ static void rcar_du_crtc_set_display_timing(struct
>>>>> rcar_du_crtc *rcrtc)
>>>>>                 return;
>>>>>
>>>>>         /* DU2 DPLL Clock Select bit workaround in R-Car H3(ES1.0) */
>>>>> -       product_reg = ioremap(PRODUCT_REG, 0x04);
>>>>> -       if (((readl(product_reg) & PRODUCT_MASK) == PRODUCT_H3_BIT)
>>>>> -               && ((readl(product_reg) & CUT_ES1_MASK) == CUT_ES1))
>>>>> +       if (cpu_is_rcar3_h3() && revision_is_rcar3_es1())
>>>>>                 h3_es1_workaround = true;
>>>>> -       iounmap(product_reg);
>>>>
>>>>
>>>>
>>>> Thanks for your efforts!
>>>>
>>>> I agree that doing ioremap() with a hard coded address and reading out
>>>> an unrelated register looks like a short term workaround. Replacing
>>>> that with something cleaner makes sense. The question in my mind is
>>>> how to make it cleaner.
>>>>
>>>> I can see that in this series you have introduced some specialized
>>>> functions to be able to check ES version. This may be slightly
>>>> cleaner, but the design comes with some drawbacks. One major drawback
>>>> is that specialized functions makes it difficult to move the code
>>>> between several architectures. So if we should clean up the code then
>>>> we should go all the way and do it in a reusable way.
>>>>
>>>> Over the years many drivers have been initially written on the SH
>>>> arch, then moved to 32-bit ARM and recently also be used for ARM v8.
>>>> Not sure about the DU driver, it may only be shared between 32-bit ARM
>>>> and 64-bit ARM instead of all 3 architectures. I think the SCIF driver
>>>> has been used on even more architectures like H8. One major reason why
>>>> we have been able to reuse code over several architectures is that
>>>> we've focus on using standard functions and stayed away from
>>>> architecture-specific extensions. In case a short term workaround is
>>>> needed then it should not break the code on other architectures. The
>>>> initial short term code above would work on any of the supported SoCs
>>>> and it would also compile on a whole bunch of other architectures for
>>>> compile testing purpose.
>>>
>>>
>>>
>>>
>>> Please correct me if I'm wrong, but the initial short term code does work
>>> on
>>> all SoCs having a PRODUCT_REG @ 0xfff00044. Correct?
>>>
>>> And my code should work identically, no?
>>>
>>> Or where do you see the difference?
>>>
>>> Going even one step further, running the initial short term code on SoCs
>>> *not* having a PRODUCT_REG @ 0xfff00044 will fail randomly, depending on
>>> the
>>> result read from that address, then.
>>>
>>> Instead, my proposal, will give at least a valid error message (assuming
>>> the
>>> device tree entries are set correctly).
>>
>>
>> Your code is an improvement to the original one for sure, and I think
>> it should work identically but with better error reporting...
>>
>>> Regarding compilation, you are correct. Most probably I should add some
>>> empty stub functions for the cpu_is_xx() and revision_is_xx() functions
>>> used
>>> on unsupported platforms in a v2.
>>
>>
>> .. however adding a specialized API like this seems like the wrong
>> direction IMO. As you probably know, the hardware IP included in SoCs
>> come from various vendors with potentially shared device drivers.
>> Think USB host controller drivers or serial ports for instance. With
>> your approach you would potentially force shared drivers to include
>> <linux/soc/renesas/rcar3-prr.h> and similar per-SoC-vendor includes
>> just to be able to check version of a specific IP. This does not scale
>> very well IMO.
>
>
>
> If a specific IP on a specific SoC needs a SoC specific (revision-) check, I
> have difficulties to imagine how this could look like better.
>
> You won't put Renesas specific registers/functions/macros in e.g.
>
> <linux/soc-revision.h>
>
> No?

I prefer not to.

> What you could do is create a
>
> <linux/soc-revision.h>
>
> include this in your source code, and this <linux/soc-revision.h> includes
> the <linux/soc/renesas/rcar3-prr.h> based on some #ifdef. Hmm, would that be
> better?

Hm.. I think this smells similar to including architecture-specific
headers from <asm/...> (or <mach/..> on 32-bit ARM) like many drivers
at least used to do. Instead of quickly sprinkling if statements for
various SoCs and their revisions I think we should break out the
workarounds needed in each driver and assign them a feature flag. Then
associate the SoC-specific DT compat string with the feature flag. Of
course the DT compat strings for various ES versions need to be
documented as well.

>> Because of this I prefer to go with some kind of shared standard API
>> instead of per-vendor solutions.
>
>
>
> Any proposals?

Like mentioned earlier I believe we can reuse DT compat strings for
this purpose.

> Several SoCs seem to do this cpu_is_xxx() stuff in mainline.

Most drivers used to include stuff from <asm/> or <mach/> as well, but
that got reworked into something more sane when the drivers needed to
support more than one architecture.

In my mind the interesting questions are why they need to use such
checks and clarifying the reasons behind cpu_is_xxx() functions were
selected in the first place. They just look like the easy way out.

Normally drivers that need to support many kinds of similar but
slightly different hardware already have some detection method and/or
feature flags or whatnot to support a wide range of hardware. Look at
the 8250 driver files as one example.

>>>> Anyway, please use something standard that can be used on several
>>>> architectures without causing breakage. You should also discuss this
>>>> with the initial author of the DU driver. He may have a different
>>>> opinion than me, but I'm sure he agrees on that the driver should keep
>>>> on working on several architectures.
>>>
>>>
>>> I agree here and I'm happy do discuss this. I tried this with adding Koji
>>> Matsuoka into all discussions CC. Do you like to propose anybody else?
>>
>>
>> I suspect Matsuoka-san wrote this code with the BSP target in mind.
>> Then this was broken out into a series when trying to enable HDMI
>> support on top of mainline. Like Geert wrote in his email, the code
>> seems a bit immature at this point. So simply dropping the HDMI series
>> for now makes sense to me.
>
> This does help for the issue with the hard coded PRODUCT register for the
> moment. But please note that it don't solve the problem in long term. Once
> the code should be put back, you'll need that revision check, again. Just
> because there is different hardware out there.

Can you please explain what the problem is with the hard coded PRODUCT
register access? Of course it is not pretty, but does it break
anything? In my mind the code is dirty but does not cause any issue.
If something is broken please share a link to an email where this has
been reported.

It seems to me that certain renesas-drivers releases contain more
experimental code than others. It is common that code from linux-next
is included as well as experimental driver features. And to make it
more interesting, I suspect the commit that is adding that hard coded
PRODUCT register access is actually coming from the Renesas BSP
initially.

So it seems to me that R-Car Gen3 on-chip HDMI support needs more
effort before upstream merge is possible. This additional effort would
include cleaning up that PRODUCT register access bits. Whatever code
that ended up in some renesas-drivers release (and perhaps the BSP) is
simply some early stage prototype code.

Thanks,

/ magnus

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Magnus Damm]

Hi Dirk,

On Fri, May 27, 2016 at 4:56 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 27.05.2016 05:13, Magnus Damm wrote:
>> I don't think anyone disagrees that it makes sense to be able to
>> determine ES version during runtime. The questions in my mind are how
>> to do it
>
>
>
> I've made a proposal ;) And I'm happy to discuss technically about it.

Thanks! It would be interesting to know the reason behind your
interest in these things. For instance, if you are interested in
reducing run time memory usage, or general source code duplication
reduction. Do you have any target platform that you can upstream board
support for?

>> and the urgency.
>
>
>
> Regarding the urgency: Someone has accepted the hard coded PRODUCT register
> (and MODEMR) being in renesas-drivers, now:
>
> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177
>
> This does really hurts us.
>
> Therefore, I try to get this removed as soon as possible to hopefully get a
> Renesas BSP without this, the next time.
>
> If anybody finds an other way to remove that, that would be fine, too.

Please share the underlying issue so we can fix that.

>> So far we have decided to use the compatible string since it is a
>> common driver model abstraction already used by device drivers and
>> hardware description in DT. Using DT compat string matching we can
>> have logic in the drivers to handle ES differences if needed. As for
>> how to enable the workaround, my opinion is that the missing piece
>> consists of ES workaround code that appends ES suffix information to
>> the DT compat strings automatically during boot.
>
> Technically this sounds slightly too complicated to me. As I have to
> advocate my proposal (inspired from an other mainline SoC family) I'd think
> that my proposal is easier.

It may indeed be easier in the short term, but I feel we need to
consider how to support a wide range of SoCs in a consistent way.

>> I suppose the workaround is not yet implemented because no one has
>> deemed this topic as particularly urgent. Until it becomes urgent or a
>> new ES version appears the affected users can simply adjust the DT
>> binding themselves. This is what happened to the "sata-r8a7790-es1"
>> case above.

Can you see any technical reason why using DT compat strings wouldn't
solve your case?

Thanks,

/ magnus

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

Hi Magnus,

On 01.06.2016 07:19, Magnus Damm wrote:
> Hi Dirk,
>
> On Fri, May 27, 2016 at 4:56 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> On 27.05.2016 05:13, Magnus Damm wrote:
>>> I don't think anyone disagrees that it makes sense to be able to
>>> determine ES version during runtime. The questions in my mind are how
>>> to do it
>>
>>
>>
>> I've made a proposal ;) And I'm happy to discuss technically about it.
>
> Thanks! It would be interesting to know the reason behind your
> interest in these things. For instance, if you are interested in
> reducing run time memory usage, or general source code duplication
> reduction. Do you have any target platform that you can upstream board
> support for?
>
>>> and the urgency.
>>
>>
>>
>> Regarding the urgency: Someone has accepted the hard coded PRODUCT register
>> (and MODEMR) being in renesas-drivers, now:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177
>>
>> This does really hurts us.
>>
>> Therefore, I try to get this removed as soon as possible to hopefully get a
>> Renesas BSP without this, the next time.
>>
>> If anybody finds an other way to remove that, that would be fine, too.
>
> Please share the underlying issue so we can fix that.
>
>>> So far we have decided to use the compatible string since it is a
>>> common driver model abstraction already used by device drivers and
>>> hardware description in DT. Using DT compat string matching we can
>>> have logic in the drivers to handle ES differences if needed. As for
>>> how to enable the workaround, my opinion is that the missing piece
>>> consists of ES workaround code that appends ES suffix information to
>>> the DT compat strings automatically during boot.
>>
>> Technically this sounds slightly too complicated to me. As I have to
>> advocate my proposal (inspired from an other mainline SoC family) I'd think
>> that my proposal is easier.
>
> It may indeed be easier in the short term, but I feel we need to
> consider how to support a wide range of SoCs in a consistent way.
>
>>> I suppose the workaround is not yet implemented because no one has
>>> deemed this topic as particularly urgent. Until it becomes urgent or a
>>> new ES version appears the affected users can simply adjust the DT
>>> binding themselves. This is what happened to the "sata-r8a7790-es1"
>>> case above.
>
> Can you see any technical reason why using DT compat strings wouldn't
> solve your case?


I'll try to explain my understanding. Please re-ask or correct me if I 
fail ;)


First of all, I can talk only about R-Car3. There, at the moment, we 
have ES1, ES2 and ES3 documented, already. We don't know, yet, if these 
are really needed in the code, but it might be.

Taking the example above, we then would have

compatible = "renesas,sata-r8a7790", "sata-r8a7790-es1", 
"sata-r8a7790-es2", "sata-r8a7790-es3";

in the device tree. Do we want that? No! Because its no run time detection.


If we don't want that, and we want runtime detection, I think anywhere 
in this thread it was mentioned that the "es1" string is appended to 
sata-r8a7790 at runtime? If so, could you point us to the code for that?

Then we have anything like

if(PRODUCT_REG & ES1 == ES1)
	append ES1 to the compatible string

in the boot code. This is one additional indirection to my proposal, 
needing boot time. In the driver you then add

	{
		.compatible = "renesas,sata-r8a7790-es1",
		.data = (void *)RCAR_R8A7790_ES1_SATA
	},
	{
		.compatible = "renesas,sata-r8a7790-es2",
		.data = (void *)RCAR_R8A7790_ES2_SATA
	},
	{
		.compatible = "renesas,sata-r8a7790-es3",
		.data = (void *)RCAR_R8A7790_ES3_SATA
	},

Hmm?

All this just to avoid an (SoC specific) include?


Best regards

Dirk

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Geert Uytterhoeven]

Hi Dirk,

On Wed, Jun 1, 2016 at 9:19 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 01.06.2016 07:19, Magnus Damm wrote:
>> On Fri, May 27, 2016 at 4:56 PM, Dirk Behme <dirk.behme@de.bosch.com>
>> wrote:
>>> On 27.05.2016 05:13, Magnus Damm wrote:
>>>> I don't think anyone disagrees that it makes sense to be able to
>>>> determine ES version during runtime. The questions in my mind are how
>>>> to do it
>>>
>>> I've made a proposal ;) And I'm happy to discuss technically about it.
>>
>> Thanks! It would be interesting to know the reason behind your
>> interest in these things. For instance, if you are interested in
>> reducing run time memory usage, or general source code duplication
>> reduction. Do you have any target platform that you can upstream board
>> support for?
>>
>>>> and the urgency.
>>>
>>> Regarding the urgency: Someone has accepted the hard coded PRODUCT
>>> register
>>> (and MODEMR) being in renesas-drivers, now:
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177
>>>
>>> This does really hurts us.
>>>
>>> Therefore, I try to get this removed as soon as possible to hopefully get
>>> a
>>> Renesas BSP without this, the next time.
>>>
>>> If anybody finds an other way to remove that, that would be fine, too.
>>
>> Please share the underlying issue so we can fix that.
>>
>>>> So far we have decided to use the compatible string since it is a
>>>> common driver model abstraction already used by device drivers and
>>>> hardware description in DT. Using DT compat string matching we can
>>>> have logic in the drivers to handle ES differences if needed. As for
>>>> how to enable the workaround, my opinion is that the missing piece
>>>> consists of ES workaround code that appends ES suffix information to
>>>> the DT compat strings automatically during boot.
>>>
>>> Technically this sounds slightly too complicated to me. As I have to
>>> advocate my proposal (inspired from an other mainline SoC family) I'd
>>> think
>>> that my proposal is easier.
>>
>> It may indeed be easier in the short term, but I feel we need to
>> consider how to support a wide range of SoCs in a consistent way.
>>
>>>> I suppose the workaround is not yet implemented because no one has
>>>> deemed this topic as particularly urgent. Until it becomes urgent or a
>>>> new ES version appears the affected users can simply adjust the DT
>>>> binding themselves. This is what happened to the "sata-r8a7790-es1"
>>>> case above.
>>
>> Can you see any technical reason why using DT compat strings wouldn't
>> solve your case?
>
> I'll try to explain my understanding. Please re-ask or correct me if I fail
> ;)
>
> First of all, I can talk only about R-Car3. There, at the moment, we have
> ES1, ES2 and ES3 documented, already. We don't know, yet, if these are
> really needed in the code, but it might be.
>
> Taking the example above, we then would have
>
> compatible = "renesas,sata-r8a7790", "sata-r8a7790-es1", "sata-r8a7790-es2",
> "sata-r8a7790-es3";
>
> in the device tree. Do we want that? No! Because its no run time detection.

You would only have "renesas,sata-r8a7790" for normal parts.
ES1 needs "renesas,sata-r8a7790-es1".
"esX" compatible values are the exception, not the rule.

> If we don't want that, and we want runtime detection, I think anywhere in
> this thread it was mentioned that the "es1" string is appended to
> sata-r8a7790 at runtime? If so, could you point us to the code for that?

The PoC is on my local SSD. Will send out soon.

> Then we have anything like
>
> if(PRODUCT_REG & ES1 == ES1)
>         append ES1 to the compatible string
>
> in the boot code. This is one additional indirection to my proposal, needing
> boot time. In the driver you then add
>
>         {
>                 .compatible = "renesas,sata-r8a7790-es1",
>                 .data = (void *)RCAR_R8A7790_ES1_SATA
>         },
>         {
>                 .compatible = "renesas,sata-r8a7790-es2",
>                 .data = (void *)RCAR_R8A7790_ES2_SATA
>         },
>         {
>                 .compatible = "renesas,sata-r8a7790-es3",
>                 .data = (void *)RCAR_R8A7790_ES3_SATA
>         },

No, only "renesas,sata-r8a7790" and "renesas,sata-r8a7790-es1", like we
already have.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

Hi Geert,

On 01.06.2016 09:27, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Wed, Jun 1, 2016 at 9:19 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> On 01.06.2016 07:19, Magnus Damm wrote:
>>> On Fri, May 27, 2016 at 4:56 PM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>> On 27.05.2016 05:13, Magnus Damm wrote:
>>>>> I don't think anyone disagrees that it makes sense to be able to
>>>>> determine ES version during runtime. The questions in my mind are how
>>>>> to do it
>>>>
>>>> I've made a proposal ;) And I'm happy to discuss technically about it.
>>>
>>> Thanks! It would be interesting to know the reason behind your
>>> interest in these things. For instance, if you are interested in
>>> reducing run time memory usage, or general source code duplication
>>> reduction. Do you have any target platform that you can upstream board
>>> support for?
>>>
>>>>> and the urgency.
>>>>
>>>> Regarding the urgency: Someone has accepted the hard coded PRODUCT
>>>> register
>>>> (and MODEMR) being in renesas-drivers, now:
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177
>>>>
>>>> This does really hurts us.
>>>>
>>>> Therefore, I try to get this removed as soon as possible to hopefully get
>>>> a
>>>> Renesas BSP without this, the next time.
>>>>
>>>> If anybody finds an other way to remove that, that would be fine, too.
>>>
>>> Please share the underlying issue so we can fix that.
>>>
>>>>> So far we have decided to use the compatible string since it is a
>>>>> common driver model abstraction already used by device drivers and
>>>>> hardware description in DT. Using DT compat string matching we can
>>>>> have logic in the drivers to handle ES differences if needed. As for
>>>>> how to enable the workaround, my opinion is that the missing piece
>>>>> consists of ES workaround code that appends ES suffix information to
>>>>> the DT compat strings automatically during boot.
>>>>
>>>> Technically this sounds slightly too complicated to me. As I have to
>>>> advocate my proposal (inspired from an other mainline SoC family) I'd
>>>> think
>>>> that my proposal is easier.
>>>
>>> It may indeed be easier in the short term, but I feel we need to
>>> consider how to support a wide range of SoCs in a consistent way.
>>>
>>>>> I suppose the workaround is not yet implemented because no one has
>>>>> deemed this topic as particularly urgent. Until it becomes urgent or a
>>>>> new ES version appears the affected users can simply adjust the DT
>>>>> binding themselves. This is what happened to the "sata-r8a7790-es1"
>>>>> case above.
>>>
>>> Can you see any technical reason why using DT compat strings wouldn't
>>> solve your case?
>>
>> I'll try to explain my understanding. Please re-ask or correct me if I fail
>> ;)
>>
>> First of all, I can talk only about R-Car3. There, at the moment, we have
>> ES1, ES2 and ES3 documented, already. We don't know, yet, if these are
>> really needed in the code, but it might be.
>>
>> Taking the example above, we then would have
>>
>> compatible = "renesas,sata-r8a7790", "sata-r8a7790-es1", "sata-r8a7790-es2",
>> "sata-r8a7790-es3";
>>
>> in the device tree. Do we want that? No! Because its no run time detection.
>
> You would only have "renesas,sata-r8a7790" for normal parts.
> ES1 needs "renesas,sata-r8a7790-es1".
> "esX" compatible values are the exception, not the rule.


Yes.


>> If we don't want that, and we want runtime detection, I think anywhere in
>> this thread it was mentioned that the "es1" string is appended to
>> sata-r8a7790 at runtime? If so, could you point us to the code for that?
>
> The PoC is on my local SSD. Will send out soon.
>
>> Then we have anything like
>>
>> if(PRODUCT_REG & ES1 == ES1)
>>         append ES1 to the compatible string
>>
>> in the boot code. This is one additional indirection to my proposal, needing
>> boot time. In the driver you then add
>>
>>         {
>>                 .compatible = "renesas,sata-r8a7790-es1",
>>                 .data = (void *)RCAR_R8A7790_ES1_SATA
>>         },
>>         {
>>                 .compatible = "renesas,sata-r8a7790-es2",
>>                 .data = (void *)RCAR_R8A7790_ES2_SATA
>>         },
>>         {
>>                 .compatible = "renesas,sata-r8a7790-es3",
>>                 .data = (void *)RCAR_R8A7790_ES3_SATA
>>         },
>
> No, only "renesas,sata-r8a7790" and "renesas,sata-r8a7790-es1", like we
> already have.


Sorry, if that was unclear. I took the example and transferred it to 
R-Car3 where we have ES1 - ES3.

So, taking this example, on R-Car3 we might end up with

	{
		.compatible = "renesas,sata-r8a7795",
		.data = (void *)RCAR_GEN2_SATA
	},
	{
		.compatible = "renesas,sata-r8a7795-es1",
		.data = (void *)RCAR_R8A7795_ES1_SATA
	},
	{
		.compatible = "renesas,sata-r8a7795-es2",
		.data = (void *)RCAR_R8A7795_ES2_SATA
	},
	{
		.compatible = "renesas,sata-r8a7795-es3",
		.data = (void *)RCAR_R8A7795_ES3_SATA
	},

?

Best regards

Dirk

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Geert Uytterhoeven]

Hi Dirk,

On Wed, Jun 1, 2016 at 9:38 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Sorry, if that was unclear. I took the example and transferred it to R-Car3
> where we have ES1 - ES3.
>
> So, taking this example, on R-Car3 we might end up with
>
>         {
>                 .compatible = "renesas,sata-r8a7795",
>                 .data = (void *)RCAR_GEN2_SATA
>         },
>         {
>                 .compatible = "renesas,sata-r8a7795-es1",
>                 .data = (void *)RCAR_R8A7795_ES1_SATA
>         },
>         {
>                 .compatible = "renesas,sata-r8a7795-es2",
>                 .data = (void *)RCAR_R8A7795_ES2_SATA
>         },
>         {
>                 .compatible = "renesas,sata-r8a7795-es3",
>                 .data = (void *)RCAR_R8A7795_ES3_SATA
>         },

Are there known differences in the SATA implementations on R-Car H3 ES1, ES2,
and ES3? If not, just "renesas,sata-r8a7795" will do.

BTW having different compatible values in the driver makes it much easier to
audit kernel sources for support for ESx deviations.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

Hi Geert,

On 01.06.2016 10:26, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Wed, Jun 1, 2016 at 9:38 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> Sorry, if that was unclear. I took the example and transferred it to R-Car3
>> where we have ES1 - ES3.
>>
>> So, taking this example, on R-Car3 we might end up with
>>
>>         {
>>                 .compatible = "renesas,sata-r8a7795",
>>                 .data = (void *)RCAR_GEN2_SATA
>>         },
>>         {
>>                 .compatible = "renesas,sata-r8a7795-es1",
>>                 .data = (void *)RCAR_R8A7795_ES1_SATA
>>         },
>>         {
>>                 .compatible = "renesas,sata-r8a7795-es2",
>>                 .data = (void *)RCAR_R8A7795_ES2_SATA
>>         },
>>         {
>>                 .compatible = "renesas,sata-r8a7795-es3",
>>                 .data = (void *)RCAR_R8A7795_ES3_SATA
>>         },
>
> Are there known differences in the SATA implementations on R-Car H3 ES1, ES2,
> and ES3?


It's just an *example* what *might* happen.


> BTW having different compatible values in the driver makes it much easier to
> audit kernel sources for support for ESx deviations.


Why?

Having these compatible values in the driver doesn't tell you if the 
drivers uses them at all. No?

So you have to look into the driver code (not the compatible values) if 
and how the ESx are handled. And there, I don't think it makes any 
difference if you look for

if (priv->type == RCAR_R8A7790_ES1_SATA)
	ap->flags	|= ATA_FLAG_NO_DIPM;

or

if (revision_is_rcar2_es1)
	ap->flags	|= ATA_FLAG_NO_DIPM;

?


Best regards

Dirk

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Geert Uytterhoeven]

Hi Dirk,

On Wed, Jun 1, 2016 at 10:36 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 01.06.2016 10:26, Geert Uytterhoeven wrote:
>> On Wed, Jun 1, 2016 at 9:38 AM, Dirk Behme <dirk.behme@de.bosch.com>
>> wrote:
>>> Sorry, if that was unclear. I took the example and transferred it to
>>> R-Car3
>>> where we have ES1 - ES3.
>>>
>>> So, taking this example, on R-Car3 we might end up with
>>>
>>>         {
>>>                 .compatible = "renesas,sata-r8a7795",
>>>                 .data = (void *)RCAR_GEN2_SATA
>>>         },
>>>         {
>>>                 .compatible = "renesas,sata-r8a7795-es1",
>>>                 .data = (void *)RCAR_R8A7795_ES1_SATA
>>>         },
>>>         {
>>>                 .compatible = "renesas,sata-r8a7795-es2",
>>>                 .data = (void *)RCAR_R8A7795_ES2_SATA
>>>         },
>>>         {
>>>                 .compatible = "renesas,sata-r8a7795-es3",
>>>                 .data = (void *)RCAR_R8A7795_ES3_SATA
>>>         },
>>
>> Are there known differences in the SATA implementations on R-Car H3 ES1,
>> ES2,
>> and ES3?
>
> It's just an *example* what *might* happen.
>
>> BTW having different compatible values in the driver makes it much easier
>> to
>> audit kernel sources for support for ESx deviations.
>
> Why?
>
> Having these compatible values in the driver doesn't tell you if the drivers
> uses them at all. No?

The clue is that we do not add "renesas,*-es%u" compatible values to
drivers, unless ES%u needs to be handled specially.

> So you have to look into the driver code (not the compatible values) if and
> how the ESx are handled. And there, I don't think it makes any difference if
> you look for
>
> if (priv->type == RCAR_R8A7790_ES1_SATA)
>         ap->flags       |= ATA_FLAG_NO_DIPM;
>
> or
>
> if (revision_is_rcar2_es1)
>         ap->flags       |= ATA_FLAG_NO_DIPM;
>
> ?

... so 'git grep "renesas,.*-es[0-9]"' is all you need.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

On 01.06.2016 10:40, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Wed, Jun 1, 2016 at 10:36 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> On 01.06.2016 10:26, Geert Uytterhoeven wrote:
>>> On Wed, Jun 1, 2016 at 9:38 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>> Sorry, if that was unclear. I took the example and transferred it to
>>>> R-Car3
>>>> where we have ES1 - ES3.
>>>>
>>>> So, taking this example, on R-Car3 we might end up with
>>>>
>>>>         {
>>>>                 .compatible = "renesas,sata-r8a7795",
>>>>                 .data = (void *)RCAR_GEN2_SATA
>>>>         },
>>>>         {
>>>>                 .compatible = "renesas,sata-r8a7795-es1",
>>>>                 .data = (void *)RCAR_R8A7795_ES1_SATA
>>>>         },
>>>>         {
>>>>                 .compatible = "renesas,sata-r8a7795-es2",
>>>>                 .data = (void *)RCAR_R8A7795_ES2_SATA
>>>>         },
>>>>         {
>>>>                 .compatible = "renesas,sata-r8a7795-es3",
>>>>                 .data = (void *)RCAR_R8A7795_ES3_SATA
>>>>         },
>>>
>>> Are there known differences in the SATA implementations on R-Car H3 ES1,
>>> ES2,
>>> and ES3?
>>
>> It's just an *example* what *might* happen.
>>
>>> BTW having different compatible values in the driver makes it much easier
>>> to
>>> audit kernel sources for support for ESx deviations.
>>
>> Why?
>>
>> Having these compatible values in the driver doesn't tell you if the drivers
>> uses them at all. No?
>
> The clue is that we do not add "renesas,*-es%u" compatible values to
> drivers, unless ES%u needs to be handled specially.
>
>> So you have to look into the driver code (not the compatible values) if and
>> how the ESx are handled. And there, I don't think it makes any difference if
>> you look for
>>
>> if (priv->type == RCAR_R8A7790_ES1_SATA)
>>         ap->flags       |= ATA_FLAG_NO_DIPM;
>>
>> or
>>
>> if (revision_is_rcar2_es1)
>>         ap->flags       |= ATA_FLAG_NO_DIPM;
>>
>> ?
>
> ... so 'git grep "renesas,.*-es[0-9]"' is all you need.


'git grep "revision_is_rcar2_es[0-9]"'

;)

Re: [PATCH 0/4] ARM: Renesas: R-Car3: Add product register support

[Dirk Behme]

Hi Magnus,

On 01.06.2016 07:19, Magnus Damm wrote:
> Hi Dirk,
>
> On Fri, May 27, 2016 at 4:56 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> On 27.05.2016 05:13, Magnus Damm wrote:
>>> I don't think anyone disagrees that it makes sense to be able to
>>> determine ES version during runtime. The questions in my mind are how
>>> to do it
>>
>>
>>
>> I've made a proposal ;) And I'm happy to discuss technically about it.
>
> Thanks! It would be interesting to know the reason behind your
> interest in these things.


Regarding the general removal of hard coded registers:

This breaks virtualization. If you use a virtualization solution, e.g. 
Xen, which does the memory access rights based on the device tree, each 
peripheral memory access done by the Linux kernel is trapped by the 
hypervisor, then. Because it doesn't know anything about the memory 
range accessed.


Regarding the PRODUCTREG specifically, we already see that it's used for 
engineering sample (ES) detection in some drivers. So not having a 
(device tree) based solution for this, does block all driver development 
(upstreaming) where some ES detection might be needed.


>>> and the urgency.
>>
>>
>>
>> Regarding the urgency: Someone has accepted the hard coded PRODUCT register
>> (and MODEMR) being in renesas-drivers, now:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/drivers/gpu/drm/rcar-du/rcar_du_crtc.c?h=topic/gen3-latest#n177
>>
>> This does really hurts us.
>>
>> Therefore, I try to get this removed as soon as possible to hopefully get a
>> Renesas BSP without this, the next time.
>>
>> If anybody finds an other way to remove that, that would be fine, too.
>
> Please share the underlying issue so we can fix that.


See above.


>>> So far we have decided to use the compatible string since it is a
>>> common driver model abstraction already used by device drivers and
>>> hardware description in DT. Using DT compat string matching we can
>>> have logic in the drivers to handle ES differences if needed. As for
>>> how to enable the workaround, my opinion is that the missing piece
>>> consists of ES workaround code that appends ES suffix information to
>>> the DT compat strings automatically during boot.
>>
>> Technically this sounds slightly too complicated to me. As I have to
>> advocate my proposal (inspired from an other mainline SoC family) I'd think
>> that my proposal is easier.
>
> It may indeed be easier in the short term, but I feel we need to
> consider how to support a wide range of SoCs in a consistent way.
>
>>> I suppose the workaround is not yet implemented because no one has
>>> deemed this topic as particularly urgent. Until it becomes urgent or a
>>> new ES version appears the affected users can simply adjust the DT
>>> binding themselves. This is what happened to the "sata-r8a7790-es1"
>>> case above.
>
> Can you see any technical reason why using DT compat strings wouldn't
> solve your case?


It most probably will work technically, but I see a lot overhead 
regarding boot time and maintenance in the proposed DT compat strings 
solution.

That solution would mean that for each ESx "workaround" we have to do in 
a driver

a) the generic detection code has to be extended to create the driver 
specific *and* ESx specific compatible node

b) the driver has to be extended to contain the new runtime compatible node

c) the driver has to be extended to handle the ESx specific "workaround"

d) the device tree documentation has to be extended for the new compatible

These are the implementation / maintenance steps. Additionally, 
regarding the runtime (boot time does matter!) this does mean that at 
kernel's boot time the ESx information from the hardware register is 
"translated" into a device tree compatible, this is added to the device 
tree at runtime, the device driver has to check the device tree and then 
apply the specific ESx handling.

With my solution, from above list only (c) has to be done. That means 
that you don't have to touch any generic code. And the driver has just 
to call one function.

If you like, compare the mainline i.MX6 way of doing this.


Best regards

Dirk