[PATCH] mtd: spi-nor: fixed spansion quad enable
Cyrille Pitchen
cyrille.pitchen at atmel.com
Fri Oct 21 06:44:45 PDT 2016
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 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...
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/
>
More information about the linux-mtd
mailing list