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

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

[Lukasz Majewski]

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.

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

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

[Peng Fan]

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?

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

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

[Lukasz Majewski]

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).

> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-WIP.patch
Type: text/x-patch
Size: 1199 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190508/a02ee699/attachment.bin>
-------------- 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/a02ee699/attachment.sig>

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

[Lukasz Majewski]

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)?

> > 
> > 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
-------------- 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/185091df/attachment.sig>

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

[Peng Fan]

> 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@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@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

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

[Lukasz Majewski]

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) ?

> 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
-------------- 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/11abbade/attachment.sig>

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

[BOUGH CHEN]

> -----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.


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

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

[Lukasz Majewski]

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>

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

[Peng Fan]

> -----Original Message-----
> From: Lukasz Majewski [mailto:lukma at denx.de]
> Sent: 2019年5月8日 14: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"
> 
> 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)?

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;
+       }

        for (div = 1; div <= 16; div++)
                if ((sdhc_clk / (div * pre_div)) <= clock)


Thanks,
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

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

[Lukasz Majewski]

On Wed, 8 May 2019 13:59:14 +0000
Peng Fan <peng.fan@nxp.com> wrote:

> > -----Original Message-----
> > From: Lukasz Majewski [mailto:lukma at denx.de]
> > Sent: 2019年5月8日 14: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"
> > 
> > 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)?  
> 
> 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 
} else
	pre_div = 1;

was added there for a purpose, so I'm wondering why it can be easily
removed now.

> 
>         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.

> 
> Thanks,
> 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
-------------- 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/1466773c/attachment.sig>

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

[Peng Fan]

> 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@nxp.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > Sent: 2019年5月8日 14: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"
> > >
> > > 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)?
> >
> > 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.

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@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

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

[Peng Fan]

> 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@nxp.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > > Sent: 2019年5月8日 14: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"
> > > >
> > > > 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)?
> > >
> > > 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@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

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

[Peng Fan]

Hi Lukasz,

Do you expect me to pick this patch, just see this patch was delegated to Stefano?

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@nxp.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > > > Sent: 2019年5月8日 14: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"
> > > > >
> > > > > 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)?
> > > >
> > > > 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@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

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

[Lukasz Majewski]

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). 

> 
> 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@nxp.com> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > > > > Sent: 2019年5月8日 14: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"
> > > > > >
> > > > > > 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)?  
> > > > >
> > > > > 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@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/20190520/a7eed713/attachment-0001.sig>

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

[Peng Fan]

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. I was waiting IT to fix my ssh push issue,
so delayed. I still have to use my github to PR (:

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@nxp.com>
> > > > > wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > > > > > Sent: 2019年5月8日 14: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"
> > > > > > >
> > > > > > > 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)?
> > > > > >
> > > > > > 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@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

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

[Lukasz Majewski]

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@nxp.com> wrote:
> > > > > >  
> > > > > > > > -----Original Message-----
> > > > > > > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > > > > > > Sent: 2019年5月8日 14: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"
> > > > > > > >
> > > > > > > > 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)?  
> > > > > > >
> > > > > > > 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@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  




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>