[PATCH] mtd: spinand: Do not update the QE bit on devices without one

[PATCH] mtd: spinand: Do not update the QE bit on devices without one

[Sagnik Sasmal]

Commit be0b86c648bf ("mtd: spinand: Gather all the bus interface
steps in one single function") moved quad-enable setup into
spinand_configure_chip(). The new code only determines whether quad
mode is needed when SPINAND_HAS_QE_BIT is set, but calls
spinand_init_quad_enable() unconditionally. This clears configuration
register bit 0 on devices without a QE bit.

That bit is not universally a QE bit. On the Winbond W25N02KV it is
H-DIS, which disables the active-low HOLD function. Clearing H-DIS
enables HOLD during single and dual I/O operations. If IO3 is not kept
high, the flash can pause a command and ignore clock and data.

H-DIS is not restored by the FFh reset command, allowing the incorrect
state to survive an SoC warm reboot while the flash remains powered.

Before the refactoring, spinand_init_quad_enable() returned without
touching the configuration register when SPINAND_HAS_QE_BIT was not
set. Restore that behavior.

The regression was reproduced on a Jio JIDU6401 with an MT7986 SoC and
a W25N02KV. With Linux 6.18.44, sysupgrade failed and the following warm
reboot hung in BL2. With this change applied, both sysupgrade and warm
reboot completed successfully.

Fixes: be0b86c648bf ("mtd: spinand: Gather all the bus interface steps in one single function")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Sagnik Sasmal <sagnik@sagnik.me>
---
 drivers/mtd/nand/spi/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 8bf9301f25e7..89e9f5410cfa 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -281,6 +281,9 @@ static int spinand_init_cfg_cache(struct spinand_device *spinand)
 static int spinand_init_quad_enable(struct spinand_device *spinand,
 				    bool enable)
 {
+	if (!(spinand->flags & SPINAND_HAS_QE_BIT))
+		return 0;
+
 	return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE,
 			       enable ? CFG_QUAD_ENABLE : 0);
 }
-- 
2.53.0

Re: [PATCH] mtd: spinand: Do not update the QE bit on devices without one

[Miquel Raynal]

Hello,

On 10/09/2026 at 11:33:23 GMT, Sagnik Sasmal <sagnik@sagnik.me> wrote:

> Commit be0b86c648bf ("mtd: spinand: Gather all the bus interface
> steps in one single function") moved quad-enable setup into
> spinand_configure_chip(). The new code only determines whether quad
> mode is needed when SPINAND_HAS_QE_BIT is set, but calls
> spinand_init_quad_enable() unconditionally. This clears configuration
> register bit 0 on devices without a QE bit.
>
> That bit is not universally a QE bit. On the Winbond W25N02KV it is
> H-DIS, which disables the active-low HOLD function. Clearing H-DIS
> enables HOLD during single and dual I/O operations. If IO3 is not kept
> high, the flash can pause a command and ignore clock and data.
>
> H-DIS is not restored by the FFh reset command, allowing the incorrect
> state to survive an SoC warm reboot while the flash remains powered.
>
> Before the refactoring, spinand_init_quad_enable() returned without
> touching the configuration register when SPINAND_HAS_QE_BIT was not
> set. Restore that behavior.
>
> The regression was reproduced on a Jio JIDU6401 with an MT7986 SoC and
> a W25N02KV. With Linux 6.18.44, sysupgrade failed and the following warm
> reboot hung in BL2. With this change applied, both sysupgrade and warm
> reboot completed successfully.
>
> Fixes: be0b86c648bf ("mtd: spinand: Gather all the bus interface steps in one single function")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Sagnik Sasmal <sagnik@sagnik.me>

Thanks for the report, and the debug and the proposal. Sorry for the
breakage.

While your solution works, I believe it would be cleaner do it this way:

--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1797,11 +1797,11 @@ static int spinand_configure_chip(struct spinand_device *spinand)
                    spinand->ssdr_op_templates.write_cache->data.buswidth == 4 ||
                    spinand->ssdr_op_templates.update_cache->data.buswidth == 4)
                        quad_enable = true;
-       }
 
-       ret = spinand_init_quad_enable(spinand, quad_enable);
-       if (ret)
-               return ret;
+               ret = spinand_init_quad_enable(spinand, quad_enable);
+               if (ret)
+                       return ret;
+       }
 
        if (spinand->configure_chip) {
                ret = spinand->configure_chip(spinand, SSDR);

Do you mind validating this and using something like this in your v2?
This would avoid to check the same flag in two different places. It also
keeps the spinand_init_quad_enable() minimal.

Thanks,
Miquèl

[PATCH v2] mtd: spinand: Do not update the QE bit on devices without one

[Sagnik Sasmal]

Commit be0b86c648bf ("mtd: spinand: Gather all the bus interface
steps in one single function") moved quad-enable setup into
spinand_configure_chip(). The new code only determines whether quad
mode is needed when SPINAND_HAS_QE_BIT is set, but calls
spinand_init_quad_enable() unconditionally. This clears configuration
register bit 0 on devices without a QE bit.

That bit is not universally a QE bit. On the Winbond W25N02KV it is
H-DIS, which disables the active-low HOLD function. Clearing H-DIS
enables HOLD during single and dual I/O operations. If IO3 is not kept
high, the flash can pause a command and ignore clock and data.

H-DIS is not restored by the FFh reset command, allowing the incorrect
state to survive an SoC warm reboot while the flash remains powered.

Before the refactoring, spinand_init_quad_enable() returned without
touching the configuration register on devices without
SPINAND_HAS_QE_BIT. Restore that behavior by only calling the helper
when the flag is set.

The regression was reproduced on a JioRouter JIDU6401 with an MT7986
SoC and a W25N02KV. With Linux 6.18.44, sysupgrade failed and the
following warm reboot hung in BL2. With this change applied, both
sysupgrade and warm reboot completed successfully.

Fixes: be0b86c648bf ("mtd: spinand: Gather all the bus interface steps in one single function")
Cc: stable@vger.kernel.org
Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
Assisted-by: LLM
Signed-off-by: Sagnik Sasmal <sagnik@sagnik.me>
---
I really appreciate the prompt review and your suggestion for a cleaner
approach. Thank you.

Changes in v2:
- Call spinand_init_quad_enable() only inside the
  SPINAND_HAS_QE_BIT condition, as suggested by Miquel.
- Retested on a JioRouter JIDU6401; sysupgrade and the following warm
  reboot completed successfully.

 drivers/mtd/nand/spi/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 43bbe2629..e59581d35 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1813,11 +1813,11 @@ static int spinand_configure_chip(struct spinand_device *spinand)
 		    spinand->ssdr_op_templates.write_cache->data.buswidth == 4 ||
 		    spinand->ssdr_op_templates.update_cache->data.buswidth == 4)
 			quad_enable = true;
-	}
 
-	ret = spinand_init_quad_enable(spinand, quad_enable);
-	if (ret)
-		return ret;
+		ret = spinand_init_quad_enable(spinand, quad_enable);
+		if (ret)
+			return ret;
+	}
 
 	if (spinand->configure_chip) {
 		ret = spinand->configure_chip(spinand, SSDR);
-- 
2.53.0

Re: [PATCH v2] mtd: spinand: Do not update the QE bit on devices without one

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Uninitialized return value in `spinand_configure_chip()` can cause device probe failure.
--

commit 25afeaf20864d4f79fe9a314c125190119930965
Author: Sagnik Sasmal <sagnik@sagnik.me>

mtd: spinand: Do not update the QE bit on devices without one

This patch moves the call to spinand_init_quad_enable() inside the
SPINAND_HAS_QE_BIT condition to avoid clearing configuration register bit 0
on devices without a QE bit.

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 43bbe2629b85b..e59581d3596b7 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1813,11 +1813,11 @@ static int spinand_configure_chip(struct spinand_device *spinand)
>  		    spinand->ssdr_op_templates.write_cache->data.buswidth == 4 ||
>  		    spinand->ssdr_op_templates.update_cache->data.buswidth == 4)
>  			quad_enable = true;
> -	}
>  
> -	ret = spinand_init_quad_enable(spinand, quad_enable);
> -	if (ret)
> -		return ret;
> +		ret = spinand_init_quad_enable(spinand, quad_enable);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (spinand->configure_chip) {
>  		ret = spinand->configure_chip(spinand, SSDR);

[Severity: High]
Does this code leave 'ret' uninitialized if the device lacks both the
SPINAND_HAS_QE_BIT flag and a configure_chip callback?

Looking at the rest of spinand_configure_chip() in drivers/mtd/nand/spi/core.c:

static int spinand_configure_chip(struct spinand_device *spinand)
{
	bool odtr = false, quad_enable = false;
	int ret;

	...

try_ssdr:
	if (spinand->flags & SPINAND_HAS_QE_BIT) {
		...
		ret = spinand_init_quad_enable(spinand, quad_enable);
		if (ret)
			return ret;
	}

	if (spinand->configure_chip) {
		ret = spinand->configure_chip(spinand, SSDR);
		if (ret)
			return ret;
	}

	return ret;
}

If neither condition is met, it appears the function will return an
uninitialized stack variable. If this stack memory holds a non-zero value,
the caller spinand_init_flash() may treat it as a failure and abort device
initialization.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010701a08c36e962-eb595dbe-b80b-44eb-858d-c1033f8a86a1-000000@eu-central-1.amazonses.com?part=1

[PATCH v3] mtd: spinand: Do not update the QE bit on devices without one

[Sagnik Sasmal]

Commit be0b86c648bf ("mtd: spinand: Gather all the bus interface
steps in one single function") moved quad-enable setup into
spinand_configure_chip(). The new code only determines whether quad
mode is needed when SPINAND_HAS_QE_BIT is set, but calls
spinand_init_quad_enable() unconditionally. This clears configuration
register bit 0 on devices without a QE bit.

That bit is not universally a QE bit. On the Winbond W25N02KV it is
H-DIS, which disables the active-low HOLD function. Clearing H-DIS
enables HOLD during single and dual I/O operations. If IO3 is not kept
high, the flash can pause a command and ignore clock and data.

H-DIS is not restored by the FFh reset command, allowing the incorrect
state to survive an SoC warm reboot while the flash remains powered.

Before the refactoring, spinand_init_quad_enable() returned without
touching the configuration register on devices without
SPINAND_HAS_QE_BIT. Restore that behavior by only calling the helper
when the flag is set.

Return zero explicitly once SSDR configuration completes, as all errors
are returned immediately. This avoids returning an uninitialized value
when neither optional configuration step runs.

The regression was reproduced on a JioRouter JIDU6401 with an MT7986
SoC and a W25N02KV. With Linux 6.18.44, sysupgrade failed and the
following warm reboot hung in BL2. With this change applied, both
sysupgrade and warm reboot completed successfully.

Fixes: be0b86c648bf ("mtd: spinand: Gather all the bus interface steps in one single function")
Cc: stable@vger.kernel.org
Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
Assisted-by: LLM
Signed-off-by: Sagnik Sasmal <sagnik@sagnik.me>
---
Changes in v3:
- Return zero explicitly after successful SSDR configuration, avoiding an
  uninitialized return value on devices with neither SPINAND_HAS_QE_BIT
  nor a configure_chip callback, as reported by sashiko-bot.
- Compile-tested for arm64 with W=1.

Changes in v2:
- Call spinand_init_quad_enable() only inside the
  SPINAND_HAS_QE_BIT condition, as suggested by Miquel.
- Retested on a JioRouter JIDU6401; sysupgrade and the following warm
  reboot completed successfully.

 drivers/mtd/nand/spi/core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 43bbe2629..95353777d 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1813,11 +1813,11 @@ static int spinand_configure_chip(struct spinand_device *spinand)
 		    spinand->ssdr_op_templates.write_cache->data.buswidth == 4 ||
 		    spinand->ssdr_op_templates.update_cache->data.buswidth == 4)
 			quad_enable = true;
-	}
 
-	ret = spinand_init_quad_enable(spinand, quad_enable);
-	if (ret)
-		return ret;
+		ret = spinand_init_quad_enable(spinand, quad_enable);
+		if (ret)
+			return ret;
+	}
 
 	if (spinand->configure_chip) {
 		ret = spinand->configure_chip(spinand, SSDR);
@@ -1825,7 +1825,7 @@ static int spinand_configure_chip(struct spinand_device *spinand)
 			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int spinand_init_flash(struct spinand_device *spinand)
-- 
2.53.0