[PATCH 0/2] mtd: spi-nor: winbond: Add support for flashes with several dies

[PATCH 0/2] mtd: spi-nor: winbond: Add support for flashes with several dies

[Miquel Raynal]

Some Winbond devices are made of up smaller stacked dies, like for
instance W25Q01JV which is a 1G-bit serial flash memory made of two
512M-bit dies, and W25Q02JV which is made of four of them. Internally,
the dies can either be in the active state (only one at a time), the
others being in the idle state. Problem: there are commands impacting
all 4 dies, for which we monitor the state of the chip reading the
status register, but the status register only gives the status of the
active die. It was observed a possible internal deviation of up to 200us
between each die when doing similar operations, which can lead to
races. This series aims at supporting these chips properly.

---
Miquel Raynal (2):
      mtd: spi-nor: winbond: Add support for w25q01jv
      mtd: spi-nor: winbond: Add support for w25q02jv

 drivers/mtd/spi-nor/winbond.c | 87 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)
---
base-commit: cfb48994051444aa6b620e48898f95ea91d193c5
change-id: 20241223-winbond-6-12-rc1-nor-volatile-bit-ffa03c566b92

Best regards,
-- 
Miquel Raynal <miquel.raynal@bootlin.com>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Miquel Raynal]

Add support for Winbond w25q01jv spi-nor chip.

This chip is internally made of two dies with linear addressing
capabilities to make it transparent to the user that two dies were
used. There is one drawback however, the read status operation is racy
as the status bit only gives the active die status and not the status of
the other die. For commands affecting the two dies, it means if another
command is sent too fast after the first die has returned a valid status
(deviation can be up to 200us), the chip will get corrupted/in an
unstable state.

This chip hence requires a better status register read. There are three
solutions here:
- Either we wait about 200us after getting a first status ready
feedback, to cover possible internal deviations.
- Or we always check all internal dies (which takes about 30us per die).

This second option being always faster and also being totally safe, we
implement a specific hook for the status register read. flash_speed
benchmarks show no difference with this implementation, compared to the
regular status read core function, the difference being part of the
natural deviation with this benchmark:

	> Without the fixup
	$ flash_speed /dev/mtd0 -c100 -d
	eraseblock write speed is 442 KiB/s
	eraseblock read speed is 1606 KiB/s
	page write speed is 439 KiB/s
	page read speed is 1520 KiB/s
	2 page write speed is 441 KiB/s
	2 page read speed is 1562 KiB/s
	erase speed is 68 KiB/s

	> With the fixup
	$ flash_speed /dev/mtd0 -c100 -d
	eraseblock write speed is 428 KiB/s
	eraseblock read speed is 1626 KiB/s
	page write speed is 426 KiB/s
	page read speed is 1538 KiB/s
	2 page write speed is 426 KiB/s
	2 page read speed is 1574 KiB/s
	erase speed is 66 KiB/s

As there are very few situations where this can actually happen, a
status register write being the most likely one, another possibility
might have been to use volatile writes instead of non-volatile writes,
as most of the deviation comes from the action of writing the bit. But
this would overlook other possible actions where both dies can be used
at the same time like a chip erase (or any erase over the die boundary
in general). This last approach would have the least impact but because
it does not feel like it is totally safe to use and because the impact
of the second solution presented above is also negligible, we keep this
second approach for now (which can be further tuned later if it appears
to be too impacting in the end).

However, the fixup, whatever which one we pick, must be applied on
multi-die chips, which hence must be properly flagged. The SFDP tables
implemented give a lot of information but the die details are part of an
optional table that is not implemented, hence we use a post parsing
fixup hook to set the params->n_dice value manually.

Link: https://www.winbond.com/resource-files/W25Q01JV%20SPI%20RevE%2003042024%20Plus.pdf
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Here is the basic test procedure output:

$ cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
w25q01jv
$ cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
ef4021
$ cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
winbond
$ xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450060101ff00060110800000ff84000102d00000ff03000102f000
00ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffe520fbffffffff3f44eb086b083b42bbfeffffffffff
0000ffff40eb0c200f5210d800003602a60082ea14e2e96376337a757a75
f7a2d55c19f74dffe970f9a5ffffffffffffffffffffffffffffffffff0a
f0ff21ffdcff
$ md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
a7b9dbf76e99a33db99e557b6676588a  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
$ dd if=/dev/urandom of=./qspi_test bs=1M count=1
1+0 records in
1+0 records out
$ mtd_debug write /dev/mtd0 0 1048576 qspi_test
Copied 1048576 bytes from qspi_test to address 0x00000000 in flash
$ mtd_debug erase /dev/mtd0 0 1048576
Erased 1048576 bytes from address 0x00000000 in flash
$ mtd_debug read /dev/mtd0 0 1048576 qspi_read
Copied 1048576 bytes from address 0x00000000 in flash to qspi_read
$ hexdump qspi_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0100000
$ mtd_debug write /dev/mtd0 0 1048576 qspi_test
Copied 1048576 bytes from qspi_test to address 0x00000000 in flash
$ mtd_debug read /dev/mtd0 0 1048576 qspi_read
Copied 1048576 bytes from address 0x00000000 in flash to qspi_read
$ sha1sum qspi_test qspi_read
becf3097c0bbff0dd6f204ffe5bf575e6c43f792  qspi_test
becf3097c0bbff0dd6f204ffe5bf575e6c43f792  qspi_read
---
 drivers/mtd/spi-nor/winbond.c | 82 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 8d0a00d69e1233988876a15479d73c5fe899c542..4691e7a27ba1d70c75932c4e6b60fe36102138be 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -10,6 +10,7 @@
 
 #define WINBOND_NOR_OP_RDEAR	0xc8	/* Read Extended Address Register */
 #define WINBOND_NOR_OP_WREAR	0xc5	/* Write Extended Address Register */
+#define WINBOND_NOR_OP_SELDIE	0xc2	/* Select active die */
 
 #define WINBOND_NOR_WREAR_OP(buf)					\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(WINBOND_NOR_OP_WREAR, 0),		\
@@ -17,6 +18,12 @@
 		   SPI_MEM_OP_NO_DUMMY,					\
 		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
 
+#define WINBOND_NOR_SELDIE_OP(buf)					\
+	SPI_MEM_OP(SPI_MEM_OP_CMD(WINBOND_NOR_OP_SELDIE, 0),		\
+		   SPI_MEM_OP_NO_ADDR,					\
+		   SPI_MEM_OP_NO_DUMMY,					\
+		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
+
 static int
 w25q128_post_bfpt_fixups(struct spi_nor *nor,
 			 const struct sfdp_parameter_header *bfpt_header,
@@ -66,6 +73,26 @@ static const struct spi_nor_fixups w25q256_fixups = {
 	.post_bfpt = w25q256_post_bfpt_fixups,
 };
 
+static int
+w25q0xjv_post_bfpt_fixups(struct spi_nor *nor,
+			  const struct sfdp_parameter_header *bfpt_header,
+			  const struct sfdp_bfpt *bfpt)
+{
+	/*
+	 * SFDP supports dice numbers, but this information is only available in
+	 * optional additional tables which are not provided by these chips.
+	 * Dice number has an impact though, because these devices need extra
+	 * care when reading the busy bit.
+	 */
+	nor->params->n_dice = nor->params->size / SZ_64M;
+
+	return 0;
+}
+
+static const struct spi_nor_fixups w25q0xjv_fixups = {
+	.post_bfpt = w25q0xjv_post_bfpt_fixups,
+};
+
 static const struct flash_info winbond_nor_parts[] = {
 	{
 		.id = SNOR_ID(0xef, 0x30, 0x10),
@@ -146,6 +173,11 @@ static const struct flash_info winbond_nor_parts[] = {
 		.name = "w25q512jvq",
 		.size = SZ_64M,
 		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
+	}, {
+		.id = SNOR_ID(0xef, 0x40, 0x21),
+		.name = "w25q01jv",
+		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
+		.fixups = &w25q0xjv_fixups,
 	}, {
 		.id = SNOR_ID(0xef, 0x50, 0x12),
 		.name = "w25q20bw",
@@ -289,6 +321,37 @@ static int winbond_nor_write_ear(struct spi_nor *nor, u8 ear)
 	return ret;
 }
 
+/**
+ * winbond_nor_select_die() - Set active die.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @die:	die to set active.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int winbond_nor_select_die(struct spi_nor *nor, u8 die)
+{
+	int ret;
+
+	nor->bouncebuf[0] = die;
+
+	if (nor->spimem) {
+		struct spi_mem_op op = WINBOND_NOR_SELDIE_OP(nor->bouncebuf);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor,
+						       WINBOND_NOR_OP_SELDIE,
+						       nor->bouncebuf, 1);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d selecting die %d\n", ret, die);
+
+	return ret;
+}
+
 /**
  * winbond_nor_set_4byte_addr_mode() - Set 4-byte address mode for Winbond
  * flashes.
@@ -322,6 +385,22 @@ static int winbond_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 	return spi_nor_write_disable(nor);
 }
 
+static int winbond_multi_die_ready(struct spi_nor *nor)
+{
+	int ret, i;
+
+	for (i = 0; i < nor->params->n_dice; i++) {
+		ret = winbond_nor_select_die(nor, i);
+		if (ret)
+			return ret;
+
+		if (!spi_nor_sr_ready(nor))
+			return 0;
+	}
+
+	return 1;
+}
+
 static const struct spi_nor_otp_ops winbond_nor_otp_ops = {
 	.read = spi_nor_otp_read_secr,
 	.write = spi_nor_otp_write_secr,
@@ -334,6 +413,9 @@ static int winbond_nor_late_init(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter *params = nor->params;
 
+	if (params->n_dice > 1)
+		params->ready = winbond_multi_die_ready;
+
 	if (params->otp.org)
 		params->otp.ops = &winbond_nor_otp_ops;
 

-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: spi-nor: winbond: Add support for w25q02jv

[Miquel Raynal]

Add support for Winbond w25q02jv spi-nor chip which shares most of
w25q01jv's specificities as, this time, the chip is made of 4 different
dies.

Link: https://www.winbond.com/resource-files/W25Q02JV_DTR%20RevD%2007092024%20Plus.pdf
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Here is the basic test procedure output:

$ cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
w25q02jv
$ cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
ef7022
$ cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
winbond
$ xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450060101ff00060110800000ff84000102d00000ff03000102f000
00ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffe520fbffffffff7f44eb086b083b42bbfeffffffffff
0000ffff40eb0c200f5210d800003602a60082ea14e2e96376337a757a75
f7a2d55c19f74dffe970f9a5ffffffffffffffffffffffffffffffffff0a
f0ff21ffdcff
$ md5suum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
3e7da68579120c32a12517d2b6634c3c  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
$ dd if=/dev/urandom of=./qspi_test bs=1M count=1
1+0 records in
1+0 records out
$ mtd_debug write /dev/mtd0 0 1048576 qspi_test
Copied 1048576 bytes from qspi_test to address 0x00000000 in flash
$ mtd_debug erase /dev/mtd0 0 1048576
Erased 1048576 bytes from address 0x00000000 in flash
$ mtd_debug read /dev/mtd0 0 1048576 qspi_read
Copied 1048576 bytes from address 0x00000000 in flash to qspi_read
$ hexdump qspi_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0100000
$ mtd_debug write /dev/mtd0 0 1048576 qspi_test
Copied 1048576 bytes from qspi_test to address 0x00000000 in flash
$ mtd_debug read /dev/mtd0 0 1048576 qspi_read
Copied 1048576 bytes from address 0x00000000 in flash to qspi_read
$ sha1sum qspi_test qspi_read
c662ae4e6b1268e23d5a5e930995213d2e4044cc  qspi_test
c662ae4e6b1268e23d5a5e930995213d2e4044cc  qspi_read
---
 drivers/mtd/spi-nor/winbond.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 4691e7a27ba1d70c75932c4e6b60fe36102138be..888367a8ee9967b5d7c7886dd2c6a1024f659609 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -253,6 +253,11 @@ static const struct flash_info winbond_nor_parts[] = {
 	}, {
 		.id = SNOR_ID(0xef, 0x70, 0x19),
 		.name = "w25q256jvm",
+	}, {
+		.id = SNOR_ID(0xef, 0x70, 0x22),
+		.name = "w25q02jv",
+		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
+		.fixups = &w25q0xjv_fixups,
 	}, {
 		.id = SNOR_ID(0xef, 0x71, 0x19),
 		.name = "w25m512jv",

-- 
2.47.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Pratyush Yadav]

On Tue, Dec 24 2024, Miquel Raynal wrote:

> Add support for Winbond w25q01jv spi-nor chip.
>
> This chip is internally made of two dies with linear addressing
> capabilities to make it transparent to the user that two dies were
> used. There is one drawback however, the read status operation is racy
> as the status bit only gives the active die status and not the status of
> the other die. For commands affecting the two dies, it means if another
> command is sent too fast after the first die has returned a valid status
> (deviation can be up to 200us), the chip will get corrupted/in an
> unstable state.
>
> This chip hence requires a better status register read. There are three
> solutions here:
> - Either we wait about 200us after getting a first status ready
> feedback, to cover possible internal deviations.
> - Or we always check all internal dies (which takes about 30us per die).
>
> This second option being always faster and also being totally safe, we
> implement a specific hook for the status register read. flash_speed

Makes sense.

> benchmarks show no difference with this implementation, compared to the
> regular status read core function, the difference being part of the
> natural deviation with this benchmark:
>
> 	> Without the fixup
> 	$ flash_speed /dev/mtd0 -c100 -d
> 	eraseblock write speed is 442 KiB/s
> 	eraseblock read speed is 1606 KiB/s
> 	page write speed is 439 KiB/s
> 	page read speed is 1520 KiB/s
> 	2 page write speed is 441 KiB/s
> 	2 page read speed is 1562 KiB/s
> 	erase speed is 68 KiB/s
>
> 	> With the fixup
> 	$ flash_speed /dev/mtd0 -c100 -d
> 	eraseblock write speed is 428 KiB/s
> 	eraseblock read speed is 1626 KiB/s
> 	page write speed is 426 KiB/s
> 	page read speed is 1538 KiB/s
> 	2 page write speed is 426 KiB/s
> 	2 page read speed is 1574 KiB/s
> 	erase speed is 66 KiB/s
>
> As there are very few situations where this can actually happen, a
> status register write being the most likely one, another possibility
> might have been to use volatile writes instead of non-volatile writes,
> as most of the deviation comes from the action of writing the bit. But
> this would overlook other possible actions where both dies can be used
> at the same time like a chip erase (or any erase over the die boundary
> in general). This last approach would have the least impact but because
> it does not feel like it is totally safe to use and because the impact
> of the second solution presented above is also negligible, we keep this
> second approach for now (which can be further tuned later if it appears
> to be too impacting in the end).

I am a bit confused by this paragraph. What do you mean by "this" in the
first sentence? What do status register writes have to do with the ready
bit being racy? I would assume those would be nearly instant since
status registers are usually volatile. What do volatile writes mean in
this context?
>
> However, the fixup, whatever which one we pick, must be applied on
> multi-die chips, which hence must be properly flagged. The SFDP tables
> implemented give a lot of information but the die details are part of an
> optional table that is not implemented, hence we use a post parsing
> fixup hook to set the params->n_dice value manually.
>
> Link: https://www.winbond.com/resource-files/W25Q01JV%20SPI%20RevE%2003042024%20Plus.pdf
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>
> Here is the basic test procedure output:
>
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
> w25q01jv
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
> ef4021
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
> winbond
> $ xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 53464450060101ff00060110800000ff84000102d00000ff03000102f000
> 00ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffe520fbffffffff3f44eb086b083b42bbfeffffffffff
> 0000ffff40eb0c200f5210d800003602a60082ea14e2e96376337a757a75
> f7a2d55c19f74dffe970f9a5ffffffffffffffffffffffffffffffffff0a
> f0ff21ffdcff
> $ md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> a7b9dbf76e99a33db99e557b6676588a  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> $ dd if=/dev/urandom of=./qspi_test bs=1M count=1
> 1+0 records in
> 1+0 records out
> $ mtd_debug write /dev/mtd0 0 1048576 qspi_test
> Copied 1048576 bytes from qspi_test to address 0x00000000 in flash
> $ mtd_debug erase /dev/mtd0 0 1048576
> Erased 1048576 bytes from address 0x00000000 in flash
> $ mtd_debug read /dev/mtd0 0 1048576 qspi_read
> Copied 1048576 bytes from address 0x00000000 in flash to qspi_read
> $ hexdump qspi_read
> 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
> *
> 0100000
> $ mtd_debug write /dev/mtd0 0 1048576 qspi_test
> Copied 1048576 bytes from qspi_test to address 0x00000000 in flash
> $ mtd_debug read /dev/mtd0 0 1048576 qspi_read
> Copied 1048576 bytes from address 0x00000000 in flash to qspi_read
> $ sha1sum qspi_test qspi_read
> becf3097c0bbff0dd6f204ffe5bf575e6c43f792  qspi_test
> becf3097c0bbff0dd6f204ffe5bf575e6c43f792  qspi_read
> ---
>  drivers/mtd/spi-nor/winbond.c | 82 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 8d0a00d69e1233988876a15479d73c5fe899c542..4691e7a27ba1d70c75932c4e6b60fe36102138be 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -10,6 +10,7 @@
>  
>  #define WINBOND_NOR_OP_RDEAR	0xc8	/* Read Extended Address Register */
>  #define WINBOND_NOR_OP_WREAR	0xc5	/* Write Extended Address Register */
> +#define WINBOND_NOR_OP_SELDIE	0xc2	/* Select active die */
>  
>  #define WINBOND_NOR_WREAR_OP(buf)					\
>  	SPI_MEM_OP(SPI_MEM_OP_CMD(WINBOND_NOR_OP_WREAR, 0),		\
> @@ -17,6 +18,12 @@
>  		   SPI_MEM_OP_NO_DUMMY,					\
>  		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
>  
> +#define WINBOND_NOR_SELDIE_OP(buf)					\
> +	SPI_MEM_OP(SPI_MEM_OP_CMD(WINBOND_NOR_OP_SELDIE, 0),		\
> +		   SPI_MEM_OP_NO_ADDR,					\
> +		   SPI_MEM_OP_NO_DUMMY,					\
> +		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
> +
>  static int
>  w25q128_post_bfpt_fixups(struct spi_nor *nor,
>  			 const struct sfdp_parameter_header *bfpt_header,
> @@ -66,6 +73,26 @@ static const struct spi_nor_fixups w25q256_fixups = {
>  	.post_bfpt = w25q256_post_bfpt_fixups,
>  };
>  
> +static int
> +w25q0xjv_post_bfpt_fixups(struct spi_nor *nor,
> +			  const struct sfdp_parameter_header *bfpt_header,
> +			  const struct sfdp_bfpt *bfpt)
> +{
> +	/*
> +	 * SFDP supports dice numbers, but this information is only available in
> +	 * optional additional tables which are not provided by these chips.
> +	 * Dice number has an impact though, because these devices need extra
> +	 * care when reading the busy bit.
> +	 */
> +	nor->params->n_dice = nor->params->size / SZ_64M;

n_dice is set by spi_nor_parse_sccr_mc(), which runs _after_ post-BFPT
fixups. This doesn't matter in practice since you say that the chip
doesn't have a SCCR_MC table but I think it still is a good idea to
follow the initialization order and do this in the post-SFDP hook.

> +
> +	return 0;
> +}
> +
> +static const struct spi_nor_fixups w25q0xjv_fixups = {
> +	.post_bfpt = w25q0xjv_post_bfpt_fixups,
> +};
> +
>  static const struct flash_info winbond_nor_parts[] = {
>  	{
>  		.id = SNOR_ID(0xef, 0x30, 0x10),
> @@ -146,6 +173,11 @@ static const struct flash_info winbond_nor_parts[] = {
>  		.name = "w25q512jvq",
>  		.size = SZ_64M,
>  		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
> +	}, {
> +		.id = SNOR_ID(0xef, 0x40, 0x21),
> +		.name = "w25q01jv",

We no longer set the name for new flash entries. But knowing the flash
name for an entry is still useful, so make this a comment on top of the
entry.

> +		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,

Since the flash has an SFDP table, you probably don't need these flags.
Can you try removing this line and see if things still work fine?

> +		.fixups = &w25q0xjv_fixups,
>  	}, {
>  		.id = SNOR_ID(0xef, 0x50, 0x12),
>  		.name = "w25q20bw",
> @@ -289,6 +321,37 @@ static int winbond_nor_write_ear(struct spi_nor *nor, u8 ear)
>  	return ret;
>  }
>  
> +/**
> + * winbond_nor_select_die() - Set active die.
> + * @nor:	pointer to 'struct spi_nor'.
> + * @die:	die to set active.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int winbond_nor_select_die(struct spi_nor *nor, u8 die)
> +{
> +	int ret;
> +
> +	nor->bouncebuf[0] = die;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op = WINBOND_NOR_SELDIE_OP(nor->bouncebuf);
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_write_reg(nor,
> +						       WINBOND_NOR_OP_SELDIE,
> +						       nor->bouncebuf, 1);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d selecting die %d\n", ret, die);
> +
> +	return ret;
> +}
> +
>  /**
>   * winbond_nor_set_4byte_addr_mode() - Set 4-byte address mode for Winbond
>   * flashes.
> @@ -322,6 +385,22 @@ static int winbond_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  	return spi_nor_write_disable(nor);
>  }
>  

Adding a short comment about why this is needed would be nice, and
readers won't always have to do a git blame to find out.

> +static int winbond_multi_die_ready(struct spi_nor *nor)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < nor->params->n_dice; i++) {
> +		ret = winbond_nor_select_die(nor, i);
> +		if (ret)
> +			return ret;
> +
> +		if (!spi_nor_sr_ready(nor))

spi_nor_sr_ready() can also return -errno, which would be treated here
as being ready, which obviously isn't right. This should also check for
a return value < 0.

> +			return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  static const struct spi_nor_otp_ops winbond_nor_otp_ops = {
>  	.read = spi_nor_otp_read_secr,
>  	.write = spi_nor_otp_write_secr,
> @@ -334,6 +413,9 @@ static int winbond_nor_late_init(struct spi_nor *nor)
>  {
>  	struct spi_nor_flash_parameter *params = nor->params;
>  
> +	if (params->n_dice > 1)
> +		params->ready = winbond_multi_die_ready;
> +

Is this true for all multi-die Winbond flashes, and going to hold true
for future ones? If not, I suppose this should go in the flash-specific
fixup hook. Do it in either the flash-specific late_init hook, or in the
post_sfdp hook, I have no strong opinions.

>  	if (params->otp.org)
>  		params->otp.ops = &winbond_nor_otp_ops;

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: spi-nor: winbond: Add support for w25q02jv

[Pratyush Yadav]

On Tue, Dec 24 2024, Miquel Raynal wrote:

> Add support for Winbond w25q02jv spi-nor chip which shares most of
> w25q01jv's specificities as, this time, the chip is made of 4 different
> dies.
>
> Link: https://www.winbond.com/resource-files/W25Q02JV_DTR%20RevD%2007092024%20Plus.pdf
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
[...]
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 4691e7a27ba1d70c75932c4e6b60fe36102138be..888367a8ee9967b5d7c7886dd2c6a1024f659609 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -253,6 +253,11 @@ static const struct flash_info winbond_nor_parts[] = {
>  	}, {
>  		.id = SNOR_ID(0xef, 0x70, 0x19),
>  		.name = "w25q256jvm",
> +	}, {
> +		.id = SNOR_ID(0xef, 0x70, 0x22),
> +		.name = "w25q02jv",
> +		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,

Same comments as previous patch about name and no_sfdp_flags.

> +		.fixups = &w25q0xjv_fixups,
>  	}, {
>  		.id = SNOR_ID(0xef, 0x71, 0x19),
>  		.name = "w25m512jv",

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Miquel Raynal]

Hello Pratyush,

On 24/12/2024 at 21:15:41 GMT, Pratyush Yadav <pratyush@kernel.org> wrote:

> On Tue, Dec 24 2024, Miquel Raynal wrote:
>
>> Add support for Winbond w25q01jv spi-nor chip.
>>
>> This chip is internally made of two dies with linear addressing
>> capabilities to make it transparent to the user that two dies were
>> used. There is one drawback however, the read status operation is racy
>> as the status bit only gives the active die status and not the status of
>> the other die. For commands affecting the two dies, it means if another
>> command is sent too fast after the first die has returned a valid status
>> (deviation can be up to 200us), the chip will get corrupted/in an
>> unstable state.
>>
>> This chip hence requires a better status register read. There are three
>> solutions here:
>> - Either we wait about 200us after getting a first status ready
>> feedback, to cover possible internal deviations.
>> - Or we always check all internal dies (which takes about 30us per die).
>>
>> This second option being always faster and also being totally safe, we
>> implement a specific hook for the status register read. flash_speed
>
> Makes sense.
>
>> benchmarks show no difference with this implementation, compared to the
>> regular status read core function, the difference being part of the
>> natural deviation with this benchmark:
>>
>> 	> Without the fixup
>> 	$ flash_speed /dev/mtd0 -c100 -d
>> 	eraseblock write speed is 442 KiB/s
>> 	eraseblock read speed is 1606 KiB/s
>> 	page write speed is 439 KiB/s
>> 	page read speed is 1520 KiB/s
>> 	2 page write speed is 441 KiB/s
>> 	2 page read speed is 1562 KiB/s
>> 	erase speed is 68 KiB/s
>>
>> 	> With the fixup
>> 	$ flash_speed /dev/mtd0 -c100 -d
>> 	eraseblock write speed is 428 KiB/s
>> 	eraseblock read speed is 1626 KiB/s
>> 	page write speed is 426 KiB/s
>> 	page read speed is 1538 KiB/s
>> 	2 page write speed is 426 KiB/s
>> 	2 page read speed is 1574 KiB/s
>> 	erase speed is 66 KiB/s
>>
>> As there are very few situations where this can actually happen, a
>> status register write being the most likely one, another possibility
>> might have been to use volatile writes instead of non-volatile writes,
>> as most of the deviation comes from the action of writing the bit. But
>> this would overlook other possible actions where both dies can be used
>> at the same time like a chip erase (or any erase over the die boundary
>> in general). This last approach would have the least impact but because
>> it does not feel like it is totally safe to use and because the impact
>> of the second solution presented above is also negligible, we keep this
>> second approach for now (which can be further tuned later if it appears
>> to be too impacting in the end).
>
> I am a bit confused by this paragraph. What do you mean by "this" in
> the

"this" = "the race condition"

> first sentence? What do status register writes have to do with the ready
> bit being racy?

The bug that has been experienced followed this sequence:
- send the write enable command (non-volatile)
- wait for the ready/busy bit, ie. wait for the WEL bit to be set
  because it is non-volatile write
- active die is ready, (but idle die is not!)
- enter 4-byte address mode, only the die that is ready processes the
  command.

We only observed the issue in this particular case which involves
writing the status register, because it is one of the very few commands
targeting all dies at the same time.

I assume another sequence that might lead to a similar issue might be a
chip erasure, as all dies are involved in parallel, but maybe there are
other situations I did not think about which might be racy as well.

> I would assume those would be nearly instant since
> status registers are usually volatile. What do volatile writes mean in
> this context?

You are actually right. Status register bits can be volatile (in this
case writing the bits themselves is almost instant) but currently when
we allow this register to be writable by sending the write enable (06h)
command, the non-volatile way is used, ie. the state of the bit itself
is stored in non-volatile memory and write durations can vary from one
die to another.

Winbond chips (maybe this is a shared capability?) accepts another
command, "Write Enable for Volatile Status Register (50h)", which
specifically change the status register bits to use the volatile method.

Hence, if the only situation we want to solve is the status register
access, then we may just enable this command (this is the third solution
I tried to explain in the commit log), but if we think there are other
racy situations, this approach is not complete and we must fallback to
one of the approaches listed above.

>>
>> However, the fixup, whatever which one we pick, must be applied on
>> multi-die chips, which hence must be properly flagged. The SFDP tables
>> implemented give a lot of information but the die details are part of an
>> optional table that is not implemented, hence we use a post parsing
>> fixup hook to set the params->n_dice value manually.
>>
>> Link: https://www.winbond.com/resource-files/W25Q01JV%20SPI%20RevE%2003042024%20Plus.pdf
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>
>> Here is the basic test procedure output:
>>
>> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
>> w25q01jv
>> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
>> ef4021
>> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
>> winbond
>> $ xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
>> 53464450060101ff00060110800000ff84000102d00000ff03000102f000
>> 00ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffe520fbffffffff3f44eb086b083b42bbfeffffffffff
>> 0000ffff40eb0c200f5210d800003602a60082ea14e2e96376337a757a75
>> f7a2d55c19f74dffe970f9a5ffffffffffffffffffffffffffffffffff0a
>> f0ff21ffdcff
>> $ md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
>> a7b9dbf76e99a33db99e557b6676588a  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
>> $ dd if=/dev/urandom of=./qspi_test bs=1M count=1
>> 1+0 records in
>> 1+0 records out
>> $ mtd_debug write /dev/mtd0 0 1048576 qspi_test
>> Copied 1048576 bytes from qspi_test to address 0x00000000 in flash
>> $ mtd_debug erase /dev/mtd0 0 1048576
>> Erased 1048576 bytes from address 0x00000000 in flash
>> $ mtd_debug read /dev/mtd0 0 1048576 qspi_read
>> Copied 1048576 bytes from address 0x00000000 in flash to qspi_read
>> $ hexdump qspi_read
>> 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
>> *
>> 0100000
>> $ mtd_debug write /dev/mtd0 0 1048576 qspi_test
>> Copied 1048576 bytes from qspi_test to address 0x00000000 in flash
>> $ mtd_debug read /dev/mtd0 0 1048576 qspi_read
>> Copied 1048576 bytes from address 0x00000000 in flash to qspi_read
>> $ sha1sum qspi_test qspi_read
>> becf3097c0bbff0dd6f204ffe5bf575e6c43f792  qspi_test
>> becf3097c0bbff0dd6f204ffe5bf575e6c43f792  qspi_read
>> ---
>>  drivers/mtd/spi-nor/winbond.c | 82 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 82 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
>> index 8d0a00d69e1233988876a15479d73c5fe899c542..4691e7a27ba1d70c75932c4e6b60fe36102138be 100644
>> --- a/drivers/mtd/spi-nor/winbond.c
>> +++ b/drivers/mtd/spi-nor/winbond.c
>> @@ -10,6 +10,7 @@
>>  
>>  #define WINBOND_NOR_OP_RDEAR	0xc8	/* Read Extended Address Register */
>>  #define WINBOND_NOR_OP_WREAR	0xc5	/* Write Extended Address Register */
>> +#define WINBOND_NOR_OP_SELDIE	0xc2	/* Select active die */
>>  
>>  #define WINBOND_NOR_WREAR_OP(buf)					\
>>  	SPI_MEM_OP(SPI_MEM_OP_CMD(WINBOND_NOR_OP_WREAR, 0),		\
>> @@ -17,6 +18,12 @@
>>  		   SPI_MEM_OP_NO_DUMMY,					\
>>  		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
>>  
>> +#define WINBOND_NOR_SELDIE_OP(buf)					\
>> +	SPI_MEM_OP(SPI_MEM_OP_CMD(WINBOND_NOR_OP_SELDIE, 0),		\
>> +		   SPI_MEM_OP_NO_ADDR,					\
>> +		   SPI_MEM_OP_NO_DUMMY,					\
>> +		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
>> +
>>  static int
>>  w25q128_post_bfpt_fixups(struct spi_nor *nor,
>>  			 const struct sfdp_parameter_header *bfpt_header,
>> @@ -66,6 +73,26 @@ static const struct spi_nor_fixups w25q256_fixups = {
>>  	.post_bfpt = w25q256_post_bfpt_fixups,
>>  };
>>  
>> +static int
>> +w25q0xjv_post_bfpt_fixups(struct spi_nor *nor,
>> +			  const struct sfdp_parameter_header *bfpt_header,
>> +			  const struct sfdp_bfpt *bfpt)
>> +{
>> +	/*
>> +	 * SFDP supports dice numbers, but this information is only available in
>> +	 * optional additional tables which are not provided by these chips.
>> +	 * Dice number has an impact though, because these devices need extra
>> +	 * care when reading the busy bit.
>> +	 */
>> +	nor->params->n_dice = nor->params->size / SZ_64M;
>
> n_dice is set by spi_nor_parse_sccr_mc(), which runs _after_ post-BFPT
> fixups. This doesn't matter in practice since you say that the chip
> doesn't have a SCCR_MC table but I think it still is a good idea to
> follow the initialization order and do this in the post-SFDP hook.

I must have been mislead by the names, indeed, I'll check.

>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_nor_fixups w25q0xjv_fixups = {
>> +	.post_bfpt = w25q0xjv_post_bfpt_fixups,
>> +};
>> +
>>  static const struct flash_info winbond_nor_parts[] = {
>>  	{
>>  		.id = SNOR_ID(0xef, 0x30, 0x10),
>> @@ -146,6 +173,11 @@ static const struct flash_info winbond_nor_parts[] = {
>>  		.name = "w25q512jvq",
>>  		.size = SZ_64M,
>>  		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>> +	}, {
>> +		.id = SNOR_ID(0xef, 0x40, 0x21),
>> +		.name = "w25q01jv",
>
> We no longer set the name for new flash entries. But knowing the flash
> name for an entry is still useful, so make this a comment on top of the
> entry.

Actually without the flash entry the fixups cannot apply, so I'd expect
any flash with fixups (like this one) to be listed at least? Said
otherwise, what comment would you expect here?

>
>> +		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>
> Since the flash has an SFDP table, you probably don't need these flags.
> Can you try removing this line and see if things still work fine?

Gasp. Second time I forget to remove these. Yes, this is a copy-paste
left-over which is not needed. I'll double check it is actually not
needed though.

>
>> +		.fixups = &w25q0xjv_fixups,
>>  	}, {
>>  		.id = SNOR_ID(0xef, 0x50, 0x12),
>>  		.name = "w25q20bw",
>> @@ -289,6 +321,37 @@ static int winbond_nor_write_ear(struct spi_nor *nor, u8 ear)
>>  	return ret;
>>  }
>>  
>> +/**
>> + * winbond_nor_select_die() - Set active die.
>> + * @nor:	pointer to 'struct spi_nor'.
>> + * @die:	die to set active.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int winbond_nor_select_die(struct spi_nor *nor, u8 die)
>> +{
>> +	int ret;
>> +
>> +	nor->bouncebuf[0] = die;
>> +
>> +	if (nor->spimem) {
>> +		struct spi_mem_op op = WINBOND_NOR_SELDIE_OP(nor->bouncebuf);
>> +
>> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
>> +
>> +		ret = spi_mem_exec_op(nor->spimem, &op);
>> +	} else {
>> +		ret = spi_nor_controller_ops_write_reg(nor,
>> +						       WINBOND_NOR_OP_SELDIE,
>> +						       nor->bouncebuf, 1);
>> +	}
>> +
>> +	if (ret)
>> +		dev_dbg(nor->dev, "error %d selecting die %d\n", ret, die);
>> +
>> +	return ret;
>> +}
>> +
>>  /**
>>   * winbond_nor_set_4byte_addr_mode() - Set 4-byte address mode for Winbond
>>   * flashes.
>> @@ -322,6 +385,22 @@ static int winbond_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>>  	return spi_nor_write_disable(nor);
>>  }
>>  
>
> Adding a short comment about why this is needed would be nice, and
> readers won't always have to do a git blame to find out.

Sure.

>
>> +static int winbond_multi_die_ready(struct spi_nor *nor)
>> +{
>> +	int ret, i;
>> +
>> +	for (i = 0; i < nor->params->n_dice; i++) {
>> +		ret = winbond_nor_select_die(nor, i);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (!spi_nor_sr_ready(nor))
>
> spi_nor_sr_ready() can also return -errno, which would be treated here
> as being ready, which obviously isn't right. This should also check for
> a return value < 0.

That's true.

>
>> +			return 0;
>> +	}
>> +
>> +	return 1;
>> +}
>> +
>>  static const struct spi_nor_otp_ops winbond_nor_otp_ops = {
>>  	.read = spi_nor_otp_read_secr,
>>  	.write = spi_nor_otp_write_secr,
>> @@ -334,6 +413,9 @@ static int winbond_nor_late_init(struct spi_nor *nor)
>>  {
>>  	struct spi_nor_flash_parameter *params = nor->params;
>>  
>> +	if (params->n_dice > 1)
>> +		params->ready = winbond_multi_die_ready;
>> +
>
> Is this true for all multi-die Winbond flashes, and going to hold true
> for future ones? If not, I suppose this should go in the flash-specific
> fixup hook. Do it in either the flash-specific late_init hook, or in the
> post_sfdp hook, I have no strong opinions.

This is an information hard to gather, I assumed it was always like that
with Winbond flashes, I'll try to find more about it.

Thanks for the review!
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Miquel Raynal]

Hello Pratyush,

>>  static const struct spi_nor_otp_ops winbond_nor_otp_ops = {
>>  	.read = spi_nor_otp_read_secr,
>>  	.write = spi_nor_otp_write_secr,
>> @@ -334,6 +413,9 @@ static int winbond_nor_late_init(struct spi_nor *nor)
>>  {
>>  	struct spi_nor_flash_parameter *params = nor->params;
>>  
>> +	if (params->n_dice > 1)
>> +		params->ready = winbond_multi_die_ready;
>> +
>
> Is this true for all multi-die Winbond flashes, and going to hold true
> for future ones? If not, I suppose this should go in the flash-specific
> fixup hook. Do it in either the flash-specific late_init hook, or in the
> post_sfdp hook, I have no strong opinions.

So, after talking to Winbond, it appears that we can reduce the scope of
this fixup to the following parts which are impacted:
- W25Q0{1,2}JV
- W25H0{1,2}JV
- W25Q0{1,2}NW
- W35T0{1,2}NW
Future chips being fixed in hardware.

I'll probably use the post_sfdp flash-specific hook as you suggest.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Miquel Raynal]

Hello Pratyush,

>> +	}, {
>> +		.id = SNOR_ID(0xef, 0x40, 0x21),
>> +		.name = "w25q01jv",
>
> We no longer set the name for new flash entries. But knowing the flash
> name for an entry is still useful, so make this a comment on top of the
> entry.

I just connected the dots and understood your request. I will respin a
series will all your comments addressed. Thanks!

Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Pratyush Yadav]

On Thu, Jan 09 2025, Miquel Raynal wrote:

> Hello Pratyush,
>
>>> +	}, {
>>> +		.id = SNOR_ID(0xef, 0x40, 0x21),
>>> +		.name = "w25q01jv",
>>
>> We no longer set the name for new flash entries. But knowing the flash
>> name for an entry is still useful, so make this a comment on top of the
>> entry.
>
> I just connected the dots and understood your request. I will respin a
> series will all your comments addressed. Thanks!

Yeah sorry, I was on vacation and didn't get around to opening my
computer too often. Your v3 gets my suggestion right, thanks!

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Pratyush Yadav]

On Mon, Dec 30 2024, Miquel Raynal wrote:

> Hello Pratyush,
>
> On 24/12/2024 at 21:15:41 GMT, Pratyush Yadav <pratyush@kernel.org> wrote:
>
>> On Tue, Dec 24 2024, Miquel Raynal wrote:
[...]
>>> As there are very few situations where this can actually happen, a
>>> status register write being the most likely one, another possibility
>>> might have been to use volatile writes instead of non-volatile writes,
>>> as most of the deviation comes from the action of writing the bit. But
>>> this would overlook other possible actions where both dies can be used
>>> at the same time like a chip erase (or any erase over the die boundary
>>> in general). This last approach would have the least impact but because
>>> it does not feel like it is totally safe to use and because the impact
>>> of the second solution presented above is also negligible, we keep this
>>> second approach for now (which can be further tuned later if it appears
>>> to be too impacting in the end).
>>
>> I am a bit confused by this paragraph. What do you mean by "this" in
>> the
>
> "this" = "the race condition"
>
>> first sentence? What do status register writes have to do with the ready
>> bit being racy?
>
> The bug that has been experienced followed this sequence:
> - send the write enable command (non-volatile)
> - wait for the ready/busy bit, ie. wait for the WEL bit to be set
>   because it is non-volatile write
> - active die is ready, (but idle die is not!)
> - enter 4-byte address mode, only the die that is ready processes the
>   command.
>
> We only observed the issue in this particular case which involves
> writing the status register, because it is one of the very few commands
> targeting all dies at the same time.
>
> I assume another sequence that might lead to a similar issue might be a
> chip erasure, as all dies are involved in parallel, but maybe there are
> other situations I did not think about which might be racy as well.


>
>> I would assume those would be nearly instant since
>> status registers are usually volatile. What do volatile writes mean in
>> this context?
>
> You are actually right. Status register bits can be volatile (in this
> case writing the bits themselves is almost instant) but currently when
> we allow this register to be writable by sending the write enable (06h)
> command, the non-volatile way is used, ie. the state of the bit itself
> is stored in non-volatile memory and write durations can vary from one
> die to another.

Okay, that is strange behaviour. Normally the status registers are
always volatile, and don't have a non-volatile counterpart.

>
> Winbond chips (maybe this is a shared capability?) accepts another
> command, "Write Enable for Volatile Status Register (50h)", which
> specifically change the status register bits to use the volatile method.
>
> Hence, if the only situation we want to solve is the status register
> access, then we may just enable this command (this is the third solution
> I tried to explain in the commit log), but if we think there are other
> racy situations, this approach is not complete and we must fallback to
> one of the approaches listed above.

I am not quite sure how you fix the write-enable-being-racy bug with
your patch. If you look at the code, spi_nor_write_enable() only calls
the write enable command (06h), and does not call
spi_nor_wait_till_ready() after that. After the write enable, it
immediately executes the program or erase operation. So you never
actually wait for all dies to be ready after a write enable.

You can see an example in spi_nor_write(). It does:

    spi_nor_write_enable() -> spi_nor_write_data() -> spi_nor_wait_till_ready()

Do you have a consistent reproducer for the race? If so, does the patch
actually somehow make the race go away? If so, I would be curious to
know why.

>
>>>
>>> However, the fixup, whatever which one we pick, must be applied on
>>> multi-die chips, which hence must be properly flagged. The SFDP tables
>>> implemented give a lot of information but the die details are part of an
>>> optional table that is not implemented, hence we use a post parsing
>>> fixup hook to set the params->n_dice value manually.
>>>
[...]

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Miquel Raynal]

Hello Pratyush,

>> Winbond chips (maybe this is a shared capability?) accepts another
>> command, "Write Enable for Volatile Status Register (50h)", which
>> specifically change the status register bits to use the volatile method.
>>
>> Hence, if the only situation we want to solve is the status register
>> access, then we may just enable this command (this is the third solution
>> I tried to explain in the commit log), but if we think there are other
>> racy situations, this approach is not complete and we must fallback to
>> one of the approaches listed above.
>
> I am not quite sure how you fix the write-enable-being-racy bug with
> your patch. If you look at the code, spi_nor_write_enable() only calls
> the write enable command (06h), and does not call
> spi_nor_wait_till_ready() after that. After the write enable, it
> immediately executes the program or erase operation. So you never
> actually wait for all dies to be ready after a write enable.

I will double check but my understanding is that the *status register*
write is racy, not the spi_nor_write_enable().

> You can see an example in spi_nor_write(). It does:
>
>     spi_nor_write_enable() -> spi_nor_write_data() ->
>     spi_nor_wait_till_ready()

What is racy is: act on all dies then check the status of a single die.

> Do you have a consistent reproducer for the race? If so, does the patch
> actually somehow make the race go away? If so, I would be curious to
> know why.

Not with Linux, it is a problem that has been (consistently) observed
using an rtos. It's been analysed so we know what the issue is and we
want to make sure this cannot happen using Linux.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Pratyush Yadav]

On Tue, Jan 14 2025, Miquel Raynal wrote:

> Hello Pratyush,
>
>>> Winbond chips (maybe this is a shared capability?) accepts another
>>> command, "Write Enable for Volatile Status Register (50h)", which
>>> specifically change the status register bits to use the volatile method.
>>>
>>> Hence, if the only situation we want to solve is the status register
>>> access, then we may just enable this command (this is the third solution
>>> I tried to explain in the commit log), but if we think there are other
>>> racy situations, this approach is not complete and we must fallback to
>>> one of the approaches listed above.
>>
>> I am not quite sure how you fix the write-enable-being-racy bug with
>> your patch. If you look at the code, spi_nor_write_enable() only calls
>> the write enable command (06h), and does not call
>> spi_nor_wait_till_ready() after that. After the write enable, it
>> immediately executes the program or erase operation. So you never
>> actually wait for all dies to be ready after a write enable.
>
> I will double check but my understanding is that the *status register*
> write is racy, not the spi_nor_write_enable().

Okay, I am confused because you said earlier that:

> The bug that has been experienced followed this sequence:
> - send the write enable command (non-volatile)
> - wait for the ready/busy bit, ie. wait for the WEL bit to be set
>   because it is non-volatile write
> - active die is ready, (but idle die is not!)
> - enter 4-byte address mode, only the die that is ready processes the
>   command.

Which says the WEL bit being set itself is racy. What I understand from
that is one die is ready to take writes and the other is not. Now when
you try to write the SR to enable 4B mode, it would only work on the die
that got the WEL set. The other one ignores it and stays in 3B mode. Do
I understand this correctly? To fix this you need to wait after the
write enable, before you initiate the write SR operation.

>
>> You can see an example in spi_nor_write(). It does:
>>
>>     spi_nor_write_enable() -> spi_nor_write_data() ->
>>     spi_nor_wait_till_ready()
>
> What is racy is: act on all dies then check the status of a single die.

Your patch fixes all such operations, except write enable IIUC. For
operations such as write SR (or any other register) or chip erase, we
would call spi_nor_wait_till_ready(), and your patch would make sure all
dies are ready.

But when write enable itself is racy, then we would need to add a wait
after the write enable, which your patch does not do. I am a bit
confused right now whether that is an actual problem or I just misread
your message. If write enable itself isn't racy, then the v3 series
should be good to go.

>
>> Do you have a consistent reproducer for the race? If so, does the patch
>> actually somehow make the race go away? If so, I would be curious to
>> know why.
>
> Not with Linux, it is a problem that has been (consistently) observed
> using an rtos. It's been analysed so we know what the issue is and we
> want to make sure this cannot happen using Linux.
>
> Thanks,
> Miquèl

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Miquel Raynal]

Hello Pratyush,

> Okay, I am confused because you said earlier that:
>
>> The bug that has been experienced followed this sequence:
>> - send the write enable command (non-volatile)
>> - wait for the ready/busy bit, ie. wait for the WEL bit to be set
>>   because it is non-volatile write
>> - active die is ready, (but idle die is not!)
>> - enter 4-byte address mode, only the die that is ready processes the
>>   command.
>
> Which says the WEL bit being set itself is racy. What I understand from
> that is one die is ready to take writes and the other is not. Now when
> you try to write the SR to enable 4B mode, it would only work on the die
> that got the WEL set. The other one ignores it and stays in 3B mode. Do
> I understand this correctly? To fix this you need to wait after the
> write enable, before you initiate the write SR operation.

Actually I think you're right. The thing is, Winbond WEL bit are
non-volatile by default, whereas you were assuming it would be. Maybe
the proper fix is to do both?
- Using the volatile 'write enable'
and
- Making sure we wait after the (other) commands tampering with all dies.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Pratyush Yadav]

On Wed, Jan 15 2025, Miquel Raynal wrote:

> Hello Pratyush,
>
>> Okay, I am confused because you said earlier that:
>>
>>> The bug that has been experienced followed this sequence:
>>> - send the write enable command (non-volatile)
>>> - wait for the ready/busy bit, ie. wait for the WEL bit to be set
>>>   because it is non-volatile write
>>> - active die is ready, (but idle die is not!)
>>> - enter 4-byte address mode, only the die that is ready processes the
>>>   command.
>>
>> Which says the WEL bit being set itself is racy. What I understand from
>> that is one die is ready to take writes and the other is not. Now when
>> you try to write the SR to enable 4B mode, it would only work on the die
>> that got the WEL set. The other one ignores it and stays in 3B mode. Do
>> I understand this correctly? To fix this you need to wait after the
>> write enable, before you initiate the write SR operation.
>
> Actually I think you're right. The thing is, Winbond WEL bit are
> non-volatile by default, whereas you were assuming it would be. Maybe
> the proper fix is to do both?
> - Using the volatile 'write enable'
> and
> - Making sure we wait after the (other) commands tampering with all dies.

Yeah. You can do that or you can do the wait after sending the write
enable. Either case would need you to implement a custom write enable
function for the chip so from the code perspective, it should be about
the same. Though I do think using the volatile write enable seems to be
faster and easier to reason about.

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Miquel Raynal]

Hi Pratyush,

> Okay, I am confused because you said earlier that:
>
>> The bug that has been experienced followed this sequence:
>> - send the write enable command (non-volatile)
>> - wait for the ready/busy bit, ie. wait for the WEL bit to be set
>>   because it is non-volatile write
>> - active die is ready, (but idle die is not!)
>> - enter 4-byte address mode, only the die that is ready processes the
>>   command.
>
> Which says the WEL bit being set itself is racy. What I understand from
> that is one die is ready to take writes and the other is not. Now when
> you try to write the SR to enable 4B mode, it would only work on the die
> that got the WEL set. The other one ignores it and stays in 3B mode. Do
> I understand this correctly? To fix this you need to wait after the
> write enable, before you initiate the write SR operation.

Sorry for the confusion, I got myself confused as well. I double checked
with Winbond and I think I have the correct explanation now:

The WEL bit is volatile, there is no delay when setting it (well, about
10ns, but no specific deviation).

On most chips, WEL enables all write operations:
  * (single/dual/quad) page programs
  * (sector/block/chip) erases
  * status register write
  * erase/program of other internal registers (like security registers)

On Winbond, the above applies, but with the usual "Write Enable command
(06h)", the status register bits are non-volatile, ie. they are stored
in non-volatile cells (which takes time to program and are subject
to deviations across dies). Hence, they added another command, called
"Write Enable for Volatile Status Register (50h)" which is an addition
to the usual "Write Enable command (06h)" which causes:
- enabling writes on the status register only (if the WEL bit is not yet
  set)
- using volatile writes for the status register bits (ie. they are using
  some kind of local cache which update almost immediately).

So basically, if you do the following:
- status register write
- check the status bit with the standard helper
- (and quickly after) do anything else on the idle die
In this case you could experience a race, but that is not related to the
Write Enable command.

In general I believe enabling volatile status register writes would not
be useful as long as we have the "read the status from all dies"
workaround.

Let me know if something is still unclear.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: winbond: Add support for w25q01jv

[Pratyush Yadav]

On Mon, Jan 20 2025, Miquel Raynal wrote:

> Hi Pratyush,
>
>> Okay, I am confused because you said earlier that:
>>
>>> The bug that has been experienced followed this sequence:
>>> - send the write enable command (non-volatile)
>>> - wait for the ready/busy bit, ie. wait for the WEL bit to be set
>>>   because it is non-volatile write
>>> - active die is ready, (but idle die is not!)
>>> - enter 4-byte address mode, only the die that is ready processes the
>>>   command.
>>
>> Which says the WEL bit being set itself is racy. What I understand from
>> that is one die is ready to take writes and the other is not. Now when
>> you try to write the SR to enable 4B mode, it would only work on the die
>> that got the WEL set. The other one ignores it and stays in 3B mode. Do
>> I understand this correctly? To fix this you need to wait after the
>> write enable, before you initiate the write SR operation.
>
> Sorry for the confusion, I got myself confused as well. I double checked
> with Winbond and I think I have the correct explanation now:
>
> The WEL bit is volatile, there is no delay when setting it (well, about
> 10ns, but no specific deviation).
>
> On most chips, WEL enables all write operations:
>   * (single/dual/quad) page programs
>   * (sector/block/chip) erases
>   * status register write
>   * erase/program of other internal registers (like security registers)
>
> On Winbond, the above applies, but with the usual "Write Enable command
> (06h)", the status register bits are non-volatile, ie. they are stored
> in non-volatile cells (which takes time to program and are subject
> to deviations across dies). Hence, they added another command, called
> "Write Enable for Volatile Status Register (50h)" which is an addition
> to the usual "Write Enable command (06h)" which causes:
> - enabling writes on the status register only (if the WEL bit is not yet
>   set)
> - using volatile writes for the status register bits (ie. they are using
>   some kind of local cache which update almost immediately).
>
> So basically, if you do the following:
> - status register write
> - check the status bit with the standard helper
> - (and quickly after) do anything else on the idle die
> In this case you could experience a race, but that is not related to the
> Write Enable command.
>
> In general I believe enabling volatile status register writes would not
> be useful as long as we have the "read the status from all dies"
> workaround.
>
> Let me know if something is still unclear.

Ah, okay. That all makes sense now. With this explanation, your patches
should be doing the right thing. I will do a round of review on the v3
soon, but they looked mostly good when I last took a brief look.

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/