* [PATCH v2] mmc: renesas_sdhi: Add clk margin for sdhi hs clock
@ 2022-09-19 8:48 Biju Das
2022-09-19 10:06 ` Geert Uytterhoeven
0 siblings, 1 reply; 3+ messages in thread
From: Biju Das @ 2022-09-19 8:48 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
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(-)
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 */
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] mmc: renesas_sdhi: Add clk margin for sdhi hs clock
2022-09-19 8:48 [PATCH v2] mmc: renesas_sdhi: Add clk margin for sdhi hs clock Biju Das
@ 2022-09-19 10:06 ` Geert Uytterhoeven
2022-09-21 15:45 ` Biju Das
0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2022-09-19 10:06 UTC (permalink / raw)
To: Biju Das
Cc: Ulf Hansson, Wolfram Sang, linux-mmc, linux-renesas-soc,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH v2] mmc: renesas_sdhi: Add clk margin for sdhi hs clock
2022-09-19 10:06 ` Geert Uytterhoeven
@ 2022-09-21 15:45 ` Biju Das
0 siblings, 0 replies; 3+ messages in thread
From: Biju Das @ 2022-09-21 15:45 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Ulf Hansson, Wolfram Sang, linux-mmc, linux-renesas-soc,
Geert Uytterhoeven, Chris Paterson, Biju Das,
Prabhakar Mahadev Lad
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-21 15:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-19 8:48 [PATCH v2] mmc: renesas_sdhi: Add clk margin for sdhi hs clock Biju Das
2022-09-19 10:06 ` Geert Uytterhoeven
2022-09-21 15:45 ` Biju Das
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).