[PATCH v4] mmc: renesas_sdhi: Fix rounding errors

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>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
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 | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 6edbf5c161ab..d1b8130ee37f 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,10 +154,17 @@ 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)).
 	 */
+	new_upper_limit = (new_clock << i) + ((new_clock << i) >> 10);
 	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)) {
+		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))

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

On Mon, Sep 26, 2022 at 06:10:02PM +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>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Looks very good to me now! Thanks for keeping at it:

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

Hi Biju,

On Mon, Sep 26, 2022 at 7:10 PM Biju Das [off-list ref] 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>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
v3->v4:
 * Added Tested-by tag from Wolfram.
 * Updated commit description and code comment with real example.

Thanks for the update!

Unfortunately this patch causes a change in the clock frequencies
used on R-Car M2-W:

    -clk_summary:          sd0                 97500000
    +clk_summary:          sd0                 32500000
    -clk_summary:             sdhi0            97500000
    +clk_summary:             sdhi0            32500000

    -clk_summary:             sd3              12786885
    +clk_summary:             sd3              12187500
    -clk_summary:                sdhi3         12786885
    +clk_summary:                sdhi3         12187500

    -clk_summary:             sd2              12786885
    +clk_summary:             sd2              12187500
    -clk_summary:                sdhi2         12786885
    +clk_summary:                sdhi2         12187500

Mea culpa: while new_clock is a constant inside the loop, i is not!
So it should be moved back inside the loop below.
With that change, R-Car M2-W is happy again, and I noticed no
regression on R-Car H3 ES2.0.

                                     ^^^^^^^^^^^^^^^^
Probably this should become new_upper_limit too, for consistency?
It doesn't seem to matter in my testing, though.

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

Hi Geert,

Thanks for the testing.

Subject: Re: [PATCH v4] mmc: renesas_sdhi: Fix rounding errors

Hi Biju,

On Mon, Sep 26, 2022 at 7:10 PM Biju Das [off-list ref]
wrote:

quoted

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>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
v3->v4:
 * Added Tested-by tag from Wolfram.
 * Updated commit description and code comment with real example.

Thanks for the update!

Unfortunately this patch causes a change in the clock frequencies used
on R-Car M2-W:

    -clk_summary:          sd0                 97500000
    +clk_summary:          sd0                 32500000
    -clk_summary:             sdhi0            97500000
    +clk_summary:             sdhi0            32500000

    -clk_summary:             sd3              12786885
    +clk_summary:             sd3              12187500
    -clk_summary:                sdhi3         12786885
    +clk_summary:                sdhi3         12187500

    -clk_summary:             sd2              12786885
    +clk_summary:             sd2              12187500
    -clk_summary:                sdhi2         12786885
    +clk_summary:                sdhi2         12187500

That is not good.

quoted

--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c

quoted

@@ -153,10 +154,17 @@ static unsigned int

renesas_sdhi_clk_update(struct tmio_mmc_host *host,

quoted

         * 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

quoted

+        * clk rate jumping to lower rate due to rounding error (eg:

RZ/G2L has

quoted

+        * 3 clk sources 533.333333 MHz, 400 MHz and 266.666666 MHz.

The request

quoted

+        * for 533.333333 MHz will selects a slower 400 MHz due to

rounding

quoted

+        * error (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333

Hz)).

quoted

         */
+       new_upper_limit = (new_clock << i) + ((new_clock << i) >>

10);

Mea culpa: while new_clock is a constant inside the loop, i is not!
So it should be moved back inside the loop below.
With that change, R-Car M2-W is happy again, and I noticed no
regression on R-Car H3 ES2.0.

OK.

quoted

        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)) {
+               if (freq > new_upper_limit) {
                        /* Too fast; look for a slightly slower

option */

quoted

                        freq = clk_round_rate(ref_clk, (new_clock <<

i) / 4 * 3);

quoted

                        if (freq > (new_clock << i))

                                     ^^^^^^^^^^^^^^^^ Probably this
should become new_upper_limit too, for consistency?
It doesn't seem to matter in my testing, though.

OK. Will do the below change in next version.

-	new_upper_limit = (new_clock << i) + ((new_clock << i) >> 10);
 	for (i = min(9, ilog2(UINT_MAX / new_clock)); i >= 0; i--) {
 		freq = clk_round_rate(ref_clk, 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;
 		}

Cheers,
Biju

Hi Geert,

Subject: RE: [PATCH v4] mmc: renesas_sdhi: Fix rounding errors

Hi Geert,

Thanks for the testing.

quoted

Subject: Re: [PATCH v4] mmc: renesas_sdhi: Fix rounding errors

Hi Biju,

On Mon, Sep 26, 2022 at 7:10 PM Biju Das

[off-list ref]

quoted

wrote:

quoted

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

quoted

quoted

the clock rate.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
v3->v4:
 * Added Tested-by tag from Wolfram.
 * Updated commit description and code comment with real example.

Thanks for the update!

Unfortunately this patch causes a change in the clock frequencies

used

quoted

on R-Car M2-W:

    -clk_summary:          sd0                 97500000
    +clk_summary:          sd0                 32500000
    -clk_summary:             sdhi0            97500000
    +clk_summary:             sdhi0            32500000

    -clk_summary:             sd3              12786885
    +clk_summary:             sd3              12187500
    -clk_summary:                sdhi3         12786885
    +clk_summary:                sdhi3         12187500

    -clk_summary:             sd2              12786885
    +clk_summary:             sd2              12187500
    -clk_summary:                sdhi2         12786885
    +clk_summary:                sdhi2         12187500

That is not good.

Other than this, it is better to test performance as well to
check any regression due to second fix in this patch.

Note:
After mounting, I use below command to check performance.
dd if=/dev/zero of=test oflag=direct bs=8M count=64

Cheers,
Biju

quoted

quoted

--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c

quoted

@@ -153,10 +154,17 @@ static unsigned int

renesas_sdhi_clk_update(struct tmio_mmc_host *host,

quoted

         * greater than, new_clock.  As we can divide by 1 << i

for

quoted

quoted

         * any i in [0, 9] we want the input clock to be as close

as

quoted

quoted

         * possible, but no greater than, new_clock << i.
+        *
+        * Add an upper limit of 1/1024 rate higher to the clock

rate to fix

quoted

+        * clk rate jumping to lower rate due to rounding error

(eg:

quoted

RZ/G2L has

quoted

+        * 3 clk sources 533.333333 MHz, 400 MHz and 266.666666

MHz.

quoted

The request

quoted

+        * for 533.333333 MHz will selects a slower 400 MHz due to

rounding

quoted

+        * error (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333

Hz)).

quoted

         */
+       new_upper_limit = (new_clock << i) + ((new_clock << i) >>

10);

Mea culpa: while new_clock is a constant inside the loop, i is not!
So it should be moved back inside the loop below.
With that change, R-Car M2-W is happy again, and I noticed no
regression on R-Car H3 ES2.0.

OK.

quoted

quoted

        for (i = min(9, ilog2(UINT_MAX / new_clock)); i >= 0; i--)

{

quoted

quoted

                freq = clk_round_rate(ref_clk, new_clock << i);
-               if (freq > (new_clock << i)) {
+               if (freq > new_upper_limit) {
                        /* Too fast; look for a slightly slower

option */

quoted

                        freq = clk_round_rate(ref_clk, (new_clock

<<

quoted

i) / 4 * 3);

quoted

                        if (freq > (new_clock << i))

                                     ^^^^^^^^^^^^^^^^ Probably this
should become new_upper_limit too, for consistency?
It doesn't seem to matter in my testing, though.

OK. Will do the below change in next version.

-	new_upper_limit = (new_clock << i) + ((new_clock << i) >> 10);
 	for (i = min(9, ilog2(UINT_MAX / new_clock)); i >= 0; i--) {
 		freq = clk_round_rate(ref_clk, 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;
 		}

Cheers,
Biju