[PATCH 0/4] mtd: nand: mxc_nand: Convert to exec_op

[PATCH 0/4] mtd: nand: mxc_nand: Convert to exec_op

[Sascha Hauer]

This series converts the mxc_nand driver over to exec_op which gets
us rid of a bunch of legacy code. The motivation for this series is
a board that has a NAND chip connected that needs 4bit ECC whereas the
i.MX27 hardware only supports 1Bit Hamming ECC. With this series the
driver now supports software BCH ECC.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Sascha Hauer (4):
      mtd: nand: mxc_nand: separate page read from ecc calc
      mtd: nand: mxc_nand: implement exec_op
      mtd: nand: mxc_nand: support software ECC
      mtd: nand: mxc_nand: disable subpage reads

 drivers/mtd/nand/raw/mxc_nand.c | 568 ++++++++++++++++------------------------
 1 file changed, 219 insertions(+), 349 deletions(-)
---
base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
change-id: 20240417-mtd-nand-mxc-nand-exec-op-38b3b80c4377

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

[PATCH 0/4] mtd: nand: mxc_nand: Convert to exec_op

[Sascha Hauer]

This series converts the mxc_nand driver over to exec_op which gets
us rid of a bunch of legacy code. The motivation for this series is
a board that has a NAND chip connected that needs 4bit ECC whereas the
i.MX27 hardware only supports 1Bit Hamming ECC. With this series the
driver now supports software BCH ECC.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Sascha Hauer (4):
      mtd: nand: mxc_nand: separate page read from ecc calc
      mtd: nand: mxc_nand: implement exec_op
      mtd: nand: mxc_nand: support software ECC
      mtd: nand: mxc_nand: disable subpage reads

 drivers/mtd/nand/raw/mxc_nand.c | 568 ++++++++++++++++------------------------
 1 file changed, 219 insertions(+), 349 deletions(-)
---
base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
change-id: 20240417-mtd-nand-mxc-nand-exec-op-38b3b80c4377

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/4] mtd: nand: mxc_nand: separate page read from ecc calc

[Sascha Hauer]

Our read_page hook currently reads out a page and also counts and
returns the number of bitflips. In upcoming exec_op conversion we'll
need to read the page data in exec_op, but the bitflip information
will be needed in mxc_nand_read_page(). To ease exec_op conversion
separate the page read out from the bitflip evaluation.

For the v2/v3 controllers we can leave the bitflip information in the
status register for later evaluation. For the v1 controller this is
not possible, because the status register is overwritten with each
subpage read. We therefore store the bitflip information in the private
data.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 140 ++++++++++++++++++++++++----------------
 1 file changed, 86 insertions(+), 54 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index 003008355b3c2..3fe0b471f4a2d 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -20,6 +20,7 @@
 #include <linux/irq.h>
 #include <linux/completion.h>
 #include <linux/of.h>
+#include <linux/bitfield.h>
 
 #define DRIVER_NAME "mxc_nand"
 
@@ -47,6 +48,8 @@
 #define NFC_V1_V2_CONFIG1		(host->regs + 0x1a)
 #define NFC_V1_V2_CONFIG2		(host->regs + 0x1c)
 
+#define NFC_V1_V2_ECC_STATUS_RESULT_ERM GENMASK(3, 2)
+
 #define NFC_V2_CONFIG1_ECC_MODE_4	(1 << 0)
 #define NFC_V1_V2_CONFIG1_SP_EN		(1 << 2)
 #define NFC_V1_V2_CONFIG1_ECC_EN	(1 << 3)
@@ -132,7 +135,7 @@ struct mxc_nand_devtype_data {
 	uint16_t (*get_dev_status)(struct mxc_nand_host *);
 	int (*check_int)(struct mxc_nand_host *);
 	void (*irq_control)(struct mxc_nand_host *, int);
-	u32 (*get_ecc_status)(struct mxc_nand_host *);
+	u32 (*get_ecc_status)(struct nand_chip *);
 	const struct mtd_ooblayout_ops *ooblayout;
 	void (*select_chip)(struct nand_chip *chip, int cs);
 	int (*setup_interface)(struct nand_chip *chip, int csline,
@@ -175,6 +178,7 @@ struct mxc_nand_host {
 	int			eccsize;
 	int			used_oobsize;
 	int			active_cs;
+	unsigned int		ecc_stats_v1;
 
 	struct completion	op_completion;
 
@@ -406,19 +410,81 @@ static void irq_control(struct mxc_nand_host *host, int activate)
 	}
 }
 
-static u32 get_ecc_status_v1(struct mxc_nand_host *host)
+static u32 get_ecc_status_v1(struct nand_chip *chip)
 {
-	return readw(NFC_V1_V2_ECC_STATUS_RESULT);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	unsigned int ecc_stats, max_bitflips = 0;
+	int no_subpages, i;
+
+	no_subpages = mtd->writesize >> 9;
+
+	ecc_stats = host->ecc_stats_v1;
+
+	for (i = 0; i < no_subpages; i++) {
+		switch (ecc_stats & 0x3) {
+		case 0:
+		default:
+			break;
+		case 1:
+			mtd->ecc_stats.corrected++;
+			max_bitflips = 1;
+			break;
+		case 2:
+			mtd->ecc_stats.failed++;
+			break;
+		}
+
+		ecc_stats >>= 2;
+	}
+
+	return max_bitflips;
 }
 
-static u32 get_ecc_status_v2(struct mxc_nand_host *host)
+static u32 get_ecc_status_v2_v3(struct nand_chip *chip, unsigned int ecc_stat)
 {
-	return readl(NFC_V1_V2_ECC_STATUS_RESULT);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	u8 ecc_bit_mask, err_limit;
+	unsigned int max_bitflips = 0;
+	int no_subpages, err;
+
+	ecc_bit_mask = (host->eccsize == 4) ? 0x7 : 0xf;
+	err_limit = (host->eccsize == 4) ? 0x4 : 0x8;
+
+	no_subpages = mtd->writesize >> 9;
+
+	do {
+		err = ecc_stat & ecc_bit_mask;
+		if (err > err_limit) {
+			mtd->ecc_stats.failed++;
+		} else {
+			mtd->ecc_stats.corrected += err;
+			max_bitflips = max_t(unsigned int, max_bitflips, err);
+		}
+
+		ecc_stat >>= 4;
+	} while (--no_subpages);
+
+	return max_bitflips;
 }
 
-static u32 get_ecc_status_v3(struct mxc_nand_host *host)
+static u32 get_ecc_status_v2(struct nand_chip *chip)
 {
-	return readl(NFC_V3_ECC_STATUS_RESULT);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+
+	u32 ecc_stat = readl(NFC_V1_V2_ECC_STATUS_RESULT);
+
+	return get_ecc_status_v2_v3(chip, ecc_stat);
+}
+
+static u32 get_ecc_status_v3(struct nand_chip *chip)
+{
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+
+	u32 ecc_stat = readl(NFC_V3_ECC_STATUS_RESULT);
+
+	return get_ecc_status_v2_v3(chip, ecc_stat);
 }
 
 static irqreturn_t mxc_nfc_irq(int irq, void *dev_id)
@@ -712,9 +778,9 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	unsigned int bitflips_corrected = 0;
 	int no_subpages;
 	int i;
+	unsigned int ecc_stats = 0;
 
 	host->devtype_data->enable_hwecc(chip, ecc);
 
@@ -727,8 +793,6 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 	no_subpages = mtd->writesize >> 9;
 
 	for (i = 0; i < no_subpages; i++) {
-		uint16_t ecc_stats;
-
 		/* NANDFC buffer 0 is used for page read/write */
 		writew((host->active_cs << 4) | i, NFC_V1_V2_BUF_ADDR);
 
@@ -737,32 +801,18 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 		/* Wait for operation to complete */
 		wait_op_done(host, true);
 
-		ecc_stats = get_ecc_status_v1(host);
-
-		ecc_stats >>= 2;
-
-		if (buf && ecc) {
-			switch (ecc_stats & 0x3) {
-			case 0:
-			default:
-				break;
-			case 1:
-				mtd->ecc_stats.corrected++;
-				bitflips_corrected = 1;
-				break;
-			case 2:
-				mtd->ecc_stats.failed++;
-				break;
-			}
-		}
+		ecc_stats |= FIELD_GET(NFC_V1_V2_ECC_STATUS_RESULT_ERM,
+				       readw(NFC_V1_V2_ECC_STATUS_RESULT)) << i * 2;
 	}
 
+	host->ecc_stats_v1 = ecc_stats;
+
 	if (buf)
 		memcpy32_fromio(buf, host->main_area0, mtd->writesize);
 	if (oob)
 		copy_spare(mtd, true, oob);
 
-	return bitflips_corrected;
+	return 0;
 }
 
 static int mxc_nand_read_page_v2_v3(struct nand_chip *chip, void *buf,
@@ -770,10 +820,6 @@ static int mxc_nand_read_page_v2_v3(struct nand_chip *chip, void *buf,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	unsigned int max_bitflips = 0;
-	u32 ecc_stat, err;
-	int no_subpages;
-	u8 ecc_bit_mask, err_limit;
 
 	host->devtype_data->enable_hwecc(chip, ecc);
 
@@ -791,26 +837,7 @@ static int mxc_nand_read_page_v2_v3(struct nand_chip *chip, void *buf,
 	if (oob)
 		copy_spare(mtd, true, oob);
 
-	ecc_bit_mask = (host->eccsize == 4) ? 0x7 : 0xf;
-	err_limit = (host->eccsize == 4) ? 0x4 : 0x8;
-
-	no_subpages = mtd->writesize >> 9;
-
-	ecc_stat = host->devtype_data->get_ecc_status(host);
-
-	do {
-		err = ecc_stat & ecc_bit_mask;
-		if (err > err_limit) {
-			mtd->ecc_stats.failed++;
-		} else {
-			mtd->ecc_stats.corrected += err;
-			max_bitflips = max_t(unsigned int, max_bitflips, err);
-		}
-
-		ecc_stat >>= 4;
-	} while (--no_subpages);
-
-	return max_bitflips;
+	return 0;
 }
 
 static int mxc_nand_read_page(struct nand_chip *chip, uint8_t *buf,
@@ -818,13 +845,18 @@ static int mxc_nand_read_page(struct nand_chip *chip, uint8_t *buf,
 {
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
 	void *oob_buf;
+	int ret;
 
 	if (oob_required)
 		oob_buf = chip->oob_poi;
 	else
 		oob_buf = NULL;
 
-	return host->devtype_data->read_page(chip, buf, oob_buf, 1, page);
+	ret = host->devtype_data->read_page(chip, buf, oob_buf, 1, page);
+	if (ret)
+		return ret;
+
+	return host->devtype_data->get_ecc_status(chip);
 }
 
 static int mxc_nand_read_page_raw(struct nand_chip *chip, uint8_t *buf,

-- 
2.39.2

[PATCH 1/4] mtd: nand: mxc_nand: separate page read from ecc calc

[Sascha Hauer]

Our read_page hook currently reads out a page and also counts and
returns the number of bitflips. In upcoming exec_op conversion we'll
need to read the page data in exec_op, but the bitflip information
will be needed in mxc_nand_read_page(). To ease exec_op conversion
separate the page read out from the bitflip evaluation.

For the v2/v3 controllers we can leave the bitflip information in the
status register for later evaluation. For the v1 controller this is
not possible, because the status register is overwritten with each
subpage read. We therefore store the bitflip information in the private
data.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 140 ++++++++++++++++++++++++----------------
 1 file changed, 86 insertions(+), 54 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index 003008355b3c2..3fe0b471f4a2d 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -20,6 +20,7 @@
 #include <linux/irq.h>
 #include <linux/completion.h>
 #include <linux/of.h>
+#include <linux/bitfield.h>
 
 #define DRIVER_NAME "mxc_nand"
 
@@ -47,6 +48,8 @@
 #define NFC_V1_V2_CONFIG1		(host->regs + 0x1a)
 #define NFC_V1_V2_CONFIG2		(host->regs + 0x1c)
 
+#define NFC_V1_V2_ECC_STATUS_RESULT_ERM GENMASK(3, 2)
+
 #define NFC_V2_CONFIG1_ECC_MODE_4	(1 << 0)
 #define NFC_V1_V2_CONFIG1_SP_EN		(1 << 2)
 #define NFC_V1_V2_CONFIG1_ECC_EN	(1 << 3)
@@ -132,7 +135,7 @@ struct mxc_nand_devtype_data {
 	uint16_t (*get_dev_status)(struct mxc_nand_host *);
 	int (*check_int)(struct mxc_nand_host *);
 	void (*irq_control)(struct mxc_nand_host *, int);
-	u32 (*get_ecc_status)(struct mxc_nand_host *);
+	u32 (*get_ecc_status)(struct nand_chip *);
 	const struct mtd_ooblayout_ops *ooblayout;
 	void (*select_chip)(struct nand_chip *chip, int cs);
 	int (*setup_interface)(struct nand_chip *chip, int csline,
@@ -175,6 +178,7 @@ struct mxc_nand_host {
 	int			eccsize;
 	int			used_oobsize;
 	int			active_cs;
+	unsigned int		ecc_stats_v1;
 
 	struct completion	op_completion;
 
@@ -406,19 +410,81 @@ static void irq_control(struct mxc_nand_host *host, int activate)
 	}
 }
 
-static u32 get_ecc_status_v1(struct mxc_nand_host *host)
+static u32 get_ecc_status_v1(struct nand_chip *chip)
 {
-	return readw(NFC_V1_V2_ECC_STATUS_RESULT);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	unsigned int ecc_stats, max_bitflips = 0;
+	int no_subpages, i;
+
+	no_subpages = mtd->writesize >> 9;
+
+	ecc_stats = host->ecc_stats_v1;
+
+	for (i = 0; i < no_subpages; i++) {
+		switch (ecc_stats & 0x3) {
+		case 0:
+		default:
+			break;
+		case 1:
+			mtd->ecc_stats.corrected++;
+			max_bitflips = 1;
+			break;
+		case 2:
+			mtd->ecc_stats.failed++;
+			break;
+		}
+
+		ecc_stats >>= 2;
+	}
+
+	return max_bitflips;
 }
 
-static u32 get_ecc_status_v2(struct mxc_nand_host *host)
+static u32 get_ecc_status_v2_v3(struct nand_chip *chip, unsigned int ecc_stat)
 {
-	return readl(NFC_V1_V2_ECC_STATUS_RESULT);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	u8 ecc_bit_mask, err_limit;
+	unsigned int max_bitflips = 0;
+	int no_subpages, err;
+
+	ecc_bit_mask = (host->eccsize == 4) ? 0x7 : 0xf;
+	err_limit = (host->eccsize == 4) ? 0x4 : 0x8;
+
+	no_subpages = mtd->writesize >> 9;
+
+	do {
+		err = ecc_stat & ecc_bit_mask;
+		if (err > err_limit) {
+			mtd->ecc_stats.failed++;
+		} else {
+			mtd->ecc_stats.corrected += err;
+			max_bitflips = max_t(unsigned int, max_bitflips, err);
+		}
+
+		ecc_stat >>= 4;
+	} while (--no_subpages);
+
+	return max_bitflips;
 }
 
-static u32 get_ecc_status_v3(struct mxc_nand_host *host)
+static u32 get_ecc_status_v2(struct nand_chip *chip)
 {
-	return readl(NFC_V3_ECC_STATUS_RESULT);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+
+	u32 ecc_stat = readl(NFC_V1_V2_ECC_STATUS_RESULT);
+
+	return get_ecc_status_v2_v3(chip, ecc_stat);
+}
+
+static u32 get_ecc_status_v3(struct nand_chip *chip)
+{
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+
+	u32 ecc_stat = readl(NFC_V3_ECC_STATUS_RESULT);
+
+	return get_ecc_status_v2_v3(chip, ecc_stat);
 }
 
 static irqreturn_t mxc_nfc_irq(int irq, void *dev_id)
@@ -712,9 +778,9 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	unsigned int bitflips_corrected = 0;
 	int no_subpages;
 	int i;
+	unsigned int ecc_stats = 0;
 
 	host->devtype_data->enable_hwecc(chip, ecc);
 
@@ -727,8 +793,6 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 	no_subpages = mtd->writesize >> 9;
 
 	for (i = 0; i < no_subpages; i++) {
-		uint16_t ecc_stats;
-
 		/* NANDFC buffer 0 is used for page read/write */
 		writew((host->active_cs << 4) | i, NFC_V1_V2_BUF_ADDR);
 
@@ -737,32 +801,18 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 		/* Wait for operation to complete */
 		wait_op_done(host, true);
 
-		ecc_stats = get_ecc_status_v1(host);
-
-		ecc_stats >>= 2;
-
-		if (buf && ecc) {
-			switch (ecc_stats & 0x3) {
-			case 0:
-			default:
-				break;
-			case 1:
-				mtd->ecc_stats.corrected++;
-				bitflips_corrected = 1;
-				break;
-			case 2:
-				mtd->ecc_stats.failed++;
-				break;
-			}
-		}
+		ecc_stats |= FIELD_GET(NFC_V1_V2_ECC_STATUS_RESULT_ERM,
+				       readw(NFC_V1_V2_ECC_STATUS_RESULT)) << i * 2;
 	}
 
+	host->ecc_stats_v1 = ecc_stats;
+
 	if (buf)
 		memcpy32_fromio(buf, host->main_area0, mtd->writesize);
 	if (oob)
 		copy_spare(mtd, true, oob);
 
-	return bitflips_corrected;
+	return 0;
 }
 
 static int mxc_nand_read_page_v2_v3(struct nand_chip *chip, void *buf,
@@ -770,10 +820,6 @@ static int mxc_nand_read_page_v2_v3(struct nand_chip *chip, void *buf,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	unsigned int max_bitflips = 0;
-	u32 ecc_stat, err;
-	int no_subpages;
-	u8 ecc_bit_mask, err_limit;
 
 	host->devtype_data->enable_hwecc(chip, ecc);
 
@@ -791,26 +837,7 @@ static int mxc_nand_read_page_v2_v3(struct nand_chip *chip, void *buf,
 	if (oob)
 		copy_spare(mtd, true, oob);
 
-	ecc_bit_mask = (host->eccsize == 4) ? 0x7 : 0xf;
-	err_limit = (host->eccsize == 4) ? 0x4 : 0x8;
-
-	no_subpages = mtd->writesize >> 9;
-
-	ecc_stat = host->devtype_data->get_ecc_status(host);
-
-	do {
-		err = ecc_stat & ecc_bit_mask;
-		if (err > err_limit) {
-			mtd->ecc_stats.failed++;
-		} else {
-			mtd->ecc_stats.corrected += err;
-			max_bitflips = max_t(unsigned int, max_bitflips, err);
-		}
-
-		ecc_stat >>= 4;
-	} while (--no_subpages);
-
-	return max_bitflips;
+	return 0;
 }
 
 static int mxc_nand_read_page(struct nand_chip *chip, uint8_t *buf,
@@ -818,13 +845,18 @@ static int mxc_nand_read_page(struct nand_chip *chip, uint8_t *buf,
 {
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
 	void *oob_buf;
+	int ret;
 
 	if (oob_required)
 		oob_buf = chip->oob_poi;
 	else
 		oob_buf = NULL;
 
-	return host->devtype_data->read_page(chip, buf, oob_buf, 1, page);
+	ret = host->devtype_data->read_page(chip, buf, oob_buf, 1, page);
+	if (ret)
+		return ret;
+
+	return host->devtype_data->get_ecc_status(chip);
 }
 
 static int mxc_nand_read_page_raw(struct nand_chip *chip, uint8_t *buf,

-- 
2.39.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/4] mtd: nand: mxc_nand: implement exec_op

[Sascha Hauer]

This converts the driver to the more modern exec_op which gets us rid
of a bunch of legacy code. Tested on i.MX27 and i.MX25.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 423 ++++++++++++----------------------------
 1 file changed, 129 insertions(+), 294 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index 3fe0b471f4a2d..fc70c65dea268 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -126,8 +126,7 @@ struct mxc_nand_host;
 
 struct mxc_nand_devtype_data {
 	void (*preset)(struct mtd_info *);
-	int (*read_page)(struct nand_chip *chip, void *buf, void *oob, bool ecc,
-			 int page);
+	int (*read_page)(struct nand_chip *chip);
 	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_page)(struct mtd_info *, unsigned int);
@@ -182,8 +181,7 @@ struct mxc_nand_host {
 
 	struct completion	op_completion;
 
-	uint8_t			*data_buf;
-	unsigned int		buf_start;
+	void			*data_buf;
 
 	const struct mxc_nand_devtype_data *devtype_data;
 };
@@ -285,63 +283,6 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom, void *buf)
 	}
 }
 
-/*
- * MXC NANDFC can only perform full page+spare or spare-only read/write.  When
- * the upper layers perform a read/write buf operation, the saved column address
- * is used to index into the full page. So usually this function is called with
- * column == 0 (unless no column cycle is needed indicated by column == -1)
- */
-static void mxc_do_addr_cycle(struct mtd_info *mtd, int column, int page_addr)
-{
-	struct nand_chip *nand_chip = mtd_to_nand(mtd);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-
-	/* Write out column address, if necessary */
-	if (column != -1) {
-		host->devtype_data->send_addr(host, column & 0xff,
-					      page_addr == -1);
-		if (mtd->writesize > 512)
-			/* another col addr cycle for 2k page */
-			host->devtype_data->send_addr(host,
-						      (column >> 8) & 0xff,
-						      false);
-	}
-
-	/* Write out page address, if necessary */
-	if (page_addr != -1) {
-		/* paddr_0 - p_addr_7 */
-		host->devtype_data->send_addr(host, (page_addr & 0xff), false);
-
-		if (mtd->writesize > 512) {
-			if (mtd->size >= 0x10000000) {
-				/* paddr_8 - paddr_15 */
-				host->devtype_data->send_addr(host,
-						(page_addr >> 8) & 0xff,
-						false);
-				host->devtype_data->send_addr(host,
-						(page_addr >> 16) & 0xff,
-						true);
-			} else
-				/* paddr_8 - paddr_15 */
-				host->devtype_data->send_addr(host,
-						(page_addr >> 8) & 0xff, true);
-		} else {
-			if (nand_chip->options & NAND_ROW_ADDR_3) {
-				/* paddr_8 - paddr_15 */
-				host->devtype_data->send_addr(host,
-						(page_addr >> 8) & 0xff,
-						false);
-				host->devtype_data->send_addr(host,
-						(page_addr >> 16) & 0xff,
-						true);
-			} else
-				/* paddr_8 - paddr_15 */
-				host->devtype_data->send_addr(host,
-						(page_addr >> 8) & 0xff, true);
-		}
-	}
-}
-
 static int check_int_v3(struct mxc_nand_host *host)
 {
 	uint32_t tmp;
@@ -763,18 +704,7 @@ static void mxc_nand_enable_hwecc_v3(struct nand_chip *chip, bool enable)
 	writel(config2, NFC_V3_CONFIG2);
 }
 
-/* This functions is used by upper layer to checks if device is ready */
-static int mxc_nand_dev_ready(struct nand_chip *chip)
-{
-	/*
-	 * NFC handles R/B internally. Therefore, this function
-	 * always returns status as ready.
-	 */
-	return 1;
-}
-
-static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
-				 bool ecc, int page)
+static int mxc_nand_read_page_v1(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
@@ -782,14 +712,6 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 	int i;
 	unsigned int ecc_stats = 0;
 
-	host->devtype_data->enable_hwecc(chip, ecc);
-
-	host->devtype_data->send_cmd(host, NAND_CMD_READ0, false);
-	mxc_do_addr_cycle(mtd, 0, page);
-
-	if (mtd->writesize > 512)
-		host->devtype_data->send_cmd(host, NAND_CMD_READSTART, true);
-
 	no_subpages = mtd->writesize >> 9;
 
 	for (i = 0; i < no_subpages; i++) {
@@ -807,78 +729,72 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 
 	host->ecc_stats_v1 = ecc_stats;
 
-	if (buf)
-		memcpy32_fromio(buf, host->main_area0, mtd->writesize);
-	if (oob)
-		copy_spare(mtd, true, oob);
-
 	return 0;
 }
 
-static int mxc_nand_read_page_v2_v3(struct nand_chip *chip, void *buf,
-				    void *oob, bool ecc, int page)
+static int mxc_nand_read_page_v2_v3(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
 
-	host->devtype_data->enable_hwecc(chip, ecc);
-
-	host->devtype_data->send_cmd(host, NAND_CMD_READ0, false);
-	mxc_do_addr_cycle(mtd, 0, page);
-
-	if (mtd->writesize > 512)
-		host->devtype_data->send_cmd(host,
-				NAND_CMD_READSTART, true);
-
 	host->devtype_data->send_page(mtd, NFC_OUTPUT);
 
-	if (buf)
-		memcpy32_fromio(buf, host->main_area0, mtd->writesize);
-	if (oob)
-		copy_spare(mtd, true, oob);
-
 	return 0;
 }
 
 static int mxc_nand_read_page(struct nand_chip *chip, uint8_t *buf,
 			      int oob_required, int page)
 {
+	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	void *oob_buf;
 	int ret;
 
-	if (oob_required)
-		oob_buf = chip->oob_poi;
-	else
-		oob_buf = NULL;
+	host->devtype_data->enable_hwecc(chip, true);
 
-	ret = host->devtype_data->read_page(chip, buf, oob_buf, 1, page);
+	ret = nand_read_page_op(chip, page, 0, buf, mtd->writesize);
 	if (ret)
 		return ret;
 
+	if (oob_required)
+		copy_spare(mtd, true, chip->oob_poi);
+
 	return host->devtype_data->get_ecc_status(chip);
 }
 
 static int mxc_nand_read_page_raw(struct nand_chip *chip, uint8_t *buf,
 				  int oob_required, int page)
 {
+	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	void *oob_buf;
+	int ret;
+
+	host->devtype_data->enable_hwecc(chip, false);
+
+	ret = nand_read_page_op(chip, page, 0, buf, mtd->writesize);
+	if (ret)
+		return ret;
 
 	if (oob_required)
-		oob_buf = chip->oob_poi;
-	else
-		oob_buf = NULL;
+		copy_spare(mtd, true, chip->oob_poi);
 
-	return host->devtype_data->read_page(chip, buf, oob_buf, 0, page);
+	return 0;
 }
 
 static int mxc_nand_read_oob(struct nand_chip *chip, int page)
 {
+	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	int ret;
+
+	host->devtype_data->enable_hwecc(chip, false);
+
+	ret = nand_read_page_op(chip, page, 0, host->data_buf, mtd->writesize);
+	if (ret)
+		return ret;
 
-	return host->devtype_data->read_page(chip, NULL, chip->oob_poi, 0,
-					     page);
+	copy_spare(mtd, true, chip->oob_poi);
+
+	return 0;
 }
 
 static int mxc_nand_write_page(struct nand_chip *chip, const uint8_t *buf,
@@ -889,17 +805,7 @@ static int mxc_nand_write_page(struct nand_chip *chip, const uint8_t *buf,
 
 	host->devtype_data->enable_hwecc(chip, ecc);
 
-	host->devtype_data->send_cmd(host, NAND_CMD_SEQIN, false);
-	mxc_do_addr_cycle(mtd, 0, page);
-
-	memcpy32_toio(host->main_area0, buf, mtd->writesize);
-	copy_spare(mtd, false, chip->oob_poi);
-
-	host->devtype_data->send_page(mtd, NFC_INPUT);
-	host->devtype_data->send_cmd(host, NAND_CMD_PAGEPROG, true);
-	mxc_do_addr_cycle(mtd, 0, page);
-
-	return 0;
+	return nand_prog_page_op(chip, page, 0, buf, mtd->writesize);
 }
 
 static int mxc_nand_write_page_ecc(struct nand_chip *chip, const uint8_t *buf,
@@ -924,66 +830,6 @@ static int mxc_nand_write_oob(struct nand_chip *chip, int page)
 	return mxc_nand_write_page(chip, host->data_buf, false, page);
 }
 
-static u_char mxc_nand_read_byte(struct nand_chip *nand_chip)
-{
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-	uint8_t ret;
-
-	/* Check for status request */
-	if (host->status_request)
-		return host->devtype_data->get_dev_status(host) & 0xFF;
-
-	if (nand_chip->options & NAND_BUSWIDTH_16) {
-		/* only take the lower byte of each word */
-		ret = *(uint16_t *)(host->data_buf + host->buf_start);
-
-		host->buf_start += 2;
-	} else {
-		ret = *(uint8_t *)(host->data_buf + host->buf_start);
-		host->buf_start++;
-	}
-
-	dev_dbg(host->dev, "%s: ret=0x%hhx (start=%u)\n", __func__, ret, host->buf_start);
-	return ret;
-}
-
-/* Write data of length len to buffer buf. The data to be
- * written on NAND Flash is first copied to RAMbuffer. After the Data Input
- * Operation by the NFC, the data is written to NAND Flash */
-static void mxc_nand_write_buf(struct nand_chip *nand_chip, const u_char *buf,
-			       int len)
-{
-	struct mtd_info *mtd = nand_to_mtd(nand_chip);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-	u16 col = host->buf_start;
-	int n = mtd->oobsize + mtd->writesize - col;
-
-	n = min(n, len);
-
-	memcpy(host->data_buf + col, buf, n);
-
-	host->buf_start += n;
-}
-
-/* Read the data buffer from the NAND Flash. To read the data from NAND
- * Flash first the data output cycle is initiated by the NFC, which copies
- * the data to RAMbuffer. This data of length len is then copied to buffer buf.
- */
-static void mxc_nand_read_buf(struct nand_chip *nand_chip, u_char *buf,
-			      int len)
-{
-	struct mtd_info *mtd = nand_to_mtd(nand_chip);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-	u16 col = host->buf_start;
-	int n = mtd->oobsize + mtd->writesize - col;
-
-	n = min(n, len);
-
-	memcpy(buf, host->data_buf + col, n);
-
-	host->buf_start += n;
-}
-
 /* This function is used by upper layer for select and
  * deselect of the NAND chip */
 static void mxc_nand_select_chip_v1_v3(struct nand_chip *nand_chip, int chip)
@@ -1360,107 +1206,6 @@ static void preset_v3(struct mtd_info *mtd)
 	writel(0, NFC_V3_DELAY_LINE);
 }
 
-/* Used by the upper layer to write command to NAND Flash for
- * different operations to be carried out on NAND Flash */
-static void mxc_nand_command(struct nand_chip *nand_chip, unsigned command,
-			     int column, int page_addr)
-{
-	struct mtd_info *mtd = nand_to_mtd(nand_chip);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-
-	dev_dbg(host->dev, "mxc_nand_command (cmd = 0x%x, col = 0x%x, page = 0x%x)\n",
-	      command, column, page_addr);
-
-	/* Reset command state information */
-	host->status_request = false;
-
-	/* Command pre-processing step */
-	switch (command) {
-	case NAND_CMD_RESET:
-		host->devtype_data->preset(mtd);
-		host->devtype_data->send_cmd(host, command, false);
-		break;
-
-	case NAND_CMD_STATUS:
-		host->buf_start = 0;
-		host->status_request = true;
-
-		host->devtype_data->send_cmd(host, command, true);
-		WARN_ONCE(column != -1 || page_addr != -1,
-			  "Unexpected column/row value (cmd=%u, col=%d, row=%d)\n",
-			  command, column, page_addr);
-		mxc_do_addr_cycle(mtd, column, page_addr);
-		break;
-
-	case NAND_CMD_READID:
-		host->devtype_data->send_cmd(host, command, true);
-		mxc_do_addr_cycle(mtd, column, page_addr);
-		host->devtype_data->send_read_id(host);
-		host->buf_start = 0;
-		break;
-
-	case NAND_CMD_ERASE1:
-	case NAND_CMD_ERASE2:
-		host->devtype_data->send_cmd(host, command, false);
-		WARN_ONCE(column != -1,
-			  "Unexpected column value (cmd=%u, col=%d)\n",
-			  command, column);
-		mxc_do_addr_cycle(mtd, column, page_addr);
-
-		break;
-	case NAND_CMD_PARAM:
-		host->devtype_data->send_cmd(host, command, false);
-		mxc_do_addr_cycle(mtd, column, page_addr);
-		host->devtype_data->send_page(mtd, NFC_OUTPUT);
-		memcpy32_fromio(host->data_buf, host->main_area0, 512);
-		host->buf_start = 0;
-		break;
-	default:
-		WARN_ONCE(1, "Unimplemented command (cmd=%u)\n",
-			  command);
-		break;
-	}
-}
-
-static int mxc_nand_set_features(struct nand_chip *chip, int addr,
-				 u8 *subfeature_param)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	int i;
-
-	host->buf_start = 0;
-
-	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
-		chip->legacy.write_byte(chip, subfeature_param[i]);
-
-	memcpy32_toio(host->main_area0, host->data_buf, mtd->writesize);
-	host->devtype_data->send_cmd(host, NAND_CMD_SET_FEATURES, false);
-	mxc_do_addr_cycle(mtd, addr, -1);
-	host->devtype_data->send_page(mtd, NFC_INPUT);
-
-	return 0;
-}
-
-static int mxc_nand_get_features(struct nand_chip *chip, int addr,
-				 u8 *subfeature_param)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	int i;
-
-	host->devtype_data->send_cmd(host, NAND_CMD_GET_FEATURES, false);
-	mxc_do_addr_cycle(mtd, addr, -1);
-	host->devtype_data->send_page(mtd, NFC_OUTPUT);
-	memcpy32_fromio(host->data_buf, host->main_area0, 512);
-	host->buf_start = 0;
-
-	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
-		*subfeature_param++ = chip->legacy.read_byte(chip);
-
-	return 0;
-}
-
 /*
  * The generic flash bbt descriptors overlap with our ecc
  * hardware, so define some i.MX specific ones.
@@ -1717,9 +1462,106 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
 	return host->devtype_data->setup_interface(chip, chipnr, conf);
 }
 
+static int mxcnd_exec_op(struct nand_chip *chip,
+			 const struct nand_operation *op,
+			 bool check_only)
+{
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	int i, j, buf_len;
+	void *buf_read = NULL;
+	const void *buf_write = NULL;
+	const struct nand_op_instr *instr;
+	bool readid = false;
+	bool statusreq = false;
+
+	dev_dbg(host->dev, "%s: %d instructions\n", __func__, op->ninstrs);
+
+	for (i = 0; i < op->ninstrs; i++) {
+		instr = &op->instrs[i];
+
+		nand_op_trace("  ", instr);
+
+		switch (instr->type) {
+		case NAND_OP_WAITRDY_INSTR:
+			/*
+			 * NFC handles R/B internally. Therefore, this function
+			 * always returns status as ready.
+			 */
+			break;
+		case NAND_OP_CMD_INSTR:
+			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
+
+			if (instr->ctx.cmd.opcode == NAND_CMD_READID)
+				readid = true;
+			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
+				statusreq = true;
+
+			break;
+		case NAND_OP_ADDR_INSTR:
+			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
+				bool islast = j == instr->ctx.addr.naddrs - 1;
+				host->devtype_data->send_addr(host, instr->ctx.addr.addrs[j], islast);
+			}
+			break;
+		case NAND_OP_DATA_OUT_INSTR:
+			buf_write = instr->ctx.data.buf.out;
+			buf_len = instr->ctx.data.len;
+
+			memcpy32_toio(host->main_area0, buf_write, buf_len);
+			copy_spare(mtd, false, chip->oob_poi);
+
+			host->devtype_data->send_page(mtd, NFC_INPUT);
+
+			break;
+		case NAND_OP_DATA_IN_INSTR:
+
+			buf_read = instr->ctx.data.buf.in;
+			buf_len = instr->ctx.data.len;
+
+			if (readid) {
+				host->devtype_data->send_read_id(host);
+				readid = false;
+
+				memcpy32_fromio(host->data_buf, host->main_area0, buf_len * 2);
+
+				if (chip->options & NAND_BUSWIDTH_16) {
+					u8 *bufr = buf_read;
+					u16 *bufw = host->data_buf;
+					for (j = 0; j < buf_len; j++)
+						bufr[j] = bufw[j];
+				} else {
+					memcpy(buf_read, host->data_buf, buf_len);
+				}
+				break;
+			}
+
+			if (statusreq) {
+				*(u8*)buf_read = host->devtype_data->get_dev_status(host);
+				statusreq = false;
+				break;
+			}
+
+			host->devtype_data->read_page(chip);
+
+			if (IS_ALIGNED(buf_len, 4)) {
+				memcpy32_fromio(buf_read, host->main_area0, buf_len);
+			} else {
+				memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
+				memcpy(buf_read, host->data_buf, buf_len);
+			}
+
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static const struct nand_controller_ops mxcnd_controller_ops = {
 	.attach_chip = mxcnd_attach_chip,
 	.setup_interface = mxcnd_setup_interface,
+	.exec_op = mxcnd_exec_op,
 };
 
 static int mxcnd_probe(struct platform_device *pdev)
@@ -1752,13 +1594,6 @@ static int mxcnd_probe(struct platform_device *pdev)
 
 	nand_set_controller_data(this, host);
 	nand_set_flash_node(this, pdev->dev.of_node);
-	this->legacy.dev_ready = mxc_nand_dev_ready;
-	this->legacy.cmdfunc = mxc_nand_command;
-	this->legacy.read_byte = mxc_nand_read_byte;
-	this->legacy.write_buf = mxc_nand_write_buf;
-	this->legacy.read_buf = mxc_nand_read_buf;
-	this->legacy.set_features = mxc_nand_set_features;
-	this->legacy.get_features = mxc_nand_get_features;
 
 	host->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(host->clk))

-- 
2.39.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/4] mtd: nand: mxc_nand: implement exec_op

[Sascha Hauer]

This converts the driver to the more modern exec_op which gets us rid
of a bunch of legacy code. Tested on i.MX27 and i.MX25.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 423 ++++++++++++----------------------------
 1 file changed, 129 insertions(+), 294 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index 3fe0b471f4a2d..fc70c65dea268 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -126,8 +126,7 @@ struct mxc_nand_host;
 
 struct mxc_nand_devtype_data {
 	void (*preset)(struct mtd_info *);
-	int (*read_page)(struct nand_chip *chip, void *buf, void *oob, bool ecc,
-			 int page);
+	int (*read_page)(struct nand_chip *chip);
 	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_page)(struct mtd_info *, unsigned int);
@@ -182,8 +181,7 @@ struct mxc_nand_host {
 
 	struct completion	op_completion;
 
-	uint8_t			*data_buf;
-	unsigned int		buf_start;
+	void			*data_buf;
 
 	const struct mxc_nand_devtype_data *devtype_data;
 };
@@ -285,63 +283,6 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom, void *buf)
 	}
 }
 
-/*
- * MXC NANDFC can only perform full page+spare or spare-only read/write.  When
- * the upper layers perform a read/write buf operation, the saved column address
- * is used to index into the full page. So usually this function is called with
- * column == 0 (unless no column cycle is needed indicated by column == -1)
- */
-static void mxc_do_addr_cycle(struct mtd_info *mtd, int column, int page_addr)
-{
-	struct nand_chip *nand_chip = mtd_to_nand(mtd);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-
-	/* Write out column address, if necessary */
-	if (column != -1) {
-		host->devtype_data->send_addr(host, column & 0xff,
-					      page_addr == -1);
-		if (mtd->writesize > 512)
-			/* another col addr cycle for 2k page */
-			host->devtype_data->send_addr(host,
-						      (column >> 8) & 0xff,
-						      false);
-	}
-
-	/* Write out page address, if necessary */
-	if (page_addr != -1) {
-		/* paddr_0 - p_addr_7 */
-		host->devtype_data->send_addr(host, (page_addr & 0xff), false);
-
-		if (mtd->writesize > 512) {
-			if (mtd->size >= 0x10000000) {
-				/* paddr_8 - paddr_15 */
-				host->devtype_data->send_addr(host,
-						(page_addr >> 8) & 0xff,
-						false);
-				host->devtype_data->send_addr(host,
-						(page_addr >> 16) & 0xff,
-						true);
-			} else
-				/* paddr_8 - paddr_15 */
-				host->devtype_data->send_addr(host,
-						(page_addr >> 8) & 0xff, true);
-		} else {
-			if (nand_chip->options & NAND_ROW_ADDR_3) {
-				/* paddr_8 - paddr_15 */
-				host->devtype_data->send_addr(host,
-						(page_addr >> 8) & 0xff,
-						false);
-				host->devtype_data->send_addr(host,
-						(page_addr >> 16) & 0xff,
-						true);
-			} else
-				/* paddr_8 - paddr_15 */
-				host->devtype_data->send_addr(host,
-						(page_addr >> 8) & 0xff, true);
-		}
-	}
-}
-
 static int check_int_v3(struct mxc_nand_host *host)
 {
 	uint32_t tmp;
@@ -763,18 +704,7 @@ static void mxc_nand_enable_hwecc_v3(struct nand_chip *chip, bool enable)
 	writel(config2, NFC_V3_CONFIG2);
 }
 
-/* This functions is used by upper layer to checks if device is ready */
-static int mxc_nand_dev_ready(struct nand_chip *chip)
-{
-	/*
-	 * NFC handles R/B internally. Therefore, this function
-	 * always returns status as ready.
-	 */
-	return 1;
-}
-
-static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
-				 bool ecc, int page)
+static int mxc_nand_read_page_v1(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
@@ -782,14 +712,6 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 	int i;
 	unsigned int ecc_stats = 0;
 
-	host->devtype_data->enable_hwecc(chip, ecc);
-
-	host->devtype_data->send_cmd(host, NAND_CMD_READ0, false);
-	mxc_do_addr_cycle(mtd, 0, page);
-
-	if (mtd->writesize > 512)
-		host->devtype_data->send_cmd(host, NAND_CMD_READSTART, true);
-
 	no_subpages = mtd->writesize >> 9;
 
 	for (i = 0; i < no_subpages; i++) {
@@ -807,78 +729,72 @@ static int mxc_nand_read_page_v1(struct nand_chip *chip, void *buf, void *oob,
 
 	host->ecc_stats_v1 = ecc_stats;
 
-	if (buf)
-		memcpy32_fromio(buf, host->main_area0, mtd->writesize);
-	if (oob)
-		copy_spare(mtd, true, oob);
-
 	return 0;
 }
 
-static int mxc_nand_read_page_v2_v3(struct nand_chip *chip, void *buf,
-				    void *oob, bool ecc, int page)
+static int mxc_nand_read_page_v2_v3(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
 
-	host->devtype_data->enable_hwecc(chip, ecc);
-
-	host->devtype_data->send_cmd(host, NAND_CMD_READ0, false);
-	mxc_do_addr_cycle(mtd, 0, page);
-
-	if (mtd->writesize > 512)
-		host->devtype_data->send_cmd(host,
-				NAND_CMD_READSTART, true);
-
 	host->devtype_data->send_page(mtd, NFC_OUTPUT);
 
-	if (buf)
-		memcpy32_fromio(buf, host->main_area0, mtd->writesize);
-	if (oob)
-		copy_spare(mtd, true, oob);
-
 	return 0;
 }
 
 static int mxc_nand_read_page(struct nand_chip *chip, uint8_t *buf,
 			      int oob_required, int page)
 {
+	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	void *oob_buf;
 	int ret;
 
-	if (oob_required)
-		oob_buf = chip->oob_poi;
-	else
-		oob_buf = NULL;
+	host->devtype_data->enable_hwecc(chip, true);
 
-	ret = host->devtype_data->read_page(chip, buf, oob_buf, 1, page);
+	ret = nand_read_page_op(chip, page, 0, buf, mtd->writesize);
 	if (ret)
 		return ret;
 
+	if (oob_required)
+		copy_spare(mtd, true, chip->oob_poi);
+
 	return host->devtype_data->get_ecc_status(chip);
 }
 
 static int mxc_nand_read_page_raw(struct nand_chip *chip, uint8_t *buf,
 				  int oob_required, int page)
 {
+	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	void *oob_buf;
+	int ret;
+
+	host->devtype_data->enable_hwecc(chip, false);
+
+	ret = nand_read_page_op(chip, page, 0, buf, mtd->writesize);
+	if (ret)
+		return ret;
 
 	if (oob_required)
-		oob_buf = chip->oob_poi;
-	else
-		oob_buf = NULL;
+		copy_spare(mtd, true, chip->oob_poi);
 
-	return host->devtype_data->read_page(chip, buf, oob_buf, 0, page);
+	return 0;
 }
 
 static int mxc_nand_read_oob(struct nand_chip *chip, int page)
 {
+	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	int ret;
+
+	host->devtype_data->enable_hwecc(chip, false);
+
+	ret = nand_read_page_op(chip, page, 0, host->data_buf, mtd->writesize);
+	if (ret)
+		return ret;
 
-	return host->devtype_data->read_page(chip, NULL, chip->oob_poi, 0,
-					     page);
+	copy_spare(mtd, true, chip->oob_poi);
+
+	return 0;
 }
 
 static int mxc_nand_write_page(struct nand_chip *chip, const uint8_t *buf,
@@ -889,17 +805,7 @@ static int mxc_nand_write_page(struct nand_chip *chip, const uint8_t *buf,
 
 	host->devtype_data->enable_hwecc(chip, ecc);
 
-	host->devtype_data->send_cmd(host, NAND_CMD_SEQIN, false);
-	mxc_do_addr_cycle(mtd, 0, page);
-
-	memcpy32_toio(host->main_area0, buf, mtd->writesize);
-	copy_spare(mtd, false, chip->oob_poi);
-
-	host->devtype_data->send_page(mtd, NFC_INPUT);
-	host->devtype_data->send_cmd(host, NAND_CMD_PAGEPROG, true);
-	mxc_do_addr_cycle(mtd, 0, page);
-
-	return 0;
+	return nand_prog_page_op(chip, page, 0, buf, mtd->writesize);
 }
 
 static int mxc_nand_write_page_ecc(struct nand_chip *chip, const uint8_t *buf,
@@ -924,66 +830,6 @@ static int mxc_nand_write_oob(struct nand_chip *chip, int page)
 	return mxc_nand_write_page(chip, host->data_buf, false, page);
 }
 
-static u_char mxc_nand_read_byte(struct nand_chip *nand_chip)
-{
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-	uint8_t ret;
-
-	/* Check for status request */
-	if (host->status_request)
-		return host->devtype_data->get_dev_status(host) & 0xFF;
-
-	if (nand_chip->options & NAND_BUSWIDTH_16) {
-		/* only take the lower byte of each word */
-		ret = *(uint16_t *)(host->data_buf + host->buf_start);
-
-		host->buf_start += 2;
-	} else {
-		ret = *(uint8_t *)(host->data_buf + host->buf_start);
-		host->buf_start++;
-	}
-
-	dev_dbg(host->dev, "%s: ret=0x%hhx (start=%u)\n", __func__, ret, host->buf_start);
-	return ret;
-}
-
-/* Write data of length len to buffer buf. The data to be
- * written on NAND Flash is first copied to RAMbuffer. After the Data Input
- * Operation by the NFC, the data is written to NAND Flash */
-static void mxc_nand_write_buf(struct nand_chip *nand_chip, const u_char *buf,
-			       int len)
-{
-	struct mtd_info *mtd = nand_to_mtd(nand_chip);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-	u16 col = host->buf_start;
-	int n = mtd->oobsize + mtd->writesize - col;
-
-	n = min(n, len);
-
-	memcpy(host->data_buf + col, buf, n);
-
-	host->buf_start += n;
-}
-
-/* Read the data buffer from the NAND Flash. To read the data from NAND
- * Flash first the data output cycle is initiated by the NFC, which copies
- * the data to RAMbuffer. This data of length len is then copied to buffer buf.
- */
-static void mxc_nand_read_buf(struct nand_chip *nand_chip, u_char *buf,
-			      int len)
-{
-	struct mtd_info *mtd = nand_to_mtd(nand_chip);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-	u16 col = host->buf_start;
-	int n = mtd->oobsize + mtd->writesize - col;
-
-	n = min(n, len);
-
-	memcpy(buf, host->data_buf + col, n);
-
-	host->buf_start += n;
-}
-
 /* This function is used by upper layer for select and
  * deselect of the NAND chip */
 static void mxc_nand_select_chip_v1_v3(struct nand_chip *nand_chip, int chip)
@@ -1360,107 +1206,6 @@ static void preset_v3(struct mtd_info *mtd)
 	writel(0, NFC_V3_DELAY_LINE);
 }
 
-/* Used by the upper layer to write command to NAND Flash for
- * different operations to be carried out on NAND Flash */
-static void mxc_nand_command(struct nand_chip *nand_chip, unsigned command,
-			     int column, int page_addr)
-{
-	struct mtd_info *mtd = nand_to_mtd(nand_chip);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
-
-	dev_dbg(host->dev, "mxc_nand_command (cmd = 0x%x, col = 0x%x, page = 0x%x)\n",
-	      command, column, page_addr);
-
-	/* Reset command state information */
-	host->status_request = false;
-
-	/* Command pre-processing step */
-	switch (command) {
-	case NAND_CMD_RESET:
-		host->devtype_data->preset(mtd);
-		host->devtype_data->send_cmd(host, command, false);
-		break;
-
-	case NAND_CMD_STATUS:
-		host->buf_start = 0;
-		host->status_request = true;
-
-		host->devtype_data->send_cmd(host, command, true);
-		WARN_ONCE(column != -1 || page_addr != -1,
-			  "Unexpected column/row value (cmd=%u, col=%d, row=%d)\n",
-			  command, column, page_addr);
-		mxc_do_addr_cycle(mtd, column, page_addr);
-		break;
-
-	case NAND_CMD_READID:
-		host->devtype_data->send_cmd(host, command, true);
-		mxc_do_addr_cycle(mtd, column, page_addr);
-		host->devtype_data->send_read_id(host);
-		host->buf_start = 0;
-		break;
-
-	case NAND_CMD_ERASE1:
-	case NAND_CMD_ERASE2:
-		host->devtype_data->send_cmd(host, command, false);
-		WARN_ONCE(column != -1,
-			  "Unexpected column value (cmd=%u, col=%d)\n",
-			  command, column);
-		mxc_do_addr_cycle(mtd, column, page_addr);
-
-		break;
-	case NAND_CMD_PARAM:
-		host->devtype_data->send_cmd(host, command, false);
-		mxc_do_addr_cycle(mtd, column, page_addr);
-		host->devtype_data->send_page(mtd, NFC_OUTPUT);
-		memcpy32_fromio(host->data_buf, host->main_area0, 512);
-		host->buf_start = 0;
-		break;
-	default:
-		WARN_ONCE(1, "Unimplemented command (cmd=%u)\n",
-			  command);
-		break;
-	}
-}
-
-static int mxc_nand_set_features(struct nand_chip *chip, int addr,
-				 u8 *subfeature_param)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	int i;
-
-	host->buf_start = 0;
-
-	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
-		chip->legacy.write_byte(chip, subfeature_param[i]);
-
-	memcpy32_toio(host->main_area0, host->data_buf, mtd->writesize);
-	host->devtype_data->send_cmd(host, NAND_CMD_SET_FEATURES, false);
-	mxc_do_addr_cycle(mtd, addr, -1);
-	host->devtype_data->send_page(mtd, NFC_INPUT);
-
-	return 0;
-}
-
-static int mxc_nand_get_features(struct nand_chip *chip, int addr,
-				 u8 *subfeature_param)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct mxc_nand_host *host = nand_get_controller_data(chip);
-	int i;
-
-	host->devtype_data->send_cmd(host, NAND_CMD_GET_FEATURES, false);
-	mxc_do_addr_cycle(mtd, addr, -1);
-	host->devtype_data->send_page(mtd, NFC_OUTPUT);
-	memcpy32_fromio(host->data_buf, host->main_area0, 512);
-	host->buf_start = 0;
-
-	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
-		*subfeature_param++ = chip->legacy.read_byte(chip);
-
-	return 0;
-}
-
 /*
  * The generic flash bbt descriptors overlap with our ecc
  * hardware, so define some i.MX specific ones.
@@ -1717,9 +1462,106 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
 	return host->devtype_data->setup_interface(chip, chipnr, conf);
 }
 
+static int mxcnd_exec_op(struct nand_chip *chip,
+			 const struct nand_operation *op,
+			 bool check_only)
+{
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	int i, j, buf_len;
+	void *buf_read = NULL;
+	const void *buf_write = NULL;
+	const struct nand_op_instr *instr;
+	bool readid = false;
+	bool statusreq = false;
+
+	dev_dbg(host->dev, "%s: %d instructions\n", __func__, op->ninstrs);
+
+	for (i = 0; i < op->ninstrs; i++) {
+		instr = &op->instrs[i];
+
+		nand_op_trace("  ", instr);
+
+		switch (instr->type) {
+		case NAND_OP_WAITRDY_INSTR:
+			/*
+			 * NFC handles R/B internally. Therefore, this function
+			 * always returns status as ready.
+			 */
+			break;
+		case NAND_OP_CMD_INSTR:
+			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
+
+			if (instr->ctx.cmd.opcode == NAND_CMD_READID)
+				readid = true;
+			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
+				statusreq = true;
+
+			break;
+		case NAND_OP_ADDR_INSTR:
+			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
+				bool islast = j == instr->ctx.addr.naddrs - 1;
+				host->devtype_data->send_addr(host, instr->ctx.addr.addrs[j], islast);
+			}
+			break;
+		case NAND_OP_DATA_OUT_INSTR:
+			buf_write = instr->ctx.data.buf.out;
+			buf_len = instr->ctx.data.len;
+
+			memcpy32_toio(host->main_area0, buf_write, buf_len);
+			copy_spare(mtd, false, chip->oob_poi);
+
+			host->devtype_data->send_page(mtd, NFC_INPUT);
+
+			break;
+		case NAND_OP_DATA_IN_INSTR:
+
+			buf_read = instr->ctx.data.buf.in;
+			buf_len = instr->ctx.data.len;
+
+			if (readid) {
+				host->devtype_data->send_read_id(host);
+				readid = false;
+
+				memcpy32_fromio(host->data_buf, host->main_area0, buf_len * 2);
+
+				if (chip->options & NAND_BUSWIDTH_16) {
+					u8 *bufr = buf_read;
+					u16 *bufw = host->data_buf;
+					for (j = 0; j < buf_len; j++)
+						bufr[j] = bufw[j];
+				} else {
+					memcpy(buf_read, host->data_buf, buf_len);
+				}
+				break;
+			}
+
+			if (statusreq) {
+				*(u8*)buf_read = host->devtype_data->get_dev_status(host);
+				statusreq = false;
+				break;
+			}
+
+			host->devtype_data->read_page(chip);
+
+			if (IS_ALIGNED(buf_len, 4)) {
+				memcpy32_fromio(buf_read, host->main_area0, buf_len);
+			} else {
+				memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
+				memcpy(buf_read, host->data_buf, buf_len);
+			}
+
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static const struct nand_controller_ops mxcnd_controller_ops = {
 	.attach_chip = mxcnd_attach_chip,
 	.setup_interface = mxcnd_setup_interface,
+	.exec_op = mxcnd_exec_op,
 };
 
 static int mxcnd_probe(struct platform_device *pdev)
@@ -1752,13 +1594,6 @@ static int mxcnd_probe(struct platform_device *pdev)
 
 	nand_set_controller_data(this, host);
 	nand_set_flash_node(this, pdev->dev.of_node);
-	this->legacy.dev_ready = mxc_nand_dev_ready;
-	this->legacy.cmdfunc = mxc_nand_command;
-	this->legacy.read_byte = mxc_nand_read_byte;
-	this->legacy.write_buf = mxc_nand_write_buf;
-	this->legacy.read_buf = mxc_nand_read_buf;
-	this->legacy.set_features = mxc_nand_set_features;
-	this->legacy.get_features = mxc_nand_get_features;
 
 	host->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(host->clk))

-- 
2.39.2

[PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Sascha Hauer]

To support software ECC we still need the driver provided read_oob,
read_page_raw and write_page_raw ops, so set them unconditionally
no matter which engine_type we use. The OOB layout on the other hand
represents the layout the i.MX ECC hardware uses, so set this only
when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.

With these changes the driver can be used with software BCH ECC which
is useful for NAND chips that require a stronger ECC than the i.MX
hardware supports.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index fc70c65dea268..f44c130dca18d 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
 	chip->ecc.bytes = host->devtype_data->eccbytes;
 	host->eccsize = host->devtype_data->eccsize;
 	chip->ecc.size = 512;
-	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
+
+	chip->ecc.read_oob = mxc_nand_read_oob;
+	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
+	chip->ecc.write_page_raw = mxc_nand_write_page_raw;
 
 	switch (chip->ecc.engine_type) {
 	case NAND_ECC_ENGINE_TYPE_ON_HOST:
+		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
 		chip->ecc.read_page = mxc_nand_read_page;
-		chip->ecc.read_page_raw = mxc_nand_read_page_raw;
-		chip->ecc.read_oob = mxc_nand_read_oob;
 		chip->ecc.write_page = mxc_nand_write_page_ecc;
-		chip->ecc.write_page_raw = mxc_nand_write_page_raw;
 		chip->ecc.write_oob = mxc_nand_write_oob;
 		break;
 

-- 
2.39.2

[PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Sascha Hauer]

To support software ECC we still need the driver provided read_oob,
read_page_raw and write_page_raw ops, so set them unconditionally
no matter which engine_type we use. The OOB layout on the other hand
represents the layout the i.MX ECC hardware uses, so set this only
when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.

With these changes the driver can be used with software BCH ECC which
is useful for NAND chips that require a stronger ECC than the i.MX
hardware supports.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index fc70c65dea268..f44c130dca18d 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
 	chip->ecc.bytes = host->devtype_data->eccbytes;
 	host->eccsize = host->devtype_data->eccsize;
 	chip->ecc.size = 512;
-	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
+
+	chip->ecc.read_oob = mxc_nand_read_oob;
+	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
+	chip->ecc.write_page_raw = mxc_nand_write_page_raw;
 
 	switch (chip->ecc.engine_type) {
 	case NAND_ECC_ENGINE_TYPE_ON_HOST:
+		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
 		chip->ecc.read_page = mxc_nand_read_page;
-		chip->ecc.read_page_raw = mxc_nand_read_page_raw;
-		chip->ecc.read_oob = mxc_nand_read_oob;
 		chip->ecc.write_page = mxc_nand_write_page_ecc;
-		chip->ecc.write_page_raw = mxc_nand_write_page_raw;
 		chip->ecc.write_oob = mxc_nand_write_oob;
 		break;
 

-- 
2.39.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

The NAND core enabled subpage reads when a largepage NAND is used with
SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
clear the flag again.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index f44c130dca18d..19b46210bd194 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
 	if (err)
 		goto escan;
 
+	this->options &= ~NAND_SUBPAGE_READ;
+
 	/* Register the partitions */
 	err = mtd_device_parse_register(mtd, part_probes, NULL, NULL, 0);
 	if (err)

-- 
2.39.2

[PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

The NAND core enabled subpage reads when a largepage NAND is used with
SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
clear the flag again.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/raw/mxc_nand.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index f44c130dca18d..19b46210bd194 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
 	if (err)
 		goto escan;
 
+	this->options &= ~NAND_SUBPAGE_READ;
+
 	/* Register the partitions */
 	err = mtd_device_parse_register(mtd, part_probes, NULL, NULL, 0);
 	if (err)

-- 
2.39.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:
> The NAND core enabled subpage reads when a largepage NAND is used with
> SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> clear the flag again.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> index f44c130dca18d..19b46210bd194 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
>  	if (err)
>  		goto escan;
>  
> +	this->options &= ~NAND_SUBPAGE_READ;
> +

Nah, it doesn't work like this. It turns out the BBT is read using
subpage reads before we can disable them here.

This is the code in nand_scan_tail() we stumble upon:

	/* Large page NAND with SOFT_ECC should support subpage reads */
	switch (ecc->engine_type) {
	case NAND_ECC_ENGINE_TYPE_SOFT:
		if (chip->page_shift > 9)
			chip->options |= NAND_SUBPAGE_READ;
		break;

	default:
		break;
	}

So the code assumes subpage reads are ok when SOFT_ECC is in use, which
in my case is not true. I guess some drivers depend on the
NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
likely not an option.  Any ideas what to do?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:
> The NAND core enabled subpage reads when a largepage NAND is used with
> SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> clear the flag again.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> index f44c130dca18d..19b46210bd194 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
>  	if (err)
>  		goto escan;
>  
> +	this->options &= ~NAND_SUBPAGE_READ;
> +

Nah, it doesn't work like this. It turns out the BBT is read using
subpage reads before we can disable them here.

This is the code in nand_scan_tail() we stumble upon:

	/* Large page NAND with SOFT_ECC should support subpage reads */
	switch (ecc->engine_type) {
	case NAND_ECC_ENGINE_TYPE_SOFT:
		if (chip->page_shift > 9)
			chip->options |= NAND_SUBPAGE_READ;
		break;

	default:
		break;
	}

So the code assumes subpage reads are ok when SOFT_ECC is in use, which
in my case is not true. I guess some drivers depend on the
NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
likely not an option.  Any ideas what to do?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:

> On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:
> > The NAND core enabled subpage reads when a largepage NAND is used with
> > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > clear the flag again.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > index f44c130dca18d..19b46210bd194 100644
> > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> >  	if (err)
> >  		goto escan;
> >  
> > +	this->options &= ~NAND_SUBPAGE_READ;
> > +  
> 
> Nah, it doesn't work like this. It turns out the BBT is read using
> subpage reads before we can disable them here.
>
> This is the code in nand_scan_tail() we stumble upon:
> 
> 	/* Large page NAND with SOFT_ECC should support subpage reads */
> 	switch (ecc->engine_type) {
> 	case NAND_ECC_ENGINE_TYPE_SOFT:
> 		if (chip->page_shift > 9)
> 			chip->options |= NAND_SUBPAGE_READ;
> 		break;
> 
> 	default:
> 		break;
> 	}
> 
> So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> in my case is not true. I guess some drivers depend on the
> NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> likely not an option.  Any ideas what to do?

Can you elaborate why subpage reads are not an option in your
situation? While subpage writes depend on chip capabilities, reads
however should always work: it's just the controller selecting the
column where to start and then reading less data than it could from the
NAND cache. It's a very basic NAND controller feature, and I remember
this was working on eg. an i.MX27.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:

> On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:
> > The NAND core enabled subpage reads when a largepage NAND is used with
> > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > clear the flag again.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > index f44c130dca18d..19b46210bd194 100644
> > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> >  	if (err)
> >  		goto escan;
> >  
> > +	this->options &= ~NAND_SUBPAGE_READ;
> > +  
> 
> Nah, it doesn't work like this. It turns out the BBT is read using
> subpage reads before we can disable them here.
>
> This is the code in nand_scan_tail() we stumble upon:
> 
> 	/* Large page NAND with SOFT_ECC should support subpage reads */
> 	switch (ecc->engine_type) {
> 	case NAND_ECC_ENGINE_TYPE_SOFT:
> 		if (chip->page_shift > 9)
> 			chip->options |= NAND_SUBPAGE_READ;
> 		break;
> 
> 	default:
> 		break;
> 	}
> 
> So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> in my case is not true. I guess some drivers depend on the
> NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> likely not an option.  Any ideas what to do?

Can you elaborate why subpage reads are not an option in your
situation? While subpage writes depend on chip capabilities, reads
however should always work: it's just the controller selecting the
column where to start and then reading less data than it could from the
NAND cache. It's a very basic NAND controller feature, and I remember
this was working on eg. an i.MX27.

Thanks,
Miquèl

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> 
> > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:
> > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > clear the flag again.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > index f44c130dca18d..19b46210bd194 100644
> > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > >  	if (err)
> > >  		goto escan;
> > >  
> > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > +  
> > 
> > Nah, it doesn't work like this. It turns out the BBT is read using
> > subpage reads before we can disable them here.
> >
> > This is the code in nand_scan_tail() we stumble upon:
> > 
> > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > 	switch (ecc->engine_type) {
> > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > 		if (chip->page_shift > 9)
> > 			chip->options |= NAND_SUBPAGE_READ;
> > 		break;
> > 
> > 	default:
> > 		break;
> > 	}
> > 
> > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > in my case is not true. I guess some drivers depend on the
> > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > likely not an option.  Any ideas what to do?
> 
> Can you elaborate why subpage reads are not an option in your
> situation? While subpage writes depend on chip capabilities, reads
> however should always work: it's just the controller selecting the
> column where to start and then reading less data than it could from the
> NAND cache. It's a very basic NAND controller feature, and I remember
> this was working on eg. an i.MX27.

On the i.MX27 reading a full 2k page means triggering one read operation
per 512 bytes in the NAND controller, so it would be possible to read
subpages by triggering only one read operation instead of four in a row.

The newer SoCs like i.MX25 always read a full page with a single read
operation. We could likely read subpages by temporarily configuring the
controller for a 512b page size NAND.

I just realized the real problem comes with reading the OOB data. With
software BCH the NAND layer hardcodes the read_subpage hook to
nand_read_subpage() which uses nand_change_read_column_op() to read the
OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
be implemented in the i.MX NAND driver. Right now the controller indeed
reads some data and then the SRAM buffer really contains part of the
desired OOB data, but also part of the user data.

We might overcome these problems, but I am not sure if it's worth it.
It's ancient hardware that I don't want to put too much effort into and
I doubt that the end result would have a better performance when we need
one operation to read the subpage and another one to read OOB as opposed
to always read full pages (which is only one operation, say one
interrupt latency, for each page read).

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> 
> > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:
> > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > clear the flag again.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > index f44c130dca18d..19b46210bd194 100644
> > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > >  	if (err)
> > >  		goto escan;
> > >  
> > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > +  
> > 
> > Nah, it doesn't work like this. It turns out the BBT is read using
> > subpage reads before we can disable them here.
> >
> > This is the code in nand_scan_tail() we stumble upon:
> > 
> > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > 	switch (ecc->engine_type) {
> > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > 		if (chip->page_shift > 9)
> > 			chip->options |= NAND_SUBPAGE_READ;
> > 		break;
> > 
> > 	default:
> > 		break;
> > 	}
> > 
> > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > in my case is not true. I guess some drivers depend on the
> > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > likely not an option.  Any ideas what to do?
> 
> Can you elaborate why subpage reads are not an option in your
> situation? While subpage writes depend on chip capabilities, reads
> however should always work: it's just the controller selecting the
> column where to start and then reading less data than it could from the
> NAND cache. It's a very basic NAND controller feature, and I remember
> this was working on eg. an i.MX27.

On the i.MX27 reading a full 2k page means triggering one read operation
per 512 bytes in the NAND controller, so it would be possible to read
subpages by triggering only one read operation instead of four in a row.

The newer SoCs like i.MX25 always read a full page with a single read
operation. We could likely read subpages by temporarily configuring the
controller for a 512b page size NAND.

I just realized the real problem comes with reading the OOB data. With
software BCH the NAND layer hardcodes the read_subpage hook to
nand_read_subpage() which uses nand_change_read_column_op() to read the
OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
be implemented in the i.MX NAND driver. Right now the controller indeed
reads some data and then the SRAM buffer really contains part of the
desired OOB data, but also part of the user data.

We might overcome these problems, but I am not sure if it's worth it.
It's ancient hardware that I don't want to put too much effort into and
I doubt that the end result would have a better performance when we need
one operation to read the subpage and another one to read OOB as opposed
to always read full pages (which is only one operation, say one
interrupt latency, for each page read).

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 13:43:15 +0200:

> On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:
> > Hi Sascha,
> > 
> > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> >   
> > > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:  
> > > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > > clear the flag again.
> > > > 
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > ---
> > > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > index f44c130dca18d..19b46210bd194 100644
> > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > > >  	if (err)
> > > >  		goto escan;
> > > >  
> > > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > > +    
> > > 
> > > Nah, it doesn't work like this. It turns out the BBT is read using
> > > subpage reads before we can disable them here.
> > >
> > > This is the code in nand_scan_tail() we stumble upon:
> > > 
> > > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > > 	switch (ecc->engine_type) {
> > > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > > 		if (chip->page_shift > 9)
> > > 			chip->options |= NAND_SUBPAGE_READ;
> > > 		break;
> > > 
> > > 	default:
> > > 		break;
> > > 	}
> > > 
> > > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > > in my case is not true. I guess some drivers depend on the
> > > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > > likely not an option.  Any ideas what to do?  
> > 
> > Can you elaborate why subpage reads are not an option in your
> > situation? While subpage writes depend on chip capabilities, reads
> > however should always work: it's just the controller selecting the
> > column where to start and then reading less data than it could from the
> > NAND cache. It's a very basic NAND controller feature, and I remember
> > this was working on eg. an i.MX27.  
> 
> On the i.MX27 reading a full 2k page means triggering one read operation
> per 512 bytes in the NAND controller, so it would be possible to read
> subpages by triggering only one read operation instead of four in a row.
> 
> The newer SoCs like i.MX25 always read a full page with a single read
> operation. We could likely read subpages by temporarily configuring the
> controller for a 512b page size NAND.
> 
> I just realized the real problem comes with reading the OOB data. With
> software BCH the NAND layer hardcodes the read_subpage hook to
> nand_read_subpage() which uses nand_change_read_column_op() to read the
> OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
> be implemented in the i.MX NAND driver. Right now the controller indeed
> reads some data and then the SRAM buffer really contains part of the
> desired OOB data, but also part of the user data.

NAND_CMD_RNDOUT is impossible to avoid, the controller surely supports
it and the core really need it anyway. Basically reading from a NAND
chip is a matter of:

- asking the chip to "sample" the NAND array and store a full page
  (data+OOB) in its internal SRAM
- waiting for it to happen
- reading from the chip's SRAM, any length, any offset. Of course the
  offset and length must be aligned with the on-host ECC engine when
  there is one.

Supporting this command must be straightforward with ->exec_op(), here
is the pattern:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1454

> We might overcome these problems, but I am not sure if it's worth it.
> It's ancient hardware that I don't want to put too much effort into and
> I doubt that the end result would have a better performance when we need
> one operation to read the subpage and another one to read OOB as opposed
> to always read full pages

We shall definitely avoid doing several read operations, but as
explained above, you can move the internal SRAM pointer at no cost
("read from cache" commands, named "changed_column" in the core), so the
performance penalty is negligible.

> (which is only one operation, say one
> interrupt latency, for each page read).

The mxc_nand.c driver was my first ever NAND controller driver re-write
but unfortunately the quality was too bad for being submitted at that
time. My goal was the same as yours. Quickly after we introduced
->exec_op() and thus my initial re-work was trash. But I think it was
close to this:
https://github.com/miquelraynal/linux/blob/perso/mtd-next/mxc-nand/drivers/mtd/nand/mxc_nand_v2.c
Maybe that can help.

Thanks,
Miquèl

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 13:43:15 +0200:

> On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:
> > Hi Sascha,
> > 
> > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> >   
> > > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:  
> > > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > > clear the flag again.
> > > > 
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > ---
> > > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > index f44c130dca18d..19b46210bd194 100644
> > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > > >  	if (err)
> > > >  		goto escan;
> > > >  
> > > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > > +    
> > > 
> > > Nah, it doesn't work like this. It turns out the BBT is read using
> > > subpage reads before we can disable them here.
> > >
> > > This is the code in nand_scan_tail() we stumble upon:
> > > 
> > > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > > 	switch (ecc->engine_type) {
> > > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > > 		if (chip->page_shift > 9)
> > > 			chip->options |= NAND_SUBPAGE_READ;
> > > 		break;
> > > 
> > > 	default:
> > > 		break;
> > > 	}
> > > 
> > > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > > in my case is not true. I guess some drivers depend on the
> > > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > > likely not an option.  Any ideas what to do?  
> > 
> > Can you elaborate why subpage reads are not an option in your
> > situation? While subpage writes depend on chip capabilities, reads
> > however should always work: it's just the controller selecting the
> > column where to start and then reading less data than it could from the
> > NAND cache. It's a very basic NAND controller feature, and I remember
> > this was working on eg. an i.MX27.  
> 
> On the i.MX27 reading a full 2k page means triggering one read operation
> per 512 bytes in the NAND controller, so it would be possible to read
> subpages by triggering only one read operation instead of four in a row.
> 
> The newer SoCs like i.MX25 always read a full page with a single read
> operation. We could likely read subpages by temporarily configuring the
> controller for a 512b page size NAND.
> 
> I just realized the real problem comes with reading the OOB data. With
> software BCH the NAND layer hardcodes the read_subpage hook to
> nand_read_subpage() which uses nand_change_read_column_op() to read the
> OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
> be implemented in the i.MX NAND driver. Right now the controller indeed
> reads some data and then the SRAM buffer really contains part of the
> desired OOB data, but also part of the user data.

NAND_CMD_RNDOUT is impossible to avoid, the controller surely supports
it and the core really need it anyway. Basically reading from a NAND
chip is a matter of:

- asking the chip to "sample" the NAND array and store a full page
  (data+OOB) in its internal SRAM
- waiting for it to happen
- reading from the chip's SRAM, any length, any offset. Of course the
  offset and length must be aligned with the on-host ECC engine when
  there is one.

Supporting this command must be straightforward with ->exec_op(), here
is the pattern:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1454

> We might overcome these problems, but I am not sure if it's worth it.
> It's ancient hardware that I don't want to put too much effort into and
> I doubt that the end result would have a better performance when we need
> one operation to read the subpage and another one to read OOB as opposed
> to always read full pages

We shall definitely avoid doing several read operations, but as
explained above, you can move the internal SRAM pointer at no cost
("read from cache" commands, named "changed_column" in the core), so the
performance penalty is negligible.

> (which is only one operation, say one
> interrupt latency, for each page read).

The mxc_nand.c driver was my first ever NAND controller driver re-write
but unfortunately the quality was too bad for being submitted at that
time. My goal was the same as yours. Quickly after we introduced
->exec_op() and thus my initial re-work was trash. But I think it was
close to this:
https://github.com/miquelraynal/linux/blob/perso/mtd-next/mxc-nand/drivers/mtd/nand/mxc_nand_v2.c
Maybe that can help.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

On Fri, Apr 19, 2024 at 11:46:57AM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 13:43:15 +0200:
> 
> > On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:
> > > Hi Sascha,
> > > 
> > > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> > >   
> > > > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:  
> > > > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > > > clear the flag again.
> > > > > 
> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > ---
> > > > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > index f44c130dca18d..19b46210bd194 100644
> > > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > > > >  	if (err)
> > > > >  		goto escan;
> > > > >  
> > > > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > > > +    
> > > > 
> > > > Nah, it doesn't work like this. It turns out the BBT is read using
> > > > subpage reads before we can disable them here.
> > > >
> > > > This is the code in nand_scan_tail() we stumble upon:
> > > > 
> > > > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > > > 	switch (ecc->engine_type) {
> > > > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > > > 		if (chip->page_shift > 9)
> > > > 			chip->options |= NAND_SUBPAGE_READ;
> > > > 		break;
> > > > 
> > > > 	default:
> > > > 		break;
> > > > 	}
> > > > 
> > > > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > > > in my case is not true. I guess some drivers depend on the
> > > > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > > > likely not an option.  Any ideas what to do?  
> > > 
> > > Can you elaborate why subpage reads are not an option in your
> > > situation? While subpage writes depend on chip capabilities, reads
> > > however should always work: it's just the controller selecting the
> > > column where to start and then reading less data than it could from the
> > > NAND cache. It's a very basic NAND controller feature, and I remember
> > > this was working on eg. an i.MX27.  
> > 
> > On the i.MX27 reading a full 2k page means triggering one read operation
> > per 512 bytes in the NAND controller, so it would be possible to read
> > subpages by triggering only one read operation instead of four in a row.
> > 
> > The newer SoCs like i.MX25 always read a full page with a single read
> > operation. We could likely read subpages by temporarily configuring the
> > controller for a 512b page size NAND.
> > 
> > I just realized the real problem comes with reading the OOB data. With
> > software BCH the NAND layer hardcodes the read_subpage hook to
> > nand_read_subpage() which uses nand_change_read_column_op() to read the
> > OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
> > be implemented in the i.MX NAND driver. Right now the controller indeed
> > reads some data and then the SRAM buffer really contains part of the
> > desired OOB data, but also part of the user data.
> 
> NAND_CMD_RNDOUT is impossible to avoid,

Apparently it has been possible until now. NAND_CMD_RNDOUT has never
been used with this driver and it also doesn't work like expected.

One problem is that the read_page_raw() and write_page_raw() are not
implemented like supposed by the NAND layer. The i.MX NAND controller
uses a syndrome type ECC layout, meaning that the user data and OOB data
is interleaved, so the raw r/w functions should normally pass/expect the
page data in interleaved format. Unfortunately the raw functions are not
implemented like that, instead they detangle the data themselves. This
also means that setting the cursor using NAND_CMD_RNDOUT will not put
the cursor at a meaningful place, as the raw functions are not really
exect/return the raw page data.

This could be fixed, but the raw operations are also exposed to
userspace, so fixing these would mean that we might break some userspace
applications.

The other point is that with using software BCH ecc the NAND layer
requests me to read 7 bytes at offset 0x824. This can't be really
implemented in the i.MX NAND driver. It only allows us to read a full
512 byte subpage, so whenever the NAND layer requests me to read a few
bytes the controller will always transfer 512 bytes from which I then
ignore most of it (and possibly trigger another 512 bytes transfer when
reading the ECC for the next subpage).

I think these issues can all be handled somehow, but this comes at a
rather high price, both in effort and the risk of breaking userspace.
It would be far easier to tell the NAND layer not to do subpage reads.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

On Fri, Apr 19, 2024 at 11:46:57AM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 13:43:15 +0200:
> 
> > On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:
> > > Hi Sascha,
> > > 
> > > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> > >   
> > > > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:  
> > > > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > > > clear the flag again.
> > > > > 
> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > ---
> > > > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > index f44c130dca18d..19b46210bd194 100644
> > > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > > > >  	if (err)
> > > > >  		goto escan;
> > > > >  
> > > > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > > > +    
> > > > 
> > > > Nah, it doesn't work like this. It turns out the BBT is read using
> > > > subpage reads before we can disable them here.
> > > >
> > > > This is the code in nand_scan_tail() we stumble upon:
> > > > 
> > > > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > > > 	switch (ecc->engine_type) {
> > > > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > > > 		if (chip->page_shift > 9)
> > > > 			chip->options |= NAND_SUBPAGE_READ;
> > > > 		break;
> > > > 
> > > > 	default:
> > > > 		break;
> > > > 	}
> > > > 
> > > > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > > > in my case is not true. I guess some drivers depend on the
> > > > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > > > likely not an option.  Any ideas what to do?  
> > > 
> > > Can you elaborate why subpage reads are not an option in your
> > > situation? While subpage writes depend on chip capabilities, reads
> > > however should always work: it's just the controller selecting the
> > > column where to start and then reading less data than it could from the
> > > NAND cache. It's a very basic NAND controller feature, and I remember
> > > this was working on eg. an i.MX27.  
> > 
> > On the i.MX27 reading a full 2k page means triggering one read operation
> > per 512 bytes in the NAND controller, so it would be possible to read
> > subpages by triggering only one read operation instead of four in a row.
> > 
> > The newer SoCs like i.MX25 always read a full page with a single read
> > operation. We could likely read subpages by temporarily configuring the
> > controller for a 512b page size NAND.
> > 
> > I just realized the real problem comes with reading the OOB data. With
> > software BCH the NAND layer hardcodes the read_subpage hook to
> > nand_read_subpage() which uses nand_change_read_column_op() to read the
> > OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
> > be implemented in the i.MX NAND driver. Right now the controller indeed
> > reads some data and then the SRAM buffer really contains part of the
> > desired OOB data, but also part of the user data.
> 
> NAND_CMD_RNDOUT is impossible to avoid,

Apparently it has been possible until now. NAND_CMD_RNDOUT has never
been used with this driver and it also doesn't work like expected.

One problem is that the read_page_raw() and write_page_raw() are not
implemented like supposed by the NAND layer. The i.MX NAND controller
uses a syndrome type ECC layout, meaning that the user data and OOB data
is interleaved, so the raw r/w functions should normally pass/expect the
page data in interleaved format. Unfortunately the raw functions are not
implemented like that, instead they detangle the data themselves. This
also means that setting the cursor using NAND_CMD_RNDOUT will not put
the cursor at a meaningful place, as the raw functions are not really
exect/return the raw page data.

This could be fixed, but the raw operations are also exposed to
userspace, so fixing these would mean that we might break some userspace
applications.

The other point is that with using software BCH ecc the NAND layer
requests me to read 7 bytes at offset 0x824. This can't be really
implemented in the i.MX NAND driver. It only allows us to read a full
512 byte subpage, so whenever the NAND layer requests me to read a few
bytes the controller will always transfer 512 bytes from which I then
ignore most of it (and possibly trigger another 512 bytes transfer when
reading the ECC for the next subpage).

I think these issues can all be handled somehow, but this comes at a
rather high price, both in effort and the risk of breaking userspace.
It would be far easier to tell the NAND layer not to do subpage reads.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/4] mtd: nand: mxc_nand: implement exec_op

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:29 +0200:

> This converts the driver to the more modern exec_op which gets us rid
> of a bunch of legacy code. Tested on i.MX27 and i.MX25.

Thanks a lot for this contribution!

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

...

>  static int mxc_nand_read_page_raw(struct nand_chip *chip, uint8_t *buf,
>  				  int oob_required, int page)
>  {
> +	struct mtd_info *mtd = nand_to_mtd(chip);
>  	struct mxc_nand_host *host = nand_get_controller_data(chip);
> -	void *oob_buf;
> +	int ret;
> +
> +	host->devtype_data->enable_hwecc(chip, false);

In general the expected logic would be to keep the ECC engine disabled
and just enable/use it/disable in the page helpers.

> +
> +	ret = nand_read_page_op(chip, page, 0, buf, mtd->writesize);
> +	if (ret)
> +		return ret;
>  
>  	if (oob_required)
> -		oob_buf = chip->oob_poi;
> -	else
> -		oob_buf = NULL;
> +		copy_spare(mtd, true, chip->oob_poi);
>  
> -	return host->devtype_data->read_page(chip, buf, oob_buf, 0, page);
> +	return 0;
>  }
>  

...

>  static int mxcnd_probe(struct platform_device *pdev)
> @@ -1752,13 +1594,6 @@ static int mxcnd_probe(struct platform_device *pdev)
>  
>  	nand_set_controller_data(this, host);
>  	nand_set_flash_node(this, pdev->dev.of_node);
> -	this->legacy.dev_ready = mxc_nand_dev_ready;
> -	this->legacy.cmdfunc = mxc_nand_command;
> -	this->legacy.read_byte = mxc_nand_read_byte;
> -	this->legacy.write_buf = mxc_nand_write_buf;
> -	this->legacy.read_buf = mxc_nand_read_buf;
> -	this->legacy.set_features = mxc_nand_set_features;
> -	this->legacy.get_features = mxc_nand_get_features;

Very nice diff overall. I'm fine with the first two patches, do you
mind if I merge 1 and 2 for now? We need to discuss further the subpage
issue.

As mentioned above, I would welcome a patch setting the HW ECC engine to
false by default and only enabling it in the page helpers (when using
the on-host ECC engine of course). This would be a good minor step,
with or without software ECC support.

Thanks,
Miquèl

Re: [PATCH 2/4] mtd: nand: mxc_nand: implement exec_op

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:29 +0200:

> This converts the driver to the more modern exec_op which gets us rid
> of a bunch of legacy code. Tested on i.MX27 and i.MX25.

Thanks a lot for this contribution!

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

...

>  static int mxc_nand_read_page_raw(struct nand_chip *chip, uint8_t *buf,
>  				  int oob_required, int page)
>  {
> +	struct mtd_info *mtd = nand_to_mtd(chip);
>  	struct mxc_nand_host *host = nand_get_controller_data(chip);
> -	void *oob_buf;
> +	int ret;
> +
> +	host->devtype_data->enable_hwecc(chip, false);

In general the expected logic would be to keep the ECC engine disabled
and just enable/use it/disable in the page helpers.

> +
> +	ret = nand_read_page_op(chip, page, 0, buf, mtd->writesize);
> +	if (ret)
> +		return ret;
>  
>  	if (oob_required)
> -		oob_buf = chip->oob_poi;
> -	else
> -		oob_buf = NULL;
> +		copy_spare(mtd, true, chip->oob_poi);
>  
> -	return host->devtype_data->read_page(chip, buf, oob_buf, 0, page);
> +	return 0;
>  }
>  

...

>  static int mxcnd_probe(struct platform_device *pdev)
> @@ -1752,13 +1594,6 @@ static int mxcnd_probe(struct platform_device *pdev)
>  
>  	nand_set_controller_data(this, host);
>  	nand_set_flash_node(this, pdev->dev.of_node);
> -	this->legacy.dev_ready = mxc_nand_dev_ready;
> -	this->legacy.cmdfunc = mxc_nand_command;
> -	this->legacy.read_byte = mxc_nand_read_byte;
> -	this->legacy.write_buf = mxc_nand_write_buf;
> -	this->legacy.read_buf = mxc_nand_read_buf;
> -	this->legacy.set_features = mxc_nand_set_features;
> -	this->legacy.get_features = mxc_nand_get_features;

Very nice diff overall. I'm fine with the first two patches, do you
mind if I merge 1 and 2 for now? We need to discuss further the subpage
issue.

As mentioned above, I would welcome a patch setting the HW ECC engine to
false by default and only enabling it in the page helpers (when using
the on-host ECC engine of course). This would be a good minor step,
with or without software ECC support.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:

> To support software ECC we still need the driver provided read_oob,
> read_page_raw and write_page_raw ops, so set them unconditionally
> no matter which engine_type we use. The OOB layout on the other hand
> represents the layout the i.MX ECC hardware uses, so set this only
> when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> 
> With these changes the driver can be used with software BCH ECC which
> is useful for NAND chips that require a stronger ECC than the i.MX
> hardware supports.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> index fc70c65dea268..f44c130dca18d 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
>  	chip->ecc.bytes = host->devtype_data->eccbytes;
>  	host->eccsize = host->devtype_data->eccsize;
>  	chip->ecc.size = 512;
> -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> +
> +	chip->ecc.read_oob = mxc_nand_read_oob;
> +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;
>  
>  	switch (chip->ecc.engine_type) {
>  	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> +		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
>  		chip->ecc.read_page = mxc_nand_read_page;
> -		chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> -		chip->ecc.read_oob = mxc_nand_read_oob;
>  		chip->ecc.write_page = mxc_nand_write_page_ecc;
> -		chip->ecc.write_page_raw = mxc_nand_write_page_raw;
>  		chip->ecc.write_oob = mxc_nand_write_oob;
>  		break;

You also need to disable the ECC engine by default (and then you're
free to use the raw page helpers).

I thought patch 4 was needed for this patch to work, do you confirm?
Otherwise with the little change requested I might also merge this one.

Thanks, Miquèl

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:

> To support software ECC we still need the driver provided read_oob,
> read_page_raw and write_page_raw ops, so set them unconditionally
> no matter which engine_type we use. The OOB layout on the other hand
> represents the layout the i.MX ECC hardware uses, so set this only
> when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> 
> With these changes the driver can be used with software BCH ECC which
> is useful for NAND chips that require a stronger ECC than the i.MX
> hardware supports.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> index fc70c65dea268..f44c130dca18d 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
>  	chip->ecc.bytes = host->devtype_data->eccbytes;
>  	host->eccsize = host->devtype_data->eccsize;
>  	chip->ecc.size = 512;
> -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> +
> +	chip->ecc.read_oob = mxc_nand_read_oob;
> +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;
>  
>  	switch (chip->ecc.engine_type) {
>  	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> +		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
>  		chip->ecc.read_page = mxc_nand_read_page;
> -		chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> -		chip->ecc.read_oob = mxc_nand_read_oob;
>  		chip->ecc.write_page = mxc_nand_write_page_ecc;
> -		chip->ecc.write_page_raw = mxc_nand_write_page_raw;
>  		chip->ecc.write_oob = mxc_nand_write_oob;
>  		break;

You also need to disable the ECC engine by default (and then you're
free to use the raw page helpers).

I thought patch 4 was needed for this patch to work, do you confirm?
Otherwise with the little change requested I might also merge this one.

Thanks, Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Miquel,

miquel.raynal@bootlin.com wrote on Mon, 6 May 2024 16:05:08 +0200:

> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> 
> > To support software ECC we still need the driver provided read_oob,
> > read_page_raw and write_page_raw ops, so set them unconditionally
> > no matter which engine_type we use. The OOB layout on the other hand
> > represents the layout the i.MX ECC hardware uses, so set this only
> > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > 
> > With these changes the driver can be used with software BCH ECC which
> > is useful for NAND chips that require a stronger ECC than the i.MX
> > hardware supports.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > index fc70c65dea268..f44c130dca18d 100644
> > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> >  	host->eccsize = host->devtype_data->eccsize;
> >  	chip->ecc.size = 512;
> > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > +
> > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;

A second thought on this. Maybe you should consider keeping these for
on-host operations only.

The read/write_page_raw operations are supposed to detangle the data
organization to show a proper [all data][all oob] organization to the
user. But of course if the data is stored differently when using
software ECC, you'll expect the implementation to be different (and the
core provides such helpers, even though in your case they use RNDOUT
which is not yet supported).

> >  
> >  	switch (chip->ecc.engine_type) {
> >  	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> > +		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> >  		chip->ecc.read_page = mxc_nand_read_page;
> > -		chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > -		chip->ecc.read_oob = mxc_nand_read_oob;
> >  		chip->ecc.write_page = mxc_nand_write_page_ecc;
> > -		chip->ecc.write_page_raw = mxc_nand_write_page_raw;
> >  		chip->ecc.write_oob = mxc_nand_write_oob;
> >  		break;  
> 
> You also need to disable the ECC engine by default (and then you're
> free to use the raw page helpers).
> 
> I thought patch 4 was needed for this patch to work, do you confirm?
> Otherwise with the little change requested I might also merge this one.
> 
> Thanks, Miquèl

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Miquel,

miquel.raynal@bootlin.com wrote on Mon, 6 May 2024 16:05:08 +0200:

> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> 
> > To support software ECC we still need the driver provided read_oob,
> > read_page_raw and write_page_raw ops, so set them unconditionally
> > no matter which engine_type we use. The OOB layout on the other hand
> > represents the layout the i.MX ECC hardware uses, so set this only
> > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > 
> > With these changes the driver can be used with software BCH ECC which
> > is useful for NAND chips that require a stronger ECC than the i.MX
> > hardware supports.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > index fc70c65dea268..f44c130dca18d 100644
> > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> >  	host->eccsize = host->devtype_data->eccsize;
> >  	chip->ecc.size = 512;
> > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > +
> > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;

A second thought on this. Maybe you should consider keeping these for
on-host operations only.

The read/write_page_raw operations are supposed to detangle the data
organization to show a proper [all data][all oob] organization to the
user. But of course if the data is stored differently when using
software ECC, you'll expect the implementation to be different (and the
core provides such helpers, even though in your case they use RNDOUT
which is not yet supported).

> >  
> >  	switch (chip->ecc.engine_type) {
> >  	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> > +		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> >  		chip->ecc.read_page = mxc_nand_read_page;
> > -		chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > -		chip->ecc.read_oob = mxc_nand_read_oob;
> >  		chip->ecc.write_page = mxc_nand_write_page_ecc;
> > -		chip->ecc.write_page_raw = mxc_nand_write_page_raw;
> >  		chip->ecc.write_oob = mxc_nand_write_oob;
> >  		break;  
> 
> You also need to disable the ECC engine by default (and then you're
> free to use the raw page helpers).
> 
> I thought patch 4 was needed for this patch to work, do you confirm?
> Otherwise with the little change requested I might also merge this one.
> 
> Thanks, Miquèl


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Mon, 22 Apr 2024 12:53:38 +0200:

> On Fri, Apr 19, 2024 at 11:46:57AM +0200, Miquel Raynal wrote:
> > Hi Sascha,
> > 
> > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 13:43:15 +0200:
> >   
> > > On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:  
> > > > Hi Sascha,
> > > > 
> > > > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> > > >     
> > > > > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:    
> > > > > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > > > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > > > > clear the flag again.
> > > > > > 
> > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > index f44c130dca18d..19b46210bd194 100644
> > > > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > > > > >  	if (err)
> > > > > >  		goto escan;
> > > > > >  
> > > > > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > > > > +      
> > > > > 
> > > > > Nah, it doesn't work like this. It turns out the BBT is read using
> > > > > subpage reads before we can disable them here.
> > > > >
> > > > > This is the code in nand_scan_tail() we stumble upon:
> > > > > 
> > > > > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > > > > 	switch (ecc->engine_type) {
> > > > > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > > > > 		if (chip->page_shift > 9)
> > > > > 			chip->options |= NAND_SUBPAGE_READ;
> > > > > 		break;
> > > > > 
> > > > > 	default:
> > > > > 		break;
> > > > > 	}
> > > > > 
> > > > > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > > > > in my case is not true. I guess some drivers depend on the
> > > > > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > > > > likely not an option.  Any ideas what to do?    
> > > > 
> > > > Can you elaborate why subpage reads are not an option in your
> > > > situation? While subpage writes depend on chip capabilities, reads
> > > > however should always work: it's just the controller selecting the
> > > > column where to start and then reading less data than it could from the
> > > > NAND cache. It's a very basic NAND controller feature, and I remember
> > > > this was working on eg. an i.MX27.    
> > > 
> > > On the i.MX27 reading a full 2k page means triggering one read operation
> > > per 512 bytes in the NAND controller, so it would be possible to read
> > > subpages by triggering only one read operation instead of four in a row.
> > > 
> > > The newer SoCs like i.MX25 always read a full page with a single read
> > > operation. We could likely read subpages by temporarily configuring the
> > > controller for a 512b page size NAND.
> > > 
> > > I just realized the real problem comes with reading the OOB data. With
> > > software BCH the NAND layer hardcodes the read_subpage hook to
> > > nand_read_subpage() which uses nand_change_read_column_op() to read the
> > > OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
> > > be implemented in the i.MX NAND driver. Right now the controller indeed
> > > reads some data and then the SRAM buffer really contains part of the
> > > desired OOB data, but also part of the user data.  
> > 
> > NAND_CMD_RNDOUT is impossible to avoid,  
> 
> Apparently it has been possible until now. NAND_CMD_RNDOUT has never
> been used with this driver and it also doesn't work like expected.
> 
> One problem is that the read_page_raw() and write_page_raw() are not
> implemented like supposed by the NAND layer. The i.MX NAND controller
> uses a syndrome type ECC layout, meaning that the user data and OOB data
> is interleaved, so the raw r/w functions should normally pass/expect the
> page data in interleaved format. Unfortunately the raw functions are not
> implemented like that, instead they detangle the data themselves. This
> also means that setting the cursor using NAND_CMD_RNDOUT will not put
> the cursor at a meaningful place, as the raw functions are not really
> exect/return the raw page data.
> 
> This could be fixed, but the raw operations are also exposed to
> userspace, so fixing these would mean that we might break some userspace
> applications.

As answered to patch 3/4 I believe you need other raw page helpers for
the software ECC path, just because the existing functions are tight to
the on-host ECC logic and do what they are expected to do (I believe).

Creating another set of raw page helpers should be straightforward to
do if that's really needed (mainly for performance purposes, but we're
not yet there). Using the core helpers should work, the only thing is
supporting properly the NAND_CMD_RNDOUT path, which should be possible
at a rather low cost, it really is a very very basic command, I know no
controller without this feature, even old ones.

> The other point is that with using software BCH ecc the NAND layer
> requests me to read 7 bytes at offset 0x824. This can't be really
> implemented in the i.MX NAND driver. It only allows us to read a full
> 512 byte subpage, so whenever the NAND layer requests me to read a few
> bytes the controller will always transfer 512 bytes from which I then
> ignore most of it (and possibly trigger another 512 bytes transfer when
> reading the ECC for the next subpage).

If you manage to get the NAND_CMD_RNDOUT op working I believe you'll be
tempted to use memcpy32_fromio() with just a slightly rounded up length.

> I think these issues can all be handled somehow, but this comes at a
> rather high price, both in effort and the risk of breaking userspace.
> It would be far easier to tell the NAND layer not to do subpage reads.

I understand it may feel like that but that is likely not the right
approach. I just think about another possibility: using monolithic
reads if the controller is too constrained this way you might end up
avoiding the RNDOUT command (might require a bit of tweaking in the
core, I no longer remember exactly).

Good luck, I really appreciate this effort.
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Mon, 22 Apr 2024 12:53:38 +0200:

> On Fri, Apr 19, 2024 at 11:46:57AM +0200, Miquel Raynal wrote:
> > Hi Sascha,
> > 
> > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 13:43:15 +0200:
> >   
> > > On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:  
> > > > Hi Sascha,
> > > > 
> > > > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> > > >     
> > > > > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:    
> > > > > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > > > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > > > > clear the flag again.
> > > > > > 
> > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > index f44c130dca18d..19b46210bd194 100644
> > > > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > > > > >  	if (err)
> > > > > >  		goto escan;
> > > > > >  
> > > > > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > > > > +      
> > > > > 
> > > > > Nah, it doesn't work like this. It turns out the BBT is read using
> > > > > subpage reads before we can disable them here.
> > > > >
> > > > > This is the code in nand_scan_tail() we stumble upon:
> > > > > 
> > > > > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > > > > 	switch (ecc->engine_type) {
> > > > > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > > > > 		if (chip->page_shift > 9)
> > > > > 			chip->options |= NAND_SUBPAGE_READ;
> > > > > 		break;
> > > > > 
> > > > > 	default:
> > > > > 		break;
> > > > > 	}
> > > > > 
> > > > > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > > > > in my case is not true. I guess some drivers depend on the
> > > > > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > > > > likely not an option.  Any ideas what to do?    
> > > > 
> > > > Can you elaborate why subpage reads are not an option in your
> > > > situation? While subpage writes depend on chip capabilities, reads
> > > > however should always work: it's just the controller selecting the
> > > > column where to start and then reading less data than it could from the
> > > > NAND cache. It's a very basic NAND controller feature, and I remember
> > > > this was working on eg. an i.MX27.    
> > > 
> > > On the i.MX27 reading a full 2k page means triggering one read operation
> > > per 512 bytes in the NAND controller, so it would be possible to read
> > > subpages by triggering only one read operation instead of four in a row.
> > > 
> > > The newer SoCs like i.MX25 always read a full page with a single read
> > > operation. We could likely read subpages by temporarily configuring the
> > > controller for a 512b page size NAND.
> > > 
> > > I just realized the real problem comes with reading the OOB data. With
> > > software BCH the NAND layer hardcodes the read_subpage hook to
> > > nand_read_subpage() which uses nand_change_read_column_op() to read the
> > > OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
> > > be implemented in the i.MX NAND driver. Right now the controller indeed
> > > reads some data and then the SRAM buffer really contains part of the
> > > desired OOB data, but also part of the user data.  
> > 
> > NAND_CMD_RNDOUT is impossible to avoid,  
> 
> Apparently it has been possible until now. NAND_CMD_RNDOUT has never
> been used with this driver and it also doesn't work like expected.
> 
> One problem is that the read_page_raw() and write_page_raw() are not
> implemented like supposed by the NAND layer. The i.MX NAND controller
> uses a syndrome type ECC layout, meaning that the user data and OOB data
> is interleaved, so the raw r/w functions should normally pass/expect the
> page data in interleaved format. Unfortunately the raw functions are not
> implemented like that, instead they detangle the data themselves. This
> also means that setting the cursor using NAND_CMD_RNDOUT will not put
> the cursor at a meaningful place, as the raw functions are not really
> exect/return the raw page data.
> 
> This could be fixed, but the raw operations are also exposed to
> userspace, so fixing these would mean that we might break some userspace
> applications.

As answered to patch 3/4 I believe you need other raw page helpers for
the software ECC path, just because the existing functions are tight to
the on-host ECC logic and do what they are expected to do (I believe).

Creating another set of raw page helpers should be straightforward to
do if that's really needed (mainly for performance purposes, but we're
not yet there). Using the core helpers should work, the only thing is
supporting properly the NAND_CMD_RNDOUT path, which should be possible
at a rather low cost, it really is a very very basic command, I know no
controller without this feature, even old ones.

> The other point is that with using software BCH ecc the NAND layer
> requests me to read 7 bytes at offset 0x824. This can't be really
> implemented in the i.MX NAND driver. It only allows us to read a full
> 512 byte subpage, so whenever the NAND layer requests me to read a few
> bytes the controller will always transfer 512 bytes from which I then
> ignore most of it (and possibly trigger another 512 bytes transfer when
> reading the ECC for the next subpage).

If you manage to get the NAND_CMD_RNDOUT op working I believe you'll be
tempted to use memcpy32_fromio() with just a slightly rounded up length.

> I think these issues can all be handled somehow, but this comes at a
> rather high price, both in effort and the risk of breaking userspace.
> It would be far easier to tell the NAND layer not to do subpage reads.

I understand it may feel like that but that is likely not the right
approach. I just think about another possibility: using monolithic
reads if the controller is too constrained this way you might end up
avoiding the RNDOUT command (might require a bit of tweaking in the
core, I no longer remember exactly).

Good luck, I really appreciate this effort.
Miquèl

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Sascha Hauer]

On Mon, May 06, 2024 at 05:51:06PM +0200, Miquel Raynal wrote:
> Hi Miquel,
> 
> miquel.raynal@bootlin.com wrote on Mon, 6 May 2024 16:05:08 +0200:
> 
> > Hi Sascha,
> > 
> > s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> > 
> > > To support software ECC we still need the driver provided read_oob,
> > > read_page_raw and write_page_raw ops, so set them unconditionally
> > > no matter which engine_type we use. The OOB layout on the other hand
> > > represents the layout the i.MX ECC hardware uses, so set this only
> > > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > > 
> > > With these changes the driver can be used with software BCH ECC which
> > > is useful for NAND chips that require a stronger ECC than the i.MX
> > > hardware supports.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > index fc70c65dea268..f44c130dca18d 100644
> > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> > >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> > >  	host->eccsize = host->devtype_data->eccsize;
> > >  	chip->ecc.size = 512;
> > > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > > +
> > > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;
> 
> A second thought on this. Maybe you should consider keeping these for
> on-host operations only.
> 
> The read/write_page_raw operations are supposed to detangle the data
> organization to show a proper [all data][all oob] organization to the
> user.

Let me take one step back. The organisation in the raw NAND is like this
when using hardware ECC:

[512b data0][16b oob0][512b data1][16b oob1][512b data2][16b oob2][512b data3][16b oob3]

For a standard 2k+64b NAND. The read/write_page_raw operations detangle
this and present the data to the user like this:

[2048b data][64b OOB]

Is this the correct behaviour or should that be changed?
(Side note: The GPMI NAND driver behaves differently here. It has the
same interleaved organisation on the chip and also presents the same
interleaved organisation to the user when using read_page_raw)


With my current approach for software ECC the same layout is used on the
NAND chip. It would interleave the data with the OOB on the NAND chip
and, since using the same read/write_page_raw operations, also presents
[2048b data][64b OOB] to the user.

This works fine currently, but means that NAND_CMD_RNDOUT can't be used.
Using NAND_CMD_RNDOUT to position the cursor at offset 512b for example
doesn't give you the second subpage, but instead oob0. Positioning the
cursor at offset 2048 doesn't give you the start of OOB, but some
position in the middle of data3.

Ok, NAND_CMD_RNDOUT can't be used for hardware ECC and there's no way
around it. For software ECC we could change the organisation in the chip
to be [2048b data][64b oob]. With that NAND_CMD_RNDOUT then could be
used with software ECC.

You say that NAND_CMD_RNDOUT is a basic command that is supported by all
controllers, and yes, it is also supported with the mxc_nand controller.
You just can't control how many bytes are transferred between the NAND
chip and the controller. When using NAND_CMD_RNDOUT to read a few bytes
at a certain page offset we'll end up reading 512 bytes discarding most
of it. For the next ECC block we would move the cursor forward using
another NAND_CMD_RNDOUT command, again read 512 bytes and discard most
it (altough the desired data would have been in the first read already).

So I think NAND_CMD_RNDOUT should really be avoided for this controller,
eventhough we might be able to support it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Sascha Hauer]

On Mon, May 06, 2024 at 05:51:06PM +0200, Miquel Raynal wrote:
> Hi Miquel,
> 
> miquel.raynal@bootlin.com wrote on Mon, 6 May 2024 16:05:08 +0200:
> 
> > Hi Sascha,
> > 
> > s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> > 
> > > To support software ECC we still need the driver provided read_oob,
> > > read_page_raw and write_page_raw ops, so set them unconditionally
> > > no matter which engine_type we use. The OOB layout on the other hand
> > > represents the layout the i.MX ECC hardware uses, so set this only
> > > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > > 
> > > With these changes the driver can be used with software BCH ECC which
> > > is useful for NAND chips that require a stronger ECC than the i.MX
> > > hardware supports.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > index fc70c65dea268..f44c130dca18d 100644
> > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> > >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> > >  	host->eccsize = host->devtype_data->eccsize;
> > >  	chip->ecc.size = 512;
> > > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > > +
> > > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;
> 
> A second thought on this. Maybe you should consider keeping these for
> on-host operations only.
> 
> The read/write_page_raw operations are supposed to detangle the data
> organization to show a proper [all data][all oob] organization to the
> user.

Let me take one step back. The organisation in the raw NAND is like this
when using hardware ECC:

[512b data0][16b oob0][512b data1][16b oob1][512b data2][16b oob2][512b data3][16b oob3]

For a standard 2k+64b NAND. The read/write_page_raw operations detangle
this and present the data to the user like this:

[2048b data][64b OOB]

Is this the correct behaviour or should that be changed?
(Side note: The GPMI NAND driver behaves differently here. It has the
same interleaved organisation on the chip and also presents the same
interleaved organisation to the user when using read_page_raw)


With my current approach for software ECC the same layout is used on the
NAND chip. It would interleave the data with the OOB on the NAND chip
and, since using the same read/write_page_raw operations, also presents
[2048b data][64b OOB] to the user.

This works fine currently, but means that NAND_CMD_RNDOUT can't be used.
Using NAND_CMD_RNDOUT to position the cursor at offset 512b for example
doesn't give you the second subpage, but instead oob0. Positioning the
cursor at offset 2048 doesn't give you the start of OOB, but some
position in the middle of data3.

Ok, NAND_CMD_RNDOUT can't be used for hardware ECC and there's no way
around it. For software ECC we could change the organisation in the chip
to be [2048b data][64b oob]. With that NAND_CMD_RNDOUT then could be
used with software ECC.

You say that NAND_CMD_RNDOUT is a basic command that is supported by all
controllers, and yes, it is also supported with the mxc_nand controller.
You just can't control how many bytes are transferred between the NAND
chip and the controller. When using NAND_CMD_RNDOUT to read a few bytes
at a certain page offset we'll end up reading 512 bytes discarding most
of it. For the next ECC block we would move the cursor forward using
another NAND_CMD_RNDOUT command, again read 512 bytes and discard most
it (altough the desired data would have been in the first read already).

So I think NAND_CMD_RNDOUT should really be avoided for this controller,
eventhough we might be able to support it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

On Mon, May 06, 2024 at 06:41:08PM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Mon, 22 Apr 2024 12:53:38 +0200:
> 
> > On Fri, Apr 19, 2024 at 11:46:57AM +0200, Miquel Raynal wrote:
> > > Hi Sascha,
> > > 
> > > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 13:43:15 +0200:
> > >   
> > > > On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:  
> > > > > Hi Sascha,
> > > > > 
> > > > > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> > > > >     
> > > > > > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:    
> > > > > > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > > > > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > > > > > clear the flag again.
> > > > > > > 
> > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > > ---
> > > > > > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > > > > > >  1 file changed, 2 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > > index f44c130dca18d..19b46210bd194 100644
> > > > > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > > > > > >  	if (err)
> > > > > > >  		goto escan;
> > > > > > >  
> > > > > > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > > > > > +      
> > > > > > 
> > > > > > Nah, it doesn't work like this. It turns out the BBT is read using
> > > > > > subpage reads before we can disable them here.
> > > > > >
> > > > > > This is the code in nand_scan_tail() we stumble upon:
> > > > > > 
> > > > > > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > > > > > 	switch (ecc->engine_type) {
> > > > > > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > > > > > 		if (chip->page_shift > 9)
> > > > > > 			chip->options |= NAND_SUBPAGE_READ;
> > > > > > 		break;
> > > > > > 
> > > > > > 	default:
> > > > > > 		break;
> > > > > > 	}
> > > > > > 
> > > > > > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > > > > > in my case is not true. I guess some drivers depend on the
> > > > > > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > > > > > likely not an option.  Any ideas what to do?    
> > > > > 
> > > > > Can you elaborate why subpage reads are not an option in your
> > > > > situation? While subpage writes depend on chip capabilities, reads
> > > > > however should always work: it's just the controller selecting the
> > > > > column where to start and then reading less data than it could from the
> > > > > NAND cache. It's a very basic NAND controller feature, and I remember
> > > > > this was working on eg. an i.MX27.    
> > > > 
> > > > On the i.MX27 reading a full 2k page means triggering one read operation
> > > > per 512 bytes in the NAND controller, so it would be possible to read
> > > > subpages by triggering only one read operation instead of four in a row.
> > > > 
> > > > The newer SoCs like i.MX25 always read a full page with a single read
> > > > operation. We could likely read subpages by temporarily configuring the
> > > > controller for a 512b page size NAND.
> > > > 
> > > > I just realized the real problem comes with reading the OOB data. With
> > > > software BCH the NAND layer hardcodes the read_subpage hook to
> > > > nand_read_subpage() which uses nand_change_read_column_op() to read the
> > > > OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
> > > > be implemented in the i.MX NAND driver. Right now the controller indeed
> > > > reads some data and then the SRAM buffer really contains part of the
> > > > desired OOB data, but also part of the user data.  
> > > 
> > > NAND_CMD_RNDOUT is impossible to avoid,  
> > 
> > Apparently it has been possible until now. NAND_CMD_RNDOUT has never
> > been used with this driver and it also doesn't work like expected.
> > 
> > One problem is that the read_page_raw() and write_page_raw() are not
> > implemented like supposed by the NAND layer. The i.MX NAND controller
> > uses a syndrome type ECC layout, meaning that the user data and OOB data
> > is interleaved, so the raw r/w functions should normally pass/expect the
> > page data in interleaved format. Unfortunately the raw functions are not
> > implemented like that, instead they detangle the data themselves. This
> > also means that setting the cursor using NAND_CMD_RNDOUT will not put
> > the cursor at a meaningful place, as the raw functions are not really
> > exect/return the raw page data.
> > 
> > This could be fixed, but the raw operations are also exposed to
> > userspace, so fixing these would mean that we might break some userspace
> > applications.
> 
> As answered to patch 3/4 I believe you need other raw page helpers for
> the software ECC path, just because the existing functions are tight to
> the on-host ECC logic and do what they are expected to do (I believe).
> 
> Creating another set of raw page helpers should be straightforward to
> do if that's really needed (mainly for performance purposes, but we're
> not yet there). Using the core helpers should work, the only thing is
> supporting properly the NAND_CMD_RNDOUT path, which should be possible
> at a rather low cost, it really is a very very basic command, I know no
> controller without this feature, even old ones.
> 
> > The other point is that with using software BCH ecc the NAND layer
> > requests me to read 7 bytes at offset 0x824. This can't be really
> > implemented in the i.MX NAND driver. It only allows us to read a full
> > 512 byte subpage, so whenever the NAND layer requests me to read a few
> > bytes the controller will always transfer 512 bytes from which I then
> > ignore most of it (and possibly trigger another 512 bytes transfer when
> > reading the ECC for the next subpage).
> 
> If you manage to get the NAND_CMD_RNDOUT op working I believe you'll be
> tempted to use memcpy32_fromio() with just a slightly rounded up length.

The overhead due to word rounding in memcpy32_fromio() is negligible, my
concern is that the controller will read 512 bytes from the NAND chip SRAM
to the controllers internal SRAM each time this command is used.

> 
> > I think these issues can all be handled somehow, but this comes at a
> > rather high price, both in effort and the risk of breaking userspace.
> > It would be far easier to tell the NAND layer not to do subpage reads.
> 
> I understand it may feel like that but that is likely not the right
> approach. I just think about another possibility: using monolithic
> reads if the controller is too constrained this way you might end up
> avoiding the RNDOUT command (might require a bit of tweaking in the
> core, I no longer remember exactly).

Not sure if I understand you correctly. I think using monolithic reads
is exactly what I am trying to archieve with disallowing subpage reads.

Subpage reads are rather unnecessary anyway. They are a performance
improvement when scanning the NAND for bad blocks and while reading
the UBI EC and VID headers. The former can be avoided with a
BBT and the latter with UBI fastmap (at least for the boot time critical
path when attaching a UBI).

> 
> Good luck, I really appreciate this effort.

Thanks. I'll do my best to get this done.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: nand: mxc_nand: disable subpage reads

[Sascha Hauer]

On Mon, May 06, 2024 at 06:41:08PM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Mon, 22 Apr 2024 12:53:38 +0200:
> 
> > On Fri, Apr 19, 2024 at 11:46:57AM +0200, Miquel Raynal wrote:
> > > Hi Sascha,
> > > 
> > > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 13:43:15 +0200:
> > >   
> > > > On Thu, Apr 18, 2024 at 11:32:44AM +0200, Miquel Raynal wrote:  
> > > > > Hi Sascha,
> > > > > 
> > > > > s.hauer@pengutronix.de wrote on Thu, 18 Apr 2024 08:48:08 +0200:
> > > > >     
> > > > > > On Wed, Apr 17, 2024 at 09:13:31AM +0200, Sascha Hauer wrote:    
> > > > > > > The NAND core enabled subpage reads when a largepage NAND is used with
> > > > > > > SOFT_ECC. The i.MX NAND controller doesn't support subpage reads, so
> > > > > > > clear the flag again.
> > > > > > > 
> > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > > ---
> > > > > > >  drivers/mtd/nand/raw/mxc_nand.c | 2 ++
> > > > > > >  1 file changed, 2 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > > index f44c130dca18d..19b46210bd194 100644
> > > > > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > > > @@ -1667,6 +1667,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> > > > > > >  	if (err)
> > > > > > >  		goto escan;
> > > > > > >  
> > > > > > > +	this->options &= ~NAND_SUBPAGE_READ;
> > > > > > > +      
> > > > > > 
> > > > > > Nah, it doesn't work like this. It turns out the BBT is read using
> > > > > > subpage reads before we can disable them here.
> > > > > >
> > > > > > This is the code in nand_scan_tail() we stumble upon:
> > > > > > 
> > > > > > 	/* Large page NAND with SOFT_ECC should support subpage reads */
> > > > > > 	switch (ecc->engine_type) {
> > > > > > 	case NAND_ECC_ENGINE_TYPE_SOFT:
> > > > > > 		if (chip->page_shift > 9)
> > > > > > 			chip->options |= NAND_SUBPAGE_READ;
> > > > > > 		break;
> > > > > > 
> > > > > > 	default:
> > > > > > 		break;
> > > > > > 	}
> > > > > > 
> > > > > > So the code assumes subpage reads are ok when SOFT_ECC is in use, which
> > > > > > in my case is not true. I guess some drivers depend on the
> > > > > > NAND_SUBPAGE_READ bit magically be set, so simply removing this code is
> > > > > > likely not an option.  Any ideas what to do?    
> > > > > 
> > > > > Can you elaborate why subpage reads are not an option in your
> > > > > situation? While subpage writes depend on chip capabilities, reads
> > > > > however should always work: it's just the controller selecting the
> > > > > column where to start and then reading less data than it could from the
> > > > > NAND cache. It's a very basic NAND controller feature, and I remember
> > > > > this was working on eg. an i.MX27.    
> > > > 
> > > > On the i.MX27 reading a full 2k page means triggering one read operation
> > > > per 512 bytes in the NAND controller, so it would be possible to read
> > > > subpages by triggering only one read operation instead of four in a row.
> > > > 
> > > > The newer SoCs like i.MX25 always read a full page with a single read
> > > > operation. We could likely read subpages by temporarily configuring the
> > > > controller for a 512b page size NAND.
> > > > 
> > > > I just realized the real problem comes with reading the OOB data. With
> > > > software BCH the NAND layer hardcodes the read_subpage hook to
> > > > nand_read_subpage() which uses nand_change_read_column_op() to read the
> > > > OOB data. This uses NAND_CMD_RNDOUT and I have now idea if/how this can
> > > > be implemented in the i.MX NAND driver. Right now the controller indeed
> > > > reads some data and then the SRAM buffer really contains part of the
> > > > desired OOB data, but also part of the user data.  
> > > 
> > > NAND_CMD_RNDOUT is impossible to avoid,  
> > 
> > Apparently it has been possible until now. NAND_CMD_RNDOUT has never
> > been used with this driver and it also doesn't work like expected.
> > 
> > One problem is that the read_page_raw() and write_page_raw() are not
> > implemented like supposed by the NAND layer. The i.MX NAND controller
> > uses a syndrome type ECC layout, meaning that the user data and OOB data
> > is interleaved, so the raw r/w functions should normally pass/expect the
> > page data in interleaved format. Unfortunately the raw functions are not
> > implemented like that, instead they detangle the data themselves. This
> > also means that setting the cursor using NAND_CMD_RNDOUT will not put
> > the cursor at a meaningful place, as the raw functions are not really
> > exect/return the raw page data.
> > 
> > This could be fixed, but the raw operations are also exposed to
> > userspace, so fixing these would mean that we might break some userspace
> > applications.
> 
> As answered to patch 3/4 I believe you need other raw page helpers for
> the software ECC path, just because the existing functions are tight to
> the on-host ECC logic and do what they are expected to do (I believe).
> 
> Creating another set of raw page helpers should be straightforward to
> do if that's really needed (mainly for performance purposes, but we're
> not yet there). Using the core helpers should work, the only thing is
> supporting properly the NAND_CMD_RNDOUT path, which should be possible
> at a rather low cost, it really is a very very basic command, I know no
> controller without this feature, even old ones.
> 
> > The other point is that with using software BCH ecc the NAND layer
> > requests me to read 7 bytes at offset 0x824. This can't be really
> > implemented in the i.MX NAND driver. It only allows us to read a full
> > 512 byte subpage, so whenever the NAND layer requests me to read a few
> > bytes the controller will always transfer 512 bytes from which I then
> > ignore most of it (and possibly trigger another 512 bytes transfer when
> > reading the ECC for the next subpage).
> 
> If you manage to get the NAND_CMD_RNDOUT op working I believe you'll be
> tempted to use memcpy32_fromio() with just a slightly rounded up length.

The overhead due to word rounding in memcpy32_fromio() is negligible, my
concern is that the controller will read 512 bytes from the NAND chip SRAM
to the controllers internal SRAM each time this command is used.

> 
> > I think these issues can all be handled somehow, but this comes at a
> > rather high price, both in effort and the risk of breaking userspace.
> > It would be far easier to tell the NAND layer not to do subpage reads.
> 
> I understand it may feel like that but that is likely not the right
> approach. I just think about another possibility: using monolithic
> reads if the controller is too constrained this way you might end up
> avoiding the RNDOUT command (might require a bit of tweaking in the
> core, I no longer remember exactly).

Not sure if I understand you correctly. I think using monolithic reads
is exactly what I am trying to archieve with disallowing subpage reads.

Subpage reads are rather unnecessary anyway. They are a performance
improvement when scanning the NAND for bad blocks and while reading
the UBI EC and VID headers. The former can be avoided with a
BBT and the latter with UBI fastmap (at least for the boot time critical
path when attaching a UBI).

> 
> Good luck, I really appreciate this effort.

Thanks. I'll do my best to get this done.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Tue, 7 May 2024 09:12:30 +0200:

> On Mon, May 06, 2024 at 05:51:06PM +0200, Miquel Raynal wrote:
> > Hi Miquel,
> > 
> > miquel.raynal@bootlin.com wrote on Mon, 6 May 2024 16:05:08 +0200:
> >   
> > > Hi Sascha,
> > > 
> > > s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> > >   
> > > > To support software ECC we still need the driver provided read_oob,
> > > > read_page_raw and write_page_raw ops, so set them unconditionally
> > > > no matter which engine_type we use. The OOB layout on the other hand
> > > > represents the layout the i.MX ECC hardware uses, so set this only
> > > > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > > > 
> > > > With these changes the driver can be used with software BCH ECC which
> > > > is useful for NAND chips that require a stronger ECC than the i.MX
> > > > hardware supports.
> > > > 
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > ---
> > > >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > index fc70c65dea268..f44c130dca18d 100644
> > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> > > >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> > > >  	host->eccsize = host->devtype_data->eccsize;
> > > >  	chip->ecc.size = 512;
> > > > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > > > +
> > > > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > > > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > > > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;  
> > 
> > A second thought on this. Maybe you should consider keeping these for
> > on-host operations only.
> > 
> > The read/write_page_raw operations are supposed to detangle the data
> > organization to show a proper [all data][all oob] organization to the
> > user.  
> 
> Let me take one step back. The organisation in the raw NAND is like this
> when using hardware ECC:
> 
> [512b data0][16b oob0][512b data1][16b oob1][512b data2][16b oob2][512b data3][16b oob3]
> 
> For a standard 2k+64b NAND. The read/write_page_raw operations detangle
> this and present the data to the user like this:
> 
> [2048b data][64b OOB]
> 
> Is this the correct behaviour or should that be changed?

I believe so, yes.

> (Side note: The GPMI NAND driver behaves differently here. It has the
> same interleaved organisation on the chip and also presents the same
> interleaved organisation to the user when using read_page_raw)

I'd say the GPMI driver is wrong?

> With my current approach for software ECC the same layout is used on the
> NAND chip. It would interleave the data with the OOB on the NAND chip
> and, since using the same read/write_page_raw operations, also presents
> [2048b data][64b OOB] to the user.

No need, I believe the only reason for interleaving is that your
hardware ECC engine works like that (writes the ECC bytes slightly
after each chunk of data). So if you don't use on-host hardware ECC,
you don't need to deal with this data layout.

> This works fine currently, but means that NAND_CMD_RNDOUT can't be used.
> Using NAND_CMD_RNDOUT to position the cursor at offset 512b for example
> doesn't give you the second subpage, but instead oob0. Positioning the
> cursor at offset 2048 doesn't give you the start of OOB, but some
> position in the middle of data3.
> 
> Ok, NAND_CMD_RNDOUT can't be used for hardware ECC and there's no way
> around it. For software ECC we could change the organisation in the chip
> to be [2048b data][64b oob]. With that NAND_CMD_RNDOUT then could be
> used with software ECC.
> 
> You say that NAND_CMD_RNDOUT is a basic command that is supported by all
> controllers, and yes, it is also supported with the mxc_nand controller.
> You just can't control how many bytes are transferred between the NAND
> chip and the controller. When using NAND_CMD_RNDOUT to read a few bytes
> at a certain page offset we'll end up reading 512 bytes discarding most
> of it. For the next ECC block we would move the cursor forward using
> another NAND_CMD_RNDOUT command, again read 512 bytes and discard most
> it (altough the desired data would have been in the first read already).

I'm not sure the controller limitations are so bad in this case. The
core helpers (using the same example) will ask for:
- 512b at offset 0
- 512b at offset 512...
- and finally 64b at offset 2048.
In practice it does not look like a huge drawback? I don't understand
in which case so much data would be read and then discarded?

> So I think NAND_CMD_RNDOUT should really be avoided for this controller,
> eventhough we might be able to support it.

I also mentioned the monolithic accessors which try to avoid these
random column changes. You probably want to check them out, they might
just avoid the need for NAND_CMD_RNDOUT by forcing full page accesses
directly. The reason why they were introduced is not exactly our
current use case, but it feels like they might be handy.

658beb663960 ("mtd: rawnand: Expose monolithic read/write_page_raw() helpers")
0e7f4b64ea46 ("mtd: rawnand: Allow controllers to overload soft ECC hooks")

Thanks,
Miquèl

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Tue, 7 May 2024 09:12:30 +0200:

> On Mon, May 06, 2024 at 05:51:06PM +0200, Miquel Raynal wrote:
> > Hi Miquel,
> > 
> > miquel.raynal@bootlin.com wrote on Mon, 6 May 2024 16:05:08 +0200:
> >   
> > > Hi Sascha,
> > > 
> > > s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> > >   
> > > > To support software ECC we still need the driver provided read_oob,
> > > > read_page_raw and write_page_raw ops, so set them unconditionally
> > > > no matter which engine_type we use. The OOB layout on the other hand
> > > > represents the layout the i.MX ECC hardware uses, so set this only
> > > > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > > > 
> > > > With these changes the driver can be used with software BCH ECC which
> > > > is useful for NAND chips that require a stronger ECC than the i.MX
> > > > hardware supports.
> > > > 
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > ---
> > > >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > index fc70c65dea268..f44c130dca18d 100644
> > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> > > >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> > > >  	host->eccsize = host->devtype_data->eccsize;
> > > >  	chip->ecc.size = 512;
> > > > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > > > +
> > > > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > > > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > > > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;  
> > 
> > A second thought on this. Maybe you should consider keeping these for
> > on-host operations only.
> > 
> > The read/write_page_raw operations are supposed to detangle the data
> > organization to show a proper [all data][all oob] organization to the
> > user.  
> 
> Let me take one step back. The organisation in the raw NAND is like this
> when using hardware ECC:
> 
> [512b data0][16b oob0][512b data1][16b oob1][512b data2][16b oob2][512b data3][16b oob3]
> 
> For a standard 2k+64b NAND. The read/write_page_raw operations detangle
> this and present the data to the user like this:
> 
> [2048b data][64b OOB]
> 
> Is this the correct behaviour or should that be changed?

I believe so, yes.

> (Side note: The GPMI NAND driver behaves differently here. It has the
> same interleaved organisation on the chip and also presents the same
> interleaved organisation to the user when using read_page_raw)

I'd say the GPMI driver is wrong?

> With my current approach for software ECC the same layout is used on the
> NAND chip. It would interleave the data with the OOB on the NAND chip
> and, since using the same read/write_page_raw operations, also presents
> [2048b data][64b OOB] to the user.

No need, I believe the only reason for interleaving is that your
hardware ECC engine works like that (writes the ECC bytes slightly
after each chunk of data). So if you don't use on-host hardware ECC,
you don't need to deal with this data layout.

> This works fine currently, but means that NAND_CMD_RNDOUT can't be used.
> Using NAND_CMD_RNDOUT to position the cursor at offset 512b for example
> doesn't give you the second subpage, but instead oob0. Positioning the
> cursor at offset 2048 doesn't give you the start of OOB, but some
> position in the middle of data3.
> 
> Ok, NAND_CMD_RNDOUT can't be used for hardware ECC and there's no way
> around it. For software ECC we could change the organisation in the chip
> to be [2048b data][64b oob]. With that NAND_CMD_RNDOUT then could be
> used with software ECC.
> 
> You say that NAND_CMD_RNDOUT is a basic command that is supported by all
> controllers, and yes, it is also supported with the mxc_nand controller.
> You just can't control how many bytes are transferred between the NAND
> chip and the controller. When using NAND_CMD_RNDOUT to read a few bytes
> at a certain page offset we'll end up reading 512 bytes discarding most
> of it. For the next ECC block we would move the cursor forward using
> another NAND_CMD_RNDOUT command, again read 512 bytes and discard most
> it (altough the desired data would have been in the first read already).

I'm not sure the controller limitations are so bad in this case. The
core helpers (using the same example) will ask for:
- 512b at offset 0
- 512b at offset 512...
- and finally 64b at offset 2048.
In practice it does not look like a huge drawback? I don't understand
in which case so much data would be read and then discarded?

> So I think NAND_CMD_RNDOUT should really be avoided for this controller,
> eventhough we might be able to support it.

I also mentioned the monolithic accessors which try to avoid these
random column changes. You probably want to check them out, they might
just avoid the need for NAND_CMD_RNDOUT by forcing full page accesses
directly. The reason why they were introduced is not exactly our
current use case, but it feels like they might be handy.

658beb663960 ("mtd: rawnand: Expose monolithic read/write_page_raw() helpers")
0e7f4b64ea46 ("mtd: rawnand: Allow controllers to overload soft ECC hooks")

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Sascha Hauer]

On Tue, May 07, 2024 at 09:45:38AM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Tue, 7 May 2024 09:12:30 +0200:
> 
> > On Mon, May 06, 2024 at 05:51:06PM +0200, Miquel Raynal wrote:
> > > Hi Miquel,
> > > 
> > > miquel.raynal@bootlin.com wrote on Mon, 6 May 2024 16:05:08 +0200:
> > >   
> > > > Hi Sascha,
> > > > 
> > > > s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> > > >   
> > > > > To support software ECC we still need the driver provided read_oob,
> > > > > read_page_raw and write_page_raw ops, so set them unconditionally
> > > > > no matter which engine_type we use. The OOB layout on the other hand
> > > > > represents the layout the i.MX ECC hardware uses, so set this only
> > > > > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > > > > 
> > > > > With these changes the driver can be used with software BCH ECC which
> > > > > is useful for NAND chips that require a stronger ECC than the i.MX
> > > > > hardware supports.
> > > > > 
> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > ---
> > > > >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > index fc70c65dea268..f44c130dca18d 100644
> > > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> > > > >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> > > > >  	host->eccsize = host->devtype_data->eccsize;
> > > > >  	chip->ecc.size = 512;
> > > > > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > > > > +
> > > > > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > > > > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > > > > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;  
> > > 
> > > A second thought on this. Maybe you should consider keeping these for
> > > on-host operations only.
> > > 
> > > The read/write_page_raw operations are supposed to detangle the data
> > > organization to show a proper [all data][all oob] organization to the
> > > user.  
> > 
> > Let me take one step back. The organisation in the raw NAND is like this
> > when using hardware ECC:
> > 
> > [512b data0][16b oob0][512b data1][16b oob1][512b data2][16b oob2][512b data3][16b oob3]
> > 
> > For a standard 2k+64b NAND. The read/write_page_raw operations detangle
> > this and present the data to the user like this:
> > 
> > [2048b data][64b OOB]
> > 
> > Is this the correct behaviour or should that be changed?
> 
> I believe so, yes.
> 
> > (Side note: The GPMI NAND driver behaves differently here. It has the
> > same interleaved organisation on the chip and also presents the same
> > interleaved organisation to the user when using read_page_raw)
> 
> I'd say the GPMI driver is wrong?
> 
> > With my current approach for software ECC the same layout is used on the
> > NAND chip. It would interleave the data with the OOB on the NAND chip
> > and, since using the same read/write_page_raw operations, also presents
> > [2048b data][64b OOB] to the user.
> 
> No need, I believe the only reason for interleaving is that your
> hardware ECC engine works like that (writes the ECC bytes slightly
> after each chunk of data). So if you don't use on-host hardware ECC,
> you don't need to deal with this data layout.

Right, I could use a different layout for software ECC. Using the same
layout for both hardware and software ECC is just quite convenient as
the same mxc_nand_read_page_raw()/mxc_nand_write_page_raw() could be
used for both software and hardware ECC.

Another thing that might be worth considering is that if we use
different functions for raw read/write page is that we would get
different views on the same raw page data if we switch from software to
hardware ECC or the other way round which might be confusing.

> 
> > This works fine currently, but means that NAND_CMD_RNDOUT can't be used.
> > Using NAND_CMD_RNDOUT to position the cursor at offset 512b for example
> > doesn't give you the second subpage, but instead oob0. Positioning the
> > cursor at offset 2048 doesn't give you the start of OOB, but some
> > position in the middle of data3.
> > 
> > Ok, NAND_CMD_RNDOUT can't be used for hardware ECC and there's no way
> > around it. For software ECC we could change the organisation in the chip
> > to be [2048b data][64b oob]. With that NAND_CMD_RNDOUT then could be
> > used with software ECC.
> > 
> > You say that NAND_CMD_RNDOUT is a basic command that is supported by all
> > controllers, and yes, it is also supported with the mxc_nand controller.
> > You just can't control how many bytes are transferred between the NAND
> > chip and the controller. When using NAND_CMD_RNDOUT to read a few bytes
> > at a certain page offset we'll end up reading 512 bytes discarding most
> > of it. For the next ECC block we would move the cursor forward using
> > another NAND_CMD_RNDOUT command, again read 512 bytes and discard most
> > it (altough the desired data would have been in the first read already).
> 
> I'm not sure the controller limitations are so bad in this case. The
> core helpers (using the same example) will ask for:
> - 512b at offset 0
> - 512b at offset 512...
> - and finally 64b at offset 2048.
> In practice it does not look like a huge drawback? I don't understand
> in which case so much data would be read and then discarded?

Yes, you're right. I misread the code and thought the core would read
the ECC separately for each subpage. In fact it doesn't do so, the ECC
is always read in one go even for multiple subpages.

> 
> > So I think NAND_CMD_RNDOUT should really be avoided for this controller,
> > eventhough we might be able to support it.
> 
> I also mentioned the monolithic accessors which try to avoid these
> random column changes. You probably want to check them out, they might
> just avoid the need for NAND_CMD_RNDOUT by forcing full page accesses
> directly. The reason why they were introduced is not exactly our
> current use case, but it feels like they might be handy.
> 
> 658beb663960 ("mtd: rawnand: Expose monolithic read/write_page_raw() helpers")
> 0e7f4b64ea46 ("mtd: rawnand: Allow controllers to overload soft ECC hooks")

Yes, I already make use of 0e7f4b64ea46. My problem is only the ecc.read_subpage
hook which can't be overwritten and AFAIK this is the only way
NAND_CMD_RNDOUT might be used in my case.

I think my favourite solution would be to:

- store data/OOB interleaved for both hardware and software ECC
- For software ECC use a similar OOB layout as used with hardware
  ECC. This allows us to read a subpage including its ECC data in
  a single step (just like with hardware ECC the controller just
  reads 512b + 16b for each subpage)
- Allow to disable subpage reads in the NAND core

As a further optimisation we could make ecc.read_subpage overwritable
for ecc->engine_type == NAND_ECC_ENGINE_TYPE_SOFT && ecc->algo ==
NAND_ECC_ALGO_BCH. With the OOB layout described above that would be
easily implementable with the mxc_nand controller.

What do you think?

If you insist I would go the path of making NAND_CMD_RNDOUT work for
software ECC, although I think it would cause me extra work with no
clear gain for me.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Sascha Hauer]

On Tue, May 07, 2024 at 09:45:38AM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Tue, 7 May 2024 09:12:30 +0200:
> 
> > On Mon, May 06, 2024 at 05:51:06PM +0200, Miquel Raynal wrote:
> > > Hi Miquel,
> > > 
> > > miquel.raynal@bootlin.com wrote on Mon, 6 May 2024 16:05:08 +0200:
> > >   
> > > > Hi Sascha,
> > > > 
> > > > s.hauer@pengutronix.de wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> > > >   
> > > > > To support software ECC we still need the driver provided read_oob,
> > > > > read_page_raw and write_page_raw ops, so set them unconditionally
> > > > > no matter which engine_type we use. The OOB layout on the other hand
> > > > > represents the layout the i.MX ECC hardware uses, so set this only
> > > > > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > > > > 
> > > > > With these changes the driver can be used with software BCH ECC which
> > > > > is useful for NAND chips that require a stronger ECC than the i.MX
> > > > > hardware supports.
> > > > > 
> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > ---
> > > > >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > index fc70c65dea268..f44c130dca18d 100644
> > > > > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > > > > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > > > > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> > > > >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> > > > >  	host->eccsize = host->devtype_data->eccsize;
> > > > >  	chip->ecc.size = 512;
> > > > > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > > > > +
> > > > > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > > > > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > > > > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;  
> > > 
> > > A second thought on this. Maybe you should consider keeping these for
> > > on-host operations only.
> > > 
> > > The read/write_page_raw operations are supposed to detangle the data
> > > organization to show a proper [all data][all oob] organization to the
> > > user.  
> > 
> > Let me take one step back. The organisation in the raw NAND is like this
> > when using hardware ECC:
> > 
> > [512b data0][16b oob0][512b data1][16b oob1][512b data2][16b oob2][512b data3][16b oob3]
> > 
> > For a standard 2k+64b NAND. The read/write_page_raw operations detangle
> > this and present the data to the user like this:
> > 
> > [2048b data][64b OOB]
> > 
> > Is this the correct behaviour or should that be changed?
> 
> I believe so, yes.
> 
> > (Side note: The GPMI NAND driver behaves differently here. It has the
> > same interleaved organisation on the chip and also presents the same
> > interleaved organisation to the user when using read_page_raw)
> 
> I'd say the GPMI driver is wrong?
> 
> > With my current approach for software ECC the same layout is used on the
> > NAND chip. It would interleave the data with the OOB on the NAND chip
> > and, since using the same read/write_page_raw operations, also presents
> > [2048b data][64b OOB] to the user.
> 
> No need, I believe the only reason for interleaving is that your
> hardware ECC engine works like that (writes the ECC bytes slightly
> after each chunk of data). So if you don't use on-host hardware ECC,
> you don't need to deal with this data layout.

Right, I could use a different layout for software ECC. Using the same
layout for both hardware and software ECC is just quite convenient as
the same mxc_nand_read_page_raw()/mxc_nand_write_page_raw() could be
used for both software and hardware ECC.

Another thing that might be worth considering is that if we use
different functions for raw read/write page is that we would get
different views on the same raw page data if we switch from software to
hardware ECC or the other way round which might be confusing.

> 
> > This works fine currently, but means that NAND_CMD_RNDOUT can't be used.
> > Using NAND_CMD_RNDOUT to position the cursor at offset 512b for example
> > doesn't give you the second subpage, but instead oob0. Positioning the
> > cursor at offset 2048 doesn't give you the start of OOB, but some
> > position in the middle of data3.
> > 
> > Ok, NAND_CMD_RNDOUT can't be used for hardware ECC and there's no way
> > around it. For software ECC we could change the organisation in the chip
> > to be [2048b data][64b oob]. With that NAND_CMD_RNDOUT then could be
> > used with software ECC.
> > 
> > You say that NAND_CMD_RNDOUT is a basic command that is supported by all
> > controllers, and yes, it is also supported with the mxc_nand controller.
> > You just can't control how many bytes are transferred between the NAND
> > chip and the controller. When using NAND_CMD_RNDOUT to read a few bytes
> > at a certain page offset we'll end up reading 512 bytes discarding most
> > of it. For the next ECC block we would move the cursor forward using
> > another NAND_CMD_RNDOUT command, again read 512 bytes and discard most
> > it (altough the desired data would have been in the first read already).
> 
> I'm not sure the controller limitations are so bad in this case. The
> core helpers (using the same example) will ask for:
> - 512b at offset 0
> - 512b at offset 512...
> - and finally 64b at offset 2048.
> In practice it does not look like a huge drawback? I don't understand
> in which case so much data would be read and then discarded?

Yes, you're right. I misread the code and thought the core would read
the ECC separately for each subpage. In fact it doesn't do so, the ECC
is always read in one go even for multiple subpages.

> 
> > So I think NAND_CMD_RNDOUT should really be avoided for this controller,
> > eventhough we might be able to support it.
> 
> I also mentioned the monolithic accessors which try to avoid these
> random column changes. You probably want to check them out, they might
> just avoid the need for NAND_CMD_RNDOUT by forcing full page accesses
> directly. The reason why they were introduced is not exactly our
> current use case, but it feels like they might be handy.
> 
> 658beb663960 ("mtd: rawnand: Expose monolithic read/write_page_raw() helpers")
> 0e7f4b64ea46 ("mtd: rawnand: Allow controllers to overload soft ECC hooks")

Yes, I already make use of 0e7f4b64ea46. My problem is only the ecc.read_subpage
hook which can't be overwritten and AFAIK this is the only way
NAND_CMD_RNDOUT might be used in my case.

I think my favourite solution would be to:

- store data/OOB interleaved for both hardware and software ECC
- For software ECC use a similar OOB layout as used with hardware
  ECC. This allows us to read a subpage including its ECC data in
  a single step (just like with hardware ECC the controller just
  reads 512b + 16b for each subpage)
- Allow to disable subpage reads in the NAND core

As a further optimisation we could make ecc.read_subpage overwritable
for ecc->engine_type == NAND_ECC_ENGINE_TYPE_SOFT && ecc->algo ==
NAND_ECC_ALGO_BCH. With the OOB layout described above that would be
easily implementable with the mxc_nand controller.

What do you think?

If you insist I would go the path of making NAND_CMD_RNDOUT work for
software ECC, although I think it would cause me extra work with no
clear gain for me.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Sascha,

> > No need, I believe the only reason for interleaving is that your
> > hardware ECC engine works like that (writes the ECC bytes slightly
> > after each chunk of data). So if you don't use on-host hardware ECC,
> > you don't need to deal with this data layout.  
> 
> Right, I could use a different layout for software ECC. Using the same
> layout for both hardware and software ECC is just quite convenient as
> the same mxc_nand_read_page_raw()/mxc_nand_write_page_raw() could be
> used for both software and hardware ECC.

I'm not sure I see why it would be more convenient, as you basically
don't need to provide anything if you use software ECC engines besides
a minimal exec_op() implementation. Anyway, that's clearly not the good
approach for software ECC: the engine decides where it wants to put the
data, there is just no reason to complexify the software layout (which
is free from any constraints).

> Another thing that might be worth considering is that if we use
> different functions for raw read/write page is that we would get
> different views on the same raw page data if we switch from software to
> hardware ECC or the other way round which might be confusing.

Don't worry about that: it's impossible to manage. Data layout might be
different of course, but most importantly once you've chosen an ECC
configuration, any access with another configuration will simply fail.
And I'm not just talking about the ECC/step size parameters, each
engine has it's own base polynomial on which it bases all its internal
calculations, and besides trying very hard to mimic your hardware
engine in software for some very good reason, you'll never want to do
that. Especially since the very first reason why you want software
support is usually to go beyond your hardware engine capabilities in
terms of strength.

Here is a blog post about such situation, if deemed useful:
https://bootlin.com/blog/supporting-a-misbehaving-nand-ecc-engine/

> > > This works fine currently, but means that NAND_CMD_RNDOUT can't be used.
> > > Using NAND_CMD_RNDOUT to position the cursor at offset 512b for example
> > > doesn't give you the second subpage, but instead oob0. Positioning the
> > > cursor at offset 2048 doesn't give you the start of OOB, but some
> > > position in the middle of data3.
> > > 
> > > Ok, NAND_CMD_RNDOUT can't be used for hardware ECC and there's no way
> > > around it. For software ECC we could change the organisation in the chip
> > > to be [2048b data][64b oob]. With that NAND_CMD_RNDOUT then could be
> > > used with software ECC.
> > > 
> > > You say that NAND_CMD_RNDOUT is a basic command that is supported by all
> > > controllers, and yes, it is also supported with the mxc_nand controller.
> > > You just can't control how many bytes are transferred between the NAND
> > > chip and the controller. When using NAND_CMD_RNDOUT to read a few bytes
> > > at a certain page offset we'll end up reading 512 bytes discarding most
> > > of it. For the next ECC block we would move the cursor forward using
> > > another NAND_CMD_RNDOUT command, again read 512 bytes and discard most
> > > it (altough the desired data would have been in the first read already).  
> > 
> > I'm not sure the controller limitations are so bad in this case. The
> > core helpers (using the same example) will ask for:
> > - 512b at offset 0
> > - 512b at offset 512...
> > - and finally 64b at offset 2048.
> > In practice it does not look like a huge drawback? I don't understand
> > in which case so much data would be read and then discarded?  
> 
> Yes, you're right. I misread the code and thought the core would read
> the ECC separately for each subpage. In fact it doesn't do so, the ECC
> is always read in one go even for multiple subpages.

"interleaved" layouts actually force us to perform so much
sub-readings, that's probably one reason why there is no reason to try
using an interleaved layout with software ECC engines.

> > > So I think NAND_CMD_RNDOUT should really be avoided for this controller,
> > > eventhough we might be able to support it.  
> > 
> > I also mentioned the monolithic accessors which try to avoid these
> > random column changes. You probably want to check them out, they might
> > just avoid the need for NAND_CMD_RNDOUT by forcing full page accesses
> > directly. The reason why they were introduced is not exactly our
> > current use case, but it feels like they might be handy.
> > 
> > 658beb663960 ("mtd: rawnand: Expose monolithic read/write_page_raw() helpers")
> > 0e7f4b64ea46 ("mtd: rawnand: Allow controllers to overload soft ECC hooks")  
> 
> Yes, I already make use of 0e7f4b64ea46. My problem is only the ecc.read_subpage
> hook which can't be overwritten and AFAIK this is the only way
> NAND_CMD_RNDOUT might be used in my case.

It needs to be supported, we don't expect in the core that this command
will not be supported. There may be some constraints and limitations,
and this we can workaround them somehow, but we expect support for
NAND_CMD_RNDOUT.

Look at all users of nand_change_read_column_op(), NAND manufacturer
drivers use it, jedec/onfi drivers use it as well in case of
bitflip in the parameter page, and the core may want to use it (although
in your case I don't think it actually does if you don't try to support
over complex layouts, as software ECC engines will always request the
OOB data whereas subpages are not used in this situation).

> I think my favourite solution would be to:
> 
> - store data/OOB interleaved for both hardware and software ECC
> - For software ECC use a similar OOB layout as used with hardware
>   ECC. This allows us to read a subpage including its ECC data in
>   a single step (just like with hardware ECC the controller just
>   reads 512b + 16b for each subpage)
> - Allow to disable subpage reads in the NAND core
> 
> As a further optimisation we could make ecc.read_subpage overwritable
> for ecc->engine_type == NAND_ECC_ENGINE_TYPE_SOFT && ecc->algo ==
> NAND_ECC_ALGO_BCH. With the OOB layout described above that would be
> easily implementable with the mxc_nand controller.
> 
> What do you think?

I'm sorry but I feel like I need to answer "no" to all three items
above. It would be totally backwards.

> If you insist I would go the path of making NAND_CMD_RNDOUT work for
> software ECC, although I think it would cause me extra work with no
> clear gain for me.

Yes, please. I believe this command is not that complex to implement,
even with strong (and clearly advertised) limitations. I had a look at
your exec_op() implementation and to the datasheet of the imx27, the
controller clearly supports CMD/ADDR/CMD/DATAIN ops. It's true that you
can only request 16, 512 or 528 bytes, but, why not? It just needs
to be clearly identified that reading more data is not supported. Once
the data is in the local SRAM you just take what you need and done.
From a performance perspective, I don't think this operation will be
used often, at least not in the fast path (see above why), but we need
it for the driver to work properly in all situations.

There is perhaps something that is missing in your current
implementation though: there is a check_only boolean in ->exec_op()
which might require additional handling so that the core does not try to
perform unsupported operations. You can do that either manually by
checking the ops entirely by hand if there are only a couple that
cannot be supported, or leverage the core parser otherwise.

In this case you can have a look at the use of the "struct
nand_op_parser" in the subsystem. Please also don't hesitate to take
inspiration from other drivers, as you might need to advertise
limitations such as a maximum number of command, address or
data cycles (in this case, 528 or 512 if it's easier).

Thanks,
Miquèl

Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Sascha,

> > No need, I believe the only reason for interleaving is that your
> > hardware ECC engine works like that (writes the ECC bytes slightly
> > after each chunk of data). So if you don't use on-host hardware ECC,
> > you don't need to deal with this data layout.  
> 
> Right, I could use a different layout for software ECC. Using the same
> layout for both hardware and software ECC is just quite convenient as
> the same mxc_nand_read_page_raw()/mxc_nand_write_page_raw() could be
> used for both software and hardware ECC.

I'm not sure I see why it would be more convenient, as you basically
don't need to provide anything if you use software ECC engines besides
a minimal exec_op() implementation. Anyway, that's clearly not the good
approach for software ECC: the engine decides where it wants to put the
data, there is just no reason to complexify the software layout (which
is free from any constraints).

> Another thing that might be worth considering is that if we use
> different functions for raw read/write page is that we would get
> different views on the same raw page data if we switch from software to
> hardware ECC or the other way round which might be confusing.

Don't worry about that: it's impossible to manage. Data layout might be
different of course, but most importantly once you've chosen an ECC
configuration, any access with another configuration will simply fail.
And I'm not just talking about the ECC/step size parameters, each
engine has it's own base polynomial on which it bases all its internal
calculations, and besides trying very hard to mimic your hardware
engine in software for some very good reason, you'll never want to do
that. Especially since the very first reason why you want software
support is usually to go beyond your hardware engine capabilities in
terms of strength.

Here is a blog post about such situation, if deemed useful:
https://bootlin.com/blog/supporting-a-misbehaving-nand-ecc-engine/

> > > This works fine currently, but means that NAND_CMD_RNDOUT can't be used.
> > > Using NAND_CMD_RNDOUT to position the cursor at offset 512b for example
> > > doesn't give you the second subpage, but instead oob0. Positioning the
> > > cursor at offset 2048 doesn't give you the start of OOB, but some
> > > position in the middle of data3.
> > > 
> > > Ok, NAND_CMD_RNDOUT can't be used for hardware ECC and there's no way
> > > around it. For software ECC we could change the organisation in the chip
> > > to be [2048b data][64b oob]. With that NAND_CMD_RNDOUT then could be
> > > used with software ECC.
> > > 
> > > You say that NAND_CMD_RNDOUT is a basic command that is supported by all
> > > controllers, and yes, it is also supported with the mxc_nand controller.
> > > You just can't control how many bytes are transferred between the NAND
> > > chip and the controller. When using NAND_CMD_RNDOUT to read a few bytes
> > > at a certain page offset we'll end up reading 512 bytes discarding most
> > > of it. For the next ECC block we would move the cursor forward using
> > > another NAND_CMD_RNDOUT command, again read 512 bytes and discard most
> > > it (altough the desired data would have been in the first read already).  
> > 
> > I'm not sure the controller limitations are so bad in this case. The
> > core helpers (using the same example) will ask for:
> > - 512b at offset 0
> > - 512b at offset 512...
> > - and finally 64b at offset 2048.
> > In practice it does not look like a huge drawback? I don't understand
> > in which case so much data would be read and then discarded?  
> 
> Yes, you're right. I misread the code and thought the core would read
> the ECC separately for each subpage. In fact it doesn't do so, the ECC
> is always read in one go even for multiple subpages.

"interleaved" layouts actually force us to perform so much
sub-readings, that's probably one reason why there is no reason to try
using an interleaved layout with software ECC engines.

> > > So I think NAND_CMD_RNDOUT should really be avoided for this controller,
> > > eventhough we might be able to support it.  
> > 
> > I also mentioned the monolithic accessors which try to avoid these
> > random column changes. You probably want to check them out, they might
> > just avoid the need for NAND_CMD_RNDOUT by forcing full page accesses
> > directly. The reason why they were introduced is not exactly our
> > current use case, but it feels like they might be handy.
> > 
> > 658beb663960 ("mtd: rawnand: Expose monolithic read/write_page_raw() helpers")
> > 0e7f4b64ea46 ("mtd: rawnand: Allow controllers to overload soft ECC hooks")  
> 
> Yes, I already make use of 0e7f4b64ea46. My problem is only the ecc.read_subpage
> hook which can't be overwritten and AFAIK this is the only way
> NAND_CMD_RNDOUT might be used in my case.

It needs to be supported, we don't expect in the core that this command
will not be supported. There may be some constraints and limitations,
and this we can workaround them somehow, but we expect support for
NAND_CMD_RNDOUT.

Look at all users of nand_change_read_column_op(), NAND manufacturer
drivers use it, jedec/onfi drivers use it as well in case of
bitflip in the parameter page, and the core may want to use it (although
in your case I don't think it actually does if you don't try to support
over complex layouts, as software ECC engines will always request the
OOB data whereas subpages are not used in this situation).

> I think my favourite solution would be to:
> 
> - store data/OOB interleaved for both hardware and software ECC
> - For software ECC use a similar OOB layout as used with hardware
>   ECC. This allows us to read a subpage including its ECC data in
>   a single step (just like with hardware ECC the controller just
>   reads 512b + 16b for each subpage)
> - Allow to disable subpage reads in the NAND core
> 
> As a further optimisation we could make ecc.read_subpage overwritable
> for ecc->engine_type == NAND_ECC_ENGINE_TYPE_SOFT && ecc->algo ==
> NAND_ECC_ALGO_BCH. With the OOB layout described above that would be
> easily implementable with the mxc_nand controller.
> 
> What do you think?

I'm sorry but I feel like I need to answer "no" to all three items
above. It would be totally backwards.

> If you insist I would go the path of making NAND_CMD_RNDOUT work for
> software ECC, although I think it would cause me extra work with no
> clear gain for me.

Yes, please. I believe this command is not that complex to implement,
even with strong (and clearly advertised) limitations. I had a look at
your exec_op() implementation and to the datasheet of the imx27, the
controller clearly supports CMD/ADDR/CMD/DATAIN ops. It's true that you
can only request 16, 512 or 528 bytes, but, why not? It just needs
to be clearly identified that reading more data is not supported. Once
the data is in the local SRAM you just take what you need and done.
From a performance perspective, I don't think this operation will be
used often, at least not in the fast path (see above why), but we need
it for the driver to work properly in all situations.

There is perhaps something that is missing in your current
implementation though: there is a check_only boolean in ->exec_op()
which might require additional handling so that the core does not try to
perform unsupported operations. You can do that either manually by
checking the ops entirely by hand if there are only a couple that
cannot be supported, or leverage the core parser otherwise.

In this case you can have a look at the use of the "struct
nand_op_parser" in the subsystem. Please also don't hesitate to take
inspiration from other drivers, as you might need to advertise
limitations such as a maximum number of command, address or
data cycles (in this case, 528 or 512 if it's easier).

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/