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
