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@denx.de] Sent: 2019年5月8日 14:38 To: Peng Fan peng.fan@nxp.com Cc: u-boot@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@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;
} elsepre_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(®s->mixctrl) | > MIX_CTRL_DDREN, ®s->mixctrl); > - sdhc_clk >>= 1; > - } > - > if (clock < mmc->cfg->f_min) > clock = mmc->cfg->f_min; > > - if (sdhc_clk / 16 > clock) { > - for (; pre_div < 256; pre_div *= 2) > - if ((sdhc_clk / pre_div) <= (clock * > 16)) > - break; > - } else > - pre_div = 1; > + while (sdhc_clk / (16 * pre_div * ddr_pre_div) > > clock && pre_div < 256) > + pre_div *= 2; > > - for (div = 1; div <= 16; div++) > - if ((sdhc_clk / (div * pre_div)) <= clock) > - break; > + while (sdhc_clk / (div * pre_div * ddr_pre_div) > > clock && div < 16) > + div++; > > pre_div >>= 1; > div -= 1; > -- > 2.11.0
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director:
Wolfgang
Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80
Email:
lukma@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@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@denx.de