[PATCH 0/4] spi: spi-mem/mtd: spinand: Prevent SPI NAND continuous reads on am65/am62

[PATCH 0/4] spi: spi-mem/mtd: spinand: Prevent SPI NAND continuous reads on am65/am62

[Miquel Raynal]

TI errata i2351 explains there is a problem with CS handling, SPI NOR
are immune to the problem, but the CS being deasserted spuriously when
there is DMA arbitration on long accesses (every 1023 bytes), SPI NAND
continuous reads cannot be leveraged.

Link: https://www.ti.com/lit/er/sprz544c/sprz544c.pdf

I created a 2-page read setup for testing all variants available on a
W35N chip wired to this controller. I reliably observed all variants to
always (in my tests) report correct data, except the 8D-8D-8D
variant *when setting an extended number of dummy cycles* (>= 12, so 24
dummy bytes). In this case, I got the first 6144 bytes correct (over
8192), the rest being full of ones (0xFF), indicating the CS has likely
been deasserted there. This is not exactly 6 x 1023 bytes (?), but
close.

This series shall be applied on top of the SPI NAND continuous read
series that I am also carrying, but ideally not too far in the future
because there are Winbond (continuous read capable) NAND chips mounted
on TI platforms AM62 based which could make use of that new feature and
expose the issue described above.

I am expecting feedback on:
- the approach: is a controller capability flag relevant? if not, what
  else could be?
- the naming: is the name of the flag explicit enough/relevant?

Mark, as agreed for the continuous read series, I will offer to you an
immutable tag with the spi-mem patches, if you agree I can include the
spi-mem patch from this series as well (once the discussion on the
approach/naming settled).

To: Mark Brown <broonie@kernel.org>
To: Richard Weinberger <richard@nod.at>
To: Vignesh Raghavendra <vigneshr@ti.com>
Cc: Michael Walle <mwalle@kernel.org>
Cc: Takahiro Kuwano <takahiro.kuwano@infineon.com>
Cc: Pratyush Yadav <pratyush@kernel.org>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Steam Lin <STLin2@winbond.com>
Cc: Santhosh Kumar K <s-k6@ti.com>
Cc: linux-spi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mtd@lists.infradead.org

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
Miquel Raynal (4):
      spi: spi-mem: Add a no_cs_assertion capability
      mtd: spinand: Make sure continuous read is always disabled during probe
      mtd: spinand: Prevent continuous reads on some controllers
      spi: cadence-qspi: Prevent SPI NAND continuous reads

 drivers/mtd/nand/spi/core.c       | 12 +++++++-----
 drivers/spi/spi-cadence-quadspi.c |  8 ++++++++
 include/linux/spi/spi-mem.h       |  6 +++++-
 3 files changed, 20 insertions(+), 6 deletions(-)
---
base-commit: 559f264e403e4d58d56a17595c60a1de011c5e20
change-id: 20260318-winbond-v7-0-rc1-cadence-cont-read-bfa0d7258496
prerequisite-message-id: <20260318104750.581596-1-miquel.raynal@bootlin.com>
prerequisite-patch-id: e403edd76450cac2c2bca0e261e20654445bc9ac
prerequisite-message-id: <20260325170450.1118324-1-miquel.raynal@bootlin.com>
prerequisite-patch-id: bab6ebb9d753ab004f259843daebf3e13c838c85
prerequisite-change-id: 20251204-winbond-v6-18-rc1-cont-read-664791ddb263:v2
prerequisite-patch-id: 23bdb7fca293bc75c7a071352c791984e3e10819
prerequisite-patch-id: 51b67ce47dc7dd35e658a80933d3e0537deb1710
prerequisite-patch-id: 1d0ea4a7e4adf9f01c1673c25c1bf30599d8c99d
prerequisite-patch-id: 632c50a86db5d2c71a554787bdb3c587bb392627
prerequisite-patch-id: 487ce86af2f5e5e477b03790b8c3d96cfe3d13d7
prerequisite-patch-id: 5abac789e99db720c4ef1b5d0794ec96caeb418f
prerequisite-patch-id: 8ab6698328813504dfd8055548b888938809ee09
prerequisite-patch-id: d65ae506d7df43532be29e189baf5d0d52150bd9
prerequisite-patch-id: c050d153bd677e8d9d424109569cb1a38c07a109
prerequisite-patch-id: b207d31ea2d2bc3318696e0563fd4d9a85d0389d
prerequisite-patch-id: c268dcd6b16b440b1d82388d0ecfc982d36e1c59

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

[PATCH 1/4] spi: spi-mem: Add a no_cs_assertion capability

[Miquel Raynal]

Some controllers are 'smart', and that's a problem.

For instance, the Cadence quadspi controller is capable of deasserting
the CS automatically whenever a too long period of time without any data
to transfer elapses.

This 'feature' combined with a loaded interconnect with arbitration, a
"long" transfer may be split into smaller DMA transfers. In this case
the controller may allow itself to deassert the CS between chunks.

Deasserting the CS stops any ongoing continuous read. Reasserting it
later to continue the reading will only result in the host getting
garbage.

In this case, the host controller driver has no control over the CS
state, so we cannot reliably enable continuous reads. Flag this
limitation through a spi-mem controller capability.

The inversion in the flag name (starting with 'no_') is voluntary, in
order to avoid the need to set this flag in all controller drivers. Only
the broken controllers shall set this bit, the default being that the
controller masters its CS fully.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
I am open to suggestions regarding the naming of this flag.
---
 include/linux/spi/spi-mem.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index bd2a73d46980..de153719a08e 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -373,7 +373,10 @@ struct spi_controller_mem_ops {
  * @swap16: Supports swapping bytes on a 16 bit boundary when configured in
  *	    Octal DTR
  * @per_op_freq: Supports per operation frequency switching
- * @secondary_op_tmpl: Supports leveraging a secondary memory operation template
+ * @no_cs_assertion: The controller may automatically deassert the CS if there
+ *                   is a pause in the transfer (eg. internal bus contention or
+ *                   DMA arbitration on an interconnect). Features such as NAND
+ *                   continuous reads shall not be leveraged.
  */
 struct spi_controller_mem_caps {
 	bool dtr;
@@ -381,6 +384,7 @@ struct spi_controller_mem_caps {
 	bool swap16;
 	bool per_op_freq;
 	bool secondary_op_tmpl;
+	bool no_cs_assertion;
 };
 
 #define spi_mem_controller_is_capable(ctlr, cap)	\

-- 
2.51.1

[PATCH 2/4] mtd: spinand: Make sure continuous read is always disabled during probe

[Miquel Raynal]

Recent changes made sure whenever we were using continuous reads, we
would first start by disabling the feature to ensure a well proven and
stable probe sequence. For development purposes, it might also matter to
make sure we always disable continuous reads at first, in case the ECC
configuration would change. Doing this "automatically" will become even
more relevant when we add extra controller flags to prevent continuous
reads at all.

Ensure we disable continuous reads if the feature is available on the
chip, regardless of whether it will be used or not.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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 45c3afb9cceb..dbe2c463fe01 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -953,11 +953,7 @@ static void spinand_cont_read_init(struct spinand_device *spinand)
 	enum nand_ecc_engine_type engine_type = nand->ecc.ctx.conf.engine_type;
 
 	/* OOBs cannot be retrieved so external/on-host ECC engine won't work */
-	if (spinand->set_cont_read &&
-	    (engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE ||
-	     engine_type == NAND_ECC_ENGINE_TYPE_NONE)) {
-		spinand->cont_read_possible = true;
-
+	if (spinand->set_cont_read) {
 		/*
 		 * Ensure continuous read is disabled on probe.
 		 * Some devices retain this state across soft reset,
@@ -965,6 +961,10 @@ static void spinand_cont_read_init(struct spinand_device *spinand)
 		 * in false positive returns from spinand_isbad().
 		 */
 		spinand_cont_read_enable(spinand, false);
+
+		if (engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE ||
+		    engine_type == NAND_ECC_ENGINE_TYPE_NONE)
+			spinand->cont_read_possible = true;
 	}
 }
 

-- 
2.51.1

[PATCH 3/4] mtd: spinand: Prevent continuous reads on some controllers

[Miquel Raynal]

Some controllers do not have full control over the CS line state and may
deassert it under certain conditions in the middle of a (long)
transfer.

Continuous reads are stopped with a CS deassert, hence both features
cannot live together.

Whenever a controller flags that it cannot maintain the CS state
reliably, disable continuous reads entirely.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
SPI NOR reads are not affected, hence only a solution in SPI NAND is
needed.
---
 drivers/mtd/nand/spi/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index dbe2c463fe01..82f4dd441541 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -951,6 +951,7 @@ static void spinand_cont_read_init(struct spinand_device *spinand)
 {
 	struct nand_device *nand = spinand_to_nand(spinand);
 	enum nand_ecc_engine_type engine_type = nand->ecc.ctx.conf.engine_type;
+	struct spi_controller *ctlr = spinand->spimem->spi->controller;
 
 	/* OOBs cannot be retrieved so external/on-host ECC engine won't work */
 	if (spinand->set_cont_read) {
@@ -962,8 +963,9 @@ static void spinand_cont_read_init(struct spinand_device *spinand)
 		 */
 		spinand_cont_read_enable(spinand, false);
 
-		if (engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE ||
-		    engine_type == NAND_ECC_ENGINE_TYPE_NONE)
+		if ((engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE ||
+		     engine_type == NAND_ECC_ENGINE_TYPE_NONE) &&
+		    !spi_mem_controller_is_capable(ctlr, no_cs_assertion))
 			spinand->cont_read_possible = true;
 	}
 }

-- 
2.51.1

[PATCH 4/4] spi: cadence-qspi: Prevent SPI NAND continuous reads

[Miquel Raynal]

TI AM62Ax errata i2351, entitled "OSPI: Direct Access Controller (DAC)
does not support Continuous Read mode with NAND Flash", explains that
the CS can be deasserted almost at any time during a transfer (basically
there is an interconnect arbitration every 1023 byte). This is an
expected internal behaviour of the controller, but this leads to
spurious CS deasserts. These are totally forbidden during SPI NAND
continuous read transfers, because they indicate an end of transfer and
the continuous read is then stopped on the flash side.

I initially tried to query the flash type and geometry to decide whether
to apply a spi message size limit (there is a spi helper for that) but
we cannot reliably get up to the MTD structure to discriminate if it is
a NOR or a NAND we are playing with (it is not relevant to limit SPI NOR
transfers, they are not affected). If we actually take this path, what
limitation shall we enforce? The errata mentions 1023B, this is super
low, less than a typical page size (about 2k or 4k, for most of them),
so this is not usable. On my side, I only observed this problem on a
2-page read in octal DTR mode with more than 12 dummy cycles. The
writesize summed with the oobsize could be a relevant boundary, but it
is still quite arbitrary. Hence, I opted for implementing a controller
capability flag, which then is used to decide whether the SPI NAND
continuous read feature can be enabled.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
I do not know if all flavours of this controller have the same
limitation, or whether it is integration specific. As I only found
mention of this errata for the AM62 processor, I opted for limiting the
flag to a single compatible.
---
 drivers/spi/spi-cadence-quadspi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 649ff55333f0..9f9b3013aa5d 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1740,6 +1740,12 @@ static const struct spi_controller_mem_caps cqspi_mem_caps = {
 	.per_op_freq = true,
 };
 
+static const struct spi_controller_mem_caps cqspi_am654_mem_caps = {
+	.dtr = true,
+	.per_op_freq = true,
+	.no_cs_assertion = true,
+};
+
 static int cqspi_setup_flash(struct cqspi_st *cqspi)
 {
 	struct platform_device *pdev = cqspi->pdev;
@@ -1797,6 +1803,8 @@ static int cqspi_probe(struct platform_device *pdev)
 	host->mode_bits = SPI_RX_QUAD | SPI_RX_DUAL;
 	host->mem_ops = &cqspi_mem_ops;
 	host->mem_caps = &cqspi_mem_caps;
+	if (of_device_is_compatible(pdev->dev.of_node, "ti,am654-ospi"))
+		host->mem_caps = &cqspi_am654_mem_caps;
 
 	cqspi = spi_controller_get_devdata(host);
 	if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi"))

-- 
2.51.1

Re: [PATCH 0/4] spi: spi-mem/mtd: spinand: Prevent SPI NAND continuous reads on am65/am62

[Miquel Raynal]

Hi Mark,

> Mark, as agreed for the continuous read series, I will offer to you an
> immutable tag with the spi-mem patches, if you agree I can include the
> spi-mem patch from this series as well (once the discussion on the
> approach/naming settled).

As discussed, I am preparing an immutable branch with the spi-mem
patches of the "Winbond SPI NAND continuous read" series.

This series is related but I wanted to double check with you: can I take
the spi-mem patch and include it in that immutable branch which I will
share with you for inclusion in the spi tree?

Thanks,
Miquèl