The Winbond "w25q128" (actual vendor name W25Q128JV) has exactly the same flags as the sibling device "w25q128jv". The devices both require unlocking to enable write access.
The actual product naming between devices vs the Linux strings in winbond.c:
0xef4018: "w25q128" W25Q128JV-IM/JM 0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ
The latter device, "w25q128jv" supports features named DTQ and QPI, otherwise it is the same.
Not having the right flags has the annoying side effect that write access does not work.
After this patch I can write to the flash on the Inteno XG6846 router.
The flash memory also supports dual and quad SPI modes. This does not currently manifest, but by turning on SFDP parsing, the right SPI modes are emitted in /sys/kernel/debug/spi-nor/spi1.0/capabilities for this chip, so we also turn on this.
Cc: stable@vger.kernel.org Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Linus Walleij linus.walleij@linaro.org --- Changes in v2: - Only add the write access flags. - Use SFDP parsing to properly detect the various available SPI modes. - Link to v1: https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c... --- drivers/mtd/spi-nor/winbond.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c index 834d6ba5ce70..6c82e525c801 100644 --- a/drivers/mtd/spi-nor/winbond.c +++ b/drivers/mtd/spi-nor/winbond.c @@ -121,7 +121,8 @@ static const struct flash_info winbond_nor_parts[] = { { "w25q80bl", INFO(0xef4014, 0, 64 * 1024, 16) NO_SFDP_FLAGS(SECT_4K) }, { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) - NO_SFDP_FLAGS(SECT_4K) }, + PARSE_SFDP + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, { "w25q256", INFO(0xef4019, 0, 64 * 1024, 512) NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) .fixups = &w25q256_fixups },
--- base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 change-id: 20230711-spi-nor-winbond-w25q128-321a602ee267
Best regards,
Hi, Linus,
On 13.07.2023 00:59, Linus Walleij wrote:
The Winbond "w25q128" (actual vendor name W25Q128JV) has exactly the same flags as the sibling device "w25q128jv". The devices both require unlocking to enable write access.
The actual product naming between devices vs the Linux strings in winbond.c:
0xef4018: "w25q128" W25Q128JV-IM/JM 0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ
The latter device, "w25q128jv" supports features named DTQ and QPI, otherwise it is the same.
Not having the right flags has the annoying side effect that write access does not work.
I guess you refer to the locking flags. Probably your flash has the non volatile block protection (BP) bits from the Status Register set, which means the entire flash is write protected. The factory default for these bits is 0/disabled on this flash so someone must have played with them. The reason why one may want write protection set is to avoid inadvertent writes during power-up. One can control whether to disable the software write protection at boot time with the MTD_SPI_NOR_SWP_ configs.
After this patch I can write to the flash on the Inteno XG6846 router.
The flash memory also supports dual and quad SPI modes. This does not currently manifest, but by
The fasted mode is chosen after SFDP parsing, so you should use quad reads if your controller also supports 4 I/O lines.
turning on SFDP parsing, the right SPI modes are emitted in /sys/kernel/debug/spi-nor/spi1.0/capabilities for this chip, so we also turn on this.
Cc: stable@vger.kernel.org Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Linus Walleij linus.walleij@linaro.org
Changes in v2:
- Only add the write access flags.
- Use SFDP parsing to properly detect the various available SPI modes.
- Link to v1: https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c...
drivers/mtd/spi-nor/winbond.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c index 834d6ba5ce70..6c82e525c801 100644 --- a/drivers/mtd/spi-nor/winbond.c +++ b/drivers/mtd/spi-nor/winbond.c @@ -121,7 +121,8 @@ static const struct flash_info winbond_nor_parts[] = { { "w25q80bl", INFO(0xef4014, 0, 64 * 1024, 16) NO_SFDP_FLAGS(SECT_4K) }, { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
while here try, using INFO with INFO(0xef4018, 0, 0, 0), those parameters shall be discovered at run-time, so we prepare to get rid of explicitly setting them sooner or later.
NO_SFDP_FLAGS(SECT_4K) },
PARSE_SFDPFLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
Looks good. Also I would like you to run a small sanity test, just to make sure the flash works after your changes. You can do that with mtd_debug utility, see an example on Miquel's commit message from: https://lore.kernel.org/linux-mtd/d479489736ee193609816dc2003bd0fb@walle.cc/...
Cheers, ta
Hi,
Am 2023-07-13 05:32, schrieb Tudor Ambarus:
Hi, Linus,
On 13.07.2023 00:59, Linus Walleij wrote:
The Winbond "w25q128" (actual vendor name W25Q128JV) has exactly the same flags as the sibling device "w25q128jv". The devices both require unlocking to enable write access.
The actual product naming between devices vs the Linux strings in winbond.c:
0xef4018: "w25q128" W25Q128JV-IM/JM 0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ
The latter device, "w25q128jv" supports features named DTQ and QPI, otherwise it is the same.
Not having the right flags has the annoying side effect that write access does not work.
I guess you refer to the locking flags. Probably your flash has the non volatile block protection (BP) bits from the Status Register set, which means the entire flash is write protected. The factory default for these bits is 0/disabled on this flash so someone must have played with them. The reason why one may want write protection set is to avoid inadvertent writes during power-up. One can control whether to disable the software write protection at boot time with the MTD_SPI_NOR_SWP_ configs.
After this patch I can write to the flash on the Inteno XG6846 router.
The flash memory also supports dual and quad SPI modes. This does not currently manifest, but by
The fasted mode is chosen after SFDP parsing, so you should use quad reads if your controller also supports 4 I/O lines.
turning on SFDP parsing, the right SPI modes are emitted in /sys/kernel/debug/spi-nor/spi1.0/capabilities for this chip, so we also turn on this.
Cc: stable@vger.kernel.org Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Linus Walleij linus.walleij@linaro.org
Changes in v2:
- Only add the write access flags.
- Use SFDP parsing to properly detect the various available SPI modes.
- Link to v1:
https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c...
drivers/mtd/spi-nor/winbond.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c index 834d6ba5ce70..6c82e525c801 100644 --- a/drivers/mtd/spi-nor/winbond.c +++ b/drivers/mtd/spi-nor/winbond.c @@ -121,7 +121,8 @@ static const struct flash_info winbond_nor_parts[] = { { "w25q80bl", INFO(0xef4014, 0, 64 * 1024, 16) NO_SFDP_FLAGS(SECT_4K) }, { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
while here try, using INFO with INFO(0xef4018, 0, 0, 0), those parameters shall be discovered at run-time, so we prepare to get rid of explicitly setting them sooner or later.
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.
NO_SFDP_FLAGS(SECT_4K) },
Thus, I'd also keep this one.
-michael
PARSE_SFDPFLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },Looks good. Also I would like you to run a small sanity test, just to make sure the flash works after your changes. You can do that with mtd_debug utility, see an example on Miquel's commit message from: https://lore.kernel.org/linux-mtd/d479489736ee193609816dc2003bd0fb@walle.cc/...
Cheers, ta
On 13.07.2023 10:01, Michael Walle wrote:
Hi,
Am 2023-07-13 05:32, schrieb Tudor Ambarus:
Hi, Linus,
On 13.07.2023 00:59, Linus Walleij wrote:
The Winbond "w25q128" (actual vendor name W25Q128JV) has exactly the same flags as the sibling device "w25q128jv". The devices both require unlocking to enable write access.
The actual product naming between devices vs the Linux strings in winbond.c:
0xef4018: "w25q128" W25Q128JV-IM/JM 0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ
The latter device, "w25q128jv" supports features named DTQ and QPI, otherwise it is the same.
Not having the right flags has the annoying side effect that write access does not work.
I guess you refer to the locking flags. Probably your flash has the non volatile block protection (BP) bits from the Status Register set, which means the entire flash is write protected. The factory default for these bits is 0/disabled on this flash so someone must have played with them. The reason why one may want write protection set is to avoid inadvertent writes during power-up. One can control whether to disable the software write protection at boot time with the MTD_SPI_NOR_SWP_ configs.
After this patch I can write to the flash on the Inteno XG6846 router.
The flash memory also supports dual and quad SPI modes. This does not currently manifest, but by
The fasted mode is chosen after SFDP parsing, so you should use quad reads if your controller also supports 4 I/O lines.
turning on SFDP parsing, the right SPI modes are emitted in /sys/kernel/debug/spi-nor/spi1.0/capabilities for this chip, so we also turn on this.
Cc: stable@vger.kernel.org Suggested-by: Michael Walle michael@walle.cc Signed-off-by: Linus Walleij linus.walleij@linaro.org
Changes in v2:
- Only add the write access flags.
- Use SFDP parsing to properly detect the various
available SPI modes.
- Link to v1:
https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c...
drivers/mtd/spi-nor/winbond.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c index 834d6ba5ce70..6c82e525c801 100644 --- a/drivers/mtd/spi-nor/winbond.c +++ b/drivers/mtd/spi-nor/winbond.c @@ -121,7 +121,8 @@ static const struct flash_info winbond_nor_parts[] = { { "w25q80bl", INFO(0xef4014, 0, 64 * 1024, 16) NO_SFDP_FLAGS(SECT_4K) }, { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
while here try, using INFO with INFO(0xef4018, 0, 0, 0), those parameters shall be discovered at run-time, so we prepare to get rid of explicitly setting them sooner or later.
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.
I'd take the risk and break the older ones if there are some that don't define SFDP indeed, just to handle the conflict properly. We can't encourage code based on assumptions otherwise we'll get back to the knotted spi-nor code that we tried to untie in the last years.
- NO_SFDP_FLAGS(SECT_4K) },
Thus, I'd also keep this one.
Keeping this one does not have the effect that you want as SECT_4K is used in spi_nor_no_sfdp_init_params() which is not called when PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if bugs will be reported, I commit I'll fix them in the same release cycle.
If both of you agree, I'll amend Linus's v4 patch when applying.
Cheers, ta
Hi,
while here try, using INFO with INFO(0xef4018, 0, 0, 0), those parameters shall be discovered at run-time, so we prepare to get rid of explicitly setting them sooner or later.
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.
I'd take the risk and break the older ones if there are some that don't define SFDP indeed, just to handle the conflict properly. We can't encourage code based on assumptions otherwise we'll get back to the knotted spi-nor code that we tried to untie in the last years.
Wait a minute, now I'm the more conservative one of the both of us? (:
Jokes aside, basically you are saying that if there are two flashes with the same id, one supports JEDEC one doesn't, we can brake the latter one.
- NO_SFDP_FLAGS(SECT_4K) },
Thus, I'd also keep this one.
Keeping this one does not have the effect that you want as SECT_4K is used in spi_nor_no_sfdp_init_params() which is not called when PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if bugs will be reported, I commit I'll fix them in the same release cycle.
Mhh, I should have been more curious to why Linus needed the PARSE_SFDP flag in the first place. Taking a closer look, that is because in the legacy behavior, the SFDP is only read if the chip supports dual or quad read (whatever the rationale was for that).
Also, if PARSE_SFDP is set, none of the no_sfdp_flags are ever handled. If the chip doesn't support the SFDP is will just fail probing.
As I'm reading the code right now, for the new behavior it is either * expect the flash supports SFDP, if not, probe fails * expect the flash to don't support SFDP, no SFDP is ever read at all
Shouldn't we handle the third case in the new behavior, too: * start with the static data we have and try reading SFDP to amend/correct it.
If not, will you accept breakage for future flashes, too? Looking at winbond.c for example, I guess all chips with 0xef40.. 0xef50.. and 0xef60.. supports SFDP nowadays and most of them only have SECT_4K set.
If both of you agree, I'll amend Linus's v4 patch when applying.
So it would be the first chip without flags at all? Then we could drop the entry entirely :) And if we do this, then we should also drop all the other entries for the newer winbond flashes.
If you decide to break older flashes, then I'd prefer to also drop the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).
-michael
On 7/18/23 07:25, Michael Walle wrote:
Hi,
while here try, using INFO with INFO(0xef4018, 0, 0, 0), those parameters shall be discovered at run-time, so we prepare to get rid of explicitly setting them sooner or later.
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.
I'd take the risk and break the older ones if there are some that don't define SFDP indeed, just to handle the conflict properly. We can't encourage code based on assumptions otherwise we'll get back to the knotted spi-nor code that we tried to untie in the last years.
Wait a minute, now I'm the more conservative one of the both of us? (:
;)
Jokes aside, basically you are saying that if there are two flashes with the same id, one supports JEDEC one doesn't, we can brake the latter one.
I say I don't want to suffocate the code based on assumptions and I'm ready to take the risk and break some presumably old flashes that don't support SFDP. If we know for sure that we break old variants of this flash ID, then we have to rework the core now. Otherwise I'll rework it when a bug is reported.
- NO_SFDP_FLAGS(SECT_4K) },
Thus, I'd also keep this one.
Keeping this one does not have the effect that you want as SECT_4K is used in spi_nor_no_sfdp_init_params() which is not called when PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if bugs will be reported, I commit I'll fix them in the same release cycle.
Mhh, I should have been more curious to why Linus needed the PARSE_SFDP flag in the first place. Taking a closer look, that is because in the legacy behavior, the SFDP is only read if the chip supports dual or quad read (whatever the rationale was for that).
dual or quad reads are params that could be discovered in the first version of SFDP, that why they associated them with reading SFDP. Not great, but it worked.
Also, if PARSE_SFDP is set, none of the no_sfdp_flags are ever handled. If the chip doesn't support the SFDP is will just fail probing.
As I'm reading the code right now, for the new behavior it is either * expect the flash supports SFDP, if not, probe fails * expect the flash to don't support SFDP, no SFDP is ever read at all
sort of. It's more elaborate than that. Check spi_nor_init_params().
Shouldn't we handle the third case in the new behavior, too: * start with the static data we have and try reading SFDP to amend/correct it.
This case is already supported and it's the old way of initializing flash parameters. Check spi_nor_init_params_deprecated().
I don't want to do that for the SFDP capable flashes for now, otherwise we'll have again parameters initialized all over the place, which results in ugly hard to read code. You have the fixup hooks if you need to amend SFDP data. And since the first table of SFDP is mandatory (BFPT), if you set PARSE_SFDP and get an error, then you shouldn't have set PARSE_SFDP in the first place. Optional SFDP tables return void and we have a rollback mechanism for the parameters set in those optional tables in case of errors.
If not, will you accept breakage for future flashes, too? Looking at winbond.c for example, I guess all chips with 0xef40.. 0xef50.. and 0xef60.. supports SFDP nowadays and most of them only have SECT_4K set.
I will. Note that you have to actually have a physical flash to do changes, I don't queue untested patches.
If both of you agree, I'll amend Linus's v4 patch when applying.
So it would be the first chip without flags at all? Then we could drop the entry entirely :) And if we do this, then we should also
No, you have the locking flags that can't be discovered by parsing SFDP, thus you need to define a flash entry for it.
drop all the other entries for the newer winbond flashes.
If you can test it, and there's no dedicated compatible for that flash, I'm ok to drop them.
If you decide to break older flashes, then I'd prefer to also drop the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).
Isn't v4 already doing this? I'll amend it if not. Waiting for ack from both you and Linus.
Cheers, ta
On 7/18/23 08:44, Tudor Ambarus wrote:
If you decide to break older flashes, then I'd prefer to also drop the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).
Isn't v4 already doing this? I'll amend it if not. Waiting for ack from both you and Linus.
It needs another round of testing if we set n_sectors and sector_size to 0, thus Linus has to spend a little more time on it, sorry.
Let's agree on the final version and close the topic. Cheers, ta
Jokes aside, basically you are saying that if there are two flashes with the same id, one supports JEDEC one doesn't, we can brake the latter one.
I say I don't want to suffocate the code based on assumptions and I'm ready to take the risk and break some presumably old flashes that don't support SFDP. If we know for sure that we break old variants of this flash ID, then we have to rework the core now. Otherwise I'll rework it when a bug is reported.
Ok, fair enough.
- NO_SFDP_FLAGS(SECT_4K) },
Thus, I'd also keep this one.
Keeping this one does not have the effect that you want as SECT_4K is used in spi_nor_no_sfdp_init_params() which is not called when PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if bugs will be reported, I commit I'll fix them in the same release cycle.
Mhh, I should have been more curious to why Linus needed the PARSE_SFDP flag in the first place. Taking a closer look, that is because in the legacy behavior, the SFDP is only read if the chip supports dual or quad read (whatever the rationale was for that).
dual or quad reads are params that could be discovered in the first version of SFDP, that why they associated them with reading SFDP. Not great, but it worked.
Also, if PARSE_SFDP is set, none of the no_sfdp_flags are ever handled. If the chip doesn't support the SFDP is will just fail probing.
As I'm reading the code right now, for the new behavior it is either * expect the flash supports SFDP, if not, probe fails * expect the flash to don't support SFDP, no SFDP is ever read at all
sort of. It's more elaborate than that. Check spi_nor_init_params().
I had checked that. And there is either parse_sfdp = true, which will fail if SFDP is not available - or SPI_NOR_SKIP_SFDP, which won't handle SFDP at all. And then there is the third case which will handle legacy behavior. So what am I missing? )
Shouldn't we handle the third case in the new behavior, too: * start with the static data we have and try reading SFDP to amend/correct it.
This case is already supported and it's the old way of initializing flash parameters. Check spi_nor_init_params_deprecated().
But it's not encouraged for new flashes and it doesn't work. Otherwise we wouldn't have a problem here. It (suprisingly!) only parses SFDP if the dual or quad flags are set.
I don't want to do that for the SFDP capable flashes for now, otherwise we'll have again parameters initialized all over the place, which results in ugly hard to read code. You have the fixup hooks if you need to amend SFDP data. And since the first table of SFDP is mandatory (BFPT), if you set PARSE_SFDP and get an error, then you shouldn't have set PARSE_SFDP in the first place.
Unless of course there are flashes with the same id where one supports SFDP and another don't. But as you said, we can handle that one if there is actual breakage. I'm fine with that.
Optional SFDP tables return void and we have a rollback mechanism for the parameters set in those optional tables in case of errors.
If not, will you accept breakage for future flashes, too? Looking at winbond.c for example, I guess all chips with 0xef40.. 0xef50.. and 0xef60.. supports SFDP nowadays and most of them only have SECT_4K set.
I will. Note that you have to actually have a physical flash to do changes, I don't queue untested patches.
If both of you agree, I'll amend Linus's v4 patch when applying.
So it would be the first chip without flags at all? Then we could drop the entry entirely :) And if we do this, then we should also
No, you have the locking flags that can't be discovered by parsing SFDP, thus you need to define a flash entry for it.
Ah, right, they were added :)
drop all the other entries for the newer winbond flashes.
If you can test it, and there's no dedicated compatible for that flash, I'm ok to drop them.
I've had a look at the dedicated compatibles, too. They are only needed for the spi core to probe. But if I read the code correctly, it should work just fine with the generic driver (even if it is probed by name). Right?
If you decide to break older flashes, then I'd prefer to also drop the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).
Isn't v4 already doing this? I'll amend it if not. Waiting for ack from both you and Linus.
FWIW, I'm fine with the removed no_sfdp_flags if INFO(, 0, 0, 0).
-michael
Hi, Linus,
On 7/18/23 09:32, Michael Walle wrote:
FWIW, I'm fine with the removed no_sfdp_flags if INFO(, 0, 0, 0).
We'll need a v5 where you test again the flash with mtd_utils, as we want to get rid of n_sectors and sectors_size and instead determine them from SFDP. We agreed that the flash entry should be defined with the following params:
{ "w25q128", INFO(0xef4018, 0, 0, 0) PARSE_SFDP FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
Thanks! ta
On Tue, Jul 18, 2023 at 11:47 AM Tudor Ambarus tudor.ambarus@linaro.org wrote:
Hi, Linus,
On 7/18/23 09:32, Michael Walle wrote:
FWIW, I'm fine with the removed no_sfdp_flags if INFO(, 0, 0, 0).
We'll need a v5 where you test again the flash with mtd_utils, as we want to get rid of n_sectors and sectors_size and instead determine them from SFDP. We agreed that the flash entry should be defined with the following params:
{ "w25q128", INFO(0xef4018, 0, 0, 0) PARSE_SFDP FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
Roger that!
I sent a v5 which does this, tested by taking the reported size in dmesg and divide by reported (correct) eraseblock size and it results in the same number of sectors as well.
I put the details into the commit message.
Yours, Linus Walleij
linux-stable-mirror@lists.linaro.org
-
Linus Walleij -
Michael Walle -
Tudor Ambarus