[PATCH] clk: renesas: rcar-gen3: Extend SDn divider table

[PATCH] clk: renesas: rcar-gen3: Extend SDn divider table

[Dirk Behme]

The clock dividers x 1/1 and x 1/2 might be used with clock stop bit
enabled or not. While this table contains the enabled flavor (STPnHCK == 0).
The version for stopped clock (STPnHCK == 1) is missing. This might
result in warnings like [1] because no valid clock divider is found.
Resulting in a 0 divider.

There are Renesas ARM Trusted Firmware version out there which e.g.
configure 0x201 (shifted logical right by 2: 0x80) and with this match
the added { STPnHCK | 0, 1 }:

https://github.com/renesas-rcar/arm-trusted-firmware/blob/rcar_gen3_v2.3/drivers/renesas/rcar/emmc/emmc_init.c#L108

[1]

------------[ cut here ]------------
sd1h: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set
WARNING: CPU: 1 PID: 1 at drivers/clk/clk-divider.c:141 divider_recalc_rate+0x48/0x70
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.1.52 #1
Hardware name: Custom board based on r8a7796 (DT)
pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : divider_recalc_rate+0x48/0x70
lr : divider_recalc_rate+0x48/0x70
sp : ffff800008033820
x29: ffff800008033820 x28: 0000000000000000 x27: 0000000000000001
x26: ffff80000894c19f x25: 0000000000000000 x24: ffff800008033958
x23: ffff0004402fc4a8 x22: ffff0004402fc400 x21: ffff0004402f8400
x20: 0000000000000000 x19: 000000002fadcf80 x18: ffff80000875c290
x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
x14: fffffffffffc0000 x13: 0a74657320746f6e x12: 204f52455a5f574f
x11: 4c4c415f52454449 x10: 5649445f4b4c4320 x9 : 00080000000000ff
x8 : 0000000000000003 x7 : ffff800008034000 x6 : ffff800008030000
x5 : ffff000440188000 x4 : ffff800008034000 x3 : 0000000000000001
x2 : 2836f9a35ff96400 x1 : 2836f9a35ff96400 x0 : 0000000000000000
Call trace:
 divider_recalc_rate+0x48/0x70
 clk_divider_recalc_rate+0x48/0x50
 __clk_register+0x450/0x5d0
 clk_hw_register+0x28/0x40
 __clk_hw_register_divider+0x148/0x18c
 clk_register_divider_table+0x48/0x60
 cpg_sdh_clk_register+0x88/0xd0
 rcar_gen3_cpg_clk_register+0x168/0x490
 cpg_mssr_register_core_clk+0x16c/0x1c0
 cpg_mssr_probe+0x128/0x280
 platform_probe+0x64/0xb0
 really_probe+0x148/0x278
 __driver_probe_device+0xec/0x104
 driver_probe_device+0x38/0xf0
 __driver_attach+0x4c/0xfc
 bus_for_each_dev+0x78/0xbc
 driver_attach+0x20/0x28
 bus_add_driver+0x17c/0x1d0
 driver_register+0xac/0xe8
 __platform_driver_probe+0x88/0xe0
 cpg_mssr_init+0x20/0x28
 do_one_initcall+0x88/0x1c8
 kernel_init_freeable+0x2b0/0x2b8
 kernel_init+0x20/0x124
 ret_from_fork+0x10/0x20
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------

Fixes: bb6d3fa98a41 ("clk: renesas: rcar-gen3: Switch to new SD clock handling")
CC: Wolfram Sang <wsa+renesas@sang-engineering.com>
CC: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 drivers/clk/renesas/rcar-cpg-lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/renesas/rcar-cpg-lib.c b/drivers/clk/renesas/rcar-cpg-lib.c
index e2e0447de190..4d6271714755 100644
--- a/drivers/clk/renesas/rcar-cpg-lib.c
+++ b/drivers/clk/renesas/rcar-cpg-lib.c
@@ -70,8 +70,8 @@ void cpg_simple_notifier_register(struct raw_notifier_head *notifiers,
 #define STPnHCK	BIT(9 - SDnSRCFC_SHIFT)
 
 static const struct clk_div_table cpg_sdh_div_table[] = {
-	{ 0, 1 }, { 1, 2 }, { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 },
-	{ STPnHCK | 4, 16 }, { 0, 0 },
+	{ 0, 1 }, { 1, 2 }, { STPnHCK | 0, 1 }, { STPnHCK | 1, 2 },
+	{ STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 }, { 0, 0 },
 };
 
 struct clk * __init cpg_sdh_clk_register(const char *name,
-- 
2.28.0

Re: [PATCH] clk: renesas: rcar-gen3: Extend SDn divider table

[Wolfram Sang]

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

Hi Dirk,

long time no see!

> There are Renesas ARM Trusted Firmware version out there which e.g.
> configure 0x201 (shifted logical right by 2: 0x80) and with this match
> the added { STPnHCK | 0, 1 }:

IIRC, that means that the ATF uses 200MHz for the data channel but
disables the 800MHz for the SCC. Because of that, I assume ATF doesn't
do tuning then? Isn't that risky to operate at 200MHz without tuning?

>  static const struct clk_div_table cpg_sdh_div_table[] = {
> -	{ 0, 1 }, { 1, 2 }, { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 },
> -	{ STPnHCK | 4, 16 }, { 0, 0 },
> +	{ 0, 1 }, { 1, 2 }, { STPnHCK | 0, 1 }, { STPnHCK | 1, 2 },
> +	{ STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 }, { 0, 0 },

Anyhow, since such ATF seems to be in the wild then, I assume we should
at least support reading such configuration values. I'd reorder it like
this, though:

 +	{ 0, 1 }, { STPnHCK | 0, 1 }, { 1, 2 }, { STPnHCK | 1, 2 },
 +	{ STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 }, { 0, 0 },

And probably add a comment that the duplicate entries are only for
reading and are not recommended for use with Linux (which will still use
the first matching pair i.e. without STPnHCK).

Geert, does this all make sense to you?

All the best,

   Wolfram


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

Re: [PATCH] clk: renesas: rcar-gen3: Extend SDn divider table

[Behme Dirk (CM/ESO2)]

Hi Wolfram,

On 20.09.2023 22:41, Wolfram Sang wrote:
> Hi Dirk,
> 
> long time no see!

Got a chance to look at kernel 6.1 :)

>> There are Renesas ARM Trusted Firmware version out there which e.g.
>> configure 0x201 (shifted logical right by 2: 0x80) and with this match
>> the added { STPnHCK | 0, 1 }:
> 
> IIRC, that means that the ATF uses 200MHz for the data channel but
> disables the 800MHz for the SCC. Because of that, I assume ATF doesn't
> do tuning then? Isn't that risky to operate at 200MHz without tuning?
> 
>>   static const struct clk_div_table cpg_sdh_div_table[] = {
>> -	{ 0, 1 }, { 1, 2 }, { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 },
>> -	{ STPnHCK | 4, 16 }, { 0, 0 },
>> +	{ 0, 1 }, { 1, 2 }, { STPnHCK | 0, 1 }, { STPnHCK | 1, 2 },
>> +	{ STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 }, { 0, 0 },
> 
> Anyhow, since such ATF seems to be in the wild then, I assume we should
> at least support reading such configuration values. I'd reorder it like
> this, though:
> 
>   +	{ 0, 1 }, { STPnHCK | 0, 1 }, { 1, 2 }, { STPnHCK | 1, 2 },
>   +	{ STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 }, { 0, 0 },
> 
> And probably add a comment that the duplicate entries are only for
> reading and are not recommended for use with Linux (which will still use
> the first matching pair i.e. without STPnHCK).

Yes, I can do that :)

> Geert, does this all make sense to you?

I was just wondering why this table includes STPnHCK? I mean, this is 
'just' a translation table between the value read from the register to 
the divider (1, 2, 4, 8, 16)? Couldn't we mask 7 instead of 8 bits and 
with this drop STPnHCK from the comparison? The resulting divider would 
stay the same. Or even better just mask SDnSRCFC[2:0] (i.e. 3 bits)?

Best regards

dirk

Re: [PATCH] clk: renesas: rcar-gen3: Extend SDn divider table

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Sep 20, 2023 at 10:41 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > There are Renesas ARM Trusted Firmware version out there which e.g.
> > configure 0x201 (shifted logical right by 2: 0x80) and with this match
> > the added { STPnHCK | 0, 1 }:
>
> IIRC, that means that the ATF uses 200MHz for the data channel but
> disables the 800MHz for the SCC. Because of that, I assume ATF doesn't
> do tuning then? Isn't that risky to operate at 200MHz without tuning?

Perhaps these products don't care about using SDHI for booting (i.e. use
HyperFLASH instead), and thus expect only the application OS (Linux)
to use SDHI?

> >  static const struct clk_div_table cpg_sdh_div_table[] = {
> > -     { 0, 1 }, { 1, 2 }, { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 },
> > -     { STPnHCK | 4, 16 }, { 0, 0 },
> > +     { 0, 1 }, { 1, 2 }, { STPnHCK | 0, 1 }, { STPnHCK | 1, 2 },
> > +     { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 }, { 0, 0 },
>
> Anyhow, since such ATF seems to be in the wild then, I assume we should
> at least support reading such configuration values. I'd reorder it like
> this, though:
>
>  +      { 0, 1 }, { STPnHCK | 0, 1 }, { 1, 2 }, { STPnHCK | 1, 2 },
>  +      { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 }, { 0, 0 },
>
> And probably add a comment that the duplicate entries are only for
> reading and are not recommended for use with Linux (which will still use
> the first matching pair i.e. without STPnHCK).
>
> Geert, does this all make sense to you?

I'm sure you remember better than me the relation between and the
impact of the stop bit and the various divider values ;-)

An alternative would be to let cpg_sdh_clk_register() sanitize the
pre-existing contents of the SD-IFn Clock Frequency Control Register,
so there would be no need to extend cpg_sdh_div_table[].  An advantage
of that approach would be that it can handle all invalid combinations,
not just the few that have been seen in the wild.
(following the old networking mantra: "be strict when sinding, be
liberal when receiving').

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-gen3: Extend SDn divider table

[Wolfram Sang]

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


> I was just wondering why this table includes STPnHCK? I mean, this is 'just'
> a translation table between the value read from the register to the divider
> (1, 2, 4, 8, 16)?

I thought this table was used both ways? Need to check, but can only do
later today.


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

Re: [PATCH] clk: renesas: rcar-gen3: Extend SDn divider table

[Wolfram Sang]

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


> > IIRC, that means that the ATF uses 200MHz for the data channel but
> > disables the 800MHz for the SCC. Because of that, I assume ATF doesn't
> > do tuning then? Isn't that risky to operate at 200MHz without tuning?
> 
> Perhaps these products don't care about using SDHI for booting (i.e. use
> HyperFLASH instead), and thus expect only the application OS (Linux)
> to use SDHI?

Even then, I think ATF needs fixing because it sets up a clock divider
which is not recommended.

> An alternative would be to let cpg_sdh_clk_register() sanitize the
> pre-existing contents of the SD-IFn Clock Frequency Control Register,
> so there would be no need to extend cpg_sdh_div_table[].  An advantage
> of that approach would be that it can handle all invalid combinations,
> not just the few that have been seen in the wild.
> (following the old networking mantra: "be strict when sinding, be
> liberal when receiving').

That sounds very reasonable to me.


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

Re: [PATCH] clk: renesas: rcar-gen3: Extend SDn divider table

[Wolfram Sang]

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


> > An alternative would be to let cpg_sdh_clk_register() sanitize the
> > pre-existing contents of the SD-IFn Clock Frequency Control Register,
> > so there would be no need to extend cpg_sdh_div_table[].  An advantage
> > of that approach would be that it can handle all invalid combinations,
> > not just the few that have been seen in the wild.
> > (following the old networking mantra: "be strict when sinding, be
> > liberal when receiving').
> 
> That sounds very reasonable to me.

Thinking further: Sanitizing a pre-existing value of SDH means also
sanitizing the value of SD because only specific combinations of these
are recommended (or even "allowed" as I read it). This is getting a bit
complicated. What about just applying a default value to SDH and SD
which is from the recommended set of parameters? That will also help
when probing the clocks. Once SDHI probes, it will modify clocks anyhow.

Opinions?


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

Re: [PATCH] clk: renesas: rcar-gen3: Extend SDn divider table

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Sep 26, 2023 at 9:48 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > An alternative would be to let cpg_sdh_clk_register() sanitize the
> > > pre-existing contents of the SD-IFn Clock Frequency Control Register,
> > > so there would be no need to extend cpg_sdh_div_table[].  An advantage
> > > of that approach would be that it can handle all invalid combinations,
> > > not just the few that have been seen in the wild.
> > > (following the old networking mantra: "be strict when sinding, be
> > > liberal when receiving').
> >
> > That sounds very reasonable to me.
>
> Thinking further: Sanitizing a pre-existing value of SDH means also
> sanitizing the value of SD because only specific combinations of these
> are recommended (or even "allowed" as I read it). This is getting a bit
> complicated. What about just applying a default value to SDH and SD
> which is from the recommended set of parameters? That will also help
> when probing the clocks. Once SDHI probes, it will modify clocks anyhow.

Sounds fine to me, thanks!

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-gen3: Extend SDn divider table

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Sep 26, 2023 at 9:59 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Sep 26, 2023 at 9:48 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > > > An alternative would be to let cpg_sdh_clk_register() sanitize the
> > > > pre-existing contents of the SD-IFn Clock Frequency Control Register,
> > > > so there would be no need to extend cpg_sdh_div_table[].  An advantage
> > > > of that approach would be that it can handle all invalid combinations,
> > > > not just the few that have been seen in the wild.
> > > > (following the old networking mantra: "be strict when sinding, be
> > > > liberal when receiving').
> > >
> > > That sounds very reasonable to me.
> >
> > Thinking further: Sanitizing a pre-existing value of SDH means also
> > sanitizing the value of SD because only specific combinations of these
> > are recommended (or even "allowed" as I read it). This is getting a bit
> > complicated. What about just applying a default value to SDH and SD
> > which is from the recommended set of parameters? That will also help
> > when probing the clocks. Once SDHI probes, it will modify clocks anyhow.
>
> Sounds fine to me, thanks!

On second thought, on a system where SDHI is assigned to the RT core,
this would interfere with the software running on the RT core.  So I
think it would only be safe to overwrite with a default value when
the current register values are invalid.

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