RE: [PATCH v2] mmc: renesas_sdhi: Add clk margin for sdhi hs clock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thanks for the feedback.

> Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx
> 
> 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




[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux