Hi Geert,
Thank you for the review.
On Thu, Oct 29, 2020 at 2:29 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Thu, Oct 29, 2020 at 11:55 AM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> 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
