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