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