[U-Boot] [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode clock setting issue"
Lukasz Majewski
lukma at denx.de
Mon May 20 07:47:24 UTC 2019
Hi Peng,
> Hi Lukasz,
>
> > Subject: Re: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode
> > clock setting issue"
> >
> > Hi Peng,
> >
> > > Hi Lukasz,
> > >
> > > Do you expect me to pick this patch, just see this patch was
> > > delegated to Stefano?
> >
> > I think that you shall decide with Stefano who would take this
> > patch.
> >
> > For me there is no difference, I would just like to have it in main
> > line ASAP (depends who prepare PR first).
>
> ok. I'll send a PR to Tom today after CI done.
Great. Thanks.
> I was waiting IT to
> fix my ssh push issue, so delayed. I still have to use my github to
> PR (:
I do know this topic well ... (Large companies have very strict rules
regarding perceiving firewalls).
>
> Regards,
> Peng.
>
> >
> > >
> > > Regards,
> > > Peng.
> > >
> > > > Subject: RE: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode
> > > > clock setting issue"
> > > >
> > > > > Subject: RE: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr
> > > > > mode clock setting issue"
> > > > >
> > > > > > Subject: Re: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr
> > > > > > mode clock setting issue"
> > > > > >
> > > > > > On Wed, 8 May 2019 13:59:14 +0000 Peng Fan
> > > > > > <peng.fan at nxp.com> wrote:
> > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > > > > > > Sent: 2019年5月8日 14:38
> > > > > > > > To: Peng Fan <peng.fan at nxp.com>
> > > > > > > > Cc: u-boot at lists.denx.de; Tom Rini <trini at konsulko.com>;
> > > > > > > > Marcel Ziswiler <marcel.ziswiler at toradex.com>; Fabio
> > > > > > > > Estevam <fabio.estevam at nxp.com>; dl-uboot-imx
> > > > > > > > <uboot-imx at nxp.com>; sbabic at denx.de; Stefan Agner
> > > > > > > > <stefan.agner at toradex.com>; BOUGH
> > > > > > CHEN
> > > > > > > > <haibo.chen at nxp.com>; Ye Li <ye.li at nxp.com>
> > > > > > > > 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)?
> > > > > > >
> > > > > > > Could you help verify whether the patch fixes you issue?
> > > > > > >
> > > > > > > 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;
> > > > > > > + }
> > > > > >
> > > > > > Please examine this code thoroughly and provide patch.
> > > > >
> > > > > The pre_div should not override the initialization value at
> > > > > the beginning of the function.
> > > > >
> > > > > >
> > > > > > The
> > > > > > } else
> > > > > > pre_div = 1;
> > > > > >
> > > > > > was added there for a purpose, so I'm wondering why it can
> > > > > > be easily removed now.
> > > > >
> > > > > The else was wrongly added. It is not correct.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > for (div = 1; div <= 16; div++)
> > > > > > > if ((sdhc_clk / (div * pre_div)) <= clock)
> > > > > > >
> > > > > >
> > > > > > As I've stated above - is the above for() correct?
> > > > > >
> > > > > > In the original code it was div < 16, but here it is div <=
> > > > > > 16.
> > > > >
> > > > > Checking i.MX53 SDHC DVS, it supports [1,16], so should use
> > > > > "<=16", other i.MX has same.
> > > >
> > > > Should use "<16". Will pick up your revert to avoid regression.
> > > >
> > > > Thanks,
> > > > Peng.
> > > >
> > > > >
> > > > > Divisor:
> > > > > This register is used to provide a more exact divisor to
> > > > > generate the desired SD clock frequency.
> > > > > NOTE: The divider can even support odd divisor without
> > > > > deterioration of duty cycle.
> > > > > The setting are as following:
> > > > > 0x0 Divisor by 1
> > > > > 0x1 Divisor by 2
> > > > > 0xe Divisor by 15
> > > > > 0xf Divisor by 16
> > > > >
> > > > > Regards,
> > > > > Peng.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > 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
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > 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
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190520/7de80f41/attachment.sig>
More information about the U-Boot
mailing list