[PATCH] mtd: spi-nor: do not issue the legacy RDCR (0x35) in 8D-8D-8D mode

[PATCH] mtd: spi-nor: do not issue the legacy RDCR (0x35) in 8D-8D-8D mode

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

spi_nor_init_default_params() unconditionally sets opcodes.read_sr2 to
the legacy RDCR opcode (0x35) for every flash. This default is meant for
the (x)STR quad-enable case (QE bit in SR2 bit1, read back via 0x35);
it was never intended for 8D-8D-8D. During spi_nor_init(),
spi_nor_cache_sr_lock_bits() reads SR1 and SR2 to seed the debugfs lock
bit cache, and this runs after the flash has already been switched to
its runtime protocol - including 8D-8D-8D (OPI/DOPI) mode.

RDCR (0x35) is the single-byte "read SR2/configuration register" opcode
and is only meaningful in the (x)STR protocols. In 8D-8D-8D mode a flash
exposes its second status/configuration register through a
vendor-specific indirect register space, or does not expose it at all,
so issuing a bare 0x35 is an unsupported command. The flash drives no
data, the controller RX FIFO stays empty and the register read times
out. On i.MX FlexSPI this is seen as a -ETIMEDOUT and a controller
WARN() during probe, e.g. with a Micron MT35xU (2c 5b 1b) and a
Macronix (c2 ..) octal flash, killing the spi-nor probe.

This is not a per-vendor quirk: it affects any octal-DTR flash that
never got a dedicated 8D-capable SR2 read opcode installed by its
manufacturer driver. Guard the read in the core instead of patching
each vendor: in spi_nor_read_sr2(), when the current register protocol
is 8D-8D-8D and read_sr2 is still the default RDCR, return -EOPNOTSUPP
rather than sending 0x35. spi_nor_read_sr2_careful() (the swp.c early
init user) now routes through spi_nor_read_sr2() and treats -EOPNOTSUPP
as "no SR2", using the QE-based guess so the lock bit caching does not
fail on OPI/DOPI flashes.

A manufacturer driver that provides a real 8D-capable SR2 read opcode
(anything other than the default RDCR) is unaffected by this guard.

Assisted-by: LLM
Fixes: b7b63475903c ("mtd: spi-nor: Create a local SR cache")
Fixes: 63489002d397 ("mtd: spi-nor: Refactor Read Status/Write Status support")
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mtd/spi-nor/core.c | 15 +++++++++++++++
 drivers/mtd/spi-nor/swp.c  | 21 ++++++++++++++-------
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 90524c58602e1c27cd83951ab0cdd3669a2ebfd6..9e176777fae35d815db53fda0056e40ad986a208 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -844,6 +844,21 @@ int spi_nor_read_sr2(struct spi_nor *nor, u8 *sr2)
 	if (!params->opcodes.read_sr2)
 		return -EINVAL;
 
+	/*
+	 * RDCR (0x35) is the legacy single-byte "read SR2/configuration
+	 * register" opcode and is only meaningful in the (x)STR protocols. In
+	 * 8D-8D-8D (OPI/DOPI) mode flashes expose their second status or
+	 * configuration register through a vendor-specific indirect register
+	 * space, or do not expose it at all, so issuing a bare 0x35 is an
+	 * unsupported command: the flash drives no data and the controller
+	 * read times out. Unless a manufacturer driver installed a dedicated
+	 * 8D-capable read_sr2 opcode (i.e. something other than the default
+	 * RDCR), skip the SR2 read in 8D mode.
+	 */
+	if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR &&
+	    params->opcodes.read_sr2 == SPINOR_OP_RDCR)
+		return -EOPNOTSUPP;
+
 	return spi_nor_read_sr_ll(nor, params->opcodes.read_sr2, sr2, 1);
 }
 
diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 7e667e4ca84d5101471e60bc839b2ecc23958294..f8dea179f09930af76b8026773d8b648e620fa81 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -211,19 +211,26 @@ static int spi_nor_read_sr2_careful(struct spi_nor *nor, u8 *sr2)
 	int ret;
 
 	if (params->opcodes.read_sr2) {
-		ret = spi_nor_read_sr_ll(nor, params->opcodes.read_sr2, sr2, 1);
-		if (ret)
+		ret = spi_nor_read_sr2(nor, sr2);
+		/*
+		 * spi_nor_read_sr2() returns -EOPNOTSUPP when the only SR2
+		 * read opcode is the legacy RDCR (0x35), which cannot be
+		 * issued in 8D-8D-8D mode. In that case use the QE-based guess
+		 * below instead, so early init (SR lock bit caching) does not
+		 * fail on OPI/DOPI flashes.
+		 */
+		if (ret != -EOPNOTSUPP)
 			return ret;
-	} else if ((spi_nor_get_protocol_width(nor->read_proto) == 4 ||
-		   spi_nor_get_protocol_width(nor->write_proto) == 4) &&
-		   nor->params->quad_enable) {
+	}
+
+	if ((spi_nor_get_protocol_width(nor->read_proto) == 4 ||
+	     spi_nor_get_protocol_width(nor->write_proto) == 4) &&
+	    nor->params->quad_enable) {
 		/*
 		 * Make sure the QE bit is persistently kept. qe_mask[1] will be
 		 * 0 if the QE bit is in SR1.
 		 */
 		*sr2 = params->qe_mask[1];
-	} else {
-		return 0;
 	}
 
 	return 0;

---
base-commit: ba52b770f89bd2f3771b98edb92c4fde68ca8cc4
change-id: 20260928-spi-nor-fix-eb7ebe22a740

Best regards,
-- 
Haibo Chen <haibo.chen@nxp.com>

Re: [PATCH] mtd: spi-nor: do not issue the legacy RDCR (0x35) in 8D-8D-8D mode

[Miquel Raynal]

Hi Haibo,

> +	/*
> +	 * RDCR (0x35) is the legacy single-byte "read SR2/configuration
> +	 * register" opcode and is only meaningful in the (x)STR protocols. In
> +	 * 8D-8D-8D (OPI/DOPI) mode flashes expose their second status or
> +	 * configuration register through a vendor-specific indirect register
> +	 * space, or do not expose it at all, so issuing a bare 0x35 is an
> +	 * unsupported command: the flash drives no data and the controller
> +	 * read times out. Unless a manufacturer driver installed a dedicated
> +	 * 8D-capable read_sr2 opcode (i.e. something other than the default
> +	 * RDCR), skip the SR2 read in 8D mode.
> +	 */
> +	if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR &&
> +	    params->opcodes.read_sr2 == SPINOR_OP_RDCR)
> +		return -EOPNOTSUPP;

I believe this is not the correct solution. The correct solution would
be to clear opcodes.read_sr2 for ODTR flashes by default (if that's
really a default?) and then handle the read_sr2() error correctly in the
debugfs caching.

Also please tell your LLM not to put huge comments like that to explain
what is already in the commit log.

Thanks,
Miquèl