| Message ID | 20220919084838.3066429-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] mmc: renesas_sdhi: Add clk margin for sdhi hs clock | expand |
Hi Biju, On Mon, Sep 19, 2022 at 10:48 AM 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 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@bp.renesas.com> > --- > 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@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
Hi Geert, Thanks for the feedback. > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > Subject: Re: [PATCH v2] mmc: renesas_sdhi: Add clk margin for sdhi hs > clock > > Hi Biju, > > On Mon, Sep 19, 2022 at 10:48 AM 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 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@bp.renesas.com> > > --- > > 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"? OK. > > 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. OK. > > I would also say something about rounding errors, as that is what is > really the problem (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 > Hz). OK Agreed. Will send v3 with these changes. Cheers, Biju > > > + 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@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
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 6edbf5c161ab..14f41398d3a0 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,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); + 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 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@bp.renesas.com> --- v1->v2: * Add a comment explaining why margin is needed and set it to that particular value. --- drivers/mmc/host/renesas_sdhi_core.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)