| Message ID | 1323161655-19050-1-git-send-email-Shengzhou.Liu@freescale.com |
|---|---|
| State | Accepted |
| Commit | d8251108e0def5a2f15124a8e6314b14bfa5eb9c |
| Headers | show |
On 12/06/2011 02:54 AM, Shengzhou Liu wrote: > There was a bug for fmr initialization, which lead to fmr was always 0x100 > in fsl_elbc_chip_init() and caused FCM command timeout before calling > fsl_elbc_chip_init_tail(), now we initialize CWTO to maximum timeout value > and not relying on the setting of bootloader. > > Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> > --- > v2: make fmr not relying on the setting of bootloader. > > drivers/mtd/nand/fsl_elbc_nand.c | 10 +++++----- > 1 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c > index eedd8ee..4f405a0 100644 > --- a/drivers/mtd/nand/fsl_elbc_nand.c > +++ b/drivers/mtd/nand/fsl_elbc_nand.c > @@ -659,9 +659,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) > if (chip->pagemask & 0xff000000) > al++; > > - /* add to ECCM mode set in fsl_elbc_init */ > - priv->fmr |= (12 << FMR_CWTO_SHIFT) | /* Timeout > 12 ms */ > - (al << FMR_AL_SHIFT); > + priv->fmr |= al << FMR_AL_SHIFT; > > dev_dbg(priv->dev, "fsl_elbc_init: nand->numchips = %d\n", > chip->numchips); > @@ -764,8 +762,10 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv) > priv->mtd.priv = chip; > priv->mtd.owner = THIS_MODULE; > > - /* Set the ECCM according to the settings in bootloader.*/ > - priv->fmr = in_be32(&lbc->fmr) & FMR_ECCM; > + /* set timeout to maximum */ > + priv->fmr = 15 << FMR_CWTO_SHIFT; > + if (in_be32(&lbc->bank[priv->bank].or) & OR_FCM_PGS) > + priv->fmr |= FMR_ECCM; Please do not change the way ECCM is handled. We probably should have done it this way from the start, but at this point it breaks compatibility if you have a large page flash and the firmware didn't touch NAND. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, December 07, 2011 1:16 AM > To: Liu Shengzhou-B36685 > Cc: linuxppc-dev@lists.ozlabs.org; linux-mtd@lists.infradead.org; > dwmw2@infradead.org; Gala Kumar-B11780 > Subject: Re: [PATCH 1/2 v2] mtd/nand: fixup for fmr initialization of > Freescale NAND controller > > On 12/06/2011 02:54 AM, Shengzhou Liu wrote: > > There was a bug for fmr initialization, which lead to fmr was always > > 0x100 in fsl_elbc_chip_init() and caused FCM command timeout before > > calling fsl_elbc_chip_init_tail(), now we initialize CWTO to maximum > > timeout value and not relying on the setting of bootloader. > > > > Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> > > --- > > v2: make fmr not relying on the setting of bootloader. > > > > drivers/mtd/nand/fsl_elbc_nand.c | 10 +++++----- > > 1 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mtd/nand/fsl_elbc_nand.c > > b/drivers/mtd/nand/fsl_elbc_nand.c > > index eedd8ee..4f405a0 100644 > > --- a/drivers/mtd/nand/fsl_elbc_nand.c > > +++ b/drivers/mtd/nand/fsl_elbc_nand.c > > @@ -659,9 +659,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info > *mtd) > > if (chip->pagemask & 0xff000000) > > al++; > > > > - /* add to ECCM mode set in fsl_elbc_init */ > > - priv->fmr |= (12 << FMR_CWTO_SHIFT) | /* Timeout > 12 ms */ > > - (al << FMR_AL_SHIFT); > > + priv->fmr |= al << FMR_AL_SHIFT; > > > > dev_dbg(priv->dev, "fsl_elbc_init: nand->numchips = %d\n", > > chip->numchips); > > @@ -764,8 +762,10 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd > *priv) > > priv->mtd.priv = chip; > > priv->mtd.owner = THIS_MODULE; > > > > - /* Set the ECCM according to the settings in bootloader.*/ > > - priv->fmr = in_be32(&lbc->fmr) & FMR_ECCM; > > + /* set timeout to maximum */ > > + priv->fmr = 15 << FMR_CWTO_SHIFT; > > + if (in_be32(&lbc->bank[priv->bank].or) & OR_FCM_PGS) > > + priv->fmr |= FMR_ECCM; > > Please do not change the way ECCM is handled. We probably should have > done it this way from the start, but at this point it breaks > compatibility if you have a large page flash and the firmware didn't > touch NAND. > > -Scott [Shengzhou] This patch doesn't change the way ECCM is handled, it's still same as before, just make sure CWTO timeout is set to maximum.
On 12/07/2011 12:30 AM, Liu Shengzhou-B36685 wrote: > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Wednesday, December 07, 2011 1:16 AM >> To: Liu Shengzhou-B36685 >> Cc: linuxppc-dev@lists.ozlabs.org; linux-mtd@lists.infradead.org; >> dwmw2@infradead.org; Gala Kumar-B11780 >> Subject: Re: [PATCH 1/2 v2] mtd/nand: fixup for fmr initialization of >> Freescale NAND controller >> >> On 12/06/2011 02:54 AM, Shengzhou Liu wrote: >>> There was a bug for fmr initialization, which lead to fmr was always >>> 0x100 in fsl_elbc_chip_init() and caused FCM command timeout before >>> calling fsl_elbc_chip_init_tail(), now we initialize CWTO to maximum >>> timeout value and not relying on the setting of bootloader. >>> >>> Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> >>> --- >>> v2: make fmr not relying on the setting of bootloader. >>> >>> drivers/mtd/nand/fsl_elbc_nand.c | 10 +++++----- >>> 1 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c >>> b/drivers/mtd/nand/fsl_elbc_nand.c >>> index eedd8ee..4f405a0 100644 >>> --- a/drivers/mtd/nand/fsl_elbc_nand.c >>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c >>> @@ -659,9 +659,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info >> *mtd) >>> if (chip->pagemask & 0xff000000) >>> al++; >>> >>> - /* add to ECCM mode set in fsl_elbc_init */ >>> - priv->fmr |= (12 << FMR_CWTO_SHIFT) | /* Timeout > 12 ms */ >>> - (al << FMR_AL_SHIFT); >>> + priv->fmr |= al << FMR_AL_SHIFT; >>> >>> dev_dbg(priv->dev, "fsl_elbc_init: nand->numchips = %d\n", >>> chip->numchips); >>> @@ -764,8 +762,10 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd >> *priv) >>> priv->mtd.priv = chip; >>> priv->mtd.owner = THIS_MODULE; >>> >>> - /* Set the ECCM according to the settings in bootloader.*/ >>> - priv->fmr = in_be32(&lbc->fmr) & FMR_ECCM; >>> + /* set timeout to maximum */ >>> + priv->fmr = 15 << FMR_CWTO_SHIFT; >>> + if (in_be32(&lbc->bank[priv->bank].or) & OR_FCM_PGS) >>> + priv->fmr |= FMR_ECCM; >> >> Please do not change the way ECCM is handled. We probably should have >> done it this way from the start, but at this point it breaks >> compatibility if you have a large page flash and the firmware didn't >> touch NAND. >> >> -Scott > [Shengzhou] This patch doesn't change the way ECCM is handled, it's still same as before, just make sure CWTO timeout is set to maximum. It does change it. It used to use the existing value in FMR, and now it sets it based on ORn[PGS]. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Thursday, December 08, 2011 1:17 AM > To: Liu Shengzhou-B36685 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; linux- > mtd@lists.infradead.org; dwmw2@infradead.org; Gala Kumar-B11780 > Subject: Re: [PATCH 1/2 v2] mtd/nand: fixup for fmr initialization of > Freescale NAND controller > > On 12/07/2011 12:30 AM, Liu Shengzhou-B36685 wrote: > > > >> -----Original Message----- > >> From: Wood Scott-B07421 > >> Sent: Wednesday, December 07, 2011 1:16 AM > >> To: Liu Shengzhou-B36685 > >> Cc: linuxppc-dev@lists.ozlabs.org; linux-mtd@lists.infradead.org; > >> dwmw2@infradead.org; Gala Kumar-B11780 > >> Subject: Re: [PATCH 1/2 v2] mtd/nand: fixup for fmr initialization of > >> Freescale NAND controller > >> > >> On 12/06/2011 02:54 AM, Shengzhou Liu wrote: > >>> There was a bug for fmr initialization, which lead to fmr was > >>> always 0x100 in fsl_elbc_chip_init() and caused FCM command timeout > >>> before calling fsl_elbc_chip_init_tail(), now we initialize CWTO to > >>> maximum timeout value and not relying on the setting of bootloader. > >>> > >>> Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> > >>> --- > >>> v2: make fmr not relying on the setting of bootloader. > >>> > >>> drivers/mtd/nand/fsl_elbc_nand.c | 10 +++++----- > >>> 1 files changed, 5 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c > >>> b/drivers/mtd/nand/fsl_elbc_nand.c > >>> index eedd8ee..4f405a0 100644 > >>> --- a/drivers/mtd/nand/fsl_elbc_nand.c > >>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c > >>> @@ -659,9 +659,7 @@ static int fsl_elbc_chip_init_tail(struct > >>> mtd_info > >> *mtd) > >>> if (chip->pagemask & 0xff000000) > >>> al++; > >>> > >>> - /* add to ECCM mode set in fsl_elbc_init */ > >>> - priv->fmr |= (12 << FMR_CWTO_SHIFT) | /* Timeout > 12 ms */ > >>> - (al << FMR_AL_SHIFT); > >>> + priv->fmr |= al << FMR_AL_SHIFT; > >>> > >>> dev_dbg(priv->dev, "fsl_elbc_init: nand->numchips = %d\n", > >>> chip->numchips); > >>> @@ -764,8 +762,10 @@ static int fsl_elbc_chip_init(struct > >>> fsl_elbc_mtd > >> *priv) > >>> priv->mtd.priv = chip; > >>> priv->mtd.owner = THIS_MODULE; > >>> > >>> - /* Set the ECCM according to the settings in bootloader.*/ > >>> - priv->fmr = in_be32(&lbc->fmr) & FMR_ECCM; > >>> + /* set timeout to maximum */ > >>> + priv->fmr = 15 << FMR_CWTO_SHIFT; > >>> + if (in_be32(&lbc->bank[priv->bank].or) & OR_FCM_PGS) > >>> + priv->fmr |= FMR_ECCM; > >> > >> Please do not change the way ECCM is handled. We probably should > >> have done it this way from the start, but at this point it breaks > >> compatibility if you have a large page flash and the firmware didn't > >> touch NAND. > >> > >> -Scott > > [Shengzhou] This patch doesn't change the way ECCM is handled, it's > still same as before, just make sure CWTO timeout is set to maximum. > > It does change it. It used to use the existing value in FMR, and now it > sets it based on ORn[PGS]. > > -Scott [Shengzhou] In u-boot: #ifdef CONFIG_FSL_ELBC_FMR priv->fmr = CONFIG_FSL_ELBC_FMR; #else priv->fmr = (15 << FMR_CWTO_SHIFT) | (2 << FMR_AL_SHIFT); or = in_be32(&elbc_ctrl->regs->bank[priv->bank].or); if (or & OR_FCM_PGS) priv->fmr |= FMR_ECCM; #endif In kernel: It used to be " priv->fmr = in_be32(&lbc->fmr) & FMR_ECCM ", so fmr was always 0x100(or 0,depend on ORn[PGS]), CWTO was 0(timeout was minimum). In this patch, for not relying on bootloader, fmr is initialized as what u-boot does, except FMR_AL_SHIFT is handled in fsl_elbc_chip_init_tail and without definition of CONFIG_FSL_ELBC_FMR. So, it doesn't change it. Do we still need CONFIG_FSL_ELBC_FMR in kernel?
On 12/07/2011 09:36 PM, Liu Shengzhou-B36685 wrote: > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Thursday, December 08, 2011 1:17 AM >> To: Liu Shengzhou-B36685 >> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; linux- >> mtd@lists.infradead.org; dwmw2@infradead.org; Gala Kumar-B11780 >> Subject: Re: [PATCH 1/2 v2] mtd/nand: fixup for fmr initialization of >> Freescale NAND controller >> >> On 12/07/2011 12:30 AM, Liu Shengzhou-B36685 wrote: >>> [Shengzhou] This patch doesn't change the way ECCM is handled, it's >> still same as before, just make sure CWTO timeout is set to maximum. >> >> It does change it. It used to use the existing value in FMR, and now it >> sets it based on ORn[PGS]. >> >> -Scott > > [Shengzhou] > In u-boot: > #ifdef CONFIG_FSL_ELBC_FMR > priv->fmr = CONFIG_FSL_ELBC_FMR; > #else > priv->fmr = (15 << FMR_CWTO_SHIFT) | (2 << FMR_AL_SHIFT); > or = in_be32(&elbc_ctrl->regs->bank[priv->bank].or); > if (or & OR_FCM_PGS) > priv->fmr |= FMR_ECCM; > #endif > > In kernel: It used to be " priv->fmr = in_be32(&lbc->fmr) & FMR_ECCM > ", so fmr was always 0x100(or 0,depend on ORn[PGS]), CWTO was > 0(timeout was minimum). In this patch, for not relying on > bootloader, fmr is initialized as what u-boot does, except > FMR_AL_SHIFT is handled in fsl_elbc_chip_init_tail and without > definition of CONFIG_FSL_ELBC_FMR. > > So, it doesn't change it. You're assuming that the above U-Boot code is always run. This depends on whether the NAND driver is enabled in U-Boot. In the future, though, it might also depend on whether a NAND command is actually run in U-Boot -- this makes the setting of FMR non-deterministic between boots, which is worse than a one-time breakage of an unusual setup (driver not enabled in U-Boot at all). So it is a change, but I now think it's a change we should make. The changelog should mention that this is happening, though. > Do we still need CONFIG_FSL_ELBC_FMR in kernel? We do not want such a compile-time constant in the kernel. Use ORn[PGS] as the patch currently does. -Scott
diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c index eedd8ee..4f405a0 100644 --- a/drivers/mtd/nand/fsl_elbc_nand.c +++ b/drivers/mtd/nand/fsl_elbc_nand.c @@ -659,9 +659,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) if (chip->pagemask & 0xff000000) al++; - /* add to ECCM mode set in fsl_elbc_init */ - priv->fmr |= (12 << FMR_CWTO_SHIFT) | /* Timeout > 12 ms */ - (al << FMR_AL_SHIFT); + priv->fmr |= al << FMR_AL_SHIFT; dev_dbg(priv->dev, "fsl_elbc_init: nand->numchips = %d\n", chip->numchips); @@ -764,8 +762,10 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv) priv->mtd.priv = chip; priv->mtd.owner = THIS_MODULE; - /* Set the ECCM according to the settings in bootloader.*/ - priv->fmr = in_be32(&lbc->fmr) & FMR_ECCM; + /* set timeout to maximum */ + priv->fmr = 15 << FMR_CWTO_SHIFT; + if (in_be32(&lbc->bank[priv->bank].or) & OR_FCM_PGS) + priv->fmr |= FMR_ECCM; /* fill in nand_chip structure */ /* set up function call table */
There was a bug for fmr initialization, which lead to fmr was always 0x100 in fsl_elbc_chip_init() and caused FCM command timeout before calling fsl_elbc_chip_init_tail(), now we initialize CWTO to maximum timeout value and not relying on the setting of bootloader. Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> --- v2: make fmr not relying on the setting of bootloader. drivers/mtd/nand/fsl_elbc_nand.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)