Hi Geert-san,
Thank you for your review!
> From: Geert Uytterhoeven, Sent: Wednesday, April 27, 2022 11:26 PM
>
> On Mon, Apr 25, 2022 at 8:42 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > Initial CPG support for R-Car V4H (r8a779g0).
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/r8a779g0-cpg-mssr.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * r8a779g0 Clock Pulse Generator / Module Standby and Software Reset
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corp.
> > + *
> > + * Based on r8a779g0-cpg-mssr.c
>
> I.e. based on itself? ;-)
Oops! It's r8a779f0 instead :)
> > +static const struct cpg_core_clk r8a779g0_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_GEN4_MAIN, CLK_EXTAL),
> > + DEF_BASE(".pll1", CLK_PLL1, CLK_TYPE_GEN4_PLL1, CLK_MAIN),
> > + DEF_BASE(".pll2", CLK_PLL2, CLK_TYPE_GEN4_PLL2, CLK_MAIN),
> > + DEF_BASE(".pll3", CLK_PLL3, CLK_TYPE_GEN4_PLL3, CLK_MAIN),
> > + DEF_BASE(".pll4", CLK_PLL4, CLK_TYPE_GEN4_PLL4, CLK_MAIN),
> > + DEF_BASE(".pll5", CLK_PLL5, CLK_TYPE_GEN4_PLL5, CLK_MAIN),
> > + DEF_BASE(".pll6", CLK_PLL6, CLK_TYPE_GEN4_PLL6, CLK_MAIN),
> > +
> > + DEF_FIXED(".pll1_div2", CLK_PLL1_DIV2, CLK_PLL1, 2, 1),
> > + DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 2, 1),
> > + DEF_FIXED(".pll3_div2", CLK_PLL3_DIV2, CLK_PLL3, 2, 1),
> > + DEF_FIXED(".pll4_div2", CLK_PLL4_DIV2, CLK_PLL4, 2, 1),
> > + DEF_FIXED(".pll5_div2", CLK_PLL5_DIV2, CLK_PLL5, 2, 1),
> > + DEF_FIXED(".pll5_div4", CLK_PLL5_DIV4, CLK_PLL5_DIV2, 2, 1),
> > + DEF_FIXED(".pll6_div2", CLK_PLL6_DIV2, CLK_PLL6, 2, 1),
> > + DEF_FIXED(".s0", CLK_S0, CLK_PLL1_DIV2, 2, 1),
> > + DEF_FIXED(".s0_vio", CLK_S0_VIO, CLK_PLL1_DIV2, 2, 1),
> > + DEF_FIXED(".s0_vc", CLK_S0_VC, CLK_PLL1_DIV2, 2, 1),
> > + DEF_FIXED(".s0_hsc", CLK_S0_HSC, CLK_PLL1_DIV2, 2, 1),
> > + DEF_FIXED(".sv_vip", CLK_SV_VIP, CLK_PLL1_DIV2, 2, 1),
>
> CLK_SV_VIP runs at 640 instead of 800 MHz, so CLK_PLL1 / 5?
You're correct. I'll fix it.
> > + DEF_FIXED(".sv_ir", CLK_SV_IR, CLK_PLL1_DIV2, 2, 1),
>
> Likewise for CLK_SV_IR.
Yes, I'll fix it.
> > + DEF_BASE(".sdsrc", CLK_SDSRC, CLK_TYPE_GEN4_SDSRC, CLK_PLL5),
> > + DEF_RATE(".oco", CLK_OCO, 32768),
> > +
> > + DEF_BASE(".rpcsrc", CLK_RPCSRC, CLK_TYPE_GEN4_RPCSRC, CLK_PLL5),
> > + DEF_BASE(".rpc", R8A779G0_CLK_RPC, CLK_TYPE_GEN4_RPC, CLK_RPCSRC),
>
> "rpc".
I got it.
> > + DEF_BASE("rpcd2", R8A779G0_CLK_RPCD2, CLK_TYPE_GEN4_RPCD2, R8A779G0_CLK_RPC),
>
> Please move "rpc" and "rpcd2" to Core Clock Outputs below,
> as they are not Internal Core Clocks.
I'll fix it.
> > + DEF_FIXED(".vio", CLK_VIO, CLK_PLL5_DIV2, 3, 1),
> > + DEF_FIXED(".vc", CLK_VC, CLK_PLL5_DIV2, 3, 1),
> > +
> > + /* Core Clock Outputs */
> > + DEF_FIXED("s0d2", R8A779G0_CLK_S0D2, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d3", R8A779G0_CLK_S0D3, CLK_S0, 3, 1),
> > + DEF_FIXED("s0d4", R8A779G0_CLK_S0D4, CLK_S0, 4, 1),
> > + DEF_FIXED("cl16m", R8A779G0_CLK_CL16M, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d1_vio", R8A779G0_CLK_S0D1_VIO, CLK_S0_VIO, 1, 1),
> > + DEF_FIXED("s0d2_vio", R8A779G0_CLK_S0D2_VIO, CLK_S0_VIO, 2, 1),
> > + DEF_FIXED("s0d4_vio", R8A779G0_CLK_S0D4_VIO, CLK_S0_VIO, 4, 1),
> > + DEF_FIXED("s0d8_vio", R8A779G0_CLK_S0D8_VIO, CLK_S0_VIO, 8, 1),
> > + DEF_FIXED("s0d1_vc", R8A779G0_CLK_S0D1_VC, CLK_S0_VC, 1, 1),
> > + DEF_FIXED("s0d2_vc", R8A779G0_CLK_S0D2_VC, CLK_S0_VC, 2, 1),
> > + DEF_FIXED("s0d4_vc", R8A779G0_CLK_S0D4_VC, CLK_S0_VC, 4, 1),
> > + DEF_FIXED("s0d2_mm", R8A779G0_CLK_S0D2_MM, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d4_mm", R8A779G0_CLK_S0D4_MM, CLK_S0, 4, 1),
> > + DEF_FIXED("cl16m_mm", R8A779G0_CLK_CL16M_MM, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d2_u3dg", R8A779G0_CLK_S0D2_U3DG, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d4_u3dg", R8A779G0_CLK_S0D4_U3DG, CLK_S0, 4, 1),
> > + DEF_FIXED("s0d2_rt", R8A779G0_CLK_S0D2_RT, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d3_rt", R8A779G0_CLK_S0D3_RT, CLK_S0, 3, 1),
> > + DEF_FIXED("s0d4_rt", R8A779G0_CLK_S0D4_RT, CLK_S0, 4, 1),
> > + DEF_FIXED("s0d6_rt", R8A779G0_CLK_S0D6_RT, CLK_S0, 6, 1),
> > + DEF_FIXED("s0d24_rt", R8A779G0_CLK_S0D24_RT, CLK_S0, 24, 1),
> > + DEF_FIXED("cl16m_rt", R8A779G0_CLK_CL16M_RT, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d2_per", R8A779G0_CLK_S0D2_PER, CLK_S0, 2, 1),
> > + DEF_FIXED("s0d3_per", R8A779G0_CLK_S0D3_PER, CLK_S0, 3, 1),
>
> Missing "s0d4_per".
Oops. I'll add it.
> > + DEF_FIXED("s0d6_per", R8A779G0_CLK_S0D6_PER, CLK_S0, 6, 1),
> > + DEF_FIXED("s0d12_per", R8A779G0_CLK_S0D12_PER, CLK_S0, 12, 1),
> > + DEF_FIXED("s0d24_per", R8A779G0_CLK_S0D24_PER, CLK_S0, 24, 1),
> > + DEF_FIXED("cl16m_per", R8A779G0_CLK_CL16M_PER, CLK_S0, 48, 1),
> > + DEF_FIXED("s0d1_hsc", R8A779G0_CLK_S0D1_HSC, CLK_S0_HSC, 1, 1),
> > + DEF_FIXED("s0d2_hsc", R8A779G0_CLK_S0D2_HSC, CLK_S0_HSC, 2, 1),
> > + DEF_FIXED("s0d4_hsc", R8A779G0_CLK_S0D4_HSC, CLK_S0_HSC, 4, 1),
> > + DEF_FIXED("cl16m_hsc", R8A779G0_CLK_CL16M_HSC, CLK_S0_HSC, 48, 1),
> > + DEF_FIXED("s0d2_cc", R8A779G0_CLK_S0D2_CC, CLK_S0, 2, 1),
> > + DEF_FIXED("svd1_ir", R8A779G0_CLK_SVD1_IR, CLK_SV_IR, 1, 1),
> > + DEF_FIXED("svd2_ir", R8A779G0_CLK_SVD2_IR, CLK_SV_IR, 2, 1),
> > + DEF_FIXED("svd1_vip", R8A779G0_CLK_SVD1_VIP, CLK_SV_VIP, 1, 1),
> > + DEF_FIXED("svd2_vip", R8A779G0_CLK_SVD2_VIP, CLK_SV_VIP, 2, 1),
> > + DEF_FIXED("s0d2_cc", R8A779G0_CLK_S0D2_CC, CLK_S0, 2, 1),
>
> "s0d2_cc" is already defined 5 lines above.
Oops. I'll remove this.
> > + DEF_FIXED("cbfusa", R8A779G0_CLK_CBFUSA, CLK_EXTAL, 2, 1),
> > + DEF_FIXED("cpex", R8A779G0_CLK_CPEX, CLK_EXTAL, 2, 1),
> > + DEF_FIXED("viobus", R8A779G0_CLK_VIOBUS, CLK_VIO, 1, 1),
> > + DEF_FIXED("viobusd2", R8A779G0_CLK_VIOBUSD2, CLK_VIO, 2, 1),
> > + DEF_FIXED("vcbus", R8A779G0_CLK_VCBUS, CLK_VC, 1, 1),
> > + DEF_FIXED("vcbusd2", R8A779G0_CLK_VCBUSD2, CLK_VC, 2, 1),
> > +
> > + DEF_GEN4_SD("sd0", R8A779G0_CLK_SD0, CLK_SDSRC, 0x870),
> > + DEF_DIV6P1("mso", R8A779G0_CLK_MSO, CLK_PLL5_DIV4, 0x87c),
> > +
> > + DEF_GEN4_OSC("osc", R8A779G0_CLK_OSC, CLK_EXTAL, 8),
> > + DEF_GEN4_MDSEL("r", R8A779G0_CLK_R, 29, CLK_EXTALR, 1, CLK_OCO, 1),
> > +};
> > +
> > +static const struct mssr_mod_clk r8a779g0_mod_clks[] __initconst = {
> > + DEF_MOD("hscif0", 514, R8A779G0_CLK_S0D3_PER),
> > + DEF_MOD("hscif1", 515, R8A779G0_CLK_S0D3_PER),
> > + DEF_MOD("hscif2", 516, R8A779G0_CLK_S0D3_PER),
> > + DEF_MOD("hscif3", 517, R8A779G0_CLK_S0D3_PER),
> > +};
> > +
> > +/*
> > + * CPG Clock Data
> > + */
> > +/*
> > + * MD EXTAL PLL1 PLL2 PLL3 PLL4 PLL5 PLL6 OSC
> > + * 14 13 (MHz)
> > + * ----------------------------------------------------------------
>
> You may want to make the horizontal line longer.
I'll fix it.
> > + * 0 0 16.66 / 1 x192 x204 x192 x144 x192 x168 /8
> > + * 0 1 20 / 1 x160 x170 x160 x120 x160 x140 /8
> > + * 1 0 Prohibited setting
> > + * 1 1 33.33 / 2 x192 x204 x192 x144 x192 x168 /8
>
> The last column values (OSC) should be /15, /19, resp. /38.
I completely misunderstood these parameters. I'll fix it.
> > + */
> > +#define CPG_PLL_CONFIG_INDEX(md) ((((md) & BIT(14)) >> 13) | \
> > + (((md) & BIT(13)) >> 13))
> > +
> > +static const struct rcar_gen4_cpg_pll_config cpg_pll_configs[4] = {
> > + /* EXTAL div PLL1 mult/div PLL2 mult/div PLL3 mult/div PLL4 mult/div PLL5 mult/div PLL6 mult/div
> OSC prediv */
> > + { 1, 192, 1, 204, 1, 192, 1, 144, 1, 192, 1, 168, 1,
> 8, },
> > + { 1, 160, 1, 170, 1, 160, 1, 120, 1, 160, 1, 140, 1,
> 8, },
> > + { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> 0, },
> > + { 2, 192, 1, 204, 1, 192, 1, 144, 1, 192, 1, 168, 1,
> 8, },
>
> The last column values should be 15, 19, 0, resp. 38.
I got it.
> The rest LGTM.
Thanks!
Best regards,
Yoshihiro Shimoda
