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.

quoted

 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.

quoted

--- 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)

quoted

        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.

quoted

+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