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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux