[PATCH v7 0/2] mtd: fsl-quadspi: add support to create dynamic LUT entry

[PATCH v7 0/2] mtd: fsl-quadspi: add support to create dynamic LUT entry

[Yogesh Gaur]

fsl-quadspi.c driver creates different LUT entries for various cmds like
read, write, erase, readid etc. These entries are created statically when
driver gets intiliazed.

To support various cmds e.g. different read cmds like SPINOR_OP_READ,
SPINOR_OP_READ_4B, SPINOR_OP_READ_FAST_4B, SPINOR_OP_READ_FAST,
SPINOR_OP_READ_1_1_2, SPINOR_OP_READ_1_2_2 etc driver needs to
have different LUT entries for different cmds.

FSL QUADSPI controller has 4 chip select and one can use different flashes
on all different chip select.
We analyzed that the flashes have different parameters for the same command
and this is true for NAND devices.
Also reg_protocol information, needed for read register, read any register
commands, read SFDP register, not available at time of spi_nor_scan.
As FSL QUADSPI controller has limitation in number of LUT entries, max 16,
thus require to have dynamic LUT creation support.

Thus to have support for different flash devices, this patch-set adds
support to create LUT entry in run-time based on the exact cmnd requested.
Information like cmnd opcode, protocol info, dummy bit info etc required
for creating LUT entries parsed from instance of passed 'struct spi_nor'.

Also fix initialize sequence of AHB read, move
fsl_qspi_init_ahb_read() in  fsl_qspi_nor_setup() function.

Tested with "S25FS512S" flash and have checked read, write, erase
functionality using mtd_debug utility.
For read have checked hwcaps as SNOR_HWCAPS_READ_FAST,
SNOR_HWCAPS_READ_1_2_2 and SNOR_HWCAPS_READ_1_4_4
i.e. read cmds SPINOR_OP_READ_FAST_4B [0x0c],
SPINOR_OP_READ_1_2_2_4B [0xbc] and SPINOR_OP_READ_1_4_4_4B [0xec]

Yogesh Gaur (2):
  mtd: fsl-quadspi: add support to create dynamic LUT entry
  mtd: fsl-quadspi: fix init AHB read in fsl_qspi_nor_setup()

Changes for v7:
- Incorporated Han review comments.

Changes for v6:
- Incorporated Boris review comments.
- Added dedicated LUT for AHB read cmd, required for devmem Read.

Changes for v5:
- Fix LUT preparation for SPINOR_OP_READ and SPINOR_OP_READ_4B cmds.

Changes for v4:
- Drop patch
'mtd: fsl-quadspi: update hwcaps read capabilities as READ_1_4_4', it
would be taken care in next-series.
- Added patch for 'fix initialize sequence of AHB read'.

Changes for v3:
- Add STOP instruction for prepared LUT and remove memset of 4 LUT reg.
- Update information in cover letter.

Changes for v2:
- Swap patch sequences in the series to solve git bissect issue.

 drivers/mtd/spi-nor/fsl-quadspi.c | 484 +++++++++++++++++++++++++-------------
 1 file changed, 324 insertions(+), 160 deletions(-)

-- 
1.9.1

[PATCH v7 1/2] mtd: fsl-quadspi: add support to create dynamic LUT entry

[Yogesh Gaur]

Add support to create dynamic LUT entry.

Current approach of creating LUT entries for various cmds like read, write,
erase, readid, readsr, we, wd etc is that when QSPI controller gets
initialized at that time static LUT entries for these cmds get created.

Patch add support to create the LUT at run time based on the operation
being performed.

Added API fsl_qspi_prepare_lut(), this API would going to be called from
fsl_qspi_read_reg, fsl_qspi_write_reg, fsl_qspi_write, fsl_qspi_read and
fsl_qspi_erase APIs.
Added new struct fsl_qspi_mem_op having fields required to fill in LUT
register for requested command.
Required values for every CMD like opcode, addrlen, dummy_cycles, data_size
and pad bytes being filled in respective calling function and then API
fsl_qspi_prepare_lut fill LUT register for requested command.
Required values are fetched from instance of 'struct spi_nor'.

AHB Read, memory mapped, can be triggered using driver interface as well
from 'devmem' interface.
For AHB read, LUT seq programmed in QUADSPI_BFGENCR register used for
Read operation. For Read initiated from 'devmem' working properly,
required to have dedicated LUT seq for AHB read.

Signed-off-by: Suresh Gupta <suresh.gupta@nxp.com>
Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
---
Changes for v7:
- Incorporated Han review comments.
Changes for v6:
- Incorporated Boris review comments.
- Added dedicated LUT for AHB read cmd, required for devmem Read.
Changes for v5:
- Fix LUT preparation for SPINOR_OP_READ and SPINOR_OP_READ_4B cmds.
Changes for v4:
- Correct version numbering.
Changes for v3:
- Add STOP instruction for prepared LUT and remove memset of 4 LUT reg.
Changes for v2:
- Swap patch sequences in the series to solve git bissect issue.

 drivers/mtd/spi-nor/fsl-quadspi.c | 480 +++++++++++++++++++++++++-------------
 1 file changed, 322 insertions(+), 158 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 89306cf..64b8bb3 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -142,10 +142,14 @@
  *  | INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
  *  ---------------------------------------------------
  */
-#define OPRND0_SHIFT		0
-#define PAD0_SHIFT		8
-#define INSTR0_SHIFT		10
-#define OPRND1_SHIFT		16
+#define PAD_SHIFT		8
+#define INSTR_SHIFT		10
+#define OPRND_SHIFT		16
+
+/* Macros for constructing the LUT register. */
+#define LUT_DEF(idx, ins, pad, opr)			  \
+	((((ins) << INSTR_SHIFT) | ((pad) << PAD_SHIFT) | \
+	(opr)) << (((idx) % 2) * OPRND_SHIFT))
 
 /* Instruction set for the LUT register. */
 #define LUT_STOP		0
@@ -181,30 +185,19 @@
 #define ADDR24BIT		0x18
 #define ADDR32BIT		0x20
 
-/* Macros for constructing the LUT register. */
-#define LUT0(ins, pad, opr)						\
-		(((opr) << OPRND0_SHIFT) | ((LUT_##pad) << PAD0_SHIFT) | \
-		((LUT_##ins) << INSTR0_SHIFT))
-
-#define LUT1(ins, pad, opr)	(LUT0(ins, pad, opr) << OPRND1_SHIFT)
-
 /* other macros for LUT register. */
 #define QUADSPI_LUT(x)          (QUADSPI_LUT_BASE + (x) * 4)
 #define QUADSPI_LUT_NUM		64
 
-/* SEQID -- we can have 16 seqids at most. */
-#define SEQID_READ		0
-#define SEQID_WREN		1
-#define SEQID_WRDI		2
-#define SEQID_RDSR		3
-#define SEQID_SE		4
-#define SEQID_CHIP_ERASE	5
-#define SEQID_PP		6
-#define SEQID_RDID		7
-#define SEQID_WRSR		8
-#define SEQID_RDCR		9
-#define SEQID_EN4B		10
-#define SEQID_BRWR		11
+/*
+ * SEQID -- we can have 16 seqids at most.
+ * LUT0 programmed by bootloader
+ * LUT1 programmed for IP register access cmds
+ * LUT2 programmed for AHB read, required for READ from devmem interface
+ */
+#define SEQID_LUT0_BOOTLOADER	0
+#define SEQID_LUT1_IPCMD	1
+#define SEQID_LUT2_AHBREAD	2
 
 #define QUADSPI_MIN_IOMAP SZ_4M
 
@@ -268,6 +261,68 @@ struct fsl_qspi_devtype_data {
 };
 
 #define FSL_QSPI_MAX_CHIP	4
+
+/* enum qspi_mem_data_dir - describes the direction of a QSPI memory data
+ *			    transfer from QSPI controller prespective.
+ * QSPI_MEM_NO_DATA	- no data transfer initiated
+ * QSPI_MEM_DATA_IN	- data coming from the SPI memory
+ * QSPI_MEM_DATA_OUT	- data sent to the SPI memory
+ */
+enum qspi_mem_data_dir {
+	QSPI_MEM_NO_DATA,
+	QSPI_MEM_DATA_IN,
+	QSPI_MEM_DATA_OUT,
+};
+
+/* enum qspi_cmd__mode - describes the mode of the running command
+ * QSPI_CMD_MODE_IP	- data transfer using IP register access
+ * QSPI_CMD_MODE_AHB	- data transfer via AMBA AHB bus directly
+ */
+enum qspi_cmd_mode {
+	QSPI_CMD_MODE_IP,
+	QSPI_CMD_MODE_AHB,
+};
+
+/* struct fsl_qspi_mem_op - describes the QSPI memory operation
+ * cmd.opcode: operation opcode
+ * cmd.pad: number of IO lines used to transmit the command
+ * addr.addrlen: QSPI working in 3/4-byte mode. Can be zero if the operation
+ *		 does not need to send an address
+ * addr.pad: number of IO lines used to transmit the address cycles
+ * dummy.nbytes: number of dummy cycles to send after an opcode or address.
+ *		 Can be zero if the operation does not require dummy cycles
+ * dummy.pad: number of IO lanes used to transmit the dummy bytes
+ * data.dir: direction of the transfer
+ * data.nytes: number of address bytes to send. Can be zero for fsl_qspi_write,
+ *	       actual transfer size provided when CMD is triggered.
+ * data.pad: number of IO lanes used to transmit the data bytes
+ * mode: mode of the running command, default is IP mode
+ */
+struct fsl_qspi_mem_op {
+	struct {
+		u8 opcode;
+		u8 pad;
+	} cmd;
+
+	struct {
+		u8 addrlen;
+		u8 pad;
+	} addr;
+
+	struct {
+		u8 nbytes;
+		u8 pad;
+	} dummy;
+
+	struct {
+		enum qspi_mem_data_dir dir;
+		unsigned int nbytes;
+		u8 pad;
+	} data;
+
+	enum qspi_cmd_mode mode;
+};
+
 struct fsl_qspi {
 	struct spi_nor nor[FSL_QSPI_MAX_CHIP];
 	void __iomem *iobase;
@@ -287,6 +342,7 @@ struct fsl_qspi {
 	bool big_endian;
 	struct mutex lock;
 	struct pm_qos_request pm_qos_req;
+	struct fsl_qspi_mem_op *op_data;
 };
 
 static inline int needs_swap_endian(struct fsl_qspi *q)
@@ -368,136 +424,92 @@ static irqreturn_t fsl_qspi_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void fsl_qspi_init_lut(struct fsl_qspi *q)
+static inline s8 pad_count(s8 pad_val)
+{
+	s8 count = -1;
+
+	if (!pad_val)
+		return 0;
+
+	while (pad_val) {
+		pad_val >>= 1;
+		count++;
+	}
+	return count;
+}
+
+/*
+ * Prepare LUT values for requested CMD using struct fsl_qspi_mem_op
+ * LUT prepared in format:
+ *  ---------------------------------------------------
+ *  | INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
+ *  ---------------------------------------------------
+ * values from struct fsl_qspi_mem_op are saved for different
+ * operands like CMD, ADDR, DUMMY and DATA in above format based
+ * on provided input value.
+ * For case of AHB (XIP) operation, use different LUT seqid,
+ * as read from QSPI controller can be triggered using this driver
+ * and also using "devmem" utility.
+ * For read trigger using "devmem" controller use LUT seqid saved in
+ * QUADSPI_BFGENCR register.
+ */
+static void fsl_qspi_prepare_lut(struct spi_nor *nor,
+				 struct fsl_qspi_mem_op *op)
 {
+	struct fsl_qspi *q = nor->priv;
 	void __iomem *base = q->iobase;
-	int rxfifo = q->devtype_data->rxfifo;
 	u32 lut_base;
-	int i;
+	u32 lutval[4] = {};
+	int lutidx = 0, i;
+
+	lutval[lutidx / 2] |= LUT_DEF(lutidx,
+				      LUT_CMD,
+				      pad_count(op->cmd.pad),
+				      op->cmd.opcode);
+	lutidx++;
+
+	if (op->addr.addrlen) {
+		lutval[lutidx / 2] |= LUT_DEF(lutidx,
+					      LUT_ADDR,
+					      pad_count(op->addr.pad),
+					      op->addr.addrlen);
+		lutidx++;
+	}
 
-	struct spi_nor *nor = &q->nor[0];
-	u8 addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
-	u8 read_op = nor->read_opcode;
-	u8 read_dm = nor->read_dummy;
+	if (op->dummy.nbytes) {
+		lutval[lutidx / 2] |= LUT_DEF(lutidx,
+					      LUT_DUMMY,
+					      pad_count(op->dummy.pad),
+					      op->dummy.nbytes);
+		lutidx++;
+	}
 
-	fsl_qspi_unlock_lut(q);
+	if (op->data.dir) {
+		lutval[lutidx / 2] |= LUT_DEF(lutidx,
+					      op->data.dir == QSPI_MEM_DATA_IN ?
+					      LUT_FSL_READ : LUT_FSL_WRITE,
+					      pad_count(op->data.pad),
+					      op->data.nbytes);
+		lutidx++;
+	}
 
-	/* Clear all the LUT table */
-	for (i = 0; i < QUADSPI_LUT_NUM; i++)
-		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
-
-	/* Read */
-	lut_base = SEQID_READ * 4;
-
-	qspi_writel(q, LUT0(CMD, PAD1, read_op) | LUT1(ADDR, PAD1, addrlen),
-			base + QUADSPI_LUT(lut_base));
-	qspi_writel(q, LUT0(DUMMY, PAD1, read_dm) |
-		    LUT1(FSL_READ, PAD4, rxfifo),
-			base + QUADSPI_LUT(lut_base + 1));
-
-	/* Write enable */
-	lut_base = SEQID_WREN * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WREN),
-			base + QUADSPI_LUT(lut_base));
-
-	/* Page Program */
-	lut_base = SEQID_PP * 4;
-
-	qspi_writel(q, LUT0(CMD, PAD1, nor->program_opcode) |
-		    LUT1(ADDR, PAD1, addrlen),
-			base + QUADSPI_LUT(lut_base));
-	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
-			base + QUADSPI_LUT(lut_base + 1));
-
-	/* Read Status */
-	lut_base = SEQID_RDSR * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDSR) |
-			LUT1(FSL_READ, PAD1, 0x1),
-			base + QUADSPI_LUT(lut_base));
-
-	/* Erase a sector */
-	lut_base = SEQID_SE * 4;
-
-	qspi_writel(q, LUT0(CMD, PAD1, nor->erase_opcode) |
-		    LUT1(ADDR, PAD1, addrlen),
-			base + QUADSPI_LUT(lut_base));
-
-	/* Erase the whole chip */
-	lut_base = SEQID_CHIP_ERASE * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_CHIP_ERASE),
-			base + QUADSPI_LUT(lut_base));
-
-	/* READ ID */
-	lut_base = SEQID_RDID * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDID) |
-			LUT1(FSL_READ, PAD1, 0x8),
-			base + QUADSPI_LUT(lut_base));
-
-	/* Write Register */
-	lut_base = SEQID_WRSR * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WRSR) |
-			LUT1(FSL_WRITE, PAD1, 0x2),
-			base + QUADSPI_LUT(lut_base));
-
-	/* Read Configuration Register */
-	lut_base = SEQID_RDCR * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDCR) |
-			LUT1(FSL_READ, PAD1, 0x1),
-			base + QUADSPI_LUT(lut_base));
-
-	/* Write disable */
-	lut_base = SEQID_WRDI * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WRDI),
-			base + QUADSPI_LUT(lut_base));
-
-	/* Enter 4 Byte Mode (Micron) */
-	lut_base = SEQID_EN4B * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_EN4B),
-			base + QUADSPI_LUT(lut_base));
-
-	/* Enter 4 Byte Mode (Spansion) */
-	lut_base = SEQID_BRWR * 4;
-	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
-			base + QUADSPI_LUT(lut_base));
+	lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_STOP, 0, 0);
+	lutidx++;
 
-	fsl_qspi_lock_lut(q);
-}
+	dev_dbg(q->dev, "cmd:%x lut:[%x, %x, %x, %x]\n", op->cmd.opcode,
+			lutval[0], lutval[1], lutval[2], lutval[3]);
 
-/* Get the SEQID for the command */
-static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
-{
-	switch (cmd) {
-	case SPINOR_OP_READ_1_1_4:
-		return SEQID_READ;
-	case SPINOR_OP_WREN:
-		return SEQID_WREN;
-	case SPINOR_OP_WRDI:
-		return SEQID_WRDI;
-	case SPINOR_OP_RDSR:
-		return SEQID_RDSR;
-	case SPINOR_OP_SE:
-		return SEQID_SE;
-	case SPINOR_OP_CHIP_ERASE:
-		return SEQID_CHIP_ERASE;
-	case SPINOR_OP_PP:
-		return SEQID_PP;
-	case SPINOR_OP_RDID:
-		return SEQID_RDID;
-	case SPINOR_OP_WRSR:
-		return SEQID_WRSR;
-	case SPINOR_OP_RDCR:
-		return SEQID_RDCR;
-	case SPINOR_OP_EN4B:
-		return SEQID_EN4B;
-	case SPINOR_OP_BRWR:
-		return SEQID_BRWR;
-	default:
-		if (cmd == q->nor[0].erase_opcode)
-			return SEQID_SE;
-		dev_err(q->dev, "Unsupported cmd 0x%.2x\n", cmd);
-		break;
-	}
-	return -EINVAL;
+	/* Dynamic LUT */
+	if (op->mode == QSPI_CMD_MODE_IP)
+		lut_base = SEQID_LUT1_IPCMD * 4;
+	else
+		lut_base = SEQID_LUT2_AHBREAD * 4;
+
+	/* Write values in LUT register. */
+	fsl_qspi_unlock_lut(q);
+	for (i = 0; i < ARRAY_SIZE(lutval); i++)
+		qspi_writel(q, lutval[i], base + QUADSPI_LUT(lut_base + i));
+	fsl_qspi_lock_lut(q);
 }
 
 static int
@@ -532,7 +544,7 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
 	} while (1);
 
 	/* trigger the LUT now */
-	seqid = fsl_qspi_get_seqid(q, cmd);
+	seqid = SEQID_LUT1_IPCMD;
 	qspi_writel(q, (seqid << QUADSPI_IPCR_SEQID_SHIFT) | len,
 			base + QUADSPI_IPCR);
 
@@ -648,6 +660,19 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
 	qspi_writel(q, nor_size * 4 + q->memmap_phy, base + QUADSPI_SFB2AD);
 }
 
+/* Clear only decision making fields of struct fsl_qspi_mem_op.
+ * Not used memset to clear whole structure as clearing of fields required
+ * to be done for every CMD and it would be performance hit.
+ */
+static inline void fsl_clr_qspi_mem_data(struct fsl_qspi_mem_op *op)
+{
+	op->cmd.opcode = 0;
+	op->addr.addrlen = 0;
+	op->dummy.nbytes = 0;
+	op->data.dir = QSPI_MEM_NO_DATA;
+	op->mode = QSPI_CMD_MODE_IP;
+}
+
 /*
  * There are two different ways to read out the data from the flash:
  *  the "IP Command Read" and the "AHB Command Read".
@@ -684,12 +709,42 @@ static void fsl_qspi_init_ahb_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);
+	/* Set dynamic LUT entry as lut sequence for AHB Read . */
+	seqid = SEQID_LUT2_AHBREAD;
+
+	/* Save SEQID_LUT2_AHBREAD in QUADSPI_BFGENCR, required for AHB Read */
 	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
 		q->iobase + QUADSPI_BFGENCR);
 }
 
+/* Prepare LUT for AHB read - required for read from devmem interface */
+static void fsl_qspi_prep_ahb_read(struct fsl_qspi *q)
+{
+	struct fsl_qspi_mem_op *op = q->op_data;
+	struct spi_nor *nor = q->nor;
+	enum spi_nor_protocol protocol = 0;
+
+	fsl_clr_qspi_mem_data(op);
+	protocol = nor->read_proto;
+	op->cmd.opcode = nor->read_opcode;
+	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
+
+	op->addr.addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
+	op->addr.pad = spi_nor_get_protocol_addr_nbits(protocol);
+
+	op->dummy.nbytes = nor->read_dummy;
+	op->dummy.pad = spi_nor_get_protocol_data_nbits(protocol);
+
+	op->data.dir = QSPI_MEM_DATA_IN;
+	op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
+	/* Data size ignored for AHB Read. */
+	op->data.nbytes = 0;
+
+	op->mode = QSPI_CMD_MODE_AHB;
+
+	fsl_qspi_prepare_lut(nor, op);
+}
+
 /* This function was used to prepare and enable QSPI clock */
 static int fsl_qspi_clk_prep_enable(struct fsl_qspi *q)
 {
@@ -728,7 +783,6 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
 	void __iomem *base = q->iobase;
 	u32 reg;
 	int ret;
-
 	/* disable and unprepare clock to avoid glitch pass to controller */
 	fsl_qspi_clk_disable_unprep(q);
 
@@ -746,9 +800,6 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
 		base + QUADSPI_MCR);
 	udelay(1);
 
-	/* Init the LUT table. */
-	fsl_qspi_init_lut(q);
-
 	/* Disable the module */
 	qspi_writel(q, QUADSPI_MCR_MDIS_MASK | QUADSPI_MCR_RESERVED_MASK,
 			base + QUADSPI_MCR);
@@ -791,12 +842,12 @@ static int fsl_qspi_nor_setup_last(struct fsl_qspi *q)
 	if (ret)
 		return ret;
 
-	/* Init the LUT table again. */
-	fsl_qspi_init_lut(q);
-
 	/* Init for AHB read */
 	fsl_qspi_init_ahb_read(q);
 
+	/* Prepare LUT for AHB read - required for read from devmem interface */
+	fsl_qspi_prep_ahb_read(q);
+
 	return 0;
 }
 
@@ -819,7 +870,25 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 {
 	int ret;
 	struct fsl_qspi *q = nor->priv;
+	struct fsl_qspi_mem_op *op = q->op_data;
+	enum spi_nor_protocol protocol = 0;
+
+	fsl_clr_qspi_mem_data(op);
+	protocol = nor->reg_proto;
+
+	/*
+	 * Fill required entry of struct fsl_qspi_mem_op to prepare
+	 * LUT for requested cmd.
+	 */
+	op->cmd.opcode = opcode;
+	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
+
+	op->data.dir = QSPI_MEM_DATA_IN;
+	op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
+	op->data.nbytes = len;
 
+	/* Addrlen and Dummy info not required for READ_REG cmds */
+	fsl_qspi_prepare_lut(nor, op);
 	ret = fsl_qspi_runcmd(q, opcode, 0, len);
 	if (ret)
 		return ret;
@@ -832,8 +901,22 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 {
 	struct fsl_qspi *q = nor->priv;
 	int ret;
+	struct fsl_qspi_mem_op *op = q->op_data;
+	enum spi_nor_protocol protocol = 0;
+
+	fsl_clr_qspi_mem_data(op);
+	protocol = nor->reg_proto;
+
+	/*
+	 * Fill required entry of struct fsl_qspi_mem_op to prepare
+	 * LUT for requested cmd.
+	 */
+	op->cmd.opcode = opcode;
+	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
 
 	if (!buf) {
+		/* Addrlen, Dummy and Data info not required for WRITE_REG */
+		fsl_qspi_prepare_lut(nor, op);
 		ret = fsl_qspi_runcmd(q, opcode, 0, 1);
 		if (ret)
 			return ret;
@@ -842,6 +925,12 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 			fsl_qspi_invalid(q);
 
 	} else if (len > 0) {
+		/* Addrlen and Dummy not requ with BUF as non-null */
+		op->data.dir = QSPI_MEM_DATA_OUT;
+		op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
+		op->data.nbytes = q->devtype_data->txfifo;
+
+		fsl_qspi_prepare_lut(nor, op);
 		ret = fsl_qspi_nor_write(q, nor, opcode, 0,
 					(u32 *)buf, len);
 		if (ret > 0)
@@ -858,8 +947,33 @@ static ssize_t fsl_qspi_write(struct spi_nor *nor, loff_t to,
 			      size_t len, const u_char *buf)
 {
 	struct fsl_qspi *q = nor->priv;
-	ssize_t ret = fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
-					 (u32 *)buf, len);
+	ssize_t ret;
+	struct fsl_qspi_mem_op *op = q->op_data;
+	enum spi_nor_protocol protocol = 0;
+
+	fsl_clr_qspi_mem_data(op);
+	protocol = nor->write_proto;
+
+	/*
+	 * Fill required entry of struct fsl_qspi_mem_op to prepare
+	 * LUT for requested cmd.
+	 */
+	op->cmd.opcode = nor->program_opcode;
+	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
+
+	op->addr.addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
+	op->addr.pad = spi_nor_get_protocol_addr_nbits(protocol);
+
+	op->data.dir = QSPI_MEM_DATA_OUT;
+	op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
+
+	/* TX data size provided when QSPI cmd trigger. */
+	op->data.nbytes = 0;
+
+	/* Dummy info not required for WRITE cmd */
+	fsl_qspi_prepare_lut(nor, op);
+	ret = fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
+				 (u32 *)buf, len);
 
 	/* invalid the data in the AHB buffer. */
 	fsl_qspi_invalid(q);
@@ -871,6 +985,32 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
 {
 	struct fsl_qspi *q = nor->priv;
 	u8 cmd = nor->read_opcode;
+	struct fsl_qspi_mem_op *op = q->op_data;
+	enum spi_nor_protocol protocol = 0;
+
+	fsl_clr_qspi_mem_data(op);
+	protocol = nor->read_proto;
+
+	/*
+	 * Fill required entry of struct fsl_qspi_mem_op to prepare
+	 * LUT for requested cmd.
+	 */
+	op->cmd.opcode = cmd;
+	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
+
+	op->addr.addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
+	op->addr.pad = spi_nor_get_protocol_addr_nbits(protocol);
+
+	op->dummy.nbytes = nor->read_dummy;
+	op->dummy.pad = spi_nor_get_protocol_data_nbits(protocol);
+
+	op->data.dir = QSPI_MEM_DATA_IN;
+	op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
+	op->data.nbytes = q->devtype_data->rxfifo;
+
+	op->mode = QSPI_CMD_MODE_AHB;
+
+	fsl_qspi_prepare_lut(nor, op);
 
 	/* if necessary,ioremap buffer before AHB read, */
 	if (!q->ahb_addr) {
@@ -916,6 +1056,22 @@ static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
 {
 	struct fsl_qspi *q = nor->priv;
 	int ret;
+	struct fsl_qspi_mem_op *op = q->op_data;
+
+	fsl_clr_qspi_mem_data(op);
+
+	/*
+	 * Fill required entry of struct fsl_qspi_mem_op to prepare
+	 * LUT for requested cmd.
+	 */
+	op->cmd.opcode = nor->erase_opcode;
+	op->cmd.pad = LUT_PAD1;
+
+	op->addr.addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
+	op->addr.pad = LUT_PAD1;
+
+	/* Dummy and Data info not required for Erase cmd */
+	fsl_qspi_prepare_lut(nor, op);
 
 	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
 		nor->mtd.erasesize / 1024, q->chip_base_addr, (u32)offs);
@@ -967,12 +1123,18 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct spi_nor *nor;
 	struct mtd_info *mtd;
+	struct fsl_qspi_mem_op *mem_op_data;
 	int ret, i = 0;
 
 	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
 	if (!q)
 		return -ENOMEM;
 
+	mem_op_data = kmalloc(sizeof(struct fsl_qspi_mem_op), GFP_KERNEL);
+	if (!mem_op_data)
+		return -ENOMEM;
+	q->op_data = mem_op_data;
+
 	q->nor_num = of_get_child_count(dev->of_node);
 	if (!q->nor_num || q->nor_num > FSL_QSPI_MAX_CHIP)
 		return -ENODEV;
@@ -1120,6 +1282,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 irq_failed:
 	fsl_qspi_clk_disable_unprep(q);
 clk_failed:
+	kfree(q->op_data);
 	dev_err(dev, "Freescale QuadSPI probe failed\n");
 	return ret;
 }
@@ -1145,6 +1308,7 @@ static int fsl_qspi_remove(struct platform_device *pdev)
 	if (q->ahb_addr)
 		iounmap(q->ahb_addr);
 
+	kfree(q->op_data);
 	return 0;
 }
 
-- 
1.9.1

[PATCH v7 2/2] mtd: fsl-quadspi: fix init AHB read in fsl_qspi_nor_setup()

[Yogesh Gaur]

Move AHB read initialize in fsl_qspi_nor_setup().

In file spi-nor.c, func spi_nor_scan() calls nor->read() thru
spi_nor_read_sfdp() API.

Presently, fsl_qspi_init_ahb_read() called from fsl_qspi_nor_setup_last().
Func fsl_qspi_probe() calls spi_nor_scan() first and then calls
fsl_qspi_nor_setup_last().

When nor->read() being called from inside spi_nor_read_sfdp(), QSPI cntlr
AHB mode initialization is not yet done results in data read error.

Initialize AHB read inside fsl_qspi_nor_setup().

Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
---
Changes for v7:
- None
Changes for v6:
- None
Changes for v5:
- None
Changes for v4:
- None
Changes for v3:
- None
Changes for v2:
- None

 drivers/mtd/spi-nor/fsl-quadspi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 64b8bb3..424094a 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -820,6 +820,9 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
 	/* enable the interrupt */
 	qspi_writel(q, QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
 
+	/* Init for AHB read */
+	fsl_qspi_init_ahb_read(q);
+
 	return 0;
 }
 
@@ -842,9 +845,6 @@ static int fsl_qspi_nor_setup_last(struct fsl_qspi *q)
 	if (ret)
 		return ret;
 
-	/* Init for AHB read */
-	fsl_qspi_init_ahb_read(q);
-
 	/* Prepare LUT for AHB read - required for read from devmem interface */
 	fsl_qspi_prep_ahb_read(q);
 
-- 
1.9.1

Re: [PATCH v7 1/2] mtd: fsl-quadspi: add support to create dynamic LUT entry

[Frieder Schrempf]

Hi Yogesh,

On 21.03.2018 12:26, Yogesh Gaur wrote:
> Add support to create dynamic LUT entry.
> 
> Current approach of creating LUT entries for various cmds like read, write,
> erase, readid, readsr, we, wd etc is that when QSPI controller gets
> initialized at that time static LUT entries for these cmds get created.
> 
> Patch add support to create the LUT at run time based on the operation
> being performed.
> 
> Added API fsl_qspi_prepare_lut(), this API would going to be called from
> fsl_qspi_read_reg, fsl_qspi_write_reg, fsl_qspi_write, fsl_qspi_read and
> fsl_qspi_erase APIs.
> Added new struct fsl_qspi_mem_op having fields required to fill in LUT
> register for requested command.
> Required values for every CMD like opcode, addrlen, dummy_cycles, data_size
> and pad bytes being filled in respective calling function and then API
> fsl_qspi_prepare_lut fill LUT register for requested command.
> Required values are fetched from instance of 'struct spi_nor'.
> 
> AHB Read, memory mapped, can be triggered using driver interface as well
> from 'devmem' interface.
> For AHB read, LUT seq programmed in QUADSPI_BFGENCR register used for
> Read operation. For Read initiated from 'devmem' working properly,
> required to have dedicated LUT seq for AHB read.
> 
> Signed-off-by: Suresh Gupta <suresh.gupta@nxp.com>
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> ---
> Changes for v7:
> - Incorporated Han review comments.
> Changes for v6:
> - Incorporated Boris review comments.
> - Added dedicated LUT for AHB read cmd, required for devmem Read.
> Changes for v5:
> - Fix LUT preparation for SPINOR_OP_READ and SPINOR_OP_READ_4B cmds.
> Changes for v4:
> - Correct version numbering.
> Changes for v3:
> - Add STOP instruction for prepared LUT and remove memset of 4 LUT reg.
> Changes for v2:
> - Swap patch sequences in the series to solve git bissect issue.
> 
>   drivers/mtd/spi-nor/fsl-quadspi.c | 480 +++++++++++++++++++++++++-------------
>   1 file changed, 322 insertions(+), 158 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 89306cf..64b8bb3 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -142,10 +142,14 @@
>    *  | INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
>    *  ---------------------------------------------------
>    */
> -#define OPRND0_SHIFT		0
> -#define PAD0_SHIFT		8
> -#define INSTR0_SHIFT		10
> -#define OPRND1_SHIFT		16
> +#define PAD_SHIFT		8
> +#define INSTR_SHIFT		10
> +#define OPRND_SHIFT		16
> +
> +/* Macros for constructing the LUT register. */
> +#define LUT_DEF(idx, ins, pad, opr)			  \
> +	((((ins) << INSTR_SHIFT) | ((pad) << PAD_SHIFT) | \
> +	(opr)) << (((idx) % 2) * OPRND_SHIFT))
>   
>   /* Instruction set for the LUT register. */
>   #define LUT_STOP		0
> @@ -181,30 +185,19 @@
>   #define ADDR24BIT		0x18
>   #define ADDR32BIT		0x20
>   
> -/* Macros for constructing the LUT register. */
> -#define LUT0(ins, pad, opr)						\
> -		(((opr) << OPRND0_SHIFT) | ((LUT_##pad) << PAD0_SHIFT) | \
> -		((LUT_##ins) << INSTR0_SHIFT))
> -
> -#define LUT1(ins, pad, opr)	(LUT0(ins, pad, opr) << OPRND1_SHIFT)

If you remove the LUT0 and LUT1 macros and use LUT_DEF() instead, you 
should probably also remove LUT_PAD1, LUT_PAD2 and LUT_PAD4 just above 
these lines, as they are not needed anymore.

> -
>   /* other macros for LUT register. */
>   #define QUADSPI_LUT(x)          (QUADSPI_LUT_BASE + (x) * 4)
>   #define QUADSPI_LUT_NUM		64
>   
> -/* SEQID -- we can have 16 seqids at most. */
> -#define SEQID_READ		0
> -#define SEQID_WREN		1
> -#define SEQID_WRDI		2
> -#define SEQID_RDSR		3
> -#define SEQID_SE		4
> -#define SEQID_CHIP_ERASE	5
> -#define SEQID_PP		6
> -#define SEQID_RDID		7
> -#define SEQID_WRSR		8
> -#define SEQID_RDCR		9
> -#define SEQID_EN4B		10
> -#define SEQID_BRWR		11
> +/*
> + * SEQID -- we can have 16 seqids at most.
> + * LUT0 programmed by bootloader
> + * LUT1 programmed for IP register access cmds
> + * LUT2 programmed for AHB read, required for READ from devmem interface
> + */
> +#define SEQID_LUT0_BOOTLOADER	0
> +#define SEQID_LUT1_IPCMD	1
> +#define SEQID_LUT2_AHBREAD	2
>   
>   #define QUADSPI_MIN_IOMAP SZ_4M
>   
> @@ -268,6 +261,68 @@ struct fsl_qspi_devtype_data {
>   };
>   
>   #define FSL_QSPI_MAX_CHIP	4
> +
> +/* enum qspi_mem_data_dir - describes the direction of a QSPI memory data
> + *			    transfer from QSPI controller prespective.
> + * QSPI_MEM_NO_DATA	- no data transfer initiated
> + * QSPI_MEM_DATA_IN	- data coming from the SPI memory
> + * QSPI_MEM_DATA_OUT	- data sent to the SPI memory
> + */
> +enum qspi_mem_data_dir {
> +	QSPI_MEM_NO_DATA,
> +	QSPI_MEM_DATA_IN,
> +	QSPI_MEM_DATA_OUT,
> +};
> +
> +/* enum qspi_cmd__mode - describes the mode of the running command
> + * QSPI_CMD_MODE_IP	- data transfer using IP register access
> + * QSPI_CMD_MODE_AHB	- data transfer via AMBA AHB bus directly
> + */
> +enum qspi_cmd_mode {
> +	QSPI_CMD_MODE_IP,
> +	QSPI_CMD_MODE_AHB,
> +};
> +
> +/* struct fsl_qspi_mem_op - describes the QSPI memory operation
> + * cmd.opcode: operation opcode
> + * cmd.pad: number of IO lines used to transmit the command
> + * addr.addrlen: QSPI working in 3/4-byte mode. Can be zero if the operation
> + *		 does not need to send an address
> + * addr.pad: number of IO lines used to transmit the address cycles
> + * dummy.nbytes: number of dummy cycles to send after an opcode or address.
> + *		 Can be zero if the operation does not require dummy cycles
> + * dummy.pad: number of IO lanes used to transmit the dummy bytes

You should decide whether to use "lines" or "lanes". I think "lines" 
fits better.

> + * data.dir: direction of the transfer
> + * data.nytes: number of address bytes to send. Can be zero for fsl_qspi_write,
> + *	       actual transfer size provided when CMD is triggered.

s/data.nytes/data.nbytes
s/address/data

> + * data.pad: number of IO lanes used to transmit the data bytes

same as above: s/lanes/lines

> + * mode: mode of the running command, default is IP mode
> + */
> +struct fsl_qspi_mem_op {
> +	struct {
> +		u8 opcode;
> +		u8 pad;
> +	} cmd;
> +
> +	struct {
> +		u8 addrlen;
> +		u8 pad;
> +	} addr;
> +
> +	struct {
> +		u8 nbytes;

You name this "nbytes", but from the code and the description above it 
seems like it holds the number of dummy clock cycles.

To match this with Boris' upcoming spi-mem implementation it would 
probably be best to actually save the number of dummy bytes instead.

> +		u8 pad;
> +	} dummy;
> +
> +	struct {
> +		enum qspi_mem_data_dir dir;
> +		unsigned int nbytes;
> +		u8 pad;
> +	} data;
> +
> +	enum qspi_cmd_mode mode;
> +};
> +
>   struct fsl_qspi {
>   	struct spi_nor nor[FSL_QSPI_MAX_CHIP];
>   	void __iomem *iobase;
> @@ -287,6 +342,7 @@ struct fsl_qspi {
>   	bool big_endian;
>   	struct mutex lock;
>   	struct pm_qos_request pm_qos_req;
> +	struct fsl_qspi_mem_op *op_data;
>   };
>   
>   static inline int needs_swap_endian(struct fsl_qspi *q)
> @@ -368,136 +424,92 @@ static irqreturn_t fsl_qspi_irq_handler(int irq, void *dev_id)
>   	return IRQ_HANDLED;
>   }
>   
> -static void fsl_qspi_init_lut(struct fsl_qspi *q)
> +static inline s8 pad_count(s8 pad_val)
> +{
> +	s8 count = -1;
> +
> +	if (!pad_val)
> +		return 0;
> +
> +	while (pad_val) {
> +		pad_val >>= 1;
> +		count++;
> +	}
> +	return count;
> +}

I don't get why you use signed values for input and output of this 
function. Valid values for pad_val are > 0 and for the return value >= 0.

Anyway I guess you could replace this function with something like:
#define LUT_PAD(x) (fls(x) - 1)

This would return -1 for x = 0, but that's invalid anyway and should 
never happen.

> +
> +/*
> + * Prepare LUT values for requested CMD using struct fsl_qspi_mem_op
> + * LUT prepared in format:
> + *  ---------------------------------------------------
> + *  | INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
> + *  ---------------------------------------------------
> + * values from struct fsl_qspi_mem_op are saved for different
> + * operands like CMD, ADDR, DUMMY and DATA in above format based
> + * on provided input value.
> + * For case of AHB (XIP) operation, use different LUT seqid,
> + * as read from QSPI controller can be triggered using this driver
> + * and also using "devmem" utility.
> + * For read trigger using "devmem" controller use LUT seqid saved in
> + * QUADSPI_BFGENCR register.
> + */
> +static void fsl_qspi_prepare_lut(struct spi_nor *nor,
> +				 struct fsl_qspi_mem_op *op)
>   {
> +	struct fsl_qspi *q = nor->priv;
>   	void __iomem *base = q->iobase;
> -	int rxfifo = q->devtype_data->rxfifo;
>   	u32 lut_base;
> -	int i;
> +	u32 lutval[4] = {};
> +	int lutidx = 0, i;
> +
> +	lutval[lutidx / 2] |= LUT_DEF(lutidx,
> +				      LUT_CMD,
> +				      pad_count(op->cmd.pad),
> +				      op->cmd.opcode);
> +	lutidx++;
> +
> +	if (op->addr.addrlen) {
> +		lutval[lutidx / 2] |= LUT_DEF(lutidx,
> +					      LUT_ADDR,
> +					      pad_count(op->addr.pad),
> +					      op->addr.addrlen);
> +		lutidx++;
> +	}
>   
> -	struct spi_nor *nor = &q->nor[0];
> -	u8 addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> -	u8 read_op = nor->read_opcode;
> -	u8 read_dm = nor->read_dummy;
> +	if (op->dummy.nbytes) {
> +		lutval[lutidx / 2] |= LUT_DEF(lutidx,
> +					      LUT_DUMMY,
> +					      pad_count(op->dummy.pad),
> +					      op->dummy.nbytes);
> +		lutidx++;
> +	}
>   
> -	fsl_qspi_unlock_lut(q);
> +	if (op->data.dir) {
> +		lutval[lutidx / 2] |= LUT_DEF(lutidx,
> +					      op->data.dir == QSPI_MEM_DATA_IN ?
> +					      LUT_FSL_READ : LUT_FSL_WRITE,
> +					      pad_count(op->data.pad),
> +					      op->data.nbytes);
> +		lutidx++;
> +	}
>   
> -	/* Clear all the LUT table */
> -	for (i = 0; i < QUADSPI_LUT_NUM; i++)
> -		qspi_writel(q, 0, base + QUADSPI_LUT_BASE + i * 4);
> -
> -	/* Read */
> -	lut_base = SEQID_READ * 4;
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, read_op) | LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -	qspi_writel(q, LUT0(DUMMY, PAD1, read_dm) |
> -		    LUT1(FSL_READ, PAD4, rxfifo),
> -			base + QUADSPI_LUT(lut_base + 1));
> -
> -	/* Write enable */
> -	lut_base = SEQID_WREN * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WREN),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* Page Program */
> -	lut_base = SEQID_PP * 4;
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, nor->program_opcode) |
> -		    LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -	qspi_writel(q, LUT0(FSL_WRITE, PAD1, 0),
> -			base + QUADSPI_LUT(lut_base + 1));
> -
> -	/* Read Status */
> -	lut_base = SEQID_RDSR * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDSR) |
> -			LUT1(FSL_READ, PAD1, 0x1),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* Erase a sector */
> -	lut_base = SEQID_SE * 4;
> -
> -	qspi_writel(q, LUT0(CMD, PAD1, nor->erase_opcode) |
> -		    LUT1(ADDR, PAD1, addrlen),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* Erase the whole chip */
> -	lut_base = SEQID_CHIP_ERASE * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_CHIP_ERASE),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* READ ID */
> -	lut_base = SEQID_RDID * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDID) |
> -			LUT1(FSL_READ, PAD1, 0x8),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* Write Register */
> -	lut_base = SEQID_WRSR * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WRSR) |
> -			LUT1(FSL_WRITE, PAD1, 0x2),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* Read Configuration Register */
> -	lut_base = SEQID_RDCR * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_RDCR) |
> -			LUT1(FSL_READ, PAD1, 0x1),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* Write disable */
> -	lut_base = SEQID_WRDI * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_WRDI),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* Enter 4 Byte Mode (Micron) */
> -	lut_base = SEQID_EN4B * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_EN4B),
> -			base + QUADSPI_LUT(lut_base));
> -
> -	/* Enter 4 Byte Mode (Spansion) */
> -	lut_base = SEQID_BRWR * 4;
> -	qspi_writel(q, LUT0(CMD, PAD1, SPINOR_OP_BRWR),
> -			base + QUADSPI_LUT(lut_base));
> +	lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_STOP, 0, 0);
> +	lutidx++;
>   
> -	fsl_qspi_lock_lut(q);
> -}
> +	dev_dbg(q->dev, "cmd:%x lut:[%x, %x, %x, %x]\n", op->cmd.opcode,
> +			lutval[0], lutval[1], lutval[2], lutval[3]);
>   
> -/* Get the SEQID for the command */
> -static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
> -{
> -	switch (cmd) {
> -	case SPINOR_OP_READ_1_1_4:
> -		return SEQID_READ;
> -	case SPINOR_OP_WREN:
> -		return SEQID_WREN;
> -	case SPINOR_OP_WRDI:
> -		return SEQID_WRDI;
> -	case SPINOR_OP_RDSR:
> -		return SEQID_RDSR;
> -	case SPINOR_OP_SE:
> -		return SEQID_SE;
> -	case SPINOR_OP_CHIP_ERASE:
> -		return SEQID_CHIP_ERASE;
> -	case SPINOR_OP_PP:
> -		return SEQID_PP;
> -	case SPINOR_OP_RDID:
> -		return SEQID_RDID;
> -	case SPINOR_OP_WRSR:
> -		return SEQID_WRSR;
> -	case SPINOR_OP_RDCR:
> -		return SEQID_RDCR;
> -	case SPINOR_OP_EN4B:
> -		return SEQID_EN4B;
> -	case SPINOR_OP_BRWR:
> -		return SEQID_BRWR;
> -	default:
> -		if (cmd == q->nor[0].erase_opcode)
> -			return SEQID_SE;
> -		dev_err(q->dev, "Unsupported cmd 0x%.2x\n", cmd);
> -		break;
> -	}
> -	return -EINVAL;
> +	/* Dynamic LUT */
> +	if (op->mode == QSPI_CMD_MODE_IP)
> +		lut_base = SEQID_LUT1_IPCMD * 4;
> +	else
> +		lut_base = SEQID_LUT2_AHBREAD * 4;
> +
> +	/* Write values in LUT register. */
> +	fsl_qspi_unlock_lut(q);
> +	for (i = 0; i < ARRAY_SIZE(lutval); i++)
> +		qspi_writel(q, lutval[i], base + QUADSPI_LUT(lut_base + i));
> +	fsl_qspi_lock_lut(q);
>   }
>   
>   static int
> @@ -532,7 +544,7 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
>   	} while (1);
>   
>   	/* trigger the LUT now */
> -	seqid = fsl_qspi_get_seqid(q, cmd);
> +	seqid = SEQID_LUT1_IPCMD;
>   	qspi_writel(q, (seqid << QUADSPI_IPCR_SEQID_SHIFT) | len,
>   			base + QUADSPI_IPCR);

You can remove the seqid variable and just put in SEQID_LUT1_IPCMD directly.

>   
> @@ -648,6 +660,19 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
>   	qspi_writel(q, nor_size * 4 + q->memmap_phy, base + QUADSPI_SFB2AD);
>   }
>   
> +/* Clear only decision making fields of struct fsl_qspi_mem_op.
> + * Not used memset to clear whole structure as clearing of fields required
> + * to be done for every CMD and it would be performance hit.
> + */
> +static inline void fsl_clr_qspi_mem_data(struct fsl_qspi_mem_op *op)
> +{
> +	op->cmd.opcode = 0;
> +	op->addr.addrlen = 0;
> +	op->dummy.nbytes = 0;
> +	op->data.dir = QSPI_MEM_NO_DATA;
> +	op->mode = QSPI_CMD_MODE_IP;
> +}
> +
>   /*
>    * There are two different ways to read out the data from the flash:
>    *  the "IP Command Read" and the "AHB Command Read".
> @@ -684,12 +709,42 @@ static void fsl_qspi_init_ahb_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);
> +	/* Set dynamic LUT entry as lut sequence for AHB Read . */
> +	seqid = SEQID_LUT2_AHBREAD;
> +
> +	/* Save SEQID_LUT2_AHBREAD in QUADSPI_BFGENCR, required for AHB Read */
>   	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
>   		q->iobase + QUADSPI_BFGENCR);

You can remove the seqid variable and just put in SEQID_LUT2_AHBREAD 
directly.

>   }
>   
> +/* Prepare LUT for AHB read - required for read from devmem interface */
> +static void fsl_qspi_prep_ahb_read(struct fsl_qspi *q)
> +{
> +	struct fsl_qspi_mem_op *op = q->op_data;
> +	struct spi_nor *nor = q->nor;
> +	enum spi_nor_protocol protocol = 0;
> +
> +	fsl_clr_qspi_mem_data(op);
> +	protocol = nor->read_proto;

You could do this assignment during the initialization of the variable 
above:

enum spi_nor_protocol protocol = nor->read_proto;

> +	op->cmd.opcode = nor->read_opcode;
> +	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
> +
> +	op->addr.addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> +	op->addr.pad = spi_nor_get_protocol_addr_nbits(protocol);
> +
> +	op->dummy.nbytes = nor->read_dummy;
> +	op->dummy.pad = spi_nor_get_protocol_data_nbits(protocol);
> +
> +	op->data.dir = QSPI_MEM_DATA_IN;
> +	op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
> +	/* Data size ignored for AHB Read. */
> +	op->data.nbytes = 0;
> +
> +	op->mode = QSPI_CMD_MODE_AHB;
> +
> +	fsl_qspi_prepare_lut(nor, op);
> +}
> +
>   /* This function was used to prepare and enable QSPI clock */
>   static int fsl_qspi_clk_prep_enable(struct fsl_qspi *q)
>   {
> @@ -728,7 +783,6 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
>   	void __iomem *base = q->iobase;
>   	u32 reg;
>   	int ret;
> -
>   	/* disable and unprepare clock to avoid glitch pass to controller */
>   	fsl_qspi_clk_disable_unprep(q);
>   
> @@ -746,9 +800,6 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
>   		base + QUADSPI_MCR);
>   	udelay(1);
>   
> -	/* Init the LUT table. */
> -	fsl_qspi_init_lut(q);
> -
>   	/* Disable the module */
>   	qspi_writel(q, QUADSPI_MCR_MDIS_MASK | QUADSPI_MCR_RESERVED_MASK,
>   			base + QUADSPI_MCR);
> @@ -791,12 +842,12 @@ static int fsl_qspi_nor_setup_last(struct fsl_qspi *q)
>   	if (ret)
>   		return ret;
>   
> -	/* Init the LUT table again. */
> -	fsl_qspi_init_lut(q);
> -
>   	/* Init for AHB read */
>   	fsl_qspi_init_ahb_read(q);
>   
> +	/* Prepare LUT for AHB read - required for read from devmem interface */
> +	fsl_qspi_prep_ahb_read(q);
> +
>   	return 0;
>   }
>   
> @@ -819,7 +870,25 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>   {
>   	int ret;
>   	struct fsl_qspi *q = nor->priv;
> +	struct fsl_qspi_mem_op *op = q->op_data;
> +	enum spi_nor_protocol protocol = 0;
> +
> +	fsl_clr_qspi_mem_data(op);
> +	protocol = nor->reg_proto;

same as above: move this assignment to the definition of protocol

> +
> +	/*
> +	 * Fill required entry of struct fsl_qspi_mem_op to prepare
> +	 * LUT for requested cmd.
> +	 */
> +	op->cmd.opcode = opcode;
> +	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
> +
> +	op->data.dir = QSPI_MEM_DATA_IN;
> +	op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
> +	op->data.nbytes = len;
>   
> +	/* Addrlen and Dummy info not required for READ_REG cmds */
> +	fsl_qspi_prepare_lut(nor, op);
>   	ret = fsl_qspi_runcmd(q, opcode, 0, len);
>   	if (ret)
>   		return ret;
> @@ -832,8 +901,22 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>   {
>   	struct fsl_qspi *q = nor->priv;
>   	int ret;
> +	struct fsl_qspi_mem_op *op = q->op_data;
> +	enum spi_nor_protocol protocol = 0;
> +
> +	fsl_clr_qspi_mem_data(op);
> +	protocol = nor->reg_proto;

same as above: move this assignment to the definition of protocol

> +
> +	/*
> +	 * Fill required entry of struct fsl_qspi_mem_op to prepare
> +	 * LUT for requested cmd.
> +	 */
> +	op->cmd.opcode = opcode;
> +	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
>   
>   	if (!buf) {
> +		/* Addrlen, Dummy and Data info not required for WRITE_REG */
> +		fsl_qspi_prepare_lut(nor, op);
>   		ret = fsl_qspi_runcmd(q, opcode, 0, 1);
>   		if (ret)
>   			return ret;
> @@ -842,6 +925,12 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>   			fsl_qspi_invalid(q);
>   
>   	} else if (len > 0) {
> +		/* Addrlen and Dummy not requ with BUF as non-null */
> +		op->data.dir = QSPI_MEM_DATA_OUT;
> +		op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
> +		op->data.nbytes = q->devtype_data->txfifo;
> +
> +		fsl_qspi_prepare_lut(nor, op);
>   		ret = fsl_qspi_nor_write(q, nor, opcode, 0,
>   					(u32 *)buf, len);
>   		if (ret > 0)
> @@ -858,8 +947,33 @@ static ssize_t fsl_qspi_write(struct spi_nor *nor, loff_t to,
>   			      size_t len, const u_char *buf)
>   {
>   	struct fsl_qspi *q = nor->priv;
> -	ssize_t ret = fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
> -					 (u32 *)buf, len);
> +	ssize_t ret;
> +	struct fsl_qspi_mem_op *op = q->op_data;
> +	enum spi_nor_protocol protocol = 0;
> +
> +	fsl_clr_qspi_mem_data(op);
> +	protocol = nor->write_proto;

same as above: move this assignment to the definition of protocol

> +
> +	/*
> +	 * Fill required entry of struct fsl_qspi_mem_op to prepare
> +	 * LUT for requested cmd.
> +	 */
> +	op->cmd.opcode = nor->program_opcode;
> +	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
> +
> +	op->addr.addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> +	op->addr.pad = spi_nor_get_protocol_addr_nbits(protocol);
> +
> +	op->data.dir = QSPI_MEM_DATA_OUT;
> +	op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
> +
> +	/* TX data size provided when QSPI cmd trigger. */
> +	op->data.nbytes = 0;
> +
> +	/* Dummy info not required for WRITE cmd */
> +	fsl_qspi_prepare_lut(nor, op);
> +	ret = fsl_qspi_nor_write(q, nor, nor->program_opcode, to,
> +				 (u32 *)buf, len);
>   
>   	/* invalid the data in the AHB buffer. */
>   	fsl_qspi_invalid(q);
> @@ -871,6 +985,32 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>   {
>   	struct fsl_qspi *q = nor->priv;
>   	u8 cmd = nor->read_opcode;
> +	struct fsl_qspi_mem_op *op = q->op_data;
> +	enum spi_nor_protocol protocol = 0;
> +
> +	fsl_clr_qspi_mem_data(op);
> +	protocol = nor->read_proto;

same as above: move this assignment to the definition of protocol

> +
> +	/*
> +	 * Fill required entry of struct fsl_qspi_mem_op to prepare
> +	 * LUT for requested cmd.
> +	 */
> +	op->cmd.opcode = cmd;
> +	op->cmd.pad = spi_nor_get_protocol_inst_nbits(protocol);
> +
> +	op->addr.addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> +	op->addr.pad = spi_nor_get_protocol_addr_nbits(protocol);
> +
> +	op->dummy.nbytes = nor->read_dummy;
> +	op->dummy.pad = spi_nor_get_protocol_data_nbits(protocol);
> +
> +	op->data.dir = QSPI_MEM_DATA_IN;
> +	op->data.pad = spi_nor_get_protocol_data_nbits(protocol);
> +	op->data.nbytes = q->devtype_data->rxfifo;
> +
> +	op->mode = QSPI_CMD_MODE_AHB;
> +
> +	fsl_qspi_prepare_lut(nor, op);
>   
>   	/* if necessary,ioremap buffer before AHB read, */
>   	if (!q->ahb_addr) {
> @@ -916,6 +1056,22 @@ static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
>   {
>   	struct fsl_qspi *q = nor->priv;
>   	int ret;
> +	struct fsl_qspi_mem_op *op = q->op_data;
> +
> +	fsl_clr_qspi_mem_data(op);
> +
> +	/*
> +	 * Fill required entry of struct fsl_qspi_mem_op to prepare
> +	 * LUT for requested cmd.
> +	 */
> +	op->cmd.opcode = nor->erase_opcode;
> +	op->cmd.pad = LUT_PAD1;

replace LUT_PAD1 with pad_count(1) or better with the macro proposed 
above: LUT_PAD(1)

> +
> +	op->addr.addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> +	op->addr.pad = LUT_PAD1;

same here

> +
> +	/* Dummy and Data info not required for Erase cmd */
> +	fsl_qspi_prepare_lut(nor, op);
>   
>   	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
>   		nor->mtd.erasesize / 1024, q->chip_base_addr, (u32)offs);
> @@ -967,12 +1123,18 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>   	struct resource *res;
>   	struct spi_nor *nor;
>   	struct mtd_info *mtd;
> +	struct fsl_qspi_mem_op *mem_op_data;
>   	int ret, i = 0;
>   
>   	q = devm_kzalloc(dev, sizeof(*q), GFP_KERNEL);
>   	if (!q)
>   		return -ENOMEM;
>   
> +	mem_op_data = kmalloc(sizeof(struct fsl_qspi_mem_op), GFP_KERNEL);

Is there any reason why you don't use devm_kzalloc() here and remove the 
kfree() calls below?

Anyway you don't really need to store the mem_op in the controller data. 
You could just pass it to fsl_qspi_runcmd() on each call. But I guess 
this would lead more into the direction of an implementation with 
exec_op() like in Boris' PoC for the spi-mem framework and therefore it 
might be part of a future patch.

Thanks,

Frieder

> +	if (!mem_op_data)
> +		return -ENOMEM;
> +	q->op_data = mem_op_data;
> +
>   	q->nor_num = of_get_child_count(dev->of_node);
>   	if (!q->nor_num || q->nor_num > FSL_QSPI_MAX_CHIP)
>   		return -ENODEV;
> @@ -1120,6 +1282,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>   irq_failed:
>   	fsl_qspi_clk_disable_unprep(q);
>   clk_failed:
> +	kfree(q->op_data);
>   	dev_err(dev, "Freescale QuadSPI probe failed\n");
>   	return ret;
>   }
> @@ -1145,6 +1308,7 @@ static int fsl_qspi_remove(struct platform_device *pdev)
>   	if (q->ahb_addr)
>   		iounmap(q->ahb_addr);
>   
> +	kfree(q->op_data);
>   	return 0;
>   }
>   
> 

Re: [PATCH v7 2/2] mtd: fsl-quadspi: fix init AHB read in fsl_qspi_nor_setup()

[Frieder Schrempf]

Hi Yogesh,

On 21.03.2018 12:26, Yogesh Gaur wrote:
> Move AHB read initialize in fsl_qspi_nor_setup().
> 
> In file spi-nor.c, func spi_nor_scan() calls nor->read() thru
> spi_nor_read_sfdp() API.
> 
> Presently, fsl_qspi_init_ahb_read() called from fsl_qspi_nor_setup_last().
> Func fsl_qspi_probe() calls spi_nor_scan() first and then calls
> fsl_qspi_nor_setup_last().
> 
> When nor->read() being called from inside spi_nor_read_sfdp(), QSPI cntlr
> AHB mode initialization is not yet done results in data read error.
> 
> Initialize AHB read inside fsl_qspi_nor_setup().
> 
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> ---
> Changes for v7:
> - None
> Changes for v6:
> - None
> Changes for v5:
> - None
> Changes for v4:
> - None
> Changes for v3:
> - None
> Changes for v2:
> - None
> 
>   drivers/mtd/spi-nor/fsl-quadspi.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 64b8bb3..424094a 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -820,6 +820,9 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
>   	/* enable the interrupt */
>   	qspi_writel(q, QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
>   
> +	/* Init for AHB read */
> +	fsl_qspi_init_ahb_read(q);

Maybe I missed something, but it seems if you move 
fsl_qspi_init_ahb_read() here and fsl_qspi_read() is used before 
fsl_qspi_nor_setup_last() is called, then the read command is triggered 
with an uninitialized LUT entry (SEQID_LUT2_AHBREAD).

So shouldn't the LUT entry for AHB read be initialized with some default 
values here, before spi_nor_scan() is called?

Thanks,

Frieder

> +
>   	return 0;
>   }
>   
> @@ -842,9 +845,6 @@ static int fsl_qspi_nor_setup_last(struct fsl_qspi *q)
>   	if (ret)
>   		return ret;
>   
> -	/* Init for AHB read */
> -	fsl_qspi_init_ahb_read(q);
> -
>   	/* Prepare LUT for AHB read - required for read from devmem interface */
>   	fsl_qspi_prep_ahb_read(q);
>   
> 

RE: [PATCH v7 2/2] mtd: fsl-quadspi: fix init AHB read in fsl_qspi_nor_setup()

[Yogesh Narayan Gaur]

Hi Frieder,

> -----Original Message-----
> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of
> Frieder Schrempf
> Sent: Thursday, March 22, 2018 2:49 PM
> To: Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com>; linux-
> mtd@lists.infradead.org
> Cc: boris.brezillon@free-electrons.com; marek.vasut@gmail.com; Prabhakar
> Kushwaha <prabhakar.kushwaha@nxp.com>; Suresh Gupta
> <suresh.gupta@nxp.com>; cyrille.pitchen@wedev4u.fr; Han Xu
> <han.xu@nxp.com>; computersforpeace@gmail.com; festevam@gmail.com
> Subject: Re: [PATCH v7 2/2] mtd: fsl-quadspi: fix init AHB read in
> fsl_qspi_nor_setup()
> 
> Hi Yogesh,
> 
> On 21.03.2018 12:26, Yogesh Gaur wrote:
> > Move AHB read initialize in fsl_qspi_nor_setup().
> >
> > In file spi-nor.c, func spi_nor_scan() calls nor->read() thru
> > spi_nor_read_sfdp() API.
> >
> > Presently, fsl_qspi_init_ahb_read() called from fsl_qspi_nor_setup_last().
> > Func fsl_qspi_probe() calls spi_nor_scan() first and then calls
> > fsl_qspi_nor_setup_last().
> >
> > When nor->read() being called from inside spi_nor_read_sfdp(), QSPI
> > cntlr AHB mode initialization is not yet done results in data read error.
> >
> > Initialize AHB read inside fsl_qspi_nor_setup().
> >
> > Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> > ---
> > Changes for v7:
> > - None
> > Changes for v6:
> > - None
> > Changes for v5:
> > - None
> > Changes for v4:
> > - None
> > Changes for v3:
> > - None
> > Changes for v2:
> > - None
> >
> >   drivers/mtd/spi-nor/fsl-quadspi.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c
> > b/drivers/mtd/spi-nor/fsl-quadspi.c
> > index 64b8bb3..424094a 100644
> > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > @@ -820,6 +820,9 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
> >   	/* enable the interrupt */
> >   	qspi_writel(q, QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
> >
> > +	/* Init for AHB read */
> > +	fsl_qspi_init_ahb_read(q);
> 
> Maybe I missed something, but it seems if you move
> fsl_qspi_init_ahb_read() here and fsl_qspi_read() is used before
> fsl_qspi_nor_setup_last() is called, then the read command is triggered with an
> uninitialized LUT entry (SEQID_LUT2_AHBREAD).
> 
For every call to fsl_qspi_read(), I am preparing the LUT before doing actual read so LUT entry for SEQID_LUT2_AHBREAD is never be uninitialized.

Rationale behind moving fsl_qspi_init_ahb_read() from fsl_qspi_nor_setup_last() to fsl_qspi_nor_setup() is to initialize registers which are required for AHB read operation like setting of values in QUADSPI_BUF3CR, QUADSPI_BFGENCR etc registers as fsl_qspi_read() is called, before call to fsl_qspi_nor_setup_last(), through spi_nor_read_sfdp().

--
Thanks
Yogesh Gaur

> So shouldn't the LUT entry for AHB read be initialized with some default values
> here, before spi_nor_scan() is called?
> 
> Thanks,
> 
> Frieder
> 
> > +
> >   	return 0;
> >   }
> >
> > @@ -842,9 +845,6 @@ static int fsl_qspi_nor_setup_last(struct fsl_qspi *q)
> >   	if (ret)
> >   		return ret;
> >
> > -	/* Init for AHB read */
> > -	fsl_qspi_init_ahb_read(q);
> > -
> >   	/* Prepare LUT for AHB read - required for read from devmem interface
> */
> >   	fsl_qspi_prep_ahb_read(q);
> >
> >
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr
> adead.org%2Fmailman%2Flistinfo%2Flinux-
> mtd%2F&data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C427c291bd9c
> f4eafa3bc08d58fd60d88%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C636573071879930483&sdata=qL4febRxR%2BvWiSOcnvxgqLzqOX%2BqTV4PA
> UorfjgcLUI%3D&reserved=0

Re: [PATCH v7 2/2] mtd: fsl-quadspi: fix init AHB read in fsl_qspi_nor_setup()

[Frieder Schrempf]

Hi Yogesh,

On 22.03.2018 11:37, Yogesh Narayan Gaur wrote:
> Hi Frieder,
> 
>> -----Original Message-----
>> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of
>> Frieder Schrempf
>> Sent: Thursday, March 22, 2018 2:49 PM
>> To: Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com>; linux-
>> mtd@lists.infradead.org
>> Cc: boris.brezillon@free-electrons.com; marek.vasut@gmail.com; Prabhakar
>> Kushwaha <prabhakar.kushwaha@nxp.com>; Suresh Gupta
>> <suresh.gupta@nxp.com>; cyrille.pitchen@wedev4u.fr; Han Xu
>> <han.xu@nxp.com>; computersforpeace@gmail.com; festevam@gmail.com
>> Subject: Re: [PATCH v7 2/2] mtd: fsl-quadspi: fix init AHB read in
>> fsl_qspi_nor_setup()
>>
>> Hi Yogesh,
>>
>> On 21.03.2018 12:26, Yogesh Gaur wrote:
>>> Move AHB read initialize in fsl_qspi_nor_setup().
>>>
>>> In file spi-nor.c, func spi_nor_scan() calls nor->read() thru
>>> spi_nor_read_sfdp() API.
>>>
>>> Presently, fsl_qspi_init_ahb_read() called from fsl_qspi_nor_setup_last().
>>> Func fsl_qspi_probe() calls spi_nor_scan() first and then calls
>>> fsl_qspi_nor_setup_last().
>>>
>>> When nor->read() being called from inside spi_nor_read_sfdp(), QSPI
>>> cntlr AHB mode initialization is not yet done results in data read error.
>>>
>>> Initialize AHB read inside fsl_qspi_nor_setup().
>>>
>>> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
>>> ---
>>> Changes for v7:
>>> - None
>>> Changes for v6:
>>> - None
>>> Changes for v5:
>>> - None
>>> Changes for v4:
>>> - None
>>> Changes for v3:
>>> - None
>>> Changes for v2:
>>> - None
>>>
>>>    drivers/mtd/spi-nor/fsl-quadspi.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c
>>> b/drivers/mtd/spi-nor/fsl-quadspi.c
>>> index 64b8bb3..424094a 100644
>>> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
>>> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
>>> @@ -820,6 +820,9 @@ static int fsl_qspi_nor_setup(struct fsl_qspi *q)
>>>    	/* enable the interrupt */
>>>    	qspi_writel(q, QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
>>>
>>> +	/* Init for AHB read */
>>> +	fsl_qspi_init_ahb_read(q);
>>
>> Maybe I missed something, but it seems if you move
>> fsl_qspi_init_ahb_read() here and fsl_qspi_read() is used before
>> fsl_qspi_nor_setup_last() is called, then the read command is triggered with an
>> uninitialized LUT entry (SEQID_LUT2_AHBREAD).
>>
> For every call to fsl_qspi_read(), I am preparing the LUT before doing actual read so LUT entry for SEQID_LUT2_AHBREAD is never be uninitialized.

Ok, I missed that. Thank you for clarifying this.

Thanks,

Frieder

> 
> Rationale behind moving fsl_qspi_init_ahb_read() from fsl_qspi_nor_setup_last() to fsl_qspi_nor_setup() is to initialize registers which are required for AHB read operation like setting of values in QUADSPI_BUF3CR, QUADSPI_BFGENCR etc registers as fsl_qspi_read() is called, before call to fsl_qspi_nor_setup_last(), through spi_nor_read_sfdp().
> 
> --
> Thanks
> Yogesh Gaur
> 
>> So shouldn't the LUT entry for AHB read be initialized with some default values
>> here, before spi_nor_scan() is called?
>>
>> Thanks,
>>
>> Frieder
>>
>>> +
>>>    	return 0;
>>>    }
>>>
>>> @@ -842,9 +845,6 @@ static int fsl_qspi_nor_setup_last(struct fsl_qspi *q)
>>>    	if (ret)
>>>    		return ret;
>>>
>>> -	/* Init for AHB read */
>>> -	fsl_qspi_init_ahb_read(q);
>>> -
>>>    	/* Prepare LUT for AHB read - required for read from devmem interface
>> */
>>>    	fsl_qspi_prep_ahb_read(q);
>>>
>>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> https://smex12-5-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2femea01.safelinks.protection.outlook.com%2f%3furl%3dhttp%253A%252F%252Flists.infr&umid=271ec41c-fb0a-44d3-8e2f-390bb6c9a99b&auth=541e45255b6517100d80c2b1b80b6933b203c492-5a522acd906e3d7618eaba4ad9ad144f477c8f9c
>> adead.org%2Fmailman%2Flistinfo%2Flinux-
>> mtd%2F&data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C427c291bd9c
>> f4eafa3bc08d58fd60d88%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
>> 7C636573071879930483&sdata=qL4febRxR%2BvWiSOcnvxgqLzqOX%2BqTV4PA
>> UorfjgcLUI%3D&reserved=0