[PATCH v3] mmc: renesas_sdhi: Fix rounding errors

[PATCH v3] 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:- (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz).

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>
---
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 | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 6edbf5c161ab..45ec15ece1f5 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,15 @@ 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.
+	 *
+	 * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = 533333332 Hz <
+	 * 533333333 Hz) add an upper limit of 1/1024 rate higher to the clock
+	 * rate.
 	 */
+	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 +187,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 +201,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 v3] mmc: renesas_sdhi: Fix rounding errors

[Wolfram Sang]

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

Hi,

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

I really like the new variable names.

> +	 * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = 533333332 Hz <

(What is the '-' after 'eg:' for?)

> +	 * 533333333 Hz) add an upper limit of 1/1024 rate higher to the clock
> +	 * rate.

I know Geert suggesgted this. I think, however, this description is too
short. It should be mentioned IMHO that this rounding error can lead to
the selection of a clock which is way off (the 400MHz one). I liked this
example from v2. Geert, what do you say?

Happy hacking,

   Wolfram


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

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

[Wolfram Sang]

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

On Thu, Sep 22, 2022 at 09:55:11AM +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:- (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz).
> 
> 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>

Despite the discussion about the comments, the patch does not change any
clock selection on my R-Car M3-N based Salvator-XS, both for eMMC and
some SD cards. So:

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


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

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

[Biju Das]

Hi Wolfram,

Thanks for the feedback.

> Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
> 
> Hi,
> 
> > 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.
> 
> I really like the new variable names.
> 
> > +	 * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = 533333332
> Hz
> > +<
> 
> (What is the '-' after 'eg:' for?)

Regarding 'eg:-', I got this habit from school days. On exams, I usually
write for eg:- to provide some examples.

OK, Will remove '-' after 'eg:'.

> 
> > +	 * 533333333 Hz) add an upper limit of 1/1024 rate higher to the
> clock
> > +	 * rate.
> 
> I know Geert suggesgted this. I think, however, this description is
> too short. It should be mentioned IMHO that this rounding error can
> lead to the selection of a clock which is way off (the 400MHz one). I
> liked this example from v2. Geert, what do you say?

Yes, I can put back the real example from v2, if that is useful.
533MHz->400 MHz is a big jump due to this rounding error and it has
impacted the performance.

Waiting for Geert's feedback.

Cheers,
Biju

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

[Geert Uytterhoeven]

Hi Biju,

On Sun, Sep 25, 2022 at 7:06 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors

> > > 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.
> >
> > I really like the new variable names.
> >
> > > +    * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = 533333332
> > Hz
> > > +<
> >
> > (What is the '-' after 'eg:' for?)
>
> Regarding 'eg:-', I got this habit from school days. On exams, I usually
> write for eg:- to provide some examples.
>
> OK, Will remove '-' after 'eg:'.
>
> >
> > > +    * 533333333 Hz) add an upper limit of 1/1024 rate higher to the
> > clock
> > > +    * rate.
> >
> > I know Geert suggesgted this. I think, however, this description is
> > too short. It should be mentioned IMHO that this rounding error can
> > lead to the selection of a clock which is way off (the 400MHz one). I
> > liked this example from v2. Geert, what do you say?
>
> Yes, I can put back the real example from v2, if that is useful.
> 533MHz->400 MHz is a big jump due to this rounding error and it has
> impacted the performance.
>
> Waiting for Geert's feedback.

I agree with Wolfram.
I'll give your patch a try on my collective tomorrow.

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

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
> 
> Hi Biju,
> 
> On Sun, Sep 25, 2022 at 7:06 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
> 
> > > > 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.
> > >
> > > I really like the new variable names.
> > >
> > > > +    * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 =
> > > > + 533333332
> > > Hz
> > > > +<
> > >
> > > (What is the '-' after 'eg:' for?)
> >
> > Regarding 'eg:-', I got this habit from school days. On exams, I
> > usually write for eg:- to provide some examples.
> >
> > OK, Will remove '-' after 'eg:'.
> >
> > >
> > > > +    * 533333333 Hz) add an upper limit of 1/1024 rate higher to
> > > > + the
> > > clock
> > > > +    * rate.
> > >
> > > I know Geert suggesgted this. I think, however, this description
> is
> > > too short. It should be mentioned IMHO that this rounding error
> can
> > > lead to the selection of a clock which is way off (the 400MHz
> one).
> > > I liked this example from v2. Geert, what do you say?
> >
> > Yes, I can put back the real example from v2, if that is useful.
> > 533MHz->400 MHz is a big jump due to this rounding error and it has
> > impacted the performance.
> >
> > Waiting for Geert's feedback.
> 
> I agree with Wolfram.

OK, Will add example along with Tested-by tag from Wolfram.

> I'll give your patch a try on my collective tomorrow.

Thank you.

Cheers,
Biju

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

[Biju Das]

Hi Wolfram,

> Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
> 
> On Thu, Sep 22, 2022 at 09:55:11AM +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:- (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz).
> >
> > 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>
> 
> Despite the discussion about the comments, the patch does not change
> any clock selection on my R-Car M3-N based Salvator-XS, both for eMMC
> and some SD cards. So:
> 
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for testing. I have incorporated the review comments and send v4.

Cheers,
Biju