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

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

[Wolfram Sang]

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@sang-engineering.com>
---

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

 drivers/clk/renesas/r8a779f0-cpg-mssr.c |  2 +-
 drivers/clk/renesas/r8a779g0-cpg-mssr.c |  2 +-
 drivers/clk/renesas/rcar-gen4-cpg.c     | 10 +++++++++-
 drivers/clk/renesas/rcar-gen4-cpg.h     |  5 ++++-
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/renesas/r8a779f0-cpg-mssr.c b/drivers/clk/renesas/r8a779f0-cpg-mssr.c
index 862b49736bf0..0c3aa529e4fa 100644
--- 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;
+		}
 		break;
 
 	case CLK_TYPE_GEN4_SDH:
diff --git a/drivers/clk/renesas/rcar-gen4-cpg.h b/drivers/clk/renesas/rcar-gen4-cpg.h
index 0b15dcfdca7b..e5237b2f5205 100644
--- a/drivers/clk/renesas/rcar-gen4-cpg.h
+++ b/drivers/clk/renesas/rcar-gen4-cpg.h
@@ -32,6 +32,10 @@ enum rcar_gen4_clk_types {
 	CLK_TYPE_GEN4_SOC_BASE,
 };
 
+#define DEF_GEN4_SDSRC(_name, _id, _parent0, _parent1, _offset)	\
+	DEF_BASE(_name, _id, CLK_TYPE_GEN4_SDSRC, \
+		 (_parent0) << 16 | (_parent1), .offset = _offset)
+
 #define DEF_GEN4_SDH(_name, _id, _parent, _offset)	\
 	DEF_BASE(_name, _id, CLK_TYPE_GEN4_SDH, _parent, .offset = _offset)
 
@@ -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,
-- 
2.35.1

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@sang-engineering.com> 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@sang-engineering.com>

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@linux-m68k.org

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]

Hi Geert,

> > 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?

TBH, no :) I did that to be future proof in case the register gets moved
somewhere else. Also, this is consistent how we did it with DEF_GEN3_SD.

> >         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.

We don't have to complicate the code unnecessarily. If you think the
diagram is flawed, then we can keep the current code. I changed the code
because I was confused when checking 'clk_summary' with the diagram and
wanted to make it proper to reduce my confusion.

My patches to enable eMMC on Spider have a significantly lower
throughput than the BSP, so this was the first step of trying to verify
things and get the clocks in shape.

If you call it superfluous, then we can drop it. No hard feelings here.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Jun 28, 2022 at 1:08 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > 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?
>
> TBH, no :) I did that to be future proof in case the register gets moved
> somewhere else. Also, this is consistent how we did it with DEF_GEN3_SD.

In case of DEF_GEN3_SD(), we have a register offset parameter because
most affected SoCs have multiple SDHI instances, and multiple registers
to control their clocks.

> > >         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.
>
> We don't have to complicate the code unnecessarily. If you think the
> diagram is flawed, then we can keep the current code. I changed the code
> because I was confused when checking 'clk_summary' with the diagram and
> wanted to make it proper to reduce my confusion.
>
> My patches to enable eMMC on Spider have a significantly lower
> throughput than the BSP, so this was the first step of trying to verify
> things and get the clocks in shape.
>
> If you call it superfluous, then we can drop it. No hard feelings here.

OK, so let's drop this.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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