RE: [PATCH v3] clk: renesas: cpg-mssr: Add R7S9210 support

From: Chris Brandt <Chris.Brandt@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-clk <linux-clk@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS 
	<devicetree@vger.kernel.org>,
	Linux-Renesas" <linux-renesas-soc@vger.kernel.org>,
	Simon Horman <horms+renesas@verge.net.au>
Subject: RE: [PATCH v3] clk: renesas: cpg-mssr: Add R7S9210 support
Date: Thu, 6 Sep 2018 14:31:34 +0000	[thread overview]
Message-ID: <TY1PR01MB15620B77FCA81E17753508058A010@TY1PR01MB1562.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdUSVTk0FPmQSYJ+M1e4ds__7882Viseo6ra1TUBvnSbPQ@mail.gmail.com>

Hi Geert,

Thanks for your review.

On Thursday, September 06, 2018, Geert Uytterhoeven wrote:
> > +#define CPG_FRQCR      0x00
> > +#define CPG_CKIOSEL    0xF0
> > +#define CPG_SCLKSEL    0xF4
> 
> The last two are unused?

In this driver they are not. I can remove them.

> > +
> > +#define PORTL_PIDR     0xFCFFE074
> 
> Unused?

Oops. That was left over from when I first was reading the port pin to 
find out the mode. But then I realize I could get the info from DT.

> 
> > +static u8 cpg_mode;
> > +
> > +/* Internal Clock ratio table */
> > +static const unsigned int ratio_tab[5][5] = {
> > +                       /* I,  G,  B, P1, P0 */
> 
> Use a struct instead?
> 
> struct {
>         unsigned int i;
>         unsigned int g;
>         unsigned int ib
>         unsigned int p1;
>         unsigned int p0;
> } ratio_tab[5] = { ... }

That's a good idea. Thanks.

> 
> > +                       {  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 */
> 
> The P0 divider is fixed to 32, so you can remove it from the table?

OK, I can do that. I was just doing it to be consistent.

> > +       /* Internal Core Clocks */
> > +       CLK_MAIN,
> > +       CLK_PLL,
> > +       CLK_I,
> > +       CLK_G,
> > +       CLK_B,
> > +       CLK_P1,
> > +       CLK_P1C,
> > +       CLK_P0,
> 
> The last six are not used and can be removed
> (the driver uses R7S9210_CLK_* instead).

OK.

> > +static const struct mssr_mod_clk r7s9210_mod_clks[] __initconst = {
> > +       DEF_MOD_STB("ostm0",     36,    R7S9210_CLK_P1C),
> > +       DEF_MOD_STB("ostm1",     35,    R7S9210_CLK_P1C),
> > +       DEF_MOD_STB("ostm2",     34,    R7S9210_CLK_P1C),
> 
> I think the table is easier to read if you sort by MSTP number.

OK. I will switch them all around.

> > +       /* Adjust the dividers based on the current FRQCR setting */
> > +       if (core->id == CLK_MAIN) {
> > +
> > +               /* If EXTAL is above 12MHz, then we know it is Mode 1 */
> > +               if (clk_get_rate((struct clk *)parent) > 12000000)
> 
> Why the cast?

I was getting compiler warning because parent was const.

	const struct clk *parent;

../drivers/clk/renesas/r7s9210-cpg-mssr.c:144:20: warning: passing argument 1 of ‘clk_get_rate’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   if (clk_get_rate(parent) > 12000000)
                    ^
In file included from ../drivers/clk/renesas/r7s9210-cpg-mssr.c:12:0:
../include/linux/clk.h:463:15: note: expected ‘struct clk *’ but argument is of type ‘const struct clk *’
 unsigned long clk_get_rate(struct clk *clk);
               ^
make[1]: Leaving directory '/home/renesas/tools/upstream/renesas-drivers/.out_rza2m'

I can remove const, then I don't need the cast anymore.

> > +       /* Module Clocks */
> > +       .mod_clks = r7s9210_mod_clks,
> > +       .num_mod_clks = ARRAY_SIZE(r7s9210_mod_clks),
> > +       .num_hw_mod_clks = 11 * 32, /* includes STBCR0/1/2 which don't
> exist */
> 
> According to the HW manual, STBCR1/2 do not exist?

The problem is that there are registers named "STBCR1" and "STBCR2", but
they are not pure MSTP registers, and they sit at a different address 
location.

Would it be better if I say the MSTP registers start at STBCR3, and just
subtract "30" from the numbers in the device tree?

> > +static const u16 stbcr[] = {
> > +       0x000, 0x000, 0x014, 0x410, 0x414, 0x418, 0x41C, 0x420,
> 
> stbcr[1] should be 0x010?

Technically, stbcr[0] = 0x10, stbcr[1] = 0x14,
I was just thinking that I would never be access it anyway since they 
are not really MSTP registers.

> 
> > +       0x424, 0x428, 0x42C, 0x430, 0x434, 0x460,
> 
> The last 3 don't exist?

Opps, you're right! They are left over from when I change the table around.
Thanks.

> > +               } else {
> > +                       idx = MOD_CLK_PACK_10(clkidx);
> 
> MOD_CLK_PACK()

Good find! I would have broken existing drivers!

> > +#ifdef CONFIG_CLK_R7S9210
> > +       {
> > +               .compatible = "renesas,r7s9210-cpg-mssr",
> > +               .data = &r7s9210_cpg_mssr_info,
> > +       },
> 
> Please preserve sort order.

> >  extern const struct cpg_mssr_info r8a77980_cpg_mssr_info;
> >  extern const struct cpg_mssr_info r8a77990_cpg_mssr_info;
> >  extern const struct cpg_mssr_info r8a77995_cpg_mssr_info;
> > +extern const struct cpg_mssr_info r7s9210_cpg_mssr_info;
> 
> Please preserve sort order.

OK

> > +/* R7S9210 CPG Core Clocks */
> > +#define R7S9210_CLK_PLL                        0
> 
> Should that be an internal clock, not referred to from DT?
> There's already the internal CLK_PLL clock.

OK, I see what you mean. I will leave it out of this header file.

> > +#define R7S9210_CLK_I                  1
> > +#define R7S9210_CLK_G                  2
> > +#define R7S9210_CLK_B                  3
> > +#define R7S9210_CLK_P1                 4
> > +#define R7S9210_CLK_P1C                        5
> > +#define R7S9210_CLK_P0                 6
> 
> The comment in Figure 6.1 suggests there's also P0C, but that may be a
> mistake, as I can find no other references to it?

Funny, I didn't see that in there before.
Like you said, it's probably just a mistake.
I'll point that out to the device team.

> What about other clocks: BSCCLK, OCTCLK, HYPCLK, and SPICLK?

At this time, I'm considering them 'out of scope' from this driver as 
they really need to be set up early in device boot (ie, u-boot).
Unless you were just thinking of adding them as #defines, but never 
really using them.

Chris