Hi Chris,

On Wed, Aug 29, 2018 at 3:29 PM Chris Brandt [off-list ref] wrote:

Add support for the R7S9210 (RZ/A2) Clock Pulse Generator and Module
Standby.

Thanks for your patch!

The Module Standby HW in the RZ/A series is very close to R-Car HW, except
for how the registers are laid out.
The MSTP registers are only 8-bits wide, there is no status registers
(MSTPST), and the register offsets are a little different. Since the RZ/A

MSTPSR

hardware manuals refer to these registers as the Standby Control Registers,
we'll use that name to distinguish the RZ/A type for the R-Car type.

from the R-Car type.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

The last two are unused?

+
+#define PORTL_PIDR     0xFCFFE074

Unused?

+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] = { ... }

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

+                       };
+
+enum rz_clk_types {
+       CLK_TYPE_RZA_MAIN = CLK_TYPE_CUSTOM,
+       CLK_TYPE_RZA_PLL,
+};
+
+enum clk_ids {
+       /* Core Clock Outputs exported to DT */
+       LAST_DT_CORE_CLK = R7S9210_CLK_P0,
+
+       /* External Input Clocks */
+       CLK_EXTAL,
+
+       /* 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).

+
+       /* Module Clocks */
+       MOD_CLK_BASE
+};
+
+static struct cpg_core_clk r7s9210_core_clks[] = {
+       /* External Clock Inputs */
+       DEF_INPUT("extal",     CLK_EXTAL),
+
+       /* Internal Core Clocks */
+       DEF_BASE(".main",       CLK_MAIN, CLK_TYPE_RZA_MAIN, CLK_EXTAL),
+       DEF_BASE(".pll",       CLK_PLL, CLK_TYPE_RZA_PLL, CLK_MAIN),
+
+       /* Core Clock Outputs */
+       DEF_FIXED("i",      R7S9210_CLK_I,     CLK_PLL,          2, 1),
+       DEF_FIXED("g",      R7S9210_CLK_G,     CLK_PLL,          4, 1),
+       DEF_FIXED("b",      R7S9210_CLK_B,     CLK_PLL,          8, 1),
+       DEF_FIXED("p1",     R7S9210_CLK_P1,    CLK_PLL,         16, 1),
+       DEF_FIXED("p1c",    R7S9210_CLK_P1C,   CLK_PLL,         16, 1),
+       DEF_FIXED("p0",     R7S9210_CLK_P0,    CLK_PLL,         32, 1),
+};
+
+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.

+
+       DEF_MOD_STB("scif0",     47,    R7S9210_CLK_P1C),
+       DEF_MOD_STB("scif1",     46,    R7S9210_CLK_P1C),
+       DEF_MOD_STB("scif2",     45,    R7S9210_CLK_P1C),
+       DEF_MOD_STB("scif3",     44,    R7S9210_CLK_P1C),
+       DEF_MOD_STB("scif4",     43,    R7S9210_CLK_P1C),
+
+       DEF_MOD_STB("ether0",    65,    R7S9210_CLK_B),
+       DEF_MOD_STB("ether1",    64,    R7S9210_CLK_B),
+
+       DEF_MOD_STB("i2c0",      87,    R7S9210_CLK_P1),
+       DEF_MOD_STB("i2c1",      86,    R7S9210_CLK_P1),
+       DEF_MOD_STB("i2c2",      85,    R7S9210_CLK_P1),
+       DEF_MOD_STB("i2c3",      84,    R7S9210_CLK_P1),
+
+};
+
+struct clk * __init rza2_cpg_clk_register(struct device *dev,
+       const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
+       struct clk **clks, void __iomem *base,
+       struct raw_notifier_head *notifiers)
+{
+       const struct clk *parent;
+       unsigned int mult = 1;
+       unsigned int div = 1;
+       u16 frqcr;
+       u8 index;
+       int i;
+
+       parent = clks[core->parent];
+       if (IS_ERR(parent))
+               return ERR_CAST(parent);
+
+       switch (core->id) {
+       case CLK_MAIN:
+               break;
+
+       case CLK_PLL:
+               if (cpg_mode)
+                       mult = 44;      /* Divider 1 is 1/2 */
+               else
+                       mult = 88;      /* Divider 1 is 1 */
+               break;
+
+       default:
+               return ERR_PTR(-EINVAL);
+       }
+
+       /* 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?

+                       cpg_mode = 1;
+
+               frqcr = clk_readl(base + CPG_FRQCR) & 0xFFF;
+               if (frqcr == 0x012)
+                       index = 0;
+               else if (frqcr == 0x112)
+                       index = 1;
+               else if (frqcr == 0x212)
+                       index = 2;
+               else if (frqcr == 0x322)
+                       index = 3;
+               else if (frqcr == 0x333)
+                       index = 4;
+               else
+                       BUG_ON(1);      /* Illegal FRQCR value */
+
+               for (i = 0; i < ARRAY_SIZE(r7s9210_core_clks); i++) {
+                       switch (r7s9210_core_clks[i].id) {
+                       case R7S9210_CLK_I:
+                               r7s9210_core_clks[i].div = ratio_tab[index][0];
+                               break;
+                       case R7S9210_CLK_G:
+                               r7s9210_core_clks[i].div = ratio_tab[index][1];
+                               break;
+                       case R7S9210_CLK_B:
+                               r7s9210_core_clks[i].div = ratio_tab[index][2];
+                               break;
+                       case R7S9210_CLK_P1:
+                       case R7S9210_CLK_P1C:
+                               r7s9210_core_clks[i].div = ratio_tab[index][3];
+                               break;
+                       case R7S9210_CLK_P0:
+                               r7s9210_core_clks[i].div = ratio_tab[index][4];
+                               break;
+                       }
+               }
+       }
+
+       return clk_register_fixed_factor(NULL, core->name,
+                                        __clk_get_name(parent), 0, mult, div);
+}
+
+const struct cpg_mssr_info r7s9210_cpg_mssr_info __initconst = {
+       /* Core Clocks */
+       .core_clks = r7s9210_core_clks,
+       .num_core_clks = ARRAY_SIZE(r7s9210_core_clks),
+       .last_dt_core_clk = LAST_DT_CORE_CLK,
+       .num_total_core_clks = MOD_CLK_BASE,
+
+       /* 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?

+
+       /* Callbacks */
+       .cpg_clk_register = rza2_cpg_clk_register,
+
+       /* RZ/A2 has Standby Control Registers */
+       .stbyctrl = true,
+};

stbcr[1] should be 0x010?

+       0x424, 0x428, 0x42C, 0x430, 0x434, 0x460,

The last 3 don't exist?

+};

MOD_CLK_PACK()

Please preserve sort order.

"PACK" is a bit of a misnomer, as it no longer packs, but expands from base-10
to base-32  ;-)

Please preserve sort order.

Should that be an internal clock, not referred to from DT?
There's already the internal CLK_PLL clock.

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

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

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