Message ID | 20180711170313.80321-1-chris.brandt@renesas.com |
---|---|
Headers | show |
Series | clk: renesas: mstp: Add support for RZ/A2 | expand |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html