[PATCH v2 RFT] clk: renesas: rcar-gen3: Extend SDnH divider table

[PATCH v2 RFT] clk: renesas: rcar-gen3: Extend SDnH divider table

[Wolfram Sang]

From: Dirk Behme <dirk.behme@de.bosch.com>

The clock dividers might be used with clock stop bit enabled or not.
Current tables only support recommended values from the datasheet. This
might result in warnings like below 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

------------[ 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
...
------------[ cut here ]------------

Fixes: bb6d3fa98a41 ("clk: renesas: rcar-gen3: Switch to new SD clock handling")
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
[wsa: extended the table to 5 entries, added comments, reword commit message a little]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

After some discussion on IRC, Geert and I concluded that Dirk's approach
is good but we wanted to extend it to all 5 possibilities. Also,
comments explaining the situation. I added these. It works on my Ebisu
board (R-Car E3). Dirk, could you kindly test on your system?

 drivers/clk/renesas/rcar-cpg-lib.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/renesas/rcar-cpg-lib.c b/drivers/clk/renesas/rcar-cpg-lib.c
index e2e0447de190..b1d9a9a55569 100644
--- a/drivers/clk/renesas/rcar-cpg-lib.c
+++ b/drivers/clk/renesas/rcar-cpg-lib.c
@@ -70,8 +70,20 @@ 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 },
+	/*
+	 * These values are recommended by the datasheet. Because they come
+	 * first, Linux will only use these.
+	 */
+	{ 0, 1 }, { 1, 2 }, { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 },
+	/*
+	 * These values are not recommended because STPnHCK is wrong. But they
+	 * have been seen because of broken firmware. So, we support reading
+	 * them but Linux will sanitize them when initializing through
+	 * recalc_rate.
+	 */
+	{ STPnHCK | 0, 1 }, { STPnHCK | 1, 2 },  { 2, 4 }, { 3, 8 }, { 4, 16 },
+	/* Sentinel */
+	{ 0, 0 }
 };
 
 struct clk * __init cpg_sdh_clk_register(const char *name,
-- 
2.30.2

Re: [PATCH v2 RFT] clk: renesas: rcar-gen3: Extend SDnH divider table

[Behme Dirk (CM/ESO2)]

On 28.09.2023 10:03, Wolfram Sang wrote:
> From: Dirk Behme <dirk.behme@de.bosch.com>
> 
> The clock dividers might be used with clock stop bit enabled or not.
> Current tables only support recommended values from the datasheet. This
> might result in warnings like below 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
> 
> ------------[ 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
> ...
> ------------[ cut here ]------------
> 
> Fixes: bb6d3fa98a41 ("clk: renesas: rcar-gen3: Switch to new SD clock handling")
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> [wsa: extended the table to 5 entries, added comments, reword commit message a little]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> After some discussion on IRC, Geert and I concluded that Dirk's approach
> is good but we wanted to extend it to all 5 possibilities. Also,
> comments explaining the situation. I added these. It works on my Ebisu
> board (R-Car E3). Dirk, could you kindly test on your system?


Seems to work nicely :)

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>

Thanks!

Dirk


>   drivers/clk/renesas/rcar-cpg-lib.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/renesas/rcar-cpg-lib.c b/drivers/clk/renesas/rcar-cpg-lib.c
> index e2e0447de190..b1d9a9a55569 100644
> --- a/drivers/clk/renesas/rcar-cpg-lib.c
> +++ b/drivers/clk/renesas/rcar-cpg-lib.c
> @@ -70,8 +70,20 @@ 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 },
> +	/*
> +	 * These values are recommended by the datasheet. Because they come
> +	 * first, Linux will only use these.
> +	 */
> +	{ 0, 1 }, { 1, 2 }, { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 },
> +	/*
> +	 * These values are not recommended because STPnHCK is wrong. But they
> +	 * have been seen because of broken firmware. So, we support reading
> +	 * them but Linux will sanitize them when initializing through
> +	 * recalc_rate.
> +	 */
> +	{ STPnHCK | 0, 1 }, { STPnHCK | 1, 2 },  { 2, 4 }, { 3, 8 }, { 4, 16 },
> +	/* Sentinel */
> +	{ 0, 0 }
>   };
>   
>   struct clk * __init cpg_sdh_clk_register(const char *name,

Re: [PATCH v2 RFT] clk: renesas: rcar-gen3: Extend SDnH divider table

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Sep 29, 2023 at 8:14 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> From: Dirk Behme <dirk.behme@de.bosch.com>
>
> The clock dividers might be used with clock stop bit enabled or not.
> Current tables only support recommended values from the datasheet. This
> might result in warnings like below 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
>
> ------------[ 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
> ...
> ------------[ cut here ]------------
>
> Fixes: bb6d3fa98a41 ("clk: renesas: rcar-gen3: Switch to new SD clock handling")
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> [wsa: extended the table to 5 entries, added comments, reword commit message a little]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk-for-v6.7.

> --- a/drivers/clk/renesas/rcar-cpg-lib.c
> +++ b/drivers/clk/renesas/rcar-cpg-lib.c
> @@ -70,8 +70,20 @@ 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 },
> +       /*
> +        * These values are recommended by the datasheet. Because they come
> +        * first, Linux will only use these.
> +        */
> +       { 0, 1 }, { 1, 2 }, { STPnHCK | 2, 4 }, { STPnHCK | 3, 8 }, { STPnHCK | 4, 16 },

Would you mind if I would wrap this to 80 columns (like the original)
while applying?

> +       /*
> +        * These values are not recommended because STPnHCK is wrong. But they
> +        * have been seen because of broken firmware. So, we support reading
> +        * them but Linux will sanitize them when initializing through
> +        * recalc_rate.
> +        */
> +       { STPnHCK | 0, 1 }, { STPnHCK | 1, 2 },  { 2, 4 }, { 3, 8 }, { 4, 16 },
> +       /* Sentinel */
> +       { 0, 0 }
>  };
>
>  struct clk * __init cpg_sdh_clk_register(const char *name,

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