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

[PATCH v2 0/3] 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.

After discussion with Miquel I have now changed the way the data is
written in software BCH ECC mode. The controller normally interleaves
user data with OOB data in a NAND page. In this version I reversed the
interleaving before writing a page and after reading a page. This way
the data on the NAND chip is arranged in the way the NAND layer expects
it and commands like NAND_CMD_RNDOUT work as expected.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Changes in v2:
- Enable hw ecc only when needed and leave it disabled otherwise
- Use untangled data/oob layout for software BCH ECC
- Link to v1: https://lore.kernel.org/r/20240417-mtd-nand-mxc-nand-exec-op-v1-0-d12564fe54e9@pengutronix.de

---
Sascha Hauer (3):
      mtd: nand: mxc_nand: separate page read from ecc calc
      mtd: nand: mxc_nand: implement exec_op
      mtd: nand: mxc_nand: support software ECC

 drivers/mtd/nand/raw/mxc_nand.c | 651 ++++++++++++++++++----------------------
 1 file changed, 296 insertions(+), 355 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 v2 1/3] 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 v2 2/3] 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 | 449 +++++++++++++---------------------------
 1 file changed, 146 insertions(+), 303 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index 3fe0b471f4a2d..1e8b4553e03ba 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,15 +712,11 @@ 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;
+	if (mtd->writesize)
+		no_subpages = mtd->writesize >> 9;
+	else
+		/* READ PARAMETER PAGE is called when mtd->writesize is not yet set */
+		no_subpages = 1;
 
 	for (i = 0; i < no_subpages; i++) {
 		/* NANDFC buffer 0 is used for page read/write */
@@ -807,181 +733,104 @@ 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 = nand_read_page_op(chip, page, 0, buf, mtd->writesize);
+
+	host->devtype_data->enable_hwecc(chip, false);
 
-	ret = host->devtype_data->read_page(chip, buf, oob_buf, 1, page);
 	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 mxc_nand_host *host = nand_get_controller_data(chip);
-	void *oob_buf;
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	int ret;
+
+	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 mxc_nand_host *host = nand_get_controller_data(chip);
-
-	return host->devtype_data->read_page(chip, NULL, chip->oob_poi, 0,
-					     page);
-}
-
-static int mxc_nand_write_page(struct nand_chip *chip, const uint8_t *buf,
-			       bool ecc, 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, 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);
+	ret = nand_read_page_op(chip, page, 0, host->data_buf, mtd->writesize);
+	if (ret)
+		return ret;
 
-	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);
+	copy_spare(mtd, true, chip->oob_poi);
 
 	return 0;
 }
 
 static int mxc_nand_write_page_ecc(struct nand_chip *chip, const uint8_t *buf,
 				   int oob_required, int page)
-{
-	return mxc_nand_write_page(chip, buf, true, page);
-}
-
-static int mxc_nand_write_page_raw(struct nand_chip *chip, const uint8_t *buf,
-				   int oob_required, int page)
-{
-	return mxc_nand_write_page(chip, buf, false, page);
-}
-
-static int mxc_nand_write_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;
 
-	memset(host->data_buf, 0xff, mtd->writesize);
-
-	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;
+	host->devtype_data->enable_hwecc(chip, true);
 
-	if (nand_chip->options & NAND_BUSWIDTH_16) {
-		/* only take the lower byte of each word */
-		ret = *(uint16_t *)(host->data_buf + host->buf_start);
+	ret = nand_prog_page_op(chip, page, 0, buf, mtd->writesize);
 
-		host->buf_start += 2;
-	} else {
-		ret = *(uint8_t *)(host->data_buf + host->buf_start);
-		host->buf_start++;
-	}
+	host->devtype_data->enable_hwecc(chip, false);
 
-	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)
+static int mxc_nand_write_page_raw(struct nand_chip *chip, const uint8_t *buf,
+				   int oob_required, int page)
 {
-	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);
+	struct mtd_info *mtd = nand_to_mtd(chip);
 
-	host->buf_start += n;
+	return nand_prog_page_op(chip, page, 0, buf, mtd->writesize);
 }
 
-/* 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)
+static int mxc_nand_write_oob(struct nand_chip *chip, int page)
 {
-	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);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
 
-	memcpy(buf, host->data_buf + col, n);
+	memset(host->data_buf, 0xff, mtd->writesize);
 
-	host->buf_start += n;
+	return nand_prog_page_op(chip, page, 0, host->data_buf, mtd->writesize);
 }
 
 /* This function is used by upper layer for select and
@@ -1360,107 +1209,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 +1465,111 @@ 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);
+
+	if (check_only)
+		return 0;
+
+	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:
+			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
+				host->devtype_data->send_page(mtd, NFC_INPUT);
+
+			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);
+			if (chip->oob_poi)
+				copy_spare(mtd, false, chip->oob_poi);
+
+			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 +1602,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 v2 3/3] mtd: nand: mxc_nand: support software ECC

[Sascha Hauer]

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.

The controller normally interleaves user data with OOB data when
accessing the NAND chip. With Software BCH ECC we write the data
to the NAND in a way that the raw data on the NAND chip matches the
way the NAND layer sees it. This way commands like NAND_CMD_RNDOUT
work as expected.

This was tested on i.MX27 but should work on the other SoCs supported
by this driver as well.

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

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index 1e8b4553e03ba..7004fd57c80f7 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -178,6 +178,7 @@ struct mxc_nand_host {
 	int			used_oobsize;
 	int			active_cs;
 	unsigned int		ecc_stats_v1;
+	unsigned int		column;
 
 	struct completion	op_completion;
 
@@ -1397,10 +1398,10 @@ 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);
 
 	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;
@@ -1465,6 +1466,54 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
 	return host->devtype_data->setup_interface(chip, chipnr, conf);
 }
 
+static void copy_page_to_sram(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(this);
+	void *buf = host->data_buf;
+	unsigned int n_subpages = mtd->writesize / 512;
+	int oob_per_subpage, i;
+
+	/* mtd->writesize is not set during ident scanning */
+	if (!n_subpages)
+		n_subpages = 1;
+
+	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
+
+	for (i = 0; i < n_subpages; i++) {
+		memcpy16_toio(host->main_area0 + i * 512, buf, 512);
+		buf += 512;
+
+		memcpy16_toio(host->spare0 + i * host->devtype_data->spare_len, buf,
+			      oob_per_subpage);
+		buf += oob_per_subpage;
+	}
+}
+
+static void copy_page_from_sram(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(this);
+	void *buf = host->data_buf;
+	unsigned int n_subpages = mtd->writesize / 512;
+	int oob_per_subpage, i;
+
+	/* mtd->writesize is not set during ident scanning */
+	if (!n_subpages)
+		n_subpages = 1;
+
+	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
+
+	for (i = 0; i < n_subpages; i++) {
+		memcpy16_fromio(buf, host->main_area0 + i * 512, 512);
+		buf += 512;
+
+		memcpy16_fromio(buf, host->spare0 + i * host->devtype_data->spare_len,
+				oob_per_subpage);
+		buf += oob_per_subpage;
+	}
+}
+
 static int mxcnd_exec_op(struct nand_chip *chip,
 			 const struct nand_operation *op,
 			 bool check_only)
@@ -1496,8 +1545,11 @@ static int mxcnd_exec_op(struct nand_chip *chip,
 			 */
 			break;
 		case NAND_OP_CMD_INSTR:
-			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
+			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG) {
+				if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST)
+					copy_page_to_sram(mtd);
 				host->devtype_data->send_page(mtd, NFC_INPUT);
+			}
 
 			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
 
@@ -1506,6 +1558,8 @@ static int mxcnd_exec_op(struct nand_chip *chip,
 			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
 				statusreq = true;
 
+			host->column = 0;
+
 			break;
 		case NAND_OP_ADDR_INSTR:
 			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
@@ -1517,9 +1571,14 @@ static int mxcnd_exec_op(struct nand_chip *chip,
 			buf_write = instr->ctx.data.buf.out;
 			buf_len = instr->ctx.data.len;
 
-			memcpy32_toio(host->main_area0, buf_write, buf_len);
-			if (chip->oob_poi)
-				copy_spare(mtd, false, chip->oob_poi);
+			if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
+				memcpy32_toio(host->main_area0, buf_write, buf_len);
+				if (chip->oob_poi)
+					copy_spare(mtd, false, chip->oob_poi);
+			} else {
+				memcpy(host->data_buf + host->column, buf_write, buf_len);
+				host->column += buf_len;
+			}
 
 			break;
 		case NAND_OP_DATA_IN_INSTR:
@@ -1552,11 +1611,18 @@ static int mxcnd_exec_op(struct nand_chip *chip,
 
 			host->devtype_data->read_page(chip);
 
-			if (IS_ALIGNED(buf_len, 4)) {
-				memcpy32_fromio(buf_read, host->main_area0, buf_len);
+			if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
+				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);
+				}
 			} else {
-				memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
-				memcpy(buf_read, host->data_buf, buf_len);
+				if (!host->column)
+					copy_page_from_sram(mtd);
+				memcpy(buf_read, host->data_buf + host->column, buf_len);
+				host->column += buf_len;
 			}
 
 			break;

-- 
2.39.2


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

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

[Miquel Raynal]

Hi Sascha,

> @@ -1717,9 +1465,111 @@ 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);

Maybe you want to get rid of this debug line.

> +
> +	if (check_only)
> +		return 0;
> +
> +	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.

This is no longer a standalone function, maybe:

"The controller handles the R/B pin internally, therefore there is
nothing to do here."

> +			 */
> +			break;
> +		case NAND_OP_CMD_INSTR:
> +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
> +				host->devtype_data->send_page(mtd, NFC_INPUT);
> +
> +			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);
> +			if (chip->oob_poi)
> +				copy_spare(mtd, false, chip->oob_poi);

This copy should not be needed. It should be in your page accessors if
needed.

> +
> +			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;
> +}

Otherwise I'm very happy with the look.

Thanks,
Miquèl

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

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

[Miquel Raynal]

miquel.raynal@bootlin.com wrote on Mon, 13 May 2024 09:19:02 +0200:

> Hi Sascha,
> 
> > @@ -1717,9 +1465,111 @@ 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);  
> 
> Maybe you want to get rid of this debug line.
> 
> > +
> > +	if (check_only)
> > +		return 0;
> > +
> > +	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.  
> 
> This is no longer a standalone function, maybe:
> 
> "The controller handles the R/B pin internally, therefore there is
> nothing to do here."

And this is actually very wrong.

You should call wait_op_done() instead.

> 
> > +			 */
> > +			break;
> > +		case NAND_OP_CMD_INSTR:
> > +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
> > +				host->devtype_data->send_page(mtd, NFC_INPUT);

Actually this is not the right place. You should trigger the transfer
from controller SRAM to NAND (and the other way around) in the
NAND_OP_DATA_OUT_INSTR case.

Here you should just call ->send_cmd.

> > +
> > +			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);
> > +			if (chip->oob_poi)
> > +				copy_spare(mtd, false, chip->oob_poi);  
> 
> This copy should not be needed. It should be in your page accessors if
> needed.
> 
> > +
> > +			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;
> > +}  
> 
> Otherwise I'm very happy with the look.
> 
> Thanks,
> Miquèl


Thanks,
Miquèl

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

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

[Miquel Raynal]

Hi Sascha,

s.hauer@pengutronix.de wrote on Wed, 08 May 2024 13:08:29 +0200:

> 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.
> 
> The controller normally interleaves user data with OOB data when
> accessing the NAND chip. With Software BCH ECC we write the data
> to the NAND in a way that the raw data on the NAND chip matches the
> way the NAND layer sees it. This way commands like NAND_CMD_RNDOUT
> work as expected.
> 
> This was tested on i.MX27 but should work on the other SoCs supported
> by this driver as well.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/raw/mxc_nand.c | 84 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> index 1e8b4553e03ba..7004fd57c80f7 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -178,6 +178,7 @@ struct mxc_nand_host {
>  	int			used_oobsize;
>  	int			active_cs;
>  	unsigned int		ecc_stats_v1;
> +	unsigned int		column;
>  
>  	struct completion	op_completion;
>  
> @@ -1397,10 +1398,10 @@ 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);
>  
>  	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;
> @@ -1465,6 +1466,54 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
>  	return host->devtype_data->setup_interface(chip, chipnr, conf);
>  }
>  
> +static void copy_page_to_sram(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(this);
> +	void *buf = host->data_buf;
> +	unsigned int n_subpages = mtd->writesize / 512;
> +	int oob_per_subpage, i;
> +
> +	/* mtd->writesize is not set during ident scanning */
> +	if (!n_subpages)
> +		n_subpages = 1;
> +
> +	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
> +
> +	for (i = 0; i < n_subpages; i++) {
> +		memcpy16_toio(host->main_area0 + i * 512, buf, 512);
> +		buf += 512;
> +
> +		memcpy16_toio(host->spare0 + i * host->devtype_data->spare_len, buf,
> +			      oob_per_subpage);
> +		buf += oob_per_subpage;
> +	}
> +}
> +
> +static void copy_page_from_sram(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(this);
> +	void *buf = host->data_buf;
> +	unsigned int n_subpages = mtd->writesize / 512;
> +	int oob_per_subpage, i;
> +
> +	/* mtd->writesize is not set during ident scanning */
> +	if (!n_subpages)
> +		n_subpages = 1;
> +
> +	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
> +
> +	for (i = 0; i < n_subpages; i++) {
> +		memcpy16_fromio(buf, host->main_area0 + i * 512, 512);
> +		buf += 512;
> +
> +		memcpy16_fromio(buf, host->spare0 + i * host->devtype_data->spare_len,
> +				oob_per_subpage);
> +		buf += oob_per_subpage;
> +	}
> +}
> +
>  static int mxcnd_exec_op(struct nand_chip *chip,
>  			 const struct nand_operation *op,
>  			 bool check_only)
> @@ -1496,8 +1545,11 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  			 */
>  			break;
>  		case NAND_OP_CMD_INSTR:
> -			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
> +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG) {
> +				if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST)
> +					copy_page_to_sram(mtd);
>  				host->devtype_data->send_page(mtd, NFC_INPUT);
> +			}

Same as before: data moves should not happen here.

>  
>  			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
>  
> @@ -1506,6 +1558,8 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
>  				statusreq = true;
>  
> +			host->column = 0;
> +

This is risky but maybe this is the trick to get NAND_CMD_RNDOUT
working? If yes maybe it is worth separating from this patchset, if
possible, with a comment explaining what this is all about (the
controller's SRAM being some kind of 1:1 mapping with the NAND SRAM,
thus we need to write the data at the correct offset in the controller
SRAM, I guess?).

Please write the comment above when defining the column field, and
also, if my understanding is correct, can we rename it to sram_column
or something like that?

>  			break;
>  		case NAND_OP_ADDR_INSTR:
>  			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
> @@ -1517,9 +1571,14 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  			buf_write = instr->ctx.data.buf.out;
>  			buf_len = instr->ctx.data.len;
>  
> -			memcpy32_toio(host->main_area0, buf_write, buf_len);
> -			if (chip->oob_poi)
> -				copy_spare(mtd, false, chip->oob_poi);
> +			if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
> +				memcpy32_toio(host->main_area0, buf_write, buf_len);
> +				if (chip->oob_poi)
> +					copy_spare(mtd, false, chip->oob_poi);
> +			} else {
> +				memcpy(host->data_buf + host->column, buf_write, buf_len);
> +				host->column += buf_len;
> +			}
>  
>  			break;
>  		case NAND_OP_DATA_IN_INSTR:
> @@ -1552,11 +1611,18 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  
>  			host->devtype_data->read_page(chip);
>  
> -			if (IS_ALIGNED(buf_len, 4)) {
> -				memcpy32_fromio(buf_read, host->main_area0, buf_len);
> +			if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
> +				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);
> +				}
>  			} else {
> -				memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
> -				memcpy(buf_read, host->data_buf, buf_len);
> +				if (!host->column)
> +					copy_page_from_sram(mtd);
> +				memcpy(buf_read, host->data_buf + host->column, buf_len);
> +				host->column += buf_len;
>  			}
>  
>  			break;
> 


Thanks,
Miquèl

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

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

[Sascha Hauer]

Hi Miquel,

On Mon, May 13, 2024 at 09:32:56AM +0200, Miquel Raynal wrote:
> 
> miquel.raynal@bootlin.com wrote on Mon, 13 May 2024 09:19:02 +0200:
> 
> > Hi Sascha,
> > 
> > > @@ -1717,9 +1465,111 @@ 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);  
> > 
> > Maybe you want to get rid of this debug line.
> > 
> > > +
> > > +	if (check_only)
> > > +		return 0;
> > > +
> > > +	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.  
> > 
> > This is no longer a standalone function, maybe:
> > 
> > "The controller handles the R/B pin internally, therefore there is
> > nothing to do here."
> 
> And this is actually very wrong.
> 
> You should call wait_op_done() instead.
> 
> > 
> > > +			 */
> > > +			break;
> > > +		case NAND_OP_CMD_INSTR:
> > > +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
> > > +				host->devtype_data->send_page(mtd, NFC_INPUT);
> 
> Actually this is not the right place. You should trigger the transfer
> from controller SRAM to NAND (and the other way around) in the
> NAND_OP_DATA_OUT_INSTR case.
> 
> Here you should just call ->send_cmd.

I tried to get away here with using the standard nand_write_page_raw()
function. This does multiple NAND_OP_DATA_OUT_INSTR ops before finally
sending a NAND_CMD_PAGEPROG command. With software BCH ECC I collect all
data being written in memory and copy it to the controller SRAM en bloc.
I have to do this after the final NAND_OP_DATA_OUT_INSTR op. During a
NAND_OP_DATA_OUT_INSTR I don't know if this is going to be the last one
or if other NAND_OP_DATA_OUT_INSTR ops follow, so I do the copy to SRAM
right before a NAND_CMD_PAGEPROG.

I could move the ->send_page() call to NAND_OP_DATA_OUT_INSTR, but then
I have to overwrite the ecc->write_page_raw() hook. Also it must be
clear that the NAND core will never do multiple NAND_OP_DATA_OUT_INSTR
when programming a page.

Side note: I decided to collect the page data in a memory buffer rather
than in the controller SRAM directly because it seemed too complicated
and error prone to find the correct offset in SRAM for random column
addresses. Also the SRAM can only do word and halfword accesses, so I
additionally would have to emulate byte accesses with read-modify-write
halfword accesses. While certainly doable I'd like to defer this to a
future optimisation exercise.

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 v2 3/3] mtd: nand: mxc_nand: support software ECC

[Sascha Hauer]

On Mon, May 13, 2024 at 09:42:17AM +0200, Miquel Raynal wrote:
> Hi Sascha,
> 
> s.hauer@pengutronix.de wrote on Wed, 08 May 2024 13:08:29 +0200:
> 
> > 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.
> > 
> > The controller normally interleaves user data with OOB data when
> > accessing the NAND chip. With Software BCH ECC we write the data
> > to the NAND in a way that the raw data on the NAND chip matches the
> > way the NAND layer sees it. This way commands like NAND_CMD_RNDOUT
> > work as expected.
> > 
> > This was tested on i.MX27 but should work on the other SoCs supported
> > by this driver as well.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/nand/raw/mxc_nand.c | 84 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 75 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > index 1e8b4553e03ba..7004fd57c80f7 100644
> > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > @@ -178,6 +178,7 @@ struct mxc_nand_host {
> >  	int			used_oobsize;
> >  	int			active_cs;
> >  	unsigned int		ecc_stats_v1;
> > +	unsigned int		column;
> >  
> >  	struct completion	op_completion;
> >  
> > @@ -1397,10 +1398,10 @@ 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);
> >  
> >  	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;
> > @@ -1465,6 +1466,54 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
> >  	return host->devtype_data->setup_interface(chip, chipnr, conf);
> >  }
> >  
> > +static void copy_page_to_sram(struct mtd_info *mtd)
> > +{
> > +	struct nand_chip *this = mtd_to_nand(mtd);
> > +	struct mxc_nand_host *host = nand_get_controller_data(this);
> > +	void *buf = host->data_buf;
> > +	unsigned int n_subpages = mtd->writesize / 512;
> > +	int oob_per_subpage, i;
> > +
> > +	/* mtd->writesize is not set during ident scanning */
> > +	if (!n_subpages)
> > +		n_subpages = 1;
> > +
> > +	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
> > +
> > +	for (i = 0; i < n_subpages; i++) {
> > +		memcpy16_toio(host->main_area0 + i * 512, buf, 512);
> > +		buf += 512;
> > +
> > +		memcpy16_toio(host->spare0 + i * host->devtype_data->spare_len, buf,
> > +			      oob_per_subpage);
> > +		buf += oob_per_subpage;
> > +	}
> > +}
> > +
> > +static void copy_page_from_sram(struct mtd_info *mtd)
> > +{
> > +	struct nand_chip *this = mtd_to_nand(mtd);
> > +	struct mxc_nand_host *host = nand_get_controller_data(this);
> > +	void *buf = host->data_buf;
> > +	unsigned int n_subpages = mtd->writesize / 512;
> > +	int oob_per_subpage, i;
> > +
> > +	/* mtd->writesize is not set during ident scanning */
> > +	if (!n_subpages)
> > +		n_subpages = 1;
> > +
> > +	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
> > +
> > +	for (i = 0; i < n_subpages; i++) {
> > +		memcpy16_fromio(buf, host->main_area0 + i * 512, 512);
> > +		buf += 512;
> > +
> > +		memcpy16_fromio(buf, host->spare0 + i * host->devtype_data->spare_len,
> > +				oob_per_subpage);
> > +		buf += oob_per_subpage;
> > +	}
> > +}
> > +
> >  static int mxcnd_exec_op(struct nand_chip *chip,
> >  			 const struct nand_operation *op,
> >  			 bool check_only)
> > @@ -1496,8 +1545,11 @@ static int mxcnd_exec_op(struct nand_chip *chip,
> >  			 */
> >  			break;
> >  		case NAND_OP_CMD_INSTR:
> > -			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
> > +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG) {
> > +				if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST)
> > +					copy_page_to_sram(mtd);
> >  				host->devtype_data->send_page(mtd, NFC_INPUT);
> > +			}
> 
> Same as before: data moves should not happen here.
> 
> >  
> >  			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
> >  
> > @@ -1506,6 +1558,8 @@ static int mxcnd_exec_op(struct nand_chip *chip,
> >  			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
> >  				statusreq = true;
> >  
> > +			host->column = 0;
> > +
> 
> This is risky but maybe this is the trick to get NAND_CMD_RNDOUT
> working? If yes maybe it is worth separating from this patchset, if
> possible, with a comment explaining what this is all about (the
> controller's SRAM being some kind of 1:1 mapping with the NAND SRAM,
> thus we need to write the data at the correct offset in the controller
> SRAM, I guess?).

I don't think it's possible to separate introducing the host->column
variable to a separate patch, because this variable is only used with
NAND_ECC_ENGINE_TYPE_SOFT and on the other hand NAND_ECC_ENGINE_TYPE_SOFT
only works with this variable.

Anyway, adding a description what this is all about is a good idea and
I'll do this. In fact I already tried to add one when I found out that
it's really hard to put it into words, so I sent without 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 v2 2/3] mtd: nand: mxc_nand: implement exec_op

[Miquel Raynal]

Hi Sascha,

> > > > +	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.    
> > > 
> > > This is no longer a standalone function, maybe:
> > > 
> > > "The controller handles the R/B pin internally, therefore there is
> > > nothing to do here."  
> > 
> > And this is actually very wrong.
> > 
> > You should call wait_op_done() instead.
> >   
> > >   
> > > > +			 */
> > > > +			break;
> > > > +		case NAND_OP_CMD_INSTR:
> > > > +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG)
> > > > +				host->devtype_data->send_page(mtd, NFC_INPUT);  
> > 
> > Actually this is not the right place. You should trigger the transfer
> > from controller SRAM to NAND (and the other way around) in the
> > NAND_OP_DATA_OUT_INSTR case.
> > 
> > Here you should just call ->send_cmd.  
> 
> I tried to get away here with using the standard nand_write_page_raw()
> function. This does multiple NAND_OP_DATA_OUT_INSTR ops before finally
> sending a NAND_CMD_PAGEPROG command. With software BCH ECC I collect all
> data being written in memory and copy it to the controller SRAM en bloc.
> I have to do this after the final NAND_OP_DATA_OUT_INSTR op. During a
> NAND_OP_DATA_OUT_INSTR I don't know if this is going to be the last one
> or if other NAND_OP_DATA_OUT_INSTR ops follow, so I do the copy to SRAM
> right before a NAND_CMD_PAGEPROG.

I fully understand the mindset, but we had too many issues with this
this using ->cmdfunc() and ->cmd_ctrl(), so we introduced ->exec_op()
exactly for this purpose: controller drivers shall not try to guess
what the core wants. The core now provides the full operation and also a
way to check if the operation is supported.

So if something is unsupported, just report it is unsupported.

> I could move the ->send_page() call to NAND_OP_DATA_OUT_INSTR, but then
> I have to overwrite the ecc->write_page_raw() hook.

I guess your issue is the core trying to write the in-band data
and later the oob data. I totally get why you're doing that now, makes
sense, but this is not how ->exec_op() should be implemented, let me
explain (and propose a solution).

In general, it is preferred to have a clean ->exec_op()
implementation (in case we later extend the core) that is capable of
doing the ops correctly and *just* what has been requested. Drivers
should refuse the operations they don't support instead of trying
to work-arouding them. So here the correct way is probably to provide a
->write_page_raw() hook to simply avoid the unsupported ops. In your
case you just need to reference nand_monolithic_write_page_raw() and
you're done.

Also please check the cases you don't support and refuse them in
->exec_op() (both in the normal *and* in the check_only path). This is
also part of ->exec_op()'s design and is very important. I believe you
should take inspiration from the pl35x-nand-controller.c driver which
typically covers all the known-to-be-working cases with just three
patterns filled in a parser structure (true/false means the instruction
is mandatory or can be skipped, please have a look to
pl35x_nandc_op_parser). You will notice all cases in the parser
reference the same function in the end (you can do the same). This way
you know only supported patterns will be used and the core can easily
check whether something "new" is supported or not. Right now the
current mxc ->exec_op() implementation says it supports all
instructions, which is of course a lie.

Side note: Core helpers were initially written with the more flexible
approach, ie. splitting the operations in smaller pieces. This
typically does not work with some controllers such as yours. What I
want is the driver telling the core it should not try to split the ops
if that's the case. I know the core is a bit flacky regarding these
checks. There is currently only the data_only_read supported_op check
that is used for early discovery (see nand_onfi_detect()), but in case
you have issues with this type of situation, let me know, we (I) can
improve the core.

> Also it must be
> clear that the NAND core will never do multiple NAND_OP_DATA_OUT_INSTR
> when programming a page.

That's typically something that could happen (in practice I don't have
any plans for that, this is theoretically speaking), hence using the
nand_op_parser structure will ensure if we ever extend the core, the
driver won't break.

> Side note: I decided to collect the page data in a memory buffer rather
> than in the controller SRAM directly because it seemed too complicated
> and error prone to find the correct offset in SRAM for random column
> addresses. Also the SRAM can only do word and halfword accesses, so I
> additionally would have to emulate byte accesses with read-modify-write
> halfword accesses. While certainly doable I'd like to defer this to a
> future optimisation exercise.

Oh yeah, totally agreed. I saw the implementation and I agree we should
aim for a working driver. Then if you want to improve it for
performance reasons, changes will be welcome.

Thanks,
Miquèl

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

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

[Sascha Hauer]

On Mon, May 13, 2024 at 09:32:56AM +0200, Miquel Raynal wrote:
> 
> miquel.raynal@bootlin.com wrote on Mon, 13 May 2024 09:19:02 +0200:
> 
> > Hi Sascha,
> > 
> > > @@ -1717,9 +1465,111 @@ 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);  
> > 
> > Maybe you want to get rid of this debug line.

Ok.

> > 
> > > +
> > > +	if (check_only)
> > > +		return 0;
> > > +
> > > +	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.  
> > 
> > This is no longer a standalone function, maybe:
> > 
> > "The controller handles the R/B pin internally, therefore there is
> > nothing to do here."

Ok.

> 
> And this is actually very wrong.
> 
> You should call wait_op_done() instead.

No, I don't think so. wait_op_done() is called to wait for the interrupt
of the controller indicating a basic operation is done. A basic operation
can be that a command is being sent or an address byte has been sent to
the chip during an address cycle.

With this arbitrary example:

	struct nand_op_instr instrs[] = {
		NAND_OP_CMD(NAND_CMD_READ0, 0),
		NAND_OP_ADDR(4, addrs, 0),
		NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
		NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
				 NAND_COMMON_TIMING_NS(conf, tRR_min)),
		NAND_OP_DATA_IN(len, buf, 0),
	};

I'll call wait_op_done() once for NAND_CMD_READ0, four times for the NAND_OP_ADDR,
then once again for the NAND_CMD_READSTART command and four times (on i.MX27, once
per subpage) for the NAND_OP_DATA_IN operation. Calling wait_op_done() for the
NAND_OP_WAIT_RDY operation woul only timeout because there is no operation in flight
currently.

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 v2 2/3] mtd: nand: mxc_nand: implement exec_op

[Miquel Raynal]

Hi Sascha,

> > > > +		case NAND_OP_WAITRDY_INSTR:
> > > > +			/*
> > > > +			 * NFC handles R/B internally. Therefore, this function
> > > > +			 * always returns status as ready.    
> > > 
> > > This is no longer a standalone function, maybe:
> > > 
> > > "The controller handles the R/B pin internally, therefore there is
> > > nothing to do here."  
> 
> Ok.
> 
> > 
> > And this is actually very wrong.
> > 
> > You should call wait_op_done() instead.  
> 
> No, I don't think so. wait_op_done() is called to wait for the interrupt
> of the controller indicating a basic operation is done. A basic operation
> can be that a command is being sent or an address byte has been sent to
> the chip during an address cycle.
> 
> With this arbitrary example:
> 
> 	struct nand_op_instr instrs[] = {
> 		NAND_OP_CMD(NAND_CMD_READ0, 0),
> 		NAND_OP_ADDR(4, addrs, 0),
> 		NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
> 		NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
> 				 NAND_COMMON_TIMING_NS(conf, tRR_min)),
> 		NAND_OP_DATA_IN(len, buf, 0),
> 	};
> 
> I'll call wait_op_done() once for NAND_CMD_READ0, four times for the NAND_OP_ADDR,
> then once again for the NAND_CMD_READSTART command and four times (on i.MX27, once
> per subpage) for the NAND_OP_DATA_IN operation. Calling wait_op_done() for the
> NAND_OP_WAIT_RDY operation woul only timeout because there is no operation in flight
> currently.

Ah, ok, makes sense as well. If you want (this is not mandatory) you
may want to rename the function wait_instr_done() to fit the core's
naming: an operation being an aggregate of instructions/cycles.

Thanks,
Miquèl

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