Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH] mmc: renesas_sdhi: Add margins for main clock and hs
> clock
>
> Hi Biju,
>
> On Tue, Sep 13, 2022 at 5:15 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
>
> 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.
Cheers,
Biju
>
> > if (priv->clkh)
> > clk_set_rate(priv->clk, best_freq >> clkh_shift);
>
> 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
