Re: [PATCH] clk: renesas: rcar-gen4: implement SDSRC properly

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Jun 15, 2022 at 12:12 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> Depending on the divider setting, SDSRC can have a different parent.
> Implement this when reading the divider, to get a correct clock tree.
> Setting the divider is left to the bootloader for now.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>

Thanks for your patch!

> Tested on my Spider board (r8a779f0). Only build tested for r8a779g0 but
> the docs for the registers are the same.

While the SDSRCSEL bits are the same, the register at offset 0x8a4 is
called SD0CKCR1 on R-Car S4-8, and CKSRCSELCR on R-Car V4H.
I guess that is why you removed the definition of SD0CKCR1, and stored
the register offset in DEF_GEN4_SDSRC(), despite both being the same?

> --- a/drivers/clk/renesas/r8a779f0-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a779f0-cpg-mssr.c
> @@ -71,7 +71,7 @@ static const struct cpg_core_clk r8a779f0_core_clks[] __initconst = {
>         DEF_FIXED(".pll6_div2", CLK_PLL6_DIV2,  CLK_PLL6,       2, 1),
>         DEF_FIXED(".s0",        CLK_S0,         CLK_PLL1_DIV2,  2, 1),
>
> -       DEF_BASE(".sdsrc",      CLK_SDSRC,      CLK_TYPE_GEN4_SDSRC, CLK_PLL5),
> +       DEF_GEN4_SDSRC(".sdsrc", CLK_SDSRC,     CLK_PLL5_DIV2,  CLK_PLL5, 0x08a4),
>         DEF_RATE(".oco",        CLK_OCO,        32768),
>
>         DEF_BASE(".rpcsrc",     CLK_RPCSRC,     CLK_TYPE_GEN4_RPCSRC, CLK_PLL5),
> diff --git a/drivers/clk/renesas/r8a779g0-cpg-mssr.c b/drivers/clk/renesas/r8a779g0-cpg-mssr.c
> index 3fc4233b1ead..c8cd32cf4606 100644
> --- a/drivers/clk/renesas/r8a779g0-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a779g0-cpg-mssr.c
> @@ -86,7 +86,7 @@ static const struct cpg_core_clk r8a779g0_core_clks[] __initconst = {
>         DEF_FIXED(".s0_hsc",    CLK_S0_HSC,     CLK_PLL1_DIV2,  2, 1),
>         DEF_FIXED(".sv_vip",    CLK_SV_VIP,     CLK_PLL1,       5, 1),
>         DEF_FIXED(".sv_ir",     CLK_SV_IR,      CLK_PLL1,       5, 1),
> -       DEF_BASE(".sdsrc",      CLK_SDSRC,      CLK_TYPE_GEN4_SDSRC, CLK_PLL5),
> +       DEF_GEN4_SDSRC(".sdsrc", CLK_SDSRC,     CLK_PLL5_DIV2,  CLK_PLL5, 0x08a4),
>         DEF_RATE(".oco",        CLK_OCO,        32768),
>
>         DEF_BASE(".rpcsrc",     CLK_RPCSRC,             CLK_TYPE_GEN4_RPCSRC, CLK_PLL5),
> diff --git a/drivers/clk/renesas/rcar-gen4-cpg.c b/drivers/clk/renesas/rcar-gen4-cpg.c
> index c7ed43d6aa67..c6662ec10292 100644
> --- a/drivers/clk/renesas/rcar-gen4-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen4-cpg.c
> @@ -240,7 +240,15 @@ struct clk * __init rcar_gen4_cpg_clk_register(struct device *dev,
>                                           base, core->div, core->offset);
>
>         case CLK_TYPE_GEN4_SDSRC:
> -               div = ((readl(base + SD0CKCR1) >> 29) & 0x03) + 4;
> +               value = (readl(base + core->offset) >> 29) & 3;
> +               if (value) {
> +                       div = value + 4;
> +               } else {
> +                       parent = clks[core->parent >> 16];
> +                       if (IS_ERR(parent))
> +                               return ERR_CAST(parent);
> +                       div = 2;
> +               }

So this gives the exact same divider of PLL5 before.

The clock diagram indeed shows different paths for value 0
(PLL5 -> 1/2 -> 1/2) and values 1 and 2 (PLL5 -> {1/5 or 1/6}).
But the textual description for SDSRC says "The SDSRC divider divides
PLL5 output clock", matching the original code.

Do we have to complicate the code? ;-)
I guess the clock diagram was based on the diagram for R-Car H3
(which has two daisy-chained fixed 1/2 dividers), with the new 1/5
and 1/6 dividers added.

> --- a/drivers/clk/renesas/rcar-gen4-cpg.h
> +++ b/drivers/clk/renesas/rcar-gen4-cpg.h
> @@ -67,7 +71,6 @@ struct rcar_gen4_cpg_pll_config {
>  };
>
>  #define CPG_RPCCKCR    0x874
> -#define SD0CKCR1       0x8a4
>
>  struct clk *rcar_gen4_cpg_clk_register(struct device *dev,
>         const struct cpg_core_clk *core, const struct cpg_mssr_info *info,

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