Re: [PATCH v2] clk: renesas: r8a774c0: Add RPC clocks

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Geert,

Thank you for the review.

On Thu, Oct 29, 2020 at 2:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Oct 29, 2020 at 11:55 AM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > Describe the RPCSRC internal clock and the RPC[D2] clocks derived from it,
> > as well as the RPC-IF module clock, in the RZ/G2E (R8A774C0) CPG/MSSR
> > driver.
>
> Thanks for your patch!
>
> > Add new clk type CLK_TYPE_GEN3E3_RPCSRC to handle registering rpcsrc
> > clock as the source for RPCSRC can be either PLL0/PLL1 and this depends
> > on MD[1:4] pins where as compared to other R-Car Gen3 SoC's the RPCSRC
> > clock source is always PLL1.
> >
> > MD[4] MD[3] MD[2] MD[1]
> >   0     0     0    1     -> RPCSRC CLK source is PLL1
> >   0     0     1    1     -> RPCSRC CLK source is PLL1
> >   0     1     0    0     -> RPCSRC CLK source is PLL1
> >   1     0     1    1     -> RPCSRC CLK source is PLL1
> >   x     x     x    x     -> For any other values RPCSRC CLK source is PLL0
>
> AFAIU, the _initial values_ of the RPCCKCR bits depend on the MD pins.
> They can still be changed at run-time, and might have been changed by
> the bootloader before transferring control to Linux.
>
> > R-Car Gen3 manual Rev.2.20 has in-correct information related to
> > determining the clock source for RPCSRC.
>
> Which part of the information is not correct?
> Where can I find corrected information?
> Is my understanding above incorrect, too?
>
R-Car Gen3 HW manual mentions the below statement (page 529, Rev.2.20 manual):
[R-Car E3]
When (MD4, MD3, MD2, MD1) = (0, 0, 0, 1) or (0, 1, 0, 0): DIV[2:0] =
011, DIV[4:3] = 00 (300 MHz PLL0)

Confirming with internal team this should be below:

When (MD4, MD3, MD2, MD1) = (0, 0, 0, 1) or (0, 1, 0, 0): DIV[2:0] =
011, DIV[4:3] = 00 (80 MHz PLL1)

This should be fixed in the next version of the document, and when
available I'll ask Chris P to send it across.

> > --- a/drivers/clk/renesas/r8a774c0-cpg-mssr.c
> > +++ b/drivers/clk/renesas/r8a774c0-cpg-mssr.c
>
> > @@ -73,6 +74,12 @@ static const struct cpg_core_clk r8a774c0_core_clks[] __initconst = {
> >         DEF_FIXED(".s2",       CLK_S2,             CLK_PLL1,       4, 1),
> >         DEF_FIXED(".s3",       CLK_S3,             CLK_PLL1,       6, 1),
> >         DEF_FIXED(".sdsrc",    CLK_SDSRC,          CLK_PLL1,       2, 1),
> > +       DEF_BASE(".rpcsrc",    CLK_RPCSRC, CLK_TYPE_GEN3E3_RPCSRC, (CLK_PLL1 << 16) | CLK_PLL0),
>
> You may want to add a new DEF_* helper macro for this.
>
> > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> > @@ -441,6 +441,14 @@ static const struct clk_div_table cpg_rpcsrc_div_table[] = {
> >         { 2, 5 }, { 3, 6 }, { 0, 0 },
> >  };
> >
> > +static const struct clk_div_table cpg_rpcsrc_e3_pll0_div_table[] = {
> > +       { 2, 8 }, { 0, 0 },
> > +};
> > +
> > +static const struct clk_div_table cpg_rpcsrc_e3_pll1_div_table[] = {
> > +       { 0, 5 }, { 1, 3 }, { 3, 2 }, { 0, 0 },
> > +};
> > +
> >  static const struct clk_div_table cpg_rpc_div_table[] = {
> >         { 1, 2 }, { 3, 4 }, { 5, 6 }, { 7, 8 }, { 0, 0 },
> >  };
> > @@ -515,6 +523,18 @@ static struct clk * __init cpg_rpcd2_clk_register(const char *name,
> >         return clk;
> >  }
> >
> > +static int __init cpg_rpcsrc_e3_get_parent(u32 mode)
> > +{
> > +       unsigned int e3_rpcsrc = (mode & GENMASK(4, 1)) >> 1;
> > +       unsigned int pll1[] = { 0x1, 0x3, 0x4, 0xb, };
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(pll1); i++)
> > +               if (e3_rpcsrc == pll1[i])
> > +                       return 1;
> > +
> > +       return 0;
> > +}
> >
> >  static const struct rcar_gen3_cpg_pll_config *cpg_pll_config __initdata;
> >  static unsigned int cpg_clk_extalr __initdata;
> > @@ -552,6 +572,7 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
> >         const struct clk *parent;
> >         unsigned int mult = 1;
> >         unsigned int div = 1;
> > +       int e3_rpcsrc_parent;
> >         u32 value;
> >
> >         parent = clks[core->parent & 0xffff];   /* some types use high bits */
> > @@ -696,6 +717,22 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
> >                                                   cpg_rpcsrc_div_table,
> >                                                   &cpg_lock);
> >
> > +       case CLK_TYPE_GEN3E3_RPCSRC:
> > +               e3_rpcsrc_parent = cpg_rpcsrc_e3_get_parent(cpg_mode);
>
> This is not correct if the boot loader has changed the parent clock.
>
You mean by manually togelling the MD pins before we get into Linux ?

> > +               if (e3_rpcsrc_parent) {
> > +                       parent = clks[core->parent >> 16];
> > +                       if (IS_ERR(parent))
> > +                               return ERR_CAST(parent);
> > +               }
> > +
> > +               return clk_register_divider_table(NULL, core->name,
> > +                                                 __clk_get_name(parent), 0,
> > +                                                 base + CPG_RPCCKCR, 3, 2, 0,
> > +                                                 e3_rpcsrc_parent ?
> > +                                                 cpg_rpcsrc_e3_pll1_div_table :
> > +                                                 cpg_rpcsrc_e3_pll0_div_table,
> > +                                                 &cpg_lock);
> > +
>
> So you want to keep the parent clock selection fixed, but still allow
> the system to change the divider?
> Why not support changing the parent too, by modeling this as a composite
> clock consisting of a mux and a divider?
>
I will investigate this further (read more about composite clocks).
Below are the values for RPC and RPCD2,

RPCϕ = 320 MHz, RPCD2ϕ = 160MHz.
RPCϕ = 160 MHz, RPCD2ϕ = 80MHz.
RPCϕ = 80 MHz, RPCD2ϕ = 40MHz.

Cheers,
Prabhakar