[U-Boot] [PATCH 37/41] mmc: fsl_esdhc: fix sd/mmc ddr mode clock setting issue

[Benoît Thébaudeau]

Dear Peng Fan,

Some other remarks.

On Tue, May 29, 2018 at 3:45 AM, Peng Fan <peng.fan at nxp.com> wrote:
>
>
>> -----Original Message-----
>> From: Benoît Thébaudeau [mailto:benoit.thebaudeau.dev at gmail.com]
>> Sent: 2018年5月29日 6:32
>> To: Peng Fan <peng.fan at nxp.com>
>> Cc: sbabic at denx.de; Fabio Estevam <fabio.estevam at nxp.com>; U-Boot
>> <u-boot at lists.denx.de>; Bough Chen <haibo.chen at nxp.com>
>> Subject: Re: [U-Boot] [PATCH 37/41] mmc: fsl_esdhc: fix sd/mmc ddr mode clock
>> setting issue
>>
>> Dear Peng Fan,
>>
>> On Mon, May 28, 2018 at 2:25 PM, Peng Fan <peng.fan at nxp.com> wrote:
>> > From: Ye Li <ye.li at nxp.com>
>> >
>> > When sd/mmc work at DDR mode, like HS400/HS400ES/DDR52/DDR50 mode,
>> the
>> > actual clock rate is just half of the expected clock.
>> >
>> > This patch set the DDR_EN bit first for DDR mode, hardware divide the
>> > usdhc clock automatically, then follow the original sdr clock setting
>> > method.
>> >
>> > Signed-off-by: Haibo Chen <haibo.chen at nxp.com>
>> > Signed-off-by: Ye Li <ye.li at nxp.com>
>> > ---
>> >  drivers/mmc/fsl_esdhc.c | 24 +++++++++++++++++++-----
>> >  1 file changed, 19 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
>> > 1b062ff06d..bf4ae74847 100644
>> > --- a/drivers/mmc/fsl_esdhc.c
>> > +++ b/drivers/mmc/fsl_esdhc.c
>> > @@ -583,18 +583,32 @@ static void set_sysctl(struct fsl_esdhc_priv
>> > *priv, struct mmc *mmc, uint clock)  #else
>> >         int pre_div = 2;
>> >  #endif
>> > -       int ddr_pre_div = mmc->ddr_mode ? 2 : 1;
>> >         int sdhc_clk = priv->sdhc_clk;
>> >         uint clk;
>> >
>> > +       /*
>> > +        * For ddr mode, usdhc need to enable DDR mode first, after select
>> > +        * this DDR mode, usdhc will automatically divide the usdhc clock
>> > +        */
>> > +       if (mmc->ddr_mode) {
>> > +               writel(readl(&regs->mixctrl) | MIX_CTRL_DDREN,
>> &regs->mixctrl);

Isn't this redundant with what is done in esdhc_set_timing()?
According to the reference manual, the prescalers might have to be set
after DDREN (as done here), so perhaps DDREN does not have to be set
too in esdhc_set_timing().

>> > +               sdhc_clk >>= 1;
>> > +       }
>> > +
>> >         if (clock < mmc->cfg->f_min)
>> >                 clock = mmc->cfg->f_min;
>> >
>> > -       while (sdhc_clk / (16 * pre_div * ddr_pre_div) > clock && pre_div <
>> 256)
>> > -               pre_div *= 2;
>> > +       if ((sdhc_clk / 16) > clock) {
>> > +               for (; pre_div < 256; pre_div *= 2)
>> > +                       if ((sdhc_clk / pre_div) <= (clock * 16))
>> > +                               break;
>> > +       } else {
>> > +               pre_div = 1;
>>
>> This value is not always available for this divider. See the conditions in the
>> initialization of this variable giving its minimum value.
>
> The else {pre_div = 1;} could be removed then.

And the if conditioning the for loop becomes useless. In the end, the
new loop would be equivalent to the previous while loop.

>>
>> > +       }
>> >
>> > -       while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock && div < 16)
>> > -               div++;
>> > +       for (div = 1; div <= 16; div++)

div is already initialized in its definition, so only one of these
initializations can be kept.

>> > +               if ((sdhc_clk / (div * pre_div)) <= clock)
>> > +                       break;

The only difference with the previous while loop is the "<= 16", which
is correct, but maybe the change could have been limited to that.

>> >
>> >         pre_div >>= 1;
>> >         div -= 1;

Best regards,
Benoît