[PATCH 0/2] clk: renesas: mstp: Add support for RZ/A2

[PATCH 0/2] clk: renesas: mstp: Add support for RZ/A2

[Chris Brandt]

Add support for RZ/A2 series.
The clock HW is similar to RZ/A1, but with different dividers
and additional clocks sources.


Chris Brandt (2):
  clk: renesas: mstp: Add support for r7s9210
  clk: renesas: mstp: Document R7S9210 support

 .../bindings/clock/renesas,cpg-mstp-clocks.txt     |   1 +
 drivers/clk/renesas/Kconfig                        |   5 +
 drivers/clk/renesas/Makefile                       |   1 +
 drivers/clk/renesas/clk-mstp.c                     |   3 +
 drivers/clk/renesas/clk-rz.c                       | 155 ++++++++++++++++-----
 5 files changed, 131 insertions(+), 34 deletions(-)

-- 
2.16.1

[PATCH 1/2] clk: renesas: mstp: Add support for r7s9210

[Chris Brandt]

Add support for RZ/A2 series.
The clock HW is similar to RZ/A1, but with different dividers
and additional clocks sources.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/clk/renesas/Kconfig    |   5 ++
 drivers/clk/renesas/Makefile   |   1 +
 drivers/clk/renesas/clk-mstp.c |   3 +
 drivers/clk/renesas/clk-rz.c   | 155 ++++++++++++++++++++++++++++++++---------
 4 files changed, 130 insertions(+), 34 deletions(-)

diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
index 9022bbe1297e..b08d44b8a476 100644
--- a/drivers/clk/renesas/Kconfig
+++ b/drivers/clk/renesas/Kconfig
@@ -3,6 +3,7 @@ config CLK_RENESAS
 	default y if ARCH_RENESAS
 	select CLK_EMEV2 if ARCH_EMEV2
 	select CLK_RZA1 if ARCH_R7S72100
+	select CLK_RZA2 if ARCH_R7S9210
 	select CLK_R8A73A4 if ARCH_R8A73A4
 	select CLK_R8A7740 if ARCH_R8A7740
 	select CLK_R8A7743 if ARCH_R8A7743
@@ -45,6 +46,10 @@ config CLK_RZA1
 	bool "RZ/A1H clock support" if COMPILE_TEST
 	select CLK_RENESAS_CPG_MSTP
 
+config CLK_RZA2
+	bool "RZ/A2 clock support" if COMPILE_TEST
+	select CLK_RENESAS_CPG_MSTP
+
 config CLK_R8A73A4
 	bool "R-Mobile APE6 clock support" if COMPILE_TEST
 	select CLK_RENESAS_CPG_MSTP
diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefile
index e4aa3d6143d2..6159ee43f7ca 100644
--- a/drivers/clk/renesas/Makefile
+++ b/drivers/clk/renesas/Makefile
@@ -2,6 +2,7 @@
 # SoC
 obj-$(CONFIG_CLK_EMEV2)			+= clk-emev2.o
 obj-$(CONFIG_CLK_RZA1)			+= clk-rz.o
+obj-$(CONFIG_CLK_RZA2)			+= clk-rz.o
 obj-$(CONFIG_CLK_R8A73A4)		+= clk-r8a73a4.o
 obj-$(CONFIG_CLK_R8A7740)		+= clk-r8a7740.o
 obj-$(CONFIG_CLK_R8A7743)		+= r8a7743-cpg-mssr.o
diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index e82adcb16a52..9470ab8acc13 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -213,6 +213,9 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 	if (of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks"))
 		group->width_8bit = true;
 
+	if (of_device_is_compatible(np, "renesas,r7s9210-mstp-clocks"))
+		group->width_8bit = true;
+
 	for (i = 0; i < MSTP_MAX_CLOCKS; ++i)
 		clks[i] = ERR_PTR(-ENOENT);
 
diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c
index ac2f86d626b6..199c6ae9704c 100644
--- a/drivers/clk/renesas/clk-rz.c
+++ b/drivers/clk/renesas/clk-rz.c
@@ -1,5 +1,5 @@
 /*
- * RZ/A1 Core CPG Clocks
+ * RZ/A Core CPG Clocks
  *
  * Copyright (C) 2013 Ideas On Board SPRL
  * Copyright (C) 2014 Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
@@ -24,44 +24,95 @@ struct rz_cpg {
 
 #define CPG_FRQCR	0x10
 #define CPG_FRQCR2	0x14
+#define SWRSTCR3	0xFCFE0468
 
+/* RZ/A1 */
 #define PPR0		0xFCFE3200
 #define PIBC0		0xFCFE7000
 
-#define MD_CLK(x)	((x >> 2) & 1)	/* P0_2 */
+/* RZ/A2 */
+#define PORTL_PIDR	0xFCFFE074
+
+#define RZA1 1
+#define RZA2 2
 
 /* -----------------------------------------------------------------------------
  * Initialization
  */
+int detect_rz(void)
+{
+	void __iomem *swrstcr3;
+	static int rz_device;
+
+	if (!rz_device) {
+		swrstcr3 = ioremap_nocache(SWRSTCR3, 1);
+		BUG_ON(!swrstcr3);
+		if (ioread8(swrstcr3))
+			rz_device = RZA1;
+		else
+			rz_device = RZA2;
+		iounmap(swrstcr3);
+	}
+	return rz_device;
+}
 
-static u16 __init rz_cpg_read_mode_pins(void)
+static u8 __init rz_cpg_read_mode_pin(void)
 {
-	void __iomem *ppr0, *pibc0;
-	u16 modes;
-
-	ppr0 = ioremap_nocache(PPR0, 2);
-	pibc0 = ioremap_nocache(PIBC0, 2);
-	BUG_ON(!ppr0 || !pibc0);
-	iowrite16(4, pibc0);	/* enable input buffer */
-	modes = ioread16(ppr0);
-	iounmap(ppr0);
-	iounmap(pibc0);
-
-	return modes;
+	void __iomem *ppr0, *pibc0, *pidr;
+	u8 mode;
+
+	if (detect_rz() == RZA1) {
+		/* RZ/A1 */
+		/* MD_CLK pin is P0_2 */
+		ppr0 = ioremap_nocache(PPR0, 2);
+		pibc0 = ioremap_nocache(PIBC0, 2);
+		BUG_ON(!ppr0 || !pibc0);
+		iowrite16(4, pibc0);	/* enable input buffer */
+		mode = (u8)((ioread16(ppr0) >> 2) & 1);
+		iounmap(ppr0);
+		iounmap(pibc0);
+	} else {
+		/* RZ/A2 */
+		/* MD_CLK pin is PL_1 */
+		pidr = ioremap_nocache(PORTL_PIDR, 1);
+		BUG_ON(!pidr);
+		mode = (ioread8(pidr) >> 1) & 1;
+		iounmap(pidr);
+	}
+
+	return mode;
 }
 
 static struct clk * __init
 rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *name)
 {
 	u32 val;
-	unsigned mult;
-	static const unsigned frqcr_tab[4] = { 3, 2, 0, 1 };
+	unsigned int mult, div;
+	static const unsigned int rza1_frqcr_tab[4] = { 3, 2, 0, 1 };
+	static const unsigned int rza2_frqcr_tab[5][5] = {
+				/* I,  G,  B, P1, P0 */
+				{  2,  4,  8, 16, 32 },	/* FRQCR = 0x012 */
+				{  4,  4,  8, 16, 32 },	/* FRQCR = 0x112 */
+				{  8,  4,  8, 16, 32 },	/* FRQCR = 0x212 */
+				{ 16,  8, 16, 16, 32 },	/* FRQCR = 0x322 */
+				{ 16, 16, 32, 32, 32 },	/* FRQCR = 0x333 */
+				};
 
-	if (strcmp(name, "pll") == 0) {
-		unsigned int cpg_mode = MD_CLK(rz_cpg_read_mode_pins());
-		const char *parent_name = of_clk_get_parent_name(np, cpg_mode);
+	unsigned int cpg_mode = rz_cpg_read_mode_pin();
 
-		mult = cpg_mode ? (32 / 4) : 30;
+
+	if (strcmp(name, "pll") == 0) {
+		const char *parent_name;
+
+		if (detect_rz() == RZA1) {
+			/* RZ/A1 */
+			parent_name = of_clk_get_parent_name(np, cpg_mode);
+			mult = cpg_mode ? (32 / 4) : 30;
+		} else {
+			/* RZ/A2 */
+			parent_name = of_clk_get_parent_name(np, 0);
+			mult = 88;
+		}
 
 		return clk_register_fixed_factor(NULL, name, parent_name, 0, mult, 1);
 	}
@@ -70,19 +121,55 @@ rz_cpg_register_clock(struct device_node *np, struct rz_cpg *cpg, const char *na
 	if (!cpg->reg)
 		return ERR_PTR(-ENXIO);
 
-	/* FIXME:"i" and "g" are variable clocks with non-integer dividers (e.g. 2/3)
-	 * and the constraint that always g <= i. To get the rz platform started,
-	 * let them run at fixed current speed and implement the details later.
-	 */
-	if (strcmp(name, "i") == 0)
-		val = (readl(cpg->reg + CPG_FRQCR) >> 8) & 3;
-	else if (strcmp(name, "g") == 0)
-		val = readl(cpg->reg + CPG_FRQCR2) & 3;
-	else
-		return ERR_PTR(-EINVAL);
-
-	mult = frqcr_tab[val];
-	return clk_register_fixed_factor(NULL, name, "pll", 0, mult, 3);
+	if (detect_rz() == RZA1) {
+		/* FIXME:"i" and "g" are variable clocks with non-integer dividers (e.g. 2/3)
+		 * and the constraint that always g <= i. To get the rz platform started,
+		 * let them run at fixed current speed and implement the details later.
+		 */
+		if (strcmp(name, "i") == 0)
+			val = (readl(cpg->reg + CPG_FRQCR) >> 8) & 3;
+		else if (strcmp(name, "g") == 0)
+			val = readl(cpg->reg + CPG_FRQCR2) & 3;
+		else
+			return ERR_PTR(-EINVAL);
+		mult = rza1_frqcr_tab[val];
+		div = 3;
+	} else {
+		/* RZ/A2 */
+		val = clk_readl(cpg->reg + CPG_FRQCR) & 0xFFF;
+		if (val == 0x012)
+			val = 0;
+		else if (val == 0x112)
+			val = 1;
+		else if (val == 0x212)
+			val = 2;
+		else if (val == 0x322)
+			val = 3;
+		else if (val == 0x333)
+			val = 4;
+		else
+			BUG_ON(1);	/* Illegal FRQCR value */
+
+		if (strcmp(name, "i") == 0)
+			div = rza2_frqcr_tab[val][0];
+		else if (strcmp(name, "g") == 0)
+			div = rza2_frqcr_tab[val][1];
+		else if (strcmp(name, "b") == 0)
+			div = rza2_frqcr_tab[val][2];
+		else if ((strcmp(name, "p1") == 0) ||
+			 (strcmp(name, "p1c") == 0))
+			div = rza2_frqcr_tab[val][3];
+		else if (strcmp(name, "p0") == 0)
+			div = rza2_frqcr_tab[val][4];
+		else
+			return ERR_PTR(-EINVAL);
+
+		mult = 1;
+		if (cpg_mode)
+			div *= 2;	/* div 2 circuit before PLL */
+
+	}
+	return clk_register_fixed_factor(NULL, name, "pll", 0, mult, div);
 }
 
 static void __init rz_cpg_clocks_init(struct device_node *np)
-- 
2.16.1

[PATCH 2/2] clk: renesas: mstp: Document R7S9210 support

[Chris Brandt]

Renesas R7S9210 SoC also has the CPG MSTP clocks.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
index da578ebdda28..bb470d9bc07f 100644
--- a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
+++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
@@ -11,6 +11,7 @@ Required Properties:
 
   - compatible: Must be one of the following
     - "renesas,r7s72100-mstp-clocks" for R7S72100 (RZ) MSTP gate clocks
+    - "renesas,r7s9210-mstp-clocks" for R7S9210 (RZ A2) MSTP gate clocks
     - "renesas,r8a73a4-mstp-clocks" for R8A73A4 (R-Mobile APE6) MSTP gate clocks
     - "renesas,r8a7740-mstp-clocks" for R8A7740 (R-Mobile A1) MSTP gate clocks
     - "renesas,r8a7778-mstp-clocks" for R8A7778 (R-Car M1) MSTP gate clocks
-- 
2.16.1

Re: [PATCH 2/2] clk: renesas: mstp: Document R7S9210 support

[Geert Uytterhoeven]

On Wed, Jul 11, 2018 at 7:03 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> Renesas R7S9210 SoC also has the CPG MSTP clocks.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 1/2] clk: renesas: mstp: Add support for r7s9210

[Geert Uytterhoeven]

Hi Chris,

Thanks for your patch!

On Wed, Jul 11, 2018 at 7:03 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> [PATCH 1/2] clk: renesas: mstp: Add support for r7s9210

Please drop the "mstp", as the largest share of this patch is not about
MSTP clocks.

> Add support for RZ/A2 series.
> The clock HW is similar to RZ/A1, but with different dividers
> and additional clocks sources.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

>  drivers/clk/renesas/Kconfig    |   5 ++
>  drivers/clk/renesas/Makefile   |   1 +
>  drivers/clk/renesas/clk-mstp.c |   3 +
>  drivers/clk/renesas/clk-rz.c   | 155 ++++++++++++++++++++++++++++++++---------

You're adding ca. 100 new lines to an existing driver of 126 lines, most of
which are depending on the result of detect_rz()?
So I think you're best of adding a complete new driver clk-rza2.c, matching
against "renesas,r7s9210-cpg-clocks".
The "renesas,rz-cpg-clocks" won't be needed for RZ/A2.
And perhaps rename clk-rz.c to clk-rza1.c, and change its match string to
"renesas,r7s72100-cpg-clocks"?

BTW, please use fcfe0020 as the base address for the CPG (which requires
changing the register offsets in the driver), to avoid the warning we're
seeing with "make dtbs W=1" for RZ/A1:

    Warning (unique_unit_address): /soc/watchdog@fcfe0000: duplicate
unit-address (also used in node /soc/cpg_clocks@fcfe0000)

BTW2, I guess I can't convince you to write a modern new clock driver using
a single register block, describing all core and module clocks in C tables?
That would avoid making mistakes in keeping the clocks/clock-indices/
clock-output-names properties in the mstp clock nodes in DT in sync.
It would also make your life easier if you ever decide to support software
reset using the Software Reset Control Register in the same register
block.

> --- a/drivers/clk/renesas/clk-mstp.c
> +++ b/drivers/clk/renesas/clk-mstp.c
> @@ -213,6 +213,9 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
>         if (of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks"))
>                 group->width_8bit = true;
>
> +       if (of_device_is_compatible(np, "renesas,r7s9210-mstp-clocks"))

You can merge the test with the test for RZ/A1 above.

> +               group->width_8bit = true;
> +
>         for (i = 0; i < MSTP_MAX_CLOCKS; ++i)
>                 clks[i] = ERR_PTR(-ENOENT);
>
> diff --git a/drivers/clk/renesas/clk-rz.c b/drivers/clk/renesas/clk-rz.c
> index ac2f86d626b6..199c6ae9704c 100644
> --- a/drivers/clk/renesas/clk-rz.c
> +++ b/drivers/clk/renesas/clk-rz.c
> @@ -24,44 +24,95 @@ struct rz_cpg {
>
>  #define CPG_FRQCR      0x10
>  #define CPG_FRQCR2     0x14
> +#define SWRSTCR3       0xFCFE0468
>
> +/* RZ/A1 */
>  #define PPR0           0xFCFE3200
>  #define PIBC0          0xFCFE7000
>
> -#define MD_CLK(x)      ((x >> 2) & 1)  /* P0_2 */
> +/* RZ/A2 */
> +#define PORTL_PIDR     0xFCFFE074
> +
> +#define RZA1 1
> +#define RZA2 2
>
>  /* -----------------------------------------------------------------------------
>   * Initialization
>   */
> +int detect_rz(void)
> +{
> +       void __iomem *swrstcr3;
> +       static int rz_device;
> +
> +       if (!rz_device) {
> +               swrstcr3 = ioremap_nocache(SWRSTCR3, 1);
> +               BUG_ON(!swrstcr3);
> +               if (ioread8(swrstcr3))
> +                       rz_device = RZA1;
> +               else
> +                       rz_device = RZA2;
> +               iounmap(swrstcr3);
> +       }
> +       return rz_device;
> +}

Please use the compatible value for differentiating (issue is moot with a
separate driver).

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 1/2] clk: renesas: mstp: Add support for r7s9210

[Chris Brandt]

Hi Geert,

As always, thank you for your review!


On Friday, July 13, 2018, Geert Uytterhoeven wrote:
> Please drop the "mstp", as the largest share of this patch is not about
> MSTP clocks.

OK.

> >  drivers/clk/renesas/clk-rz.c   | 155 ++++++++++++++++++++++++++++++++--
> -------
> 
> You're adding ca. 100 new lines to an existing driver of 126 lines, most
> of
> which are depending on the result of detect_rz()?
> So I think you're best of adding a complete new driver clk-rza2.c,
> matching
> against "renesas,r7s9210-cpg-clocks".
> The "renesas,rz-cpg-clocks" won't be needed for RZ/A2.
> And perhaps rename clk-rz.c to clk-rza1.c, and change its match string to
> "renesas,r7s72100-cpg-clocks"?

OK. I was just trying to reduce the number of files, but I guess it's 
not that big of a deal.


> BTW, please use fcfe0020 as the base address for the CPG (which requires
> changing the register offsets in the driver), to avoid the warning we're
> seeing with "make dtbs W=1" for RZ/A1:
> 
>     Warning (unique_unit_address): /soc/watchdog@fcfe0000: duplicate
> unit-address (also used in node /soc/cpg_clocks@fcfe0000)
> 
> BTW2, I guess I can't convince you to write a modern new clock driver
> using
> a single register block, describing all core and module clocks in C
> tables?
> That would avoid making mistakes in keeping the clocks/clock-indices/
> clock-output-names properties in the mstp clock nodes in DT in sync.
> It would also make your life easier if you ever decide to support software
> reset using the Software Reset Control Register in the same register
> block.

I'll have a look before I go back and make all of these changes. 
Honestly, I spent the most time on trying to figure out how to detect what RZ I
was running one (which goes away once I have separate .c files) and 
also how to turn the new dividers in the HW Manual into code that would 
actual work correctly.
Meaning, if I had known better, I might have started with the new method
from the start.

Also, I don't really want to switch later to the new method and then 
change all my Device Trees, so if I'm going to use it, I better start now.

I've got a nice long 14 hour plane ride this weekend to Japan, so I will
have a look and try to understand how I can make the switch to the new 
method. I might come back with questions later. I assume like last time,
I'll have to add support for 8-bit MSTP registers, and also my way of 
waiting for the clock bit to be set since RZ/A does not have the clock 
status bits like R-Car.


> > --- a/drivers/clk/renesas/clk-mstp.c
> > +++ b/drivers/clk/renesas/clk-mstp.c
> > @@ -213,6 +213,9 @@ static void __init cpg_mstp_clocks_init(struct
> device_node *np)
> >         if (of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks"))
> >                 group->width_8bit = true;
> >
> > +       if (of_device_is_compatible(np, "renesas,r7s9210-mstp-clocks"))
> 
> You can merge the test with the test for RZ/A1 above.

OK.


> > +int detect_rz(void)
> > +{
> > +       void __iomem *swrstcr3;
> > +       static int rz_device;
> > +
> > +       if (!rz_device) {
> > +               swrstcr3 = ioremap_nocache(SWRSTCR3, 1);
> > +               BUG_ON(!swrstcr3);
> > +               if (ioread8(swrstcr3))
> > +                       rz_device = RZA1;
> > +               else
> > +                       rz_device = RZA2;
> > +               iounmap(swrstcr3);
> > +       }
> > +       return rz_device;
> > +}
> 
> Please use the compatible value for differentiating (issue is moot with a
> separate driver).

I agree.


Thanks,
Chris

RE: [PATCH 1/2] clk: renesas: mstp: Add support for r7s9210

[Chris Brandt]

Hi Geert,

As always, thank you for your review!


On Friday, July 13, 2018, Geert Uytterhoeven wrote:
> Please drop the "mstp", as the largest share of this patch is not about
> MSTP clocks.

OK.

> >  drivers/clk/renesas/clk-rz.c   | 155 ++++++++++++++++++++++++++++++++--
> -------
> 
> You're adding ca. 100 new lines to an existing driver of 126 lines, most
> of
> which are depending on the result of detect_rz()?
> So I think you're best of adding a complete new driver clk-rza2.c,
> matching
> against "renesas,r7s9210-cpg-clocks".
> The "renesas,rz-cpg-clocks" won't be needed for RZ/A2.
> And perhaps rename clk-rz.c to clk-rza1.c, and change its match string to
> "renesas,r7s72100-cpg-clocks"?

OK. I was just trying to reduce the number of files, but I guess it's 
not that big of a deal.


> BTW, please use fcfe0020 as the base address for the CPG (which requires
> changing the register offsets in the driver), to avoid the warning we're
> seeing with "make dtbs W=1" for RZ/A1:
> 
>     Warning (unique_unit_address): /soc/watchdog@fcfe0000: duplicate
> unit-address (also used in node /soc/cpg_clocks@fcfe0000)
> 
> BTW2, I guess I can't convince you to write a modern new clock driver
> using
> a single register block, describing all core and module clocks in C
> tables?
> That would avoid making mistakes in keeping the clocks/clock-indices/
> clock-output-names properties in the mstp clock nodes in DT in sync.
> It would also make your life easier if you ever decide to support software
> reset using the Software Reset Control Register in the same register
> block.

I'll have a look before I go back and make all of these changes. 
Honestly, I spent the most time on trying to figure out how to detect what RZ I
was running one (which goes away once I have separate .c files) and 
also how to turn the new dividers in the HW Manual into code that would 
actual work correctly.
Meaning, if I had known better, I might have started with the new method
from the start.

Also, I don't really want to switch later to the new method and then 
change all my Device Trees, so if I'm going to use it, I better start now.

I've got a nice long 14 hour plane ride this weekend to Japan, so I will
have a look and try to understand how I can make the switch to the new 
method. I might come back with questions later. I assume like last time,
I'll have to add support for 8-bit MSTP registers, and also my way of 
waiting for the clock bit to be set since RZ/A does not have the clock 
status bits like R-Car.


> > --- a/drivers/clk/renesas/clk-mstp.c
> > +++ b/drivers/clk/renesas/clk-mstp.c
> > @@ -213,6 +213,9 @@ static void __init cpg_mstp_clocks_init(struct
> device_node *np)
> >         if (of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks"))
> >                 group->width_8bit = true;
> >
> > +       if (of_device_is_compatible(np, "renesas,r7s9210-mstp-clocks"))
> 
> You can merge the test with the test for RZ/A1 above.

OK.


> > +int detect_rz(void)
> > +{
> > +       void __iomem *swrstcr3;
> > +       static int rz_device;
> > +
> > +       if (!rz_device) {
> > +               swrstcr3 = ioremap_nocache(SWRSTCR3, 1);
> > +               BUG_ON(!swrstcr3);
> > +               if (ioread8(swrstcr3))
> > +                       rz_device = RZA1;
> > +               else
> > +                       rz_device = RZA2;
> > +               iounmap(swrstcr3);
> > +       }
> > +       return rz_device;
> > +}
> 
> Please use the compatible value for differentiating (issue is moot with a
> separate driver).

I agree.


Thanks,
Chris

RE: [PATCH 1/2] clk: renesas: mstp: Add support for r7s9210

[Chris Brandt]

SGkgR2VlcnQsDQoNCkFzIGFsd2F5cywgdGhhbmsgeW91IGZvciB5b3VyIHJldmlldyENCg0KDQpP
biBGcmlkYXksIEp1bHkgMTMsIDIwMTgsIEdlZXJ0IFV5dHRlcmhvZXZlbiB3cm90ZToNCj4gUGxl
YXNlIGRyb3AgdGhlICJtc3RwIiwgYXMgdGhlIGxhcmdlc3Qgc2hhcmUgb2YgdGhpcyBwYXRjaCBp
cyBub3QgYWJvdXQNCj4gTVNUUCBjbG9ja3MuDQoNCk9LLg0KDQo+ID4gIGRyaXZlcnMvY2xrL3Jl
bmVzYXMvY2xrLXJ6LmMgICB8IDE1NSArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0t
DQo+IC0tLS0tLS0NCj4gDQo+IFlvdSdyZSBhZGRpbmcgY2EuIDEwMCBuZXcgbGluZXMgdG8gYW4g
ZXhpc3RpbmcgZHJpdmVyIG9mIDEyNiBsaW5lcywgbW9zdA0KPiBvZg0KPiB3aGljaCBhcmUgZGVw
ZW5kaW5nIG9uIHRoZSByZXN1bHQgb2YgZGV0ZWN0X3J6KCk/DQo+IFNvIEkgdGhpbmsgeW91J3Jl
IGJlc3Qgb2YgYWRkaW5nIGEgY29tcGxldGUgbmV3IGRyaXZlciBjbGstcnphMi5jLA0KPiBtYXRj
aGluZw0KPiBhZ2FpbnN0ICJyZW5lc2FzLHI3czkyMTAtY3BnLWNsb2NrcyIuDQo+IFRoZSAicmVu
ZXNhcyxyei1jcGctY2xvY2tzIiB3b24ndCBiZSBuZWVkZWQgZm9yIFJaL0EyLg0KPiBBbmQgcGVy
aGFwcyByZW5hbWUgY2xrLXJ6LmMgdG8gY2xrLXJ6YTEuYywgYW5kIGNoYW5nZSBpdHMgbWF0Y2gg
c3RyaW5nIHRvDQo+ICJyZW5lc2FzLHI3czcyMTAwLWNwZy1jbG9ja3MiPw0KDQpPSy4gSSB3YXMg
anVzdCB0cnlpbmcgdG8gcmVkdWNlIHRoZSBudW1iZXIgb2YgZmlsZXMsIGJ1dCBJIGd1ZXNzIGl0
J3MgDQpub3QgdGhhdCBiaWcgb2YgYSBkZWFsLg0KDQoNCj4gQlRXLCBwbGVhc2UgdXNlIGZjZmUw
MDIwIGFzIHRoZSBiYXNlIGFkZHJlc3MgZm9yIHRoZSBDUEcgKHdoaWNoIHJlcXVpcmVzDQo+IGNo
YW5naW5nIHRoZSByZWdpc3RlciBvZmZzZXRzIGluIHRoZSBkcml2ZXIpLCB0byBhdm9pZCB0aGUg
d2FybmluZyB3ZSdyZQ0KPiBzZWVpbmcgd2l0aCAibWFrZSBkdGJzIFc9MSIgZm9yIFJaL0ExOg0K
PiANCj4gICAgIFdhcm5pbmcgKHVuaXF1ZV91bml0X2FkZHJlc3MpOiAvc29jL3dhdGNoZG9nQGZj
ZmUwMDAwOiBkdXBsaWNhdGUNCj4gdW5pdC1hZGRyZXNzIChhbHNvIHVzZWQgaW4gbm9kZSAvc29j
L2NwZ19jbG9ja3NAZmNmZTAwMDApDQo+IA0KPiBCVFcyLCBJIGd1ZXNzIEkgY2FuJ3QgY29udmlu
Y2UgeW91IHRvIHdyaXRlIGEgbW9kZXJuIG5ldyBjbG9jayBkcml2ZXINCj4gdXNpbmcNCj4gYSBz
aW5nbGUgcmVnaXN0ZXIgYmxvY2ssIGRlc2NyaWJpbmcgYWxsIGNvcmUgYW5kIG1vZHVsZSBjbG9j
a3MgaW4gQw0KPiB0YWJsZXM/DQo+IFRoYXQgd291bGQgYXZvaWQgbWFraW5nIG1pc3Rha2VzIGlu
IGtlZXBpbmcgdGhlIGNsb2Nrcy9jbG9jay1pbmRpY2VzLw0KPiBjbG9jay1vdXRwdXQtbmFtZXMg
cHJvcGVydGllcyBpbiB0aGUgbXN0cCBjbG9jayBub2RlcyBpbiBEVCBpbiBzeW5jLg0KPiBJdCB3
b3VsZCBhbHNvIG1ha2UgeW91ciBsaWZlIGVhc2llciBpZiB5b3UgZXZlciBkZWNpZGUgdG8gc3Vw
cG9ydCBzb2Z0d2FyZQ0KPiByZXNldCB1c2luZyB0aGUgU29mdHdhcmUgUmVzZXQgQ29udHJvbCBS
ZWdpc3RlciBpbiB0aGUgc2FtZSByZWdpc3Rlcg0KPiBibG9jay4NCg0KSSdsbCBoYXZlIGEgbG9v
ayBiZWZvcmUgSSBnbyBiYWNrIGFuZCBtYWtlIGFsbCBvZiB0aGVzZSBjaGFuZ2VzLiANCkhvbmVz
dGx5LCBJIHNwZW50IHRoZSBtb3N0IHRpbWUgb24gdHJ5aW5nIHRvIGZpZ3VyZSBvdXQgaG93IHRv
IGRldGVjdCB3aGF0IFJaIEkNCndhcyBydW5uaW5nIG9uZSAod2hpY2ggZ29lcyBhd2F5IG9uY2Ug
SSBoYXZlIHNlcGFyYXRlIC5jIGZpbGVzKSBhbmQgDQphbHNvIGhvdyB0byB0dXJuIHRoZSBuZXcg
ZGl2aWRlcnMgaW4gdGhlIEhXIE1hbnVhbCBpbnRvIGNvZGUgdGhhdCB3b3VsZCANCmFjdHVhbCB3
b3JrIGNvcnJlY3RseS4NCk1lYW5pbmcsIGlmIEkgaGFkIGtub3duIGJldHRlciwgSSBtaWdodCBo
YXZlIHN0YXJ0ZWQgd2l0aCB0aGUgbmV3IG1ldGhvZA0KZnJvbSB0aGUgc3RhcnQuDQoNCkFsc28s
IEkgZG9uJ3QgcmVhbGx5IHdhbnQgdG8gc3dpdGNoIGxhdGVyIHRvIHRoZSBuZXcgbWV0aG9kIGFu
ZCB0aGVuIA0KY2hhbmdlIGFsbCBteSBEZXZpY2UgVHJlZXMsIHNvIGlmIEknbSBnb2luZyB0byB1
c2UgaXQsIEkgYmV0dGVyIHN0YXJ0IG5vdy4NCg0KSSd2ZSBnb3QgYSBuaWNlIGxvbmcgMTQgaG91
ciBwbGFuZSByaWRlIHRoaXMgd2Vla2VuZCB0byBKYXBhbiwgc28gSSB3aWxsDQpoYXZlIGEgbG9v
ayBhbmQgdHJ5IHRvIHVuZGVyc3RhbmQgaG93IEkgY2FuIG1ha2UgdGhlIHN3aXRjaCB0byB0aGUg
bmV3IA0KbWV0aG9kLiBJIG1pZ2h0IGNvbWUgYmFjayB3aXRoIHF1ZXN0aW9ucyBsYXRlci4gSSBh
c3N1bWUgbGlrZSBsYXN0IHRpbWUsDQpJJ2xsIGhhdmUgdG8gYWRkIHN1cHBvcnQgZm9yIDgtYml0
IE1TVFAgcmVnaXN0ZXJzLCBhbmQgYWxzbyBteSB3YXkgb2YgDQp3YWl0aW5nIGZvciB0aGUgY2xv
Y2sgYml0IHRvIGJlIHNldCBzaW5jZSBSWi9BIGRvZXMgbm90IGhhdmUgdGhlIGNsb2NrIA0Kc3Rh
dHVzIGJpdHMgbGlrZSBSLUNhci4NCg0KDQo+ID4gLS0tIGEvZHJpdmVycy9jbGsvcmVuZXNhcy9j
bGstbXN0cC5jDQo+ID4gKysrIGIvZHJpdmVycy9jbGsvcmVuZXNhcy9jbGstbXN0cC5jDQo+ID4g
QEAgLTIxMyw2ICsyMTMsOSBAQCBzdGF0aWMgdm9pZCBfX2luaXQgY3BnX21zdHBfY2xvY2tzX2lu
aXQoc3RydWN0DQo+IGRldmljZV9ub2RlICpucCkNCj4gPiAgICAgICAgIGlmIChvZl9kZXZpY2Vf
aXNfY29tcGF0aWJsZShucCwgInJlbmVzYXMscjdzNzIxMDAtbXN0cC1jbG9ja3MiKSkNCj4gPiAg
ICAgICAgICAgICAgICAgZ3JvdXAtPndpZHRoXzhiaXQgPSB0cnVlOw0KPiA+DQo+ID4gKyAgICAg
ICBpZiAob2ZfZGV2aWNlX2lzX2NvbXBhdGlibGUobnAsICJyZW5lc2FzLHI3czkyMTAtbXN0cC1j
bG9ja3MiKSkNCj4gDQo+IFlvdSBjYW4gbWVyZ2UgdGhlIHRlc3Qgd2l0aCB0aGUgdGVzdCBmb3Ig
UlovQTEgYWJvdmUuDQoNCk9LLg0KDQoNCj4gPiAraW50IGRldGVjdF9yeih2b2lkKQ0KPiA+ICt7
DQo+ID4gKyAgICAgICB2b2lkIF9faW9tZW0gKnN3cnN0Y3IzOw0KPiA+ICsgICAgICAgc3RhdGlj
IGludCByel9kZXZpY2U7DQo+ID4gKw0KPiA+ICsgICAgICAgaWYgKCFyel9kZXZpY2UpIHsNCj4g
PiArICAgICAgICAgICAgICAgc3dyc3RjcjMgPSBpb3JlbWFwX25vY2FjaGUoU1dSU1RDUjMsIDEp
Ow0KPiA+ICsgICAgICAgICAgICAgICBCVUdfT04oIXN3cnN0Y3IzKTsNCj4gPiArICAgICAgICAg
ICAgICAgaWYgKGlvcmVhZDgoc3dyc3RjcjMpKQ0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAg
IHJ6X2RldmljZSA9IFJaQTE7DQo+ID4gKyAgICAgICAgICAgICAgIGVsc2UNCj4gPiArICAgICAg
ICAgICAgICAgICAgICAgICByel9kZXZpY2UgPSBSWkEyOw0KPiA+ICsgICAgICAgICAgICAgICBp
b3VubWFwKHN3cnN0Y3IzKTsNCj4gPiArICAgICAgIH0NCj4gPiArICAgICAgIHJldHVybiByel9k
ZXZpY2U7DQo+ID4gK30NCj4gDQo+IFBsZWFzZSB1c2UgdGhlIGNvbXBhdGlibGUgdmFsdWUgZm9y
IGRpZmZlcmVudGlhdGluZyAoaXNzdWUgaXMgbW9vdCB3aXRoIGENCj4gc2VwYXJhdGUgZHJpdmVy
KS4NCg0KSSBhZ3JlZS4NCg0KDQpUaGFua3MsDQpDaHJpcw0KDQo=

Re: [PATCH 0/2] clk: renesas: mstp: Add support for RZ/A2

[Geert Uytterhoeven]

Hi Chris,

(Oops, I wrote this response, but forgot to send it)

On Wed, Jul 11, 2018 at 7:03 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> Add support for RZ/A2 series.
> The clock HW is similar to RZ/A1, but with different dividers
> and additional clocks sources.

Thanks for your series!

> Chris Brandt (2):
>   clk: renesas: mstp: Add support for r7s9210
>   clk: renesas: mstp: Document R7S9210 support
>
>  .../bindings/clock/renesas,cpg-mstp-clocks.txt     |   1 +
>  drivers/clk/renesas/Kconfig                        |   5 +
>  drivers/clk/renesas/Makefile                       |   1 +
>  drivers/clk/renesas/clk-mstp.c                     |   3 +
>  drivers/clk/renesas/clk-rz.c                       | 155 ++++++++++++++++-----
>  5 files changed, 131 insertions(+), 34 deletions(-)

No patch to update
Documentation/devicetree/bindings/clock/renesas,rz-cpg-clocks.txt?

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 2/2] clk: renesas: mstp: Document R7S9210 support

[Rob Herring]

On Wed, Jul 11, 2018 at 12:03:13PM -0500, Chris Brandt wrote:
> Renesas R7S9210 SoC also has the CPG MSTP clocks.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/2] clk: renesas: mstp: Document R7S9210 support

[Geert Uytterhoeven]

Hi Chris,

On Wed, Jul 11, 2018 at 7:03 PM Chris Brandt <chris.brandt@renesas.com> wrote:
>
> Renesas R7S9210 SoC also has the CPG MSTP clocks.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

FTR, I consider this superseded by "[PATCH v3] clk: renesas: cpg-mssr: Add
R7S9210 support" (https://www.spinics.net/lists/linux-clk/msg29669.html).

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