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....@nxp.com> > > > > > wrote: > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Lukasz Majewski [mailto:lu...@denx.de] > > > > > > > Sent: 2019年5月8日 14:38 > > > > > > > To: Peng Fan <peng....@nxp.com> > > > > > > > Cc: u-boot@lists.denx.de; Tom Rini <tr...@konsulko.com>; > > > > > > > Marcel Ziswiler <marcel.ziswi...@toradex.com>; Fabio Estevam > > > > > > > <fabio.este...@nxp.com>; dl-uboot-imx <uboot-...@nxp.com>; > > > > > > > sba...@denx.de; Stefan Agner <stefan.ag...@toradex.com>; > > > > > > > BOUGH > > > > > CHEN > > > > > > > <haibo.c...@nxp.com>; Ye Li <ye...@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 > > > > > > > <lu...@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 <lu...@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: > > > > > > > > lu...@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: > > > > > > > lu...@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: > > > > > lu...@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: > lu...@denx.de _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
