[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 at atmel.com]
>> Envoyé : jeudi 20 octobre 2016 17:23
>> À : Esponde, Joel <Joel.Esponde at Honeywell.com>; linux-
>> mtd at 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