Message ID | 20201029105515.16309-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | [v2] clk: renesas: r8a774c0: Add RPC clocks | expand |
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? > --- 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. > + 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? Gr{oetje,eeting}s, Geert
Hi Prabhakar, On Thu, Oct 29, 2020 at 3:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > 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. > > + 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? To clarify: basically you have two options here: 1. Model this clock as a non-mutable clock, based on the register settings at the time the kernel boots. I.e. register it as a fixed-divider clock. This is how we handle the PLLx clocks. 2. Model this clock as a fully-programmable clock. I.e. implement both dynamic parent selection and dynamic divider selection. You have picked something in between ;-) Gr{oetje,eeting}s, Geert
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
Hi Prabhakar, On Fri, Oct 30, 2020 at 11:13 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Thu, Oct 29, 2020 at 2:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > 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) That indeed doesn't match the values in the DIV[4:0] bits description. > 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. OK, that does match the 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 ? No, by writing to the RPCCKCR register. Remember, the _initial_ values are determined by the MD pins. They can still be changed. E.g. on R-Car D3, I verified that changing PLL0CR.CKSEL at runtime does work. In the end, we decided to just look at MD12 instead (IIRC because the CKSEL bit was removed from later documentation, but Rev 2.20 documents it again ;-) Gr{oetje,eeting}s, Geert
diff --git a/drivers/clk/renesas/r8a774c0-cpg-mssr.c b/drivers/clk/renesas/r8a774c0-cpg-mssr.c index 9fc9fa9e531a..cccb20de4d4b 100644 --- a/drivers/clk/renesas/r8a774c0-cpg-mssr.c +++ b/drivers/clk/renesas/r8a774c0-cpg-mssr.c @@ -44,6 +44,7 @@ enum clk_ids { CLK_S2, CLK_S3, CLK_SDSRC, + CLK_RPCSRC, CLK_RINT, CLK_OCO, @@ -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), + + DEF_BASE("rpc", R8A774C0_CLK_RPC, CLK_TYPE_GEN3_RPC, + CLK_RPCSRC), + DEF_BASE("rpcd2", R8A774C0_CLK_RPCD2, CLK_TYPE_GEN3_RPCD2, + R8A774C0_CLK_RPC), DEF_DIV6_RO(".r", CLK_RINT, CLK_EXTAL, CPG_RCKCR, 32), @@ -199,6 +206,7 @@ static const struct mssr_mod_clk r8a774c0_mod_clks[] __initconst = { DEF_MOD("can-fd", 914, R8A774C0_CLK_S3D2), DEF_MOD("can-if1", 915, R8A774C0_CLK_S3D4), DEF_MOD("can-if0", 916, R8A774C0_CLK_S3D4), + DEF_MOD("rpc-if", 917, R8A774C0_CLK_RPCD2), DEF_MOD("i2c6", 918, R8A774C0_CLK_S3D2), DEF_MOD("i2c5", 919, R8A774C0_CLK_S3D2), DEF_MOD("i2c-dvfs", 926, R8A774C0_CLK_CP), diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c index 488f8b3980c5..90a30416c9cf 100644 --- 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); + 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); + case CLK_TYPE_GEN3_RPC: return cpg_rpc_clk_register(core->name, base, __clk_get_name(parent), notifiers); diff --git a/drivers/clk/renesas/rcar-gen3-cpg.h b/drivers/clk/renesas/rcar-gen3-cpg.h index c4ac80cac6a0..74b95ab64046 100644 --- a/drivers/clk/renesas/rcar-gen3-cpg.h +++ b/drivers/clk/renesas/rcar-gen3-cpg.h @@ -24,6 +24,7 @@ enum rcar_gen3_clk_types { CLK_TYPE_GEN3_OSC, /* OSC EXTAL predivider and fixed divider */ CLK_TYPE_GEN3_RCKSEL, /* Select parent/divider using RCKCR.CKSEL */ CLK_TYPE_GEN3_RPCSRC, + CLK_TYPE_GEN3E3_RPCSRC, CLK_TYPE_GEN3_RPC, CLK_TYPE_GEN3_RPCD2,