Hi Biju,
On Mon, Sep 19, 2022 at 10:48 AM 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 this
> clock. On RZ/G2L platforms, due to lack of this margin leads to the
> selection of a clock source with a lower clock rate compared to a higher
> one.
>
> This patch adds a margin of (1/1024) higher to hs clock rate and there
> by avoid looking for a smaller clock option.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v1->v2:
> * Add a comment explaining why margin is needed and set it to
> that particular value.
Thanks for the update!
> --- 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,16 @@ 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)) {
> + /*
> + * Add a margin of 1/1024 rate higher to the clock rate in order
> + * to avoid looking for a slower clock on boundary cases (eg:
> + * RZ/G2L has 3 clk sources 533.333333 MHz, 400 MHz and
> + * 266.666666 MHz. The request for 533.333332 MHz will look for
> + * slower option and it selects a slower 400 MHz source clock
> + * instead of 533.333333 MHz).
> + */
> + new_clock_margin = (new_clock << i) + ((new_clock << i) >> 10);
Unlike in the case below, "new_clock_margin" is not the margin, but
the actual value plus the margin. So perhaps "new_upper_limit"?
Also, this value is constant inside the loop, so its calculation can
be moved outside, and its comment can be combined with the existing
comment just before the loop.
I would also say something about rounding errors, as that is what is
really the problem (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz).
> + 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))
> @@ -181,6 +191,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 +205,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 */
The rest LGTM.
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
