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

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Geert,

> Subject: RE: [PATCH v4] mmc: renesas_sdhi: Fix rounding errors
> 
> 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
> <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>
> > > 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.

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

> 
> >
> > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> >
> > > @@ -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);
> >
> > 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.
> 
> >
> > >         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))
> >                                      ^^^^^^^^^^^^^^^^ 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