[PATCH v5] mmc: renesas_sdhi: Fix rounding errors

[PATCH v5] mmc: renesas_sdhi: Fix rounding errors

[Biju Das]

Due to clk rounding errors on RZ/G2L platforms, it selects a clock source
with a lower clock rate compared to a higher one.
For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333
33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz.

This patch fixes this issue by adding a margin of (1/1024) higher to
the clock rate.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4->v5:
 * Moved upper limit calculation inside the for loop as it caused
   regression on R-Car M2-W board.
 * Removed Rb tag from Wolfram as there is some new changes.
v3->v4:
 * Added Tested-by tag from Wolfram.
 * Updated commit description and code comment with real example.
v2->v3:
 * Renamed the variable new_clock_margin->new_upper_limit in renesas_sdhi_clk_
   update()
 * Moved setting of new_upper_limit outside for loop.
 * Updated the comment section to mention the rounding errors and merged with
   existing comment out side the for loop.
 * Updated commit description. 
v1->v2:
 * Add a comment explaining why margin is needed and set it to
   that particular value.
---
 drivers/mmc/host/renesas_sdhi_core.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 6edbf5c161ab..b970699743e0 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -128,6 +128,7 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
 	struct clk *ref_clk = priv->clk;
 	unsigned int freq, diff, best_freq = 0, diff_min = ~0;
 	unsigned int new_clock, clkh_shift = 0;
+	unsigned int new_upper_limit;
 	int i;
 
 	/*
@@ -153,13 +154,20 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
 	 * greater than, new_clock.  As we can divide by 1 << i for
 	 * any i in [0, 9] we want the input clock to be as close as
 	 * possible, but no greater than, new_clock << i.
+	 *
+	 * Add an upper limit of 1/1024 rate higher to the clock rate to fix
+	 * clk rate jumping to lower rate due to rounding error (eg: RZ/G2L has
+	 * 3 clk sources 533.333333 MHz, 400 MHz and 266.666666 MHz. The request
+	 * for 533.333333 MHz will selects a slower 400 MHz due to rounding
+	 * error (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz)).
 	 */
 	for (i = min(9, ilog2(UINT_MAX / new_clock)); i >= 0; i--) {
 		freq = clk_round_rate(ref_clk, new_clock << i);
-		if (freq > (new_clock << i)) {
+		new_upper_limit = (new_clock << i) + ((new_clock << i) >> 10);
+		if (freq > new_upper_limit) {
 			/* Too fast; look for a slightly slower option */
 			freq = clk_round_rate(ref_clk, (new_clock << i) / 4 * 3);
-			if (freq > (new_clock << i))
+			if (freq > new_upper_limit)
 				continue;
 		}
 
@@ -181,6 +189,7 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
 static void renesas_sdhi_set_clock(struct tmio_mmc_host *host,
 				   unsigned int new_clock)
 {
+	unsigned int clk_margin;
 	u32 clk = 0, clock;
 
 	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
@@ -194,7 +203,13 @@ static void renesas_sdhi_set_clock(struct tmio_mmc_host *host,
 	host->mmc->actual_clock = renesas_sdhi_clk_update(host, new_clock);
 	clock = host->mmc->actual_clock / 512;
 
-	for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
+	/*
+	 * Add a margin of 1/1024 rate higher to the clock rate in order
+	 * to avoid clk variable setting a value of 0 due to the margin
+	 * provided for actual_clock in renesas_sdhi_clk_update().
+	 */
+	clk_margin = new_clock >> 10;
+	for (clk = 0x80000080; new_clock + clk_margin >= (clock << 1); clk >>= 1)
 		clock <<= 1;
 
 	/* 1/1 clock is option */
-- 
2.25.1

Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors

[Geert Uytterhoeven]

On Wed, Sep 28, 2022 at 1:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Due to clk rounding errors on RZ/G2L platforms, it selects a clock source
> with a lower clock rate compared to a higher one.
> For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333
> 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz.
>
> This patch fixes this issue by adding a margin of (1/1024) higher to
> the clock rate.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v4->v5:
>  * Moved upper limit calculation inside the for loop as it caused
>    regression on R-Car M2-W board.
>  * Removed Rb tag from Wolfram as there is some new changes.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Still works fine on R-Car Gen2/Gen3, so:
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v5] mmc: renesas_sdhi: Fix rounding errors

[Wolfram Sang]

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

On Fri, Sep 30, 2022 at 11:09:07AM +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 28, 2022 at 1:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Due to clk rounding errors on RZ/G2L platforms, it selects a clock source
> > with a lower clock rate compared to a higher one.
> > For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333
> > 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz.
> >
> > This patch fixes this issue by adding a margin of (1/1024) higher to
> > the clock rate.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v4->v5:
> >  * Moved upper limit calculation inside the for loop as it caused
> >    regression on R-Car M2-W board.
> >  * Removed Rb tag from Wolfram as there is some new changes.
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Still works fine on R-Car Gen2/Gen3, so:
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

I'll test this patch on Monday.


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

Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors

[Wolfram Sang]

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

On Wed, Sep 28, 2022 at 12:07:55PM +0100, Biju Das wrote:
> Due to clk rounding errors on RZ/G2L platforms, it selects a clock source
> with a lower clock rate compared to a higher one.
> For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333
> 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz.
> 
> This patch fixes this issue by adding a margin of (1/1024) higher to
> the clock rate.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Can only test on Gen3 currently, but clock settings are the same there.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks!


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

Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors

[Ulf Hansson]

On Mon, 3 Oct 2022 at 19:29, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> On Wed, Sep 28, 2022 at 12:07:55PM +0100, Biju Das wrote:
> > Due to clk rounding errors on RZ/G2L platforms, it selects a clock source
> > with a lower clock rate compared to a higher one.
> > For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333
> > 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz.
> >
> > This patch fixes this issue by adding a margin of (1/1024) higher to
> > the clock rate.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> Can only test on Gen3 currently, but clock settings are the same there.
>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Thanks!
>

Can someone tell me, if there is a corresponding fixes tag we could use here?

Or is this just a general bugfix that we should tag for stable?

Kind regards
Uffe

Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors

[Wolfram Sang]

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


> Or is this just a general bugfix that we should tag for stable?

I'd think this because I assume there is no commit causing the rounding
errors. But maybe Biju has something to add?


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

RE: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors

[Biju Das]

Hi Wolfram Sang & Ulf Hansson,

> Subject: Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors
> 
> 
> > Or is this just a general bugfix that we should tag for stable?
> 
> I'd think this because I assume there is no commit causing the
> rounding errors. But maybe Biju has something to add?

There is rounding error present since commit [1], but no HW at that time to
introduce the error. Then we added RZ/G2L support and we started seeing this
issue after [2].

So may be treat this an enhancement patch or fixes to [1] or [2]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/renesas_sdhi_core.c?h=v6.0&id=bb6d3fa98a418b071c5f735e75558604f5f4af66
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/renesas/r9a07g044-cpg.c?h=v6.0&id=b289cdecc7c3e25e001cde260c882e4d9a8b0772

Cheers,
Biju

Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors

[Ulf Hansson]

On Thu, 6 Oct 2022 at 18:07, Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Wolfram Sang & Ulf Hansson,
>
> > Subject: Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors
> >
> >
> > > Or is this just a general bugfix that we should tag for stable?
> >
> > I'd think this because I assume there is no commit causing the
> > rounding errors. But maybe Biju has something to add?
>
> There is rounding error present since commit [1], but no HW at that time to
> introduce the error. Then we added RZ/G2L support and we started seeing this
> issue after [2].
>
> So may be treat this an enhancement patch or fixes to [1] or [2]

Alright, I have added a fixes tag [1] and a stable tag - and applied
it for fixes, thanks!

Kind regards
Uffe

>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/renesas_sdhi_core.c?h=v6.0&id=bb6d3fa98a418b071c5f735e75558604f5f4af66
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/renesas/r9a07g044-cpg.c?h=v6.0&id=b289cdecc7c3e25e001cde260c882e4d9a8b0772
>
> Cheers,
> Biju
>