| Message ID | 1476971026-9665-1-git-send-email-joel.esponde@honeywell.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Joël, Le 20/10/2016 à 15:43, 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 pre check of the quad enable bit to avoid a useless and time consuming writing to the flash, > - 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. > --- > drivers/mtd/spi-nor/spi-nor.c | 31 +++++++++++++++++++++++++++---- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index d0fc165..df43cd7 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1246,18 +1246,41 @@ static int spansion_quad_enable(struct spi_nor *nor) > int ret; > int quad_en = CR_QUAD_EN_SPAN << 8; > > + /* check quad enable bit > + * as S25FL127S takes 200 ms to execute each write of SR & CR > + * registers even if data is the same, write step will be shorted > + * if not needed > + */ > + ret = read_cr(nor); Sorry but the Read Configuration Register (35h) command is not supported by all Spansion (or Spansion-like) memories. For more details, please refer to the JEDEC JESD216B specification, especially the part dealing with the "Quad Enable Requirements". An extract of this specification can also be found in this patch: https://patchwork.ozlabs.org/patch/678408/ Please have a look at values 001b, 100b and 101b for the bits[22:20] of the 15th DWORD. I think your patch is likely to introduce a regression on some old memory parts. However, *if* your S25FL127S memory implements the SFDP tables correctly, bits[22:20] of the 15th DWORD should be set to 101b, hence using SFDP patch above should fix the issue: spansion_new_quad_enable() will be called instead of the current spansion_quad_enable(). This new function adds the very same test on the Quad Enable bit as your patch does. If you want to test the SFDP patch, you will need to apply the whole series which introduces some improvement on QSPI memory support: http://lists.infradead.org/pipermail/linux-mtd/2016-October/069554.html I'm just cautious with Spansion memories: I tested one sample, and I guess it was a S25FL127S, which claimed to be compliant with JESD216 rev B (minor 6) but words after the 9th one were all 0xffffffff. JESD216 (minor 0) describes only 9 DWORDs in the Basic Flash Parameter Table JESD216 rev A (minor 5) introduces 7 new DWORDS, hence 16 DWORDS in the (BFPT). the 15th DWORD provides the Quad Enable Requirements, which describes the procedure to set the Quad Enable bit. If the BFPT of your S25FL127S is incomplete but the memory still claiming to be JESD216 rev B compliant, neither spansion_quad_enable() nor spansion_new_quad_enable() would be called. Since the Quad Enable bit is non-volatile on Spansion memories, it will work if the bit as already been set before. Best regards, Cyrille > + if (ret < 0) { > + dev_err(nor->dev, "error %d reading CR\n", ret); > + return ret; > + } > + if (ret & CR_QUAD_EN_SPAN) { > + /* quad enable bit is already set */ > + return 0; > + } > + > + /* set SR & CR, enable quad I/O */ > write_enable(nor); > > ret = write_sr_cr(nor, quad_en); > if (ret < 0) { > - dev_err(nor->dev, > - "error while writing configuration register\n"); > + dev_err(nor->dev, "error while writing SR and CR registers\n"); > return -EINVAL; > } > > - /* read back and check it */ > + ret = spi_nor_wait_till_ready(nor); > + if (ret) > + return ret; > + > + /* read CR and check it */ > ret = read_cr(nor); > - if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { > + if (ret < 0) { > + dev_err(nor->dev, "error %d reading CR\n", ret); > + return ret; > + } > + if (!(ret & CR_QUAD_EN_SPAN)) { > dev_err(nor->dev, "Spansion Quad bit not set\n"); > return -EINVAL; > } >
Hi Cyrille, First, thanks for your comments ! Please, see below mines. > -----Message d'origine----- > De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] > Envoyé : jeudi 20 octobre 2016 17:23 > À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- > mtd@lists.infradead.org > Objet : Re: [PATCH] mtd: spi-nor: fixed spansion quad enable > > Hi Joël, > > Le 20/10/2016 à 15:43, 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 pre check of the quad enable bit to avoid a useless and time > > consuming writing to the flash, > > - 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. > > --- > > drivers/mtd/spi-nor/spi-nor.c | 31 +++++++++++++++++++++++++++---- > > 1 file changed, 27 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c > > b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..df43cd7 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -1246,18 +1246,41 @@ static int spansion_quad_enable(struct spi_nor > *nor) > > int ret; > > int quad_en = CR_QUAD_EN_SPAN << 8; > > > > + /* check quad enable bit > > + * as S25FL127S takes 200 ms to execute each write of SR & CR > > + * registers even if data is the same, write step will be shorted > > + * if not needed > > + */ > > + ret = read_cr(nor); > > Sorry but the Read Configuration Register (35h) command is not supported > by all Spansion (or Spansion-like) memories. I have a doubt here, if this is really the case, why read_cr() is already called at the end of the current implementation of the spansion_quad_enable() function to check that quad enable bit is set? > For more details, please refer to the JEDEC JESD216B specification, especially > the part dealing with the "Quad Enable Requirements". An extract of this > specification can also be found in this patch: > https://patchwork.ozlabs.org/patch/678408/ > > Please have a look at values 001b, 100b and 101b for the bits[22:20] of the > 15th DWORD. > > I think your patch is likely to introduce a regression on some old memory > parts. > > However, *if* your S25FL127S memory implements the SFDP tables > correctly, bits[22:20] of the 15th DWORD should be set to 101b, hence using > SFDP patch above should fix the issue: > spansion_new_quad_enable() will be called instead of the current > spansion_quad_enable(). This new function adds the very same test on the > Quad Enable bit as your patch does. > > If you want to test the SFDP patch, you will need to apply the whole series > which introduces some improvement on QSPI memory support: > http://lists.infradead.org/pipermail/linux-mtd/2016-October/069554.html Your patch using SFDP to dynamically manage memories seems really great (from a feature point of view, I am not yet able to have an opinion on the implementation...). > > I'm just cautious with Spansion memories: I tested one sample, and I guess it > was a S25FL127S, which claimed to be compliant with JESD216 rev B (minor 6) > but words after the 9th one were all 0xffffffff. > JESD216 (minor 0) describes only 9 DWORDs in the Basic Flash Parameter > Table > JESD216 rev A (minor 5) introduces 7 new DWORDS, hence 16 DWORDS in the > (BFPT). > the 15th DWORD provides the Quad Enable Requirements, which describes > the procedure to set the Quad Enable bit. > > If the BFPT of your S25FL127S is incomplete but the memory still claiming to > be > JESD216 rev B compliant, neither spansion_quad_enable() nor > spansion_new_quad_enable() would be called. Since the Quad Enable bit is > non-volatile on Spansion memories, it will work if the bit as already been set > before. > See below for code that should be IMO added because of the non-volatibility of spansion memories. > > + if (ret < 0) { > > + dev_err(nor->dev, "error %d reading CR\n", ret); > > + return ret; > > + } > > + if (ret & CR_QUAD_EN_SPAN) { > > + /* quad enable bit is already set */ > > + return 0; > > + } > > + > > + /* set SR & CR, enable quad I/O */ > > write_enable(nor); > > > > ret = write_sr_cr(nor, quad_en); > > if (ret < 0) { > > - dev_err(nor->dev, > > - "error while writing configuration register\n"); > > + dev_err(nor->dev, "error while writing SR and CR > registers\n"); > > return -EINVAL; > > } > > > > - /* read back and check it */ > > + ret = spi_nor_wait_till_ready(nor); > > + if (ret) > > + return ret; > > + I think that this wait is required with every memories that store Quad Enable bit in a non-volatile way. I am using today a stripped down kernel with a rootfs based on UBIFS. When this bit is set, UBIFS will start reading the memory 100ms after. As this bit writing step takes up to 200ms, spansion memory is not ready and UBIFS will fail when trying to read the content of the partition. As UBIFS has a recovery mechanism, it will even broke the file system. > > + /* read CR and check it */ > > ret = read_cr(nor); > > - if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { > > + if (ret < 0) { > > + dev_err(nor->dev, "error %d reading CR\n", ret); > > + return ret; > > + } This test on the validity of the returned data may also be useful. When negative errors are returned, it is likely that the CR_QUAD_EN_SPAN bit is set, and so, next test may be meaningless... > > + if (!(ret & CR_QUAD_EN_SPAN)) { > > dev_err(nor->dev, "Spansion Quad bit not set\n"); > > return -EINVAL; > > } > > Best regards, Joël
Hi Joël, Le 21/10/2016 à 12:37, Esponde, Joel a écrit : > Hi Cyrille, > > First, thanks for your comments ! > Please, see below mines. > >> -----Message d'origine----- >> De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] >> Envoyé : jeudi 20 octobre 2016 17:23 >> À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- >> mtd@lists.infradead.org >> Objet : Re: [PATCH] mtd: spi-nor: fixed spansion quad enable >> >> Hi Joël, >> >> Le 20/10/2016 à 15:43, 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 pre check of the quad enable bit to avoid a useless and time >>> consuming writing to the flash, >>> - 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. >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 31 +++++++++++++++++++++++++++---- >>> 1 file changed, 27 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c >>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..df43cd7 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -1246,18 +1246,41 @@ static int spansion_quad_enable(struct spi_nor >> *nor) >>> int ret; >>> int quad_en = CR_QUAD_EN_SPAN << 8; >>> >>> + /* check quad enable bit >>> + * as S25FL127S takes 200 ms to execute each write of SR & CR >>> + * registers even if data is the same, write step will be shorted >>> + * if not needed >>> + */ >>> + ret = read_cr(nor); >> >> Sorry but the Read Configuration Register (35h) command is not supported >> by all Spansion (or Spansion-like) memories. > > I have a doubt here, if this is really the case, why read_cr() is already called at the end of the current implementation of the spansion_quad_enable() function to check that quad enable bit is set? > That's a very good remark and I think the reason is that the current implementation of spansion_quad_enable() suffers from a bug but this one is almost harmless: I assume there is pull-up resistor on the MISO line: spansion_quad_enable() sends a 35h command, which is not supported hence ignored by some memories, then it reads a 1-byte value of FFh due to the pull-up so we pass the test which checks that the Quad Enable bit has successfully been set. Then if we now add the same test before sending the Write Status (01h) command with 2 bytes (SR1 then CR/SR2), we also read a FFh value for the Configuration Register hence we skip the Write Status command because we think the Quad Enable bit has already been set but if we are using a brand new memory still using its factory settings the Quad Enable has never been set and won't be set since we now skip the Write Status command. I think this what currently happens but I need to check with one of my memory samples if I can find one which doesn't support the Read CR command but I not sure to have such a sample... spansion_quad_enable() is used by QSPI memories from some manufacturers other than Spansion. >> For more details, please refer to the JEDEC JESD216B specification, especially >> the part dealing with the "Quad Enable Requirements". An extract of this >> specification can also be found in this patch: >> https://patchwork.ozlabs.org/patch/678408/ >> >> Please have a look at values 001b, 100b and 101b for the bits[22:20] of the >> 15th DWORD. >> >> I think your patch is likely to introduce a regression on some old memory >> parts. >> >> However, *if* your S25FL127S memory implements the SFDP tables >> correctly, bits[22:20] of the 15th DWORD should be set to 101b, hence using >> SFDP patch above should fix the issue: >> spansion_new_quad_enable() will be called instead of the current >> spansion_quad_enable(). This new function adds the very same test on the >> Quad Enable bit as your patch does. >> >> If you want to test the SFDP patch, you will need to apply the whole series >> which introduces some improvement on QSPI memory support: >> http://lists.infradead.org/pipermail/linux-mtd/2016-October/069554.html > > Your patch using SFDP to dynamically manage memories seems really great (from a feature point of view, I am not yet able to have an opinion on the implementation...). > >> >> I'm just cautious with Spansion memories: I tested one sample, and I guess it >> was a S25FL127S, which claimed to be compliant with JESD216 rev B (minor 6) >> but words after the 9th one were all 0xffffffff. >> JESD216 (minor 0) describes only 9 DWORDs in the Basic Flash Parameter >> Table >> JESD216 rev A (minor 5) introduces 7 new DWORDS, hence 16 DWORDS in the >> (BFPT). >> the 15th DWORD provides the Quad Enable Requirements, which describes >> the procedure to set the Quad Enable bit. >> >> If the BFPT of your S25FL127S is incomplete but the memory still claiming to >> be >> JESD216 rev B compliant, neither spansion_quad_enable() nor >> spansion_new_quad_enable() would be called. Since the Quad Enable bit is >> non-volatile on Spansion memories, it will work if the bit as already been set >> before. >> > > See below for code that should be IMO added because of the non-volatibility of spansion memories. > >>> + if (ret < 0) { >>> + dev_err(nor->dev, "error %d reading CR\n", ret); >>> + return ret; >>> + } >>> + if (ret & CR_QUAD_EN_SPAN) { >>> + /* quad enable bit is already set */ >>> + return 0; >>> + } >>> + >>> + /* set SR & CR, enable quad I/O */ >>> write_enable(nor); >>> >>> ret = write_sr_cr(nor, quad_en); >>> if (ret < 0) { >>> - dev_err(nor->dev, >>> - "error while writing configuration register\n"); >>> + dev_err(nor->dev, "error while writing SR and CR >> registers\n"); >>> return -EINVAL; >>> } >>> >>> - /* read back and check it */ >>> + ret = spi_nor_wait_till_ready(nor); >>> + if (ret) >>> + return ret; >>> + > > I think that this wait is required with every memories that store Quad Enable bit in a non-volatile way. > I am using today a stripped down kernel with a rootfs based on UBIFS. > When this bit is set, UBIFS will start reading the memory 100ms after. > As this bit writing step takes up to 200ms, spansion memory is not ready and UBIFS will fail when trying to read the content of the partition. > As UBIFS has a recovery mechanism, it will even broke the file system. > I agree with you about adding the spi_nor_wait_till_ready() call. I plan to add it in my SFDP patch for the spansion_new_quad_enable(), if not done yet. >>> + /* read CR and check it */ >>> ret = read_cr(nor); >>> - if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { >>> + if (ret < 0) { >>> + dev_err(nor->dev, "error %d reading CR\n", ret); >>> + return ret; >>> + } > > This test on the validity of the returned data may also be useful. > When negative errors are returned, it is likely that the CR_QUAD_EN_SPAN bit is set, and so, next test may be meaningless... > >>> + if (!(ret & CR_QUAD_EN_SPAN)) { >>> dev_err(nor->dev, "Spansion Quad bit not set\n"); >>> return -EINVAL; >>> } >>> > > Best regards, > Joël > Best regards, Cyrille
Le 21/10/2016 à 14:50, Cyrille Pitchen a écrit : > Hi Joël, > > Le 21/10/2016 à 12:37, Esponde, Joel a écrit : >> Hi Cyrille, >> >> First, thanks for your comments ! >> Please, see below mines. >> >>> -----Message d'origine----- >>> De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] >>> Envoyé : jeudi 20 octobre 2016 17:23 >>> À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- >>> mtd@lists.infradead.org >>> Objet : Re: [PATCH] mtd: spi-nor: fixed spansion quad enable >>> >>> Hi Joël, >>> >>> Le 20/10/2016 à 15:43, 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 pre check of the quad enable bit to avoid a useless and time >>>> consuming writing to the flash, >>>> - 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. >>>> --- >>>> drivers/mtd/spi-nor/spi-nor.c | 31 +++++++++++++++++++++++++++---- >>>> 1 file changed, 27 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c >>>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..df43cd7 100644 >>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>> @@ -1246,18 +1246,41 @@ static int spansion_quad_enable(struct spi_nor >>> *nor) >>>> int ret; >>>> int quad_en = CR_QUAD_EN_SPAN << 8; >>>> >>>> + /* check quad enable bit >>>> + * as S25FL127S takes 200 ms to execute each write of SR & CR >>>> + * registers even if data is the same, write step will be shorted >>>> + * if not needed >>>> + */ >>>> + ret = read_cr(nor); >>> >>> Sorry but the Read Configuration Register (35h) command is not supported >>> by all Spansion (or Spansion-like) memories. >> >> I have a doubt here, if this is really the case, why read_cr() is already called at the end of the current implementation of the spansion_quad_enable() function to check that quad enable bit is set? >> > > That's a very good remark and I think the reason is that the current > implementation of spansion_quad_enable() suffers from a bug but this one is > almost harmless: > > I assume there is pull-up resistor on the MISO line: spansion_quad_enable() > sends a 35h command, which is not supported hence ignored by some memories, > then it reads a 1-byte value of FFh due to the pull-up so we pass the test > which checks that the Quad Enable bit has successfully been set. > > Then if we now add the same test before sending the Write Status (01h) command > with 2 bytes (SR1 then CR/SR2), we also read a FFh value for the Configuration > Register hence we skip the Write Status command because we think the Quad > Enable bit has already been set but if we are using a brand new memory still > using its factory settings the Quad Enable has never been set and won't be set > since we now skip the Write Status command. > > I think this what currently happens but I need to check with one of my memory > samples if I can find one which doesn't support the Read CR command but I > not sure to have such a sample... The JESD216B specification clearly deals with 2 cases: a first one where the 35h command is not used, I guess because not supported, and a second case using both the 05h and 35h commands to read the SR1 and SR2/CR1 memory registers before writing both SR1 and SR2/CR1 with the 01h command. However I can't provide any example of memory sample falling into the first case. I guess case 1 only applies to old and buggy memories and maybe not so used in production. Besides, your patch is great so I wonder whether it makes sense to reject it just because it might introduce a regression with some unidentified memories... If so, I think we could still find a workaround later to handle those very few (deprecated) parts. The S25FL127S memory is still recommended by Cypress/Spansion for new designs. > > spansion_quad_enable() is used by QSPI memories from some manufacturers other > than Spansion. > >>> For more details, please refer to the JEDEC JESD216B specification, especially >>> the part dealing with the "Quad Enable Requirements". An extract of this >>> specification can also be found in this patch: >>> https://patchwork.ozlabs.org/patch/678408/ >>> >>> Please have a look at values 001b, 100b and 101b for the bits[22:20] of the >>> 15th DWORD. >>> >>> I think your patch is likely to introduce a regression on some old memory >>> parts. >>> >>> However, *if* your S25FL127S memory implements the SFDP tables >>> correctly, bits[22:20] of the 15th DWORD should be set to 101b, hence using >>> SFDP patch above should fix the issue: >>> spansion_new_quad_enable() will be called instead of the current >>> spansion_quad_enable(). This new function adds the very same test on the >>> Quad Enable bit as your patch does. >>> >>> If you want to test the SFDP patch, you will need to apply the whole series >>> which introduces some improvement on QSPI memory support: >>> http://lists.infradead.org/pipermail/linux-mtd/2016-October/069554.html >> >> Your patch using SFDP to dynamically manage memories seems really great (from a feature point of view, I am not yet able to have an opinion on the implementation...). >> >>> >>> I'm just cautious with Spansion memories: I tested one sample, and I guess it >>> was a S25FL127S, which claimed to be compliant with JESD216 rev B (minor 6) >>> but words after the 9th one were all 0xffffffff. >>> JESD216 (minor 0) describes only 9 DWORDs in the Basic Flash Parameter >>> Table >>> JESD216 rev A (minor 5) introduces 7 new DWORDS, hence 16 DWORDS in the >>> (BFPT). >>> the 15th DWORD provides the Quad Enable Requirements, which describes >>> the procedure to set the Quad Enable bit. >>> >>> If the BFPT of your S25FL127S is incomplete but the memory still claiming to >>> be >>> JESD216 rev B compliant, neither spansion_quad_enable() nor >>> spansion_new_quad_enable() would be called. Since the Quad Enable bit is >>> non-volatile on Spansion memories, it will work if the bit as already been set >>> before. >>> >> >> See below for code that should be IMO added because of the non-volatibility of spansion memories. >> >>>> + if (ret < 0) { >>>> + dev_err(nor->dev, "error %d reading CR\n", ret); >>>> + return ret; >>>> + } >>>> + if (ret & CR_QUAD_EN_SPAN) { >>>> + /* quad enable bit is already set */ >>>> + return 0; >>>> + } >>>> + >>>> + /* set SR & CR, enable quad I/O */ >>>> write_enable(nor); >>>> >>>> ret = write_sr_cr(nor, quad_en); >>>> if (ret < 0) { >>>> - dev_err(nor->dev, >>>> - "error while writing configuration register\n"); >>>> + dev_err(nor->dev, "error while writing SR and CR >>> registers\n"); >>>> return -EINVAL; >>>> } >>>> >>>> - /* read back and check it */ >>>> + ret = spi_nor_wait_till_ready(nor); >>>> + if (ret) >>>> + return ret; >>>> + >> >> I think that this wait is required with every memories that store Quad Enable bit in a non-volatile way. >> I am using today a stripped down kernel with a rootfs based on UBIFS. >> When this bit is set, UBIFS will start reading the memory 100ms after. >> As this bit writing step takes up to 200ms, spansion memory is not ready and UBIFS will fail when trying to read the content of the partition. >> As UBIFS has a recovery mechanism, it will even broke the file system. >> > > I agree with you about adding the spi_nor_wait_till_ready() call. I plan to > add it in my SFDP patch for the spansion_new_quad_enable(), if not done yet. > >>>> + /* read CR and check it */ >>>> ret = read_cr(nor); >>>> - if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { >>>> + if (ret < 0) { >>>> + dev_err(nor->dev, "error %d reading CR\n", ret); >>>> + return ret; >>>> + } >> >> This test on the validity of the returned data may also be useful. >> When negative errors are returned, it is likely that the CR_QUAD_EN_SPAN bit is set, and so, next test may be meaningless... >> >>>> + if (!(ret & CR_QUAD_EN_SPAN)) { >>>> dev_err(nor->dev, "Spansion Quad bit not set\n"); >>>> return -EINVAL; >>>> } >>>> >> >> Best regards, >> Joël >> > > Best regards, > > Cyrille > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
Hi Cyrille, > -----Message d'origine----- > De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] > Envoyé : vendredi 21 octobre 2016 15:45 > À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- > mtd@lists.infradead.org > Objet : Re: [PATCH] mtd: spi-nor: fixed spansion quad enable > > > That's a very good remark and I think the reason is that the current > > implementation of spansion_quad_enable() suffers from a bug but this > > one is almost harmless: > > > > I assume there is pull-up resistor on the MISO line: > > spansion_quad_enable() sends a 35h command, which is not supported > > hence ignored by some memories, then it reads a 1-byte value of FFh > > due to the pull-up so we pass the test which checks that the Quad Enable > bit has successfully been set. > > > > Then if we now add the same test before sending the Write Status (01h) > > command with 2 bytes (SR1 then CR/SR2), we also read a FFh value for > > the Configuration Register hence we skip the Write Status command > > because we think the Quad Enable bit has already been set but if we > > are using a brand new memory still using its factory settings the Quad > > Enable has never been set and won't be set since we now skip the Write > Status command. > > > > I think this what currently happens but I need to check with one of my > > memory samples if I can find one which doesn't support the Read CR > > command but I not sure to have such a sample... > > The JESD216B specification clearly deals with 2 cases: a first one where the > 35h command is not used, I guess because not supported, and a second case > using both the 05h and 35h commands to read the SR1 and SR2/CR1 memory > registers before writing both SR1 and SR2/CR1 with the 01h command. > > However I can't provide any example of memory sample falling into the first > case. I guess case 1 only applies to old and buggy memories and maybe not > so used in production. > Besides, your patch is great so I wonder whether it makes sense to reject it > just because it might introduce a regression with some unidentified > memories... > If so, I think we could still find a workaround later to handle those very few > (deprecated) parts. > > The S25FL127S memory is still recommended by Cypress/Spansion for new > designs. > I cannot help here as I have only one part number of flash memory... But from a performance point of view, it would be really great to have this pre test before writing quad enable bit to memory. In my case, I will have to keep this patch because I cannot add useless 200ms delay at boot time. Best regards, Joël
Hi Cyrille, > -----Message d'origine----- > De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] > Envoyé : vendredi 21 octobre 2016 14:50 > À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- > mtd@lists.infradead.org > Objet : Re: [PATCH] mtd: spi-nor: fixed spansion quad enable > > >>> + /* set SR & CR, enable quad I/O */ > >>> write_enable(nor); > >>> > >>> ret = write_sr_cr(nor, quad_en); > >>> if (ret < 0) { > >>> - dev_err(nor->dev, > >>> - "error while writing configuration register\n"); > >>> + dev_err(nor->dev, "error while writing SR and CR > >> registers\n"); > >>> return -EINVAL; > >>> } > >>> > >>> - /* read back and check it */ > >>> + ret = spi_nor_wait_till_ready(nor); > >>> + if (ret) > >>> + return ret; > >>> + > > > > I think that this wait is required with every memories that store Quad > Enable bit in a non-volatile way. > > I am using today a stripped down kernel with a rootfs based on UBIFS. > > When this bit is set, UBIFS will start reading the memory 100ms after. > > As this bit writing step takes up to 200ms, spansion memory is not ready > and UBIFS will fail when trying to read the content of the partition. > > As UBIFS has a recovery mechanism, it will even broke the file system. > > > > I agree with you about adding the spi_nor_wait_till_ready() call. I plan to add > it in my SFDP patch for the spansion_new_quad_enable(), if not done yet. > Good news !! Because I think that this part of the patch is the most critical. Without this, you may broke your filesystem so this is a big issue. Best regards, Joël
Hi Joel, +Marek Le 24/10/2016 à 10:33, Esponde, Joel a écrit : > Hi Cyrille, > >> -----Message d'origine----- >> De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] >> Envoyé : vendredi 21 octobre 2016 14:50 >> À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- >> mtd@lists.infradead.org >> Objet : Re: [PATCH] mtd: spi-nor: fixed spansion quad enable >> >>>>> + /* set SR & CR, enable quad I/O */ >>>>> write_enable(nor); >>>>> >>>>> ret = write_sr_cr(nor, quad_en); >>>>> if (ret < 0) { >>>>> - dev_err(nor->dev, >>>>> - "error while writing configuration register\n"); >>>>> + dev_err(nor->dev, "error while writing SR and CR >>>> registers\n"); >>>>> return -EINVAL; >>>>> } >>>>> >>>>> - /* read back and check it */ >>>>> + ret = spi_nor_wait_till_ready(nor); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>> >>> I think that this wait is required with every memories that store Quad >> Enable bit in a non-volatile way. >>> I am using today a stripped down kernel with a rootfs based on UBIFS. >>> When this bit is set, UBIFS will start reading the memory 100ms after. >>> As this bit writing step takes up to 200ms, spansion memory is not ready >> and UBIFS will fail when trying to read the content of the partition. >>> As UBIFS has a recovery mechanism, it will even broke the file system. >>> >> >> I agree with you about adding the spi_nor_wait_till_ready() call. I plan to add >> it in my SFDP patch for the spansion_new_quad_enable(), if not done yet. >> > > Good news !! > Because I think that this part of the patch is the most critical. > Without this, you may broke your filesystem so this is a big issue. > > Best regards, > > Joël > I've recently tested with two Winbond QSPI memories, W25Q256 and W25M512 and now I can confirm that the issue you reported also applies to those memories: if spi_nor_wait_till_ready() is not called after write_sr_cr(), the next SPI command may be sent too quickly and bad data may be read. Indeed, this issue occured in some bare metal (boot loader) code, when I performed a Fast Read 1-4-4 (EBh) command right after the Write Status Register (01h) command, without polling the BUSY bit with Read Status Register (05h) commands. Also the W25Q256 datasheet I have claims that the memory supports the Read Status Register2 (35h) however the data read from the SFDP table of my memory sample tells that the Quad Enable bit should be set only using the Write Status Register (01h) command with 2 data bytes (SR1 then SR2) and without using the Read Status Register 2 (35h) command: Indeed, bits[22:20] of the 15th dword in the Basic Flash Parameter Table (BFPT) are 100b and not 101b! 100b: QE is bit 1 of status register 2. It is set wia Write Status with two data bytes where bit 1 of the second byte is one. 101b: QE is bit 1 of status register 2. Status register 1 is read using Read Status instruction 05h. Status register 2 is read using instruction 35h. QE is set via Write Status instruction 01h with two data bytes where bit 1 of the second byte is one. I didn't test whether the datasheet is right and the 35h command is actually supported by those Winbond memories so their bits[22:20] of the 15th dword in the BFPT should have been set to 101b instead of 100b. However I'd rather be compliant with the JESD216B specification. So could you please send a new version of your patch adding only the spi_nor_wait_till_ready() call but without adding other read to the Status Register 2/Control Register 1 (35h)? The 2nd procedure, which uses the 35h instruction to check whether the Quad Enable bit has already been set, will be implemented by spansion_new_quad_enable() from the SFDP patch. I've already fixed this patch to add the call of spi_nor_wait_till_ready(). With both your patch and mine, we will fix the issue and still be compliant with the SFDP specification. Best regards, Cyrille
Hi Cyrille, I apologize but I did not have time to work on this. I will try to send you the patch this evening (European time). Regards, Joël Esponde Honeywell | Safety and Productivity Solutions > -----Message d'origine----- > De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] > Envoyé : mercredi 16 novembre 2016 13:58 > À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux- > mtd@lists.infradead.org; Marek Vasut <marex@denx.de> > Objet : Re: [PATCH] mtd: spi-nor: fixed spansion quad enable > > Hi Joel, > > +Marek > > Le 24/10/2016 à 10:33, Esponde, Joel a écrit : > > Hi Cyrille, > > > >> -----Message d'origine----- > >> De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com] > >> Envoyé : vendredi 21 octobre 2016 14:50 À : Esponde, Joel > >> <Joel.Esponde@Honeywell.com>; linux- mtd@lists.infradead.org Objet : > >> Re: [PATCH] mtd: spi-nor: fixed spansion quad enable > >> > >>>>> + /* set SR & CR, enable quad I/O */ > >>>>> write_enable(nor); > >>>>> > >>>>> ret = write_sr_cr(nor, quad_en); > >>>>> if (ret < 0) { > >>>>> - dev_err(nor->dev, > >>>>> - "error while writing configuration > register\n"); > >>>>> + dev_err(nor->dev, "error while writing SR and CR > >>>> registers\n"); > >>>>> return -EINVAL; > >>>>> } > >>>>> > >>>>> - /* read back and check it */ > >>>>> + ret = spi_nor_wait_till_ready(nor); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>> > >>> I think that this wait is required with every memories that store > >>> Quad > >> Enable bit in a non-volatile way. > >>> I am using today a stripped down kernel with a rootfs based on UBIFS. > >>> When this bit is set, UBIFS will start reading the memory 100ms after. > >>> As this bit writing step takes up to 200ms, spansion memory is not > >>> ready > >> and UBIFS will fail when trying to read the content of the partition. > >>> As UBIFS has a recovery mechanism, it will even broke the file system. > >>> > >> > >> I agree with you about adding the spi_nor_wait_till_ready() call. I > >> plan to add it in my SFDP patch for the spansion_new_quad_enable(), if > not done yet. > >> > > > > Good news !! > > Because I think that this part of the patch is the most critical. > > Without this, you may broke your filesystem so this is a big issue. > > > > Best regards, > > > > Joël > > > > I've recently tested with two Winbond QSPI memories, W25Q256 and > W25M512 and now I can confirm that the issue you reported also applies to > those memories: > if spi_nor_wait_till_ready() is not called after write_sr_cr(), the next SPI > command may be sent too quickly and bad data may be read. > Indeed, this issue occured in some bare metal (boot loader) code, when I > performed a Fast Read 1-4-4 (EBh) command right after the Write Status > Register > (01h) command, without polling the BUSY bit with Read Status Register (05h) > commands. > > Also the W25Q256 datasheet I have claims that the memory supports the > Read Status Register2 (35h) however the data read from the SFDP table of > my memory sample tells that the Quad Enable bit should be set only using > the Write Status Register (01h) command with 2 data bytes (SR1 then SR2) > and without using the Read Status Register 2 (35h) command: > > Indeed, bits[22:20] of the 15th dword in the Basic Flash Parameter Table > (BFPT) are 100b and not 101b! > > 100b: QE is bit 1 of status register 2. It is set wia Write Status with two > data bytes where bit 1 of the second byte is one. > > 101b: QE is bit 1 of status register 2. Status register 1 is read using Read > Status instruction 05h. Status register 2 is read using instruction 35h. > QE is set via Write Status instruction 01h with two data bytes where bit > 1 of the second byte is one. > > I didn't test whether the datasheet is right and the 35h command is actually > supported by those Winbond memories so their bits[22:20] of the 15th > dword in the BFPT should have been set to 101b instead of 100b. > However I'd rather be compliant with the JESD216B specification. > > So could you please send a new version of your patch adding only the > spi_nor_wait_till_ready() call but without adding other read to the Status > Register 2/Control Register 1 (35h)? > > The 2nd procedure, which uses the 35h instruction to check whether the > Quad Enable bit has already been set, will be implemented by > spansion_new_quad_enable() from the SFDP patch. I've already fixed this > patch to add the call of spi_nor_wait_till_ready(). > > With both your patch and mine, we will fix the issue and still be compliant > with the SFDP specification. > > Best regards, > > Cyrille
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..df43cd7 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1246,18 +1246,41 @@ static int spansion_quad_enable(struct spi_nor *nor) int ret; int quad_en = CR_QUAD_EN_SPAN << 8; + /* check quad enable bit + * as S25FL127S takes 200 ms to execute each write of SR & CR + * registers even if data is the same, write step will be shorted + * if not needed + */ + ret = read_cr(nor); + if (ret < 0) { + dev_err(nor->dev, "error %d reading CR\n", ret); + return ret; + } + if (ret & CR_QUAD_EN_SPAN) { + /* quad enable bit is already set */ + return 0; + } + + /* set SR & CR, enable quad I/O */ write_enable(nor); ret = write_sr_cr(nor, quad_en); if (ret < 0) { - dev_err(nor->dev, - "error while writing configuration register\n"); + dev_err(nor->dev, "error while writing SR and CR registers\n"); return -EINVAL; } - /* read back and check it */ + ret = spi_nor_wait_till_ready(nor); + if (ret) + return ret; + + /* read CR and check it */ ret = read_cr(nor); - if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { + if (ret < 0) { + dev_err(nor->dev, "error %d reading CR\n", ret); + return ret; + } + if (!(ret & CR_QUAD_EN_SPAN)) { dev_err(nor->dev, "Spansion Quad bit not set\n"); return -EINVAL; }