public inbox for stable@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
       [not found] <20260220094236.28-1-sanjaikumarvs@gmail.com>
@ 2026-02-20  9:42 ` Sanjaikumar V S
  2026-02-23  8:18   ` Michael Walle
  2026-02-20  9:42 ` [PATCH v2 2/2] mtd: spi-nor: core: Fix AAI mode when dirmap is not available Sanjaikumar V S
  2026-02-23 10:17 ` [PATCH v3 0/2] mtd: spi-nor: Fix SST AAI write mode Sanjaikumar V S
  2 siblings, 1 reply; 12+ messages in thread
From: Sanjaikumar V S @ 2026-02-20  9:42 UTC (permalink / raw)
  To: linux-mtd
  Cc: tudor.ambarus, pratyush, mwalle, miquel.raynal, richard, vigneshr,
	linux-kernel, Sanjaikumar V S, stable

From: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>

When writing to SST flash starting at an odd address, a single byte is
first programmed using the byte program (BP) command. After this
operation completes, the flash hardware automatically clears the Write
Enable Latch (WEL) bit.

If an AAI (Auto Address Increment) word program sequence follows, it
requires WEL to be set. Without re-enabling writes, the AAI sequence
fails.

Add spi_nor_write_enable() after the odd-address byte program, but only
when an AAI sequence will follow (len > 2 bytes remaining).

Cc: stable@vger.kernel.org
Signed-off-by: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>
---
 drivers/mtd/spi-nor/sst.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index 175211fe6a5e..fe714e6d0914 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -210,6 +210,13 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 		to++;
 		actual++;
+
+		/* BP clears WEL, re-enable if AAI sequence follows */
+		if (actual < len - 1) {
+			ret = spi_nor_write_enable(nor);
+			if (ret)
+				goto out;
+		}
 	}
 
 	/* Write out most of the data here. */
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] mtd: spi-nor: core: Fix AAI mode when dirmap is not available
       [not found] <20260220094236.28-1-sanjaikumarvs@gmail.com>
  2026-02-20  9:42 ` [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence Sanjaikumar V S
@ 2026-02-20  9:42 ` Sanjaikumar V S
  2026-02-23 10:17 ` [PATCH v3 0/2] mtd: spi-nor: Fix SST AAI write mode Sanjaikumar V S
  2 siblings, 0 replies; 12+ messages in thread
From: Sanjaikumar V S @ 2026-02-20  9:42 UTC (permalink / raw)
  To: linux-mtd
  Cc: tudor.ambarus, pratyush, mwalle, miquel.raynal, richard, vigneshr,
	linux-kernel, Sanjaikumar V S, stable

From: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>

When the SPI controller does not support direct mapping (nodirmap=true),
spi_nor_spimem_write_data() calls spi_mem_dirmap_write() which falls
back to spi_mem_no_dirmap_write(). This fallback uses the operation
template created at probe time with the standard page program opcode.

For SST flashes using AAI mode, this fails because the template cannot
handle the dynamic opcode and address byte changes required by AAI.

Fix by checking nodirmap and using spi_nor_spimem_exec_op() directly,
which uses the runtime-built operation with correct AAI configuration.

Cc: stable@vger.kernel.org
Signed-off-by: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>
---
 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index d3f8a78efd3b..7caeb508d628 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -281,7 +281,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
 	if (spi_nor_spimem_bounce(nor, &op))
 		memcpy(nor->bouncebuf, buf, op.data.nbytes);
 
-	if (nor->dirmap.wdesc) {
+	if (nor->dirmap.wdesc && !nor->dirmap.wdesc->nodirmap) {
 		nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, op.addr.val,
 					      op.data.nbytes, op.data.buf.out);
 	} else {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
  2026-02-20  9:42 ` [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence Sanjaikumar V S
@ 2026-02-23  8:18   ` Michael Walle
  2026-02-23  9:17     ` Sanjaikumar V S
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Walle @ 2026-02-23  8:18 UTC (permalink / raw)
  To: Sanjaikumar V S, linux-mtd
  Cc: tudor.ambarus, pratyush, miquel.raynal, richard, vigneshr,
	linux-kernel, Sanjaikumar V S, stable

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Hi,

On Fri Feb 20, 2026 at 10:42 AM CET, Sanjaikumar V S wrote:
> From: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>
>
> When writing to SST flash starting at an odd address, a single byte is
> first programmed using the byte program (BP) command. After this
> operation completes, the flash hardware automatically clears the Write
> Enable Latch (WEL) bit.
>
> If an AAI (Auto Address Increment) word program sequence follows, it
> requires WEL to be set. Without re-enabling writes, the AAI sequence
> fails.
>
> Add spi_nor_write_enable() after the odd-address byte program, but only
> when an AAI sequence will follow (len > 2 bytes remaining).
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>

Very strange. If that is correct, it actually never worked
correctly.

Fixes: b199489d37b2 ("mtd: spi-nor: add the framework for SPI NOR")

You've said, you didn't test this. Shouldn't it be pretty easy to
write to an odd offset? Also, from reading the code, it looks like
if write ends on an odd offset, it doesn't work either?

> ---
>  drivers/mtd/spi-nor/sst.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 175211fe6a5e..fe714e6d0914 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -210,6 +210,13 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  
>  		to++;
>  		actual++;
> +
> +		/* BP clears WEL, re-enable if AAI sequence follows */
> +		if (actual < len - 1) {

For brevity, I'd drop the repeated condition and just do the
write_enable unconditionally.

-michael

> +			ret = spi_nor_write_enable(nor);
> +			if (ret)
> +				goto out;
> +		}
>  	}
>  
>  	/* Write out most of the data here. */


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
  2026-02-23  8:18   ` Michael Walle
@ 2026-02-23  9:17     ` Sanjaikumar V S
  2026-02-23  9:29       ` Michael Walle
  0 siblings, 1 reply; 12+ messages in thread
From: Sanjaikumar V S @ 2026-02-23  9:17 UTC (permalink / raw)
  To: mwalle
  Cc: linux-kernel, linux-mtd, miquel.raynal, pratyush, richard,
	sanjaikumar.vs, sanjaikumarvs, stable, tudor.ambarus, vigneshr

Hi Michael,

Thank you for the review.

> Raises concern about writes ending at odd offsets potentially having the same issue

The odd end address case (trailing byte) is already handled in the existing code at lines 243-255:

/* Write out trailing byte if it exists. */
if (actual != len) {
    ret = spi_nor_write_enable(nor);
    ...
    ret = sst_nor_write_data(nor, to, 1, buf + actual);
}

So write_enable is already called before writing the trailing byte. My patch only addresses the odd start case where BP clears WEL before the AAI sequence begins.

> Suggests simplifying the conditional logic by removing the length check

The condition `if (actual < len - 1)` avoids an unnecessary write_enable when len == 1 (single byte write at odd address, no AAI follows). But if you prefer unconditional write_enable for simplicity, I can change it in v3.

> Notes the patch lacks runtime testing

I don't have the hardware setup to test odd-address writes at the moment. The fix is based on code analysis. I have tested patch 2/2 (dirmap fallback) on hardware.

Please let me know if you'd like me to send a v3 with the simplified unconditional write_enable.

Thanks,
Sanjaikumar

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
  2026-02-23  9:17     ` Sanjaikumar V S
@ 2026-02-23  9:29       ` Michael Walle
  2026-03-06 22:36         ` Hendrik Donner
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Walle @ 2026-02-23  9:29 UTC (permalink / raw)
  To: Sanjaikumar V S
  Cc: linux-kernel, linux-mtd, miquel.raynal, pratyush, richard,
	sanjaikumar.vs, stable, tudor.ambarus, vigneshr

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

Hi,

On Mon Feb 23, 2026 at 10:17 AM CET, Sanjaikumar V S wrote:
>> Raises concern about writes ending at odd offsets potentially
>> having the same issue
>
> The odd end address case (trailing byte) is already handled in the
> existing code at lines 243-255:
>
> /* Write out trailing byte if it exists. */
> if (actual != len) {
>     ret = spi_nor_write_enable(nor);
>     ...
>     ret = sst_nor_write_data(nor, to, 1, buf + actual);
> }

Ah, I must be blind. I stopped reading at the write_disable.

> So write_enable is already called before writing the trailing
> byte. My patch only addresses the odd start case where BP clears
> WEL before the AAI sequence begins.
>
>> Suggests simplifying the conditional logic by removing the length
>> check
>
> The condition `if (actual < len - 1)` avoids an unnecessary
> write_enable when len == 1 (single byte write at odd address, no
> AAI follows). But if you prefer unconditional write_enable for
> simplicity, I can change it in v3.

I know, but I actually don't like repeating the condition in the for
loop. So I'd prefer to have a local "needs_write_enable" boolean
which will be set to true. But then, I wouldn't care too much if
there is a write enable followed by a write disable for a rare case.

>> Notes the patch lacks runtime testing
>
> I don't have the hardware setup to test odd-address writes at the
> moment. The fix is based on code analysis. I have tested patch 2/2
> (dirmap fallback) on hardware.

I'm hesitant - because like I said, if there is really a bug - it
would have never worked correctly, since day 1. But yeah, I've also
read the datasheet and it clearly states that the byte write will
clear the write enable latch.

> Please let me know if you'd like me to send a v3 with the
> simplified unconditional write_enable.

Please see above.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 0/2] mtd: spi-nor: Fix SST AAI write mode
       [not found] <20260220094236.28-1-sanjaikumarvs@gmail.com>
  2026-02-20  9:42 ` [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence Sanjaikumar V S
  2026-02-20  9:42 ` [PATCH v2 2/2] mtd: spi-nor: core: Fix AAI mode when dirmap is not available Sanjaikumar V S
@ 2026-02-23 10:17 ` Sanjaikumar V S
  2026-02-23 10:17   ` [PATCH v3 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence Sanjaikumar V S
  2026-02-23 10:17   ` [PATCH v3 2/2] mtd: spi-nor: core: Fix AAI mode when dirmap is not available Sanjaikumar V S
  2 siblings, 2 replies; 12+ messages in thread
From: Sanjaikumar V S @ 2026-02-23 10:17 UTC (permalink / raw)
  To: mwalle
  Cc: linux-kernel, linux-mtd, miquel.raynal, pratyush, richard,
	sanjaikumarvs, sanjaikumar.vs, tudor.ambarus, stable, vigneshr

From: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>

This patch series addresses two distinct problems affecting SST flash
Auto Address Increment write functionality:

1. When writes begin at odd addresses, a single byte is programmed first
   using byte program command, which clears the Write Enable Latch. The
   driver fails to re-enable writes before the AAI sequence.

2. When the SPI controller lacks direct mapping support, the fallback
   path uses a probe-time operation template with standard page program
   opcodes instead of AAI opcodes.

Changes in v3:
- Patch 1/2: Use local boolean 'needs_write_enable' for clarity as
  suggested by Michael Walle
- Patch 1/2: Improved comment explaining the fix
- Patch 1/2: Added Fixes tag

Changes in v2:
- Split fixes into separate patches
- Added detailed commit messages

Sanjaikumar V S (2):
  mtd: spi-nor: sst: Fix write enable before AAI sequence
  mtd: spi-nor: core: Fix AAI mode when dirmap is not available

 drivers/mtd/spi-nor/core.c |  2 +-
 drivers/mtd/spi-nor/sst.c  | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

--
2.43.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
  2026-02-23 10:17 ` [PATCH v3 0/2] mtd: spi-nor: Fix SST AAI write mode Sanjaikumar V S
@ 2026-02-23 10:17   ` Sanjaikumar V S
  2026-02-23 10:17   ` [PATCH v3 2/2] mtd: spi-nor: core: Fix AAI mode when dirmap is not available Sanjaikumar V S
  1 sibling, 0 replies; 12+ messages in thread
From: Sanjaikumar V S @ 2026-02-23 10:17 UTC (permalink / raw)
  To: mwalle
  Cc: linux-kernel, linux-mtd, miquel.raynal, pratyush, richard,
	sanjaikumarvs, sanjaikumar.vs, tudor.ambarus, stable, vigneshr

From: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>

When writing to SST flash starting at an odd address, a single byte is
first programmed using the byte program (BP) command. After this
operation completes, the flash hardware automatically clears the Write
Enable Latch (WEL) bit.

If an AAI (Auto Address Increment) word program sequence follows, it
requires WEL to be set. Without re-enabling writes, the AAI sequence
fails.

Add spi_nor_write_enable() after the odd-address byte program when more
data needs to be written. Use a local boolean for clarity.

Fixes: b199489d37b2 ("mtd: spi-nor: add the framework for SPI NOR")
Cc: stable@vger.kernel.org
Signed-off-by: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>
---
 drivers/mtd/spi-nor/sst.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index 175211fe6a5e..db02c14ba16f 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -203,6 +203,8 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 	/* Start write from odd address. */
 	if (to % 2) {
+		bool needs_write_enable = (len > 1);
+
 		/* write one byte. */
 		ret = sst_nor_write_data(nor, to, 1, buf);
 		if (ret < 0)
@@ -210,6 +212,17 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 		to++;
 		actual++;
+
+		/*
+		 * Byte program clears the write enable latch. If more
+		 * data needs to be written using the AAI sequence,
+		 * re-enable writes.
+		 */
+		if (needs_write_enable) {
+			ret = spi_nor_write_enable(nor);
+			if (ret)
+				goto out;
+		}
 	}
 
 	/* Write out most of the data here. */
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 2/2] mtd: spi-nor: core: Fix AAI mode when dirmap is not available
  2026-02-23 10:17 ` [PATCH v3 0/2] mtd: spi-nor: Fix SST AAI write mode Sanjaikumar V S
  2026-02-23 10:17   ` [PATCH v3 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence Sanjaikumar V S
@ 2026-02-23 10:17   ` Sanjaikumar V S
  1 sibling, 0 replies; 12+ messages in thread
From: Sanjaikumar V S @ 2026-02-23 10:17 UTC (permalink / raw)
  To: mwalle
  Cc: linux-kernel, linux-mtd, miquel.raynal, pratyush, richard,
	sanjaikumarvs, sanjaikumar.vs, tudor.ambarus, stable, vigneshr

From: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>

When the SPI controller does not support direct mapping (nodirmap=true),
spi_nor_spimem_write_data() calls spi_mem_dirmap_write() which falls
back to spi_mem_no_dirmap_write(). This fallback uses the operation
template created at probe time with the standard page program opcode.

For SST flashes using AAI mode, this fails because the template cannot
handle the dynamic opcode and address byte changes required by AAI.

Fix by checking nodirmap and using spi_nor_spimem_exec_op() directly,
which uses the runtime-built operation with correct AAI configuration.

Cc: stable@vger.kernel.org
Signed-off-by: Sanjaikumar V S <sanjaikumar.vs@dicortech.com>
---
 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index d3f8a78efd3b..7caeb508d628 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -281,7 +281,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
 	if (spi_nor_spimem_bounce(nor, &op))
 		memcpy(nor->bouncebuf, buf, op.data.nbytes);
 
-	if (nor->dirmap.wdesc) {
+	if (nor->dirmap.wdesc && !nor->dirmap.wdesc->nodirmap) {
 		nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, op.addr.val,
 					      op.data.nbytes, op.data.buf.out);
 	} else {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
  2026-02-23  9:29       ` Michael Walle
@ 2026-03-06 22:36         ` Hendrik Donner
  2026-03-13 11:46           ` Pratyush Yadav
  0 siblings, 1 reply; 12+ messages in thread
From: Hendrik Donner @ 2026-03-06 22:36 UTC (permalink / raw)
  To: Michael Walle, Sanjaikumar V S
  Cc: linux-kernel, linux-mtd, miquel.raynal, pratyush, richard,
	sanjaikumar.vs, stable, tudor.ambarus, vigneshr

Hello,

On 2/23/26 10:29, Michael Walle wrote:
> Hi,
> 
> On Mon Feb 23, 2026 at 10:17 AM CET, Sanjaikumar V S wrote:
>>> Raises concern about writes ending at odd offsets potentially
>>> having the same issue
>>
>> The odd end address case (trailing byte) is already handled in the
>> existing code at lines 243-255:
>>
>> /* Write out trailing byte if it exists. */
>> if (actual != len) {
>>      ret = spi_nor_write_enable(nor);
>>      ...
>>      ret = sst_nor_write_data(nor, to, 1, buf + actual);
>> }
> 
> Ah, I must be blind. I stopped reading at the write_disable.
> 
>> So write_enable is already called before writing the trailing
>> byte. My patch only addresses the odd start case where BP clears
>> WEL before the AAI sequence begins.
>>
>>> Suggests simplifying the conditional logic by removing the length
>>> check
>>
>> The condition `if (actual < len - 1)` avoids an unnecessary
>> write_enable when len == 1 (single byte write at odd address, no
>> AAI follows). But if you prefer unconditional write_enable for
>> simplicity, I can change it in v3.
> 
> I know, but I actually don't like repeating the condition in the for
> loop. So I'd prefer to have a local "needs_write_enable" boolean
> which will be set to true. But then, I wouldn't care too much if
> there is a write enable followed by a write disable for a rare case.
> 
>>> Notes the patch lacks runtime testing
>>
>> I don't have the hardware setup to test odd-address writes at the
>> moment. The fix is based on code analysis. I have tested patch 2/2
>> (dirmap fallback) on hardware.
> 
> I'm hesitant - because like I said, if there is really a bug - it
> would have never worked correctly, since day 1. But yeah, I've also
> read the datasheet and it clearly states that the byte write will
> clear the write enable latch.
> 

i can confirm both patches fix real issues, i have similiar fixes
on a kernel tree i always wanted to clean up and upstream. Diffs
based on 6.6.127:

diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index 197d2c1101ed5..eaa50561ede2c 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -155,6 +155,13 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
                 if (ret)
                         goto out;

+               ret = spi_nor_write_enable(nor);
+               if (ret)
+                       goto out;
+               ret = spi_nor_wait_till_ready(nor);
+               if (ret)
+                       goto out;
+
                 to++;
                 actual++;
         }


diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1b0c6770c14e4..646bfb2e91a65 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -276,7 +276,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
         if (spi_nor_spimem_bounce(nor, &op))
                 memcpy(nor->bouncebuf, buf, op.data.nbytes);

-       if (nor->dirmap.wdesc) {
+       if (nor->dirmap.wdesc && nor->program_opcode != SPINOR_OP_AAI_WP) {
                 nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, op.addr.val,
                                               op.data.nbytes, op.data.buf.out);
         } else {


I think patch 2 of this series is the better approach though.

Regards,
Hendrik

>> Please let me know if you'd like me to send a v3 with the
>> simplified unconditional write_enable.
> 
> Please see above.
> 
> -michael
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
  2026-03-06 22:36         ` Hendrik Donner
@ 2026-03-13 11:46           ` Pratyush Yadav
  2026-03-13 12:50             ` Hendrik Donner
  0 siblings, 1 reply; 12+ messages in thread
From: Pratyush Yadav @ 2026-03-13 11:46 UTC (permalink / raw)
  To: Hendrik Donner
  Cc: Michael Walle, Sanjaikumar V S, linux-kernel, linux-mtd,
	miquel.raynal, pratyush, richard, sanjaikumar.vs, stable,
	tudor.ambarus, vigneshr

On Fri, Mar 06 2026, Hendrik Donner wrote:

> Hello,
>
> On 2/23/26 10:29, Michael Walle wrote:
>> Hi,
>> On Mon Feb 23, 2026 at 10:17 AM CET, Sanjaikumar V S wrote:
>>>> Raises concern about writes ending at odd offsets potentially
>>>> having the same issue
>>>
>>> The odd end address case (trailing byte) is already handled in the
>>> existing code at lines 243-255:
>>>
>>> /* Write out trailing byte if it exists. */
>>> if (actual != len) {
>>>      ret = spi_nor_write_enable(nor);
>>>      ...
>>>      ret = sst_nor_write_data(nor, to, 1, buf + actual);
>>> }
>> Ah, I must be blind. I stopped reading at the write_disable.
>> 
>>> So write_enable is already called before writing the trailing
>>> byte. My patch only addresses the odd start case where BP clears
>>> WEL before the AAI sequence begins.
>>>
>>>> Suggests simplifying the conditional logic by removing the length
>>>> check
>>>
>>> The condition `if (actual < len - 1)` avoids an unnecessary
>>> write_enable when len == 1 (single byte write at odd address, no
>>> AAI follows). But if you prefer unconditional write_enable for
>>> simplicity, I can change it in v3.
>> I know, but I actually don't like repeating the condition in the for
>> loop. So I'd prefer to have a local "needs_write_enable" boolean
>> which will be set to true. But then, I wouldn't care too much if
>> there is a write enable followed by a write disable for a rare case.
>> 
>>>> Notes the patch lacks runtime testing
>>>
>>> I don't have the hardware setup to test odd-address writes at the
>>> moment. The fix is based on code analysis. I have tested patch 2/2
>>> (dirmap fallback) on hardware.
>> I'm hesitant - because like I said, if there is really a bug - it
>> would have never worked correctly, since day 1. But yeah, I've also
>> read the datasheet and it clearly states that the byte write will
>> clear the write enable latch.
>> 
>
> i can confirm both patches fix real issues, i have similiar fixes
> on a kernel tree i always wanted to clean up and upstream. Diffs
> based on 6.6.127:
>
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 197d2c1101ed5..eaa50561ede2c 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -155,6 +155,13 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>                 if (ret)
>                         goto out;
>
> +               ret = spi_nor_write_enable(nor);
> +               if (ret)
> +                       goto out;
> +               ret = spi_nor_wait_till_ready(nor);
> +               if (ret)
> +                       goto out;
> +
>                 to++;
>                 actual++;
>         }
>
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1b0c6770c14e4..646bfb2e91a65 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -276,7 +276,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
>         if (spi_nor_spimem_bounce(nor, &op))
>                 memcpy(nor->bouncebuf, buf, op.data.nbytes);
>
> -       if (nor->dirmap.wdesc) {
> +       if (nor->dirmap.wdesc && nor->program_opcode != SPINOR_OP_AAI_WP) {

Why is this better? This removes the use of dirmap for all flashes other
than SST.

>                 nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, op.addr.val,
>                                               op.data.nbytes, op.data.buf.out);
>         } else {
>
>
> I think patch 2 of this series is the better approach though.

There is a v4 here:
https://lore.kernel.org/linux-mtd/20260311103057.29-1-sanjaikumarvs@gmail.com/T/#u

Can you please review and test it so we can apply it?

>
> Regards,
> Hendrik
>
>>> Please let me know if you'd like me to send a v3 with the
>>> simplified unconditional write_enable.
>> Please see above.
>> -michael
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

-- 
Regards,
Pratyush Yadav

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
  2026-03-13 11:46           ` Pratyush Yadav
@ 2026-03-13 12:50             ` Hendrik Donner
  2026-03-13 13:39               ` Pratyush Yadav
  0 siblings, 1 reply; 12+ messages in thread
From: Hendrik Donner @ 2026-03-13 12:50 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: Michael Walle, Sanjaikumar V S, linux-kernel, linux-mtd,
	miquel.raynal, richard, sanjaikumar.vs, stable, tudor.ambarus,
	vigneshr

Hello,

On 3/13/26 12:46, Pratyush Yadav wrote:
> On Fri, Mar 06 2026, Hendrik Donner wrote:
> 
>> Hello,
>>
>> On 2/23/26 10:29, Michael Walle wrote:
>>> Hi,
>>> On Mon Feb 23, 2026 at 10:17 AM CET, Sanjaikumar V S wrote:
>>>>> Raises concern about writes ending at odd offsets potentially
>>>>> having the same issue
>>>>
>>>> The odd end address case (trailing byte) is already handled in the
>>>> existing code at lines 243-255:
>>>>
>>>> /* Write out trailing byte if it exists. */
>>>> if (actual != len) {
>>>>       ret = spi_nor_write_enable(nor);
>>>>       ...
>>>>       ret = sst_nor_write_data(nor, to, 1, buf + actual);
>>>> }
>>> Ah, I must be blind. I stopped reading at the write_disable.
>>>
>>>> So write_enable is already called before writing the trailing
>>>> byte. My patch only addresses the odd start case where BP clears
>>>> WEL before the AAI sequence begins.
>>>>
>>>>> Suggests simplifying the conditional logic by removing the length
>>>>> check
>>>>
>>>> The condition `if (actual < len - 1)` avoids an unnecessary
>>>> write_enable when len == 1 (single byte write at odd address, no
>>>> AAI follows). But if you prefer unconditional write_enable for
>>>> simplicity, I can change it in v3.
>>> I know, but I actually don't like repeating the condition in the for
>>> loop. So I'd prefer to have a local "needs_write_enable" boolean
>>> which will be set to true. But then, I wouldn't care too much if
>>> there is a write enable followed by a write disable for a rare case.
>>>
>>>>> Notes the patch lacks runtime testing
>>>>
>>>> I don't have the hardware setup to test odd-address writes at the
>>>> moment. The fix is based on code analysis. I have tested patch 2/2
>>>> (dirmap fallback) on hardware.
>>> I'm hesitant - because like I said, if there is really a bug - it
>>> would have never worked correctly, since day 1. But yeah, I've also
>>> read the datasheet and it clearly states that the byte write will
>>> clear the write enable latch.
>>>
>>
>> i can confirm both patches fix real issues, i have similiar fixes
>> on a kernel tree i always wanted to clean up and upstream. Diffs
>> based on 6.6.127:
>>
>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>> index 197d2c1101ed5..eaa50561ede2c 100644
>> --- a/drivers/mtd/spi-nor/sst.c
>> +++ b/drivers/mtd/spi-nor/sst.c
>> @@ -155,6 +155,13 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>                  if (ret)
>>                          goto out;
>>
>> +               ret = spi_nor_write_enable(nor);
>> +               if (ret)
>> +                       goto out;
>> +               ret = spi_nor_wait_till_ready(nor);
>> +               if (ret)
>> +                       goto out;
>> +
>>                  to++;
>>                  actual++;
>>          }
>>
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 1b0c6770c14e4..646bfb2e91a65 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -276,7 +276,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
>>          if (spi_nor_spimem_bounce(nor, &op))
>>                  memcpy(nor->bouncebuf, buf, op.data.nbytes);
>>
>> -       if (nor->dirmap.wdesc) {
>> +       if (nor->dirmap.wdesc && nor->program_opcode != SPINOR_OP_AAI_WP) {
> 
> Why is this better? This removes the use of dirmap for all flashes other
> than SST.

i claim the opposite down below? That patch 2 of the posted patch series
looks better to me. Sorry if that was unclear.

Regards,
Hendrik

> 
>>                  nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc, op.addr.val,
>>                                                op.data.nbytes, op.data.buf.out);
>>          } else {
>>
>>
>> I think patch 2 of this series is the better approach though.
> 
> There is a v4 here:
> https://lore.kernel.org/linux-mtd/20260311103057.29-1-sanjaikumarvs@gmail.com/T/#u
> 
> Can you please review and test it so we can apply it?
> 
>>
>> Regards,
>> Hendrik
>>
>>>> Please let me know if you'd like me to send a v3 with the
>>>> simplified unconditional write_enable.
>>> Please see above.
>>> -michael
>>> ______________________________________________________
>>> Linux MTD discussion mailing list
>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence
  2026-03-13 12:50             ` Hendrik Donner
@ 2026-03-13 13:39               ` Pratyush Yadav
  0 siblings, 0 replies; 12+ messages in thread
From: Pratyush Yadav @ 2026-03-13 13:39 UTC (permalink / raw)
  To: Hendrik Donner
  Cc: Pratyush Yadav, Michael Walle, Sanjaikumar V S, linux-kernel,
	linux-mtd, miquel.raynal, richard, sanjaikumar.vs, stable,
	tudor.ambarus, vigneshr

On Fri, Mar 13 2026, Hendrik Donner wrote:

> Hello,
>
> On 3/13/26 12:46, Pratyush Yadav wrote:
>> On Fri, Mar 06 2026, Hendrik Donner wrote:
>> 
>>> Hello,
>>>
>>> On 2/23/26 10:29, Michael Walle wrote:
>>>> Hi,
>>>> On Mon Feb 23, 2026 at 10:17 AM CET, Sanjaikumar V S wrote:
>>>>>> Raises concern about writes ending at odd offsets potentially
>>>>>> having the same issue
>>>>>
>>>>> The odd end address case (trailing byte) is already handled in the
>>>>> existing code at lines 243-255:
>>>>>
>>>>> /* Write out trailing byte if it exists. */
>>>>> if (actual != len) {
>>>>>       ret = spi_nor_write_enable(nor);
>>>>>       ...
>>>>>       ret = sst_nor_write_data(nor, to, 1, buf + actual);
>>>>> }
>>>> Ah, I must be blind. I stopped reading at the write_disable.
>>>>
>>>>> So write_enable is already called before writing the trailing
>>>>> byte. My patch only addresses the odd start case where BP clears
>>>>> WEL before the AAI sequence begins.
>>>>>
>>>>>> Suggests simplifying the conditional logic by removing the length
>>>>>> check
>>>>>
>>>>> The condition `if (actual < len - 1)` avoids an unnecessary
>>>>> write_enable when len == 1 (single byte write at odd address, no
>>>>> AAI follows). But if you prefer unconditional write_enable for
>>>>> simplicity, I can change it in v3.
>>>> I know, but I actually don't like repeating the condition in the for
>>>> loop. So I'd prefer to have a local "needs_write_enable" boolean
>>>> which will be set to true. But then, I wouldn't care too much if
>>>> there is a write enable followed by a write disable for a rare case.
>>>>
>>>>>> Notes the patch lacks runtime testing
>>>>>
>>>>> I don't have the hardware setup to test odd-address writes at the
>>>>> moment. The fix is based on code analysis. I have tested patch 2/2
>>>>> (dirmap fallback) on hardware.
>>>> I'm hesitant - because like I said, if there is really a bug - it
>>>> would have never worked correctly, since day 1. But yeah, I've also
>>>> read the datasheet and it clearly states that the byte write will
>>>> clear the write enable latch.
>>>>
>>>
>>> i can confirm both patches fix real issues, i have similiar fixes
>>> on a kernel tree i always wanted to clean up and upstream. Diffs
>>> based on 6.6.127:
>>>
>>> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
>>> index 197d2c1101ed5..eaa50561ede2c 100644
>>> --- a/drivers/mtd/spi-nor/sst.c
>>> +++ b/drivers/mtd/spi-nor/sst.c
>>> @@ -155,6 +155,13 @@ static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>>                  if (ret)
>>>                          goto out;
>>>
>>> +               ret = spi_nor_write_enable(nor);
>>> +               if (ret)
>>> +                       goto out;
>>> +               ret = spi_nor_wait_till_ready(nor);
>>> +               if (ret)
>>> +                       goto out;
>>> +
>>>                  to++;
>>>                  actual++;
>>>          }
>>>
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 1b0c6770c14e4..646bfb2e91a65 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -276,7 +276,7 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
>>>          if (spi_nor_spimem_bounce(nor, &op))
>>>                  memcpy(nor->bouncebuf, buf, op.data.nbytes);
>>>
>>> -       if (nor->dirmap.wdesc) {
>>> +       if (nor->dirmap.wdesc && nor->program_opcode != SPINOR_OP_AAI_WP) {
>> Why is this better? This removes the use of dirmap for all flashes other
>> than SST.
>
> i claim the opposite down below? That patch 2 of the posted patch series
> looks better to me. Sorry if that was unclear.

Oh, then I misunderstood.

Please review and test the v4, it will help land those fixes.

[...]

-- 
Regards,
Pratyush Yadav

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-03-13 13:39 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260220094236.28-1-sanjaikumarvs@gmail.com>
2026-02-20  9:42 ` [PATCH v2 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence Sanjaikumar V S
2026-02-23  8:18   ` Michael Walle
2026-02-23  9:17     ` Sanjaikumar V S
2026-02-23  9:29       ` Michael Walle
2026-03-06 22:36         ` Hendrik Donner
2026-03-13 11:46           ` Pratyush Yadav
2026-03-13 12:50             ` Hendrik Donner
2026-03-13 13:39               ` Pratyush Yadav
2026-02-20  9:42 ` [PATCH v2 2/2] mtd: spi-nor: core: Fix AAI mode when dirmap is not available Sanjaikumar V S
2026-02-23 10:17 ` [PATCH v3 0/2] mtd: spi-nor: Fix SST AAI write mode Sanjaikumar V S
2026-02-23 10:17   ` [PATCH v3 1/2] mtd: spi-nor: sst: Fix write enable before AAI sequence Sanjaikumar V S
2026-02-23 10:17   ` [PATCH v3 2/2] mtd: spi-nor: core: Fix AAI mode when dirmap is not available Sanjaikumar V S

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox