mtd: spi-nor: winbond: fix w25q128 regression
Subsystems (2)
Michael Walle
maintainerPATCH 1/1 mtd: spi-nor: winbond: fix w25q128 regression
Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") removed the flags for non-SFDP devices. It was assumed that it wasn't in use anymore. This turned out to be wrong. Add the no_sfdp_flags as well as the size again.
---
Hartmut, Linus, could you please test it on your boards? Also, do you have a real name we should put in the Reported-by tag?
This will also need a manual backport to the stable kernels due to the new syntax. But that should be straight forward.
--- drivers/mtd/spi-nor/winbond.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/mtd/spi-nor/winbond.c +2 -0
@@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { }, { .id = SNOR_ID(0xef, 0x40, 0x18), .name = "w25q128", + .size = SZ_16M, .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, }, { .id = SNOR_ID(0xef, 0x40, 0x19), .name = "w25q256",
Signature · 1 line
2.39.2
Am 10.06.2024 um 09:48 schrieb Michael Walle:
Quoted 6 lines
Commit
83e824a4a595("mtd: spi-nor: Correct flags for Winbond w25q128") removed the flags for non-SFDP devices. It was assumed that it wasn't in use anymore. This turned out to be wrong. Add theno_sfdp_flagsas well as the size again.Reported-by e9hack <[email protected]>
Quoted 5 lines
Fixes 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128")Signed-off-by Michael Walle <[email protected]>---
Hartmut, Linus, could you please test it on your boards? Also, do you have a real name we should put in the Reported-by tag?
I cannot test it. I'm using OpenWRT on both routers. OpenWRT uses linux 6.6.32. The differences are to big.
Regards,
Hartmut
Quoted 21 lines
This will also need a manual backport to the stable kernels due to the new syntax. But that should be straight forward.
--- drivers/mtd/spi-nor/winbond.c | 2 ++ 1 file changed, 2 insertions(+)drivers/mtd/spi-nor/winbond.c +2
-0@@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { }, { .id = SNOR_ID(0xef, 0x40, 0x18), .name = "w25q128", + .size = SZ_16M, .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, }, { .id = SNOR_ID(0xef, 0x40, 0x19), .name = "w25q256",
Quoted 11 lines · Michael Walle
Commit
83e824a4a595("mtd: spi-nor: Correct flags for Winbond w25q128") removed the flags for non-SFDP devices. It was assumed that it wasn't in use anymore. This turned out to be wrong. Add theno_sfdp_flagsas well as the size again.Reported-by e9hack <[email protected]>Fixes 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128")Signed-off-by Michael Walle <[email protected]>---
Hartmut, Linus, could you please test it on your boards? Also, do you have a real name we should put in the Reported-by tag?
I'm unable to bring mainline Linux back up on this device, so sadly I can't test it right now. But it sure looks fine to me:
Yours,
Linus Walleij
Quoted 1 line · Michael Walle
Commit
83e824a4a595("mtd: spi-nor: Correct flags for Winbond w25q128")
That commit did:
- { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
- NO_SFDP_FLAGS(SECT_4K) },
+ { "w25q128", INFO(0xef4018, 0, 0, 0)
+ PARSE_SFDP
+ FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
Quoted 22 lines
removed the flags for non-SFDP devices. It was assumed that it wasn't in use anymore. This turned out to be wrong. Add the
no_sfdp_flagsas well as the size again.Reported-by e9hack <[email protected]>Fixes 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128")Signed-off-by Michael Walle <[email protected]>---
Hartmut, Linus, could you please test it on your boards? Also, do you have a real name we should put in the Reported-by tag?This will also need a manual backport to the stable kernels due to the new syntax. But that should be straight forward.
--- drivers/mtd/spi-nor/winbond.c | 2 ++ 1 file changed, 2 insertions(+)drivers/mtd/spi-nor/winbond.c +0
-0
drivers/mtd/spi-nor/winbond.c static const struct flash_info winbond_nor_parts[] = { · line 110
Quoted 6 lines
drivers/mtd/spi-nor/winbond.c +2
-0@@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { }, { .id = SNOR_ID(0xef, 0x40, 0x18), .name = "w25q128", + .size = SZ_16M, .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
and here you add dual and quad to trigger SFDP parsing I guess. All fine if the old flash supports dual and quad read. But please update the commit message describing the intention. With that ACK. Would be good to have this merged soon.
Quoted 3 lines
drivers/mtd/spi-nor/winbond.c +0
-0}, { .id = SNOR_ID(0xef, 0x40, 0x19), .name = "w25q256",
Michael Walle
maintainerHi Tudor,
Quoted 27 lines · Tudor Ambarus
Quoted 1 line · Michael Walle
Commit
83e824a4a595("mtd: spi-nor: Correct flags for Winbond w25q128")That commit did:
(quoted diff) +3
-2- { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) - NO_SFDP_FLAGS(SECT_4K) }, + { "w25q128", INFO(0xef4018, 0, 0, 0) + PARSE_SFDP + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },Quoted 18 lines
removed the flags for non-SFDP devices. It was assumed that it wasn't in use anymore. This turned out to be wrong. Add the
no_sfdp_flagsas well as the size again.Reported-by e9hack <[email protected]>Fixes 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128")Signed-off-by Michael Walle <[email protected]>---
Hartmut, Linus, could you please test it on your boards? Also, do you have a real name we should put in the Reported-by tag?This will also need a manual backport to the stable kernels due to the new syntax. But that should be straight forward.
--- drivers/mtd/spi-nor/winbond.c | 2 ++ 1 file changed, 2 insertions(+)
Quoted 16 lines
drivers/mtd/spi-nor/winbond.c static const struct flash_info winbond_nor_parts[] = { · line 110
Quoted 11 lines
drivers/mtd/spi-nor/winbond.c +2
-0@@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { }, { .id = SNOR_ID(0xef, 0x40, 0x18), .name = "w25q128", + .size = SZ_16M, .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,and here you add dual and quad to trigger SFDP parsing I guess. All fine if the old flash supports dual and quad read. But please update the commit message describing the intention. With that ACK. Would be good to have this merged soon.
Right. It's not because it will trigger the SFDP parsing, but because that what was tested by Esben. We're lucky that this will trigger the SFDP parsing ;) I'll explain that in more detail and add a Link: to the bug report mail.
-michael
Quoted 4 lines
Quoted 4 lines
drivers/mtd/spi-nor/winbond.c +0
-0}, { .id = SNOR_ID(0xef, 0x40, 0x19), .name = "w25q256",
Pratyush Yadav
maintainerQuoted 13 lines · Michael Walle
Hi Tudor,
Quoted 10 lines · Tudor Ambarus
Quoted 1 line · Michael Walle
Commit
83e824a4a595("mtd: spi-nor: Correct flags for Winbond w25q128")That commit did:
(quoted diff) +3
-2- { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) - NO_SFDP_FLAGS(SECT_4K) }, + { "w25q128", INFO(0xef4018, 0, 0, 0) + PARSE_SFDP + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
[...]
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/mtd/spi-nor/winbond.c +0
-0@@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { }, { .id = SNOR_ID(0xef, 0x40, 0x18), .name = "w25q128",Quoted 8 lines
drivers/mtd/spi-nor/winbond.c static const struct flash_info winbond_nor_parts[] = { · line 110
Quoted 3 lines
drivers/mtd/spi-nor/winbond.c +2
-0+ .size = SZ_16M, .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,and here you add dual and quad to trigger SFDP parsing I guess. All fine if the old flash supports dual and quad read. But please update the commit message describing the intention. With that ACK. Would be good to have this merged soon.
Quoted 4 lines
Right. It's not because it will trigger the SFDP parsing, but because that what was tested by Esben. We're lucky that this will trigger the SFDP parsing ;) I'll explain that in more detail and add a Link: to the bug report mail.
Should we treat this flash similar to the Macronix ones Esben sent out patches for [0]? It seems that there are some old parts without SFDP support and new ones with SFDP support. From your comment in [1]:
Quoted 3 lines
This is an entry matching various flash families from Winbond, see my reply in v1. I'm not sure we should remove these as we could break the older ones, which might or might not have SFDP tables. We don't know.
Since the entry matches multiple families, do _all_ of them support dual and quad read? If not, attempting to enable dual or quad reads on them can cause problems.
Also, for parts that _do_ have SFDP available, won't it be better to use the information in SFDP instead of our hard-coded ones anyway? Using SPI_NOR_TRY_SFDP here would let us do that.
[0] https://lore.kernel.org/linux-mtd/[email protected]/
[1] https://lore.kernel.org/linux-mtd/[email protected]/
Signature · 2 lines
Regards, Pratyush Yadav
Michael Walle
maintainerQuoted 36 lines
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
Commit
83e824a4a595("mtd: spi-nor: Correct flags for Winbond w25q128")That commit did:
(quoted diff) +3
-2- { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) - NO_SFDP_FLAGS(SECT_4K) }, + { "w25q128", INFO(0xef4018, 0, 0, 0) + PARSE_SFDP + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },[...]
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/mtd/spi-nor/winbond.c +0
-0@@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { }, { .id = SNOR_ID(0xef, 0x40, 0x18), .name = "w25q128",Quoted 8 lines
drivers/mtd/spi-nor/winbond.c static const struct flash_info winbond_nor_parts[] = { · line 110
Quoted 3 lines
drivers/mtd/spi-nor/winbond.c +2
-0+ .size = SZ_16M, .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,and here you add dual and quad to trigger SFDP parsing I guess. All fine if the old flash supports dual and quad read. But please update the commit message describing the intention. With that ACK. Would be good to have this merged soon.
Quoted 4 lines
Right. It's not because it will trigger the SFDP parsing, but because that what was tested by Esben. We're lucky that this will trigger the SFDP parsing ;) I'll explain that in more detail and add a Link: to the bug report mail.
Should we treat this flash similar to the Macronix ones Esben sent out patches for [0]? It seems that there are some old parts without SFDP support and new ones with SFDP support. From your comment in [1]:
Quoted 7 lines
Quoted 3 lines
This is an entry matching various flash families from Winbond, see my reply in v1. I'm not sure we should remove these as we could break the older ones, which might or might not have SFDP tables. We don't know.
Since the entry matches multiple families, do _all_ of them support dual and quad read? If not, attempting to enable dual or quad reads on them can cause problems.
I rely on the information Helmut provided. Also the w25q64 and the w25q256 both have these flags set. So I'd say it's less likely the 128 doesn't support it.
Quoted 3 lines
Also, for parts that _do_ have SFDP available, won't it be better to use the information in SFDP instead of our hard-coded ones anyway? Using
SPI_NOR_TRY_SFDPhere would let us do that.
Sure, but this is about fixing the referenced commit. A later patch will then move that to TRY_SFDP. We can't fix this regression by introducing new code IMHO. This seems to be the easiest fix.
-michael
Pratyush Yadav
maintainerQuoted 35 lines · Michael Walle
Quoted 35 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Commit
83e824a4a595("mtd: spi-nor: Correct flags for Winbond w25q128")That commit did:
(quoted diff) +3
-2- { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) - NO_SFDP_FLAGS(SECT_4K) }, + { "w25q128", INFO(0xef4018, 0, 0, 0) + PARSE_SFDP + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },[...]
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
drivers/mtd/spi-nor/winbond.c +0
-0@@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { }, { .id = SNOR_ID(0xef, 0x40, 0x18),Quoted 8 lines
drivers/mtd/spi-nor/winbond.c static const struct flash_info winbond_nor_parts[] = { · line 110
Quoted 4 lines
drivers/mtd/spi-nor/winbond.c +2
-0.name = "w25q128", + .size = SZ_16M, .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,and here you add dual and quad to trigger SFDP parsing I guess. All fine if the old flash supports dual and quad read. But please update the commit message describing the intention. With that ACK. Would be good to have this merged soon.
Quoted 4 lines
Right. It's not because it will trigger the SFDP parsing, but because that what was tested by Esben. We're lucky that this will trigger the SFDP parsing ;) I'll explain that in more detail and add a Link: to the bug report mail.
Should we treat this flash similar to the Macronix ones Esben sent out patches for [0]? It seems that there are some old parts without SFDP support and new ones with SFDP support. From your comment in [1]:
Quoted 3 lines
This is an entry matching various flash families from Winbond, see my reply in v1. I'm not sure we should remove these as we could break the older ones, which might or might not have SFDP tables. We don't know.
Quoted 7 lines
Quoted 3 lines
Since the entry matches multiple families, do _all_ of them support dual and quad read? If not, attempting to enable dual or quad reads on them can cause problems.
I rely on the information Helmut provided. Also the w25q64 and the w25q256 both have these flags set. So I'd say it's less likely the 128 doesn't support it.
Okay, fair enough.
Quoted 8 lines
Quoted 4 lines
Also, for parts that _do_ have SFDP available, won't it be better to use the information in SFDP instead of our hard-coded ones anyway? Using
SPI_NOR_TRY_SFDPhere would let us do that.Sure, but this is about fixing the referenced commit. A later patch will then move that to
TRY_SFDP. We can't fix this regression by introducing new code IMHO. This seems to be the easiest fix.
New code will make it harder to backport to stable kernels. Beyond that, I don't see why we can't fix a regression with new code.
Here's why I suggested this: before 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128"), all flashes with this ID got only the SECT_4K flag -- and thus only single SPI mode. After that commit, all flashes with this ID got their settings configured via SFDP. Using the TRY_SFDP approach allows both of those configurations to co-exist. Old ones still use the old configuration, new ones get to use SFDP.
Now we add a different configuration that adds dual and quad reads to these old flashes. As mentioned above, this is unlikely to cause problems, but a new configuration regardless. So _in principle_ I think TRY_SFDP would be the best balance.
But I get your point -- since both w25q64 and w25q256 have these flags, it is likely someone just never bothered updating w25q128. So this patch LGTM. I'll apply it once you send a new version with an updated commit message.
Quoted 8 lines
-michael
Signature · 2 lines
Regards, Pratyush Yadav