RE: [PATCH] mmc: renesas_sdhi: Add margins for main clock and hs clock

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

Hi Geert,

> Subject: RE: [PATCH] mmc: renesas_sdhi: Add margins for main clock and
> hs clock
> 
> Hi Geert,
> 
> > Subject: RE: [PATCH] mmc: renesas_sdhi: Add margins for main clock
> and
> > hs clock
> >
> > Hi Geert,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH] mmc: renesas_sdhi: Add margins for main clock
> > and
> > > hs clock
> > >
> > > Hi Biju,
> > >
> > > On Wed, Sep 14, 2022 at 6:44 AM Biju Das
> > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > Subject: RE: [PATCH] mmc: renesas_sdhi: Add margins for main
> > clock
> > > > > and hs clock
> > > > > > Subject: Re: [PATCH] mmc: renesas_sdhi: Add margins for main
> > > clock
> > > > > > and hs clock On Tue, Sep 13, 2022 at 5:15 PM Biju Das
> > > > > > <biju.das.jz@bp.renesas.com>
> > > > > > wrote:
> > > > > > > The SDHI high speed clock is 4 times that of the main
> clock.
> > > > > > > Currently, there is no margin added for setting the rate
> > > > > > > associated with these clocks. On RZ/G2L platforms, the
> lack
> > of
> > > > > > > these margins leads to the selection of a clock source
> with
> > a
> > > > > > > lower clock rate compared to a higher one.
> > > > > > >
> > > > > > > RZ/G2L example case:
> > > > > > > SD0 hs clock is 533333333 Hz and SD0 main clock is
> 133333333
> > > Hz.
> > > > > > > When we set the rate for the main clock 133333333, the
> > request
> > > > > > > rate for the parent becomes 533333332 (133333333 * 4) and
> > the
> > > > > > > SD0 hs clock is set as 400000000 Hz.
> > > > > > >
> > > > > > > This patch adds a margin of (1/1024) higher hs clock and
> > main
> > > > > > > clock
> > > > > > rate.
> > > > > > >
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > >
> > > > > > Thanks for your patch!
> > > > > >
> > > > > > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > > > > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > > > > > > @@ -147,6 +147,7 @@ static unsigned int
> > > > > > > renesas_sdhi_clk_update(struct
> > > > > > tmio_mmc_host *host,
> > > > > > >         }
> > > > > > >
> > > > > > >         new_clock = wanted_clock << clkh_shift;
> > > > > > > +       new_clock += new_clock >> 10;
> > > > > >
> > > > > > This changes the requested clock rate. Is that really a good
> > > idea?
> > > > > >
> > > > > > Isn't it sufficient to change the test "if (freq >
> (new_clock
> > <<
> > > i))"
> > > > > > slightly?
> > > > >
> > > > > We hardly enter this test, after it request for proper
> > wanted_clk.
> > > > >
> > > > > freq is clk_round_rate(ref_clk, new_clock << i);  and it
> > compares
> > > > > 400MHz vs 533.332 MHz.
> > > > >
> > > > > Basically wanted_clock= 133.33333 MHz and is scaled to power
> of
> > 2
> > > > > and then each iteration it scale down to power of 2 compare
> with
> > > > > round rate value.
> > > > >
> > > > > [    4.020781] ####rzg2l_cpg_sd_clk_mux_determine_rate#######
> > req-
> > > > > >rate=4266666656
> > > > > [    4.028013] ####rzg2l_cpg_sd_clk_mux_determine_rate#######
> > req-
> > > > > >rate=2133333328
> > > > > [    4.035330] ####rzg2l_cpg_sd_clk_mux_determine_rate#######
> > req-
> > > > > >rate=1066666664
> > > > > [    4.042639] ####rzg2l_cpg_sd_clk_mux_determine_rate#######
> > req-
> > > > > >rate=533333332
> > > > >
> > > > > >
> > > > > > >
> > > > > > >         /*
> > > > > > >          * We want the bus clock to be as close as
> possible
> > > to,
> > > > > > > but no @@ -172,6 +173,7 @@ static unsigned int
> > > > > > > renesas_sdhi_clk_update(struct tmio_mmc_host *host,
> > > > > > >
> > > > > > >         clk_set_rate(ref_clk, best_freq);
> > > > > > >
> > > > > > > +       best_freq += best_freq >> 10;
> > > > > >
> > > > > > Can you please elaborate why this is needed?
> > > > > > It looks counter-intuitive to me.
> > > > >
> > > > > When we try to set 133.333 MHz clk, the determine rate
> > calculates
> > > > > req-
> > > > > >rate for sd clk as 533.332 and since available clock source
> for
> > > sd0
> > > > > >clks
> > > > > are 266.6666, 400 and
> > > > > 533.333 MHz, so it select the clock source as 400MHz.
> > > >
> > > > Just to add here the main issue is coming from math calculation.
> > > >
> > > > Consider any value A
> > > >
> > > > B = A / 4
> > > > C = B * 4
> > > >
> > > > Ideally, we expect A = C, But in the below example It is not the
> > > case
> > > > (it is A != C).
> > > >
> > > > A = 533333333 (1600/3 MHz)
> > > > B = 533333333/4 = 133333333
> > > > C = 133333333 * 4 = 533333332
> > > >
> > > > Since A != C we are ending up in this situation.
> > >
> > > I am fully aware of that ;-)
> > >
> > > However, clk_round_rate() is supposed to return the closest
> matching
> > > rate.  Hence when passed 533333332, it should return 533333333.
> >
> > No, it is returning 400000000.as ref_clk->rate=400000000, and new
> clk
> > rate 533333332
> >
> > [    4.524188] ###renesas_sdhi_clk_update ++ ###
> > 400000000/533333333/533333332
> > [    4.531217] ###renesas_sdhi_clk_update -- ###
> > 400000000/400000000/533333332
> >
> > +               pr_err("###%s ++ ### %llu/%llu/%llu", __func__,
> > + clk_get_rate(ref_clk),freq, new_clock << i);
> >                 freq = clk_round_rate(ref_clk, new_clock << i);
> > +               pr_err("###%s -- ### %llu/%llu/%llu", __func__,
> > + clk_get_rate(ref_clk),freq, new_clock << i);
> >
> > > AFAIU, this is then rejected by "if (freq > (new_clock << i))",
> due
> > to
> > > being slightly too large, causing 400000000 to be picked instead.
> > >
> > > Am I missing something?
> >
> > With the above, it skips the if statement
> >
> > > I'm currently not in the position to test/verify what is actually
> > > happening.
> >
> > OK Will check.
> 
> I have patched SDHI driver to allow margin, so that it won't set a
> lower clock and added round clock operation in Clock driver for
> rounding operation.
> with that I get proper rates.
> 
> Is this solution acceptable or not?
> 
> [1]
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> b/drivers/mmc/host/renesas_sdhi_core.c
> index 6edbf5c161ab..bb7a736d46a2 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_clock_margin;
>         int i;
> 
>         /*
> @@ -155,8 +156,13 @@ static unsigned int
> renesas_sdhi_clk_update(struct tmio_mmc_host *host,
>          * possible, but no greater than, new_clock << i.
>          */
>         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_clock << i))
> +                       freq = clk_round_rate(ref_clk, new_clock <<
> i);
> +
> +               new_clock_margin = (new_clock << i);
> +               new_clock_margin += new_clock_margin >> 10;
> +
> +               if (freq > new_clock_margin) {
>                         /* Too fast; look for a slightly slower option
> */
>                         freq = clk_round_rate(ref_clk, (new_clock <<
> i) / 4 * 3);
>                         if (freq > (new_clock << i))

Looks like for SDHI, the below clock margin change should be sufficient.
As the round operation in RZ/G2L clk driver changes[2] rounds to 
nearest clk(ie,533.332MHZ to 533.333MHz)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 6edbf5c161ab..539521f84e2f 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_clock_margin;
        int i;
 
        /*
@@ -156,7 +157,9 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
         */
        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_clock_margin = (new_clock << i) + ((new_clock << i) >> 10);
+
+               if (freq > new_clock_margin) {

Cheers,
Biju