| Message ID | 20220928110755.849275-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] mmc: renesas_sdhi: Fix rounding errors | expand |
On Wed, Sep 28, 2022 at 1:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > Due to clk rounding errors on RZ/G2L platforms, it selects a clock source > with a lower clock rate compared to a higher one. > For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333 > 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz. > > This patch fixes this issue by adding a margin of (1/1024) higher to > the clock rate. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v4->v5: > * Moved upper limit calculation inside the for loop as it caused > regression on R-Car M2-W board. > * Removed Rb tag from Wolfram as there is some new changes. Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Still works fine on R-Car Gen2/Gen3, so: Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> 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
On Fri, Sep 30, 2022 at 11:09:07AM +0200, Geert Uytterhoeven wrote: > On Wed, Sep 28, 2022 at 1:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Due to clk rounding errors on RZ/G2L platforms, it selects a clock source > > with a lower clock rate compared to a higher one. > > For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333 > > 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz. > > > > This patch fixes this issue by adding a margin of (1/1024) higher to > > the clock rate. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v4->v5: > > * Moved upper limit calculation inside the for loop as it caused > > regression on R-Car M2-W board. > > * Removed Rb tag from Wolfram as there is some new changes. > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Still works fine on R-Car Gen2/Gen3, so: > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> I'll test this patch on Monday.
On Wed, Sep 28, 2022 at 12:07:55PM +0100, Biju Das wrote: > Due to clk rounding errors on RZ/G2L platforms, it selects a clock source > with a lower clock rate compared to a higher one. > For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333 > 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz. > > This patch fixes this issue by adding a margin of (1/1024) higher to > the clock rate. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Can only test on Gen3 currently, but clock settings are the same there. Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks!
On Mon, 3 Oct 2022 at 19:29, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > On Wed, Sep 28, 2022 at 12:07:55PM +0100, Biju Das wrote: > > Due to clk rounding errors on RZ/G2L platforms, it selects a clock source > > with a lower clock rate compared to a higher one. > > For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333 > > 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz. > > > > This patch fixes this issue by adding a margin of (1/1024) higher to > > the clock rate. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Can only test on Gen3 currently, but clock settings are the same there. > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Thanks! > Can someone tell me, if there is a corresponding fixes tag we could use here? Or is this just a general bugfix that we should tag for stable? Kind regards Uffe
> Or is this just a general bugfix that we should tag for stable?
I'd think this because I assume there is no commit causing the rounding
errors. But maybe Biju has something to add?
Hi Wolfram Sang & Ulf Hansson, > Subject: Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors > > > > Or is this just a general bugfix that we should tag for stable? > > I'd think this because I assume there is no commit causing the > rounding errors. But maybe Biju has something to add? There is rounding error present since commit [1], but no HW at that time to introduce the error. Then we added RZ/G2L support and we started seeing this issue after [2]. So may be treat this an enhancement patch or fixes to [1] or [2] [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/renesas_sdhi_core.c?h=v6.0&id=bb6d3fa98a418b071c5f735e75558604f5f4af66 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/renesas/r9a07g044-cpg.c?h=v6.0&id=b289cdecc7c3e25e001cde260c882e4d9a8b0772 Cheers, Biju
On Thu, 6 Oct 2022 at 18:07, Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Wolfram Sang & Ulf Hansson, > > > Subject: Re: [PATCH v5] mmc: renesas_sdhi: Fix rounding errors > > > > > > > Or is this just a general bugfix that we should tag for stable? > > > > I'd think this because I assume there is no commit causing the > > rounding errors. But maybe Biju has something to add? > > There is rounding error present since commit [1], but no HW at that time to > introduce the error. Then we added RZ/G2L support and we started seeing this > issue after [2]. > > So may be treat this an enhancement patch or fixes to [1] or [2] Alright, I have added a fixes tag [1] and a stable tag - and applied it for fixes, thanks! Kind regards Uffe > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/renesas_sdhi_core.c?h=v6.0&id=bb6d3fa98a418b071c5f735e75558604f5f4af66 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/renesas/r9a07g044-cpg.c?h=v6.0&id=b289cdecc7c3e25e001cde260c882e4d9a8b0772 > > Cheers, > Biju >
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 6edbf5c161ab..b970699743e0 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_upper_limit; int i; /* @@ -153,13 +154,20 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host, * greater than, new_clock. As we can divide by 1 << i for * any i in [0, 9] we want the input clock to be as close as * possible, but no greater than, new_clock << i. + * + * Add an upper limit of 1/1024 rate higher to the clock rate to fix + * clk rate jumping to lower rate due to rounding error (eg: RZ/G2L has + * 3 clk sources 533.333333 MHz, 400 MHz and 266.666666 MHz. The request + * for 533.333333 MHz will selects a slower 400 MHz due to rounding + * error (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz)). */ 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_upper_limit = (new_clock << i) + ((new_clock << i) >> 10); + if (freq > new_upper_limit) { /* Too fast; look for a slightly slower option */ freq = clk_round_rate(ref_clk, (new_clock << i) / 4 * 3); - if (freq > (new_clock << i)) + if (freq > new_upper_limit) continue; } @@ -181,6 +189,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 +203,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 */
Due to clk rounding errors on RZ/G2L platforms, it selects a clock source with a lower clock rate compared to a higher one. For eg: The rounding error (533333333 Hz / 4 * 4 = 533333332 Hz < 5333333 33 Hz) selects a clk source of 400 MHz instead of 533.333333 MHz. This patch fixes this issue by adding a margin of (1/1024) higher to the clock rate. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v4->v5: * Moved upper limit calculation inside the for loop as it caused regression on R-Car M2-W board. * Removed Rb tag from Wolfram as there is some new changes. v3->v4: * Added Tested-by tag from Wolfram. * Updated commit description and code comment with real example. v2->v3: * Renamed the variable new_clock_margin->new_upper_limit in renesas_sdhi_clk_ update() * Moved setting of new_upper_limit outside for loop. * Updated the comment section to mention the rounding errors and merged with existing comment out side the for loop. * Updated commit description. 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, 18 insertions(+), 3 deletions(-)