| Message ID | 20220922085511.1078256-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] mmc: renesas_sdhi: Fix rounding errors | expand |
Hi, > 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. I really like the new variable names. > + * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = 533333332 Hz < (What is the '-' after 'eg:' for?) > + * 533333333 Hz) add an upper limit of 1/1024 rate higher to the clock > + * rate. I know Geert suggesgted this. I think, however, this description is too short. It should be mentioned IMHO that this rounding error can lead to the selection of a clock which is way off (the 400MHz one). I liked this example from v2. Geert, what do you say? Happy hacking, Wolfram
On Thu, Sep 22, 2022 at 09:55:11AM +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:- (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz). > > 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> Despite the discussion about the comments, the patch does not change any clock selection on my R-Car M3-N based Salvator-XS, both for eMMC and some SD cards. So: Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Hi Wolfram, Thanks for the feedback. > Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors > > Hi, > > > 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. > > I really like the new variable names. > > > + * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = 533333332 > Hz > > +< > > (What is the '-' after 'eg:' for?) Regarding 'eg:-', I got this habit from school days. On exams, I usually write for eg:- to provide some examples. OK, Will remove '-' after 'eg:'. > > > + * 533333333 Hz) add an upper limit of 1/1024 rate higher to the > clock > > + * rate. > > I know Geert suggesgted this. I think, however, this description is > too short. It should be mentioned IMHO that this rounding error can > lead to the selection of a clock which is way off (the 400MHz one). I > liked this example from v2. Geert, what do you say? Yes, I can put back the real example from v2, if that is useful. 533MHz->400 MHz is a big jump due to this rounding error and it has impacted the performance. Waiting for Geert's feedback. Cheers, Biju
Hi Biju, On Sun, Sep 25, 2022 at 7:06 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors > > > 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. > > > > I really like the new variable names. > > > > > + * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = 533333332 > > Hz > > > +< > > > > (What is the '-' after 'eg:' for?) > > Regarding 'eg:-', I got this habit from school days. On exams, I usually > write for eg:- to provide some examples. > > OK, Will remove '-' after 'eg:'. > > > > > > + * 533333333 Hz) add an upper limit of 1/1024 rate higher to the > > clock > > > + * rate. > > > > I know Geert suggesgted this. I think, however, this description is > > too short. It should be mentioned IMHO that this rounding error can > > lead to the selection of a clock which is way off (the 400MHz one). I > > liked this example from v2. Geert, what do you say? > > Yes, I can put back the real example from v2, if that is useful. > 533MHz->400 MHz is a big jump due to this rounding error and it has > impacted the performance. > > Waiting for Geert's feedback. I agree with Wolfram. I'll give your patch a try on my collective tomorrow. 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. > Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors > > Hi Biju, > > On Sun, Sep 25, 2022 at 7:06 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors > > > > > 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. > > > > > > I really like the new variable names. > > > > > > > + * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = > > > > + 533333332 > > > Hz > > > > +< > > > > > > (What is the '-' after 'eg:' for?) > > > > Regarding 'eg:-', I got this habit from school days. On exams, I > > usually write for eg:- to provide some examples. > > > > OK, Will remove '-' after 'eg:'. > > > > > > > > > + * 533333333 Hz) add an upper limit of 1/1024 rate higher to > > > > + the > > > clock > > > > + * rate. > > > > > > I know Geert suggesgted this. I think, however, this description > is > > > too short. It should be mentioned IMHO that this rounding error > can > > > lead to the selection of a clock which is way off (the 400MHz > one). > > > I liked this example from v2. Geert, what do you say? > > > > Yes, I can put back the real example from v2, if that is useful. > > 533MHz->400 MHz is a big jump due to this rounding error and it has > > impacted the performance. > > > > Waiting for Geert's feedback. > > I agree with Wolfram. OK, Will add example along with Tested-by tag from Wolfram. > I'll give your patch a try on my collective tomorrow. Thank you. Cheers, Biju
Hi Wolfram, > Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors > > On Thu, Sep 22, 2022 at 09:55:11AM +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:- (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz). > > > > 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> > > Despite the discussion about the comments, the patch does not change > any clock selection on my R-Car M3-N based Salvator-XS, both for eMMC > and some SD cards. So: > > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for testing. I have incorporated the review comments and send v4. Cheers, Biju
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 6edbf5c161ab..45ec15ece1f5 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,10 +154,15 @@ 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. + * + * To fix rounding errors, eg:- (533333333 Hz / 4 * 4 = 533333332 Hz < + * 533333333 Hz) add an upper limit of 1/1024 rate higher to the clock + * rate. */ + new_upper_limit = (new_clock << i) + ((new_clock << i) >> 10); 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)) { + 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)) @@ -181,6 +187,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 +201,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:- (533333333 Hz / 4 * 4 = 533333332 Hz < 533333333 Hz). 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> --- 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 | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)