[PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Albert ARIBAUD (3ADEV)]

Fix QUAD read LUT sequence missing JMP_ON_CS which caused
read corruptions with non-1K-size reads on BK4R1 machine.

Add NORMAL, DUAL and FAST read sequences.

Introduce spi-bus-width property for bus subnodes, to
specify per-bus capability to use NORMAL, FAST, DUAL,
and/or QUAD reads.

Also:

Simplify ioremap/memcpy usage.

Make controller clock frequency equal to the lowest of
subnode max frequencies instead of the last one.

Limit Vybrid to use only half its RX fifo, using more will
cause corrupt reads.

Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
---
 drivers/mtd/spi-nor/fsl-quadspi.c | 204 ++++++++++++++++++++++++++++----------
 1 file changed, 152 insertions(+), 52 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 5c82e4e..9811cf2 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -41,6 +41,8 @@
 #define QUADSPI_QUIRK_TKT253890		(1 << 2)
 /* Controller cannot wake up from wait mode, TKT245618 */
 #define QUADSPI_QUIRK_TKT245618         (1 << 3)
+/* Controller can only read half a rx fifo through AHB */
+#define QUADSPI_QUIRK_AHB_READ_HALF_FIFO  (1 << 4)
 
 /* The registers */
 #define QUADSPI_MCR			0x00
@@ -117,6 +119,10 @@
 #define QUADSPI_FR			0x160
 #define QUADSPI_FR_TFF_MASK		0x1
 
+#define QUADSPI_SPTRCLR			0x15c
+#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
+#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
+
 #define QUADSPI_SFA1AD			0x180
 #define QUADSPI_SFA2AD			0x184
 #define QUADSPI_SFB1AD			0x188
@@ -193,7 +199,7 @@
 #define QUADSPI_LUT_NUM		64
 
 /* SEQID -- we can have 16 seqids at most. */
-#define SEQID_QUAD_READ		0
+#define SEQID_READ		0
 #define SEQID_WREN		1
 #define SEQID_WRDI		2
 #define SEQID_RDSR		3
@@ -205,6 +211,9 @@
 #define SEQID_RDCR		9
 #define SEQID_EN4B		10
 #define SEQID_BRWR		11
+#define SEQID_FAST_READ		12
+#define SEQID_DUAL_READ		13
+#define SEQID_QUAD_READ		14
 
 #define QUADSPI_MIN_IOMAP SZ_4M
 
@@ -229,7 +238,8 @@ static struct fsl_qspi_devtype_data vybrid_data = {
 	.rxfifo = 128,
 	.txfifo = 64,
 	.ahb_buf_size = 1024,
-	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
+	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN
+		       | QUADSPI_QUIRK_AHB_READ_HALF_FIFO,
 };
 
 static struct fsl_qspi_devtype_data imx6sx_data = {
@@ -309,6 +319,11 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
 	return q->devtype_data->driver_data & QUADSPI_QUIRK_TKT245618;
 }
 
+static inline int needs_half_fifo(struct fsl_qspi *q)
+{
+	return q->devtype_data->driver_data & QUADSPI_QUIRK_AHB_READ_HALF_FIFO;
+}
+
 /*
  * R/W functions for big- or little-endian registers:
  * The qSPI controller's endian is independent of the CPU core's endian.
@@ -373,8 +388,20 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	void __iomem *base = q->iobase;
 	int rxfifo = q->devtype_data->rxfifo;
 	u32 lut_base;
-	u8 cmd, addrlen, dummy;
+	u8 cmd, addrlen;
 	int i;
+	/* use 8 dummy cycles for all fast operations */
+	const int dummy = 8;
+
+	/* use only half fifo if controller needs that */
+	if (needs_half_fifo(q))
+		rxfifo /= 2;
+
+    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
+	if (q->nor_size <= SZ_16M)
+		addrlen = ADDR24BIT;
+	else
+		addrlen = ADDR32BIT;
 
 	fsl_qspi_unlock_lut(q);
 
@@ -382,24 +409,12 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	for (i = 0; i < QUADSPI_LUT_NUM; i++)
 		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
 
-	/* Quad Read */
-	lut_base = SEQID_QUAD_READ * 4;
-
-	if (q->nor_size <= SZ_16M) {
-		cmd = SPINOR_OP_READ_1_1_4;
-		addrlen = ADDR24BIT;
-		dummy = 8;
-	} else {
-		/* use the 4-byte address */
-		cmd = SPINOR_OP_READ_1_1_4;
-		addrlen = ADDR32BIT;
-		dummy = 8;
-	}
-
-	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
-			base + QUADSPI_LUT(lut_base));
-	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
-			base + QUADSPI_LUT(lut_base + 1));
+	/* Read */
+	lut_base = SEQID_READ * 4;
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ) | LUT1(ADDR, PAD1, addrlen),
+		base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(FSL_READ, PAD1, rxfifo) | LUT1(JMP_ON_CS, PAD1, SEQID_READ * 8),
+		base + QUADSPI_LUT(lut_base + 1));
 
 	/* Write enable */
 	lut_base = SEQID_WREN * 4;
@@ -409,16 +424,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	/* Page Program */
 	lut_base = SEQID_PP * 4;
 
-	if (q->nor_size <= SZ_16M) {
-		cmd = SPINOR_OP_PP;
-		addrlen = ADDR24BIT;
-	} else {
-		/* use the 4-byte address */
-		cmd = SPINOR_OP_PP;
-		addrlen = ADDR32BIT;
-	}
-
-	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_PP) | LUT1(ADDR, PAD1, addrlen),
 			base + QUADSPI_LUT(lut_base));
 	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
 			base + QUADSPI_LUT(lut_base + 1));
@@ -431,7 +437,6 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 
 	/* Erase a sector */
 	lut_base = SEQID_SE * 4;
-
 	cmd = q->nor[0].erase_opcode;
 	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
 
@@ -476,6 +481,33 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
 			base + QUADSPI_LUT(lut_base));
 
+	/* Fast Read */
+	lut_base = SEQID_FAST_READ * 4;
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_FAST) | LUT1(ADDR, PAD1, addrlen),
+		base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD1, rxfifo),
+		base + QUADSPI_LUT(lut_base + 1));
+	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_FAST_READ * 8),
+		base + QUADSPI_LUT(lut_base + 2));
+
+	/* Dual Read */
+	lut_base = SEQID_DUAL_READ * 4;
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_2) | LUT1(ADDR, PAD1, addrlen),
+		base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD2, rxfifo),
+		base + QUADSPI_LUT(lut_base + 1));
+	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_DUAL_READ * 8),
+		base + QUADSPI_LUT(lut_base + 2));
+
+	/* Quad Read */
+	lut_base = SEQID_QUAD_READ * 4;
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_4) | LUT1(ADDR, PAD1, addrlen),
+		base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
+		base + QUADSPI_LUT(lut_base + 1));
+	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_QUAD_READ * 8),
+		base + QUADSPI_LUT(lut_base + 2));
+
 	fsl_qspi_lock_lut(q);
 }
 
@@ -483,8 +515,8 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
 {
 	switch (cmd) {
-	case SPINOR_OP_READ_1_1_4:
-		return SEQID_QUAD_READ;
+	case SPINOR_OP_READ:
+		return SEQID_READ;
 	case SPINOR_OP_WREN:
 		return SEQID_WREN;
 	case SPINOR_OP_WRDI:
@@ -507,6 +539,12 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
 		return SEQID_EN4B;
 	case SPINOR_OP_BRWR:
 		return SEQID_BRWR;
+	case SPINOR_OP_READ_FAST:
+		return SEQID_FAST_READ;
+	case SPINOR_OP_READ_1_1_2:
+		return SEQID_DUAL_READ;
+	case SPINOR_OP_READ_1_1_4:
+		return SEQID_QUAD_READ;
 	default:
 		if (cmd == q->nor[0].erase_opcode)
 			return SEQID_SE;
@@ -676,11 +714,16 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
  * the memcpy to read the data directly. A "missed" access to the buffer
  * causes the controller to clear the buffer, and use the sequence pointed
  * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
+ *
+ * When using two ports, the SEQID to use for one port might differ from
+ * the one to use for the other (e.g., if one port can do 4-pad reads but
+ * the other cannot). So we set up a basic mode here (SEQID_READ) and we
+ * will set up the proper SEQID for the port right before doing the AHB
+ * access(es).
  */
 static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 {
 	void __iomem *base = q->iobase;
-	int seqid;
 
 	/* AHB configuration for access buffer 0/1/2 .*/
 	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
@@ -688,11 +731,13 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
 	/*
 	 * Set ADATSZ with the maximum AHB buffer size to improve the
-	 * read performance.
+	 * read performance, except when the controller should not use
+	 * more than half its RX fifo in AHB reads, in which case read
+	 * size is given in the LUT FSL_READ instructions.
 	 */
 	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
-			((q->devtype_data->ahb_buf_size / 8)
-			<< QUADSPI_BUF3CR_ADATSZ_SHIFT),
+			(needs_half_fifo(q)? 0 : q->devtype_data->ahb_buf_size / 8)
+	        << QUADSPI_BUF3CR_ADATSZ_SHIFT,
 			base + QUADSPI_BUF3CR);
 
 	/* We only use the buffer3 */
@@ -700,9 +745,8 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
 	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
 
-	/* Set the default lut sequence for AHB Read. */
-	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
-	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
+	/* Set the default lut sequence for AHB Read to READ. */
+	qspi_writel(q, SEQID_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
 		q->iobase + QUADSPI_BFGENCR);
 }
 
@@ -841,6 +885,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 		return ret;
 
 	fsl_qspi_read_data(q, len, buf);
+
 	return 0;
 }
 
@@ -887,11 +932,27 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
 {
 	struct fsl_qspi *q = nor->priv;
 	u8 cmd = nor->read_opcode;
+	uint32_t seqid;
+	size_t qlen = q->nor_size * 4;
+	int nor_idx = nor - q->nor;
+	size_t nor_ofs = q->nor_size * nor_idx;
+
+	/* Set the actual lut sequence for AHB Read from the considered nor. */
+	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
+	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
+		q->iobase + QUADSPI_BFGENCR);
+	/* Reset the AHB sequence pointer */
+	qspi_writel(q, QUADSPI_SPTRCLR_BFPTRC_MASK, q->iobase + QUADSPI_SPTRCLR);
+    /* make sure the Rx buffer is read through AHB, not IP */
+	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, q->iobase + QUADSPI_RBCT);
+
+	/* set the chip address for READID */
+	fsl_qspi_set_base_addr(q, q->nor);
 
 	/* if necessary,ioremap buffer before AHB read, */
 	if (!q->ahb_addr) {
-		q->memmap_offs = q->chip_base_addr + from;
-		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
+		q->memmap_offs = q->chip_base_addr;
+		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
 
 		q->ahb_addr = ioremap_nocache(
 				q->memmap_phy + q->memmap_offs,
@@ -906,8 +967,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
 			q->memmap_offs + q->memmap_len) {
 		iounmap(q->ahb_addr);
 
-		q->memmap_offs = q->chip_base_addr + from;
-		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
+		q->memmap_offs = q->chip_base_addr;
+		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
 		q->ahb_addr = ioremap_nocache(
 				q->memmap_phy + q->memmap_offs,
 				q->memmap_len);
@@ -917,13 +978,11 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
 		}
 	}
 
-	dev_dbg(q->dev, "cmd [%x],read from %p, len:%zd\n",
-		cmd, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
-		len);
+	dev_dbg(q->dev, "cmd [%x], read from %p, len:%zd\n",
+		cmd, q->ahb_addr + nor_ofs + from, len);
 
 	/* Read out the data directly from the AHB buffer.*/
-	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
-		len);
+	memcpy(buf, q->ahb_addr + nor_ofs + from, len);
 
 	return len;
 }
@@ -980,6 +1039,11 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	struct spi_nor *nor;
 	struct mtd_info *mtd;
 	int ret, i = 0;
+	const struct of_device_id *of_id =
+			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
+	enum read_mode readmode;
+	unsigned int buswidth;
+	u32 qspimaxfreq, spimaxfreq;
 
 	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
 	if (!q)
@@ -1050,6 +1114,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 	mutex_init(&q->lock);
 
+	/* Running MIN of all "spi-max-frequency' values present in subnodes */
+	qspimaxfreq = INT_MAX;
+
 	/* iterate the subnodes. */
 	for_each_available_child_of_node(dev->of_node, np) {
 		/* skip the holes */
@@ -1073,15 +1140,44 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		nor->prepare = fsl_qspi_prep;
 		nor->unprepare = fsl_qspi_unprep;
 
+		/* Update running MIN of subnode max frequencies */
 		ret = of_property_read_u32(np, "spi-max-frequency",
-				&q->clk_rate);
+				&spimaxfreq);
 		if (ret < 0)
 			goto mutex_failed;
+		qspimaxfreq = (qspimaxfreq > spimaxfreq) ? spimaxfreq: qspimaxfreq;
 
 		/* set the chip address for READID */
 		fsl_qspi_set_base_addr(q, nor);
 
-		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+		/* scan using the proper read mode for this subnode */
+		if (of_property_read_u32(np, "spi-bus-width", &buswidth)<0) {
+			nor->read_opcode = SPINOR_OP_READ;
+			readmode = SPI_NOR_NORMAL;
+		} else {
+			switch (buswidth) {
+			case 4:
+				nor->read_opcode = SPINOR_OP_READ_1_1_4;
+				readmode = SPI_NOR_QUAD;
+				break;
+			case 2:
+				nor->read_opcode = SPINOR_OP_READ_1_1_2;
+				readmode = SPI_NOR_DUAL;
+				break;
+			case 1:
+				nor->read_opcode = SPINOR_OP_READ_FAST;
+				readmode = SPI_NOR_FAST;
+				break;
+			default:
+				dev_warn(q->dev,
+					"ignoring spi-bus-width=%d (should be 1, 2 or 4)",
+					buswidth);
+				nor->read_opcode = SPINOR_OP_READ;
+				readmode = SPI_NOR_NORMAL;
+			}
+		}
+
+		ret = spi_nor_scan(nor, NULL, readmode);
 		if (ret)
 			goto mutex_failed;
 
@@ -1112,6 +1208,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		i++;
 	}
 
+	/* If we know the lowest subnode frequency, override driver default. */
+	if (qspimaxfreq<INT_MAX)
+		q->clk_rate = qspimaxfreq;
+
 	/* finish the rest init. */
 	ret = fsl_qspi_nor_setup_last(q);
 	if (ret)
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Albert ARIBAUD (3ADEV)]

Fix QUAD read LUT sequence missing JMP_ON_CS which caused
read corruptions with non-1K-size reads on BK4R1 machine.

Add NORMAL, DUAL and FAST read sequences.

Introduce spi-bus-width property for bus subnodes, to
specify per-bus capability to use NORMAL, FAST, DUAL,
and/or QUAD reads.

Also:

Simplify ioremap/memcpy usage.

Make controller clock frequency equal to the lowest of
subnode max frequencies instead of the last one.

Limit Vybrid to use only half its RX fifo, using more will
cause corrupt reads.

Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
---
 drivers/mtd/spi-nor/fsl-quadspi.c | 204 ++++++++++++++++++++++++++++----------
 1 file changed, 152 insertions(+), 52 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 5c82e4e..9811cf2 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -41,6 +41,8 @@
 #define QUADSPI_QUIRK_TKT253890		(1 << 2)
 /* Controller cannot wake up from wait mode, TKT245618 */
 #define QUADSPI_QUIRK_TKT245618         (1 << 3)
+/* Controller can only read half a rx fifo through AHB */
+#define QUADSPI_QUIRK_AHB_READ_HALF_FIFO  (1 << 4)
 
 /* The registers */
 #define QUADSPI_MCR			0x00
@@ -117,6 +119,10 @@
 #define QUADSPI_FR			0x160
 #define QUADSPI_FR_TFF_MASK		0x1
 
+#define QUADSPI_SPTRCLR			0x15c
+#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
+#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
+
 #define QUADSPI_SFA1AD			0x180
 #define QUADSPI_SFA2AD			0x184
 #define QUADSPI_SFB1AD			0x188
@@ -193,7 +199,7 @@
 #define QUADSPI_LUT_NUM		64
 
 /* SEQID -- we can have 16 seqids at most. */
-#define SEQID_QUAD_READ		0
+#define SEQID_READ		0
 #define SEQID_WREN		1
 #define SEQID_WRDI		2
 #define SEQID_RDSR		3
@@ -205,6 +211,9 @@
 #define SEQID_RDCR		9
 #define SEQID_EN4B		10
 #define SEQID_BRWR		11
+#define SEQID_FAST_READ		12
+#define SEQID_DUAL_READ		13
+#define SEQID_QUAD_READ		14
 
 #define QUADSPI_MIN_IOMAP SZ_4M
 
@@ -229,7 +238,8 @@ static struct fsl_qspi_devtype_data vybrid_data = {
 	.rxfifo = 128,
 	.txfifo = 64,
 	.ahb_buf_size = 1024,
-	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
+	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN
+		       | QUADSPI_QUIRK_AHB_READ_HALF_FIFO,
 };
 
 static struct fsl_qspi_devtype_data imx6sx_data = {
@@ -309,6 +319,11 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
 	return q->devtype_data->driver_data & QUADSPI_QUIRK_TKT245618;
 }
 
+static inline int needs_half_fifo(struct fsl_qspi *q)
+{
+	return q->devtype_data->driver_data & QUADSPI_QUIRK_AHB_READ_HALF_FIFO;
+}
+
 /*
  * R/W functions for big- or little-endian registers:
  * The qSPI controller's endian is independent of the CPU core's endian.
@@ -373,8 +388,20 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	void __iomem *base = q->iobase;
 	int rxfifo = q->devtype_data->rxfifo;
 	u32 lut_base;
-	u8 cmd, addrlen, dummy;
+	u8 cmd, addrlen;
 	int i;
+	/* use 8 dummy cycles for all fast operations */
+	const int dummy = 8;
+
+	/* use only half fifo if controller needs that */
+	if (needs_half_fifo(q))
+		rxfifo /= 2;
+
+    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
+	if (q->nor_size <= SZ_16M)
+		addrlen = ADDR24BIT;
+	else
+		addrlen = ADDR32BIT;
 
 	fsl_qspi_unlock_lut(q);
 
@@ -382,24 +409,12 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	for (i = 0; i < QUADSPI_LUT_NUM; i++)
 		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
 
-	/* Quad Read */
-	lut_base = SEQID_QUAD_READ * 4;
-
-	if (q->nor_size <= SZ_16M) {
-		cmd = SPINOR_OP_READ_1_1_4;
-		addrlen = ADDR24BIT;
-		dummy = 8;
-	} else {
-		/* use the 4-byte address */
-		cmd = SPINOR_OP_READ_1_1_4;
-		addrlen = ADDR32BIT;
-		dummy = 8;
-	}
-
-	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
-			base + QUADSPI_LUT(lut_base));
-	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
-			base + QUADSPI_LUT(lut_base + 1));
+	/* Read */
+	lut_base = SEQID_READ * 4;
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ) | LUT1(ADDR, PAD1, addrlen),
+		base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(FSL_READ, PAD1, rxfifo) | LUT1(JMP_ON_CS, PAD1, SEQID_READ * 8),
+		base + QUADSPI_LUT(lut_base + 1));
 
 	/* Write enable */
 	lut_base = SEQID_WREN * 4;
@@ -409,16 +424,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	/* Page Program */
 	lut_base = SEQID_PP * 4;
 
-	if (q->nor_size <= SZ_16M) {
-		cmd = SPINOR_OP_PP;
-		addrlen = ADDR24BIT;
-	} else {
-		/* use the 4-byte address */
-		cmd = SPINOR_OP_PP;
-		addrlen = ADDR32BIT;
-	}
-
-	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_PP) | LUT1(ADDR, PAD1, addrlen),
 			base + QUADSPI_LUT(lut_base));
 	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
 			base + QUADSPI_LUT(lut_base + 1));
@@ -431,7 +437,6 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 
 	/* Erase a sector */
 	lut_base = SEQID_SE * 4;
-
 	cmd = q->nor[0].erase_opcode;
 	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
 
@@ -476,6 +481,33 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
 			base + QUADSPI_LUT(lut_base));
 
+	/* Fast Read */
+	lut_base = SEQID_FAST_READ * 4;
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_FAST) | LUT1(ADDR, PAD1, addrlen),
+		base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD1, rxfifo),
+		base + QUADSPI_LUT(lut_base + 1));
+	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_FAST_READ * 8),
+		base + QUADSPI_LUT(lut_base + 2));
+
+	/* Dual Read */
+	lut_base = SEQID_DUAL_READ * 4;
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_2) | LUT1(ADDR, PAD1, addrlen),
+		base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD2, rxfifo),
+		base + QUADSPI_LUT(lut_base + 1));
+	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_DUAL_READ * 8),
+		base + QUADSPI_LUT(lut_base + 2));
+
+	/* Quad Read */
+	lut_base = SEQID_QUAD_READ * 4;
+	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_4) | LUT1(ADDR, PAD1, addrlen),
+		base + QUADSPI_LUT(lut_base));
+	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
+		base + QUADSPI_LUT(lut_base + 1));
+	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_QUAD_READ * 8),
+		base + QUADSPI_LUT(lut_base + 2));
+
 	fsl_qspi_lock_lut(q);
 }
 
@@ -483,8 +515,8 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
 {
 	switch (cmd) {
-	case SPINOR_OP_READ_1_1_4:
-		return SEQID_QUAD_READ;
+	case SPINOR_OP_READ:
+		return SEQID_READ;
 	case SPINOR_OP_WREN:
 		return SEQID_WREN;
 	case SPINOR_OP_WRDI:
@@ -507,6 +539,12 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
 		return SEQID_EN4B;
 	case SPINOR_OP_BRWR:
 		return SEQID_BRWR;
+	case SPINOR_OP_READ_FAST:
+		return SEQID_FAST_READ;
+	case SPINOR_OP_READ_1_1_2:
+		return SEQID_DUAL_READ;
+	case SPINOR_OP_READ_1_1_4:
+		return SEQID_QUAD_READ;
 	default:
 		if (cmd == q->nor[0].erase_opcode)
 			return SEQID_SE;
@@ -676,11 +714,16 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
  * the memcpy to read the data directly. A "missed" access to the buffer
  * causes the controller to clear the buffer, and use the sequence pointed
  * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
+ *
+ * When using two ports, the SEQID to use for one port might differ from
+ * the one to use for the other (e.g., if one port can do 4-pad reads but
+ * the other cannot). So we set up a basic mode here (SEQID_READ) and we
+ * will set up the proper SEQID for the port right before doing the AHB
+ * access(es).
  */
 static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 {
 	void __iomem *base = q->iobase;
-	int seqid;
 
 	/* AHB configuration for access buffer 0/1/2 .*/
 	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
@@ -688,11 +731,13 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
 	/*
 	 * Set ADATSZ with the maximum AHB buffer size to improve the
-	 * read performance.
+	 * read performance, except when the controller should not use
+	 * more than half its RX fifo in AHB reads, in which case read
+	 * size is given in the LUT FSL_READ instructions.
 	 */
 	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
-			((q->devtype_data->ahb_buf_size / 8)
-			<< QUADSPI_BUF3CR_ADATSZ_SHIFT),
+			(needs_half_fifo(q)? 0 : q->devtype_data->ahb_buf_size / 8)
+	        << QUADSPI_BUF3CR_ADATSZ_SHIFT,
 			base + QUADSPI_BUF3CR);
 
 	/* We only use the buffer3 */
@@ -700,9 +745,8 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
 	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
 
-	/* Set the default lut sequence for AHB Read. */
-	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
-	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
+	/* Set the default lut sequence for AHB Read to READ. */
+	qspi_writel(q, SEQID_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
 		q->iobase + QUADSPI_BFGENCR);
 }
 
@@ -841,6 +885,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 		return ret;
 
 	fsl_qspi_read_data(q, len, buf);
+
 	return 0;
 }
 
@@ -887,11 +932,27 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
 {
 	struct fsl_qspi *q = nor->priv;
 	u8 cmd = nor->read_opcode;
+	uint32_t seqid;
+	size_t qlen = q->nor_size * 4;
+	int nor_idx = nor - q->nor;
+	size_t nor_ofs = q->nor_size * nor_idx;
+
+	/* Set the actual lut sequence for AHB Read from the considered nor. */
+	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
+	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
+		q->iobase + QUADSPI_BFGENCR);
+	/* Reset the AHB sequence pointer */
+	qspi_writel(q, QUADSPI_SPTRCLR_BFPTRC_MASK, q->iobase + QUADSPI_SPTRCLR);
+    /* make sure the Rx buffer is read through AHB, not IP */
+	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, q->iobase + QUADSPI_RBCT);
+
+	/* set the chip address for READID */
+	fsl_qspi_set_base_addr(q, q->nor);
 
 	/* if necessary,ioremap buffer before AHB read, */
 	if (!q->ahb_addr) {
-		q->memmap_offs = q->chip_base_addr + from;
-		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
+		q->memmap_offs = q->chip_base_addr;
+		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
 
 		q->ahb_addr = ioremap_nocache(
 				q->memmap_phy + q->memmap_offs,
@@ -906,8 +967,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
 			q->memmap_offs + q->memmap_len) {
 		iounmap(q->ahb_addr);
 
-		q->memmap_offs = q->chip_base_addr + from;
-		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
+		q->memmap_offs = q->chip_base_addr;
+		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
 		q->ahb_addr = ioremap_nocache(
 				q->memmap_phy + q->memmap_offs,
 				q->memmap_len);
@@ -917,13 +978,11 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
 		}
 	}
 
-	dev_dbg(q->dev, "cmd [%x],read from %p, len:%zd\n",
-		cmd, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
-		len);
+	dev_dbg(q->dev, "cmd [%x], read from %p, len:%zd\n",
+		cmd, q->ahb_addr + nor_ofs + from, len);
 
 	/* Read out the data directly from the AHB buffer.*/
-	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
-		len);
+	memcpy(buf, q->ahb_addr + nor_ofs + from, len);
 
 	return len;
 }
@@ -980,6 +1039,11 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	struct spi_nor *nor;
 	struct mtd_info *mtd;
 	int ret, i = 0;
+	const struct of_device_id *of_id =
+			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
+	enum read_mode readmode;
+	unsigned int buswidth;
+	u32 qspimaxfreq, spimaxfreq;
 
 	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
 	if (!q)
@@ -1050,6 +1114,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 	mutex_init(&q->lock);
 
+	/* Running MIN of all "spi-max-frequency' values present in subnodes */
+	qspimaxfreq = INT_MAX;
+
 	/* iterate the subnodes. */
 	for_each_available_child_of_node(dev->of_node, np) {
 		/* skip the holes */
@@ -1073,15 +1140,44 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		nor->prepare = fsl_qspi_prep;
 		nor->unprepare = fsl_qspi_unprep;
 
+		/* Update running MIN of subnode max frequencies */
 		ret = of_property_read_u32(np, "spi-max-frequency",
-				&q->clk_rate);
+				&spimaxfreq);
 		if (ret < 0)
 			goto mutex_failed;
+		qspimaxfreq = (qspimaxfreq > spimaxfreq) ? spimaxfreq: qspimaxfreq;
 
 		/* set the chip address for READID */
 		fsl_qspi_set_base_addr(q, nor);
 
-		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+		/* scan using the proper read mode for this subnode */
+		if (of_property_read_u32(np, "spi-bus-width", &buswidth)<0) {
+			nor->read_opcode = SPINOR_OP_READ;
+			readmode = SPI_NOR_NORMAL;
+		} else {
+			switch (buswidth) {
+			case 4:
+				nor->read_opcode = SPINOR_OP_READ_1_1_4;
+				readmode = SPI_NOR_QUAD;
+				break;
+			case 2:
+				nor->read_opcode = SPINOR_OP_READ_1_1_2;
+				readmode = SPI_NOR_DUAL;
+				break;
+			case 1:
+				nor->read_opcode = SPINOR_OP_READ_FAST;
+				readmode = SPI_NOR_FAST;
+				break;
+			default:
+				dev_warn(q->dev,
+					"ignoring spi-bus-width=%d (should be 1, 2 or 4)",
+					buswidth);
+				nor->read_opcode = SPINOR_OP_READ;
+				readmode = SPI_NOR_NORMAL;
+			}
+		}
+
+		ret = spi_nor_scan(nor, NULL, readmode);
 		if (ret)
 			goto mutex_failed;
 
@@ -1112,6 +1208,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		i++;
 	}
 
+	/* If we know the lowest subnode frequency, override driver default. */
+	if (qspimaxfreq<INT_MAX)
+		q->clk_rate = qspimaxfreq;
+
 	/* finish the rest init. */
 	ret = fsl_qspi_nor_setup_last(q);
 	if (ret)
-- 
2.9.3

[PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Albert ARIBAUD (3ADEV)]

Introduce spi-bus-width property for bus subnodes, to
specify per-bus capability to use NORMAL, FAST, DUAL,
and/or QUAD reads.

Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
---
 Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
index c34aa6f..82e4eb8 100644
--- a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
+++ b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
@@ -24,6 +24,17 @@ Optional properties:
 			      (Please check the board's schematic.)
   - big-endian : That means the IP register is big endian
 
+Required subnode properties:
+  - spi-max-frequency: maximum operating frequency of the SPI device.
+                       The controller will run at the lowest of all
+                       specified frequencies (or at the default of 66 MHz).
+
+Optional subnode properties:
+  - spi-bus-width: if present, specifies how the decide should be read.
+                   1 specifies FAST read, 2 specifies DUAL read, and 4
+                   specifies QUAD read.
+                   If absent, normal (non-FAST) reads are used.
+
 Example:
 
 qspi0: quadspi@40044000 {
@@ -37,5 +48,12 @@ qspi0: quadspi@40044000 {
 
 	flash0: s25fl128s@0 {
 		....
+		spi-max-frequency = <66000000>;
+		spi-bus-width = <2>;
+	};
+	flash1: s25fl128s@1 {
+		....
+		spi-max-frequency = <88000000>;
+		spi-bus-width = <4>;
 	};
 };
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Albert ARIBAUD (3ADEV)]

Introduce spi-bus-width property for bus subnodes, to
specify per-bus capability to use NORMAL, FAST, DUAL,
and/or QUAD reads.

Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
---
 Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
index c34aa6f..82e4eb8 100644
--- a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
+++ b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
@@ -24,6 +24,17 @@ Optional properties:
 			      (Please check the board's schematic.)
   - big-endian : That means the IP register is big endian
 
+Required subnode properties:
+  - spi-max-frequency: maximum operating frequency of the SPI device.
+                       The controller will run at the lowest of all
+                       specified frequencies (or at the default of 66 MHz).
+
+Optional subnode properties:
+  - spi-bus-width: if present, specifies how the decide should be read.
+                   1 specifies FAST read, 2 specifies DUAL read, and 4
+                   specifies QUAD read.
+                   If absent, normal (non-FAST) reads are used.
+
 Example:
 
 qspi0: quadspi@40044000 {
@@ -37,5 +48,12 @@ qspi0: quadspi@40044000 {
 
 	flash0: s25fl128s@0 {
 		....
+		spi-max-frequency = <66000000>;
+		spi-bus-width = <2>;
+	};
+	flash1: s25fl128s@1 {
+		....
+		spi-max-frequency = <88000000>;
+		spi-bus-width = <4>;
 	};
 };
-- 
2.9.3

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Han Xu]

On Tue, Sep 27, 2016 at 07:59:56AM +0200, Albert ARIBAUD (3ADEV) wrote:
> Fix QUAD read LUT sequence missing JMP_ON_CS which caused
> read corruptions with non-1K-size reads on BK4R1 machine.
> 
> Add NORMAL, DUAL and FAST read sequences.
> 
> Introduce spi-bus-width property for bus subnodes, to
> specify per-bus capability to use NORMAL, FAST, DUAL,
> and/or QUAD reads.
> 
> Also:
> 
> Simplify ioremap/memcpy usage.
> 
> Make controller clock frequency equal to the lowest of
> subnode max frequencies instead of the last one.
> 
> Limit Vybrid to use only half its RX fifo, using more will
> cause corrupt reads.
> 

Too much changes in one patch, need to split to several patches.

> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 204 ++++++++++++++++++++++++++++----------
>  1 file changed, 152 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 5c82e4e..9811cf2 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -41,6 +41,8 @@
>  #define QUADSPI_QUIRK_TKT253890		(1 << 2)
>  /* Controller cannot wake up from wait mode, TKT245618 */
>  #define QUADSPI_QUIRK_TKT245618         (1 << 3)
> +/* Controller can only read half a rx fifo through AHB */
> +#define QUADSPI_QUIRK_AHB_READ_HALF_FIFO  (1 << 4)
>  
>  /* The registers */
>  #define QUADSPI_MCR			0x00
> @@ -117,6 +119,10 @@
>  #define QUADSPI_FR			0x160
>  #define QUADSPI_FR_TFF_MASK		0x1
>  
> +#define QUADSPI_SPTRCLR			0x15c
> +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
> +#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> +

offset 0x15c is SR register for i.MX, not sure where does SPTRCLR come from

>  #define QUADSPI_SFA1AD			0x180
>  #define QUADSPI_SFA2AD			0x184
>  #define QUADSPI_SFB1AD			0x188
> @@ -193,7 +199,7 @@
>  #define QUADSPI_LUT_NUM		64
>  
>  /* SEQID -- we can have 16 seqids at most. */
> -#define SEQID_QUAD_READ		0
> +#define SEQID_READ		0
>  #define SEQID_WREN		1
>  #define SEQID_WRDI		2
>  #define SEQID_RDSR		3
> @@ -205,6 +211,9 @@
>  #define SEQID_RDCR		9
>  #define SEQID_EN4B		10
>  #define SEQID_BRWR		11
> +#define SEQID_FAST_READ		12
> +#define SEQID_DUAL_READ		13
> +#define SEQID_QUAD_READ		14
>  
>  #define QUADSPI_MIN_IOMAP SZ_4M
>  
> @@ -229,7 +238,8 @@ static struct fsl_qspi_devtype_data vybrid_data = {
>  	.rxfifo = 128,
>  	.txfifo = 64,
>  	.ahb_buf_size = 1024,
> -	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
> +	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN
> +		       | QUADSPI_QUIRK_AHB_READ_HALF_FIFO,
>  };
>  
>  static struct fsl_qspi_devtype_data imx6sx_data = {
> @@ -309,6 +319,11 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
>  	return q->devtype_data->driver_data & QUADSPI_QUIRK_TKT245618;
>  }
>  
> +static inline int needs_half_fifo(struct fsl_qspi *q)
> +{
> +	return q->devtype_data->driver_data & QUADSPI_QUIRK_AHB_READ_HALF_FIFO;
> +}
> +
>  /*
>   * R/W functions for big- or little-endian registers:
>   * The qSPI controller's endian is independent of the CPU core's endian.
> @@ -373,8 +388,20 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	void __iomem *base = q->iobase;
>  	int rxfifo = q->devtype_data->rxfifo;
>  	u32 lut_base;
> -	u8 cmd, addrlen, dummy;
> +	u8 cmd, addrlen;
>  	int i;
> +	/* use 8 dummy cycles for all fast operations */
> +	const int dummy = 8;
> +
> +	/* use only half fifo if controller needs that */
> +	if (needs_half_fifo(q))
> +		rxfifo /= 2;
> +
> +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> +	if (q->nor_size <= SZ_16M)
> +		addrlen = ADDR24BIT;
> +	else
> +		addrlen = ADDR32BIT;

A better patch fetch info from nor structure, refer to
https://patchwork.ozlabs.org/patch/613429/
>  
>  	fsl_qspi_unlock_lut(q);
>  
> @@ -382,24 +409,12 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	for (i = 0; i < QUADSPI_LUT_NUM; i++)
>  		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
>  
> -	/* Quad Read */
> -	lut_base = SEQID_QUAD_READ * 4;
> -
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR24BIT;
> -		dummy = 8;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR32BIT;
> -		dummy = 8;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> -			base + QUADSPI_LUT(lut_base + 1));
> +	/* Read */
> +	lut_base = SEQID_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(FSL_READ, PAD1, rxfifo) | LUT1(JMP_ON_CS, PAD1, SEQID_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 1));
>  
>  	/* Write enable */
>  	lut_base = SEQID_WREN * 4;
> @@ -409,16 +424,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	/* Page Program */
>  	lut_base = SEQID_PP * 4;
>  
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR24BIT;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR32BIT;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_PP) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
>  	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
>  			base + QUADSPI_LUT(lut_base + 1));
> @@ -431,7 +437,6 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  
>  	/* Erase a sector */
>  	lut_base = SEQID_SE * 4;
> -
>  	cmd = q->nor[0].erase_opcode;
>  	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
>  
> @@ -476,6 +481,33 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
>  			base + QUADSPI_LUT(lut_base));
>  
> +	/* Fast Read */
> +	lut_base = SEQID_FAST_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_FAST) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD1, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_FAST_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Dual Read */
> +	lut_base = SEQID_DUAL_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_2) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD2, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_DUAL_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Quad Read */
> +	lut_base = SEQID_QUAD_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_4) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_QUAD_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
>  	fsl_qspi_lock_lut(q);
>  }
>  
> @@ -483,8 +515,8 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  {
>  	switch (cmd) {
> -	case SPINOR_OP_READ_1_1_4:
> -		return SEQID_QUAD_READ;
> +	case SPINOR_OP_READ:
> +		return SEQID_READ;
>  	case SPINOR_OP_WREN:
>  		return SEQID_WREN;
>  	case SPINOR_OP_WRDI:
> @@ -507,6 +539,12 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  		return SEQID_EN4B;
>  	case SPINOR_OP_BRWR:
>  		return SEQID_BRWR;
> +	case SPINOR_OP_READ_FAST:
> +		return SEQID_FAST_READ;
> +	case SPINOR_OP_READ_1_1_2:
> +		return SEQID_DUAL_READ;
> +	case SPINOR_OP_READ_1_1_4:
> +		return SEQID_QUAD_READ;
>  	default:
>  		if (cmd == q->nor[0].erase_opcode)
>  			return SEQID_SE;
> @@ -676,11 +714,16 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
>   * the memcpy to read the data directly. A "missed" access to the buffer
>   * causes the controller to clear the buffer, and use the sequence pointed
>   * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
> + *
> + * When using two ports, the SEQID to use for one port might differ from
> + * the one to use for the other (e.g., if one port can do 4-pad reads but
> + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> + * will set up the proper SEQID for the port right before doing the AHB
> + * access(es).
>   */

It's rare to find this use case, considering the amount of lut is only 16,
please don't add two many luts in this way. I will send a patch soon for
dynamic lut change, please add these extra luts in dynamic lut list.

>  static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  {
>  	void __iomem *base = q->iobase;
> -	int seqid;
>  
>  	/* AHB configuration for access buffer 0/1/2 .*/
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
> @@ -688,11 +731,13 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
>  	/*
>  	 * Set ADATSZ with the maximum AHB buffer size to improve the
> -	 * read performance.
> +	 * read performance, except when the controller should not use
> +	 * more than half its RX fifo in AHB reads, in which case read
> +	 * size is given in the LUT FSL_READ instructions.
>  	 */
>  	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
> -			((q->devtype_data->ahb_buf_size / 8)
> -			<< QUADSPI_BUF3CR_ADATSZ_SHIFT),
> +			(needs_half_fifo(q)? 0 : q->devtype_data->ahb_buf_size / 8)
> +	        << QUADSPI_BUF3CR_ADATSZ_SHIFT,
>  			base + QUADSPI_BUF3CR);
>  
>  	/* We only use the buffer3 */
> @@ -700,9 +745,8 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
>  	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
>  
> -	/* Set the default lut sequence for AHB Read. */
> -	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
> -	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +	/* Set the default lut sequence for AHB Read to READ. */
> +	qspi_writel(q, SEQID_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
>  		q->iobase + QUADSPI_BFGENCR);
>  }
>  
> @@ -841,6 +885,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>  		return ret;
>  
>  	fsl_qspi_read_data(q, len, buf);
> +
>  	return 0;
>  }
>  
> @@ -887,11 +932,27 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  {
>  	struct fsl_qspi *q = nor->priv;
>  	u8 cmd = nor->read_opcode;
> +	uint32_t seqid;
> +	size_t qlen = q->nor_size * 4;
> +	int nor_idx = nor - q->nor;
> +	size_t nor_ofs = q->nor_size * nor_idx;
> +
> +	/* Set the actual lut sequence for AHB Read from the considered nor. */
> +	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
> +	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +		q->iobase + QUADSPI_BFGENCR);
> +	/* Reset the AHB sequence pointer */
> +	qspi_writel(q, QUADSPI_SPTRCLR_BFPTRC_MASK, q->iobase + QUADSPI_SPTRCLR);
> +    /* make sure the Rx buffer is read through AHB, not IP */
> +	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, q->iobase + QUADSPI_RBCT);
> +
> +	/* set the chip address for READID */
> +	fsl_qspi_set_base_addr(q, q->nor);
>  
>  	/* if necessary,ioremap buffer before AHB read, */
>  	if (!q->ahb_addr) {
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
> @@ -906,8 +967,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  			q->memmap_offs + q->memmap_len) {
>  		iounmap(q->ahb_addr);
>  
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
>  				q->memmap_len);
> @@ -917,13 +978,11 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  		}
>  	}
>  
> -	dev_dbg(q->dev, "cmd [%x],read from %p, len:%zd\n",
> -		cmd, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	dev_dbg(q->dev, "cmd [%x], read from %p, len:%zd\n",
> +		cmd, q->ahb_addr + nor_ofs + from, len);
>  
>  	/* Read out the data directly from the AHB buffer.*/
> -	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	memcpy(buf, q->ahb_addr + nor_ofs + from, len);

No much diff from the previous implementation
>  
>  	return len;
>  }
> @@ -980,6 +1039,11 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	struct spi_nor *nor;
>  	struct mtd_info *mtd;
>  	int ret, i = 0;
> +	const struct of_device_id *of_id =
> +			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
> +	enum read_mode readmode;
> +	unsigned int buswidth;
> +	u32 qspimaxfreq, spimaxfreq;
>  
>  	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>  	if (!q)
> @@ -1050,6 +1114,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  
>  	mutex_init(&q->lock);
>  
> +	/* Running MIN of all "spi-max-frequency' values present in subnodes */
> +	qspimaxfreq = INT_MAX;
> +
>  	/* iterate the subnodes. */
>  	for_each_available_child_of_node(dev->of_node, np) {
>  		/* skip the holes */
> @@ -1073,15 +1140,44 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		nor->prepare = fsl_qspi_prep;
>  		nor->unprepare = fsl_qspi_unprep;
>  
> +		/* Update running MIN of subnode max frequencies */
>  		ret = of_property_read_u32(np, "spi-max-frequency",
> -				&q->clk_rate);
> +				&spimaxfreq);
>  		if (ret < 0)
>  			goto mutex_failed;
> +		qspimaxfreq = (qspimaxfreq > spimaxfreq) ? spimaxfreq: qspimaxfreq;
>  
>  		/* set the chip address for READID */
>  		fsl_qspi_set_base_addr(q, nor);
>  
> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +		/* scan using the proper read mode for this subnode */
> +		if (of_property_read_u32(np, "spi-bus-width", &buswidth)<0) {
> +			nor->read_opcode = SPINOR_OP_READ;
> +			readmode = SPI_NOR_NORMAL;
> +		} else {
> +			switch (buswidth) {
> +			case 4:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_4;
> +				readmode = SPI_NOR_QUAD;
> +				break;
> +			case 2:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_2;
> +				readmode = SPI_NOR_DUAL;
> +				break;
> +			case 1:
> +				nor->read_opcode = SPINOR_OP_READ_FAST;
> +				readmode = SPI_NOR_FAST;
> +				break;
> +			default:
> +				dev_warn(q->dev,
> +					"ignoring spi-bus-width=%d (should be 1, 2 or 4)",
> +					buswidth);
> +				nor->read_opcode = SPINOR_OP_READ;
> +				readmode = SPI_NOR_NORMAL;
> +			}
> +		}
> +
> +		ret = spi_nor_scan(nor, NULL, readmode);

Not necessary, can fetch info from nor structure
https://patchwork.ozlabs.org/patch/613429/

>  		if (ret)
>  			goto mutex_failed;
>  
> @@ -1112,6 +1208,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> +	/* If we know the lowest subnode frequency, override driver default. */
> +	if (qspimaxfreq<INT_MAX)
> +		q->clk_rate = qspimaxfreq;
> +
>  	/* finish the rest init. */
>  	ret = fsl_qspi_nor_setup_last(q);
>  	if (ret)
> -- 
> 2.9.3
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Han Xu]

On Tue, Sep 27, 2016 at 07:59:56AM +0200, Albert ARIBAUD (3ADEV) wrote:
> Fix QUAD read LUT sequence missing JMP_ON_CS which caused
> read corruptions with non-1K-size reads on BK4R1 machine.
> 
> Add NORMAL, DUAL and FAST read sequences.
> 
> Introduce spi-bus-width property for bus subnodes, to
> specify per-bus capability to use NORMAL, FAST, DUAL,
> and/or QUAD reads.
> 
> Also:
> 
> Simplify ioremap/memcpy usage.
> 
> Make controller clock frequency equal to the lowest of
> subnode max frequencies instead of the last one.
> 
> Limit Vybrid to use only half its RX fifo, using more will
> cause corrupt reads.
> 

Too much changes in one patch, need to split to several patches.

> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 204 ++++++++++++++++++++++++++++----------
>  1 file changed, 152 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 5c82e4e..9811cf2 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -41,6 +41,8 @@
>  #define QUADSPI_QUIRK_TKT253890		(1 << 2)
>  /* Controller cannot wake up from wait mode, TKT245618 */
>  #define QUADSPI_QUIRK_TKT245618         (1 << 3)
> +/* Controller can only read half a rx fifo through AHB */
> +#define QUADSPI_QUIRK_AHB_READ_HALF_FIFO  (1 << 4)
>  
>  /* The registers */
>  #define QUADSPI_MCR			0x00
> @@ -117,6 +119,10 @@
>  #define QUADSPI_FR			0x160
>  #define QUADSPI_FR_TFF_MASK		0x1
>  
> +#define QUADSPI_SPTRCLR			0x15c
> +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
> +#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> +

offset 0x15c is SR register for i.MX, not sure where does SPTRCLR come from

>  #define QUADSPI_SFA1AD			0x180
>  #define QUADSPI_SFA2AD			0x184
>  #define QUADSPI_SFB1AD			0x188
> @@ -193,7 +199,7 @@
>  #define QUADSPI_LUT_NUM		64
>  
>  /* SEQID -- we can have 16 seqids at most. */
> -#define SEQID_QUAD_READ		0
> +#define SEQID_READ		0
>  #define SEQID_WREN		1
>  #define SEQID_WRDI		2
>  #define SEQID_RDSR		3
> @@ -205,6 +211,9 @@
>  #define SEQID_RDCR		9
>  #define SEQID_EN4B		10
>  #define SEQID_BRWR		11
> +#define SEQID_FAST_READ		12
> +#define SEQID_DUAL_READ		13
> +#define SEQID_QUAD_READ		14
>  
>  #define QUADSPI_MIN_IOMAP SZ_4M
>  
> @@ -229,7 +238,8 @@ static struct fsl_qspi_devtype_data vybrid_data = {
>  	.rxfifo = 128,
>  	.txfifo = 64,
>  	.ahb_buf_size = 1024,
> -	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
> +	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN
> +		       | QUADSPI_QUIRK_AHB_READ_HALF_FIFO,
>  };
>  
>  static struct fsl_qspi_devtype_data imx6sx_data = {
> @@ -309,6 +319,11 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
>  	return q->devtype_data->driver_data & QUADSPI_QUIRK_TKT245618;
>  }
>  
> +static inline int needs_half_fifo(struct fsl_qspi *q)
> +{
> +	return q->devtype_data->driver_data & QUADSPI_QUIRK_AHB_READ_HALF_FIFO;
> +}
> +
>  /*
>   * R/W functions for big- or little-endian registers:
>   * The qSPI controller's endian is independent of the CPU core's endian.
> @@ -373,8 +388,20 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	void __iomem *base = q->iobase;
>  	int rxfifo = q->devtype_data->rxfifo;
>  	u32 lut_base;
> -	u8 cmd, addrlen, dummy;
> +	u8 cmd, addrlen;
>  	int i;
> +	/* use 8 dummy cycles for all fast operations */
> +	const int dummy = 8;
> +
> +	/* use only half fifo if controller needs that */
> +	if (needs_half_fifo(q))
> +		rxfifo /= 2;
> +
> +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> +	if (q->nor_size <= SZ_16M)
> +		addrlen = ADDR24BIT;
> +	else
> +		addrlen = ADDR32BIT;

A better patch fetch info from nor structure, refer to
https://patchwork.ozlabs.org/patch/613429/
>  
>  	fsl_qspi_unlock_lut(q);
>  
> @@ -382,24 +409,12 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	for (i = 0; i < QUADSPI_LUT_NUM; i++)
>  		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
>  
> -	/* Quad Read */
> -	lut_base = SEQID_QUAD_READ * 4;
> -
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR24BIT;
> -		dummy = 8;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR32BIT;
> -		dummy = 8;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> -			base + QUADSPI_LUT(lut_base + 1));
> +	/* Read */
> +	lut_base = SEQID_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(FSL_READ, PAD1, rxfifo) | LUT1(JMP_ON_CS, PAD1, SEQID_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 1));
>  
>  	/* Write enable */
>  	lut_base = SEQID_WREN * 4;
> @@ -409,16 +424,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	/* Page Program */
>  	lut_base = SEQID_PP * 4;
>  
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR24BIT;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR32BIT;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_PP) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
>  	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
>  			base + QUADSPI_LUT(lut_base + 1));
> @@ -431,7 +437,6 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  
>  	/* Erase a sector */
>  	lut_base = SEQID_SE * 4;
> -
>  	cmd = q->nor[0].erase_opcode;
>  	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
>  
> @@ -476,6 +481,33 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
>  			base + QUADSPI_LUT(lut_base));
>  
> +	/* Fast Read */
> +	lut_base = SEQID_FAST_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_FAST) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD1, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_FAST_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Dual Read */
> +	lut_base = SEQID_DUAL_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_2) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD2, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_DUAL_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Quad Read */
> +	lut_base = SEQID_QUAD_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_4) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_QUAD_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
>  	fsl_qspi_lock_lut(q);
>  }
>  
> @@ -483,8 +515,8 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  {
>  	switch (cmd) {
> -	case SPINOR_OP_READ_1_1_4:
> -		return SEQID_QUAD_READ;
> +	case SPINOR_OP_READ:
> +		return SEQID_READ;
>  	case SPINOR_OP_WREN:
>  		return SEQID_WREN;
>  	case SPINOR_OP_WRDI:
> @@ -507,6 +539,12 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  		return SEQID_EN4B;
>  	case SPINOR_OP_BRWR:
>  		return SEQID_BRWR;
> +	case SPINOR_OP_READ_FAST:
> +		return SEQID_FAST_READ;
> +	case SPINOR_OP_READ_1_1_2:
> +		return SEQID_DUAL_READ;
> +	case SPINOR_OP_READ_1_1_4:
> +		return SEQID_QUAD_READ;
>  	default:
>  		if (cmd == q->nor[0].erase_opcode)
>  			return SEQID_SE;
> @@ -676,11 +714,16 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
>   * the memcpy to read the data directly. A "missed" access to the buffer
>   * causes the controller to clear the buffer, and use the sequence pointed
>   * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
> + *
> + * When using two ports, the SEQID to use for one port might differ from
> + * the one to use for the other (e.g., if one port can do 4-pad reads but
> + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> + * will set up the proper SEQID for the port right before doing the AHB
> + * access(es).
>   */

It's rare to find this use case, considering the amount of lut is only 16,
please don't add two many luts in this way. I will send a patch soon for
dynamic lut change, please add these extra luts in dynamic lut list.

>  static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  {
>  	void __iomem *base = q->iobase;
> -	int seqid;
>  
>  	/* AHB configuration for access buffer 0/1/2 .*/
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
> @@ -688,11 +731,13 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
>  	/*
>  	 * Set ADATSZ with the maximum AHB buffer size to improve the
> -	 * read performance.
> +	 * read performance, except when the controller should not use
> +	 * more than half its RX fifo in AHB reads, in which case read
> +	 * size is given in the LUT FSL_READ instructions.
>  	 */
>  	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
> -			((q->devtype_data->ahb_buf_size / 8)
> -			<< QUADSPI_BUF3CR_ADATSZ_SHIFT),
> +			(needs_half_fifo(q)? 0 : q->devtype_data->ahb_buf_size / 8)
> +	        << QUADSPI_BUF3CR_ADATSZ_SHIFT,
>  			base + QUADSPI_BUF3CR);
>  
>  	/* We only use the buffer3 */
> @@ -700,9 +745,8 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
>  	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
>  
> -	/* Set the default lut sequence for AHB Read. */
> -	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
> -	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +	/* Set the default lut sequence for AHB Read to READ. */
> +	qspi_writel(q, SEQID_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
>  		q->iobase + QUADSPI_BFGENCR);
>  }
>  
> @@ -841,6 +885,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>  		return ret;
>  
>  	fsl_qspi_read_data(q, len, buf);
> +
>  	return 0;
>  }
>  
> @@ -887,11 +932,27 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  {
>  	struct fsl_qspi *q = nor->priv;
>  	u8 cmd = nor->read_opcode;
> +	uint32_t seqid;
> +	size_t qlen = q->nor_size * 4;
> +	int nor_idx = nor - q->nor;
> +	size_t nor_ofs = q->nor_size * nor_idx;
> +
> +	/* Set the actual lut sequence for AHB Read from the considered nor. */
> +	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
> +	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +		q->iobase + QUADSPI_BFGENCR);
> +	/* Reset the AHB sequence pointer */
> +	qspi_writel(q, QUADSPI_SPTRCLR_BFPTRC_MASK, q->iobase + QUADSPI_SPTRCLR);
> +    /* make sure the Rx buffer is read through AHB, not IP */
> +	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, q->iobase + QUADSPI_RBCT);
> +
> +	/* set the chip address for READID */
> +	fsl_qspi_set_base_addr(q, q->nor);
>  
>  	/* if necessary,ioremap buffer before AHB read, */
>  	if (!q->ahb_addr) {
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
> @@ -906,8 +967,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  			q->memmap_offs + q->memmap_len) {
>  		iounmap(q->ahb_addr);
>  
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
>  				q->memmap_len);
> @@ -917,13 +978,11 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  		}
>  	}
>  
> -	dev_dbg(q->dev, "cmd [%x],read from %p, len:%zd\n",
> -		cmd, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	dev_dbg(q->dev, "cmd [%x], read from %p, len:%zd\n",
> +		cmd, q->ahb_addr + nor_ofs + from, len);
>  
>  	/* Read out the data directly from the AHB buffer.*/
> -	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	memcpy(buf, q->ahb_addr + nor_ofs + from, len);

No much diff from the previous implementation
>  
>  	return len;
>  }
> @@ -980,6 +1039,11 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	struct spi_nor *nor;
>  	struct mtd_info *mtd;
>  	int ret, i = 0;
> +	const struct of_device_id *of_id =
> +			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
> +	enum read_mode readmode;
> +	unsigned int buswidth;
> +	u32 qspimaxfreq, spimaxfreq;
>  
>  	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>  	if (!q)
> @@ -1050,6 +1114,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  
>  	mutex_init(&q->lock);
>  
> +	/* Running MIN of all "spi-max-frequency' values present in subnodes */
> +	qspimaxfreq = INT_MAX;
> +
>  	/* iterate the subnodes. */
>  	for_each_available_child_of_node(dev->of_node, np) {
>  		/* skip the holes */
> @@ -1073,15 +1140,44 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		nor->prepare = fsl_qspi_prep;
>  		nor->unprepare = fsl_qspi_unprep;
>  
> +		/* Update running MIN of subnode max frequencies */
>  		ret = of_property_read_u32(np, "spi-max-frequency",
> -				&q->clk_rate);
> +				&spimaxfreq);
>  		if (ret < 0)
>  			goto mutex_failed;
> +		qspimaxfreq = (qspimaxfreq > spimaxfreq) ? spimaxfreq: qspimaxfreq;
>  
>  		/* set the chip address for READID */
>  		fsl_qspi_set_base_addr(q, nor);
>  
> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +		/* scan using the proper read mode for this subnode */
> +		if (of_property_read_u32(np, "spi-bus-width", &buswidth)<0) {
> +			nor->read_opcode = SPINOR_OP_READ;
> +			readmode = SPI_NOR_NORMAL;
> +		} else {
> +			switch (buswidth) {
> +			case 4:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_4;
> +				readmode = SPI_NOR_QUAD;
> +				break;
> +			case 2:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_2;
> +				readmode = SPI_NOR_DUAL;
> +				break;
> +			case 1:
> +				nor->read_opcode = SPINOR_OP_READ_FAST;
> +				readmode = SPI_NOR_FAST;
> +				break;
> +			default:
> +				dev_warn(q->dev,
> +					"ignoring spi-bus-width=%d (should be 1, 2 or 4)",
> +					buswidth);
> +				nor->read_opcode = SPINOR_OP_READ;
> +				readmode = SPI_NOR_NORMAL;
> +			}
> +		}
> +
> +		ret = spi_nor_scan(nor, NULL, readmode);

Not necessary, can fetch info from nor structure
https://patchwork.ozlabs.org/patch/613429/

>  		if (ret)
>  			goto mutex_failed;
>  
> @@ -1112,6 +1208,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> +	/* If we know the lowest subnode frequency, override driver default. */
> +	if (qspimaxfreq<INT_MAX)
> +		q->clk_rate = qspimaxfreq;
> +
>  	/* finish the rest init. */
>  	ret = fsl_qspi_nor_setup_last(q);
>  	if (ret)
> -- 
> 2.9.3
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Albert ARIBAUD]

Hello Han,

Le Thu, 29 Sep 2016 04:06:52 +0800, Han Xu <xhnjupt-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> a écrit :

> Too much changes in one patch, need to split to several patches.

Will do.

> > +#define QUADSPI_SPTRCLR			0x15c
> > +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
> > +#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> > +  
> 
> offset 0x15c is SR register for i.MX, not sure where does SPTRCLR come from

It's a typo -- should have been 0x16c, not 0x15c. It is the Vybrid's
Sequence Pointer Clear Register. Apparently, the bad register write did
not screw thing up enough to cause any visible issue. Thanks for
pointing it out.

> > +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> > +	if (q->nor_size <= SZ_16M)
> > +		addrlen = ADDR24BIT;
> > +	else
> > +		addrlen = ADDR32BIT;  
> 
> A better patch fetch info from nor structure, refer to
> https://patchwork.ozlabs.org/patch/613429/

v3 (https://patchwork.kernel.org/patch/9287005/) seems poised to go in
at some point. I can base my patches above it. I will mention the
dependency in my series' cover letter.

> > + * When using two ports, the SEQID to use for one port might differ from
> > + * the one to use for the other (e.g., if one port can do 4-pad reads but
> > + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> > + * will set up the proper SEQID for the port right before doing the AHB
> > + * access(es).
> >   */  
> 
> It's rare to find this use case, considering the amount of lut is only 16,
> please don't add two many luts in this way. I will send a patch soon for
> dynamic lut change, please add these extra luts in dynamic lut list.

I'm fine with switching to a dynamic LUT list; this matches the "two
ports with different characteristics" well. Is the patch already
available somewhere so that I can rebase over it in advance?

> >  	/* Read out the data directly from the AHB buffer.*/
> > -	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> > -		len);
> > +	memcpy(buf, q->ahb_addr + nor_ofs + from, len);  
> 
> No much diff from the previous implementation

Still, it simplifies the memcpy source address computation.

> > +		ret = spi_nor_scan(nor, NULL, readmode);  
> 
> Not necessary, can fetch info from nor structure
> https://patchwork.ozlabs.org/patch/613429/

Ditto.

Thanks for your review!

Cordialement,
Albert ARIBAUD
3ADEV
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Albert ARIBAUD]

Hello Han,

Le Thu, 29 Sep 2016 04:06:52 +0800, Han Xu <xhnjupt@gmail.com> a écrit :

> Too much changes in one patch, need to split to several patches.

Will do.

> > +#define QUADSPI_SPTRCLR			0x15c
> > +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
> > +#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> > +  
> 
> offset 0x15c is SR register for i.MX, not sure where does SPTRCLR come from

It's a typo -- should have been 0x16c, not 0x15c. It is the Vybrid's
Sequence Pointer Clear Register. Apparently, the bad register write did
not screw thing up enough to cause any visible issue. Thanks for
pointing it out.

> > +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> > +	if (q->nor_size <= SZ_16M)
> > +		addrlen = ADDR24BIT;
> > +	else
> > +		addrlen = ADDR32BIT;  
> 
> A better patch fetch info from nor structure, refer to
> https://patchwork.ozlabs.org/patch/613429/

v3 (https://patchwork.kernel.org/patch/9287005/) seems poised to go in
at some point. I can base my patches above it. I will mention the
dependency in my series' cover letter.

> > + * When using two ports, the SEQID to use for one port might differ from
> > + * the one to use for the other (e.g., if one port can do 4-pad reads but
> > + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> > + * will set up the proper SEQID for the port right before doing the AHB
> > + * access(es).
> >   */  
> 
> It's rare to find this use case, considering the amount of lut is only 16,
> please don't add two many luts in this way. I will send a patch soon for
> dynamic lut change, please add these extra luts in dynamic lut list.

I'm fine with switching to a dynamic LUT list; this matches the "two
ports with different characteristics" well. Is the patch already
available somewhere so that I can rebase over it in advance?

> >  	/* Read out the data directly from the AHB buffer.*/
> > -	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> > -		len);
> > +	memcpy(buf, q->ahb_addr + nor_ofs + from, len);  
> 
> No much diff from the previous implementation

Still, it simplifies the memcpy source address computation.

> > +		ret = spi_nor_scan(nor, NULL, readmode);  
> 
> Not necessary, can fetch info from nor structure
> https://patchwork.ozlabs.org/patch/613429/

Ditto.

Thanks for your review!

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Han Xu]

From: Albert ARIBAUD <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
Sent: Wednesday, September 28, 2016 3:45 PM
To: Han Xu
Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Han Xu
Subject: Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

Hello Han,

Le Thu, 29 Sep 2016 04:06:52 +0800, Han Xu <xhnjupt-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> a écrit :

> Too much changes in one patch, need to split to several patches.

Will do.

> > +#define QUADSPI_SPTRCLR                    0x15c
> > +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT               0
> > +#define QUADSPI_SPTRCLR_BFPTRC_MASK                (0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> > +
>
> offset 0x15c is SR register for i.MX, not sure where does SPTRCLR come from

It's a typo -- should have been 0x16c, not 0x15c. It is the Vybrid's
Sequence Pointer Clear Register. Apparently, the bad register write did
not screw thing up enough to cause any visible issue. Thanks for
pointing it out.

> > +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> > +   if (q->nor_size <= SZ_16M)
> > +           addrlen = ADDR24BIT;
> > +   else
> > +           addrlen = ADDR32BIT;
>
> A better patch fetch info from nor structure, refer to
> https://patchwork.ozlabs.org/patch/613429/

v3 (https://patchwork.kernel.org/patch/9287005/) seems poised to go in
at some point. I can base my patches above it. I will mention the
dependency in my series' cover letter.

> > + * When using two ports, the SEQID to use for one port might differ from
> > + * the one to use for the other (e.g., if one port can do 4-pad reads but
> > + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> > + * will set up the proper SEQID for the port right before doing the AHB
> > + * access(es).
> >   */
>
> It's rare to find this use case, considering the amount of lut is only 16,
> please don't add two many luts in this way. I will send a patch soon for
> dynamic lut change, please add these extra luts in dynamic lut list.

I'm fine with switching to a dynamic LUT list; this matches the "two
ports with different characteristics" well. Is the patch already
available somewhere so that I can rebase over it in advance?

Just sent the patch out, refer to https://patchwork.ozlabs.org/patch/676791/

> >     /* Read out the data directly from the AHB buffer.*/
> > -   memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> > -           len);
> > +   memcpy(buf, q->ahb_addr + nor_ofs + from, len);
>
> No much diff from the previous implementation

Still, it simplifies the memcpy source address computation.

> > +           ret = spi_nor_scan(nor, NULL, readmode);
>
> Not necessary, can fetch info from nor structure
> https://patchwork.ozlabs.org/patch/613429/

Ditto.

Thanks for your review!

Cordialement,
Albert ARIBAUD
3ADEV
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Han Xu]

From: Albert ARIBAUD <albert.aribaud@3adev.fr>
Sent: Wednesday, September 28, 2016 3:45 PM
To: Han Xu
Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Han Xu
Subject: Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

Hello Han,

Le Thu, 29 Sep 2016 04:06:52 +0800, Han Xu <xhnjupt@gmail.com> a écrit :

> Too much changes in one patch, need to split to several patches.

Will do.

> > +#define QUADSPI_SPTRCLR                    0x15c
> > +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT               0
> > +#define QUADSPI_SPTRCLR_BFPTRC_MASK                (0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> > +
>
> offset 0x15c is SR register for i.MX, not sure where does SPTRCLR come from

It's a typo -- should have been 0x16c, not 0x15c. It is the Vybrid's
Sequence Pointer Clear Register. Apparently, the bad register write did
not screw thing up enough to cause any visible issue. Thanks for
pointing it out.

> > +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> > +   if (q->nor_size <= SZ_16M)
> > +           addrlen = ADDR24BIT;
> > +   else
> > +           addrlen = ADDR32BIT;
>
> A better patch fetch info from nor structure, refer to
> https://patchwork.ozlabs.org/patch/613429/

v3 (https://patchwork.kernel.org/patch/9287005/) seems poised to go in
at some point. I can base my patches above it. I will mention the
dependency in my series' cover letter.

> > + * When using two ports, the SEQID to use for one port might differ from
> > + * the one to use for the other (e.g., if one port can do 4-pad reads but
> > + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> > + * will set up the proper SEQID for the port right before doing the AHB
> > + * access(es).
> >   */
>
> It's rare to find this use case, considering the amount of lut is only 16,
> please don't add two many luts in this way. I will send a patch soon for
> dynamic lut change, please add these extra luts in dynamic lut list.

I'm fine with switching to a dynamic LUT list; this matches the "two
ports with different characteristics" well. Is the patch already
available somewhere so that I can rebase over it in advance?

Just sent the patch out, refer to https://patchwork.ozlabs.org/patch/676791/

> >     /* Read out the data directly from the AHB buffer.*/
> > -   memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> > -           len);
> > +   memcpy(buf, q->ahb_addr + nor_ofs + from, len);
>
> No much diff from the previous implementation

Still, it simplifies the memcpy source address computation.

> > +           ret = spi_nor_scan(nor, NULL, readmode);
>
> Not necessary, can fetch info from nor structure
> https://patchwork.ozlabs.org/patch/613429/

Ditto.

Thanks for your review!

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Rob Herring]

On Tue, Sep 27, 2016 at 07:59:57AM +0200, Albert ARIBAUD (3ADEV) wrote:
> Introduce spi-bus-width property for bus subnodes, to
> specify per-bus capability to use NORMAL, FAST, DUAL,
> and/or QUAD reads.
> 
> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Make this a common property.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Rob Herring]

On Tue, Sep 27, 2016 at 07:59:57AM +0200, Albert ARIBAUD (3ADEV) wrote:
> Introduce spi-bus-width property for bus subnodes, to
> specify per-bus capability to use NORMAL, FAST, DUAL,
> and/or QUAD reads.
> 
> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
> ---
>  Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Make this a common property.

Rob

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Geert Uytterhoeven]

On Mon, Oct 3, 2016 at 8:51 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Sep 27, 2016 at 07:59:57AM +0200, Albert ARIBAUD (3ADEV) wrote:
>> Introduce spi-bus-width property for bus subnodes, to
>> specify per-bus capability to use NORMAL, FAST, DUAL,
>> and/or QUAD reads.
>>
>> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>
> Make this a common property.

They already exist:

Documentation/devicetree/bindings/spi/spi-bus.txt:
- spi-tx-bus-width - (optional) The bus width (number of data wires) that is
                      used for MOSI. Defaults to 1 if not present.
- spi-rx-bus-width - (optional) The bus width (number of data wires) that is
                      used for MISO. Defaults to 1 if not present.

The above are for normal/dual/quad.

"Fast" is not a property of the bus, but of the SPI slave, right? Cfr.

Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt:
- m25p,fast-read : Use the "fast read" opcode to read data from the chip instead
                   of the usual "read" opcode. This opcode is not supported by
                   all chips and support for it can not be detected at runtime.
                   Refer to your chips' datasheet to check if this is supported
                   by your chip.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Geert Uytterhoeven]

On Mon, Oct 3, 2016 at 8:51 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Sep 27, 2016 at 07:59:57AM +0200, Albert ARIBAUD (3ADEV) wrote:
>> Introduce spi-bus-width property for bus subnodes, to
>> specify per-bus capability to use NORMAL, FAST, DUAL,
>> and/or QUAD reads.
>>
>> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
>> ---
>>  Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>
> Make this a common property.

They already exist:

Documentation/devicetree/bindings/spi/spi-bus.txt:
- spi-tx-bus-width - (optional) The bus width (number of data wires) that is
                      used for MOSI. Defaults to 1 if not present.
- spi-rx-bus-width - (optional) The bus width (number of data wires) that is
                      used for MISO. Defaults to 1 if not present.

The above are for normal/dual/quad.

"Fast" is not a property of the bus, but of the SPI slave, right? Cfr.

Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt:
- m25p,fast-read : Use the "fast read" opcode to read data from the chip instead
                   of the usual "read" opcode. This opcode is not supported by
                   all chips and support for it can not be detected at runtime.
                   Refer to your chips' datasheet to check if this is supported
                   by your chip.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Cyrille Pitchen]

Hi Albert,

your patch is likely to conflict with:
https://patchwork.kernel.org/patch/9287005/

Yunhui's patch relies on the settings selected by spi-nor.c to configure the
freescale QSPI controller correctly. The freescale QSPI controller driver, like
other QSPI controller drivers, should never override settings selected in
spi_nor_scan() otherwise it will duplicate the code, will be hard to maintain
and won't work with every memory. For instance, not all memory manufacturers
use the very same op codes for Quad SPI Fast Read.
So let's spi-nor.c handle this stuff!

That's why I suggest you rebase your work on Yunhui patches.

Best regards,

Cyrille

Le 27/09/2016 à 07:59, Albert ARIBAUD (3ADEV) a écrit :
> Fix QUAD read LUT sequence missing JMP_ON_CS which caused
> read corruptions with non-1K-size reads on BK4R1 machine.
> 
> Add NORMAL, DUAL and FAST read sequences.
> 
> Introduce spi-bus-width property for bus subnodes, to
> specify per-bus capability to use NORMAL, FAST, DUAL,
> and/or QUAD reads.
> 
> Also:
> 
> Simplify ioremap/memcpy usage.
> 
> Make controller clock frequency equal to the lowest of
> subnode max frequencies instead of the last one.
> 
> Limit Vybrid to use only half its RX fifo, using more will
> cause corrupt reads.
> 
> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 204 ++++++++++++++++++++++++++++----------
>  1 file changed, 152 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 5c82e4e..9811cf2 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -41,6 +41,8 @@
>  #define QUADSPI_QUIRK_TKT253890		(1 << 2)
>  /* Controller cannot wake up from wait mode, TKT245618 */
>  #define QUADSPI_QUIRK_TKT245618         (1 << 3)
> +/* Controller can only read half a rx fifo through AHB */
> +#define QUADSPI_QUIRK_AHB_READ_HALF_FIFO  (1 << 4)
>  
>  /* The registers */
>  #define QUADSPI_MCR			0x00
> @@ -117,6 +119,10 @@
>  #define QUADSPI_FR			0x160
>  #define QUADSPI_FR_TFF_MASK		0x1
>  
> +#define QUADSPI_SPTRCLR			0x15c
> +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
> +#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> +
>  #define QUADSPI_SFA1AD			0x180
>  #define QUADSPI_SFA2AD			0x184
>  #define QUADSPI_SFB1AD			0x188
> @@ -193,7 +199,7 @@
>  #define QUADSPI_LUT_NUM		64
>  
>  /* SEQID -- we can have 16 seqids at most. */
> -#define SEQID_QUAD_READ		0
> +#define SEQID_READ		0
>  #define SEQID_WREN		1
>  #define SEQID_WRDI		2
>  #define SEQID_RDSR		3
> @@ -205,6 +211,9 @@
>  #define SEQID_RDCR		9
>  #define SEQID_EN4B		10
>  #define SEQID_BRWR		11
> +#define SEQID_FAST_READ		12
> +#define SEQID_DUAL_READ		13
> +#define SEQID_QUAD_READ		14
>  
>  #define QUADSPI_MIN_IOMAP SZ_4M
>  
> @@ -229,7 +238,8 @@ static struct fsl_qspi_devtype_data vybrid_data = {
>  	.rxfifo = 128,
>  	.txfifo = 64,
>  	.ahb_buf_size = 1024,
> -	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
> +	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN
> +		       | QUADSPI_QUIRK_AHB_READ_HALF_FIFO,
>  };
>  
>  static struct fsl_qspi_devtype_data imx6sx_data = {
> @@ -309,6 +319,11 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
>  	return q->devtype_data->driver_data & QUADSPI_QUIRK_TKT245618;
>  }
>  
> +static inline int needs_half_fifo(struct fsl_qspi *q)
> +{
> +	return q->devtype_data->driver_data & QUADSPI_QUIRK_AHB_READ_HALF_FIFO;
> +}
> +
>  /*
>   * R/W functions for big- or little-endian registers:
>   * The qSPI controller's endian is independent of the CPU core's endian.
> @@ -373,8 +388,20 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	void __iomem *base = q->iobase;
>  	int rxfifo = q->devtype_data->rxfifo;
>  	u32 lut_base;
> -	u8 cmd, addrlen, dummy;
> +	u8 cmd, addrlen;
>  	int i;
> +	/* use 8 dummy cycles for all fast operations */
> +	const int dummy = 8;
> +
> +	/* use only half fifo if controller needs that */
> +	if (needs_half_fifo(q))
> +		rxfifo /= 2;
> +
> +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> +	if (q->nor_size <= SZ_16M)
> +		addrlen = ADDR24BIT;
> +	else
> +		addrlen = ADDR32BIT;
>  
>  	fsl_qspi_unlock_lut(q);
>  
> @@ -382,24 +409,12 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	for (i = 0; i < QUADSPI_LUT_NUM; i++)
>  		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
>  
> -	/* Quad Read */
> -	lut_base = SEQID_QUAD_READ * 4;
> -
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR24BIT;
> -		dummy = 8;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR32BIT;
> -		dummy = 8;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> -			base + QUADSPI_LUT(lut_base + 1));
> +	/* Read */
> +	lut_base = SEQID_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(FSL_READ, PAD1, rxfifo) | LUT1(JMP_ON_CS, PAD1, SEQID_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 1));
>  
>  	/* Write enable */
>  	lut_base = SEQID_WREN * 4;
> @@ -409,16 +424,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	/* Page Program */
>  	lut_base = SEQID_PP * 4;
>  
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR24BIT;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR32BIT;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_PP) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
>  	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
>  			base + QUADSPI_LUT(lut_base + 1));
> @@ -431,7 +437,6 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  
>  	/* Erase a sector */
>  	lut_base = SEQID_SE * 4;
> -
>  	cmd = q->nor[0].erase_opcode;
>  	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
>  
> @@ -476,6 +481,33 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
>  			base + QUADSPI_LUT(lut_base));
>  
> +	/* Fast Read */
> +	lut_base = SEQID_FAST_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_FAST) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD1, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_FAST_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Dual Read */
> +	lut_base = SEQID_DUAL_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_2) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD2, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_DUAL_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Quad Read */
> +	lut_base = SEQID_QUAD_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_4) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_QUAD_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
>  	fsl_qspi_lock_lut(q);
>  }
>  
> @@ -483,8 +515,8 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  {
>  	switch (cmd) {
> -	case SPINOR_OP_READ_1_1_4:
> -		return SEQID_QUAD_READ;
> +	case SPINOR_OP_READ:
> +		return SEQID_READ;
>  	case SPINOR_OP_WREN:
>  		return SEQID_WREN;
>  	case SPINOR_OP_WRDI:
> @@ -507,6 +539,12 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  		return SEQID_EN4B;
>  	case SPINOR_OP_BRWR:
>  		return SEQID_BRWR;
> +	case SPINOR_OP_READ_FAST:
> +		return SEQID_FAST_READ;
> +	case SPINOR_OP_READ_1_1_2:
> +		return SEQID_DUAL_READ;
> +	case SPINOR_OP_READ_1_1_4:
> +		return SEQID_QUAD_READ;
>  	default:
>  		if (cmd == q->nor[0].erase_opcode)
>  			return SEQID_SE;
> @@ -676,11 +714,16 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
>   * the memcpy to read the data directly. A "missed" access to the buffer
>   * causes the controller to clear the buffer, and use the sequence pointed
>   * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
> + *
> + * When using two ports, the SEQID to use for one port might differ from
> + * the one to use for the other (e.g., if one port can do 4-pad reads but
> + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> + * will set up the proper SEQID for the port right before doing the AHB
> + * access(es).
>   */
>  static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  {
>  	void __iomem *base = q->iobase;
> -	int seqid;
>  
>  	/* AHB configuration for access buffer 0/1/2 .*/
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
> @@ -688,11 +731,13 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
>  	/*
>  	 * Set ADATSZ with the maximum AHB buffer size to improve the
> -	 * read performance.
> +	 * read performance, except when the controller should not use
> +	 * more than half its RX fifo in AHB reads, in which case read
> +	 * size is given in the LUT FSL_READ instructions.
>  	 */
>  	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
> -			((q->devtype_data->ahb_buf_size / 8)
> -			<< QUADSPI_BUF3CR_ADATSZ_SHIFT),
> +			(needs_half_fifo(q)? 0 : q->devtype_data->ahb_buf_size / 8)
> +	        << QUADSPI_BUF3CR_ADATSZ_SHIFT,
>  			base + QUADSPI_BUF3CR);
>  
>  	/* We only use the buffer3 */
> @@ -700,9 +745,8 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
>  	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
>  
> -	/* Set the default lut sequence for AHB Read. */
> -	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
> -	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +	/* Set the default lut sequence for AHB Read to READ. */
> +	qspi_writel(q, SEQID_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
>  		q->iobase + QUADSPI_BFGENCR);
>  }
>  
> @@ -841,6 +885,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>  		return ret;
>  
>  	fsl_qspi_read_data(q, len, buf);
> +
>  	return 0;
>  }
>  
> @@ -887,11 +932,27 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  {
>  	struct fsl_qspi *q = nor->priv;
>  	u8 cmd = nor->read_opcode;
> +	uint32_t seqid;
> +	size_t qlen = q->nor_size * 4;
> +	int nor_idx = nor - q->nor;
> +	size_t nor_ofs = q->nor_size * nor_idx;
> +
> +	/* Set the actual lut sequence for AHB Read from the considered nor. */
> +	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
> +	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +		q->iobase + QUADSPI_BFGENCR);
> +	/* Reset the AHB sequence pointer */
> +	qspi_writel(q, QUADSPI_SPTRCLR_BFPTRC_MASK, q->iobase + QUADSPI_SPTRCLR);
> +    /* make sure the Rx buffer is read through AHB, not IP */
> +	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, q->iobase + QUADSPI_RBCT);
> +
> +	/* set the chip address for READID */
> +	fsl_qspi_set_base_addr(q, q->nor);
>  
>  	/* if necessary,ioremap buffer before AHB read, */
>  	if (!q->ahb_addr) {
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
> @@ -906,8 +967,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  			q->memmap_offs + q->memmap_len) {
>  		iounmap(q->ahb_addr);
>  
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
>  				q->memmap_len);
> @@ -917,13 +978,11 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  		}
>  	}
>  
> -	dev_dbg(q->dev, "cmd [%x],read from %p, len:%zd\n",
> -		cmd, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	dev_dbg(q->dev, "cmd [%x], read from %p, len:%zd\n",
> +		cmd, q->ahb_addr + nor_ofs + from, len);
>  
>  	/* Read out the data directly from the AHB buffer.*/
> -	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	memcpy(buf, q->ahb_addr + nor_ofs + from, len);
>  
>  	return len;
>  }
> @@ -980,6 +1039,11 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	struct spi_nor *nor;
>  	struct mtd_info *mtd;
>  	int ret, i = 0;
> +	const struct of_device_id *of_id =
> +			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
> +	enum read_mode readmode;
> +	unsigned int buswidth;
> +	u32 qspimaxfreq, spimaxfreq;
>  
>  	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>  	if (!q)
> @@ -1050,6 +1114,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  
>  	mutex_init(&q->lock);
>  
> +	/* Running MIN of all "spi-max-frequency' values present in subnodes */
> +	qspimaxfreq = INT_MAX;
> +
>  	/* iterate the subnodes. */
>  	for_each_available_child_of_node(dev->of_node, np) {
>  		/* skip the holes */
> @@ -1073,15 +1140,44 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		nor->prepare = fsl_qspi_prep;
>  		nor->unprepare = fsl_qspi_unprep;
>  
> +		/* Update running MIN of subnode max frequencies */
>  		ret = of_property_read_u32(np, "spi-max-frequency",
> -				&q->clk_rate);
> +				&spimaxfreq);
>  		if (ret < 0)
>  			goto mutex_failed;
> +		qspimaxfreq = (qspimaxfreq > spimaxfreq) ? spimaxfreq: qspimaxfreq;
>  
>  		/* set the chip address for READID */
>  		fsl_qspi_set_base_addr(q, nor);
>  
> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +		/* scan using the proper read mode for this subnode */
> +		if (of_property_read_u32(np, "spi-bus-width", &buswidth)<0) {
> +			nor->read_opcode = SPINOR_OP_READ;
> +			readmode = SPI_NOR_NORMAL;
> +		} else {
> +			switch (buswidth) {
> +			case 4:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_4;
> +				readmode = SPI_NOR_QUAD;
> +				break;
> +			case 2:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_2;
> +				readmode = SPI_NOR_DUAL;
> +				break;
> +			case 1:
> +				nor->read_opcode = SPINOR_OP_READ_FAST;
> +				readmode = SPI_NOR_FAST;
> +				break;
> +			default:
> +				dev_warn(q->dev,
> +					"ignoring spi-bus-width=%d (should be 1, 2 or 4)",
> +					buswidth);
> +				nor->read_opcode = SPINOR_OP_READ;
> +				readmode = SPI_NOR_NORMAL;
> +			}
> +		}
> +
> +		ret = spi_nor_scan(nor, NULL, readmode);
>  		if (ret)
>  			goto mutex_failed;
>  
> @@ -1112,6 +1208,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> +	/* If we know the lowest subnode frequency, override driver default. */
> +	if (qspimaxfreq<INT_MAX)
> +		q->clk_rate = qspimaxfreq;
> +
>  	/* finish the rest init. */
>  	ret = fsl_qspi_nor_setup_last(q);
>  	if (ret)
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Cyrille Pitchen]

Hi Albert,

your patch is likely to conflict with:
https://patchwork.kernel.org/patch/9287005/

Yunhui's patch relies on the settings selected by spi-nor.c to configure the
freescale QSPI controller correctly. The freescale QSPI controller driver, like
other QSPI controller drivers, should never override settings selected in
spi_nor_scan() otherwise it will duplicate the code, will be hard to maintain
and won't work with every memory. For instance, not all memory manufacturers
use the very same op codes for Quad SPI Fast Read.
So let's spi-nor.c handle this stuff!

That's why I suggest you rebase your work on Yunhui patches.

Best regards,

Cyrille

Le 27/09/2016 à 07:59, Albert ARIBAUD (3ADEV) a écrit :
> Fix QUAD read LUT sequence missing JMP_ON_CS which caused
> read corruptions with non-1K-size reads on BK4R1 machine.
> 
> Add NORMAL, DUAL and FAST read sequences.
> 
> Introduce spi-bus-width property for bus subnodes, to
> specify per-bus capability to use NORMAL, FAST, DUAL,
> and/or QUAD reads.
> 
> Also:
> 
> Simplify ioremap/memcpy usage.
> 
> Make controller clock frequency equal to the lowest of
> subnode max frequencies instead of the last one.
> 
> Limit Vybrid to use only half its RX fifo, using more will
> cause corrupt reads.
> 
> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 204 ++++++++++++++++++++++++++++----------
>  1 file changed, 152 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 5c82e4e..9811cf2 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -41,6 +41,8 @@
>  #define QUADSPI_QUIRK_TKT253890		(1 << 2)
>  /* Controller cannot wake up from wait mode, TKT245618 */
>  #define QUADSPI_QUIRK_TKT245618         (1 << 3)
> +/* Controller can only read half a rx fifo through AHB */
> +#define QUADSPI_QUIRK_AHB_READ_HALF_FIFO  (1 << 4)
>  
>  /* The registers */
>  #define QUADSPI_MCR			0x00
> @@ -117,6 +119,10 @@
>  #define QUADSPI_FR			0x160
>  #define QUADSPI_FR_TFF_MASK		0x1
>  
> +#define QUADSPI_SPTRCLR			0x15c
> +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
> +#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> +
>  #define QUADSPI_SFA1AD			0x180
>  #define QUADSPI_SFA2AD			0x184
>  #define QUADSPI_SFB1AD			0x188
> @@ -193,7 +199,7 @@
>  #define QUADSPI_LUT_NUM		64
>  
>  /* SEQID -- we can have 16 seqids at most. */
> -#define SEQID_QUAD_READ		0
> +#define SEQID_READ		0
>  #define SEQID_WREN		1
>  #define SEQID_WRDI		2
>  #define SEQID_RDSR		3
> @@ -205,6 +211,9 @@
>  #define SEQID_RDCR		9
>  #define SEQID_EN4B		10
>  #define SEQID_BRWR		11
> +#define SEQID_FAST_READ		12
> +#define SEQID_DUAL_READ		13
> +#define SEQID_QUAD_READ		14
>  
>  #define QUADSPI_MIN_IOMAP SZ_4M
>  
> @@ -229,7 +238,8 @@ static struct fsl_qspi_devtype_data vybrid_data = {
>  	.rxfifo = 128,
>  	.txfifo = 64,
>  	.ahb_buf_size = 1024,
> -	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
> +	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN
> +		       | QUADSPI_QUIRK_AHB_READ_HALF_FIFO,
>  };
>  
>  static struct fsl_qspi_devtype_data imx6sx_data = {
> @@ -309,6 +319,11 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
>  	return q->devtype_data->driver_data & QUADSPI_QUIRK_TKT245618;
>  }
>  
> +static inline int needs_half_fifo(struct fsl_qspi *q)
> +{
> +	return q->devtype_data->driver_data & QUADSPI_QUIRK_AHB_READ_HALF_FIFO;
> +}
> +
>  /*
>   * R/W functions for big- or little-endian registers:
>   * The qSPI controller's endian is independent of the CPU core's endian.
> @@ -373,8 +388,20 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	void __iomem *base = q->iobase;
>  	int rxfifo = q->devtype_data->rxfifo;
>  	u32 lut_base;
> -	u8 cmd, addrlen, dummy;
> +	u8 cmd, addrlen;
>  	int i;
> +	/* use 8 dummy cycles for all fast operations */
> +	const int dummy = 8;
> +
> +	/* use only half fifo if controller needs that */
> +	if (needs_half_fifo(q))
> +		rxfifo /= 2;
> +
> +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> +	if (q->nor_size <= SZ_16M)
> +		addrlen = ADDR24BIT;
> +	else
> +		addrlen = ADDR32BIT;
>  
>  	fsl_qspi_unlock_lut(q);
>  
> @@ -382,24 +409,12 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	for (i = 0; i < QUADSPI_LUT_NUM; i++)
>  		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
>  
> -	/* Quad Read */
> -	lut_base = SEQID_QUAD_READ * 4;
> -
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR24BIT;
> -		dummy = 8;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR32BIT;
> -		dummy = 8;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> -			base + QUADSPI_LUT(lut_base + 1));
> +	/* Read */
> +	lut_base = SEQID_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(FSL_READ, PAD1, rxfifo) | LUT1(JMP_ON_CS, PAD1, SEQID_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 1));
>  
>  	/* Write enable */
>  	lut_base = SEQID_WREN * 4;
> @@ -409,16 +424,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	/* Page Program */
>  	lut_base = SEQID_PP * 4;
>  
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR24BIT;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR32BIT;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_PP) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
>  	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
>  			base + QUADSPI_LUT(lut_base + 1));
> @@ -431,7 +437,6 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  
>  	/* Erase a sector */
>  	lut_base = SEQID_SE * 4;
> -
>  	cmd = q->nor[0].erase_opcode;
>  	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
>  
> @@ -476,6 +481,33 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
>  			base + QUADSPI_LUT(lut_base));
>  
> +	/* Fast Read */
> +	lut_base = SEQID_FAST_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_FAST) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD1, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_FAST_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Dual Read */
> +	lut_base = SEQID_DUAL_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_2) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD2, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_DUAL_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Quad Read */
> +	lut_base = SEQID_QUAD_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_4) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_QUAD_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
>  	fsl_qspi_lock_lut(q);
>  }
>  
> @@ -483,8 +515,8 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  {
>  	switch (cmd) {
> -	case SPINOR_OP_READ_1_1_4:
> -		return SEQID_QUAD_READ;
> +	case SPINOR_OP_READ:
> +		return SEQID_READ;
>  	case SPINOR_OP_WREN:
>  		return SEQID_WREN;
>  	case SPINOR_OP_WRDI:
> @@ -507,6 +539,12 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  		return SEQID_EN4B;
>  	case SPINOR_OP_BRWR:
>  		return SEQID_BRWR;
> +	case SPINOR_OP_READ_FAST:
> +		return SEQID_FAST_READ;
> +	case SPINOR_OP_READ_1_1_2:
> +		return SEQID_DUAL_READ;
> +	case SPINOR_OP_READ_1_1_4:
> +		return SEQID_QUAD_READ;
>  	default:
>  		if (cmd == q->nor[0].erase_opcode)
>  			return SEQID_SE;
> @@ -676,11 +714,16 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
>   * the memcpy to read the data directly. A "missed" access to the buffer
>   * causes the controller to clear the buffer, and use the sequence pointed
>   * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
> + *
> + * When using two ports, the SEQID to use for one port might differ from
> + * the one to use for the other (e.g., if one port can do 4-pad reads but
> + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> + * will set up the proper SEQID for the port right before doing the AHB
> + * access(es).
>   */
>  static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  {
>  	void __iomem *base = q->iobase;
> -	int seqid;
>  
>  	/* AHB configuration for access buffer 0/1/2 .*/
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
> @@ -688,11 +731,13 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
>  	/*
>  	 * Set ADATSZ with the maximum AHB buffer size to improve the
> -	 * read performance.
> +	 * read performance, except when the controller should not use
> +	 * more than half its RX fifo in AHB reads, in which case read
> +	 * size is given in the LUT FSL_READ instructions.
>  	 */
>  	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
> -			((q->devtype_data->ahb_buf_size / 8)
> -			<< QUADSPI_BUF3CR_ADATSZ_SHIFT),
> +			(needs_half_fifo(q)? 0 : q->devtype_data->ahb_buf_size / 8)
> +	        << QUADSPI_BUF3CR_ADATSZ_SHIFT,
>  			base + QUADSPI_BUF3CR);
>  
>  	/* We only use the buffer3 */
> @@ -700,9 +745,8 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
>  	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
>  
> -	/* Set the default lut sequence for AHB Read. */
> -	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
> -	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +	/* Set the default lut sequence for AHB Read to READ. */
> +	qspi_writel(q, SEQID_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
>  		q->iobase + QUADSPI_BFGENCR);
>  }
>  
> @@ -841,6 +885,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>  		return ret;
>  
>  	fsl_qspi_read_data(q, len, buf);
> +
>  	return 0;
>  }
>  
> @@ -887,11 +932,27 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  {
>  	struct fsl_qspi *q = nor->priv;
>  	u8 cmd = nor->read_opcode;
> +	uint32_t seqid;
> +	size_t qlen = q->nor_size * 4;
> +	int nor_idx = nor - q->nor;
> +	size_t nor_ofs = q->nor_size * nor_idx;
> +
> +	/* Set the actual lut sequence for AHB Read from the considered nor. */
> +	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
> +	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +		q->iobase + QUADSPI_BFGENCR);
> +	/* Reset the AHB sequence pointer */
> +	qspi_writel(q, QUADSPI_SPTRCLR_BFPTRC_MASK, q->iobase + QUADSPI_SPTRCLR);
> +    /* make sure the Rx buffer is read through AHB, not IP */
> +	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, q->iobase + QUADSPI_RBCT);
> +
> +	/* set the chip address for READID */
> +	fsl_qspi_set_base_addr(q, q->nor);
>  
>  	/* if necessary,ioremap buffer before AHB read, */
>  	if (!q->ahb_addr) {
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
> @@ -906,8 +967,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  			q->memmap_offs + q->memmap_len) {
>  		iounmap(q->ahb_addr);
>  
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
>  				q->memmap_len);
> @@ -917,13 +978,11 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  		}
>  	}
>  
> -	dev_dbg(q->dev, "cmd [%x],read from %p, len:%zd\n",
> -		cmd, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	dev_dbg(q->dev, "cmd [%x], read from %p, len:%zd\n",
> +		cmd, q->ahb_addr + nor_ofs + from, len);
>  
>  	/* Read out the data directly from the AHB buffer.*/
> -	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	memcpy(buf, q->ahb_addr + nor_ofs + from, len);
>  
>  	return len;
>  }
> @@ -980,6 +1039,11 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	struct spi_nor *nor;
>  	struct mtd_info *mtd;
>  	int ret, i = 0;
> +	const struct of_device_id *of_id =
> +			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
> +	enum read_mode readmode;
> +	unsigned int buswidth;
> +	u32 qspimaxfreq, spimaxfreq;
>  
>  	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>  	if (!q)
> @@ -1050,6 +1114,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  
>  	mutex_init(&q->lock);
>  
> +	/* Running MIN of all "spi-max-frequency' values present in subnodes */
> +	qspimaxfreq = INT_MAX;
> +
>  	/* iterate the subnodes. */
>  	for_each_available_child_of_node(dev->of_node, np) {
>  		/* skip the holes */
> @@ -1073,15 +1140,44 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		nor->prepare = fsl_qspi_prep;
>  		nor->unprepare = fsl_qspi_unprep;
>  
> +		/* Update running MIN of subnode max frequencies */
>  		ret = of_property_read_u32(np, "spi-max-frequency",
> -				&q->clk_rate);
> +				&spimaxfreq);
>  		if (ret < 0)
>  			goto mutex_failed;
> +		qspimaxfreq = (qspimaxfreq > spimaxfreq) ? spimaxfreq: qspimaxfreq;
>  
>  		/* set the chip address for READID */
>  		fsl_qspi_set_base_addr(q, nor);
>  
> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +		/* scan using the proper read mode for this subnode */
> +		if (of_property_read_u32(np, "spi-bus-width", &buswidth)<0) {
> +			nor->read_opcode = SPINOR_OP_READ;
> +			readmode = SPI_NOR_NORMAL;
> +		} else {
> +			switch (buswidth) {
> +			case 4:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_4;
> +				readmode = SPI_NOR_QUAD;
> +				break;
> +			case 2:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_2;
> +				readmode = SPI_NOR_DUAL;
> +				break;
> +			case 1:
> +				nor->read_opcode = SPINOR_OP_READ_FAST;
> +				readmode = SPI_NOR_FAST;
> +				break;
> +			default:
> +				dev_warn(q->dev,
> +					"ignoring spi-bus-width=%d (should be 1, 2 or 4)",
> +					buswidth);
> +				nor->read_opcode = SPINOR_OP_READ;
> +				readmode = SPI_NOR_NORMAL;
> +			}
> +		}
> +
> +		ret = spi_nor_scan(nor, NULL, readmode);
>  		if (ret)
>  			goto mutex_failed;
>  
> @@ -1112,6 +1208,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> +	/* If we know the lowest subnode frequency, override driver default. */
> +	if (qspimaxfreq<INT_MAX)
> +		q->clk_rate = qspimaxfreq;
> +
>  	/* finish the rest init. */
>  	ret = fsl_qspi_nor_setup_last(q);
>  	if (ret)
> 

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Albert ARIBAUD]

Hi Rob and Geert,

Le Mon, 3 Oct 2016 21:57:58 +0200, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> a écrit :

> On Mon, Oct 3, 2016 at 8:51 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Tue, Sep 27, 2016 at 07:59:57AM +0200, Albert ARIBAUD (3ADEV) wrote:  
> >> Introduce spi-bus-width property for bus subnodes, to
> >> specify per-bus capability to use NORMAL, FAST, DUAL,
> >> and/or QUAD reads.
> >>
> >> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
> >> ---
> >>  Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)  
> >
> > Make this a common property.  

@Rob: do you mean common to all slaves, i.e. described
in ./Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt?

> They already exist:
> 
> Documentation/devicetree/bindings/spi/spi-bus.txt:
> - spi-tx-bus-width - (optional) The bus width (number of data wires)
> that is used for MOSI. Defaults to 1 if not present.
> - spi-rx-bus-width - (optional) The bus width (number of data wires)
> that is used for MISO. Defaults to 1 if not present.
> 
> The above are for normal/dual/quad.
> 
> "Fast" is not a property of the bus, but of the SPI slave, right? Cfr.
> 
> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt:
> - m25p,fast-read : Use the "fast read" opcode to read data from the
> chip instead of the usual "read" opcode. This opcode is not supported
> by all chips and support for it can not be detected at runtime.
>                    Refer to your chips' datasheet to check if this is
> supported by your chip.
> 
> Gr{oetje,eeting}s,

@Geert: the problem here is that on the board for which I wrote this
patch, only one of the two NOR slaves can do quad SPI; the other one
can do dual at best. Setting a bus property would prevent the
quad-capable device from doing actual quad reads.

The fsl-quadspi driver already supports per-NOR read and erase commands
to support heterogeneous NOR setups. A per-NOR bus width property would
make sense in this light.

Cordialement,
Albert ARIBAUD
3ADEV
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Albert ARIBAUD]

Hi Rob and Geert,

Le Mon, 3 Oct 2016 21:57:58 +0200, Geert Uytterhoeven
<geert@linux-m68k.org> a écrit :

> On Mon, Oct 3, 2016 at 8:51 PM, Rob Herring <robh@kernel.org> wrote:
> > On Tue, Sep 27, 2016 at 07:59:57AM +0200, Albert ARIBAUD (3ADEV) wrote:  
> >> Introduce spi-bus-width property for bus subnodes, to
> >> specify per-bus capability to use NORMAL, FAST, DUAL,
> >> and/or QUAD reads.
> >>
> >> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
> >> ---
> >>  Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)  
> >
> > Make this a common property.  

@Rob: do you mean common to all slaves, i.e. described
in ./Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt?

> They already exist:
> 
> Documentation/devicetree/bindings/spi/spi-bus.txt:
> - spi-tx-bus-width - (optional) The bus width (number of data wires)
> that is used for MOSI. Defaults to 1 if not present.
> - spi-rx-bus-width - (optional) The bus width (number of data wires)
> that is used for MISO. Defaults to 1 if not present.
> 
> The above are for normal/dual/quad.
> 
> "Fast" is not a property of the bus, but of the SPI slave, right? Cfr.
> 
> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt:
> - m25p,fast-read : Use the "fast read" opcode to read data from the
> chip instead of the usual "read" opcode. This opcode is not supported
> by all chips and support for it can not be detected at runtime.
>                    Refer to your chips' datasheet to check if this is
> supported by your chip.
> 
> Gr{oetje,eeting}s,

@Geert: the problem here is that on the board for which I wrote this
patch, only one of the two NOR slaves can do quad SPI; the other one
can do dual at best. Setting a bus property would prevent the
quad-capable device from doing actual quad reads.

The fsl-quadspi driver already supports per-NOR read and erase commands
to support heterogeneous NOR setups. A per-NOR bus width property would
make sense in this light.

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Geert Uytterhoeven]

On Tue, Oct 18, 2016 at 10:03 PM, Albert ARIBAUD
<albert.aribaud-iEu9NFBzPZE@public.gmane.org> wrote:
> Le Mon, 3 Oct 2016 21:57:58 +0200, Geert Uytterhoeven
> <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> a écrit :
>
>> On Mon, Oct 3, 2016 at 8:51 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Tue, Sep 27, 2016 at 07:59:57AM +0200, Albert ARIBAUD (3ADEV) wrote:
>> >> Introduce spi-bus-width property for bus subnodes, to
>> >> specify per-bus capability to use NORMAL, FAST, DUAL,
>> >> and/or QUAD reads.
>> >>
>> >> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
>> >> ---
>> >>  Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
>> >>  1 file changed, 18 insertions(+)
>> >
>> > Make this a common property.
>
> @Rob: do you mean common to all slaves, i.e. described
> in ./Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt?
>
>> They already exist:
>>
>> Documentation/devicetree/bindings/spi/spi-bus.txt:
>> - spi-tx-bus-width - (optional) The bus width (number of data wires)
>> that is used for MOSI. Defaults to 1 if not present.
>> - spi-rx-bus-width - (optional) The bus width (number of data wires)
>> that is used for MISO. Defaults to 1 if not present.
>>
>> The above are for normal/dual/quad.
>>
>> "Fast" is not a property of the bus, but of the SPI slave, right? Cfr.
>>
>> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt:
>> - m25p,fast-read : Use the "fast read" opcode to read data from the
>> chip instead of the usual "read" opcode. This opcode is not supported
>> by all chips and support for it can not be detected at runtime.
>>                    Refer to your chips' datasheet to check if this is
>> supported by your chip.
>>
>> Gr{oetje,eeting}s,
>
> @Geert: the problem here is that on the board for which I wrote this
> patch, only one of the two NOR slaves can do quad SPI; the other one
> can do dual at best. Setting a bus property would prevent the
> quad-capable device from doing actual quad reads.
>
> The fsl-quadspi driver already supports per-NOR read and erase commands
> to support heterogeneous NOR setups. A per-NOR bus width property would
> make sense in this light.

spi-[tx]x-bus-width is already a property for the SPI slave, not for
the whole bus.
So you can use 4 for the quad-capable device, and 1 or 2 for the other.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Geert Uytterhoeven]

On Tue, Oct 18, 2016 at 10:03 PM, Albert ARIBAUD
<albert.aribaud@3adev.fr> wrote:
> Le Mon, 3 Oct 2016 21:57:58 +0200, Geert Uytterhoeven
> <geert@linux-m68k.org> a écrit :
>
>> On Mon, Oct 3, 2016 at 8:51 PM, Rob Herring <robh@kernel.org> wrote:
>> > On Tue, Sep 27, 2016 at 07:59:57AM +0200, Albert ARIBAUD (3ADEV) wrote:
>> >> Introduce spi-bus-width property for bus subnodes, to
>> >> specify per-bus capability to use NORMAL, FAST, DUAL,
>> >> and/or QUAD reads.
>> >>
>> >> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
>> >> ---
>> >>  Documentation/devicetree/bindings/mtd/fsl-quadspi.txt | 18 ++++++++++++++++++
>> >>  1 file changed, 18 insertions(+)
>> >
>> > Make this a common property.
>
> @Rob: do you mean common to all slaves, i.e. described
> in ./Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt?
>
>> They already exist:
>>
>> Documentation/devicetree/bindings/spi/spi-bus.txt:
>> - spi-tx-bus-width - (optional) The bus width (number of data wires)
>> that is used for MOSI. Defaults to 1 if not present.
>> - spi-rx-bus-width - (optional) The bus width (number of data wires)
>> that is used for MISO. Defaults to 1 if not present.
>>
>> The above are for normal/dual/quad.
>>
>> "Fast" is not a property of the bus, but of the SPI slave, right? Cfr.
>>
>> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt:
>> - m25p,fast-read : Use the "fast read" opcode to read data from the
>> chip instead of the usual "read" opcode. This opcode is not supported
>> by all chips and support for it can not be detected at runtime.
>>                    Refer to your chips' datasheet to check if this is
>> supported by your chip.
>>
>> Gr{oetje,eeting}s,
>
> @Geert: the problem here is that on the board for which I wrote this
> patch, only one of the two NOR slaves can do quad SPI; the other one
> can do dual at best. Setting a bus property would prevent the
> quad-capable device from doing actual quad reads.
>
> The fsl-quadspi driver already supports per-NOR read and erase commands
> to support heterogeneous NOR setups. A per-NOR bus width property would
> make sense in this light.

spi-[tx]x-bus-width is already a property for the SPI slave, not for
the whole bus.
So you can use 4 for the quad-capable device, and 1 or 2 for the other.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Cyrille Pitchen]

Hi Albert,

+Yunhui

Le 27/09/2016 à 07:59, Albert ARIBAUD (3ADEV) a écrit :
> Fix QUAD read LUT sequence missing JMP_ON_CS which caused
> read corruptions with non-1K-size reads on BK4R1 machine.
> 
> Add NORMAL, DUAL and FAST read sequences.
> 
> Introduce spi-bus-width property for bus subnodes, to
> specify per-bus capability to use NORMAL, FAST, DUAL,
> and/or QUAD reads.
> 
> Also:
> 
> Simplify ioremap/memcpy usage.
> 
> Make controller clock frequency equal to the lowest of
> subnode max frequencies instead of the last one.
> 
> Limit Vybrid to use only half its RX fifo, using more will
> cause corrupt reads.
> 
> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud-iEu9NFBzPZE@public.gmane.org>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 204 ++++++++++++++++++++++++++++----------
>  1 file changed, 152 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 5c82e4e..9811cf2 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -41,6 +41,8 @@
>  #define QUADSPI_QUIRK_TKT253890		(1 << 2)
>  /* Controller cannot wake up from wait mode, TKT245618 */
>  #define QUADSPI_QUIRK_TKT245618         (1 << 3)
> +/* Controller can only read half a rx fifo through AHB */
> +#define QUADSPI_QUIRK_AHB_READ_HALF_FIFO  (1 << 4)
>  
>  /* The registers */
>  #define QUADSPI_MCR			0x00
> @@ -117,6 +119,10 @@
>  #define QUADSPI_FR			0x160
>  #define QUADSPI_FR_TFF_MASK		0x1
>  
> +#define QUADSPI_SPTRCLR			0x15c
> +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
> +#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> +
>  #define QUADSPI_SFA1AD			0x180
>  #define QUADSPI_SFA2AD			0x184
>  #define QUADSPI_SFB1AD			0x188
> @@ -193,7 +199,7 @@
>  #define QUADSPI_LUT_NUM		64
>  
>  /* SEQID -- we can have 16 seqids at most. */
> -#define SEQID_QUAD_READ		0
> +#define SEQID_READ		0
>  #define SEQID_WREN		1
>  #define SEQID_WRDI		2
>  #define SEQID_RDSR		3
> @@ -205,6 +211,9 @@
>  #define SEQID_RDCR		9
>  #define SEQID_EN4B		10
>  #define SEQID_BRWR		11
> +#define SEQID_FAST_READ		12
> +#define SEQID_DUAL_READ		13
> +#define SEQID_QUAD_READ		14
>  
>  #define QUADSPI_MIN_IOMAP SZ_4M
>  
> @@ -229,7 +238,8 @@ static struct fsl_qspi_devtype_data vybrid_data = {
>  	.rxfifo = 128,
>  	.txfifo = 64,
>  	.ahb_buf_size = 1024,
> -	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
> +	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN
> +		       | QUADSPI_QUIRK_AHB_READ_HALF_FIFO,
>  };
>  
>  static struct fsl_qspi_devtype_data imx6sx_data = {
> @@ -309,6 +319,11 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
>  	return q->devtype_data->driver_data & QUADSPI_QUIRK_TKT245618;
>  }
>  
> +static inline int needs_half_fifo(struct fsl_qspi *q)
> +{
> +	return q->devtype_data->driver_data & QUADSPI_QUIRK_AHB_READ_HALF_FIFO;
> +}
> +
>  /*
>   * R/W functions for big- or little-endian registers:
>   * The qSPI controller's endian is independent of the CPU core's endian.
> @@ -373,8 +388,20 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	void __iomem *base = q->iobase;
>  	int rxfifo = q->devtype_data->rxfifo;
>  	u32 lut_base;
> -	u8 cmd, addrlen, dummy;
> +	u8 cmd, addrlen;
>  	int i;
> +	/* use 8 dummy cycles for all fast operations */
> +	const int dummy = 8;
> +
> +	/* use only half fifo if controller needs that */
> +	if (needs_half_fifo(q))
> +		rxfifo /= 2;
> +
> +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> +	if (q->nor_size <= SZ_16M)
> +		addrlen = ADDR24BIT;
> +	else
> +		addrlen = ADDR32BIT;
>  
>  	fsl_qspi_unlock_lut(q);
>  
> @@ -382,24 +409,12 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	for (i = 0; i < QUADSPI_LUT_NUM; i++)
>  		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
>  
> -	/* Quad Read */
> -	lut_base = SEQID_QUAD_READ * 4;
> -
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR24BIT;
> -		dummy = 8;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR32BIT;
> -		dummy = 8;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> -			base + QUADSPI_LUT(lut_base + 1));
> +	/* Read */
> +	lut_base = SEQID_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(FSL_READ, PAD1, rxfifo) | LUT1(JMP_ON_CS, PAD1, SEQID_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 1));

It seems that both Yunhui and you work on the same topic, maybe you can
synchronize?
https://patchwork.ozlabs.org/patch/660356/

>  
>  	/* Write enable */
>  	lut_base = SEQID_WREN * 4;
> @@ -409,16 +424,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	/* Page Program */
>  	lut_base = SEQID_PP * 4;
>  
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR24BIT;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR32BIT;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_PP) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
>  	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
>  			base + QUADSPI_LUT(lut_base + 1));
> @@ -431,7 +437,6 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  
>  	/* Erase a sector */
>  	lut_base = SEQID_SE * 4;
> -
>  	cmd = q->nor[0].erase_opcode;
>  	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
>  
> @@ -476,6 +481,33 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
>  			base + QUADSPI_LUT(lut_base));
>  
> +	/* Fast Read */
> +	lut_base = SEQID_FAST_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_FAST) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD1, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_FAST_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Dual Read */
> +	lut_base = SEQID_DUAL_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_2) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD2, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_DUAL_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Quad Read */
> +	lut_base = SEQID_QUAD_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_4) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_QUAD_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
>  	fsl_qspi_lock_lut(q);
>  }
>  
> @@ -483,8 +515,8 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  {
>  	switch (cmd) {
> -	case SPINOR_OP_READ_1_1_4:
> -		return SEQID_QUAD_READ;
> +	case SPINOR_OP_READ:
> +		return SEQID_READ;
>  	case SPINOR_OP_WREN:
>  		return SEQID_WREN;
>  	case SPINOR_OP_WRDI:
> @@ -507,6 +539,12 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  		return SEQID_EN4B;
>  	case SPINOR_OP_BRWR:
>  		return SEQID_BRWR;
> +	case SPINOR_OP_READ_FAST:
> +		return SEQID_FAST_READ;
> +	case SPINOR_OP_READ_1_1_2:
> +		return SEQID_DUAL_READ;
> +	case SPINOR_OP_READ_1_1_4:
> +		return SEQID_QUAD_READ;

I don't think it is a good thing to select the relevant "READ" LUT entry
according to the command op code. What if spi-nor.c introduces new op codes?

Maybe you could index the LUT entries by functions:
- READ_REG: the LUT op code would be dynamically updated by your
            .read_reg(nor, opcode, ...) implementation according to opcode.
- WRITE_REG: the same as above with the .write_reg(nor, opcode, ...) hook.
- READ_SLAVE1: read memory from the first SPI-NOR memory.
- WRITE_SLAVE1: write into the first SPI-NOR memory.
- READ_SLAVE2: read memory form the 2nd SPI-NOR memory.
...
- WRITE_SLAVEN: write into the N-th SPI-NOR memory.

Also be aware even for the .read() hook the nor->read_opcode might changes
between calls!

I don't know whether those comments fit your actual needs and the Freescale SPI
controller hardware constraints but I'm always worried about the ease to maintain
SPI-NOR controller drivers when they are not "op code agnostic".

Best regards,

Cyrille

>  	default:
>  		if (cmd == q->nor[0].erase_opcode)
>  			return SEQID_SE;
> @@ -676,11 +714,16 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
>   * the memcpy to read the data directly. A "missed" access to the buffer
>   * causes the controller to clear the buffer, and use the sequence pointed
>   * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
> + *
> + * When using two ports, the SEQID to use for one port might differ from
> + * the one to use for the other (e.g., if one port can do 4-pad reads but
> + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> + * will set up the proper SEQID for the port right before doing the AHB
> + * access(es).
>   */
>  static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  {
>  	void __iomem *base = q->iobase;
> -	int seqid;
>  
>  	/* AHB configuration for access buffer 0/1/2 .*/
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
> @@ -688,11 +731,13 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
>  	/*
>  	 * Set ADATSZ with the maximum AHB buffer size to improve the
> -	 * read performance.
> +	 * read performance, except when the controller should not use
> +	 * more than half its RX fifo in AHB reads, in which case read
> +	 * size is given in the LUT FSL_READ instructions.
>  	 */
>  	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
> -			((q->devtype_data->ahb_buf_size / 8)
> -			<< QUADSPI_BUF3CR_ADATSZ_SHIFT),
> +			(needs_half_fifo(q)? 0 : q->devtype_data->ahb_buf_size / 8)
> +	        << QUADSPI_BUF3CR_ADATSZ_SHIFT,
>  			base + QUADSPI_BUF3CR);
>  
>  	/* We only use the buffer3 */
> @@ -700,9 +745,8 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
>  	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
>  
> -	/* Set the default lut sequence for AHB Read. */
> -	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
> -	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +	/* Set the default lut sequence for AHB Read to READ. */
> +	qspi_writel(q, SEQID_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
>  		q->iobase + QUADSPI_BFGENCR);
>  }
>  
> @@ -841,6 +885,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>  		return ret;
>  
>  	fsl_qspi_read_data(q, len, buf);
> +
>  	return 0;
>  }
>  
> @@ -887,11 +932,27 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  {
>  	struct fsl_qspi *q = nor->priv;
>  	u8 cmd = nor->read_opcode;
> +	uint32_t seqid;
> +	size_t qlen = q->nor_size * 4;
> +	int nor_idx = nor - q->nor;
> +	size_t nor_ofs = q->nor_size * nor_idx;
> +
> +	/* Set the actual lut sequence for AHB Read from the considered nor. */
> +	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
> +	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +		q->iobase + QUADSPI_BFGENCR);
> +	/* Reset the AHB sequence pointer */
> +	qspi_writel(q, QUADSPI_SPTRCLR_BFPTRC_MASK, q->iobase + QUADSPI_SPTRCLR);
> +    /* make sure the Rx buffer is read through AHB, not IP */
> +	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, q->iobase + QUADSPI_RBCT);
> +
> +	/* set the chip address for READID */
> +	fsl_qspi_set_base_addr(q, q->nor);
>  
>  	/* if necessary,ioremap buffer before AHB read, */
>  	if (!q->ahb_addr) {
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
> @@ -906,8 +967,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  			q->memmap_offs + q->memmap_len) {
>  		iounmap(q->ahb_addr);
>  
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
>  				q->memmap_len);
> @@ -917,13 +978,11 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  		}
>  	}
>  
> -	dev_dbg(q->dev, "cmd [%x],read from %p, len:%zd\n",
> -		cmd, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	dev_dbg(q->dev, "cmd [%x], read from %p, len:%zd\n",
> +		cmd, q->ahb_addr + nor_ofs + from, len);
>  
>  	/* Read out the data directly from the AHB buffer.*/
> -	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	memcpy(buf, q->ahb_addr + nor_ofs + from, len);
>  
>  	return len;
>  }
> @@ -980,6 +1039,11 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	struct spi_nor *nor;
>  	struct mtd_info *mtd;
>  	int ret, i = 0;
> +	const struct of_device_id *of_id =
> +			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
> +	enum read_mode readmode;
> +	unsigned int buswidth;
> +	u32 qspimaxfreq, spimaxfreq;
>  
>  	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>  	if (!q)
> @@ -1050,6 +1114,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  
>  	mutex_init(&q->lock);
>  
> +	/* Running MIN of all "spi-max-frequency' values present in subnodes */
> +	qspimaxfreq = INT_MAX;
> +
>  	/* iterate the subnodes. */
>  	for_each_available_child_of_node(dev->of_node, np) {
>  		/* skip the holes */
> @@ -1073,15 +1140,44 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		nor->prepare = fsl_qspi_prep;
>  		nor->unprepare = fsl_qspi_unprep;
>  
> +		/* Update running MIN of subnode max frequencies */
>  		ret = of_property_read_u32(np, "spi-max-frequency",
> -				&q->clk_rate);
> +				&spimaxfreq);
>  		if (ret < 0)
>  			goto mutex_failed;
> +		qspimaxfreq = (qspimaxfreq > spimaxfreq) ? spimaxfreq: qspimaxfreq;
>  
>  		/* set the chip address for READID */
>  		fsl_qspi_set_base_addr(q, nor);
>  
> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +		/* scan using the proper read mode for this subnode */
> +		if (of_property_read_u32(np, "spi-bus-width", &buswidth)<0) {
> +			nor->read_opcode = SPINOR_OP_READ;
> +			readmode = SPI_NOR_NORMAL;
> +		} else {
> +			switch (buswidth) {
> +			case 4:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_4;
> +				readmode = SPI_NOR_QUAD;
> +				break;
> +			case 2:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_2;
> +				readmode = SPI_NOR_DUAL;
> +				break;
> +			case 1:
> +				nor->read_opcode = SPINOR_OP_READ_FAST;
> +				readmode = SPI_NOR_FAST;
> +				break;
> +			default:
> +				dev_warn(q->dev,
> +					"ignoring spi-bus-width=%d (should be 1, 2 or 4)",
> +					buswidth);
> +				nor->read_opcode = SPINOR_OP_READ;
> +				readmode = SPI_NOR_NORMAL;
> +			}
> +		}
> +
> +		ret = spi_nor_scan(nor, NULL, readmode);
>  		if (ret)
>  			goto mutex_failed;
>  
> @@ -1112,6 +1208,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> +	/* If we know the lowest subnode frequency, override driver default. */
> +	if (qspimaxfreq<INT_MAX)
> +		q->clk_rate = qspimaxfreq;
> +
>  	/* finish the rest init. */
>  	ret = fsl_qspi_nor_setup_last(q);
>  	if (ret)
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Cyrille Pitchen]

Hi Albert,

+Yunhui

Le 27/09/2016 à 07:59, Albert ARIBAUD (3ADEV) a écrit :
> Fix QUAD read LUT sequence missing JMP_ON_CS which caused
> read corruptions with non-1K-size reads on BK4R1 machine.
> 
> Add NORMAL, DUAL and FAST read sequences.
> 
> Introduce spi-bus-width property for bus subnodes, to
> specify per-bus capability to use NORMAL, FAST, DUAL,
> and/or QUAD reads.
> 
> Also:
> 
> Simplify ioremap/memcpy usage.
> 
> Make controller clock frequency equal to the lowest of
> subnode max frequencies instead of the last one.
> 
> Limit Vybrid to use only half its RX fifo, using more will
> cause corrupt reads.
> 
> Signed-off-by: Albert ARIBAUD (3ADEV) <albert.aribaud@3adev.fr>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 204 ++++++++++++++++++++++++++++----------
>  1 file changed, 152 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 5c82e4e..9811cf2 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -41,6 +41,8 @@
>  #define QUADSPI_QUIRK_TKT253890		(1 << 2)
>  /* Controller cannot wake up from wait mode, TKT245618 */
>  #define QUADSPI_QUIRK_TKT245618         (1 << 3)
> +/* Controller can only read half a rx fifo through AHB */
> +#define QUADSPI_QUIRK_AHB_READ_HALF_FIFO  (1 << 4)
>  
>  /* The registers */
>  #define QUADSPI_MCR			0x00
> @@ -117,6 +119,10 @@
>  #define QUADSPI_FR			0x160
>  #define QUADSPI_FR_TFF_MASK		0x1
>  
> +#define QUADSPI_SPTRCLR			0x15c
> +#define QUADSPI_SPTRCLR_BFPTRC_SHIFT		0
> +#define QUADSPI_SPTRCLR_BFPTRC_MASK		(0x1 << QUADSPI_SPTRCLR_BFPTRC_SHIFT)
> +
>  #define QUADSPI_SFA1AD			0x180
>  #define QUADSPI_SFA2AD			0x184
>  #define QUADSPI_SFB1AD			0x188
> @@ -193,7 +199,7 @@
>  #define QUADSPI_LUT_NUM		64
>  
>  /* SEQID -- we can have 16 seqids at most. */
> -#define SEQID_QUAD_READ		0
> +#define SEQID_READ		0
>  #define SEQID_WREN		1
>  #define SEQID_WRDI		2
>  #define SEQID_RDSR		3
> @@ -205,6 +211,9 @@
>  #define SEQID_RDCR		9
>  #define SEQID_EN4B		10
>  #define SEQID_BRWR		11
> +#define SEQID_FAST_READ		12
> +#define SEQID_DUAL_READ		13
> +#define SEQID_QUAD_READ		14
>  
>  #define QUADSPI_MIN_IOMAP SZ_4M
>  
> @@ -229,7 +238,8 @@ static struct fsl_qspi_devtype_data vybrid_data = {
>  	.rxfifo = 128,
>  	.txfifo = 64,
>  	.ahb_buf_size = 1024,
> -	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN,
> +	.driver_data = QUADSPI_QUIRK_SWAP_ENDIAN
> +		       | QUADSPI_QUIRK_AHB_READ_HALF_FIFO,
>  };
>  
>  static struct fsl_qspi_devtype_data imx6sx_data = {
> @@ -309,6 +319,11 @@ static inline int needs_wakeup_wait_mode(struct fsl_qspi *q)
>  	return q->devtype_data->driver_data & QUADSPI_QUIRK_TKT245618;
>  }
>  
> +static inline int needs_half_fifo(struct fsl_qspi *q)
> +{
> +	return q->devtype_data->driver_data & QUADSPI_QUIRK_AHB_READ_HALF_FIFO;
> +}
> +
>  /*
>   * R/W functions for big- or little-endian registers:
>   * The qSPI controller's endian is independent of the CPU core's endian.
> @@ -373,8 +388,20 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	void __iomem *base = q->iobase;
>  	int rxfifo = q->devtype_data->rxfifo;
>  	u32 lut_base;
> -	u8 cmd, addrlen, dummy;
> +	u8 cmd, addrlen;
>  	int i;
> +	/* use 8 dummy cycles for all fast operations */
> +	const int dummy = 8;
> +
> +	/* use only half fifo if controller needs that */
> +	if (needs_half_fifo(q))
> +		rxfifo /= 2;
> +
> +    /* use 24-bit addresses for up to 16MB, 32-bit above 16MB */
> +	if (q->nor_size <= SZ_16M)
> +		addrlen = ADDR24BIT;
> +	else
> +		addrlen = ADDR32BIT;
>  
>  	fsl_qspi_unlock_lut(q);
>  
> @@ -382,24 +409,12 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	for (i = 0; i < QUADSPI_LUT_NUM; i++)
>  		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
>  
> -	/* Quad Read */
> -	lut_base = SEQID_QUAD_READ * 4;
> -
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR24BIT;
> -		dummy = 8;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_READ_1_1_4;
> -		addrlen = ADDR32BIT;
> -		dummy = 8;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> -			base + QUADSPI_LUT(lut_base + 1));
> +	/* Read */
> +	lut_base = SEQID_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(FSL_READ, PAD1, rxfifo) | LUT1(JMP_ON_CS, PAD1, SEQID_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 1));

It seems that both Yunhui and you work on the same topic, maybe you can
synchronize?
https://patchwork.ozlabs.org/patch/660356/

>  
>  	/* Write enable */
>  	lut_base = SEQID_WREN * 4;
> @@ -409,16 +424,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	/* Page Program */
>  	lut_base = SEQID_PP * 4;
>  
> -	if (q->nor_size <= SZ_16M) {
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR24BIT;
> -	} else {
> -		/* use the 4-byte address */
> -		cmd = SPINOR_OP_PP;
> -		addrlen = ADDR32BIT;
> -	}
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_PP) | LUT1(ADDR, PAD1, addrlen),
>  			base + QUADSPI_LUT(lut_base));
>  	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
>  			base + QUADSPI_LUT(lut_base + 1));
> @@ -431,7 +437,6 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  
>  	/* Erase a sector */
>  	lut_base = SEQID_SE * 4;
> -
>  	cmd = q->nor[0].erase_opcode;
>  	addrlen = q->nor_size <= SZ_16M ? ADDR24BIT : ADDR32BIT;
>  
> @@ -476,6 +481,33 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
>  			base + QUADSPI_LUT(lut_base));
>  
> +	/* Fast Read */
> +	lut_base = SEQID_FAST_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_FAST) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD1, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_FAST_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Dual Read */
> +	lut_base = SEQID_DUAL_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_2) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD2, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_DUAL_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
> +	/* Quad Read */
> +	lut_base = SEQID_QUAD_READ * 4;
> +	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_READ_1_1_4) | LUT1(ADDR, PAD1, addrlen),
> +		base + QUADSPI_LUT(lut_base));
> +	qspi_writel(q, LUT0(DUMMY, PAD1, dummy) | LUT1(FSL_READ, PAD4, rxfifo),
> +		base + QUADSPI_LUT(lut_base + 1));
> +	qspi_writel(q, LUT0(JMP_ON_CS, PAD1, SEQID_QUAD_READ * 8),
> +		base + QUADSPI_LUT(lut_base + 2));
> +
>  	fsl_qspi_lock_lut(q);
>  }
>  
> @@ -483,8 +515,8 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
>  static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  {
>  	switch (cmd) {
> -	case SPINOR_OP_READ_1_1_4:
> -		return SEQID_QUAD_READ;
> +	case SPINOR_OP_READ:
> +		return SEQID_READ;
>  	case SPINOR_OP_WREN:
>  		return SEQID_WREN;
>  	case SPINOR_OP_WRDI:
> @@ -507,6 +539,12 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>  		return SEQID_EN4B;
>  	case SPINOR_OP_BRWR:
>  		return SEQID_BRWR;
> +	case SPINOR_OP_READ_FAST:
> +		return SEQID_FAST_READ;
> +	case SPINOR_OP_READ_1_1_2:
> +		return SEQID_DUAL_READ;
> +	case SPINOR_OP_READ_1_1_4:
> +		return SEQID_QUAD_READ;

I don't think it is a good thing to select the relevant "READ" LUT entry
according to the command op code. What if spi-nor.c introduces new op codes?

Maybe you could index the LUT entries by functions:
- READ_REG: the LUT op code would be dynamically updated by your
            .read_reg(nor, opcode, ...) implementation according to opcode.
- WRITE_REG: the same as above with the .write_reg(nor, opcode, ...) hook.
- READ_SLAVE1: read memory from the first SPI-NOR memory.
- WRITE_SLAVE1: write into the first SPI-NOR memory.
- READ_SLAVE2: read memory form the 2nd SPI-NOR memory.
...
- WRITE_SLAVEN: write into the N-th SPI-NOR memory.

Also be aware even for the .read() hook the nor->read_opcode might changes
between calls!

I don't know whether those comments fit your actual needs and the Freescale SPI
controller hardware constraints but I'm always worried about the ease to maintain
SPI-NOR controller drivers when they are not "op code agnostic".

Best regards,

Cyrille

>  	default:
>  		if (cmd == q->nor[0].erase_opcode)
>  			return SEQID_SE;
> @@ -676,11 +714,16 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
>   * the memcpy to read the data directly. A "missed" access to the buffer
>   * causes the controller to clear the buffer, and use the sequence pointed
>   * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
> + *
> + * When using two ports, the SEQID to use for one port might differ from
> + * the one to use for the other (e.g., if one port can do 4-pad reads but
> + * the other cannot). So we set up a basic mode here (SEQID_READ) and we
> + * will set up the proper SEQID for the port right before doing the AHB
> + * access(es).
>   */
>  static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  {
>  	void __iomem *base = q->iobase;
> -	int seqid;
>  
>  	/* AHB configuration for access buffer 0/1/2 .*/
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
> @@ -688,11 +731,13 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
>  	/*
>  	 * Set ADATSZ with the maximum AHB buffer size to improve the
> -	 * read performance.
> +	 * read performance, except when the controller should not use
> +	 * more than half its RX fifo in AHB reads, in which case read
> +	 * size is given in the LUT FSL_READ instructions.
>  	 */
>  	qspi_writel(q, QUADSPI_BUF3CR_ALLMST_MASK |
> -			((q->devtype_data->ahb_buf_size / 8)
> -			<< QUADSPI_BUF3CR_ADATSZ_SHIFT),
> +			(needs_half_fifo(q)? 0 : q->devtype_data->ahb_buf_size / 8)
> +	        << QUADSPI_BUF3CR_ADATSZ_SHIFT,
>  			base + QUADSPI_BUF3CR);
>  
>  	/* We only use the buffer3 */
> @@ -700,9 +745,8 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
>  	qspi_writel(q, 0, base + QUADSPI_BUF1IND);
>  	qspi_writel(q, 0, base + QUADSPI_BUF2IND);
>  
> -	/* Set the default lut sequence for AHB Read. */
> -	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
> -	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +	/* Set the default lut sequence for AHB Read to READ. */
> +	qspi_writel(q, SEQID_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
>  		q->iobase + QUADSPI_BFGENCR);
>  }
>  
> @@ -841,6 +885,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>  		return ret;
>  
>  	fsl_qspi_read_data(q, len, buf);
> +
>  	return 0;
>  }
>  
> @@ -887,11 +932,27 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  {
>  	struct fsl_qspi *q = nor->priv;
>  	u8 cmd = nor->read_opcode;
> +	uint32_t seqid;
> +	size_t qlen = q->nor_size * 4;
> +	int nor_idx = nor - q->nor;
> +	size_t nor_ofs = q->nor_size * nor_idx;
> +
> +	/* Set the actual lut sequence for AHB Read from the considered nor. */
> +	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
> +	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> +		q->iobase + QUADSPI_BFGENCR);
> +	/* Reset the AHB sequence pointer */
> +	qspi_writel(q, QUADSPI_SPTRCLR_BFPTRC_MASK, q->iobase + QUADSPI_SPTRCLR);
> +    /* make sure the Rx buffer is read through AHB, not IP */
> +	qspi_writel(q, QUADSPI_RBCT_WMRK_MASK, q->iobase + QUADSPI_RBCT);
> +
> +	/* set the chip address for READID */
> +	fsl_qspi_set_base_addr(q, q->nor);
>  
>  	/* if necessary,ioremap buffer before AHB read, */
>  	if (!q->ahb_addr) {
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
> @@ -906,8 +967,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  			q->memmap_offs + q->memmap_len) {
>  		iounmap(q->ahb_addr);
>  
> -		q->memmap_offs = q->chip_base_addr + from;
> -		q->memmap_len = len > QUADSPI_MIN_IOMAP ? len : QUADSPI_MIN_IOMAP;
> +		q->memmap_offs = q->chip_base_addr;
> +		q->memmap_len = qlen > QUADSPI_MIN_IOMAP ? qlen : QUADSPI_MIN_IOMAP;
>  		q->ahb_addr = ioremap_nocache(
>  				q->memmap_phy + q->memmap_offs,
>  				q->memmap_len);
> @@ -917,13 +978,11 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>  		}
>  	}
>  
> -	dev_dbg(q->dev, "cmd [%x],read from %p, len:%zd\n",
> -		cmd, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	dev_dbg(q->dev, "cmd [%x], read from %p, len:%zd\n",
> +		cmd, q->ahb_addr + nor_ofs + from, len);
>  
>  	/* Read out the data directly from the AHB buffer.*/
> -	memcpy(buf, q->ahb_addr + q->chip_base_addr + from - q->memmap_offs,
> -		len);
> +	memcpy(buf, q->ahb_addr + nor_ofs + from, len);
>  
>  	return len;
>  }
> @@ -980,6 +1039,11 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	struct spi_nor *nor;
>  	struct mtd_info *mtd;
>  	int ret, i = 0;
> +	const struct of_device_id *of_id =
> +			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
> +	enum read_mode readmode;
> +	unsigned int buswidth;
> +	u32 qspimaxfreq, spimaxfreq;
>  
>  	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>  	if (!q)
> @@ -1050,6 +1114,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  
>  	mutex_init(&q->lock);
>  
> +	/* Running MIN of all "spi-max-frequency' values present in subnodes */
> +	qspimaxfreq = INT_MAX;
> +
>  	/* iterate the subnodes. */
>  	for_each_available_child_of_node(dev->of_node, np) {
>  		/* skip the holes */
> @@ -1073,15 +1140,44 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		nor->prepare = fsl_qspi_prep;
>  		nor->unprepare = fsl_qspi_unprep;
>  
> +		/* Update running MIN of subnode max frequencies */
>  		ret = of_property_read_u32(np, "spi-max-frequency",
> -				&q->clk_rate);
> +				&spimaxfreq);
>  		if (ret < 0)
>  			goto mutex_failed;
> +		qspimaxfreq = (qspimaxfreq > spimaxfreq) ? spimaxfreq: qspimaxfreq;
>  
>  		/* set the chip address for READID */
>  		fsl_qspi_set_base_addr(q, nor);
>  
> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +		/* scan using the proper read mode for this subnode */
> +		if (of_property_read_u32(np, "spi-bus-width", &buswidth)<0) {
> +			nor->read_opcode = SPINOR_OP_READ;
> +			readmode = SPI_NOR_NORMAL;
> +		} else {
> +			switch (buswidth) {
> +			case 4:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_4;
> +				readmode = SPI_NOR_QUAD;
> +				break;
> +			case 2:
> +				nor->read_opcode = SPINOR_OP_READ_1_1_2;
> +				readmode = SPI_NOR_DUAL;
> +				break;
> +			case 1:
> +				nor->read_opcode = SPINOR_OP_READ_FAST;
> +				readmode = SPI_NOR_FAST;
> +				break;
> +			default:
> +				dev_warn(q->dev,
> +					"ignoring spi-bus-width=%d (should be 1, 2 or 4)",
> +					buswidth);
> +				nor->read_opcode = SPINOR_OP_READ;
> +				readmode = SPI_NOR_NORMAL;
> +			}
> +		}
> +
> +		ret = spi_nor_scan(nor, NULL, readmode);
>  		if (ret)
>  			goto mutex_failed;
>  
> @@ -1112,6 +1208,10 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> +	/* If we know the lowest subnode frequency, override driver default. */
> +	if (qspimaxfreq<INT_MAX)
> +		q->clk_rate = qspimaxfreq;
> +
>  	/* finish the rest init. */
>  	ret = fsl_qspi_nor_setup_last(q);
>  	if (ret)
> 

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Albert ARIBAUD]

Hi Cyrille,

Le Wed, 19 Oct 2016 18:42:36 +0200, Cyrille Pitchen
<cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> a écrit :

> Hi Albert,
> 
> +Yunhui


> It seems that both Yunhui and you work on the same topic, maybe you can
> synchronize?
> https://patchwork.ozlabs.org/patch/660356/

I am already preparing a v2 based over Yunhui's series. :)

> I don't think it is a good thing to select the relevant "READ" LUT entry
> according to the command op code. What if spi-nor.c introduces new op codes?
> 
> Maybe you could index the LUT entries by functions:
> - READ_REG: the LUT op code would be dynamically updated by your
>             .read_reg(nor, opcode, ...) implementation according to opcode.
> - WRITE_REG: the same as above with the .write_reg(nor, opcode, ...) hook.
> - READ_SLAVE1: read memory from the first SPI-NOR memory.
> - WRITE_SLAVE1: write into the first SPI-NOR memory.
> - READ_SLAVE2: read memory form the 2nd SPI-NOR memory.
> ...
> - WRITE_SLAVEN: write into the N-th SPI-NOR memory.

Actually, my needs could be met using Han Xu's dynamic LUT entries
without the need to introduce 'virtual' opcodes (and without having
to 'leak back' those new opcodes into other kernel code portions).

The per-slave differences (in my case, bus width essentially) can be
managed though DT entries and corresponding per-nor flags in the driver.

> Also be aware even for the .read() hook the nor->read_opcode might changes
> between calls!

Noted -- I'll check that the driver always use the current NOR codes.

> I don't know whether those comments fit your actual needs and the Freescale SPI
> controller hardware constraints but I'm always worried about the ease to maintain
> SPI-NOR controller drivers when they are not "op code agnostic".

Understood and agreed.

Thanks for yuor feedback!
 
> Best regards,
> 
> Cyrille

Cordialement,
Albert ARIBAUD
3ADEV
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Albert ARIBAUD]

Hi Cyrille,

Le Wed, 19 Oct 2016 18:42:36 +0200, Cyrille Pitchen
<cyrille.pitchen@atmel.com> a écrit :

> Hi Albert,
> 
> +Yunhui


> It seems that both Yunhui and you work on the same topic, maybe you can
> synchronize?
> https://patchwork.ozlabs.org/patch/660356/

I am already preparing a v2 based over Yunhui's series. :)

> I don't think it is a good thing to select the relevant "READ" LUT entry
> according to the command op code. What if spi-nor.c introduces new op codes?
> 
> Maybe you could index the LUT entries by functions:
> - READ_REG: the LUT op code would be dynamically updated by your
>             .read_reg(nor, opcode, ...) implementation according to opcode.
> - WRITE_REG: the same as above with the .write_reg(nor, opcode, ...) hook.
> - READ_SLAVE1: read memory from the first SPI-NOR memory.
> - WRITE_SLAVE1: write into the first SPI-NOR memory.
> - READ_SLAVE2: read memory form the 2nd SPI-NOR memory.
> ...
> - WRITE_SLAVEN: write into the N-th SPI-NOR memory.

Actually, my needs could be met using Han Xu's dynamic LUT entries
without the need to introduce 'virtual' opcodes (and without having
to 'leak back' those new opcodes into other kernel code portions).

The per-slave differences (in my case, bus width essentially) can be
managed though DT entries and corresponding per-nor flags in the driver.

> Also be aware even for the .read() hook the nor->read_opcode might changes
> between calls!

Noted -- I'll check that the driver always use the current NOR codes.

> I don't know whether those comments fit your actual needs and the Freescale SPI
> controller hardware constraints but I'm always worried about the ease to maintain
> SPI-NOR controller drivers when they are not "op code agnostic".

Understood and agreed.

Thanks for yuor feedback!
 
> Best regards,
> 
> Cyrille

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Albert ARIBAUD]

Hi Geert,

Le Wed, 19 Oct 2016 08:57:27 +0200, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> a écrit :

> > The fsl-quadspi driver already supports per-NOR read and erase commands
> > to support heterogeneous NOR setups. A per-NOR bus width property would
> > make sense in this light.  
> 
> spi-[tx]x-bus-width is already a property for the SPI slave, not for
> the whole bus.
> So you can use 4 for the quad-capable device, and 1 or 2 for the other.

My bad, then -- I'll use these.

> Gr{oetje,eeting}s,
> 
>                         Geert

Cordialement,
Albert ARIBAUD
3ADEV
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] fsl-quadspi: introduce per-bus spi-bus-width property

[Albert ARIBAUD]

Hi Geert,

Le Wed, 19 Oct 2016 08:57:27 +0200, Geert Uytterhoeven
<geert@linux-m68k.org> a écrit :

> > The fsl-quadspi driver already supports per-NOR read and erase commands
> > to support heterogeneous NOR setups. A per-NOR bus width property would
> > make sense in this light.  
> 
> spi-[tx]x-bus-width is already a property for the SPI slave, not for
> the whole bus.
> So you can use 4 for the quad-capable device, and 1 or 2 for the other.

My bad, then -- I'll use these.

> Gr{oetje,eeting}s,
> 
>                         Geert

Cordialement,
Albert ARIBAUD
3ADEV

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Han Xu]

________________________________________
From: Albert ARIBAUD <albert.aribaud@3adev.fr>
Sent: Thursday, October 20, 2016 1:16 AM
To: Cyrille Pitchen
Cc: linux-mtd@lists.infradead.org; Yunhui Cui; devicetree@vger.kernel.org; Han Xu
Subject: Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

Hi Cyrille,

Le Wed, 19 Oct 2016 18:42:36 +0200, Cyrille Pitchen
<cyrille.pitchen@atmel.com> a écrit :

> Hi Albert,
>
> +Yunhui


> It seems that both Yunhui and you work on the same topic, maybe you can
> synchronize?
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F660356%2F&data=01%7C01%7Chan.xu%40nxp.com%7C4b7dcf6422694f68dc1408d3f8b90a4d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=WjiiPqupc9bGtqixGvPeFoBddg51SJwehI4L0Zup9RY%3D&reserved=0

I am already preparing a v2 based over Yunhui's series. :)

> I don't think it is a good thing to select the relevant "READ" LUT entry
> according to the command op code. What if spi-nor.c introduces new op codes?
>
> Maybe you could index the LUT entries by functions:
> - READ_REG: the LUT op code would be dynamically updated by your
>             .read_reg(nor, opcode, ...) implementation according to opcode.
> - WRITE_REG: the same as above with the .write_reg(nor, opcode, ...) hook.
> - READ_SLAVE1: read memory from the first SPI-NOR memory.
> - WRITE_SLAVE1: write into the first SPI-NOR memory.
> - READ_SLAVE2: read memory form the 2nd SPI-NOR memory.
> ...
> - WRITE_SLAVEN: write into the N-th SPI-NOR memory.

Actually, my needs could be met using Han Xu's dynamic LUT entries
without the need to introduce 'virtual' opcodes (and without having
to 'leak back' those new opcodes into other kernel code portions).

The per-slave differences (in my case, bus width essentially) can be
managed though DT entries and corresponding per-nor flags in the driver.

> Also be aware even for the .read() hook the nor->read_opcode might changes
> between calls!

Noted -- I'll check that the driver always use the current NOR codes.

> I don't know whether those comments fit your actual needs and the Freescale SPI
> controller hardware constraints but I'm always worried about the ease to maintain
> SPI-NOR controller drivers when they are not "op code agnostic".

Understood and agreed.

Thanks for yuor feedback!

Good. I have another question about Yunhui's patch set. Who are in charge of review
and merge code change in spi-nor.c? Some patches have been pending for a while.

> Best regards,
>
> Cyrille

Cordialement,
Albert ARIBAUD
3ADEV

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

Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

[Han Xu]

________________________________________
From: Albert ARIBAUD <albert.aribaud@3adev.fr>
Sent: Thursday, October 20, 2016 1:16 AM
To: Cyrille Pitchen
Cc: linux-mtd@lists.infradead.org; Yunhui Cui; devicetree@vger.kernel.org; Han Xu
Subject: Re: [PATCH 1/2] fsl-quadspi: fix QUAD read, add NORMAL, DUAL and FAST reads

Hi Cyrille,

Le Wed, 19 Oct 2016 18:42:36 +0200, Cyrille Pitchen
<cyrille.pitchen@atmel.com> a écrit :

> Hi Albert,
>
> +Yunhui


> It seems that both Yunhui and you work on the same topic, maybe you can
> synchronize?
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F660356%2F&data=01%7C01%7Chan.xu%40nxp.com%7C4b7dcf6422694f68dc1408d3f8b90a4d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=WjiiPqupc9bGtqixGvPeFoBddg51SJwehI4L0Zup9RY%3D&reserved=0

I am already preparing a v2 based over Yunhui's series. :)

> I don't think it is a good thing to select the relevant "READ" LUT entry
> according to the command op code. What if spi-nor.c introduces new op codes?
>
> Maybe you could index the LUT entries by functions:
> - READ_REG: the LUT op code would be dynamically updated by your
>             .read_reg(nor, opcode, ...) implementation according to opcode.
> - WRITE_REG: the same as above with the .write_reg(nor, opcode, ...) hook.
> - READ_SLAVE1: read memory from the first SPI-NOR memory.
> - WRITE_SLAVE1: write into the first SPI-NOR memory.
> - READ_SLAVE2: read memory form the 2nd SPI-NOR memory.
> ...
> - WRITE_SLAVEN: write into the N-th SPI-NOR memory.

Actually, my needs could be met using Han Xu's dynamic LUT entries
without the need to introduce 'virtual' opcodes (and without having
to 'leak back' those new opcodes into other kernel code portions).

The per-slave differences (in my case, bus width essentially) can be
managed though DT entries and corresponding per-nor flags in the driver.

> Also be aware even for the .read() hook the nor->read_opcode might changes
> between calls!

Noted -- I'll check that the driver always use the current NOR codes.

> I don't know whether those comments fit your actual needs and the Freescale SPI
> controller hardware constraints but I'm always worried about the ease to maintain
> SPI-NOR controller drivers when they are not "op code agnostic".

Understood and agreed.

Thanks for yuor feedback!

Good. I have another question about Yunhui's patch set. Who are in charge of review
and merge code change in spi-nor.c? Some patches have been pending for a while.

> Best regards,
>
> Cyrille

Cordialement,
Albert ARIBAUD
3ADEV