Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions (aliased with Winbond)

[Matthias Schiffer]

On 03/28/2016 10:56 PM, Brian Norris wrote:
> On Mon, Mar 28, 2016 at 12:52:51AM +0200, Matthias Schiffer wrote:
>> On 03/26/2016 07:57 PM, Matthias Schiffer wrote:
>>> On 12/15/2015 07:48 PM, Brian Norris wrote:
>>>> Spansion and Winbond have occasionally used the same manufacturer ID,
>>>> and they don't support the same features. Particularly, writing SR=0
>>>> seems to break read access for Spansion's s25fl064k. Unfortunately, we
>>>> don't currently have a way to differentiate these Spansion and Winbond
>>>> parts, so rather than regressing support for these Spansion flash, let's
>>>> drop the new Winbond lock/unlock support for now. We can try to address
>>>> Winbond support during the next release cycle.
>>>>
>>>> Original discussion:
>>>>
>>>> http://patchwork.ozlabs.org/patch/549173/
>>>> http://patchwork.ozlabs.org/patch/553683/
>>>>
>>>
>>> I have a few devices with a s25fl064k lying around, and I was not able to
>>> reproduce this issue. I've re-applied "mtd: spi-nor: disable protection for
>>> Winbond flash at startup" and the flash is readable just fine.
>>>
>>> On the contrary, I've come across a board with a s25fl064k that comes up
>>> locked, so removing the protection bits would be necessary. (I was not yet
>>> able to check if the patch actually fixes writing to the flash on this
>>> board, as I don't have access to the device myself, but I hope to get a
>>> response on that soon.)
>>>
>>> Regards,
>>> Matthias
>>>
>>
>> I made the mistake of trusting the kernel log and OpenWrt Wiki when making
>> my previous tests.
>>
>> All of the boards I was talking about in my last mail actually have a
>> Winbond w25q64, not a s25fl064k (two board I tested the patch on, and the
>> board that was reported to come up locked). The kernel detects the w25q64
>> as s25fl064k, as these two flash chips have the same JEDEC ID 0xef4017.
> 
> That's interesting; I didn't notice we had duplicate entries for the
> same ID. But apparently, the committers did:
> 
>   commit f2df1ae3fe8d ("mtd: m25p80: Add support for two new Spansion
>   SPI devices (S25FL-K)")
>   ...
>   "Note that both parts exhibit a Winbond manufacturer ID so they might
>   also be added to that section."
> 
> But this is interesting: I see the latest datasheet for Spansion
> s25fl064k says it supports the Block Protect bits in the Status
> Register, so presumably *some* version of s25fl064k should support
> write_sr(nor, 0) to unlock it at boot...
> 
> If Felix's initial report is indeed correct, then I think we have:
> (1) Spansion s25fl064k without Block Protect support (that breaks if you
>     try to write SR=0)
> (2) Spansion s25fl064k with Block Protect support (that requires you to
>     unlock at boot by writing SR=0 (?))
> (3) Winbond w25q64 with Block Protect support (that requires you to
>     unlock at boot by writing SR=0)
> 
> And (1)-(3) all report the same ID, and (1) is incompatible with (2) and
> (3). Am I right? Are flash vendors really this insane? Should we all
> just give up and go home?
> 
> Brian
> 

After some more tests, I believe the situation isn't actually that bad. I
don't think there are two fundamentally different revisions of the
s25fl064k, and the s25fl064k and w25q64 are actually compatible - with one
caveat: I think Spansion chips really don't like it when you don't wait
until WIP is reset after writing the status register.

As "mtd: spi-nor: wait for SR_WIP to clear on initial unlock" has fixed
that, I think we can just unconditionally unlock all flash with
SNOR_MFR_WINBOND again without breaking Spansion flash.

As I don't have a s25fl064k board at hand, this assessment is based on a
test with a different Spansion flash (the s25fl064p). The s25fl064p
exhibits the same behaviour: when I change the code to perform the initial
unlock on the chip as well, reading the flash fails. But when I also apply
"mtd: spi-nor: wait for SR_WIP to clear on initial unlock", everything is
fine again.


One more, completely unrelated note: Two Spansion flashs have a typo in
their identification string, the s25sl032p and s25sl064p are actually
called s25fl032p and s25fl064p (pretty much everything found on Google for
the wrong string refers to Linux kernel code or logs). Can these just be
fixed, or is something relying on these strings not to change (devicetrees
or something)? If we can't change them, we should at least add a comment.

Regards,
Matthias

Attachment: signature.asc
Description: OpenPGP digital signature