| Message ID | 20230919123722.2338932-1-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | clk: renesas: rcar-gen3: Extend SDn divider table | expand |
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
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
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
> 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.
> > 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.
> > 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?
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
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
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,
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(-)