[PATCH v3 0/6] mtd: rawnand: zynq: Fixes and software ECC support

[PATCH v3 0/6] mtd: rawnand: zynq: Fixes and software ECC support

[Bastien Curutchet]

This driver is deeply broken in many ways. Miquèl has cleaned it up by:
- flagging the most hideous parts (where Micron on-die ECC support is
  hardcoded)
- enabling DT support, eg. for reading the ECC configuration
- fixing software ECC support
- aligning the software ECC OOB layout to the rest of the world and
  allow a working interoperability in Linux.

I then added a patch to fix writes when soft ECC is on.

Signed-off-by: Miquel Raynal (DAVE.eu) <miquel.raynal@bootlin.com>
Signed-off-by: Bastien Curutchet (DAVE.eu) <bastien.curutchet@bootlin.com>
---
Changes in v3:
- Add patch 6 to fix write issues in soft ECC.
- Link to v2: https://lore.kernel.org/r/20260602-dave-upstream-nand-fixes-v2-0-412965ee38bf@bootlin.com

Changes in v2:
- Fix the sponsor: s/DAVE/DAVE.eu/, no other change.
- Link to v1: https://lore.kernel.org/r/20260529-dave-upstream-nand-fixes-v1-0-e5bbddb10676@bootlin.com

---
Bastien Curutchet (DAVE.eu) (1):
      mtd: rawnand: nand_base: Don't overwrite read/write_page_raw() for soft ECC

Miquel Raynal (DAVE.eu) (5):
      mtd: rawnand: zynq: Align the layout structures with the driver namespace
      mtd: rawnand: zynq: Add comments about very bad practices
      mtd: rawnand: zynq: Make sure an of node is attached
      mtd: rawnand: zynq: Do not fail the probe for no reason
      mtd: rawnand: zynq: Fix Software ECC engine support

 drivers/mtd/nand/raw/nand_base.c |   6 +-
 drivers/mtd/nand/raw/zynq_nand.c | 115 ++++++++++-----------------------------
 2 files changed, 32 insertions(+), 89 deletions(-)
---
base-commit: 4433253ecf2041f9362a763bb6cb79960921ac7e
change-id: 20260522-dave-upstream-nand-fixes-60a5336517a2

Best regards,
-- 
Bastien Curutchet (DAVE.eu) <bastien.curutchet@bootlin.com>

[PATCH v3 1/6] mtd: rawnand: zynq: Align the layout structures with the driver namespace

[Bastien Curutchet]

From: "Miquel Raynal (DAVE.eu)" <miquel.raynal@bootlin.com>

nand_oob_16/64 are names already used in the core and it is very
misleading to reuse them here for something that is Zynq specific. It
works because the core structures are not exported, but it is bad
practice, so rename them to prefix them "zynq_" instead.

Signed-off-by: Miquel Raynal (DAVE.eu) <miquel.raynal@bootlin.com>
Signed-off-by: Bastien Curutchet (DAVE.eu) <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/zynq_nand.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/zynq_nand.c b/drivers/mtd/nand/raw/zynq_nand.c
index ca1aff25a52..05b500d0961 100644
--- a/drivers/mtd/nand/raw/zynq_nand.c
+++ b/drivers/mtd/nand/raw/zynq_nand.c
@@ -185,7 +185,7 @@ static const struct zynq_nand_command_format zynq_nand_commands[] = {
 };
 
 /* Define default oob placement schemes for large and small page devices */
-static struct nand_ecclayout nand_oob_16 = {
+static struct nand_ecclayout zynq_oob_16 = {
 	.eccbytes = 3,
 	.eccpos = {0, 1, 2},
 	.oobfree = {
@@ -193,7 +193,7 @@ static struct nand_ecclayout nand_oob_16 = {
 	}
 };
 
-static struct nand_ecclayout nand_oob_64 = {
+static struct nand_ecclayout zynq_oob_64 = {
 	.eccbytes = 12,
 	.eccpos = {
 		   52, 53, 54, 55, 56, 57,
@@ -1258,9 +1258,9 @@ static int zynq_nand_probe(struct udevice *dev)
 		}
 
 		if (mtd->oobsize == 16)
-			nand_chip->ecc.layout = &nand_oob_16;
+			nand_chip->ecc.layout = &zynq_oob_16;
 		else if (mtd->oobsize == 64)
-			nand_chip->ecc.layout = &nand_oob_64;
+			nand_chip->ecc.layout = &zynq_oob_64;
 		else
 			printf("%s: No oob layout found\n", __func__);
 	}

-- 
2.54.0

[PATCH v3 2/6] mtd: rawnand: zynq: Add comments about very bad practices

[Bastien Curutchet]

From: "Miquel Raynal (DAVE.eu)" <miquel.raynal@bootlin.com>

The driver goes through very (very) wrong steps during initialization as
it completely hardcodes support for Micron chips.

Flag these sections as undesirable in case someone ever cares about
fixing this properly, or simply stumbles about these pieces of code and
wonders why this is there.

Signed-off-by: Miquel Raynal (DAVE.eu) <miquel.raynal@bootlin.com>
Signed-off-by: Bastien Curutchet (DAVE.eu) <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/zynq_nand.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/raw/zynq_nand.c b/drivers/mtd/nand/raw/zynq_nand.c
index 05b500d0961..2d07b29ecab 100644
--- a/drivers/mtd/nand/raw/zynq_nand.c
+++ b/drivers/mtd/nand/raw/zynq_nand.c
@@ -203,6 +203,10 @@ static struct nand_ecclayout zynq_oob_64 = {
 	}
 };
 
+/*
+ * FIXME: This is a very (Micron) specific layout, which should not reside at
+ * the spi/ level.
+ */
 static struct nand_ecclayout ondie_nand_oob_64 = {
 	.eccbytes = 32,
 
@@ -1155,6 +1159,10 @@ static int zynq_nand_probe(struct udevice *dev)
 	maf_id = nand_chip->read_byte(mtd);
 	dev_id = nand_chip->read_byte(mtd);
 
+	/*
+	 * FIXME: This is horribly wrong, it checks if Micron chips have on-die
+	 * ECC support enabled. This should be moved away.
+	 */
 	if ((maf_id == 0x2c) && ((dev_id == 0xf1) ||
 				 (dev_id == 0xa1) || (dev_id == 0xb1) ||
 				 (dev_id == 0xaa) || (dev_id == 0xba) ||

-- 
2.54.0

[PATCH v3 3/6] mtd: rawnand: zynq: Make sure an of node is attached

[Bastien Curutchet]

From: "Miquel Raynal (DAVE.eu)" <miquel.raynal@bootlin.com>

If no ofnode is attached by the controller driver, DT parsing does not
happen.

Signed-off-by: Miquel Raynal (DAVE.eu) <miquel.raynal@bootlin.com>
Signed-off-by: Bastien Curutchet (DAVE.eu) <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/zynq_nand.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/raw/zynq_nand.c b/drivers/mtd/nand/raw/zynq_nand.c
index 2d07b29ecab..54c25563072 100644
--- a/drivers/mtd/nand/raw/zynq_nand.c
+++ b/drivers/mtd/nand/raw/zynq_nand.c
@@ -1103,6 +1103,11 @@ static int zynq_nand_probe(struct udevice *dev)
 		return dm_scan_fdt_dev(dev);
 	}
 
+	if (!ofnode_get_child_count(of_nand))
+		nand_set_flash_node(nand_chip, of_nand);
+	else
+		nand_set_flash_node(nand_chip, ofnode_first_subnode(of_nand));
+
 	if (ofnode_read_resource(of_nand, 0, &res)) {
 		printf("Failed to get nand resource\n");
 		return -ENODEV;

-- 
2.54.0

[PATCH v3 4/6] mtd: rawnand: zynq: Do not fail the probe for no reason

[Bastien Curutchet]

From: "Miquel Raynal (DAVE.eu)" <miquel.raynal@bootlin.com>

If there is no indication about 16b buswidth, no need to fail the probe,
just assume it is the (very standard) 8b case.

Signed-off-by: Miquel Raynal (DAVE.eu) <miquel.raynal@bootlin.com>
Signed-off-by: Bastien Curutchet (DAVE.eu) <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/zynq_nand.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/zynq_nand.c b/drivers/mtd/nand/raw/zynq_nand.c
index 54c25563072..be080cbb074 100644
--- a/drivers/mtd/nand/raw/zynq_nand.c
+++ b/drivers/mtd/nand/raw/zynq_nand.c
@@ -1134,12 +1134,6 @@ static int zynq_nand_probe(struct udevice *dev)
 	nand_chip->write_buf = zynq_nand_write_buf;
 
 	is_16bit_bw = zynq_nand_check_is_16bit_bw_flash();
-	if (is_16bit_bw == NAND_BW_UNKNOWN) {
-		printf("%s: Unable detect NAND based on MIO settings\n",
-		       __func__);
-		return -EINVAL;
-	}
-
 	if (is_16bit_bw == NAND_BW_16BIT)
 		nand_chip->options = NAND_BUSWIDTH_16;
 

-- 
2.54.0

[PATCH v3 5/6] mtd: rawnand: zynq: Fix Software ECC engine support

[Bastien Curutchet]

From: "Miquel Raynal (DAVE.eu)" <miquel.raynal@bootlin.com>

Software ECC engine was not partially working but more importantly was
unaligned with the default software layout that has been in place for
decades due to the fact that the driver was enforcing the hardware ECC
layout (for wrong reasons). Fix the software ECC implementation, which
has the side effect of also aligning it with Linux and making the
interoperability work.

Reported-by: Andrea Scian <andrea.scian@dave.eu>
Signed-off-by: Miquel Raynal (DAVE.eu) <miquel.raynal@bootlin.com>
Signed-off-by: Bastien Curutchet (DAVE.eu) <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/zynq_nand.c | 88 +++++-----------------------------------
 1 file changed, 11 insertions(+), 77 deletions(-)

diff --git a/drivers/mtd/nand/raw/zynq_nand.c b/drivers/mtd/nand/raw/zynq_nand.c
index be080cbb074..a697557c722 100644
--- a/drivers/mtd/nand/raw/zynq_nand.c
+++ b/drivers/mtd/nand/raw/zynq_nand.c
@@ -631,34 +631,6 @@ static int zynq_nand_write_page_hwecc(struct mtd_info *mtd,
 	return 0;
 }
 
-/*
- * zynq_nand_write_page_swecc - [REPLACABLE] software ecc based page
- * write function
- * @mtd:	mtd info structure
- * @chip:	nand chip info structure
- * @buf:	data buffer
- * @oob_required: must write chip->oob_poi to OOB
- */
-static int zynq_nand_write_page_swecc(struct mtd_info *mtd,
-	struct nand_chip *chip, const u8 *buf, int oob_required, int page)
-{
-	int i, eccsize = chip->ecc.size;
-	int eccbytes = chip->ecc.bytes;
-	int eccsteps = chip->ecc.steps;
-	u8 *ecc_calc = chip->buffers->ecccalc;
-	const u8 *p = buf;
-	u32 *eccpos = chip->ecc.layout->eccpos;
-
-	/* Software ecc calculation */
-	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
-		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
-
-	for (i = 0; i < chip->ecc.total; i++)
-		chip->oob_poi[eccpos[i]] = ecc_calc[i];
-
-	return chip->ecc.write_page_raw(mtd, chip, buf, 1, page);
-}
-
 /*
  * nand_read_page_hwecc - Hardware ECC based page read function
  * @mtd:	Pointer to the mtd info structure
@@ -736,48 +708,6 @@ static int zynq_nand_read_page_hwecc(struct mtd_info *mtd,
 	return 0;
 }
 
-/*
- * zynq_nand_read_page_swecc - [REPLACABLE] software ecc based page
- * read function
- * @mtd:	mtd info structure
- * @chip:	nand chip info structure
- * @buf:	buffer to store read data
- * @page:	page number to read
- */
-static int zynq_nand_read_page_swecc(struct mtd_info *mtd,
-	struct nand_chip *chip, u8 *buf, int oob_required,  int page)
-{
-	int i, eccsize = chip->ecc.size;
-	int eccbytes = chip->ecc.bytes;
-	int eccsteps = chip->ecc.steps;
-	u8 *p = buf;
-	u8 *ecc_calc = chip->buffers->ecccalc;
-	u8 *ecc_code = chip->buffers->ecccode;
-	u32 *eccpos = chip->ecc.layout->eccpos;
-
-	chip->ecc.read_page_raw(mtd, chip, buf, 1, page);
-
-	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
-		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
-
-	for (i = 0; i < chip->ecc.total; i++)
-		ecc_code[i] = chip->oob_poi[eccpos[i]];
-
-	eccsteps = chip->ecc.steps;
-	p = buf;
-
-	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
-		int stat;
-
-		stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
-		if (stat < 0)
-			mtd->ecc_stats.failed++;
-		else
-			mtd->ecc_stats.corrected += stat;
-	}
-	return 0;
-}
-
 /*
  * zynq_nand_select_chip - Select the flash device
  * @mtd:	Pointer to the mtd_info structure
@@ -1219,6 +1149,16 @@ static int zynq_nand_probe(struct udevice *dev)
 		/* Use the BBT pattern descriptors */
 		nand_chip->bbt_td = &bbt_main_descr;
 		nand_chip->bbt_md = &bbt_mirror_descr;
+	} else if (nand_chip->ecc.mode == NAND_ECC_SOFT ||
+		   nand_chip->ecc.mode == NAND_ECC_SOFT_BCH ||
+		   nand_chip->ecc.mode == NAND_ECC_NONE) {
+		/* Make sure the ECC engine is off */
+		ecc_cfg = readl(&smc->reg->emcr);
+		ecc_cfg &= ~ZYNQ_MEMC_NAND_ECC_MODE_MASK;
+		writel(ecc_cfg, &smc->reg->emcr);
+
+		nand_chip->ecc.read_page_raw = zynq_nand_read_page_raw;
+		nand_chip->ecc.write_page_raw = zynq_nand_write_page_raw;
 	} else {
 		/* Hardware ECC generates 3 bytes ECC code for each 512 bytes */
 		nand_chip->ecc.mode = NAND_ECC_HW;
@@ -1255,13 +1195,7 @@ static int zynq_nand_probe(struct udevice *dev)
 			       &smc->reg->emcr);
 			break;
 		default:
-			nand_chip->ecc.mode = NAND_ECC_SOFT;
-			nand_chip->ecc.calculate = nand_calculate_ecc;
-			nand_chip->ecc.correct = nand_correct_data;
-			nand_chip->ecc.read_page = zynq_nand_read_page_swecc;
-			nand_chip->ecc.write_page = zynq_nand_write_page_swecc;
-			nand_chip->ecc.size = 256;
-			break;
+			return -EINVAL;
 		}
 
 		if (mtd->oobsize == 16)

-- 
2.54.0

[PATCH v3 6/6] mtd: rawnand: nand_base: Don't overwrite read/write_page_raw() for soft ECC

[Bastien Curutchet (DAVE.eu)]

On writes, the Zynq NAND controller needs a bit to be set in the address
register for the last 4 bytes sent. This is correctly handled by the
zynq_nand_write_page_raw() operation but the callback is overwritten
by the core when soft ECC is on. It leads to write failures.

Don't overwrite the write_page_raw callback if it already has a specific
implementation in the NAND driver.
Do the same for the read_page_raw callback to keep symetry between reads
and writes.

Signed-off-by: Bastien Curutchet (DAVE.eu) <bastien.curutchet@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 48e3685d995..d43263d9f96 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5058,8 +5058,10 @@ int nand_scan_tail(struct mtd_info *mtd)
 		ecc->read_page = nand_read_page_swecc;
 		ecc->read_subpage = nand_read_subpage;
 		ecc->write_page = nand_write_page_swecc;
-		ecc->read_page_raw = nand_read_page_raw;
-		ecc->write_page_raw = nand_write_page_raw;
+		if (!ecc->read_page_raw)
+			ecc->read_page_raw = nand_read_page_raw;
+		if (!ecc->write_page_raw)
+			ecc->write_page_raw = nand_write_page_raw;
 		ecc->read_oob = nand_read_oob_std;
 		ecc->write_oob = nand_write_oob_std;
 		if (!ecc->size)

-- 
2.54.0

Re: [PATCH v3 6/6] mtd: rawnand: nand_base: Don't overwrite read/write_page_raw() for soft ECC

[Miquel Raynal]

On 02/07/2026 at 15:08:46 +02, "Bastien Curutchet (DAVE.eu)" <bastien.curutchet@bootlin.com> wrote:

> On writes, the Zynq NAND controller needs a bit to be set in the address
> register for the last 4 bytes sent. This is correctly handled by the
> zynq_nand_write_page_raw() operation but the callback is overwritten
> by the core when soft ECC is on. It leads to write failures.
>
> Don't overwrite the write_page_raw callback if it already has a specific
> implementation in the NAND driver.
> Do the same for the read_page_raw callback to keep symetry between reads
> and writes.
>
> Signed-off-by: Bastien Curutchet (DAVE.eu)
> <bastien.curutchet@bootlin.com>

This solution is aligned with what we have done in Linux as well, thanks
for the fix which seems rather straightforward this way.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl