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

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.
This API would fetch required info like opcode, protocol info, dummy info
for creating LUT from instance of 'struct spi_nor' and then prepare LUT
entry for the required command.

Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
---
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 | 310 +++++++++++++++++++++-----------------
 1 file changed, 168 insertions(+), 142 deletions(-)

On 01/30/2018 10:49 AM, 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.
> This API would fetch required info like opcode, protocol info, dummy info
> for creating LUT from instance of 'struct spi_nor' and then prepare LUT
> entry for the required command.
>
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> ---
> 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 | 310 +++++++++++++++++++++-----------------
>   1 file changed, 168 insertions(+), 142 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 89306cf..84c341b 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -183,7 +183,7 @@
>   
>   /* Macros for constructing the LUT register. */
>   #define LUT0(ins, pad, opr)						\
> -		(((opr) << OPRND0_SHIFT) | ((LUT_##pad) << PAD0_SHIFT) | \
> +		(((opr) << OPRND0_SHIFT) | ((pad) << PAD0_SHIFT) | \
>   		((LUT_##ins) << INSTR0_SHIFT))
>   
>   #define LUT1(ins, pad, opr)	(LUT0(ins, pad, opr) << OPRND1_SHIFT)
> @@ -193,21 +193,21 @@
>   #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
> +/* LUT0 programmed by bootloader, for run-time create entry for LUT seqid 1 */
> +#define SEQID_LUT0_BOOTLOADER	0
> +#define SEQID_LUT1_RUNTIME	1
>   
>   #define QUADSPI_MIN_IOMAP SZ_4M
>   
> +enum fsl_qspi_ops {
> +	FSL_QSPI_OPS_READ = 0,
> +	FSL_QSPI_OPS_WRITE,
> +	FSL_QSPI_OPS_ERASE,
> +	FSL_QSPI_OPS_READ_REG,
> +	FSL_QSPI_OPS_WRITE_REG,
> +	FSL_QSPI_OPS_WRITE_BUF_REG,
> +};
> +
>   enum fsl_qspi_devtype {
>   	FSL_QUADSPI_VYBRID,
>   	FSL_QUADSPI_IMX6SX,
> @@ -368,136 +368,158 @@ 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 entry for the input cmd.
> + * Protocol info is present in instance of struct spi_nor, using which fields
> + * like cmd, data, addrlen along with pad info etc can be parsed.
> + */
> +static void fsl_qspi_prepare_lut(struct spi_nor *nor,
> +				 enum fsl_qspi_ops ops, u8 cmd)
> +{
> +	struct fsl_qspi *q = nor->priv;
>   	void __iomem *base = q->iobase;
>   	int rxfifo = q->devtype_data->rxfifo;
> +	int txfifo = q->devtype_data->txfifo;
>   	u32 lut_base;
> -	int i;
> +	u8 cmd_pad, addr_pad, data_pad, dummy_pad;
> +	enum spi_nor_protocol protocol = 0;
> +	u8 addrlen = 0;
> +	u8 read_dm, opcode;
> +	int stop_lut;
> +
> +	read_dm = opcode = cmd_pad = addr_pad = data_pad = dummy_pad = 0;
> +
> +	switch (ops) {
> +	case FSL_QSPI_OPS_READ_REG:
> +	case FSL_QSPI_OPS_WRITE_REG:
> +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> +		opcode = cmd;
> +		protocol = nor->reg_proto;
> +		break;
> +	case FSL_QSPI_OPS_READ:
> +		opcode = cmd;
> +		read_dm = nor->read_dummy;
> +		protocol = nor->read_proto;
> +		break;
> +	case FSL_QSPI_OPS_WRITE:
> +		opcode = cmd;
> +		protocol = nor->write_proto;
> +		break;
> +	case FSL_QSPI_OPS_ERASE:
> +		opcode = cmd;
> +		break;
> +	default:
> +		dev_err(q->dev, "Unsupported operation 0x%.2x\n", ops);
> +		return;
> +	}
> +
> +	if (protocol) {
> +		cmd_pad = spi_nor_get_protocol_inst_nbits(protocol);
> +		addr_pad = spi_nor_get_protocol_addr_nbits(protocol);
> +		data_pad = spi_nor_get_protocol_data_nbits(protocol);
> +	}
>   
> -	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;
> +	dummy_pad = data_pad;
> +
> +	dev_dbg(q->dev, "ops:%x opcode:%x pad[cmd:%d, addr:%d, data:%d]\n",
> +			ops, opcode, cmd_pad, addr_pad, data_pad);
>   
>   	fsl_qspi_unlock_lut(q);
>   
> -	/* 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));
> +	/* Dynamic LUT */
> +	lut_base = SEQID_LUT1_RUNTIME * 4;
>   
> -	fsl_qspi_lock_lut(q);
> -}
> +	/* default, STOP instruction to be programmed in (lut_base + 1) reg */
> +	stop_lut = 1;
> +	switch (ops) {
> +	case FSL_QSPI_OPS_READ_REG:
> +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> +			  LUT1(FSL_READ, pad_count(data_pad), rxfifo),
> +			  base + QUADSPI_LUT(lut_base));
> +		break;
> +	case FSL_QSPI_OPS_WRITE_REG:
> +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode),
> +			  base + QUADSPI_LUT(lut_base));
> +		break;
> +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> +			  LUT1(FSL_WRITE, pad_count(data_pad), txfifo),
> +			  base + QUADSPI_LUT(lut_base));
> +		break;
> +	case FSL_QSPI_OPS_READ:
> +	case FSL_QSPI_OPS_WRITE:
> +	case FSL_QSPI_OPS_ERASE:
> +		/* Common for Read, Write and Erase ops. */
>   
> -/* 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;
> +		addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> +
> +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> +				LUT1(ADDR, pad_count(addr_pad), addrlen),
> +				base + QUADSPI_LUT(lut_base));
> +		/*
> +		 * For Erase ops - Data and Dummy not required.
> +		 * For Write ops - Dummy not required.
> +		 */
> +
> +		if (ops == FSL_QSPI_OPS_READ) {
> +
> +			/*
> +			 * For cmds SPINOR_OP_READ and SPINOR_OP_READ_4B value
> +			 * of dummy cycles are 0.
> +			 */
> +			if (read_dm)
> +				qspi_writel(q,
> +					    LUT0(DUMMY, pad_count(dummy_pad),
> +					    read_dm) |
> +					    LUT1(FSL_READ, pad_count(data_pad),
> +					    rxfifo),
> +					    base + QUADSPI_LUT(lut_base + 1));
> +			else
> +				qspi_writel(q,
> +					    LUT0(FSL_READ, pad_count(data_pad),
> +					    rxfifo),
> +					    base + QUADSPI_LUT(lut_base + 1));
> +
> +			stop_lut = 2;
> +
> +			/* TODO Add condition to check if READ is IP/AHB. */
> +
> +			/* For AHB read, add seqid in BFGENCR register. */
> +			qspi_writel(q,
> +				    SEQID_LUT1_RUNTIME <<
> +				    QUADSPI_BFGENCR_SEQID_SHIFT,
> +				    q->iobase + QUADSPI_BFGENCR);
> +		}
> +
> +		if (ops == FSL_QSPI_OPS_WRITE) {
> +			qspi_writel(q, LUT0(FSL_WRITE, pad_count(data_pad), 0),
> +					base + QUADSPI_LUT(lut_base + 1));
> +			stop_lut = 2;
> +		}
> +		break;
>   	default:
> -		if (cmd == q->nor[0].erase_opcode)
> -			return SEQID_SE;
> -		dev_err(q->dev, "Unsupported cmd 0x%.2x\n", cmd);
> +		dev_err(q->dev, "Unsupported operation 0x%.2x\n", ops);
>   		break;
>   	}
> -	return -EINVAL;
> +
> +	/* prepare LUT for STOP instruction. */
> +	qspi_writel(q, 0, base +  QUADSPI_LUT(lut_base + stop_lut));
> +
> +	fsl_qspi_lock_lut(q);
>   }
>   

It's not exact my expected dynamic implementation, this code change 
merely use 1 LUT set and need to update it for each operation. My 
previous thought was use as much LUT as possible and reserve the last 
one (or one pair) LUT(s) for dynamical update.

I am okay with this implementation. But I see some error when testing on 
i.MX7, I need to dig the issue. Please let me know on which platform did 
you test?

>   static int
> @@ -532,7 +554,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_RUNTIME;
>   	qspi_writel(q, (seqid << QUADSPI_IPCR_SEQID_SHIFT) | len,
>   			base + QUADSPI_IPCR);
>   
> @@ -684,8 +706,8 @@ 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_LUT1_RUNTIME;
>   	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
>   		q->iobase + QUADSPI_BFGENCR);
>   }
> @@ -728,7 +750,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 +767,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,9 +809,6 @@ 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);
>   
> @@ -820,6 +835,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>   	int ret;
>   	struct fsl_qspi *q = nor->priv;
>   
> +	fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_READ_REG, opcode);
>   	ret = fsl_qspi_runcmd(q, opcode, 0, len);
>   	if (ret)
>   		return ret;
> @@ -834,6 +850,8 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>   	int ret;
>   
>   	if (!buf) {
> +		/* Prepare LUT for WRITE_REG cmd with input BUF as NULL. */
> +		fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_WRITE_REG, opcode);
>   		ret = fsl_qspi_runcmd(q, opcode, 0, 1);
>   		if (ret)
>   			return ret;
> @@ -842,6 +860,8 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>   			fsl_qspi_invalid(q);
>   
>   	} else if (len > 0) {
> +		/* Prepare LUT for WRITE_REG cmd with input BUF non-NULL. */
> +		fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_WRITE_BUF_REG, opcode);
>   		ret = fsl_qspi_nor_write(q, nor, opcode, 0,
>   					(u32 *)buf, len);
>   		if (ret > 0)
> @@ -858,8 +878,11 @@ 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;
> +
> +	fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_WRITE, nor->program_opcode);
> +	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);
> @@ -872,6 +895,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t from,
>   	struct fsl_qspi *q = nor->priv;
>   	u8 cmd = nor->read_opcode;
>   
> +	fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_READ, nor->read_opcode);
> +
>   	/* if necessary,ioremap buffer before AHB read, */
>   	if (!q->ahb_addr) {
>   		q->memmap_offs = q->chip_base_addr + from;
> @@ -920,6 +945,7 @@ static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
>   	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
>   		nor->mtd.erasesize / 1024, q->chip_base_addr, (u32)offs);
>   
> +	fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_ERASE, nor->erase_opcode);
>   	ret = fsl_qspi_runcmd(q, nor->erase_opcode, offs, 0);
>   	if (ret)
>   		return ret;

Hi Han,

> -----Original Message-----
> From: Han Xu
> Sent: Tuesday, February 13, 2018 4:27 AM
> To: Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com>; linux-
> mtd@lists.infradead.org
> Cc: boris.brezillon@free-electrons.com; cyrille.pitchen@wedev4u.fr;
> computersforpeace@gmail.com; festevam@gmail.com; Prabhakar Kushwaha
> <prabhakar.kushwaha@nxp.com>; Suresh Gupta <suresh.gupta@nxp.com>
> Subject: Re: [PATCH v5 1/2] mtd: fsl-quadspi: add support to create dynamic LUT
> entry
> 
> 
> 
> On 01/30/2018 10:49 AM, 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.
> > This API would fetch required info like opcode, protocol info, dummy
> > info for creating LUT from instance of 'struct spi_nor' and then
> > prepare LUT entry for the required command.
> >
> > Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> > ---
> > 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 | 310 +++++++++++++++++++++--------------
> ---
> >   1 file changed, 168 insertions(+), 142 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c
> > b/drivers/mtd/spi-nor/fsl-quadspi.c
> > index 89306cf..84c341b 100644
> > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > @@ -183,7 +183,7 @@
> >
> >   /* Macros for constructing the LUT register. */
> >   #define LUT0(ins, pad, opr)						\
> > -		(((opr) << OPRND0_SHIFT) | ((LUT_##pad) << PAD0_SHIFT) | \
> > +		(((opr) << OPRND0_SHIFT) | ((pad) << PAD0_SHIFT) | \
> >   		((LUT_##ins) << INSTR0_SHIFT))
> >
> >   #define LUT1(ins, pad, opr)	(LUT0(ins, pad, opr) << OPRND1_SHIFT)
> > @@ -193,21 +193,21 @@
> >   #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
> > +/* LUT0 programmed by bootloader, for run-time create entry for LUT seqid
> 1 */
> > +#define SEQID_LUT0_BOOTLOADER	0
> > +#define SEQID_LUT1_RUNTIME	1
> >
> >   #define QUADSPI_MIN_IOMAP SZ_4M
> >
> > +enum fsl_qspi_ops {
> > +	FSL_QSPI_OPS_READ = 0,
> > +	FSL_QSPI_OPS_WRITE,
> > +	FSL_QSPI_OPS_ERASE,
> > +	FSL_QSPI_OPS_READ_REG,
> > +	FSL_QSPI_OPS_WRITE_REG,
> > +	FSL_QSPI_OPS_WRITE_BUF_REG,
> > +};
> > +
> >   enum fsl_qspi_devtype {
> >   	FSL_QUADSPI_VYBRID,
> >   	FSL_QUADSPI_IMX6SX,
> > @@ -368,136 +368,158 @@ 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 entry for the input cmd.
> > + * Protocol info is present in instance of struct spi_nor, using
> > +which fields
> > + * like cmd, data, addrlen along with pad info etc can be parsed.
> > + */
> > +static void fsl_qspi_prepare_lut(struct spi_nor *nor,
> > +				 enum fsl_qspi_ops ops, u8 cmd)
> > +{
> > +	struct fsl_qspi *q = nor->priv;
> >   	void __iomem *base = q->iobase;
> >   	int rxfifo = q->devtype_data->rxfifo;
> > +	int txfifo = q->devtype_data->txfifo;
> >   	u32 lut_base;
> > -	int i;
> > +	u8 cmd_pad, addr_pad, data_pad, dummy_pad;
> > +	enum spi_nor_protocol protocol = 0;
> > +	u8 addrlen = 0;
> > +	u8 read_dm, opcode;
> > +	int stop_lut;
> > +
> > +	read_dm = opcode = cmd_pad = addr_pad = data_pad = dummy_pad =
> 0;
> > +
> > +	switch (ops) {
> > +	case FSL_QSPI_OPS_READ_REG:
> > +	case FSL_QSPI_OPS_WRITE_REG:
> > +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> > +		opcode = cmd;
> > +		protocol = nor->reg_proto;
> > +		break;
> > +	case FSL_QSPI_OPS_READ:
> > +		opcode = cmd;
> > +		read_dm = nor->read_dummy;
> > +		protocol = nor->read_proto;
> > +		break;
> > +	case FSL_QSPI_OPS_WRITE:
> > +		opcode = cmd;
> > +		protocol = nor->write_proto;
> > +		break;
> > +	case FSL_QSPI_OPS_ERASE:
> > +		opcode = cmd;
> > +		break;
> > +	default:
> > +		dev_err(q->dev, "Unsupported operation 0x%.2x\n", ops);
> > +		return;
> > +	}
> > +
> > +	if (protocol) {
> > +		cmd_pad = spi_nor_get_protocol_inst_nbits(protocol);
> > +		addr_pad = spi_nor_get_protocol_addr_nbits(protocol);
> > +		data_pad = spi_nor_get_protocol_data_nbits(protocol);
> > +	}
> >
> > -	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;
> > +	dummy_pad = data_pad;
> > +
> > +	dev_dbg(q->dev, "ops:%x opcode:%x pad[cmd:%d, addr:%d,
> data:%d]\n",
> > +			ops, opcode, cmd_pad, addr_pad, data_pad);
> >
> >   	fsl_qspi_unlock_lut(q);
> >
> > -	/* 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));
> > +	/* Dynamic LUT */
> > +	lut_base = SEQID_LUT1_RUNTIME * 4;
> >
> > -	fsl_qspi_lock_lut(q);
> > -}
> > +	/* default, STOP instruction to be programmed in (lut_base + 1) reg */
> > +	stop_lut = 1;
> > +	switch (ops) {
> > +	case FSL_QSPI_OPS_READ_REG:
> > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> > +			  LUT1(FSL_READ, pad_count(data_pad), rxfifo),
> > +			  base + QUADSPI_LUT(lut_base));
> > +		break;
> > +	case FSL_QSPI_OPS_WRITE_REG:
> > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode),
> > +			  base + QUADSPI_LUT(lut_base));
> > +		break;
> > +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> > +			  LUT1(FSL_WRITE, pad_count(data_pad), txfifo),
> > +			  base + QUADSPI_LUT(lut_base));
> > +		break;
> > +	case FSL_QSPI_OPS_READ:
> > +	case FSL_QSPI_OPS_WRITE:
> > +	case FSL_QSPI_OPS_ERASE:
> > +		/* Common for Read, Write and Erase ops. */
> >
> > -/* 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;
> > +		addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> > +
> > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> > +				LUT1(ADDR, pad_count(addr_pad), addrlen),
> > +				base + QUADSPI_LUT(lut_base));
> > +		/*
> > +		 * For Erase ops - Data and Dummy not required.
> > +		 * For Write ops - Dummy not required.
> > +		 */
> > +
> > +		if (ops == FSL_QSPI_OPS_READ) {
> > +
> > +			/*
> > +			 * For cmds SPINOR_OP_READ and
> SPINOR_OP_READ_4B value
> > +			 * of dummy cycles are 0.
> > +			 */
> > +			if (read_dm)
> > +				qspi_writel(q,
> > +					    LUT0(DUMMY,
> pad_count(dummy_pad),
> > +					    read_dm) |
> > +					    LUT1(FSL_READ,
> pad_count(data_pad),
> > +					    rxfifo),
> > +					    base + QUADSPI_LUT(lut_base + 1));
> > +			else
> > +				qspi_writel(q,
> > +					    LUT0(FSL_READ,
> pad_count(data_pad),
> > +					    rxfifo),
> > +					    base + QUADSPI_LUT(lut_base + 1));
> > +
> > +			stop_lut = 2;
> > +
> > +			/* TODO Add condition to check if READ is IP/AHB. */
> > +
> > +			/* For AHB read, add seqid in BFGENCR register. */
> > +			qspi_writel(q,
> > +				    SEQID_LUT1_RUNTIME <<
> > +				    QUADSPI_BFGENCR_SEQID_SHIFT,
> > +				    q->iobase + QUADSPI_BFGENCR);
> > +		}
> > +
> > +		if (ops == FSL_QSPI_OPS_WRITE) {
> > +			qspi_writel(q, LUT0(FSL_WRITE, pad_count(data_pad),
> 0),
> > +					base + QUADSPI_LUT(lut_base + 1));
> > +			stop_lut = 2;
> > +		}
> > +		break;
> >   	default:
> > -		if (cmd == q->nor[0].erase_opcode)
> > -			return SEQID_SE;
> > -		dev_err(q->dev, "Unsupported cmd 0x%.2x\n", cmd);
> > +		dev_err(q->dev, "Unsupported operation 0x%.2x\n", ops);
> >   		break;
> >   	}
> > -	return -EINVAL;
> > +
> > +	/* prepare LUT for STOP instruction. */
> > +	qspi_writel(q, 0, base +  QUADSPI_LUT(lut_base + stop_lut));
> > +
> > +	fsl_qspi_lock_lut(q);
> >   }
> >
> 
> It's not exact my expected dynamic implementation, this code change merely
> use 1 LUT set and need to update it for each operation. My previous thought
> was use as much LUT as possible and reserve the last one (or one pair) LUT(s) for
> dynamical update.
> 
Initially our thought was also same that reserved some LUTs for cmds like RDSR, WE etc and have dynamic LUT for Read/Write/Erase operation.
But that would bring more code complexity.
As well we haven't seen performance degradation when preparing LUT for all cmnds dynamically.

> I am okay with this implementation. But I see some error when testing on i.MX7,
> I need to dig the issue. Please let me know on which platform did you test?
> 
I have tested this code on LS1088-ARDB (ARMv8) platform, this is having Spansion flash "S25FS512S".
Have done testing for
READ protocol - SNOR_HWCAPS_READ, SNOR_HWCAPS_READ_FAST, SNOR_HWCAPS_READ_1_2_2 and SNOR_HWCAPS_READ_1_4_4
Write protocol - SNOR_HWCAPS_PP

--
Regards,
Yogesh Gaur

> >   static int
> > @@ -532,7 +554,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_RUNTIME;
> >   	qspi_writel(q, (seqid << QUADSPI_IPCR_SEQID_SHIFT) | len,
> >   			base + QUADSPI_IPCR);
> >
> > @@ -684,8 +706,8 @@ 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_LUT1_RUNTIME;
> >   	qspi_writel(q, seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
> >   		q->iobase + QUADSPI_BFGENCR);
> >   }
> > @@ -728,7 +750,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 +767,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,9 +809,6 @@ 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);
> >
> > @@ -820,6 +835,7 @@ static int fsl_qspi_read_reg(struct spi_nor *nor, u8
> opcode, u8 *buf, int len)
> >   	int ret;
> >   	struct fsl_qspi *q = nor->priv;
> >
> > +	fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_READ_REG, opcode);
> >   	ret = fsl_qspi_runcmd(q, opcode, 0, len);
> >   	if (ret)
> >   		return ret;
> > @@ -834,6 +850,8 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8
> opcode, u8 *buf, int len)
> >   	int ret;
> >
> >   	if (!buf) {
> > +		/* Prepare LUT for WRITE_REG cmd with input BUF as NULL. */
> > +		fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_WRITE_REG, opcode);
> >   		ret = fsl_qspi_runcmd(q, opcode, 0, 1);
> >   		if (ret)
> >   			return ret;
> > @@ -842,6 +860,8 @@ static int fsl_qspi_write_reg(struct spi_nor *nor, u8
> opcode, u8 *buf, int len)
> >   			fsl_qspi_invalid(q);
> >
> >   	} else if (len > 0) {
> > +		/* Prepare LUT for WRITE_REG cmd with input BUF non-NULL.
> */
> > +		fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_WRITE_BUF_REG,
> opcode);
> >   		ret = fsl_qspi_nor_write(q, nor, opcode, 0,
> >   					(u32 *)buf, len);
> >   		if (ret > 0)
> > @@ -858,8 +878,11 @@ 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;
> > +
> > +	fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_WRITE, nor-
> >program_opcode);
> > +	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);
> > @@ -872,6 +895,8 @@ static ssize_t fsl_qspi_read(struct spi_nor *nor, loff_t
> from,
> >   	struct fsl_qspi *q = nor->priv;
> >   	u8 cmd = nor->read_opcode;
> >
> > +	fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_READ, nor->read_opcode);
> > +
> >   	/* if necessary,ioremap buffer before AHB read, */
> >   	if (!q->ahb_addr) {
> >   		q->memmap_offs = q->chip_base_addr + from; @@ -920,6
> +945,7 @@
> > static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
> >   	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
> >   		nor->mtd.erasesize / 1024, q->chip_base_addr, (u32)offs);
> >
> > +	fsl_qspi_prepare_lut(nor, FSL_QSPI_OPS_ERASE, nor->erase_opcode);
> >   	ret = fsl_qspi_runcmd(q, nor->erase_opcode, offs, 0);
> >   	if (ret)
> >   		return ret;

+Frieder

Hi Yogesh,

On Tue, 30 Jan 2018 22:19:29 +0530
Yogesh Gaur <yogeshnarayan.gaur@nxp.com> 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.
> This API would fetch required info like opcode, protocol info, dummy info
> for creating LUT from instance of 'struct spi_nor' and then prepare LUT
> entry for the required command.

You're probably already aware that I proposed a new "generic spi-mem
API" [1] and started to port the fsl-qspi driver [2] to this new
interface. While doing that I came to the same conclusion: given the
small overhead implied by LUT preparation, we can prepare OP sequences
dynamically instead of hardcoding them at probe time.

I know I'm talking about hypothetical changes while you're providing
patches for something that already exists and is in use, but maybe it'd
be worth discussing things a bit so that your changes go in the
direction of the spi-mem ones.

> 
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> ---
> 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 | 310 +++++++++++++++++++++-----------------
>  1 file changed, 168 insertions(+), 142 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 89306cf..84c341b 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -183,7 +183,7 @@
>  
>  /* Macros for constructing the LUT register. */
>  #define LUT0(ins, pad, opr)						\
> -		(((opr) << OPRND0_SHIFT) | ((LUT_##pad) << PAD0_SHIFT) | \
> +		(((opr) << OPRND0_SHIFT) | ((pad) << PAD0_SHIFT) | \
>  		((LUT_##ins) << INSTR0_SHIFT))
>  
>  #define LUT1(ins, pad, opr)	(LUT0(ins, pad, opr) << OPRND1_SHIFT)
> @@ -193,21 +193,21 @@
>  #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
> +/* LUT0 programmed by bootloader, for run-time create entry for LUT seqid 1 */
> +#define SEQID_LUT0_BOOTLOADER	0
> +#define SEQID_LUT1_RUNTIME	1
>  
>  #define QUADSPI_MIN_IOMAP SZ_4M
>  
> +enum fsl_qspi_ops {
> +	FSL_QSPI_OPS_READ = 0,
> +	FSL_QSPI_OPS_WRITE,
> +	FSL_QSPI_OPS_ERASE,
> +	FSL_QSPI_OPS_READ_REG,
> +	FSL_QSPI_OPS_WRITE_REG,
> +	FSL_QSPI_OPS_WRITE_BUF_REG,
> +};

Could we express things in something more extensible? I mean, something
closer to the spi_mem_op structure I add here [1]. This way the
transition to the spi-mem interface will be easier.

> +
>  enum fsl_qspi_devtype {
>  	FSL_QUADSPI_VYBRID,
>  	FSL_QUADSPI_IMX6SX,
> @@ -368,136 +368,158 @@ 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 entry for the input cmd.
> + * Protocol info is present in instance of struct spi_nor, using which fields
> + * like cmd, data, addrlen along with pad info etc can be parsed.
> + */
> +static void fsl_qspi_prepare_lut(struct spi_nor *nor,
> +				 enum fsl_qspi_ops ops, u8 cmd)
> +{
> +	struct fsl_qspi *q = nor->priv;
>  	void __iomem *base = q->iobase;
>  	int rxfifo = q->devtype_data->rxfifo;
> +	int txfifo = q->devtype_data->txfifo;
>  	u32 lut_base;
> -	int i;
> +	u8 cmd_pad, addr_pad, data_pad, dummy_pad;
> +	enum spi_nor_protocol protocol = 0;
> +	u8 addrlen = 0;
> +	u8 read_dm, opcode;
> +	int stop_lut;
> +
> +	read_dm = opcode = cmd_pad = addr_pad = data_pad = dummy_pad = 0;
> +
> +	switch (ops) {
> +	case FSL_QSPI_OPS_READ_REG:
> +	case FSL_QSPI_OPS_WRITE_REG:
> +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> +		opcode = cmd;
> +		protocol = nor->reg_proto;
> +		break;
> +	case FSL_QSPI_OPS_READ:
> +		opcode = cmd;
> +		read_dm = nor->read_dummy;
> +		protocol = nor->read_proto;
> +		break;
> +	case FSL_QSPI_OPS_WRITE:
> +		opcode = cmd;
> +		protocol = nor->write_proto;
> +		break;
> +	case FSL_QSPI_OPS_ERASE:
> +		opcode = cmd;
> +		break;
> +	default:
> +		dev_err(q->dev, "Unsupported operation 0x%.2x\n", ops);
> +		return;
> +	}
> +
> +	if (protocol) {
> +		cmd_pad = spi_nor_get_protocol_inst_nbits(protocol);
> +		addr_pad = spi_nor_get_protocol_addr_nbits(protocol);
> +		data_pad = spi_nor_get_protocol_data_nbits(protocol);
> +	}
>  
> -	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;
> +	dummy_pad = data_pad;
> +
> +	dev_dbg(q->dev, "ops:%x opcode:%x pad[cmd:%d, addr:%d, data:%d]\n",
> +			ops, opcode, cmd_pad, addr_pad, data_pad);
>  
>  	fsl_qspi_unlock_lut(q);
>  
> -	/* 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));
> +	/* Dynamic LUT */
> +	lut_base = SEQID_LUT1_RUNTIME * 4;
>  
> -	fsl_qspi_lock_lut(q);
> -}
> +	/* default, STOP instruction to be programmed in (lut_base + 1) reg */
> +	stop_lut = 1;
> +	switch (ops) {
> +	case FSL_QSPI_OPS_READ_REG:
> +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> +			  LUT1(FSL_READ, pad_count(data_pad), rxfifo),
> +			  base + QUADSPI_LUT(lut_base));
> +		break;
> +	case FSL_QSPI_OPS_WRITE_REG:
> +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode),
> +			  base + QUADSPI_LUT(lut_base));
> +		break;
> +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> +			  LUT1(FSL_WRITE, pad_count(data_pad), txfifo),
> +			  base + QUADSPI_LUT(lut_base));
> +		break;
> +	case FSL_QSPI_OPS_READ:
> +	case FSL_QSPI_OPS_WRITE:
> +	case FSL_QSPI_OPS_ERASE:
> +		/* Common for Read, Write and Erase ops. */
>  
> -/* 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;
> +		addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> +
> +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> +				LUT1(ADDR, pad_count(addr_pad), addrlen),
> +				base + QUADSPI_LUT(lut_base));
> +		/*
> +		 * For Erase ops - Data and Dummy not required.
> +		 * For Write ops - Dummy not required.
> +		 */
> +
> +		if (ops == FSL_QSPI_OPS_READ) {
> +
> +			/*
> +			 * For cmds SPINOR_OP_READ and SPINOR_OP_READ_4B value
> +			 * of dummy cycles are 0.
> +			 */
> +			if (read_dm)
> +				qspi_writel(q,
> +					    LUT0(DUMMY, pad_count(dummy_pad),
> +					    read_dm) |
> +					    LUT1(FSL_READ, pad_count(data_pad),
> +					    rxfifo),
> +					    base + QUADSPI_LUT(lut_base + 1));
> +			else
> +				qspi_writel(q,
> +					    LUT0(FSL_READ, pad_count(data_pad),
> +					    rxfifo),
> +					    base + QUADSPI_LUT(lut_base + 1));
> +
> +			stop_lut = 2;
> +
> +			/* TODO Add condition to check if READ is IP/AHB. */
> +
> +			/* For AHB read, add seqid in BFGENCR register. */
> +			qspi_writel(q,
> +				    SEQID_LUT1_RUNTIME <<
> +				    QUADSPI_BFGENCR_SEQID_SHIFT,
> +				    q->iobase + QUADSPI_BFGENCR);
> +		}
> +
> +		if (ops == FSL_QSPI_OPS_WRITE) {
> +			qspi_writel(q, LUT0(FSL_WRITE, pad_count(data_pad), 0),
> +					base + QUADSPI_LUT(lut_base + 1));
> +			stop_lut = 2;
> +		}
> +		break;
>  	default:
> -		if (cmd == q->nor[0].erase_opcode)
> -			return SEQID_SE;
> -		dev_err(q->dev, "Unsupported cmd 0x%.2x\n", cmd);
> +		dev_err(q->dev, "Unsupported operation 0x%.2x\n", ops);
>  		break;
>  	}
> -	return -EINVAL;
> +
> +	/* prepare LUT for STOP instruction. */
> +	qspi_writel(q, 0, base +  QUADSPI_LUT(lut_base + stop_lut));
> +
> +	fsl_qspi_lock_lut(q);
>  }

If you express operations in a generic way (with opcode, addr-cycles,
dummy-cycles and data-in/out-cycles), this function would not have to
grow every time a new operation is added to the SPI framework, and more
importantly, you would be able to support SPI NAND operations with
almost no modifications.

Regards,

Boris

[1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=27088
[2]https://github.com/bbrezillon/linux/commit/43cc45764b975bfbb191de3f6a37e073da1a2706
[3]https://github.com/bbrezillon/linux/commit/15782494b0850533e9417ae31954e33e157bbbae#diff-2f804a2dee71d794fa82981b87f9fe0aR1291

Hi Boris,

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Thursday, February 15, 2018 9:03 PM
> To: Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com>
> Cc: linux-mtd@lists.infradead.org; boris.brezillon@free-electrons.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;
> Frieder Schrempf <frieder.schrempf@exceet.de>
> Subject: Re: [PATCH v5 1/2] mtd: fsl-quadspi: add support to create dynamic LUT
> entry
> 
> +Frieder
> 
> Hi Yogesh,
> 
> On Tue, 30 Jan 2018 22:19:29 +0530
> Yogesh Gaur <yogeshnarayan.gaur@nxp.com> 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.
> > This API would fetch required info like opcode, protocol info, dummy
> > info for creating LUT from instance of 'struct spi_nor' and then
> > prepare LUT entry for the required command.
> 
> You're probably already aware that I proposed a new "generic spi-mem API" [1]
> and started to port the fsl-qspi driver [2] to this new interface. While doing that I
> came to the same conclusion: given the small overhead implied by LUT
> preparation, we can prepare OP sequences dynamically instead of hardcoding
> them at probe time.
> 
> I know I'm talking about hypothetical changes while you're providing patches for
> something that already exists and is in use, but maybe it'd be worth discussing
> things a bit so that your changes go in the direction of the spi-mem ones.
> 
Thanks for reviewing the code and design changes.

The new design "generic spi-mem API" proposed is completely orthogonal as per existing framework in which FSL QuadSPI driver is present in MTD subsystem.
Proposed design changes might take time to come into the mainline also this needs to be validated on different controller and on different flashes.

Right now we are focusing to have support of different modes, flashes/vendors on existing framework with current LUT number limitation.
My patch-set is a step towards achieving that by preparing LUT for individual command dynamically instead of hard-coding at probe time.

> >
> > Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> > ---
> > 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 | 310
> > +++++++++++++++++++++-----------------
> >  1 file changed, 168 insertions(+), 142 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c
> > b/drivers/mtd/spi-nor/fsl-quadspi.c
> > index 89306cf..84c341b 100644
> > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > @@ -183,7 +183,7 @@
> >
> >  /* Macros for constructing the LUT register. */
> >  #define LUT0(ins, pad, opr)						\
> > -		(((opr) << OPRND0_SHIFT) | ((LUT_##pad) << PAD0_SHIFT) | \
> > +		(((opr) << OPRND0_SHIFT) | ((pad) << PAD0_SHIFT) | \
> >  		((LUT_##ins) << INSTR0_SHIFT))
> >
> >  #define LUT1(ins, pad, opr)	(LUT0(ins, pad, opr) << OPRND1_SHIFT)
> > @@ -193,21 +193,21 @@
> >  #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
> > +/* LUT0 programmed by bootloader, for run-time create entry for LUT seqid
> 1 */
> > +#define SEQID_LUT0_BOOTLOADER	0
> > +#define SEQID_LUT1_RUNTIME	1
> >
> >  #define QUADSPI_MIN_IOMAP SZ_4M
> >
> > +enum fsl_qspi_ops {
> > +	FSL_QSPI_OPS_READ = 0,
> > +	FSL_QSPI_OPS_WRITE,
> > +	FSL_QSPI_OPS_ERASE,
> > +	FSL_QSPI_OPS_READ_REG,
> > +	FSL_QSPI_OPS_WRITE_REG,
> > +	FSL_QSPI_OPS_WRITE_BUF_REG,
> > +};
> 
> Could we express things in something more extensible? I mean, something
> closer to the spi_mem_op structure I add here [1]. This way the transition to the
> spi-mem interface will be easier.
> 
> > +
> >  enum fsl_qspi_devtype {
> >  	FSL_QUADSPI_VYBRID,
> >  	FSL_QUADSPI_IMX6SX,
> > @@ -368,136 +368,158 @@ 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 entry for the input cmd.
> > + * Protocol info is present in instance of struct spi_nor, using
> > +which fields
> > + * like cmd, data, addrlen along with pad info etc can be parsed.
> > + */
> > +static void fsl_qspi_prepare_lut(struct spi_nor *nor,
> > +				 enum fsl_qspi_ops ops, u8 cmd)
> > +{
> > +	struct fsl_qspi *q = nor->priv;
> >  	void __iomem *base = q->iobase;
> >  	int rxfifo = q->devtype_data->rxfifo;
> > +	int txfifo = q->devtype_data->txfifo;
> >  	u32 lut_base;
> > -	int i;
> > +	u8 cmd_pad, addr_pad, data_pad, dummy_pad;
> > +	enum spi_nor_protocol protocol = 0;
> > +	u8 addrlen = 0;
> > +	u8 read_dm, opcode;
> > +	int stop_lut;
> > +
> > +	read_dm = opcode = cmd_pad = addr_pad = data_pad = dummy_pad =
> 0;
> > +
> > +	switch (ops) {
> > +	case FSL_QSPI_OPS_READ_REG:
> > +	case FSL_QSPI_OPS_WRITE_REG:
> > +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> > +		opcode = cmd;
> > +		protocol = nor->reg_proto;
> > +		break;
> > +	case FSL_QSPI_OPS_READ:
> > +		opcode = cmd;
> > +		read_dm = nor->read_dummy;
> > +		protocol = nor->read_proto;
> > +		break;
> > +	case FSL_QSPI_OPS_WRITE:
> > +		opcode = cmd;
> > +		protocol = nor->write_proto;
> > +		break;
> > +	case FSL_QSPI_OPS_ERASE:
> > +		opcode = cmd;
> > +		break;
> > +	default:
> > +		dev_err(q->dev, "Unsupported operation 0x%.2x\n", ops);
> > +		return;
> > +	}
> > +
> > +	if (protocol) {
> > +		cmd_pad = spi_nor_get_protocol_inst_nbits(protocol);
> > +		addr_pad = spi_nor_get_protocol_addr_nbits(protocol);
> > +		data_pad = spi_nor_get_protocol_data_nbits(protocol);
> > +	}
> >
> > -	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;
> > +	dummy_pad = data_pad;
> > +
> > +	dev_dbg(q->dev, "ops:%x opcode:%x pad[cmd:%d, addr:%d,
> data:%d]\n",
> > +			ops, opcode, cmd_pad, addr_pad, data_pad);
> >
> >  	fsl_qspi_unlock_lut(q);
> >
> > -	/* 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));
> > +	/* Dynamic LUT */
> > +	lut_base = SEQID_LUT1_RUNTIME * 4;
> >
> > -	fsl_qspi_lock_lut(q);
> > -}
> > +	/* default, STOP instruction to be programmed in (lut_base + 1) reg */
> > +	stop_lut = 1;
> > +	switch (ops) {
> > +	case FSL_QSPI_OPS_READ_REG:
> > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> > +			  LUT1(FSL_READ, pad_count(data_pad), rxfifo),
> > +			  base + QUADSPI_LUT(lut_base));
> > +		break;
> > +	case FSL_QSPI_OPS_WRITE_REG:
> > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode),
> > +			  base + QUADSPI_LUT(lut_base));
> > +		break;
> > +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> > +			  LUT1(FSL_WRITE, pad_count(data_pad), txfifo),
> > +			  base + QUADSPI_LUT(lut_base));
> > +		break;
> > +	case FSL_QSPI_OPS_READ:
> > +	case FSL_QSPI_OPS_WRITE:
> > +	case FSL_QSPI_OPS_ERASE:
> > +		/* Common for Read, Write and Erase ops. */
> >
> > -/* 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;
> > +		addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
> > +
> > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad), opcode) |
> > +				LUT1(ADDR, pad_count(addr_pad), addrlen),
> > +				base + QUADSPI_LUT(lut_base));
> > +		/*
> > +		 * For Erase ops - Data and Dummy not required.
> > +		 * For Write ops - Dummy not required.
> > +		 */
> > +
> > +		if (ops == FSL_QSPI_OPS_READ) {
> > +
> > +			/*
> > +			 * For cmds SPINOR_OP_READ and
> SPINOR_OP_READ_4B value
> > +			 * of dummy cycles are 0.
> > +			 */
> > +			if (read_dm)
> > +				qspi_writel(q,
> > +					    LUT0(DUMMY,
> pad_count(dummy_pad),
> > +					    read_dm) |
> > +					    LUT1(FSL_READ,
> pad_count(data_pad),
> > +					    rxfifo),
> > +					    base + QUADSPI_LUT(lut_base + 1));
> > +			else
> > +				qspi_writel(q,
> > +					    LUT0(FSL_READ,
> pad_count(data_pad),
> > +					    rxfifo),
> > +					    base + QUADSPI_LUT(lut_base + 1));
> > +
> > +			stop_lut = 2;
> > +
> > +			/* TODO Add condition to check if READ is IP/AHB. */
> > +
> > +			/* For AHB read, add seqid in BFGENCR register. */
> > +			qspi_writel(q,
> > +				    SEQID_LUT1_RUNTIME <<
> > +				    QUADSPI_BFGENCR_SEQID_SHIFT,
> > +				    q->iobase + QUADSPI_BFGENCR);
> > +		}
> > +
> > +		if (ops == FSL_QSPI_OPS_WRITE) {
> > +			qspi_writel(q, LUT0(FSL_WRITE, pad_count(data_pad),
> 0),
> > +					base + QUADSPI_LUT(lut_base + 1));
> > +			stop_lut = 2;
> > +		}
> > +		break;
> >  	default:
> > -		if (cmd == q->nor[0].erase_opcode)
> > -			return SEQID_SE;
> > -		dev_err(q->dev, "Unsupported cmd 0x%.2x\n", cmd);
> > +		dev_err(q->dev, "Unsupported operation 0x%.2x\n", ops);
> >  		break;
> >  	}
> > -	return -EINVAL;
> > +
> > +	/* prepare LUT for STOP instruction. */
> > +	qspi_writel(q, 0, base +  QUADSPI_LUT(lut_base + stop_lut));
> > +
> > +	fsl_qspi_lock_lut(q);
> >  }
> 
> If you express operations in a generic way (with opcode, addr-cycles, dummy-
> cycles and data-in/out-cycles), this function would not have to grow every time
> a new operation is added to the SPI framework, and more importantly, you
> would be able to support SPI NAND operations with almost no modifications.
>
I have analysis your suggestion and feel that current patch-set is more cleaner.

Let me try to explain:
In current MTD spi-nor framework  for exposed function ptrs i.e. nor->read_reg, nor->write_reg, nor->read, nor->write and nor->erase, our's controller most of the work is common like preparing LUT command and for this fetching required info from struct spi_nor. These are pad info bits [cmd, addr, data and dummy] which are fetched from protocol, opcode, dummy cycles and rx/tx len.

One way is, as suggested by your, to parse all these info in respective function ptr definition and save them in common structure like spi_mem_op and then call prepare_lut only to prepare LUT command using spi_mem_op structure.

Other approach, which I have used is, to parse all these required info from struct spi_nor based on the Switch/Case condition in one common function instead of parsing individually for all exposed func ptr. After this prepare LUT command from fetched information.

Please suggest what approach we can follow.

--
Regards
Yogesh Gaur.

> Regards,
> 
> Boris
> 
> [1]https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatc
> hwork.ozlabs.org%2Fproject%2Flinux-
> mtd%2Flist%2F%3Fseries%3D27088&data=02%7C01%7Cyogeshnarayan.gaur%4
> 0nxp.com%7C189fe6cf2c384934d3d408d5748971d7%7C686ea1d3bc2b4c6fa92
> cd99c5c301635%7C0%7C1%7C636543056039805714&sdata=xIX5FO52%2BJlN3
> cBj8oV6BV3K0wdUxZ%2Bc1b%2F2%2BZ%2B1rgw%3D&reserved=0
> [2]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Fbbrezillon%2Flinux%2Fcommit%2F43cc45764b975bfbb191de3f6a37
> e073da1a2706&data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C189fe6
> cf2c384934d3d408d5748971d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C1%7C636543056039805714&sdata=%2FXVUfidneitX%2Ff0ds0iEIPDLy4ssVh9
> %2ByV8dSX079IM%3D&reserved=0
> [3]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Fbbrezillon%2Flinux%2Fcommit%2F15782494b0850533e9417ae31954
> e33e157bbbae%23diff-
> 2f804a2dee71d794fa82981b87f9fe0aR1291&data=02%7C01%7Cyogeshnarayan
> .gaur%40nxp.com%7C189fe6cf2c384934d3d408d5748971d7%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C1%7C636543056039805714&sdata=Xb1GgQL2
> Nhnsm6kdx3K47OSxnoH%2Bip%2FGG7L5FII3B%2Fs%3D&reserved=0
> 
> --
> Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel
> engineering
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fbootlin.
> com&data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C189fe6cf2c38493
> 4d3d408d5748971d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C6
> 36543056039805714&sdata=TGjVdQ%2BnTmS88L5A0%2FiksrAYJmpNpJGlBQrZY
> vNUSzY%3D&reserved=0

Hi Yogesh,

On Mon, 19 Feb 2018 07:07:50 +0000
Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com> wrote:

> Hi Boris,
> 
> > -----Original Message-----
> > From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> > Sent: Thursday, February 15, 2018 9:03 PM
> > To: Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com>
> > Cc: linux-mtd@lists.infradead.org;
> > boris.brezillon@free-electrons.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; Frieder Schrempf
> > <frieder.schrempf@exceet.de> Subject: Re: [PATCH v5 1/2] mtd:
> > fsl-quadspi: add support to create dynamic LUT entry
> > 
> > +Frieder
> > 
> > Hi Yogesh,
> > 
> > On Tue, 30 Jan 2018 22:19:29 +0530
> > Yogesh Gaur <yogeshnarayan.gaur@nxp.com> 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.
> > > This API would fetch required info like opcode, protocol info,
> > > dummy info for creating LUT from instance of 'struct spi_nor' and
> > > then prepare LUT entry for the required command.  
> > 
> > You're probably already aware that I proposed a new "generic
> > spi-mem API" [1] and started to port the fsl-qspi driver [2] to
> > this new interface. While doing that I came to the same conclusion:
> > given the small overhead implied by LUT preparation, we can prepare
> > OP sequences dynamically instead of hardcoding them at probe time.
> > 
> > I know I'm talking about hypothetical changes while you're
> > providing patches for something that already exists and is in use,
> > but maybe it'd be worth discussing things a bit so that your
> > changes go in the direction of the spi-mem ones. 
> Thanks for reviewing the code and design changes.
> 
> The new design "generic spi-mem API" proposed is completely
> orthogonal as per existing framework in which FSL QuadSPI driver is
> present in MTD subsystem.

Well, it's not completely orthogonal in that the fsl-qspi driver will
have to be adapted to implement the spi-mem interface, and that work is
likely to be done on top of your changes, so converging to a solution
that will be easily adaptable to the new approach is IMO a good thing.

> Proposed design changes might take time to come into the mainline
> also this needs to be validated on different controller and on
> different flashes.

That's true, but note that I'm not asking to make your patch series
depend on the spi-mem stuff, only think about implementing the dynamic
LUT config stuff in a way that makes it easy to then move to the
spi-mem interface.

> 
> Right now we are focusing to have support of different modes,
> flashes/vendors on existing framework with current LUT number
> limitation.

I understand that.

> My patch-set is a step towards achieving that by preparing LUT for
> individual command dynamically instead of hard-coding at probe time.

Yes, and that's also something we need if we transition to spi-mem,
hence the suggestion to implement this feature in a way that simplifies
the conversion to spi-mem.

> > > +/*
> > > + * Prepare LUT entry for the input cmd.
> > > + * Protocol info is present in instance of struct spi_nor, using
> > > +which fields
> > > + * like cmd, data, addrlen along with pad info etc can be parsed.
> > > + */
> > > +static void fsl_qspi_prepare_lut(struct spi_nor *nor,
> > > +				 enum fsl_qspi_ops ops, u8 cmd)
> > > +{
> > > +	struct fsl_qspi *q = nor->priv;
> > >  	void __iomem *base = q->iobase;
> > >  	int rxfifo = q->devtype_data->rxfifo;
> > > +	int txfifo = q->devtype_data->txfifo;
> > >  	u32 lut_base;
> > > -	int i;
> > > +	u8 cmd_pad, addr_pad, data_pad, dummy_pad;
> > > +	enum spi_nor_protocol protocol = 0;
> > > +	u8 addrlen = 0;
> > > +	u8 read_dm, opcode;
> > > +	int stop_lut;
> > > +
> > > +	read_dm = opcode = cmd_pad = addr_pad = data_pad =
> > > dummy_pad =  
> > 0;  
> > > +
> > > +	switch (ops) {
> > > +	case FSL_QSPI_OPS_READ_REG:
> > > +	case FSL_QSPI_OPS_WRITE_REG:
> > > +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> > > +		opcode = cmd;
> > > +		protocol = nor->reg_proto;
> > > +		break;
> > > +	case FSL_QSPI_OPS_READ:
> > > +		opcode = cmd;
> > > +		read_dm = nor->read_dummy;
> > > +		protocol = nor->read_proto;
> > > +		break;
> > > +	case FSL_QSPI_OPS_WRITE:
> > > +		opcode = cmd;
> > > +		protocol = nor->write_proto;
> > > +		break;
> > > +	case FSL_QSPI_OPS_ERASE:
> > > +		opcode = cmd;
> > > +		break;
> > > +	default:
> > > +		dev_err(q->dev, "Unsupported operation
> > > 0x%.2x\n", ops);
> > > +		return;
> > > +	}
> > > +
> > > +	if (protocol) {
> > > +		cmd_pad =
> > > spi_nor_get_protocol_inst_nbits(protocol);
> > > +		addr_pad =
> > > spi_nor_get_protocol_addr_nbits(protocol);
> > > +		data_pad =
> > > spi_nor_get_protocol_data_nbits(protocol);
> > > +	}
> > >
> > > -	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;
> > > +	dummy_pad = data_pad;
> > > +
> > > +	dev_dbg(q->dev, "ops:%x opcode:%x pad[cmd:%d, addr:%d,  
> > data:%d]\n",  
> > > +			ops, opcode, cmd_pad, addr_pad,
> > > data_pad);
> > >
> > >  	fsl_qspi_unlock_lut(q);
> > >
> > > -	/* 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));
> > > +	/* Dynamic LUT */
> > > +	lut_base = SEQID_LUT1_RUNTIME * 4;
> > >
> > > -	fsl_qspi_lock_lut(q);
> > > -}
> > > +	/* default, STOP instruction to be programmed in
> > > (lut_base + 1) reg */
> > > +	stop_lut = 1;
> > > +	switch (ops) {
> > > +	case FSL_QSPI_OPS_READ_REG:
> > > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad),
> > > opcode) |
> > > +			  LUT1(FSL_READ, pad_count(data_pad),
> > > rxfifo),
> > > +			  base + QUADSPI_LUT(lut_base));
> > > +		break;
> > > +	case FSL_QSPI_OPS_WRITE_REG:
> > > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad),
> > > opcode),
> > > +			  base + QUADSPI_LUT(lut_base));
> > > +		break;
> > > +	case FSL_QSPI_OPS_WRITE_BUF_REG:
> > > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad),
> > > opcode) |
> > > +			  LUT1(FSL_WRITE, pad_count(data_pad),
> > > txfifo),
> > > +			  base + QUADSPI_LUT(lut_base));
> > > +		break;
> > > +	case FSL_QSPI_OPS_READ:
> > > +	case FSL_QSPI_OPS_WRITE:
> > > +	case FSL_QSPI_OPS_ERASE:
> > > +		/* Common for Read, Write and Erase ops. */
> > >
> > > -/* 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;
> > > +		addrlen = (nor->addr_width == 3) ? ADDR24BIT :
> > > ADDR32BIT; +
> > > +		qspi_writel(q, LUT0(CMD, pad_count(cmd_pad),
> > > opcode) |
> > > +				LUT1(ADDR, pad_count(addr_pad),
> > > addrlen),
> > > +				base + QUADSPI_LUT(lut_base));
> > > +		/*
> > > +		 * For Erase ops - Data and Dummy not required.
> > > +		 * For Write ops - Dummy not required.
> > > +		 */
> > > +
> > > +		if (ops == FSL_QSPI_OPS_READ) {
> > > +
> > > +			/*
> > > +			 * For cmds SPINOR_OP_READ and  
> > SPINOR_OP_READ_4B value  
> > > +			 * of dummy cycles are 0.
> > > +			 */
> > > +			if (read_dm)
> > > +				qspi_writel(q,
> > > +					    LUT0(DUMMY,  
> > pad_count(dummy_pad),  
> > > +					    read_dm) |
> > > +					    LUT1(FSL_READ,  
> > pad_count(data_pad),  
> > > +					    rxfifo),
> > > +					    base +
> > > QUADSPI_LUT(lut_base + 1));
> > > +			else
> > > +				qspi_writel(q,
> > > +					    LUT0(FSL_READ,  
> > pad_count(data_pad),  
> > > +					    rxfifo),
> > > +					    base +
> > > QUADSPI_LUT(lut_base + 1)); +
> > > +			stop_lut = 2;
> > > +
> > > +			/* TODO Add condition to check if READ
> > > is IP/AHB. */ +
> > > +			/* For AHB read, add seqid in BFGENCR
> > > register. */
> > > +			qspi_writel(q,
> > > +				    SEQID_LUT1_RUNTIME <<
> > > +				    QUADSPI_BFGENCR_SEQID_SHIFT,
> > > +				    q->iobase + QUADSPI_BFGENCR);
> > > +		}
> > > +
> > > +		if (ops == FSL_QSPI_OPS_WRITE) {
> > > +			qspi_writel(q, LUT0(FSL_WRITE,
> > > pad_count(data_pad),  
> > 0),  
> > > +					base +
> > > QUADSPI_LUT(lut_base + 1));
> > > +			stop_lut = 2;
> > > +		}
> > > +		break;
> > >  	default:
> > > -		if (cmd == q->nor[0].erase_opcode)
> > > -			return SEQID_SE;
> > > -		dev_err(q->dev, "Unsupported cmd 0x%.2x\n", cmd);
> > > +		dev_err(q->dev, "Unsupported operation
> > > 0x%.2x\n", ops); break;
> > >  	}
> > > -	return -EINVAL;
> > > +
> > > +	/* prepare LUT for STOP instruction. */
> > > +	qspi_writel(q, 0, base +  QUADSPI_LUT(lut_base +
> > > stop_lut)); +
> > > +	fsl_qspi_lock_lut(q);
> > >  }  
> > 
> > If you express operations in a generic way (with opcode,
> > addr-cycles, dummy- cycles and data-in/out-cycles), this function
> > would not have to grow every time a new operation is added to the
> > SPI framework, and more importantly, you would be able to support
> > SPI NAND operations with almost no modifications. 
> I have analysis your suggestion and feel that current patch-set is
> more cleaner.
> 
> Let me try to explain:
> In current MTD spi-nor framework  for exposed function ptrs i.e.
> nor->read_reg, nor->write_reg, nor->read, nor->write and nor->erase,
> our's controller most of the work is common like preparing LUT
> command and for this fetching required info from struct spi_nor.
> These are pad info bits [cmd, addr, data and dummy] which are fetched
> from protocol, opcode, dummy cycles and rx/tx len.
> 
> One way is, as suggested by your, to parse all these info in
> respective function ptr definition and save them in common structure
> like spi_mem_op and then call prepare_lut only to prepare LUT command
> using spi_mem_op structure.

I clearly prefer this approach, but I guess you already know that :-).

> 
> Other approach, which I have used is, to parse all these required
> info from struct spi_nor based on the Switch/Case condition in one
> common function instead of parsing individually for all exposed func
> ptr. After this prepare LUT command from fetched information.
> 
> Please suggest what approach we can follow.

I already did, but I guess you're waiting for SPI NOR maintainers'
opinion here.

Regards,

Boris