| Message ID | 1450205301-32207-1-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Commit | 67b9bcd36906e12a15ffec19463afbbd6a41660e |
| Headers | show |
On Tue, Dec 15, 2015 at 10:48:20AM -0800, 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/ > > Fixes: 357ca38d4751 ("mtd: spi-nor: support lock/unlock/is_locked for Winbond") > Fixes: c6fc2171b249 ("mtd: spi-nor: disable protection for Winbond flash at startup") > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > Reported-by: Felix Fietkau <nbd@openwrt.org> > Cc: Felix Fietkau <nbd@openwrt.org> Felix, Can I get a Tested-by? I'm going to send this for 4.4 still, if possible. Brian
On Mon, Jan 04, 2016 at 06:29:28PM -0800, Brian Norris wrote: > On Tue, Dec 15, 2015 at 10:48:20AM -0800, 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/ > > > > Fixes: 357ca38d4751 ("mtd: spi-nor: support lock/unlock/is_locked for Winbond") > > Fixes: c6fc2171b249 ("mtd: spi-nor: disable protection for Winbond flash at startup") > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > Reported-by: Felix Fietkau <nbd@openwrt.org> > > Cc: Felix Fietkau <nbd@openwrt.org> > > Felix, > > Can I get a Tested-by? I'm going to send this for 4.4 still, if > possible. Despite the lack of response, pushed both to linux-mtd.git, as they are obvious responses to the reported regressions/bugs. Brian
On 2016-01-06 01:02, Brian Norris wrote: > On Mon, Jan 04, 2016 at 06:29:28PM -0800, Brian Norris wrote: >> On Tue, Dec 15, 2015 at 10:48:20AM -0800, 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/ >> > >> > Fixes: 357ca38d4751 ("mtd: spi-nor: support lock/unlock/is_locked for Winbond") >> > Fixes: c6fc2171b249 ("mtd: spi-nor: disable protection for Winbond flash at startup") >> > Signed-off-by: Brian Norris <computersforpeace@gmail.com> >> > Reported-by: Felix Fietkau <nbd@openwrt.org> >> > Cc: Felix Fietkau <nbd@openwrt.org> >> >> Felix, >> >> Can I get a Tested-by? I'm going to send this for 4.4 still, if >> possible. > > Despite the lack of response, pushed both to linux-mtd.git, as they are > obvious responses to the reported regressions/bugs. Sorry for the delay, I don't have time to test that at the moment. I'll try to find the time for it soon. - Felix
On Wed, 2016-01-06 at 01:03 +0100, Felix Fietkau wrote: > On 2016-01-06 01:02, Brian Norris wrote: > > On Mon, Jan 04, 2016 at 06:29:28PM -0800, Brian Norris wrote: > >> On Tue, Dec 15, 2015 at 10:48:20AM -0800, 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/ > >> > > >> > Fixes: 357ca38d4751 ("mtd: spi-nor: support lock/unlock/is_locked for Winbond") > >> > Fixes: c6fc2171b249 ("mtd: spi-nor: disable protection for Winbond flash at startup") > >> > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > >> > Reported-by: Felix Fietkau <nbd@openwrt.org> > >> > Cc: Felix Fietkau <nbd@openwrt.org> > >> > >> Felix, > >> > >> Can I get a Tested-by? I'm going to send this for 4.4 still, if > >> possible. > > > > Despite the lack of response, pushed both to linux-mtd.git, as they are > > obvious responses to the reported regressions/bugs. > Sorry for the delay, I don't have time to test that at the moment. I'll > try to find the time for it soon. > > - Felix Hi Brain, Sorry for later reply, I have tested these patches. it works ok on oak-rev4. Although it does not support WP operation for Winbond nor flash. Bayi
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
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
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
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
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
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
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
Hi Brian, Le 01/04/2016 22:27, Brian Norris a écrit : > 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? > Yes you're right. The Memory Type and the Memory Density bytes are assigned by the manufacturers. I guess they just set the values they want following their own logic. For instance mx25l25635e, mx25l25655e and mx25l25673g all use 0x19 for the Memory Density however both the 35e and 73g use 0x20 for the Memory Type whereas the 55e use 0x26 instead. Why? I don't know. Maybe different product families or hardware technologies. > 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.) > I guess it could be possible for them, for instance Spansion does (did?) so. However I don't expect other manufacturers to change their naming policy. My understanding is that they assume this is not an issue since we know what the actual part number of the memory is embedded in our board so we shouldn't need to read the JEDEC ID to guess this part number at runtime. Then, it seems you're right when you propose to more rely on the DT compatible string and add specific entries in the spi_nor_ids[] table with flags to declare the supported hardware capabilities. I've tried this approach in v5 of my series for support of 4byte address op codes. >> 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. > Maybe some flags in specific entries to declare some implementation quirks ? >> 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. > Indeed Macronix suggested us to use the SFDP tables. I guess all manufacturers tend to implement the latest version of the SFDP standard even if it breaks compatibility with implementations of older memories. To differentiate the MX25L25635E and MX25L25673G, they told us to combine info read from both the SFDP tables and the Status Register: on 73G the Quad Enable bit is always set and cannot be cleared whereas this bit is cleared as a default factory setting. However we pointed out that this bit is non-volatile and will be set to 1 during the very first boot so still cannot make the difference between the 35E and the 73G. Then they suggest us to try to clear the QE bit (only possible on 35E) but I don't think it can be considered as a clean implementation... Also in this particular example, I don't see how the SFDP tables could help. Nevertheless, SFDP tables might help with other topics such as what procedures to use to enable the Quad Enable bit (if it exists), to enter/leave for 4-4-4 and 0-4-4 (XIP) modes. Latest version of the SFDP tables uses bits in some words to declared the supported procedures. It is not stated explicitly but there is almost one bit/procedure per manufacturer. Some procedures are almost the same except some tiny change, which looks like a bug fixup. One procedure asks to always write 2 bytes during Write Status operations otherwise SR2 is implicitly reset hence clearing the QE bit. Its variant claims it is allowed the write only one byte for Write Status, SR2 being left unchanged. The manufacturer is not provided but the procedures match the one implemented in the spi-nor framework to enable the QE bit in Spansion memories. > 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 > Best regards, Cyrille
Hi Cyrille, On Mon, Apr 04, 2016 at 05:33:30PM +0200, Cyrille Pitchen wrote: > Le 01/04/2016 22:27, Brian Norris a écrit : > > On Wed, Mar 30, 2016 at 02:47:48PM +0200, Cyrille Pitchen wrote: > >> Just a general remark: maybe reading the JEDEC ID is not a so reliable mean to > >> discover SPI flash hardware capabilities at runtime. [...] > >> 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. > > [...] > Then, it seems you're right when you propose to more rely on the DT compatible > string and add specific entries in the spi_nor_ids[] table with flags to > declare the supported hardware capabilities. > > I've tried this approach in v5 of my series for support of 4byte address op > codes. One note about this: I think this is something that Rafal (and I, to some extent) was really trying to avoid: having to specify the exact part number in every board file / DTS file -- as that makes it much more difficult to support a lot of small variations to the same board. For example, I believe some production lines like to swap out one or more flash even on the same product, due to supply or other reasons. That's not to say we can't do this (it's necessary to do *something* more than just ID-based detection); but perhaps there's something we can still do to minimize the damage? I don't have a lot of brilliant ideas right now... (Maybe this line of discussion should be carried to your other patch thread.) > >> 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. > > > > Maybe some flags in specific entries to declare some implementation quirks ? Perhaps, if we can figure out which ones are broken, and we know that *all* flash with that ID are broken in the same way. (It'd really suck if the same ID had two different SFDP implementation...) > >> 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. > > > > Indeed Macronix suggested us to use the SFDP tables. I guess all manufacturers > tend to implement the latest version of the SFDP standard even if it breaks > compatibility with implementations of older memories. Then maybe it's time we finally bite the bullet and try to phase in some level of SFDP support. We probably don't want to (and can't) rely on it unconditionally, but maybe we can start flagging off some entries which should/shouldn't support SFDP (ideally, I'd like to flag those that *don't* support the standard properly, as we can reasonably expect new flash to get it correct now). > To differentiate the MX25L25635E and MX25L25673G, they told us to combine Why did you want to differentiate these? (I feel like I'm missing an unstated detail here.) > info read from both the SFDP tables and the Status Register: on 73G the > Quad Enable bit is always set and cannot be cleared whereas this bit is cleared > as a default factory setting. > However we pointed out that this bit is non-volatile and will be set to 1 > during the very first boot so still cannot make the difference between the > 35E and the 73G. > Then they suggest us to try to clear the QE bit (only possible on 35E) but > I don't think it can be considered as a clean implementation... > > Also in this particular example, I don't see how the SFDP tables could help. Yeah, I wasn't suggesting it for this case, exactly. But we're really gonna have problems if we can't determine anything other than density from ID. [snip] Brian
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 49883905a434..f5d59de1ee6e 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1200,8 +1200,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || JEDEC_MFR(info) == SNOR_MFR_INTEL || - JEDEC_MFR(info) == SNOR_MFR_SST || - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { + JEDEC_MFR(info) == SNOR_MFR_SST) { write_enable(nor); write_sr(nor, 0); } @@ -1217,8 +1216,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) mtd->_read = spi_nor_read; /* NOR protection support for STmicro/Micron chips and similar */ - if (JEDEC_MFR(info) == SNOR_MFR_MICRON || - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { + if (JEDEC_MFR(info) == SNOR_MFR_MICRON) { nor->flash_lock = stm_lock; nor->flash_unlock = stm_unlock; nor->flash_is_locked = stm_is_locked; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index c8723b62c4cd..bc742dac7d3a 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -25,7 +25,7 @@ #define SNOR_MFR_MACRONIX CFI_MFR_MACRONIX #define SNOR_MFR_SPANSION CFI_MFR_AMD #define SNOR_MFR_SST CFI_MFR_SST -#define SNOR_MFR_WINBOND 0xef +#define SNOR_MFR_WINBOND 0xef /* Also used by some Spansion */ /* * Note on opcode nomenclature: some opcodes have a format like
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/ Fixes: 357ca38d4751 ("mtd: spi-nor: support lock/unlock/is_locked for Winbond") Fixes: c6fc2171b249 ("mtd: spi-nor: disable protection for Winbond flash at startup") Signed-off-by: Brian Norris <computersforpeace@gmail.com> Reported-by: Felix Fietkau <nbd@openwrt.org> Cc: Felix Fietkau <nbd@openwrt.org> --- drivers/mtd/spi-nor/spi-nor.c | 6 ++---- include/linux/mtd/spi-nor.h | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-)