* [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
@ 2022-09-22 8:55 Biju Das
2022-09-24 10:18 ` Wolfram Sang
2022-09-25 16:51 ` Wolfram Sang
0 siblings, 2 replies; 7+ messages in thread
From: Biju Das @ 2022-09-22 8:55 UTC (permalink / raw)
To: Ulf Hansson
Cc: Biju Das, Wolfram Sang, linux-mmc, linux-renesas-soc,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
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(-)
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 */
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
2022-09-22 8:55 [PATCH v3] mmc: renesas_sdhi: Fix rounding errors Biju Das
@ 2022-09-24 10:18 ` Wolfram Sang
2022-09-25 17:06 ` Biju Das
2022-09-25 16:51 ` Wolfram Sang
1 sibling, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2022-09-24 10:18 UTC (permalink / raw)
To: Biju Das
Cc: Ulf Hansson, linux-mmc, linux-renesas-soc, Geert Uytterhoeven,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
[-- Attachment #1: Type: text/plain, Size: 881 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
2022-09-24 10:18 ` Wolfram Sang
@ 2022-09-25 17:06 ` Biju Das
2022-09-26 12:20 ` Geert Uytterhoeven
0 siblings, 1 reply; 7+ messages in thread
From: Biju Das @ 2022-09-25 17:06 UTC (permalink / raw)
To: Wolfram Sang
Cc: Ulf Hansson, linux-mmc, linux-renesas-soc, Geert Uytterhoeven,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
2022-09-25 17:06 ` Biju Das
@ 2022-09-26 12:20 ` Geert Uytterhoeven
2022-09-26 12:52 ` Biju Das
0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2022-09-26 12:20 UTC (permalink / raw)
To: Biju Das
Cc: Wolfram Sang, Ulf Hansson, linux-mmc, linux-renesas-soc,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
2022-09-26 12:20 ` Geert Uytterhoeven
@ 2022-09-26 12:52 ` Biju Das
0 siblings, 0 replies; 7+ messages in thread
From: Biju Das @ 2022-09-26 12:52 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Wolfram Sang, Ulf Hansson, linux-mmc, linux-renesas-soc,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
2022-09-22 8:55 [PATCH v3] mmc: renesas_sdhi: Fix rounding errors Biju Das
2022-09-24 10:18 ` Wolfram Sang
@ 2022-09-25 16:51 ` Wolfram Sang
2022-09-26 17:13 ` Biju Das
1 sibling, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2022-09-25 16:51 UTC (permalink / raw)
To: Biju Das
Cc: Ulf Hansson, linux-mmc, linux-renesas-soc, Geert Uytterhoeven,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
[-- Attachment #1: Type: text/plain, Size: 641 bytes --]
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>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v3] mmc: renesas_sdhi: Fix rounding errors
2022-09-25 16:51 ` Wolfram Sang
@ 2022-09-26 17:13 ` Biju Das
0 siblings, 0 replies; 7+ messages in thread
From: Biju Das @ 2022-09-26 17:13 UTC (permalink / raw)
To: Wolfram Sang
Cc: Ulf Hansson, linux-mmc, linux-renesas-soc, Geert Uytterhoeven,
Chris Paterson, Biju Das, Prabhakar Mahadev Lad
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-09-26 17:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-22 8:55 [PATCH v3] mmc: renesas_sdhi: Fix rounding errors Biju Das
2022-09-24 10:18 ` Wolfram Sang
2022-09-25 17:06 ` Biju Das
2022-09-26 12:20 ` Geert Uytterhoeven
2022-09-26 12:52 ` Biju Das
2022-09-25 16:51 ` Wolfram Sang
2022-09-26 17:13 ` Biju Das
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.