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

BOUGH CHEN haibo.chen at nxp.com
Wed May 8 13:03:58 UTC 2019 

> -----Original Message-----
> From: Lukasz Majewski [mailto:lukma at denx.de]
> Sent: 2019年5月8日 19: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"
> 
> Hi Peng,
> 
> > > 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(&regs->mixctrl) | MIX_CTRL_DDREN, &regs->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.
> >
> 
> Thanks for the explanation.
> 
> I'm just wondering if we shall revert the original patch and wait for proper fix
> for IMX8 or will you prepare a separate patch to fix the current mainline (so the
> "revert" patch can be dropped) ?

When we add the patch, we do not take the i.mx53 case into consideration. Sorry for this mistake.
I prefer add a fix patch rather than 'revert', because I notice there is a claim in the RM:

    If setting SDCLKFS and DVS can generate same clock frequency,(For example, in SDR mode,
    SDCLKFS = 01h is same as DVS = 01h.) SDCLKFS is highly recommended.

For both these methods, the current one or the previous one, the code prefer DVS rather than SDCLKFS, not align with the RM suggestion. 
So this time I plan to re-write the code to follow the RM suggestion. I will send a patch these week.


Regards,
Bough Chen
> 
> > 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(&regs->mixctrl) |
> > > > > > MIX_CTRL_DDREN, &regs->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

More information about the U-Boot mailing list