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?
> --- 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
