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

[Lukasz Majewski <lukma@denx.de>]

On Wed, 8 May 2019 13:03:58 +0000
BOUGH CHEN <haibo.chen@nxp.com> wrote:

> > -----Original Message-----
> > From: Lukasz Majewski [mailto:lukma at denx.de]
> > Sent: 2019年5月8日 19:38
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: u-boot at lists.denx.de; Tom Rini <trini@konsulko.com>; Marcel
> > Ziswiler <marcel.ziswiler@toradex.com>; Fabio Estevam
> > <fabio.estevam@nxp.com>; dl-uboot-imx <uboot-imx@nxp.com>;
> > sbabic at denx.de; Stefan Agner <stefan.agner@toradex.com>; BOUGH CHEN
> > <haibo.chen@nxp.com>; Ye Li <ye.li@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@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.
> 

Ok. Thanks for the update.

I'm looking forward to test your patch.

> 
> Regards,
> Bough Chen
> >   
> > > Regards,
> > > Peng.
> > >  
> > > >  
> > > > > >
> > > > > > Thanks,
> > > > > > Peng.
> > > > > >  
> > > > > > >
> > > > > > > Signed-off-by: Lukasz Majewski <lukma@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  




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/20190508/ef550c93/attachment.sig>