| Message ID | 20190507154728.27620-1-lukma@denx.de |
|---|---|
| State | Accepted |
| Commit | b6a0427554424a9d6bb563cae4f5555487f38caf |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [U-Boot] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode clock setting issue" | expand |
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(®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
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(®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
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(®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
> 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(®s->mixctrl) | MIX_CTRL_DDREN, ®s->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. 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(®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
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(®s->mixctrl) | MIX_CTRL_DDREN, ®s->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(®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
> -----Original Message----- > From: Lukasz Majewski [mailto:lukma@denx.de] > Sent: 2019年5月8日 19: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" > > 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(®s->mixctrl) | MIX_CTRL_DDREN, ®s->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(®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
On Wed, 8 May 2019 13:03:58 +0000 BOUGH CHEN <haibo.chen@nxp.com> wrote: > > -----Original Message----- > > From: Lukasz Majewski [mailto:lukma@denx.de] > > Sent: 2019年5月8日 19: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" > > > > 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(®s->mixctrl) | MIX_CTRL_DDREN, ®s->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(®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 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
> -----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; - } 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(®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
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; > - } 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(®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
> 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; > > - } 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(®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
> 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; > > > - } 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(®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
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@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; > > > > - } 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(®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
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@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; > > > > > - } 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(®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 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
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@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; > > > > > > - } 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(®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 > > > > > 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
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@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; > > > > > > > - } 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(®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 > > > > > > > > > > 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
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;
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(-)