[U-Boot] [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode clock setting issue"
Peng Fan
peng.fan at nxp.com
Wed May 8 09:44:59 UTC 2019
> Subject: Re: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode clock
> setting issue"
>
> On Wed, 8 May 2019 08:19:45 +0200
> Lukasz Majewski <lukma at denx.de> wrote:
>
> > Hi Peng,
> >
> > > Hi Lukasz,
> > >
> > > > Subject: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode clock
> > > > setting issue"
> > > >
> > > > This reverts commit 72a89e0da5ac6a4ab929b15a2b656f04f50767f6,
> > > > which causes the imx53 HSC to hang as the eMMC is not working
> > > > properly anymore.
> > > >
> > > > The exact error message:
> > > > MMC write: dev # 0, block # 2, count 927 ... mmc write failed
> > > > 0 blocks written: ERROR
> > > >
> > > > imx53 is not using the DDR mode.
> > > >
> > > > Debugging of pre_div and div generation showed that those values
> > > > are generated in a way, which is not matching the ones from
> > > > working setup.
> > > >
> > > > As the original patch was performing code refactoring, let's
> > > > revert this change, so all imx53 boards would work again.
> > >
> > > Could you share what is the clock value for your board?
> >
> > Sure, no problem:
> >
> > Working setup:
> > --------------
> >
> > MMC:
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 8 div: 12 set_sysctl: clk: 2240
> > FSL_SDHC: 0
> > Loading Environment from MMC...
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 8 div: 12 set_sysctl: clk: 2240
> >
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 8 div: 12 set_sysctl: clk: 2240
> >
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 1 div: 1 set_sysctl: clk: 272
> >
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 1 div: 0 set_sysctl: clk: 256
> >
> >
> >
> >
> > Broken:
> > -------
> > MMC:
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 8 div: 12 set_sysctl: clk: 2240
> > FSL_SDHC: 0
> > Loading Environment from MMC...
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 8 div: 12 set_sysctl: clk: 2240
> >
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 8 div: 12 set_sysctl: clk: 2240
> >
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 0 div: 3 set_sysctl: clk: 48
> >
> > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 80000000
> > pre_div: 0 div: 1 set_sysctl: clk: 16
> >
> >
> > (Please also find attached patch to reproduce debug output).
> >
>
> And maybe the most important question - why it was necessary to refactor
> this code?
>
> Parts responsible for calculating pre_div and div seems not related to ddr
> problem (one spot issue is div <= 16 , but in the original code it was div < 16)?
This patch is mostly to support HS400 DDR_EN mode,
But after I check more, seems it is not a right fix, sorry for this regression.
It does almost the same calculation as before code expect
writel(readl(®s->mixctrl) | MIX_CTRL_DDREN, ®s->mixctrl);
and one condition break others.
So the fix for you is
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 1b7de74a72..3347fbe738 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -640,8 +640,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock)
for (; pre_div < 256; pre_div *= 2)
if ((sdhc_clk / pre_div) <= (clock * 16))
break;
- } else
- pre_div = 1;
+ }
for (div = 1; div <= 16; div++)
if ((sdhc_clk / (div * pre_div)) <= clock)
With this patch, the i.MX8 HS400 will work at 100MHz not 200MHz,
But it not hang anyway. To fix the i.MX8, need define ARCH_MXC,
I'll add a new patch for ARCH_MXC part for i.MX family.
Regards,
Peng.
>
> > >
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > Signed-off-by: Lukasz Majewski <lukma at denx.de>
> > > > ---
> > > > drivers/mmc/fsl_esdhc.c | 23 +++++------------------
> > > > 1 file changed, 5 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> > > > index 1b7de74a72..377b2673a3 100644
> > > > --- a/drivers/mmc/fsl_esdhc.c
> > > > +++ b/drivers/mmc/fsl_esdhc.c
> > > > @@ -621,31 +621,18 @@ 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(®s->mixctrl) | MIX_CTRL_DDREN,
> > > > ®s->mixctrl);
> > > > - sdhc_clk >>= 1;
> > > > - }
> > > > -
> > > > if (clock < mmc->cfg->f_min)
> > > > clock = mmc->cfg->f_min;
> > > >
> > > > - 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 / (16 * pre_div * ddr_pre_div) > clock &&
> > > > pre_div < 256)
> > > > + pre_div *= 2;
> > > >
> > > > - for (div = 1; div <= 16; div++)
> > > > - if ((sdhc_clk / (div * pre_div)) <= clock)
> > > > - break;
> > > > + while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock
> > > > && div < 16)
> > > > + div++;
> > > >
> > > > pre_div >>= 1;
> > > > div -= 1;
> > > > --
> > > > 2.11.0
> > >
> >
> >
> >
> >
> > Best regards,
> >
> > Lukasz Majewski
> >
> > --
> >
> > DENX Software Engineering GmbH, Managing Director: Wolfgang
> Denk
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma at denx.de
>
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de
More information about the U-Boot
mailing list