| Message ID | 1479901660-124876-1-git-send-email-joel.esponde@honeywell.com |
|---|---|
| State | Accepted |
| Commit | 807c16253319ee6ccf8873ae64f070f7eb532cd5 |
| Headers | show |
Le 23/11/2016 à 12:47, Joël Esponde a écrit : > With the S25FL127S nor flash part, each writing to the configuration > register takes hundreds of ms. During that time, no more accesses to > the flash should be done (even reads). > > This commit adds a wait loop after the register writing until the flash > finishes its work. > > This issue could make rootfs mounting fail when the latter was done too > much closely to this quad enable bit setting step. And in this case, a > driver as UBIFS may try to recover the filesystem and may broke it > completely. > > Signed-off-by: Joël Esponde <joel.esponde@honeywell.com> Applied to git://github.com/spi-nor/linux.git Thanks! > --- > drivers/mtd/spi-nor/spi-nor.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index d0fc165..21dde52 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1255,6 +1255,13 @@ static int spansion_quad_enable(struct spi_nor *nor) > return -EINVAL; > } > > + ret = spi_nor_wait_till_ready(nor); > + if (ret) { > + dev_err(nor->dev, > + "timeout while writing configuration register\n"); > + return ret; > + } > + > /* read back and check it */ > ret = read_cr(nor); > if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { >
On 11/23/2016 12:47 PM, Joël Esponde wrote: > With the S25FL127S nor flash part, each writing to the configuration > register takes hundreds of ms. During that time, no more accesses to > the flash should be done (even reads). > > This commit adds a wait loop after the register writing until the flash > finishes its work. > > This issue could make rootfs mounting fail when the latter was done too > much closely to this quad enable bit setting step. And in this case, a > driver as UBIFS may try to recover the filesystem and may broke it > completely. Does this apply to all spansion chips or only to selected few ? > Signed-off-by: Joël Esponde <joel.esponde@honeywell.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index d0fc165..21dde52 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1255,6 +1255,13 @@ static int spansion_quad_enable(struct spi_nor *nor) > return -EINVAL; > } > > + ret = spi_nor_wait_till_ready(nor); > + if (ret) { > + dev_err(nor->dev, > + "timeout while writing configuration register\n"); > + return ret; > + } > + > /* read back and check it */ > ret = read_cr(nor); > if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { >
Hi Marek, Le 25/11/2016 à 15:17, Marek Vasut a écrit : > On 11/23/2016 12:47 PM, Joël Esponde wrote: >> With the S25FL127S nor flash part, each writing to the configuration >> register takes hundreds of ms. During that time, no more accesses to >> the flash should be done (even reads). >> >> This commit adds a wait loop after the register writing until the flash >> finishes its work. >> >> This issue could make rootfs mounting fail when the latter was done too >> much closely to this quad enable bit setting step. And in this case, a >> driver as UBIFS may try to recover the filesystem and may broke it >> completely. > > Does this apply to all spansion chips or only to selected few ? > I've recently faced the very same issue with Winbond memories, which use the same procedure as Spansion to set the Quad Enable bit. More precisely, in my case it was some bare metal (bootloader) code but the issue was the same, there was no polling of busy bit from the Status Register after having set the QE bit in the Status Register 2 / Control Register 1. Then the next SPI command came too early and failed because the memory was actually still busy. I faced this issue with Winbond W25Q256 and W25M512. Best regards, Cyrille >> Signed-off-by: Joël Esponde <joel.esponde@honeywell.com> >> --- >> drivers/mtd/spi-nor/spi-nor.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index d0fc165..21dde52 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -1255,6 +1255,13 @@ static int spansion_quad_enable(struct spi_nor *nor) >> return -EINVAL; >> } >> >> + ret = spi_nor_wait_till_ready(nor); >> + if (ret) { >> + dev_err(nor->dev, >> + "timeout while writing configuration register\n"); >> + return ret; >> + } >> + >> /* read back and check it */ >> ret = read_cr(nor); >> if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { >> > >
On 11/25/2016 03:50 PM, Cyrille Pitchen wrote: > Hi Marek, Hi, > Le 25/11/2016 à 15:17, Marek Vasut a écrit : >> On 11/23/2016 12:47 PM, Joël Esponde wrote: >>> With the S25FL127S nor flash part, each writing to the configuration >>> register takes hundreds of ms. During that time, no more accesses to >>> the flash should be done (even reads). >>> >>> This commit adds a wait loop after the register writing until the flash >>> finishes its work. >>> >>> This issue could make rootfs mounting fail when the latter was done too >>> much closely to this quad enable bit setting step. And in this case, a >>> driver as UBIFS may try to recover the filesystem and may broke it >>> completely. >> >> Does this apply to all spansion chips or only to selected few ? >> > > I've recently faced the very same issue with Winbond memories, which use > the same procedure as Spansion to set the Quad Enable bit. > More precisely, in my case it was some bare metal (bootloader) code but the > issue was the same, there was no polling of busy bit from the Status > Register after having set the QE bit in the Status Register 2 / Control > Register 1. Then the next SPI command came too early and failed because the > memory was actually still busy. > > I faced this issue with Winbond W25Q256 and W25M512. So we can leave this code as is ?
> -----Message d'origine----- > De : Marek Vasut [mailto:marek.vasut@gmail.com] > Envoyé : vendredi 25 novembre 2016 16:08 > À : Cyrille Pitchen <cyrille.pitchen@atmel.com>; Esponde, Joel > <Joel.Esponde@Honeywell.com>; linux-mtd@lists.infradead.org > Objet : Re: [PATCH v3] mtd: spi-nor: fix spansion quad enable > > On 11/25/2016 03:50 PM, Cyrille Pitchen wrote: > > Hi Marek, > > Hi, > > > Le 25/11/2016 à 15:17, Marek Vasut a écrit : > >> On 11/23/2016 12:47 PM, Joël Esponde wrote: > >>> With the S25FL127S nor flash part, each writing to the configuration > >>> register takes hundreds of ms. During that time, no more accesses > >>> to the flash should be done (even reads). > >>> > >>> This commit adds a wait loop after the register writing until the > >>> flash finishes its work. > >>> > >>> This issue could make rootfs mounting fail when the latter was done > >>> too much closely to this quad enable bit setting step. And in this > >>> case, a driver as UBIFS may try to recover the filesystem and may > >>> broke it completely. > >> > >> Does this apply to all spansion chips or only to selected few ? > >> > > > > I've recently faced the very same issue with Winbond memories, which > > use the same procedure as Spansion to set the Quad Enable bit. > > More precisely, in my case it was some bare metal (bootloader) code > > but the issue was the same, there was no polling of busy bit from the > > Status Register after having set the QE bit in the Status Register 2 / > > Control Register 1. Then the next SPI command came too early and > > failed because the memory was actually still busy. > > > > I faced this issue with Winbond W25Q256 and W25M512. > > So we can leave this code as is ? > Hi, I checked these data sheets, and in all of them, Bit 0 of the Status Register stands for WIP (Work in progress). S25FL032P http://www.cypress.com/file/196861/download#G4.1231516 S25FL064P http://www.cypress.com/file/196856/download#G3.1231516 S25FL127S http://www.cypress.com/file/177961/download#G3.1468489 S25FL128S, S25FL256S http://www.cypress.com/file/177966/download#G3.1254282 S25FL512S, S70FL01GS http://www.cypress.com/file/177971/download#G3.1238704 S25FL129P http://www.cypress.com/file/197121/download#G4.1231516 S25FL004K, S25FL008K, S25FL016K http://www.mouser.com/ds/2/380/spansion%20inc_s25fl004k-016k_00-329492.pdf#page=16&zoom=auto,61,683 S25FL128K http://www.cypress.com/file/228376/download#G4.1299435 S25FL116K, S25FL132K, S25FL164K http://www.cypress.com/file/196886/download#G3.1239521 S25FL204K http://www.cypress.com/file/196871/download#G4.1354778 I was not able to find the data sheets of S25SL parts. Based on this old patch provided by an Spansion engineer, it looks like S25SL family never existed: https://patchwork.ozlabs.org/patch/59109/ Regards, Joël
On 11/25/2016 05:01 PM, Esponde, Joel wrote: >> -----Message d'origine----- >> De : Marek Vasut [mailto:marek.vasut@gmail.com] >> Envoyé : vendredi 25 novembre 2016 16:08 >> À : Cyrille Pitchen <cyrille.pitchen@atmel.com>; Esponde, Joel >> <Joel.Esponde@Honeywell.com>; linux-mtd@lists.infradead.org >> Objet : Re: [PATCH v3] mtd: spi-nor: fix spansion quad enable >> >> On 11/25/2016 03:50 PM, Cyrille Pitchen wrote: >>> Hi Marek, >> >> Hi, >> >>> Le 25/11/2016 à 15:17, Marek Vasut a écrit : >>>> On 11/23/2016 12:47 PM, Joël Esponde wrote: >>>>> With the S25FL127S nor flash part, each writing to the configuration >>>>> register takes hundreds of ms. During that time, no more accesses >>>>> to the flash should be done (even reads). >>>>> >>>>> This commit adds a wait loop after the register writing until the >>>>> flash finishes its work. >>>>> >>>>> This issue could make rootfs mounting fail when the latter was done >>>>> too much closely to this quad enable bit setting step. And in this >>>>> case, a driver as UBIFS may try to recover the filesystem and may >>>>> broke it completely. >>>> >>>> Does this apply to all spansion chips or only to selected few ? >>>> >>> >>> I've recently faced the very same issue with Winbond memories, which >>> use the same procedure as Spansion to set the Quad Enable bit. >>> More precisely, in my case it was some bare metal (bootloader) code >>> but the issue was the same, there was no polling of busy bit from the >>> Status Register after having set the QE bit in the Status Register 2 / >>> Control Register 1. Then the next SPI command came too early and >>> failed because the memory was actually still busy. >>> >>> I faced this issue with Winbond W25Q256 and W25M512. >> >> So we can leave this code as is ? >> > > > Hi, > > I checked these data sheets, and in all of them, Bit 0 of the Status Register stands for WIP (Work in progress). > > S25FL032P http://www.cypress.com/file/196861/download#G4.1231516 > S25FL064P http://www.cypress.com/file/196856/download#G3.1231516 > S25FL127S http://www.cypress.com/file/177961/download#G3.1468489 > S25FL128S, > S25FL256S http://www.cypress.com/file/177966/download#G3.1254282 > S25FL512S, > S70FL01GS http://www.cypress.com/file/177971/download#G3.1238704 > S25FL129P http://www.cypress.com/file/197121/download#G4.1231516 > S25FL004K, > S25FL008K, > S25FL016K http://www.mouser.com/ds/2/380/spansion%20inc_s25fl004k-016k_00-329492.pdf#page=16&zoom=auto,61,683 > S25FL128K http://www.cypress.com/file/228376/download#G4.1299435 > S25FL116K, > S25FL132K, > S25FL164K http://www.cypress.com/file/196886/download#G3.1239521 > S25FL204K http://www.cypress.com/file/196871/download#G4.1354778 > > I was not able to find the data sheets of S25SL parts. > Based on this old patch provided by an Spansion engineer, it looks like S25SL family never existed: > https://patchwork.ozlabs.org/patch/59109/ Oh great, thanks a lot for checking :-) So we can leave the code as is. Regarding the S25SL, I found [1], but that might be a typo. [1] http://lxr.free-electrons.com/source/arch/powerpc/boot/dts/mpc8536ds.dts?v=3.0
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..21dde52 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1255,6 +1255,13 @@ static int spansion_quad_enable(struct spi_nor *nor) return -EINVAL; } + ret = spi_nor_wait_till_ready(nor); + if (ret) { + dev_err(nor->dev, + "timeout while writing configuration register\n"); + return ret; + } + /* read back and check it */ ret = read_cr(nor); if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
With the S25FL127S nor flash part, each writing to the configuration register takes hundreds of ms. During that time, no more accesses to the flash should be done (even reads). This commit adds a wait loop after the register writing until the flash finishes its work. This issue could make rootfs mounting fail when the latter was done too much closely to this quad enable bit setting step. And in this case, a driver as UBIFS may try to recover the filesystem and may broke it completely. Signed-off-by: Joël Esponde <joel.esponde@honeywell.com> --- drivers/mtd/spi-nor/spi-nor.c | 7 +++++++ 1 file changed, 7 insertions(+)