[PATCH 0/4] mtd: nand: omap2: Switch to exec_ops, support AM64 SoC

[PATCH 0/4] mtd: nand: omap2: Switch to exec_ops, support AM64 SoC

[Roger Quadros]

Hi,

This series gets rid of the Legacy ops and switches to exec_ops framework.

Also add driver support for TI's AM64 SoC which contains a GPMC NAND controller.

cheers,
-roger

Roger Quadros (4):
  dt-bindings: mtd: ti,gpmc-nand: Add compatible for AM64 NAND
  mtd: nand: omap2: Allow build on K3 platforms
  mtd: nand: omap2: move to exec_op interface
  mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

 .../devicetree/bindings/mtd/ti,gpmc-nand.yaml |   5 +-
 drivers/mtd/nand/raw/Kconfig                  |   2 +-
 drivers/mtd/nand/raw/omap2.c                  | 525 ++++++++----------
 3 files changed, 251 insertions(+), 281 deletions(-)

-- 
2.17.1

[PATCH 1/4] dt-bindings: mtd: ti,gpmc-nand: Add compatible for AM64 NAND

[Roger Quadros]

AM64 SoC contains the GPMC NAND controller. Add compatible for it.

Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 Documentation/devicetree/bindings/mtd/ti,gpmc-nand.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/ti,gpmc-nand.yaml b/Documentation/devicetree/bindings/mtd/ti,gpmc-nand.yaml
index beb26b9bcfb2..666ae4a78544 100644
--- a/Documentation/devicetree/bindings/mtd/ti,gpmc-nand.yaml
+++ b/Documentation/devicetree/bindings/mtd/ti,gpmc-nand.yaml
@@ -16,7 +16,10 @@ description:
 
 properties:
   compatible:
-    const: ti,omap2-nand
+    items:
+      - enum:
+          - ti,am64-nand
+          - ti,omap2-nand
 
   reg:
     maxItems: 1
-- 
2.17.1

[PATCH 2/4] mtd: nand: omap2: Allow build on K3 platforms

[Roger Quadros]

K3 platforms come with GPMC. Enable GPMC build for
K3 platforms.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/mtd/nand/raw/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index 67b7cb67c030..d719316467a1 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -40,7 +40,7 @@ config MTD_NAND_AMS_DELTA
 
 config MTD_NAND_OMAP2
 	tristate "OMAP2, OMAP3, OMAP4 and Keystone NAND controller"
-	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || COMPILE_TEST
+	depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
 	depends on HAS_IOMEM
 	help
 	  Support for NAND flash on Texas Instruments OMAP2, OMAP3, OMAP4
-- 
2.17.1

[PATCH 3/4] mtd: nand: omap2: move to exec_op interface

[Roger Quadros]

Stop using legacy interface and move to the exec_op interface.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/mtd/nand/raw/omap2.c | 490 +++++++++++++++--------------------
 1 file changed, 211 insertions(+), 279 deletions(-)

diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
index b26d4947af02..f1fc146e09b9 100644
--- a/drivers/mtd/nand/raw/omap2.c
+++ b/drivers/mtd/nand/raw/omap2.c
@@ -19,7 +19,7 @@
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/omap-dma.h>
-#include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -164,6 +164,7 @@ struct omap_nand_info {
 	u_char				*buf;
 	int					buf_len;
 	/* Interface to GPMC */
+	void __iomem			*fifo;
 	struct gpmc_nand_regs		reg;
 	struct gpmc_nand_ops		*ops;
 	bool				flash_bbt;
@@ -175,6 +176,11 @@ struct omap_nand_info {
 	unsigned int			nsteps_per_eccpg;
 	unsigned int			eccpg_size;
 	unsigned int			eccpg_bytes;
+	void (*data_in)(struct nand_chip *chip, void *buf,
+			unsigned int len, bool force_8bit);
+	void (*data_out)(struct nand_chip *chip,
+			 const void *buf, unsigned int len,
+			 bool force_8bit);
 };
 
 static inline struct omap_nand_info *mtd_to_omap(struct mtd_info *mtd)
@@ -182,6 +188,13 @@ static inline struct omap_nand_info *mtd_to_omap(struct mtd_info *mtd)
 	return container_of(mtd_to_nand(mtd), struct omap_nand_info, nand);
 }
 
+static void omap_nand_data_in(struct nand_chip *chip, void *buf,
+			      unsigned int len, bool force_8bit);
+
+static void omap_nand_data_out(struct nand_chip *chip,
+			       const void *buf, unsigned int len,
+			       bool force_8bit);
+
 /**
  * omap_prefetch_enable - configures and starts prefetch transfer
  * @cs: cs (chip select) number
@@ -241,169 +254,70 @@ static int omap_prefetch_reset(int cs, struct omap_nand_info *info)
 }
 
 /**
- * omap_hwcontrol - hardware specific access to control-lines
- * @chip: NAND chip object
- * @cmd: command to device
- * @ctrl:
- * NAND_NCE: bit 0 -> don't care
- * NAND_CLE: bit 1 -> Command Latch
- * NAND_ALE: bit 2 -> Address Latch
- *
- * NOTE: boards may use different bits for these!!
+ * omap_nand_data_in_pref - NAND data in using prefetch Prefetch engine
  */
-static void omap_hwcontrol(struct nand_chip *chip, int cmd, unsigned int ctrl)
+static void omap_nand_data_in_pref(struct nand_chip *chip, void *buf,
+				   unsigned int len, bool force_8bit)
 {
 	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
-
-	if (cmd != NAND_CMD_NONE) {
-		if (ctrl & NAND_CLE)
-			writeb(cmd, info->reg.gpmc_nand_command);
-
-		else if (ctrl & NAND_ALE)
-			writeb(cmd, info->reg.gpmc_nand_address);
-
-		else /* NAND_NCE */
-			writeb(cmd, info->reg.gpmc_nand_data);
-	}
-}
-
-/**
- * omap_read_buf8 - read data from NAND controller into buffer
- * @mtd: MTD device structure
- * @buf: buffer to store date
- * @len: number of bytes to read
- */
-static void omap_read_buf8(struct mtd_info *mtd, u_char *buf, int len)
-{
-	struct nand_chip *nand = mtd_to_nand(mtd);
-
-	ioread8_rep(nand->legacy.IO_ADDR_R, buf, len);
-}
-
-/**
- * omap_write_buf8 - write buffer to NAND controller
- * @mtd: MTD device structure
- * @buf: data buffer
- * @len: number of bytes to write
- */
-static void omap_write_buf8(struct mtd_info *mtd, const u_char *buf, int len)
-{
-	struct omap_nand_info *info = mtd_to_omap(mtd);
-	u_char *p = (u_char *)buf;
-	bool status;
-
-	while (len--) {
-		iowrite8(*p++, info->nand.legacy.IO_ADDR_W);
-		/* wait until buffer is available for write */
-		do {
-			status = info->ops->nand_writebuffer_empty();
-		} while (!status);
-	}
-}
-
-/**
- * omap_read_buf16 - read data from NAND controller into buffer
- * @mtd: MTD device structure
- * @buf: buffer to store date
- * @len: number of bytes to read
- */
-static void omap_read_buf16(struct mtd_info *mtd, u_char *buf, int len)
-{
-	struct nand_chip *nand = mtd_to_nand(mtd);
-
-	ioread16_rep(nand->legacy.IO_ADDR_R, buf, len / 2);
-}
-
-/**
- * omap_write_buf16 - write buffer to NAND controller
- * @mtd: MTD device structure
- * @buf: data buffer
- * @len: number of bytes to write
- */
-static void omap_write_buf16(struct mtd_info *mtd, const u_char * buf, int len)
-{
-	struct omap_nand_info *info = mtd_to_omap(mtd);
-	u16 *p = (u16 *) buf;
-	bool status;
-	/* FIXME try bursts of writesw() or DMA ... */
-	len >>= 1;
-
-	while (len--) {
-		iowrite16(*p++, info->nand.legacy.IO_ADDR_W);
-		/* wait until buffer is available for write */
-		do {
-			status = info->ops->nand_writebuffer_empty();
-		} while (!status);
-	}
-}
-
-/**
- * omap_read_buf_pref - read data from NAND controller into buffer
- * @chip: NAND chip object
- * @buf: buffer to store date
- * @len: number of bytes to read
- */
-static void omap_read_buf_pref(struct nand_chip *chip, u_char *buf, int len)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct omap_nand_info *info = mtd_to_omap(mtd);
 	uint32_t r_count = 0;
 	int ret = 0;
 	u32 *p = (u32 *)buf;
+	unsigned int pref_len;
 
-	/* take care of subpage reads */
-	if (len % 4) {
-		if (info->nand.options & NAND_BUSWIDTH_16)
-			omap_read_buf16(mtd, buf, len % 4);
-		else
-			omap_read_buf8(mtd, buf, len % 4);
-		p = (u32 *) (buf + len % 4);
-		len -= len % 4;
+	if (force_8bit) {
+		omap_nand_data_in(chip, buf, len, force_8bit);
+		return;
 	}
 
+	/* read 32-bit words using prefetch and remaining bytes normally */
+
 	/* configure and start prefetch transfer */
+	pref_len = len - (len & 3);
 	ret = omap_prefetch_enable(info->gpmc_cs,
-			PREFETCH_FIFOTHRESHOLD_MAX, 0x0, len, 0x0, info);
+			PREFETCH_FIFOTHRESHOLD_MAX, 0x0, pref_len, 0x0, info);
 	if (ret) {
-		/* PFPW engine is busy, use cpu copy method */
-		if (info->nand.options & NAND_BUSWIDTH_16)
-			omap_read_buf16(mtd, (u_char *)p, len);
-		else
-			omap_read_buf8(mtd, (u_char *)p, len);
+		/* prefetch engine is busy, use CPU copy method */
+		omap_nand_data_in(chip, buf, len, false);
 	} else {
 		do {
 			r_count = readl(info->reg.gpmc_prefetch_status);
 			r_count = PREFETCH_STATUS_FIFO_CNT(r_count);
 			r_count = r_count >> 2;
-			ioread32_rep(info->nand.legacy.IO_ADDR_R, p, r_count);
+			ioread32_rep(info->fifo, p, r_count);
 			p += r_count;
-			len -= r_count << 2;
-		} while (len);
-		/* disable and stop the PFPW engine */
+			pref_len -= r_count << 2;
+		} while (pref_len);
+		/* disable and stop the Prefetch engine */
 		omap_prefetch_reset(info->gpmc_cs, info);
+		/* fetch any remaining bytes */
+		if (len & 3)
+			omap_nand_data_in(chip, p, len & 3, false);
 	}
 }
 
 /**
- * omap_write_buf_pref - write buffer to NAND controller
- * @chip: NAND chip object
- * @buf: data buffer
- * @len: number of bytes to write
+ * omap_nand_data_out_pref - NAND data out using Write Posting engine
  */
-static void omap_write_buf_pref(struct nand_chip *chip, const u_char *buf,
-				int len)
+static void omap_nand_data_out_pref(struct nand_chip *chip,
+				    const void *buf, unsigned int len,
+				    bool force_8bit)
 {
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct omap_nand_info *info = mtd_to_omap(mtd);
+	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
 	uint32_t w_count = 0;
 	int i = 0, ret = 0;
 	u16 *p = (u16 *)buf;
 	unsigned long tim, limit;
 	u32 val;
 
+	if (force_8bit) {
+		omap_nand_data_out(chip, buf, len, force_8bit);
+		return;
+	}
+
 	/* take care of subpage writes */
 	if (len % 2 != 0) {
-		writeb(*buf, info->nand.legacy.IO_ADDR_W);
+		writeb(*(u8 *)buf, info->fifo);
 		p = (u16 *)(buf + 1);
 		len--;
 	}
@@ -412,18 +326,15 @@ static void omap_write_buf_pref(struct nand_chip *chip, const u_char *buf,
 	ret = omap_prefetch_enable(info->gpmc_cs,
 			PREFETCH_FIFOTHRESHOLD_MAX, 0x0, len, 0x1, info);
 	if (ret) {
-		/* PFPW engine is busy, use cpu copy method */
-		if (info->nand.options & NAND_BUSWIDTH_16)
-			omap_write_buf16(mtd, (u_char *)p, len);
-		else
-			omap_write_buf8(mtd, (u_char *)p, len);
+		/* write posting engine is busy, use CPU copy method */
+		omap_nand_data_out(chip, buf, len, false);
 	} else {
 		while (len) {
 			w_count = readl(info->reg.gpmc_prefetch_status);
 			w_count = PREFETCH_STATUS_FIFO_CNT(w_count);
 			w_count = w_count >> 1;
 			for (i = 0; (i < w_count) && len; i++, len -= 2)
-				iowrite16(*p++, info->nand.legacy.IO_ADDR_W);
+				iowrite16(*p++, info->fifo);
 		}
 		/* wait for data to flushed-out before reset the prefetch */
 		tim = 0;
@@ -451,15 +362,16 @@ static void omap_nand_dma_callback(void *data)
 
 /*
  * omap_nand_dma_transfer: configure and start dma transfer
- * @mtd: MTD device structure
+ * @chip: nand chip structure
  * @addr: virtual address in RAM of source/destination
  * @len: number of data bytes to be transferred
  * @is_write: flag for read/write operation
  */
-static inline int omap_nand_dma_transfer(struct mtd_info *mtd, void *addr,
-					unsigned int len, int is_write)
+static inline int omap_nand_dma_transfer(struct nand_chip *chip,
+					 const void *addr, unsigned int len,
+					 int is_write)
 {
-	struct omap_nand_info *info = mtd_to_omap(mtd);
+	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
 	struct dma_async_tx_descriptor *tx;
 	enum dma_data_direction dir = is_write ? DMA_TO_DEVICE :
 							DMA_FROM_DEVICE;
@@ -521,49 +433,41 @@ static inline int omap_nand_dma_transfer(struct mtd_info *mtd, void *addr,
 out_copy_unmap:
 	dma_unmap_sg(info->dma->device->dev, &sg, 1, dir);
 out_copy:
-	if (info->nand.options & NAND_BUSWIDTH_16)
-		is_write == 0 ? omap_read_buf16(mtd, (u_char *) addr, len)
-			: omap_write_buf16(mtd, (u_char *) addr, len);
-	else
-		is_write == 0 ? omap_read_buf8(mtd, (u_char *) addr, len)
-			: omap_write_buf8(mtd, (u_char *) addr, len);
+	is_write == 0 ? omap_nand_data_in(chip, (void *)addr, len, false)
+		      : omap_nand_data_out(chip, addr, len, false);
+
 	return 0;
 }
 
 /**
- * omap_read_buf_dma_pref - read data from NAND controller into buffer
- * @chip: NAND chip object
- * @buf: buffer to store date
- * @len: number of bytes to read
+ * omap_nand_data_in_dma_pref - NAND data in using DMA and Prefetch
  */
-static void omap_read_buf_dma_pref(struct nand_chip *chip, u_char *buf,
-				   int len)
+static void omap_nand_data_in_dma_pref(struct nand_chip *chip, void *buf,
+				       unsigned int len, bool force_8bit)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
 	if (len <= mtd->oobsize)
-		omap_read_buf_pref(chip, buf, len);
+		omap_nand_data_in_pref(chip, buf, len, false);
 	else
 		/* start transfer in DMA mode */
-		omap_nand_dma_transfer(mtd, buf, len, 0x0);
+		omap_nand_dma_transfer(chip, buf, len, 0x0);
 }
 
 /**
- * omap_write_buf_dma_pref - write buffer to NAND controller
- * @chip: NAND chip object
- * @buf: data buffer
- * @len: number of bytes to write
+ * omap_nand_data_out_dma_pref - NAND data out using DMA and write posting
  */
-static void omap_write_buf_dma_pref(struct nand_chip *chip, const u_char *buf,
-				    int len)
+static void omap_nand_data_out_dma_pref(struct nand_chip *chip,
+					const void *buf, unsigned int len,
+					bool force_8bit)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
 	if (len <= mtd->oobsize)
-		omap_write_buf_pref(chip, buf, len);
+		omap_nand_data_out_pref(chip, buf, len, false);
 	else
 		/* start transfer in DMA mode */
-		omap_nand_dma_transfer(mtd, (u_char *)buf, len, 0x1);
+		omap_nand_dma_transfer(chip, buf, len, 0x1);
 }
 
 /*
@@ -587,13 +491,13 @@ static irqreturn_t omap_nand_irq(int this_irq, void *dev)
 			bytes = info->buf_len;
 		else if (!info->buf_len)
 			bytes = 0;
-		iowrite32_rep(info->nand.legacy.IO_ADDR_W, (u32 *)info->buf,
+		iowrite32_rep(info->fifo, (u32 *)info->buf,
 			      bytes >> 2);
 		info->buf = info->buf + bytes;
 		info->buf_len -= bytes;
 
 	} else {
-		ioread32_rep(info->nand.legacy.IO_ADDR_R, (u32 *)info->buf,
+		ioread32_rep(info->fifo, (u32 *)info->buf,
 			     bytes >> 2);
 		info->buf = info->buf + bytes;
 
@@ -613,20 +517,17 @@ static irqreturn_t omap_nand_irq(int this_irq, void *dev)
 }
 
 /*
- * omap_read_buf_irq_pref - read data from NAND controller into buffer
- * @chip: NAND chip object
- * @buf: buffer to store date
- * @len: number of bytes to read
+ * omap_nand_data_in_irq_pref - NAND data in using Prefetch and IRQ
  */
-static void omap_read_buf_irq_pref(struct nand_chip *chip, u_char *buf,
-				   int len)
+static void omap_nand_data_in_irq_pref(struct nand_chip *chip, void *buf,
+				       unsigned int len, bool force_8bit)
 {
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct omap_nand_info *info = mtd_to_omap(mtd);
+	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
+	struct mtd_info *mtd = nand_to_mtd(&info->nand);
 	int ret = 0;
 
-	if (len <= mtd->oobsize) {
-		omap_read_buf_pref(chip, buf, len);
+	if (len <= mtd->oobsize || force_8bit) {
+		omap_nand_data_in(chip, buf, len, force_8bit);
 		return;
 	}
 
@@ -637,9 +538,11 @@ static void omap_read_buf_irq_pref(struct nand_chip *chip, u_char *buf,
 	/*  configure and start prefetch transfer */
 	ret = omap_prefetch_enable(info->gpmc_cs,
 			PREFETCH_FIFOTHRESHOLD_MAX/2, 0x0, len, 0x0, info);
-	if (ret)
+	if (ret) {
 		/* PFPW engine is busy, use cpu copy method */
-		goto out_copy;
+		omap_nand_data_in(chip, buf, len, false);
+		return;
+	}
 
 	info->buf_len = len;
 
@@ -652,31 +555,23 @@ static void omap_read_buf_irq_pref(struct nand_chip *chip, u_char *buf,
 	/* disable and stop the PFPW engine */
 	omap_prefetch_reset(info->gpmc_cs, info);
 	return;
-
-out_copy:
-	if (info->nand.options & NAND_BUSWIDTH_16)
-		omap_read_buf16(mtd, buf, len);
-	else
-		omap_read_buf8(mtd, buf, len);
 }
 
 /*
- * omap_write_buf_irq_pref - write buffer to NAND controller
- * @chip: NAND chip object
- * @buf: data buffer
- * @len: number of bytes to write
+ * omap_nand_data_out_irq_pref - NAND out using write posting and IRQ
  */
-static void omap_write_buf_irq_pref(struct nand_chip *chip, const u_char *buf,
-				    int len)
+static void omap_nand_data_out_irq_pref(struct nand_chip *chip,
+					const void *buf, unsigned int len,
+					bool force_8bit)
 {
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct omap_nand_info *info = mtd_to_omap(mtd);
+	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
+	struct mtd_info *mtd = nand_to_mtd(&info->nand);
 	int ret = 0;
 	unsigned long tim, limit;
 	u32 val;
 
-	if (len <= mtd->oobsize) {
-		omap_write_buf_pref(chip, buf, len);
+	if (len <= mtd->oobsize || force_8bit) {
+		omap_nand_data_out(chip, buf, len, force_8bit);
 		return;
 	}
 
@@ -687,9 +582,11 @@ static void omap_write_buf_irq_pref(struct nand_chip *chip, const u_char *buf,
 	/* configure and start prefetch transfer : size=24 */
 	ret = omap_prefetch_enable(info->gpmc_cs,
 		(PREFETCH_FIFOTHRESHOLD_MAX * 3) / 8, 0x0, len, 0x1, info);
-	if (ret)
+	if (ret) {
 		/* PFPW engine is busy, use cpu copy method */
-		goto out_copy;
+		omap_nand_data_out(chip, buf, len, false);
+		return;
+	}
 
 	info->buf_len = len;
 
@@ -711,12 +608,6 @@ static void omap_write_buf_irq_pref(struct nand_chip *chip, const u_char *buf,
 	/* disable and stop the PFPW engine */
 	omap_prefetch_reset(info->gpmc_cs, info);
 	return;
-
-out_copy:
-	if (info->nand.options & NAND_BUSWIDTH_16)
-		omap_write_buf16(mtd, buf, len);
-	else
-		omap_write_buf8(mtd, buf, len);
 }
 
 /**
@@ -981,50 +872,6 @@ static void omap_enable_hwecc(struct nand_chip *chip, int mode)
 	writel(val, info->reg.gpmc_ecc_config);
 }
 
-/**
- * omap_wait - wait until the command is done
- * @this: NAND Chip structure
- *
- * Wait function is called during Program and erase operations and
- * the way it is called from MTD layer, we should wait till the NAND
- * chip is ready after the programming/erase operation has completed.
- *
- * Erase can take up to 400ms and program up to 20ms according to
- * general NAND and SmartMedia specs
- */
-static int omap_wait(struct nand_chip *this)
-{
-	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(this));
-	unsigned long timeo = jiffies;
-	int status;
-
-	timeo += msecs_to_jiffies(400);
-
-	writeb(NAND_CMD_STATUS & 0xFF, info->reg.gpmc_nand_command);
-	while (time_before(jiffies, timeo)) {
-		status = readb(info->reg.gpmc_nand_data);
-		if (status & NAND_STATUS_READY)
-			break;
-		cond_resched();
-	}
-
-	status = readb(info->reg.gpmc_nand_data);
-	return status;
-}
-
-/**
- * omap_dev_ready - checks the NAND Ready GPIO line
- * @chip: NAND chip object
- *
- * Returns true if ready and false if busy.
- */
-static int omap_dev_ready(struct nand_chip *chip)
-{
-	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
-
-	return gpiod_get_value(info->ready_gpiod);
-}
-
 /**
  * omap_enable_hwecc_bch - Program GPMC to perform BCH ECC calculation
  * @chip: NAND chip object
@@ -1543,8 +1390,8 @@ static int omap_write_page_bch(struct nand_chip *chip, const uint8_t *buf,
 		chip->ecc.hwctl(chip, NAND_ECC_WRITE);
 
 		/* Write data */
-		chip->legacy.write_buf(chip, buf + (eccpg * info->eccpg_size),
-				       info->eccpg_size);
+		info->data_out(chip, buf + (eccpg * info->eccpg_size),
+			       info->eccpg_size, false);
 
 		/* Update ecc vector from GPMC result registers */
 		ret = omap_calculate_ecc_bch_multi(mtd,
@@ -1562,7 +1409,7 @@ static int omap_write_page_bch(struct nand_chip *chip, const uint8_t *buf,
 	}
 
 	/* Write ecc vector to OOB area */
-	chip->legacy.write_buf(chip, chip->oob_poi, mtd->oobsize);
+	info->data_out(chip, chip->oob_poi, mtd->oobsize, false);
 
 	return nand_prog_page_end_op(chip);
 }
@@ -1607,8 +1454,8 @@ static int omap_write_subpage_bch(struct nand_chip *chip, u32 offset,
 		chip->ecc.hwctl(chip, NAND_ECC_WRITE);
 
 		/* Write data */
-		chip->legacy.write_buf(chip, buf + (eccpg * info->eccpg_size),
-				       info->eccpg_size);
+		info->data_out(chip, buf + (eccpg * info->eccpg_size),
+			       info->eccpg_size, false);
 
 		for (step = 0; step < info->nsteps_per_eccpg; step++) {
 			unsigned int base_step = eccpg * info->nsteps_per_eccpg;
@@ -1641,7 +1488,7 @@ static int omap_write_subpage_bch(struct nand_chip *chip, u32 offset,
 	}
 
 	/* write OOB buffer to NAND device */
-	chip->legacy.write_buf(chip, chip->oob_poi, mtd->oobsize);
+	info->data_out(chip, chip->oob_poi, mtd->oobsize, false);
 
 	return nand_prog_page_end_op(chip);
 }
@@ -1984,8 +1831,8 @@ static int omap_nand_attach_chip(struct nand_chip *chip)
 	/* Re-populate low-level callbacks based on xfer modes */
 	switch (info->xfer_type) {
 	case NAND_OMAP_PREFETCH_POLLED:
-		chip->legacy.read_buf = omap_read_buf_pref;
-		chip->legacy.write_buf = omap_write_buf_pref;
+		info->data_in = omap_nand_data_in_pref;
+		info->data_out = omap_nand_data_out_pref;
 		break;
 
 	case NAND_OMAP_POLLED:
@@ -2017,8 +1864,9 @@ static int omap_nand_attach_chip(struct nand_chip *chip)
 					err);
 				return err;
 			}
-			chip->legacy.read_buf = omap_read_buf_dma_pref;
-			chip->legacy.write_buf = omap_write_buf_dma_pref;
+
+			info->data_in = omap_nand_data_in_dma_pref;
+			info->data_out = omap_nand_data_out_dma_pref;
 		}
 		break;
 
@@ -2049,9 +1897,8 @@ static int omap_nand_attach_chip(struct nand_chip *chip)
 			return err;
 		}
 
-		chip->legacy.read_buf = omap_read_buf_irq_pref;
-		chip->legacy.write_buf = omap_write_buf_irq_pref;
-
+		info->data_in = omap_nand_data_in_irq_pref;
+		info->data_out = omap_nand_data_out_irq_pref;
 		break;
 
 	default:
@@ -2217,8 +2064,105 @@ static int omap_nand_attach_chip(struct nand_chip *chip)
 	return 0;
 }
 
+static void omap_nand_data_in(struct nand_chip *chip, void *buf,
+			      unsigned int len, bool force_8bit)
+{
+	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
+	u32 alignment = ((uintptr_t)buf | len) & 3;
+
+	if (force_8bit || (alignment & 1))
+		ioread8_rep(info->fifo, buf, len);
+	else if (alignment & 3)
+		ioread16_rep(info->fifo, buf, len >> 1);
+	else
+		ioread32_rep(info->fifo, buf, len >> 2);
+}
+
+static void omap_nand_data_out(struct nand_chip *chip,
+			       const void *buf, unsigned int len,
+			       bool force_8bit)
+{
+	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
+	u32 alignment = ((uintptr_t)buf | len) & 3;
+
+	if (force_8bit || (alignment & 1))
+		iowrite8_rep(info->fifo, buf, len);
+	else if (alignment & 3)
+		iowrite16_rep(info->fifo, buf, len >> 1);
+	else
+		iowrite32_rep(info->fifo, buf, len >> 2);
+}
+
+static int omap_nand_exec_instr(struct nand_chip *chip,
+				const struct nand_op_instr *instr)
+{
+	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
+	unsigned int i;
+	int ret;
+
+	switch (instr->type) {
+	case NAND_OP_CMD_INSTR:
+		iowrite8(instr->ctx.cmd.opcode,
+			 info->reg.gpmc_nand_command);
+		break;
+
+	case NAND_OP_ADDR_INSTR:
+		for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+			iowrite8(instr->ctx.addr.addrs[i],
+				 info->reg.gpmc_nand_address);
+		}
+		break;
+
+	case NAND_OP_DATA_IN_INSTR:
+		info->data_in(chip, instr->ctx.data.buf.in,
+			      instr->ctx.data.len,
+			      instr->ctx.data.force_8bit);
+		break;
+
+	case NAND_OP_DATA_OUT_INSTR:
+		info->data_out(chip, instr->ctx.data.buf.out,
+			       instr->ctx.data.len,
+			       instr->ctx.data.force_8bit);
+		break;
+
+	case NAND_OP_WAITRDY_INSTR:
+		ret = info->ready_gpiod ?
+			nand_gpio_waitrdy(chip, info->ready_gpiod, instr->ctx.waitrdy.timeout_ms) :
+			nand_soft_waitrdy(chip, instr->ctx.waitrdy.timeout_ms);
+		if (ret)
+			return ret;
+		break;
+	}
+
+	if (instr->delay_ns)
+		ndelay(instr->delay_ns);
+
+	return 0;
+}
+
+static int omap_nand_exec_op(struct nand_chip *chip,
+			     const struct nand_operation *op,
+			     bool check_only)
+{
+	unsigned int i;
+
+	if (check_only)
+		return 0;
+
+	for (i = 0; i < op->ninstrs; i++) {
+		int ret;
+
+		ret = omap_nand_exec_instr(chip, &op->instrs[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct nand_controller_ops omap_nand_controller_ops = {
 	.attach_chip = omap_nand_attach_chip,
+	.exec_op = omap_nand_exec_op,
 };
 
 /* Shared among all NAND instances to synchronize access to the ECC Engine */
@@ -2233,6 +2177,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	int				err;
 	struct resource			*res;
 	struct device			*dev = &pdev->dev;
+	void __iomem *vaddr;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(struct omap_nand_info),
 				GFP_KERNEL);
@@ -2266,10 +2211,11 @@ static int omap_nand_probe(struct platform_device *pdev)
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	nand_chip->legacy.IO_ADDR_R = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(nand_chip->legacy.IO_ADDR_R))
-		return PTR_ERR(nand_chip->legacy.IO_ADDR_R);
+	vaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(vaddr))
+		return PTR_ERR(vaddr);
 
+	info->fifo = vaddr;
 	info->phys_base = res->start;
 
 	if (!omap_gpmc_controller_initialized) {
@@ -2280,9 +2226,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 
 	nand_chip->controller = &omap_gpmc_controller;
 
-	nand_chip->legacy.IO_ADDR_W = nand_chip->legacy.IO_ADDR_R;
-	nand_chip->legacy.cmd_ctrl  = omap_hwcontrol;
-
 	info->ready_gpiod = devm_gpiod_get_optional(&pdev->dev, "rb",
 						    GPIOD_IN);
 	if (IS_ERR(info->ready_gpiod)) {
@@ -2290,27 +2233,16 @@ static int omap_nand_probe(struct platform_device *pdev)
 		return PTR_ERR(info->ready_gpiod);
 	}
 
-	/*
-	 * If RDY/BSY line is connected to OMAP then use the omap ready
-	 * function and the generic nand_wait function which reads the status
-	 * register after monitoring the RDY/BSY line. Otherwise use a standard
-	 * chip delay which is slightly more than tR (AC Timing) of the NAND
-	 * device and read status register until you get a failure or success
-	 */
-	if (info->ready_gpiod) {
-		nand_chip->legacy.dev_ready = omap_dev_ready;
-		nand_chip->legacy.chip_delay = 0;
-	} else {
-		nand_chip->legacy.waitfunc = omap_wait;
-		nand_chip->legacy.chip_delay = 50;
-	}
-
 	if (info->flash_bbt)
 		nand_chip->bbt_options |= NAND_BBT_USE_FLASH;
 
 	/* scan NAND device connected to chip controller */
 	nand_chip->options |= info->devsize & NAND_BUSWIDTH_16;
 
+	/* default operations */
+	info->data_in = omap_nand_data_in;
+	info->data_out = omap_nand_data_out;
+
 	err = nand_scan(nand_chip, 1);
 	if (err)
 		goto return_error;
-- 
2.17.1

[PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Roger Quadros]

AM64 SoC has an issue which prevents proper 8-bit and 16-bit
reads from GPMC. We are limited to do 32-bit reads only.

Force 32-bit only reads on affected platforms.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/mtd/nand/raw/omap2.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
index f1fc146e09b9..d952de771b35 100644
--- a/drivers/mtd/nand/raw/omap2.c
+++ b/drivers/mtd/nand/raw/omap2.c
@@ -28,6 +28,7 @@
 
 #include <linux/omap-gpmc.h>
 #include <linux/platform_data/mtd-nand-omap2.h>
+#include <linux/sys_soc.h>
 
 #define	DRIVER_NAME	"omap2-nand"
 #define	OMAP_NAND_TIMEOUT_MS	5000
@@ -181,6 +182,7 @@ struct omap_nand_info {
 	void (*data_out)(struct nand_chip *chip,
 			 const void *buf, unsigned int len,
 			 bool force_8bit);
+	bool force_32bit;
 };
 
 static inline struct omap_nand_info *mtd_to_omap(struct mtd_info *mtd)
@@ -2070,6 +2072,25 @@ static void omap_nand_data_in(struct nand_chip *chip, void *buf,
 	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
 	u32 alignment = ((uintptr_t)buf | len) & 3;
 
+	if (info->force_32bit) {
+		u32 val;
+		int left;
+		u8 *ptr;
+
+		ioread32_rep(info->fifo, buf, len >> 2);
+		left = len & 0x3;
+		if (left) {
+			val = ioread32(info->fifo);
+			ptr = (u8 *)(buf + (len - left));
+			while (left--) {
+				*ptr++ = val & 0xff;
+				val >>= 8;
+			}
+		}
+
+		return;
+	}
+
 	if (force_8bit || (alignment & 1))
 		ioread8_rep(info->fifo, buf, len);
 	else if (alignment & 3)
@@ -2169,8 +2190,15 @@ static const struct nand_controller_ops omap_nand_controller_ops = {
 static struct nand_controller omap_gpmc_controller;
 static bool omap_gpmc_controller_initialized;
 
+static const struct of_device_id omap_nand_ids[];
+
 static int omap_nand_probe(struct platform_device *pdev)
 {
+	const struct soc_device_attribute k3_soc_devices[] = {
+		{ .family = "AM64X", .revision = "SR1.0" },
+		{ /* sentinel */ }
+	};
+
 	struct omap_nand_info		*info;
 	struct mtd_info			*mtd;
 	struct nand_chip		*nand_chip;
@@ -2186,6 +2214,12 @@ static int omap_nand_probe(struct platform_device *pdev)
 
 	info->pdev = pdev;
 
+	/* Some SoC's have 32-bit at least, read limitation */
+	if (soc_device_match(k3_soc_devices)) {
+		dev_info(&pdev->dev, "force 32-bit\n");
+		info->force_32bit = true;
+	}
+
 	err = omap_get_dt_info(dev, info);
 	if (err)
 		return err;
@@ -2286,6 +2320,7 @@ static int omap_nand_remove(struct platform_device *pdev)
 
 static const struct of_device_id omap_nand_ids[] = {
 	{ .compatible = "ti,omap2-nand", },
+	{ .compatible = "ti,am64-nand", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, omap_nand_ids);
-- 
2.17.1

Re: [PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Miquel Raynal]

Hi Roger,

rogerq@kernel.org wrote on Tue, 23 Nov 2021 12:36:09 +0200:

> AM64 SoC has an issue which prevents proper 8-bit and 16-bit
> reads from GPMC. We are limited to do 32-bit reads only.

First, thanks for this series!

> Force 32-bit only reads on affected platforms.
> 

Please change the commit title prefix to: "mtd: rawnand: omap2:" in
patch 2, 3, 4.
 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/mtd/nand/raw/omap2.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
> index f1fc146e09b9..d952de771b35 100644
> --- a/drivers/mtd/nand/raw/omap2.c
> +++ b/drivers/mtd/nand/raw/omap2.c
> @@ -28,6 +28,7 @@
>  
>  #include <linux/omap-gpmc.h>
>  #include <linux/platform_data/mtd-nand-omap2.h>
> +#include <linux/sys_soc.h>
>  
>  #define	DRIVER_NAME	"omap2-nand"
>  #define	OMAP_NAND_TIMEOUT_MS	5000
> @@ -181,6 +182,7 @@ struct omap_nand_info {
>  	void (*data_out)(struct nand_chip *chip,
>  			 const void *buf, unsigned int len,
>  			 bool force_8bit);
> +	bool force_32bit;

I believe we should have a driver capability instead of something in
the info structure. You can save the value here as well in the probe if
you want, but I would like this limitation to be tied to the
compatible.

>  };
>  
>  static inline struct omap_nand_info *mtd_to_omap(struct mtd_info *mtd)
> @@ -2070,6 +2072,25 @@ static void omap_nand_data_in(struct nand_chip *chip, void *buf,
>  	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
>  	u32 alignment = ((uintptr_t)buf | len) & 3;
>  
> +	if (info->force_32bit) {

I am a little bit bothered by this limitation. The force8_bit flag does
not require the driver to read only 8-bits of the fifo register, it
actually requires to use only the first 8-bits of the NAND bus (which
can also be 16-bit wide). The older implementation just limited the
number of bits reads to be 8 with ioread8, which seems to be a fine
solution but would require more accesses than using ioread16 (or
ioread32) when reading more than 1 byte on platforms with only 8-bit
busses.

My point here is that:
1- the limited controllers cannot be used with a 16-bit bus
2- non-limited controllers can use ioread16 if the bus width is 8-bits

I guess it's fine not to change the logic to avoid breaking boards so
we can just ignore [2] but I belive we should check chip->options &
NAND_BUSWIDTH_16 in ->attach_chip() and refuse probing if this flag is
set.

> +		u32 val;
> +		int left;
> +		u8 *ptr;
> +
> +		ioread32_rep(info->fifo, buf, len >> 2);
> +		left = len & 0x3;
> +		if (left) {
> +			val = ioread32(info->fifo);
> +			ptr = (u8 *)(buf + (len - left));
> +			while (left--) {
> +				*ptr++ = val & 0xff;
> +				val >>= 8;
> +			}
> +		}
> +
> +		return;
> +	}
> +
>  	if (force_8bit || (alignment & 1))
>  		ioread8_rep(info->fifo, buf, len);
>  	else if (alignment & 3)
> @@ -2169,8 +2190,15 @@ static const struct nand_controller_ops omap_nand_controller_ops = {
>  static struct nand_controller omap_gpmc_controller;
>  static bool omap_gpmc_controller_initialized;
>  
> +static const struct of_device_id omap_nand_ids[];
> +

I believe this change should be dropped.

>  static int omap_nand_probe(struct platform_device *pdev)
>  {
> +	const struct soc_device_attribute k3_soc_devices[] = {
> +		{ .family = "AM64X", .revision = "SR1.0" },
> +		{ /* sentinel */ }
> +	};
> +
>  	struct omap_nand_info		*info;
>  	struct mtd_info			*mtd;
>  	struct nand_chip		*nand_chip;
> @@ -2186,6 +2214,12 @@ static int omap_nand_probe(struct platform_device *pdev)
>  
>  	info->pdev = pdev;
>  
> +	/* Some SoC's have 32-bit at least, read limitation */
> +	if (soc_device_match(k3_soc_devices)) {
> +		dev_info(&pdev->dev, "force 32-bit\n");
> +		info->force_32bit = true;
> +	}
> +

As suggested above, just adding a capability structure tied to the
compatible string and retrieved with of_device_get_match_data() should
be enough and replace this manual tree research.

>  	err = omap_get_dt_info(dev, info);
>  	if (err)
>  		return err;
> @@ -2286,6 +2320,7 @@ static int omap_nand_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id omap_nand_ids[] = {
>  	{ .compatible = "ti,omap2-nand", },
> +	{ .compatible = "ti,am64-nand", },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, omap_nand_ids);

The conversion to exec_op looks fine otherwise :)

Thanks,
Miquèl

Re: [PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Roger Quadros]

Hi Miquel,

On 24/11/2021 14:15, Miquel Raynal wrote:
> Hi Roger,
> 
> rogerq@kernel.org wrote on Tue, 23 Nov 2021 12:36:09 +0200:
> 
>> AM64 SoC has an issue which prevents proper 8-bit and 16-bit
>> reads from GPMC. We are limited to do 32-bit reads only.
> 
> First, thanks for this series!

No problem. Just my job :)

> 
>> Force 32-bit only reads on affected platforms.
>>
> 
> Please change the commit title prefix to: "mtd: rawnand: omap2:" in
> patch 2, 3, 4.

OK.

>  
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/mtd/nand/raw/omap2.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
>> index f1fc146e09b9..d952de771b35 100644
>> --- a/drivers/mtd/nand/raw/omap2.c
>> +++ b/drivers/mtd/nand/raw/omap2.c
>> @@ -28,6 +28,7 @@
>>  
>>  #include <linux/omap-gpmc.h>
>>  #include <linux/platform_data/mtd-nand-omap2.h>
>> +#include <linux/sys_soc.h>
>>  
>>  #define	DRIVER_NAME	"omap2-nand"
>>  #define	OMAP_NAND_TIMEOUT_MS	5000
>> @@ -181,6 +182,7 @@ struct omap_nand_info {
>>  	void (*data_out)(struct nand_chip *chip,
>>  			 const void *buf, unsigned int len,
>>  			 bool force_8bit);
>> +	bool force_32bit;
> 
> I believe we should have a driver capability instead of something in
> the info structure. You can save the value here as well in the probe if
> you want, but I would like this limitation to be tied to the
> compatible.

I will discuss about this at the end.
> 
>>  };
>>  
>>  static inline struct omap_nand_info *mtd_to_omap(struct mtd_info *mtd)
>> @@ -2070,6 +2072,25 @@ static void omap_nand_data_in(struct nand_chip *chip, void *buf,
>>  	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
>>  	u32 alignment = ((uintptr_t)buf | len) & 3;
>>  
>> +	if (info->force_32bit) {
> 
> I am a little bit bothered by this limitation. The force8_bit flag does
> not require the driver to read only 8-bits of the fifo register, it
> actually requires to use only the first 8-bits of the NAND bus (which
> can also be 16-bit wide). The older implementation just limited the
> number of bits reads to be 8 with ioread8, which seems to be a fine
> solution but would require more accesses than using ioread16 (or
> ioread32) when reading more than 1 byte on platforms with only 8-bit
> busses.

I didn't understand the purpose of force8_bit flag. 
How should the driver/controller behave if we get a data_in() call with len 8 and force8_bit flag set?

e.g. if 16-bit NAND ID area contains (little-endian) 2c d3 d0 a6 66 45 67 a3 4f 4e 46 49 ab ef 90 d3
what should data_in(len = 8, force_8_bit = 1) return in buffer?

Based on what you said earlier my guess is it should return 2c d0 66 67 4f 46 ab 90?

> 
> My point here is that:
> 1- the limited controllers cannot be used with a 16-bit bus
> 2- non-limited controllers can use ioread16 if the bus width is 8-bits

Sorry, I did not understand this either. The TI GPMC controller has a configuration setting where we
set the NAND device bus width (8-bit or 16-bit). Then it automatically converts ioread16 or
ioread32 to appropriate number of 8-bit accesses or 16-bit accesses to the NAND chip.

> 
> I guess it's fine not to change the logic to avoid breaking boards so
> we can just ignore [2] but I belive we should check chip->options &
> NAND_BUSWIDTH_16 in ->attach_chip() and refuse probing if this flag is
> set.
> 
>> +		u32 val;
>> +		int left;
>> +		u8 *ptr;
>> +
>> +		ioread32_rep(info->fifo, buf, len >> 2);
>> +		left = len & 0x3;
>> +		if (left) {
>> +			val = ioread32(info->fifo);
>> +			ptr = (u8 *)(buf + (len - left));
>> +			while (left--) {
>> +				*ptr++ = val & 0xff;
>> +				val >>= 8;
>> +			}
>> +		}
>> +
>> +		return;
>> +	}
>> +
>>  	if (force_8bit || (alignment & 1))
>>  		ioread8_rep(info->fifo, buf, len);
>>  	else if (alignment & 3)
>> @@ -2169,8 +2190,15 @@ static const struct nand_controller_ops omap_nand_controller_ops = {
>>  static struct nand_controller omap_gpmc_controller;
>>  static bool omap_gpmc_controller_initialized;
>>  
>> +static const struct of_device_id omap_nand_ids[];
>> +
> 
> I believe this change should be dropped.
> 
>>  static int omap_nand_probe(struct platform_device *pdev)
>>  {
>> +	const struct soc_device_attribute k3_soc_devices[] = {
>> +		{ .family = "AM64X", .revision = "SR1.0" },
>> +		{ /* sentinel */ }
>> +	};
>> +
>>  	struct omap_nand_info		*info;
>>  	struct mtd_info			*mtd;
>>  	struct nand_chip		*nand_chip;
>> @@ -2186,6 +2214,12 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  
>>  	info->pdev = pdev;
>>  
>> +	/* Some SoC's have 32-bit at least, read limitation */
>> +	if (soc_device_match(k3_soc_devices)) {
>> +		dev_info(&pdev->dev, "force 32-bit\n");
>> +		info->force_32bit = true;
>> +	}
>> +
> 
> As suggested above, just adding a capability structure tied to the
> compatible string and retrieved with of_device_get_match_data() should
> be enough and replace this manual tree research.

The trouble comes when TI updates the silicon revision to "SR2.0" and that has the issue fixed
but still uses the same compatible. So compatible string by itself is not sufficient to identify
the troubled devices. soc_device_match() was the easiest way to address this.

> 
>>  	err = omap_get_dt_info(dev, info);
>>  	if (err)
>>  		return err;
>> @@ -2286,6 +2320,7 @@ static int omap_nand_remove(struct platform_device *pdev)
>>  
>>  static const struct of_device_id omap_nand_ids[] = {
>>  	{ .compatible = "ti,omap2-nand", },
>> +	{ .compatible = "ti,am64-nand", },
>>  	{},
>>  };
>>  MODULE_DEVICE_TABLE(of, omap_nand_ids);
> 
> The conversion to exec_op looks fine otherwise :)

Thanks :)

> 
> Thanks,
> Miquèl
> 

cheers,
-roger

Re: [PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Miquel Raynal]

Hi Roger,

rogerq@kernel.org wrote on Thu, 25 Nov 2021 16:12:01 +0200:

> Hi Miquel,
> 
> On 24/11/2021 14:15, Miquel Raynal wrote:
> > Hi Roger,
> > 
> > rogerq@kernel.org wrote on Tue, 23 Nov 2021 12:36:09 +0200:
> >   
> >> AM64 SoC has an issue which prevents proper 8-bit and 16-bit
> >> reads from GPMC. We are limited to do 32-bit reads only.  
> > 
> > First, thanks for this series!  
> 
> No problem. Just my job :)
> 
> >   
> >> Force 32-bit only reads on affected platforms.
> >>  
> > 
> > Please change the commit title prefix to: "mtd: rawnand: omap2:" in
> > patch 2, 3, 4.  
> 
> OK.
> 
> >    
> >> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> >> ---
> >>  drivers/mtd/nand/raw/omap2.c | 35 +++++++++++++++++++++++++++++++++++
> >>  1 file changed, 35 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
> >> index f1fc146e09b9..d952de771b35 100644
> >> --- a/drivers/mtd/nand/raw/omap2.c
> >> +++ b/drivers/mtd/nand/raw/omap2.c
> >> @@ -28,6 +28,7 @@
> >>  
> >>  #include <linux/omap-gpmc.h>
> >>  #include <linux/platform_data/mtd-nand-omap2.h>
> >> +#include <linux/sys_soc.h>
> >>  
> >>  #define	DRIVER_NAME	"omap2-nand"
> >>  #define	OMAP_NAND_TIMEOUT_MS	5000
> >> @@ -181,6 +182,7 @@ struct omap_nand_info {
> >>  	void (*data_out)(struct nand_chip *chip,
> >>  			 const void *buf, unsigned int len,
> >>  			 bool force_8bit);
> >> +	bool force_32bit;  
> > 
> > I believe we should have a driver capability instead of something in
> > the info structure. You can save the value here as well in the probe if
> > you want, but I would like this limitation to be tied to the
> > compatible.  
> 
> I will discuss about this at the end.
> >   
> >>  };
> >>  
> >>  static inline struct omap_nand_info *mtd_to_omap(struct mtd_info *mtd)
> >> @@ -2070,6 +2072,25 @@ static void omap_nand_data_in(struct nand_chip *chip, void *buf,
> >>  	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
> >>  	u32 alignment = ((uintptr_t)buf | len) & 3;
> >>  
> >> +	if (info->force_32bit) {  
> > 
> > I am a little bit bothered by this limitation. The force8_bit flag does
> > not require the driver to read only 8-bits of the fifo register, it
> > actually requires to use only the first 8-bits of the NAND bus (which
> > can also be 16-bit wide). The older implementation just limited the
> > number of bits reads to be 8 with ioread8, which seems to be a fine
> > solution but would require more accesses than using ioread16 (or
> > ioread32) when reading more than 1 byte on platforms with only 8-bit
> > busses.  
> 
> I didn't understand the purpose of force8_bit flag. 

Only access the lowest byte on the bus. This is only needed for
meta-data reads (like status reads or ids) where the upper byte would
be a duplicate.

> How should the driver/controller behave if we get a data_in() call with len 8 and force8_bit flag set?
> 
> e.g. if 16-bit NAND ID area contains (little-endian) 2c d3 d0 a6 66 45 67 a3 4f 4e 46 49 ab ef 90 d3
> what should data_in(len = 8, force_8_bit = 1) return in buffer?
> 
> Based on what you said earlier my guess is it should return 2c d0 66 67 4f 46 ab 90?

If on a 16-bit bus, you would receive 2c 2c d3 d3 d0 d0 a6 a6 etc and
of course that's not what you want.

> > My point here is that:
> > 1- the limited controllers cannot be used with a 16-bit bus
> > 2- non-limited controllers can use ioread16 if the bus width is 8-bits  
> 
> Sorry, I did not understand this either. The TI GPMC controller has a configuration setting where we
> set the NAND device bus width (8-bit or 16-bit). Then it automatically converts ioread16 or
> ioread32 to appropriate number of 8-bit accesses or 16-bit accesses to the NAND chip.

Ok great, in this case you should configure the bus width depending
on the actual used width (8 or 16 bits). When an 8-bit access is
requested with force_8bit, you should ensure the buswidth is changed
to 8 and then use ioread8/16/32 as you wish and then return the bus
back into its default state.

> 
> > 
> > I guess it's fine not to change the logic to avoid breaking boards so
> > we can just ignore [2] but I belive we should check chip->options &
> > NAND_BUSWIDTH_16 in ->attach_chip() and refuse probing if this flag is
> > set.
> >   
> >> +		u32 val;
> >> +		int left;
> >> +		u8 *ptr;
> >> +
> >> +		ioread32_rep(info->fifo, buf, len >> 2);
> >> +		left = len & 0x3;
> >> +		if (left) {
> >> +			val = ioread32(info->fifo);
> >> +			ptr = (u8 *)(buf + (len - left));
> >> +			while (left--) {
> >> +				*ptr++ = val & 0xff;
> >> +				val >>= 8;
> >> +			}
> >> +		}
> >> +
> >> +		return;
> >> +	}
> >> +
> >>  	if (force_8bit || (alignment & 1))
> >>  		ioread8_rep(info->fifo, buf, len);
> >>  	else if (alignment & 3)
> >> @@ -2169,8 +2190,15 @@ static const struct nand_controller_ops omap_nand_controller_ops = {
> >>  static struct nand_controller omap_gpmc_controller;
> >>  static bool omap_gpmc_controller_initialized;
> >>  
> >> +static const struct of_device_id omap_nand_ids[];
> >> +  
> > 
> > I believe this change should be dropped.
> >   
> >>  static int omap_nand_probe(struct platform_device *pdev)
> >>  {
> >> +	const struct soc_device_attribute k3_soc_devices[] = {
> >> +		{ .family = "AM64X", .revision = "SR1.0" },
> >> +		{ /* sentinel */ }
> >> +	};
> >> +
> >>  	struct omap_nand_info		*info;
> >>  	struct mtd_info			*mtd;
> >>  	struct nand_chip		*nand_chip;
> >> @@ -2186,6 +2214,12 @@ static int omap_nand_probe(struct platform_device *pdev)
> >>  
> >>  	info->pdev = pdev;
> >>  
> >> +	/* Some SoC's have 32-bit at least, read limitation */
> >> +	if (soc_device_match(k3_soc_devices)) {
> >> +		dev_info(&pdev->dev, "force 32-bit\n");
> >> +		info->force_32bit = true;
> >> +	}
> >> +  
> > 
> > As suggested above, just adding a capability structure tied to the
> > compatible string and retrieved with of_device_get_match_data() should
> > be enough and replace this manual tree research.  
> 
> The trouble comes when TI updates the silicon revision to "SR2.0" and that has the issue fixed
> but still uses the same compatible. So compatible string by itself is not sufficient to identify
> the troubled devices. soc_device_match() was the easiest way to address this.

This is precisely what compatibles are for, I believe we should declare
the necessary additional compatibles and fix the device trees that are
wrong.

> >>  	err = omap_get_dt_info(dev, info);
> >>  	if (err)
> >>  		return err;
> >> @@ -2286,6 +2320,7 @@ static int omap_nand_remove(struct platform_device *pdev)
> >>  
> >>  static const struct of_device_id omap_nand_ids[] = {
> >>  	{ .compatible = "ti,omap2-nand", },
> >> +	{ .compatible = "ti,am64-nand", },
> >>  	{},
> >>  };
> >>  MODULE_DEVICE_TABLE(of, omap_nand_ids);  
> > 
> > The conversion to exec_op looks fine otherwise :)  
> 
> Thanks :)
> 
> > 
> > Thanks,
> > Miquèl
> >   
> 
> cheers,
> -roger


Thanks,
Miquèl

Re: [PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Roger Quadros]

+Rob and DT list

Miques & Nishanth,

On 26/11/2021 11:42, Miquel Raynal wrote:
> Hi Roger,
> 
> rogerq@kernel.org wrote on Thu, 25 Nov 2021 16:12:01 +0200:
> 
>> Hi Miquel,
>>
>> On 24/11/2021 14:15, Miquel Raynal wrote:
>>> Hi Roger,
>>>
>>> rogerq@kernel.org wrote on Tue, 23 Nov 2021 12:36:09 +0200:
>>>   
>>>> AM64 SoC has an issue which prevents proper 8-bit and 16-bit
>>>> reads from GPMC. We are limited to do 32-bit reads only.  
>>>
>>> First, thanks for this series!  
>>
>> No problem. Just my job :)
>>
>>>   
>>>> Force 32-bit only reads on affected platforms.
>>>>  
>>>
>>> Please change the commit title prefix to: "mtd: rawnand: omap2:" in
>>> patch 2, 3, 4.  
>>
>> OK.
>>
>>>    
>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>>  drivers/mtd/nand/raw/omap2.c | 35 +++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 35 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
>>>> index f1fc146e09b9..d952de771b35 100644
>>>> --- a/drivers/mtd/nand/raw/omap2.c
>>>> +++ b/drivers/mtd/nand/raw/omap2.c
>>>> @@ -28,6 +28,7 @@
>>>>  
>>>>  #include <linux/omap-gpmc.h>
>>>>  #include <linux/platform_data/mtd-nand-omap2.h>
>>>> +#include <linux/sys_soc.h>
>>>>  
>>>>  #define	DRIVER_NAME	"omap2-nand"
>>>>  #define	OMAP_NAND_TIMEOUT_MS	5000
>>>> @@ -181,6 +182,7 @@ struct omap_nand_info {
>>>>  	void (*data_out)(struct nand_chip *chip,
>>>>  			 const void *buf, unsigned int len,
>>>>  			 bool force_8bit);
>>>> +	bool force_32bit;  
>>>
>>> I believe we should have a driver capability instead of something in
>>> the info structure. You can save the value here as well in the probe if
>>> you want, but I would like this limitation to be tied to the
>>> compatible.  
>>
>> I will discuss about this at the end.
>>>   
>>>>  };
>>>>  
>>>>  static inline struct omap_nand_info *mtd_to_omap(struct mtd_info *mtd)
>>>> @@ -2070,6 +2072,25 @@ static void omap_nand_data_in(struct nand_chip *chip, void *buf,
>>>>  	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
>>>>  	u32 alignment = ((uintptr_t)buf | len) & 3;
>>>>  
>>>> +	if (info->force_32bit) {  
>>>
>>> I am a little bit bothered by this limitation. The force8_bit flag does
>>> not require the driver to read only 8-bits of the fifo register, it
>>> actually requires to use only the first 8-bits of the NAND bus (which
>>> can also be 16-bit wide). The older implementation just limited the
>>> number of bits reads to be 8 with ioread8, which seems to be a fine
>>> solution but would require more accesses than using ioread16 (or
>>> ioread32) when reading more than 1 byte on platforms with only 8-bit
>>> busses.  
>>
>> I didn't understand the purpose of force8_bit flag. 
> 
> Only access the lowest byte on the bus. This is only needed for
> meta-data reads (like status reads or ids) where the upper byte would
> be a duplicate.
> 
>> How should the driver/controller behave if we get a data_in() call with len 8 and force8_bit flag set?
>>
>> e.g. if 16-bit NAND ID area contains (little-endian) 2c d3 d0 a6 66 45 67 a3 4f 4e 46 49 ab ef 90 d3
>> what should data_in(len = 8, force_8_bit = 1) return in buffer?
>>
>> Based on what you said earlier my guess is it should return 2c d0 66 67 4f 46 ab 90?
> 
> If on a 16-bit bus, you would receive 2c 2c d3 d3 d0 d0 a6 a6 etc and
> of course that's not what you want.
> 
>>> My point here is that:
>>> 1- the limited controllers cannot be used with a 16-bit bus
>>> 2- non-limited controllers can use ioread16 if the bus width is 8-bits  
>>
>> Sorry, I did not understand this either. The TI GPMC controller has a configuration setting where we
>> set the NAND device bus width (8-bit or 16-bit). Then it automatically converts ioread16 or
>> ioread32 to appropriate number of 8-bit accesses or 16-bit accesses to the NAND chip.
> 
> Ok great, in this case you should configure the bus width depending
> on the actual used width (8 or 16 bits). When an 8-bit access is
> requested with force_8bit, you should ensure the buswidth is changed
> to 8 and then use ioread8/16/32 as you wish and then return the bus
> back into its default state.
> 

OK. I will try this out. Thanks for the tip.

>>
>>>
>>> I guess it's fine not to change the logic to avoid breaking boards so
>>> we can just ignore [2] but I belive we should check chip->options &
>>> NAND_BUSWIDTH_16 in ->attach_chip() and refuse probing if this flag is
>>> set.
>>>   
>>>> +		u32 val;
>>>> +		int left;
>>>> +		u8 *ptr;
>>>> +
>>>> +		ioread32_rep(info->fifo, buf, len >> 2);
>>>> +		left = len & 0x3;
>>>> +		if (left) {
>>>> +			val = ioread32(info->fifo);
>>>> +			ptr = (u8 *)(buf + (len - left));
>>>> +			while (left--) {
>>>> +				*ptr++ = val & 0xff;
>>>> +				val >>= 8;
>>>> +			}
>>>> +		}
>>>> +
>>>> +		return;
>>>> +	}
>>>> +
>>>>  	if (force_8bit || (alignment & 1))
>>>>  		ioread8_rep(info->fifo, buf, len);
>>>>  	else if (alignment & 3)
>>>> @@ -2169,8 +2190,15 @@ static const struct nand_controller_ops omap_nand_controller_ops = {
>>>>  static struct nand_controller omap_gpmc_controller;
>>>>  static bool omap_gpmc_controller_initialized;
>>>>  
>>>> +static const struct of_device_id omap_nand_ids[];
>>>> +  
>>>
>>> I believe this change should be dropped.
>>>   
>>>>  static int omap_nand_probe(struct platform_device *pdev)
>>>>  {
>>>> +	const struct soc_device_attribute k3_soc_devices[] = {
>>>> +		{ .family = "AM64X", .revision = "SR1.0" },
>>>> +		{ /* sentinel */ }
>>>> +	};
>>>> +
>>>>  	struct omap_nand_info		*info;
>>>>  	struct mtd_info			*mtd;
>>>>  	struct nand_chip		*nand_chip;
>>>> @@ -2186,6 +2214,12 @@ static int omap_nand_probe(struct platform_device *pdev)
>>>>  
>>>>  	info->pdev = pdev;
>>>>  
>>>> +	/* Some SoC's have 32-bit at least, read limitation */
>>>> +	if (soc_device_match(k3_soc_devices)) {
>>>> +		dev_info(&pdev->dev, "force 32-bit\n");
>>>> +		info->force_32bit = true;
>>>> +	}
>>>> +  
>>>
>>> As suggested above, just adding a capability structure tied to the
>>> compatible string and retrieved with of_device_get_match_data() should
>>> be enough and replace this manual tree research.  
>>
>> The trouble comes when TI updates the silicon revision to "SR2.0" and that has the issue fixed
>> but still uses the same compatible. So compatible string by itself is not sufficient to identify
>> the troubled devices. soc_device_match() was the easiest way to address this.
> 
> This is precisely what compatibles are for, I believe we should declare
> the necessary additional compatibles and fix the device trees that are
> wrong.

AFAIK TI SoCs don't have different compatibles for different revisions of the same SoC.
My understanding is that the SoC is the same so compatible shouldn't change. Just that there were some
hardware fixes and some quirks may not be needed anymore.

Nishanth,

Could you please chime in on why SoC revisions can't use different compatibles?

> 
>>>>  	err = omap_get_dt_info(dev, info);
>>>>  	if (err)
>>>>  		return err;
>>>> @@ -2286,6 +2320,7 @@ static int omap_nand_remove(struct platform_device *pdev)
>>>>  
>>>>  static const struct of_device_id omap_nand_ids[] = {
>>>>  	{ .compatible = "ti,omap2-nand", },
>>>> +	{ .compatible = "ti,am64-nand", },
>>>>  	{},
>>>>  };
>>>>  MODULE_DEVICE_TABLE(of, omap_nand_ids);  
>>>
>>> The conversion to exec_op looks fine otherwise :)  
>>
>> Thanks :)
>>

<snip>

cheers,
-roger

Re: [PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Nishanth Menon]

On 13:10-20211126, Roger Quadros wrote:
[...]

> >>>> +	/* Some SoC's have 32-bit at least, read limitation */
> >>>> +	if (soc_device_match(k3_soc_devices)) {
> >>>> +		dev_info(&pdev->dev, "force 32-bit\n");
> >>>> +		info->force_32bit = true;
> >>>> +	}
> >>>> +  
> >>>
> >>> As suggested above, just adding a capability structure tied to the
> >>> compatible string and retrieved with of_device_get_match_data() should
> >>> be enough and replace this manual tree research.  
> >>
> >> The trouble comes when TI updates the silicon revision to "SR2.0" and that has the issue fixed
> >> but still uses the same compatible. So compatible string by itself is not sufficient to identify
> >> the troubled devices. soc_device_match() was the easiest way to address this.
> > 
> > This is precisely what compatibles are for, I believe we should declare
> > the necessary additional compatibles and fix the device trees that are
> > wrong.
> 
> AFAIK TI SoCs don't have different compatibles for different revisions of the same SoC.
> My understanding is that the SoC is the same so compatible shouldn't change. Just that there were some
> hardware fixes and some quirks may not be needed anymore.
> 
> Nishanth,
> 
> Could you please chime in on why SoC revisions can't use different compatibles?
> 

The permutations of boards (with add-on cards) and SRs become
un-manageable esp when Silicon Revisions(SRs) dont actually get into
production. Instead, what we do suggest are one of two things:
a) The dts in k.org always reflect the latest SR for the chip that is
   going into production. Older SR revisions are supported as overlays on top
   of the dtb.
b) Where possible, use the chip-id framework[1] to dynamically detect
   the variations. This might be easier with newer K3 generation SoCs.


In this instance, an overlay corresponding to older SoC might be
feasible.



[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/soc/ti/k3-socinfo.yaml
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH 1/4] dt-bindings: mtd: ti,gpmc-nand: Add compatible for AM64 NAND

[Rob Herring]

On Tue, 23 Nov 2021 12:36:06 +0200, Roger Quadros wrote:
> AM64 SoC contains the GPMC NAND controller. Add compatible for it.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  Documentation/devicetree/bindings/mtd/ti,gpmc-nand.yaml | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Roger Quadros]

Hi Nishanth,

On 29/11/2021 06:36, Nishanth Menon wrote:
> On 13:10-20211126, Roger Quadros wrote:
> [...]
> 
>>>>>> +	/* Some SoC's have 32-bit at least, read limitation */
>>>>>> +	if (soc_device_match(k3_soc_devices)) {
>>>>>> +		dev_info(&pdev->dev, "force 32-bit\n");
>>>>>> +		info->force_32bit = true;
>>>>>> +	}
>>>>>> +  
>>>>>
>>>>> As suggested above, just adding a capability structure tied to the
>>>>> compatible string and retrieved with of_device_get_match_data() should
>>>>> be enough and replace this manual tree research.  
>>>>
>>>> The trouble comes when TI updates the silicon revision to "SR2.0" and that has the issue fixed
>>>> but still uses the same compatible. So compatible string by itself is not sufficient to identify
>>>> the troubled devices. soc_device_match() was the easiest way to address this.
>>>
>>> This is precisely what compatibles are for, I believe we should declare
>>> the necessary additional compatibles and fix the device trees that are
>>> wrong.
>>
>> AFAIK TI SoCs don't have different compatibles for different revisions of the same SoC.
>> My understanding is that the SoC is the same so compatible shouldn't change. Just that there were some
>> hardware fixes and some quirks may not be needed anymore.
>>
>> Nishanth,
>>
>> Could you please chime in on why SoC revisions can't use different compatibles?
>>
> 
> The permutations of boards (with add-on cards) and SRs become
> un-manageable esp when Silicon Revisions(SRs) dont actually get into
> production. Instead, what we do suggest are one of two things:
> a) The dts in k.org always reflect the latest SR for the chip that is
>    going into production. Older SR revisions are supported as overlays on top
>    of the dtb.
> b) Where possible, use the chip-id framework[1] to dynamically detect
>    the variations. This might be easier with newer K3 generation SoCs.
> 
> 
> In this instance, an overlay corresponding to older SoC might be
> feasible.
> 

Did I understand correctly that we can use a different compatible for older SoC
in the overlay? e.g. ti,am642-es1.0 ?

If so then I can get rid of soc_device_match and use compatibles matching only in this patch.

> 
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/soc/ti/k3-socinfo.yaml
> 

cheers,
-roger

Re: [PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Nishanth Menon]

On 16:45-20211208, Roger Quadros wrote:
[..]

> Did I understand correctly that we can use a different compatible for older SoC
> in the overlay? e.g. ti,am642-es1.0 ?

If that is what we would desire. There are a few SR1.0 (not ES1.0)
examples for previous devices in [1]

NOTE: the dts in k.org will always point to the latest production SRx.y
device that has been released to market (aka products going to market).
previous pre-production SR support tends to be with overlays for various
quirks.

> 
> If so then I can get rid of soc_device_match and use compatibles matching only in this patch.

As appropriate for the subsystem.


[1] https://git.ti.com/cgit/ti-linux-kernel/ti-upstream-tools/tree/arch/arm64/boot/dts/ti

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D)/Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D