diff mbox series

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

Message ID 20180528122526.20597-38-peng.fan@nxp.com
State Superseded
Delegated to: Stefano Babic
Headers show
Series imx: add i.MX8QXP support | expand

Commit Message

Peng Fan May 28, 2018, 12:25 p.m. UTC 
From: Ye Li <ye.li@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@nxp.com>
Signed-off-by: Ye Li <ye.li@nxp.com>
---
 drivers/mmc/fsl_esdhc.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Comments

Benoît Thébaudeau May 28, 2018, 10:32 p.m. UTC | #1 
Dear Peng Fan,

On Mon, May 28, 2018 at 2:25 PM, Peng Fan <peng.fan@nxp.com> wrote:
> From: Ye Li <ye.li@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@nxp.com>
> Signed-off-by: Ye Li <ye.li@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);
> +               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.

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

Best regards,
Benoît
Peng Fan May 29, 2018, 1:45 a.m. UTC | #2 
> -----Original Message-----
> From: Benoît Thébaudeau [mailto:benoit.thebaudeau.dev@gmail.com]
> Sent: 2018年5月29日 6:32
> To: Peng Fan <peng.fan@nxp.com>
> Cc: sbabic@denx.de; Fabio Estevam <fabio.estevam@nxp.com>; U-Boot
> <u-boot@lists.denx.de>; Bough Chen <haibo.chen@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@nxp.com> wrote:
> > From: Ye Li <ye.li@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@nxp.com>
> > Signed-off-by: Ye Li <ye.li@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);
> > +               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.

Thanks,
Peng.

> 
> > +       }
> >
> > -       while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock && div < 16)
> > -               div++;
> > +       for (div = 1; div <= 16; div++)
> > +               if ((sdhc_clk / (div * pre_div)) <= clock)
> > +                       break;
> >
> >         pre_div >>= 1;
> >         div -= 1;
> 
> Best regards,
> Benoît
Benoît Thébaudeau May 29, 2018, 8:47 p.m. UTC | #3 
Dear Peng Fan,

Some other remarks.

On Tue, May 29, 2018 at 3:45 AM, Peng Fan <peng.fan@nxp.com> wrote:
>
>
>> -----Original Message-----
>> From: Benoît Thébaudeau [mailto:benoit.thebaudeau.dev@gmail.com]
>> Sent: 2018年5月29日 6:32
>> To: Peng Fan <peng.fan@nxp.com>
>> Cc: sbabic@denx.de; Fabio Estevam <fabio.estevam@nxp.com>; U-Boot
>> <u-boot@lists.denx.de>; Bough Chen <haibo.chen@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@nxp.com> wrote:
>> > From: Ye Li <ye.li@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@nxp.com>
>> > Signed-off-by: Ye Li <ye.li@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
diff mbox series

Patch

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);
+		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;
+	}
 
-	while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock && div < 16)
-		div++;
+	for (div = 1; div <= 16; div++)
+		if ((sdhc_clk / (div * pre_div)) <= clock)
+			break;
 
 	pre_div >>= 1;
 	div -= 1;