Hi Geert,

Thanks for your review.


On Thursday, September 06, 2018, Geert Uytterhoeven wrote:

quoted

+#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.

quoted

+
+#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.

quoted

+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.

quoted

+                       {  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.

quoted

+       /* 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.

quoted

+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.

quoted

+       /* 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.

quoted

+       /* 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?

quoted

+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.

quoted

+       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.

quoted

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

MOD_CLK_PACK()

Good find! I would have broken existing drivers!

quoted

+#ifdef CONFIG_CLK_R7S9210
+       {
+               .compatible = "renesas,r7s9210-cpg-mssr",
+               .data = &r7s9210_cpg_mssr_info,
+       },

Please preserve sort order.

quoted

 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

quoted

+/* 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.

quoted

+#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