RE: [PATCH v2 5/7] clk: renesas: cpg-mssr: Add support for R-Car V4H

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux