Hi Geert-san,
Thank you for your review!
> From: Geert Uytterhoeven, Sent: Thursday, September 10, 2020 10:20 PM
>
> Hi Shimoda-san,
>
> On Wed, Sep 9, 2020 at 3:13 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > Initial support for R-Car V3U (r8a779a0), including core, module
> > clocks and register access, because register specification differs
> > from R-Car Gen2/3.
> >
> > Inspired by patches in the BSP by LUU HOAI.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>
> Thanks for the update!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/r8a779a0-cpg-mssr.c
>
> > +#define CPG_PLL20CR 0x0834
> > +#define CPG_PLL21CR 0x0838
> > +#define CPG_PLL30CR 0x083c
> > +#define CPG_PLL31CR 0x0840
> > +
> > +static const struct cpg_core_clk r8a779a0_core_clks[] __initconst = {
> > + /* External Clock Inputs */
> > + DEF_INPUT("extal", CLK_EXTAL),
> > + DEF_INPUT("extalr", CLK_EXTALR),
> > +
> > + /* Internal Core Clocks */
> > + DEF_BASE(".main", CLK_MAIN, CLK_TYPE_R8A779A0_MAIN, CLK_EXTAL),
> > + DEF_BASE(".pll1", CLK_PLL1, CLK_TYPE_R8A779A0_PLL1, CLK_MAIN),
> > + DEF_BASE(".pll5", CLK_PLL5, CLK_TYPE_R8A779A0_PLL5, CLK_MAIN),
> > + DEF_PLL(".pll20", CLK_PLL20, CPG_PLL20CR),
> > + DEF_PLL(".pll21", CLK_PLL21, CPG_PLL21CR),
> > + DEF_PLL(".pll30", CLK_PLL30, CPG_PLL30CR),
> > + DEF_PLL(".pll31", CLK_PLL31, CPG_PLL31CR),
>
> Given the users of the DEF_GEN3_SD() and DEF_DIV6P1() just hardcode the
> register offsets, and the CPG_PLL*CR are used in a single place only,
> perhaps it makes sense to hardcode the offsets here, too, and drop the
> defines?
I see. I'll fix it.
> [...]
>
> > + DEF_DIV6P1("mso", R8A779A0_CLK_MSO, CLK_PLL5_DIV4, 0x87c),
> > + DEF_DIV6P1("canfd", R8A779A0_CLK_CANFD, CLK_PLL5_DIV4, 0x878),
> > + DEF_DIV6P1("csi0", R8A779A0_CLK_CSI0, CLK_PLL5_DIV4, 0x880),
>
> > +/*
> > + * CPG Clock Data
> > + */
> > +/*
> > + * MD EXTAL PLL1 PLL20 PLL30 PLL4 PLL5 PLL6 OSC
> > + * 14 13 (MHz) 21 31
> > + * --------------------------------------------------------
> > + * 0 0 16.66 x 1 x128 x216 x128 x144 x192 x128 /16
> > + * 0 1 20 x 1 x106 x180 x106 x120 x160 x106 /19
> > + * 1 0 Prohibited setting
> > + * 1 1 33.33 / 2 x128 x216 x128 x144 x192 x128 /32
>
> Please drop the PLL6 column, as PLL6 doesn't really exist.
Oops. I'll drop it.
> > + */
>
> > --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> > +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> > @@ -57,6 +57,11 @@ static const u16 mstpsr[] = {
> > 0x9A0, 0x9A4, 0x9A8, 0x9AC,
> > };
> >
> > +static const u16 mstpsr_for_v3u[] = {
> > + 0x2E00, 0x2E04, 0x2E08, 0x2E0C, 0x2E10, 0x2E14, 0x2E18, 0x2E1C,
> > + 0x2E20, 0x2E24, 0x2E28, 0x2E2C, 0x2E30, 0x2E34, 0x2E38,
> > +};
> > +
> > /*
> > * System Module Stop Control Register offsets
> > */
> > @@ -66,6 +71,11 @@ static const u16 smstpcr[] = {
> > 0x990, 0x994, 0x998, 0x99C,
> > };
> >
> > +static const u16 mstpcr_for_v3u[] = {
> > + 0x2D00, 0x2D04, 0x2D08, 0x2D0C, 0x2D10, 0x2D14, 0x2D18, 0x2D1C,
> > + 0x2D20, 0x2D24, 0x2D28, 0x2D2C, 0x2D30, 0x2D34, 0x2D38,
> > +};
>
> > @@ -939,6 +955,9 @@ static int __init cpg_mssr_common_init(struct device *dev,
> > priv->control_regs = smstpcr;
> > } else if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A) {
> > priv->control_regs = stbcr;
> > + } else if (priv->reg_layout == CLK_REG_LAYOUT_RCAR_V3U) {
> > + priv->status_regs = mstpsr_for_v3u;
> > + priv->control_regs = mstpcr_for_v3u;
>
> Missing arrays for V3U-specific srcr and srstclr.
I'll add these registers.
> > } else {
> > error = -EINVAL;
> > goto out_err;
>
> The rest looks good to me!
Thanks!
Best regards,
Yoshihiro Shimoda
