[PATCH] mtd: spi-nor: fixed spansion quad enable

[PATCH] mtd: spi-nor: fixed spansion quad enable

[Joël Esponde]

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);
+	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;
 	}
-- 
2.7.4

Re: [PATCH] mtd: spi-nor: fixed spansion quad enable

[Cyrille Pitchen]

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;
>  	}
> 

RE: [PATCH] mtd: spi-nor: fixed spansion quad enable

[Esponde, Joel]

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

Re: [PATCH] mtd: spi-nor: fixed spansion quad enable

[Cyrille Pitchen]

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

Re: [PATCH] mtd: spi-nor: fixed spansion quad enable

[Cyrille Pitchen]

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/
> 

RE: [PATCH] mtd: spi-nor: fixed spansion quad enable

[Esponde, Joel]

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

RE: [PATCH] mtd: spi-nor: fixed spansion quad enable

[Esponde, Joel]

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

Re: [PATCH] mtd: spi-nor: fixed spansion quad enable

[Cyrille Pitchen]

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

RE: [PATCH] mtd: spi-nor: fixed spansion quad enable

[Esponde, Joel]

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

[PATCH v2] mtd: spi-nor: fixed spansion quad enable

[Joël Esponde]

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.
---
 drivers/mtd/spi-nor/spi-nor.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d0fc165..a945122 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1255,7 +1255,11 @@ static int spansion_quad_enable(struct spi_nor *nor)
 		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))) {
 		dev_err(nor->dev, "Spansion Quad bit not set\n");
-- 
2.7.4

Re: [PATCH v2] mtd: spi-nor: fixed spansion quad enable

[Cyrille Pitchen]

Hi Joel,

Le 22/11/2016 à 23:24, 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.

Just few formal remarks about common usage and according to
Documentation/process/submitting-patches.rst:
- The summary phrase of the subject line should describe changes in imperative
  mood, so please use something like
  "mtd: spi-nor: fix spansion_quad_enable..." instead of
  "mtd: spi-nor: fixED spansion_quad_enable...".
- The commit message should be line wrapped at 75 columns.
- You must add your "Signed-off-by:". It is mandatory to know who's introduced
  the commit when browsing the logs.

> ---
>  drivers/mtd/spi-nor/spi-nor.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d0fc165..a945122 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1255,7 +1255,11 @@ static int spansion_quad_enable(struct spi_nor *nor)
>  		return -EINVAL;
>  	}
>  
> -	/* read back and check it */
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret)
I agree this is not done in macronix_quad_enable() but maybe a dev_err()
message may notify about the reason of the failure, just like what is done
for write_sr_cr() and read_cr().
This point is optional!

> +		return ret;
> +	
It seems there is a tab at the beginning of the empty line above.

> +	/* read CR and check it */
Your patch deals with the missing call of spi_nor_wait_till_ready() so
there is no strong reason to also modify the comment about read_cr().
The general idea is to make the patch modify only what needs to be.

>  	ret = read_cr(nor);
>  	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
>  		dev_err(nor->dev, "Spansion Quad bit not set\n");
> 

Finally, it may help you to run the scripts/checkpatch.pl tool on your patch
so this script reports you any warnings and errors in the syntax before
submitting your patch.

Best regards,

Cyrille

[PATCH v3] mtd: spi-nor: fix spansion quad enable

[Joël Esponde]

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(+)

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))) {
-- 
2.7.4

RE: [PATCH v2] mtd: spi-nor: fixed spansion quad enable

[Esponde, Joel]

Hi Cyrille,

Thanks for your explanations and patience!
I will try to avoid these "noob" mistakes next time! :-)

Joël Esponde
Honeywell | Safety and Productivity Solutions

> -----Message d'origine-----
> De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> Envoyé : mercredi 23 novembre 2016 11:39
> À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux-
> mtd@lists.infradead.org
> Objet : Re: [PATCH v2] mtd: spi-nor: fixed spansion quad enable
> 
> Hi Joel,
> 
> Le 22/11/2016 à 23:24, 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.
> 
> Just few formal remarks about common usage and according to
> Documentation/process/submitting-patches.rst:
> - The summary phrase of the subject line should describe changes in
> imperative
>   mood, so please use something like
>   "mtd: spi-nor: fix spansion_quad_enable..." instead of
>   "mtd: spi-nor: fixED spansion_quad_enable...".
> - The commit message should be line wrapped at 75 columns.
> - You must add your "Signed-off-by:". It is mandatory to know who's
> introduced
>   the commit when browsing the logs.
> 
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..a945122 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -1255,7 +1255,11 @@ static int spansion_quad_enable(struct spi_nor
> *nor)
> >  		return -EINVAL;
> >  	}
> >
> > -	/* read back and check it */
> > +	ret = spi_nor_wait_till_ready(nor);
> > +	if (ret)
> I agree this is not done in macronix_quad_enable() but maybe a dev_err()
> message may notify about the reason of the failure, just like what is done for
> write_sr_cr() and read_cr().
> This point is optional!
> 
> > +		return ret;
> > +
> It seems there is a tab at the beginning of the empty line above.
> 
> > +	/* read CR and check it */
> Your patch deals with the missing call of spi_nor_wait_till_ready() so there is
> no strong reason to also modify the comment about read_cr().
> The general idea is to make the patch modify only what needs to be.
> 
> >  	ret = read_cr(nor);
> >  	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
> >  		dev_err(nor->dev, "Spansion Quad bit not set\n");
> >
> 
> Finally, it may help you to run the scripts/checkpatch.pl tool on your patch so
> this script reports you any warnings and errors in the syntax before
> submitting your patch.
> 
> Best regards,
> 
> Cyrille

Re: [PATCH v3] mtd: spi-nor: fix spansion quad enable

[Cyrille Pitchen]

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))) {
> 

Re: [PATCH v2] mtd: spi-nor: fixed spansion quad enable

[Marek Vasut]

On 11/23/2016 03:27 PM, Esponde, Joel wrote:
> Hi Cyrille,
> 
> Thanks for your explanations and patience!
> I will try to avoid these "noob" mistakes next time! :-)

You can start by NOT top-posting, it's really frowned upon :)

> Joël Esponde
> Honeywell | Safety and Productivity Solutions
> 
>> -----Message d'origine-----
>> De : Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>> Envoyé : mercredi 23 novembre 2016 11:39
>> À : Esponde, Joel <Joel.Esponde@Honeywell.com>; linux-
>> mtd@lists.infradead.org
>> Objet : Re: [PATCH v2] mtd: spi-nor: fixed spansion quad enable
>>
>> Hi Joel,
>>
>> Le 22/11/2016 à 23:24, 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.
>>
>> Just few formal remarks about common usage and according to
>> Documentation/process/submitting-patches.rst:
>> - The summary phrase of the subject line should describe changes in
>> imperative
>>   mood, so please use something like
>>   "mtd: spi-nor: fix spansion_quad_enable..." instead of
>>   "mtd: spi-nor: fixED spansion_quad_enable...".
>> - The commit message should be line wrapped at 75 columns.
>> - You must add your "Signed-off-by:". It is mandatory to know who's
>> introduced
>>   the commit when browsing the logs.
>>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>> b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..a945122 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -1255,7 +1255,11 @@ static int spansion_quad_enable(struct spi_nor
>> *nor)
>>>  		return -EINVAL;
>>>  	}
>>>
>>> -	/* read back and check it */
>>> +	ret = spi_nor_wait_till_ready(nor);
>>> +	if (ret)
>> I agree this is not done in macronix_quad_enable() but maybe a dev_err()
>> message may notify about the reason of the failure, just like what is done for
>> write_sr_cr() and read_cr().
>> This point is optional!
>>
>>> +		return ret;
>>> +
>> It seems there is a tab at the beginning of the empty line above.
>>
>>> +	/* read CR and check it */
>> Your patch deals with the missing call of spi_nor_wait_till_ready() so there is
>> no strong reason to also modify the comment about read_cr().
>> The general idea is to make the patch modify only what needs to be.
>>
>>>  	ret = read_cr(nor);
>>>  	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
>>>  		dev_err(nor->dev, "Spansion Quad bit not set\n");
>>>
>>
>> Finally, it may help you to run the scripts/checkpatch.pl tool on your patch so
>> this script reports you any warnings and errors in the syntax before
>> submitting your patch.
>>
>> Best regards,
>>
>> Cyrille
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 


-- 
Best regards,
Marek Vasut

RE: [PATCH v2] mtd: spi-nor: fixed spansion quad enable

[Esponde, Joel]

> -----Message d'origine-----
> De : Marek Vasut [mailto:marex@denx.de]
> Envoyé : mercredi 23 novembre 2016 16:36
> À : Esponde, Joel <Joel.Esponde@Honeywell.com>; Cyrille Pitchen
> <cyrille.pitchen@atmel.com>; linux-mtd@lists.infradead.org
> Objet : Re: [PATCH v2] mtd: spi-nor: fixed spansion quad enable
> 
> On 11/23/2016 03:27 PM, Esponde, Joel wrote:
> > Hi Cyrille,
> >
> > Thanks for your explanations and patience!
> > I will try to avoid these "noob" mistakes next time! :-)
> 
> You can start by NOT top-posting, it's really frowned upon :)
> 

I see there is still a long way to go!

 Joël Esponde
Honeywell | Safety and Productivity Solutions

Re: [PATCH v3] mtd: spi-nor: fix spansion quad enable

[Marek Vasut]

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))) {
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH v3] mtd: spi-nor: fix spansion quad enable

[Cyrille Pitchen]

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))) {
>>
> 
> 

Re: [PATCH v3] mtd: spi-nor: fix spansion quad enable

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

RE: [PATCH v3] mtd: spi-nor: fix spansion quad enable

[Esponde, Joel]

> -----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

Re: [PATCH v3] mtd: spi-nor: fix spansion quad enable

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

[PATCH] mtd: spi-nor: fixed spansion quad enable

[Joël Esponde]

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);
+	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;
 	}
-- 
2.7.4