Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1364975 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions  (aliased with Winbond) Matthias Schiffer <mschiffer@universe-factory.net> - 2016-03-26 20:00 +0100
    Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions  (aliased with Winbond) Matthias Schiffer <mschiffer@universe-factory.net> - 2016-03-28 01:00 +0200
      Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions  (aliased with Winbond) Brian Norris <computersforpeace@gmail.com> - 2016-03-28 23:00 +0200
        Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions  (aliased with Winbond) Matthias Schiffer <mschiffer@universe-factory.net> - 2016-03-29 21:20 +0200
        Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions  (aliased with Winbond) Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2016-03-30 14:50 +0200
          Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions  (aliased with Winbond) James Cameron <quozl@laptop.org> - 2016-04-01 05:10 +0200
          Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions  (aliased with Winbond) Brian Norris <computersforpeace@gmail.com> - 2016-04-01 22:30 +0200

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1365209

[Multipart message — attachments visible in raw view] — view raw

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.

Matthias

[toc] | [prev] | [next] | [standalone]

#1365668

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

[toc] | [prev] | [next] | [standalone]

#1366558

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1367178

Hi all,

[...] 
> 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?
> 

Just a general remark: maybe reading the JEDEC ID is not a so reliable mean to
discover SPI flash hardware capabilities at runtime.

Two weeks ago some Macronix people came to Atmel to present us next evolutions
of SPI flashes. We took this opportunity to ask them some questions and one of
them was about memories with different hardware capabilities sharing the very
same JEDEC ID. One example is Macronix MX25L25635E vs MX25L25673G.

They explained to us that for Macronix memories, the 2byte product ID is to be
split into a 1byte code for the memory type and a second byte for the memory
denstity. For instance:
C2: Manufacturer ID, Macronix
20: Memory Type, SPI NOR flash
19: Memory density, 256Mib

Hence the JEDEC ID only provides information about the memory size and all
SPI NOR memories of a given size actually share the same JEDEC ID.

Similar cases can also be found with other manufacturers: Micron, Winbond,
Spansion... 

Also the Macronix engineers asked us how software applications drive the (Q)SPI
memories. I answered them that Linux or u-boot use a static table indexed by
the JEDEC ID, which provides the hardware capabilities. I guess they didn't
expect software developers to use the JEDEC ID for this purpose.
Well, it's just a feeling.

Then the Macronix engineers proposed to use the Serial Flash Discoverable
Parameter (SFDP) tables to make the difference between memories sharing the
same JEDEC ID. This might help us in some cases.
However we should be cautious when using this standard: last year, I've tried
to discover hardware parameters through these tables when I was working with
Spansion and Micron memories. I found out the Parameter Table Pointers inside
the SFDP Header were expressed as byte offset with one memory and as dword
offset with the other.
So I gave up using these tables since some memories diverged from the standard,
which was "work in progress" at that time.

Anyway if we cannot completely rely on the SFDP tables we could still use
DT properties but we should no longer expect to guess all hardware parameters
from the JEDEC ID alone.

Best regards,

Cyrille

[toc] | [prev] | [next] | [standalone]

#1368978

On Wed, Mar 30, 2016 at 02:47:48PM +0200, Cyrille Pitchen wrote:
> Hi all,
> 
> [...] 
> > 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?
> > 
> 
> Just a general remark: maybe reading the JEDEC ID is not a so reliable mean to
> discover SPI flash hardware capabilities at runtime.
> 
> Two weeks ago some Macronix people came to Atmel to present us next evolutions
> of SPI flashes. We took this opportunity to ask them some questions and one of
> them was about memories with different hardware capabilities sharing the very
> same JEDEC ID. One example is Macronix MX25L25635E vs MX25L25673G.
> 
> They explained to us that for Macronix memories, the 2byte product ID is to be
> split into a 1byte code for the memory type and a second byte for the memory
> denstity. For instance:
> C2: Manufacturer ID, Macronix
> 20: Memory Type, SPI NOR flash
> 19: Memory density, 256Mib
> 
> Hence the JEDEC ID only provides information about the memory size and all
> SPI NOR memories of a given size actually share the same JEDEC ID.

Yes, that's true.

(Reference: Open Firmware SPI Flash driver used at OLPC.)

> 
> Similar cases can also be found with other manufacturers: Micron, Winbond,
> Spansion... 
> 
> Also the Macronix engineers asked us how software applications drive the (Q)SPI
> memories. I answered them that Linux or u-boot use a static table indexed by
> the JEDEC ID, which provides the hardware capabilities. I guess they didn't
> expect software developers to use the JEDEC ID for this purpose.
> Well, it's just a feeling.
> 
> Then the Macronix engineers proposed to use the Serial Flash Discoverable
> Parameter (SFDP) tables to make the difference between memories sharing the
> same JEDEC ID. This might help us in some cases.
> However we should be cautious when using this standard: last year, I've tried
> to discover hardware parameters through these tables when I was working with
> Spansion and Micron memories. I found out the Parameter Table Pointers inside
> the SFDP Header were expressed as byte offset with one memory and as dword
> offset with the other.
> So I gave up using these tables since some memories diverged from the standard,
> which was "work in progress" at that time.

Yes, I too was unable to use SFDP; some devices didn't have them, some
didn't seem to be good data.

> 
> Anyway if we cannot completely rely on the SFDP tables we could still use
> DT properties but we should no longer expect to guess all hardware parameters
> from the JEDEC ID alone.

We solved this problem by not using our SPI Flash from the kernel, but
that's not the best outcome.  ;-)

> 
> Best regards,
> 
> Cyrille

-- 
James Cameron
http://quozl.netrek.org/

[toc] | [prev] | [next] | [standalone]

#1369581

On Wed, Mar 30, 2016 at 02:47:48PM +0200, Cyrille Pitchen wrote:
> Hi all,
> 
> [...] 
> > 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?
> > 
> 
> Just a general remark: maybe reading the JEDEC ID is not a so reliable mean to
> discover SPI flash hardware capabilities at runtime.
> 
> Two weeks ago some Macronix people came to Atmel to present us next evolutions
> of SPI flashes. We took this opportunity to ask them some questions and one of
> them was about memories with different hardware capabilities sharing the very
> same JEDEC ID. One example is Macronix MX25L25635E vs MX25L25673G.
> 
> They explained to us that for Macronix memories, the 2byte product ID is to be
> split into a 1byte code for the memory type and a second byte for the memory
> denstity. For instance:
> C2: Manufacturer ID, Macronix
> 20: Memory Type, SPI NOR flash
> 19: Memory density, 256Mib
> 
> Hence the JEDEC ID only provides information about the memory size and all
> SPI NOR memories of a given size actually share the same JEDEC ID.

That doesn't seem to be true though. There are flash entries for
Macronix with different ID's, but the same density. So maybe they mean
that *some* SPI NOR memories of a given size share the same JEDEC ID?

Anyway, even if they keep this pattern for the first 3 bytes, isn't it
possible for them to include extra bytes to differentiate? Or, you know,
support a standard like SFPD properly. But there are understood (but not
insurmountable) problems with that. (And anyway, SFDP doesn't tell us
all the things we need.)

> Similar cases can also be found with other manufacturers: Micron, Winbond,
> Spansion... 
> 
> Also the Macronix engineers asked us how software applications drive the (Q)SPI
> memories. I answered them that Linux or u-boot use a static table indexed by
> the JEDEC ID, which provides the hardware capabilities. I guess they didn't
> expect software developers to use the JEDEC ID for this purpose.
> Well, it's just a feeling.
> 
> Then the Macronix engineers proposed to use the Serial Flash Discoverable
> Parameter (SFDP) tables to make the difference between memories sharing the
> same JEDEC ID. This might help us in some cases.
> However we should be cautious when using this standard: last year, I've tried
> to discover hardware parameters through these tables when I was working with
> Spansion and Micron memories. I found out the Parameter Table Pointers inside
> the SFDP Header were expressed as byte offset with one memory and as dword
> offset with the other.

Yeah, I noticed this. And I think one or more of them noticed their
error and fixed it in later revs, so you can't depend on a manufacturer
always having the same broken interpretation consistently.

> So I gave up using these tables since some memories diverged from the standard,
> which was "work in progress" at that time.
> 
> Anyway if we cannot completely rely on the SFDP tables we could still use
> DT properties but we should no longer expect to guess all hardware parameters
> from the JEDEC ID alone.

In your conversations, did the vendors actually suggest a practical
method to differentiate flash? Since they've all screwed up SFDP, that's
not going to fly, unless we (e.g.) blacklist certain flash. Anyway, I'd
love to have some basic support for SFDP, even if we have to be
conservative at first. For one, I think it'd be fair to add another
compatible property "jedec,sfdp-vXXX", and then we only use that on
flash that support the actual spec.

Also rather than adding a ton of new DT properties, I think it might
make more sense to just add more DT compatible matches for the actual
part number (they're not going to start re-using part numbers are
they??). That way we don't rely on the DT writer to get every little
detail right, when the driver *should* be able to hold this info.

Brian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web