[PATCH] mtd: spi-nor: winbond: Make sure w25q{01, 02}jv behave correctly

[PATCH] mtd: spi-nor: winbond: Make sure w25q{01, 02}jv behave correctly

[Miquel Raynal]

These chips are internally made of two/four dies with linear addressing
capabilities to make it transparent to the user that two/four 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.

The solution adopted here is to iterate manually over all internal
dies (which takes about 30us per die) until all are ready. This approach
will always be faster than a blind delay which represents the maximum
deviation, while also being totally safe.

A flash-specific hook for the status register read had to be
implemented. Testing with the flash_speed benchmark in Linux shown no
difference with the existing performances (using the regular status read
core function).

As the presence of multiple dies is not filled in these chips SFDP
tables (the table containing the crucial information is optional), we
need to manually wire the hook.

This change is adapted from Linux.

Link: https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/spi/spi-nor-core.c | 101 +++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 6f352c5c0e2d..ab0baf4eb251 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -4215,6 +4215,90 @@ static struct spi_nor_fixups macronix_octal_fixups = {
 };
 #endif /* CONFIG_SPI_FLASH_MACRONIX */
 
+#if CONFIG_IS_ENABLED(SPI_FLASH_WINBOND)
+
+#define WINBOND_NOR_OP_SELDIE  0xc2    /* Select active die */
+
+struct winbond_nor_priv {
+	unsigned int n_dice;
+};
+
+/**
+ * winbond_nor_select_die() - Set active die.
+ * @nor:       pointer to 'struct spi_nor'.
+ * @die:       die to set active.
+ *
+ * Certain Winbond chips feature more than a single die. This is mostly hidden
+ * to the user, except that some chips may experience time deviation when
+ * modifying the status bits between dies, which in some corner cases may
+ * produce problematic races. Being able to explicitly select a die to check its
+ * state in this case may be useful.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int winbond_nor_select_die(struct spi_nor *nor, u8 die)
+{
+	struct spi_mem_op op =
+		SPI_MEM_OP(SPI_MEM_OP_CMD(WINBOND_NOR_OP_SELDIE, 1),
+			   SPI_MEM_OP_NO_ADDR,
+			   SPI_MEM_OP_NO_DUMMY,
+			   SPI_MEM_OP_DATA_OUT(1, &die, 0));
+
+	int ret;
+
+	spi_nor_setup_op(nor, &op, SNOR_PROTO_1_1_1);
+	ret = spi_mem_exec_op(nor->spi, &op);
+	if (ret)
+		debug("Error %d selecting die %d\n", ret, die);
+
+	return ret;
+}
+
+static int winbond_nor_multi_die_ready(struct spi_nor *nor)
+{
+	struct winbond_nor_priv *winbond_priv = nor->priv;
+	unsigned int n_dice = winbond_priv ? winbond_priv->n_dice : 1;
+	int ret, i;
+
+	for (i = 0; i < n_dice; i++) {
+		ret = winbond_nor_select_die(nor, i);
+		if (ret)
+			return ret;
+
+		ret = spi_nor_sr_ready(nor);
+		if (ret <= 0)
+			return ret;
+	}
+
+	return 1;
+}
+
+static void winbond_nor_multi_die_post_sfdp_fixups(struct spi_nor *nor,
+						   struct spi_nor_flash_parameter *param)
+{
+	struct winbond_nor_priv *winbond_priv;
+
+	winbond_priv = kmalloc(sizeof(*winbond_priv), GFP_KERNEL);
+	if (!winbond_priv)
+		return;
+
+	winbond_priv->n_dice = param->size / SZ_64M;
+
+	/*
+	 * 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->priv = winbond_priv;
+	nor->ready = winbond_nor_multi_die_ready;
+}
+
+static struct spi_nor_fixups winbond_nor_multi_die_fixups = {
+	.post_sfdp = winbond_nor_multi_die_post_sfdp_fixups,
+};
+#endif /* CONFIG_SPI_FLASH_WINBOND */
+
 /** spi_nor_octal_dtr_enable() - enable Octal DTR I/O if needed
  * @nor:                 pointer to a 'struct spi_nor'
  *
@@ -4426,6 +4510,23 @@ void spi_nor_set_fixups(struct spi_nor *nor)
 	    nor->info->flags & SPI_NOR_OCTAL_DTR_READ)
 		nor->fixups = &macronix_octal_fixups;
 #endif /* SPI_FLASH_MACRONIX */
+
+#if CONFIG_IS_ENABLED(SPI_FLASH_WINBOND)
+	if (JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) {
+		u8 multi_die_models[][2] = {
+			{ 0x40, 0x21 }, /* W25Q01JV */
+			{ 0x70, 0x22 }, /* W25Q02JV */
+		};
+		int i;
+
+		for (i = 0; i < sizeof(multi_die_models) / 2; i++) {
+			if (!memcmp(nor->info->id + 1, multi_die_models[i], 2)) {
+				nor->fixups = &winbond_nor_multi_die_fixups;
+				break;
+			}
+		}
+	}
+#endif /* SPI_FLASH_WINBOND */
 }
 
 int spi_nor_scan(struct spi_nor *nor)
-- 
2.49.0

Re: [PATCH] mtd: spi-nor: winbond: Make sure w25q{01,02}jv behave correctly

[Miquel Raynal]

Hello,

On 02/07/2025 at 11:23:13 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> These chips are internally made of two/four dies with linear addressing
> capabilities to make it transparent to the user that two/four 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.
>
> The solution adopted here is to iterate manually over all internal
> dies (which takes about 30us per die) until all are ready. This approach
> will always be faster than a blind delay which represents the maximum
> deviation, while also being totally safe.
>
> A flash-specific hook for the status register read had to be
> implemented. Testing with the flash_speed benchmark in Linux shown no
> difference with the existing performances (using the regular status read
> core function).
>
> As the presence of multiple dies is not filled in these chips SFDP
> tables (the table containing the crucial information is optional), we
> need to manually wire the hook.
>
> This change is adapted from Linux.
>
> Link: https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Same question for this one, no feedback for the past 2 months, I'm not
sure who's supposed to take these, Jagan and Vignesh you are marked M:
in maintainers, any chances this can get it?

Thanks a lot,
Miquèl

Re: [PATCH] mtd: spi-nor: winbond: Make sure w25q{01, 02}jv behave correctly

[Jagan Teki]

On Tue, Sep 9, 2025 at 3:43 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hello,
>
> On 02/07/2025 at 11:23:13 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> > These chips are internally made of two/four dies with linear addressing
> > capabilities to make it transparent to the user that two/four 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.
> >
> > The solution adopted here is to iterate manually over all internal
> > dies (which takes about 30us per die) until all are ready. This approach
> > will always be faster than a blind delay which represents the maximum
> > deviation, while also being totally safe.
> >
> > A flash-specific hook for the status register read had to be
> > implemented. Testing with the flash_speed benchmark in Linux shown no
> > difference with the existing performances (using the regular status read
> > core function).
> >
> > As the presence of multiple dies is not filled in these chips SFDP
> > tables (the table containing the crucial information is optional), we
> > need to manually wire the hook.
> >
> > This change is adapted from Linux.
> >
> > Link: https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Same question for this one, no feedback for the past 2 months, I'm not
> sure who's supposed to take these, Jagan and Vignesh you are marked M:
> in maintainers, any chances this can get it?

Unfortunately, I was off quite some-time. Need little bit of time.
Vighnesh is off for years. In the meantime, Michael will help in
review but need help on testing.

Thanks,
Jagan.

Re: [PATCH] mtd: spi-nor: winbond: Make sure w25q{01,02}jv behave correctly

[Miquel Raynal]

Hello,

On 09/09/2025 at 17:37:24 +0530, Jagan Teki <jagan@amarulasolutions.com> wrote:

> On Tue, Sep 9, 2025 at 3:43 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> Hello,
>>
>> On 02/07/2025 at 11:23:13 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> > These chips are internally made of two/four dies with linear addressing
>> > capabilities to make it transparent to the user that two/four 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.
>> >
>> > The solution adopted here is to iterate manually over all internal
>> > dies (which takes about 30us per die) until all are ready. This approach
>> > will always be faster than a blind delay which represents the maximum
>> > deviation, while also being totally safe.
>> >
>> > A flash-specific hook for the status register read had to be
>> > implemented. Testing with the flash_speed benchmark in Linux shown no
>> > difference with the existing performances (using the regular status read
>> > core function).
>> >
>> > As the presence of multiple dies is not filled in these chips SFDP
>> > tables (the table containing the crucial information is optional), we
>> > need to manually wire the hook.
>> >
>> > This change is adapted from Linux.
>> >
>> > Link: https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
>> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>
>> Same question for this one, no feedback for the past 2 months, I'm not
>> sure who's supposed to take these, Jagan and Vignesh you are marked M:
>> in maintainers, any chances this can get it?
>
> Unfortunately, I was off quite some-time. Need little bit of time.
> Vighnesh is off for years. In the meantime, Michael will help in
> review but need help on testing.

I didn't add Michael because he was not listed for this change, perhaps
he should be added to the MAINTAINERS file? (or maybe I used an old
version without noticing)

I don't know if testing each every change carefully makes sense in this
context. There have been many contributions probably worth the try (not
mine, Mikhail's) which are hard if not impossible to test by a single
person, while quite relevant. Reviewing seems quicker and more relevant?

Thanks,
Miquèl

Re: [PATCH] mtd: spi-nor: winbond: Make sure w25q{01,02}jv behave correctly

[Raghavendra, Vignesh]

On 9/9/2025 5:37 PM, Jagan Teki wrote:
> On Tue, Sep 9, 2025 at 3:43 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> Hello,
>>
>> On 02/07/2025 at 11:23:13 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>>> These chips are internally made of two/four dies with linear addressing
>>> capabilities to make it transparent to the user that two/four 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.
>>>
>>> The solution adopted here is to iterate manually over all internal
>>> dies (which takes about 30us per die) until all are ready. This approach
>>> will always be faster than a blind delay which represents the maximum
>>> deviation, while also being totally safe.
>>>
>>> A flash-specific hook for the status register read had to be
>>> implemented. Testing with the flash_speed benchmark in Linux shown no
>>> difference with the existing performances (using the regular status read
>>> core function).
>>>
>>> As the presence of multiple dies is not filled in these chips SFDP
>>> tables (the table containing the crucial information is optional), we
>>> need to manually wire the hook.
>>>
>>> This change is adapted from Linux.
>>>
>>> Link: https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>
>> Same question for this one, no feedback for the past 2 months, I'm not
>> sure who's supposed to take these, Jagan and Vignesh you are marked M:
>> in maintainers, any chances this can get it?
> 
> Unfortunately, I was off quite some-time. Need little bit of time.
> Vighnesh is off for years. 

Thats not true. I dont have access to U-Boot SPI trees. So patches have
been flowing through individual "platfor/SoC" maintainers trees
unfortunately. Tom picks the patches from TI contributors once
me/Nishanth Acks where as AMD/Xlinix bits have been picked up by Michal
Simek and so on...

Happy to help out if you help to manage a merge window every now and
then or just review things. Most of the SPI NOR/NAND follow from linux
where its reviewed by set of maintainers and tested (example this one is
tested on TI board).

> In the meantime, Michael will help in
> review but need help on testing.
> 
> Thanks,
> Jagan.

Re: [PATCH] mtd: spi-nor: winbond: Make sure w25q{01, 02}jv behave correctly

[Michael Nazzareno Trimarchi]

Hi all

On Tue, Sep 9, 2025 at 7:08 PM Raghavendra, Vignesh <vigneshr@ti.com> wrote:
>
>
>
> On 9/9/2025 5:37 PM, Jagan Teki wrote:
> > On Tue, Sep 9, 2025 at 3:43 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >>
> >> Hello,
> >>
> >> On 02/07/2025 at 11:23:13 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >>
> >>> These chips are internally made of two/four dies with linear addressing
> >>> capabilities to make it transparent to the user that two/four 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.
> >>>
> >>> The solution adopted here is to iterate manually over all internal
> >>> dies (which takes about 30us per die) until all are ready. This approach
> >>> will always be faster than a blind delay which represents the maximum
> >>> deviation, while also being totally safe.
> >>>
> >>> A flash-specific hook for the status register read had to be
> >>> implemented. Testing with the flash_speed benchmark in Linux shown no
> >>> difference with the existing performances (using the regular status read
> >>> core function).
> >>>
> >>> As the presence of multiple dies is not filled in these chips SFDP
> >>> tables (the table containing the crucial information is optional), we
> >>> need to manually wire the hook.
> >>>
> >>> This change is adapted from Linux.
> >>>
> >>> Link: https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
> >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>
> >> Same question for this one, no feedback for the past 2 months, I'm not
> >> sure who's supposed to take these, Jagan and Vignesh you are marked M:
> >> in maintainers, any chances this can get it?
> >
> > Unfortunately, I was off quite some-time. Need little bit of time.
> > Vighnesh is off for years.
>
> Thats not true. I dont have access to U-Boot SPI trees. So patches have
> been flowing through individual "platfor/SoC" maintainers trees
> unfortunately. Tom picks the patches from TI contributors once
> me/Nishanth Acks where as AMD/Xlinix bits have been picked up by Michal
> Simek and so on...
>
> Happy to help out if you help to manage a merge window every now and
> then or just review things. Most of the SPI NOR/NAND follow from linux
> where its reviewed by set of maintainers and tested (example this one is
> tested on TI board).

I will go through the patches over the weekend and review them. I will
prepare a branch
spi-nor-next and try to ask to test from there

Michael

>
> > In the meantime, Michael will help in
> > review but need help on testing.
> >
> > Thanks,
> > Jagan.
>


-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH] mtd: spi-nor: winbond: Make sure w25q{01,02}jv behave correctly

[Miquel Raynal]

Hi Michael,

On 10/09/2025 at 07:28:22 +02, Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi all
>
> On Tue, Sep 9, 2025 at 7:08 PM Raghavendra, Vignesh <vigneshr@ti.com> wrote:
>>
>>
>>
>> On 9/9/2025 5:37 PM, Jagan Teki wrote:
>> > On Tue, Sep 9, 2025 at 3:43 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>> >>
>> >> Hello,
>> >>
>> >> On 02/07/2025 at 11:23:13 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>> >>
>> >>> These chips are internally made of two/four dies with linear addressing
>> >>> capabilities to make it transparent to the user that two/four 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.
>> >>>
>> >>> The solution adopted here is to iterate manually over all internal
>> >>> dies (which takes about 30us per die) until all are ready. This approach
>> >>> will always be faster than a blind delay which represents the maximum
>> >>> deviation, while also being totally safe.
>> >>>
>> >>> A flash-specific hook for the status register read had to be
>> >>> implemented. Testing with the flash_speed benchmark in Linux shown no
>> >>> difference with the existing performances (using the regular status read
>> >>> core function).
>> >>>
>> >>> As the presence of multiple dies is not filled in these chips SFDP
>> >>> tables (the table containing the crucial information is optional), we
>> >>> need to manually wire the hook.
>> >>>
>> >>> This change is adapted from Linux.
>> >>>
>> >>> Link: https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
>> >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> >>
>> >> Same question for this one, no feedback for the past 2 months, I'm not
>> >> sure who's supposed to take these, Jagan and Vignesh you are marked M:
>> >> in maintainers, any chances this can get it?
>> >
>> > Unfortunately, I was off quite some-time. Need little bit of time.
>> > Vighnesh is off for years.
>>
>> Thats not true. I dont have access to U-Boot SPI trees. So patches have
>> been flowing through individual "platfor/SoC" maintainers trees
>> unfortunately. Tom picks the patches from TI contributors once
>> me/Nishanth Acks where as AMD/Xlinix bits have been picked up by Michal
>> Simek and so on...
>>
>> Happy to help out if you help to manage a merge window every now and
>> then or just review things. Most of the SPI NOR/NAND follow from linux
>> where its reviewed by set of maintainers and tested (example this one is
>> tested on TI board).
>
> I will go through the patches over the weekend and review them. I will
> prepare a branch
> spi-nor-next and try to ask to test from there
>
> Michael
>
>>
>> > In the meantime, Michael will help in
>> > review but need help on testing.
>> >
>> > Thanks,
>> > Jagan.
>>

I still do not see these patches in any upstream tree. Am I missing
something? If I may, these patches have been reviewed during the Linux
contribution process already, and submission happened in July, almost 5
months ago.

Thanks,
Miquèl

Re: [PATCH] mtd: spi-nor: winbond: Make sure w25q{01, 02}jv behave correctly

[Tom Rini]

On Wed, 02 Jul 2025 11:23:13 +0200, Miquel Raynal wrote:

> These chips are internally made of two/four dies with linear addressing
> capabilities to make it transparent to the user that two/four 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.
> 
> [...]

Applied to u-boot/next, thanks!

[1/1] mtd: spi-nor: winbond: Make sure w25q{01, 02}jv behave correctly
      commit: 4f76c90bad3867c402cae28b45c928626bedefff
-- 
Tom