[2/4] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

According to JESD216C (JEDEC Basic Flash Parameter Table 18th DWORD)
Octal DTR(8D-8D-8D) command and command extension (00b: same, 01b: inverse)
to add extension command mode in spi_nor struct and to add write_cr2
(Write CFG Reg 2) for 8-8-8/8D-8D-8D mode sequences enable.

Define the relevant macrons and enum to add such modes and make sure
op->xxx.dtr fields, command nbytes and extension command are properly
filled and unmask DTR and X-X-X modes in spi_nor_spimem_adjust_hwcaps()
so that DTR and X-X-X support detection is done through
spi_mem_supports_op().

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 drivers/mtd/spi-nor/spi-nor.c | 159 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/mtd/spi-nor.h   |  58 +++++++++++++--
 2 files changed, 206 insertions(+), 11 deletions(-)

Hi Mason,

On 15/11/19 2:28 pm, Mason Yang wrote:
> According to JESD216C (JEDEC Basic Flash Parameter Table 18th DWORD)
> Octal DTR(8D-8D-8D) command and command extension (00b: same, 01b: inverse)
> to add extension command mode in spi_nor struct and to add write_cr2
> (Write CFG Reg 2) for 8-8-8/8D-8D-8D mode sequences enable.
> 

But I don't see any code setting "nor->ext_cmd_mode" based on BFPT?

Any new feature that we add to spi-nor should make use of autodiscovery
feature made possible by SFDP tables. Could you modify the patch to
discover capabilities supported by flash and opcodes to be used from
SFDP table?


> Define the relevant macrons and enum to add such modes and make sure
> op->xxx.dtr fields, command nbytes and extension command are properly
> filled and unmask DTR and X-X-X modes in spi_nor_spimem_adjust_hwcaps()
> so that DTR and X-X-X support detection is done through
> spi_mem_supports_op().
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 159 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/mtd/spi-nor.h   |  58 +++++++++++++--
>  2 files changed, 206 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 7acf4a9..4cdf52d 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -320,6 +320,23 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
>  	/* convert the dummy cycles to the number of bytes */
>  	op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
>  
> +	if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
> +		op.cmd.nbytes = 2;

Can we get this info from SFDP too?

> +
> +		if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> +			op.cmd.ext_opcode = ~nor->read_opcode;
> +		else
> +			op.cmd.ext_opcode = nor->read_opcode;
> +
> +		if (spi_nor_protocol_is_8D_8D_8D(nor->read_proto)) {
> +			op.dummy.nbytes *= 2;

> +			op.cmd.dtr = true;
> +			op.addr.dtr = true;
> +			op.dummy.dtr = true;
> +			op.data.dtr = true;

Can we have a macro for this initialization?


> +		}
> +	}
> +
>  	return spi_nor_spimem_xfer_data(nor, &op);
>  }
>  
> @@ -367,6 +384,21 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
>  	if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
>  		op.addr.nbytes = 0;
>  
> +	if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
> +		op.cmd.nbytes = 2;
> +
> +		if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> +			op.cmd.ext_opcode = ~nor->program_opcode;
> +		else
> +			op.cmd.ext_opcode = nor->program_opcode;
> +
> +		if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto)) {
> +			op.cmd.dtr = true;
> +			op.addr.dtr = true;
> +			op.data.dtr = true;
> +		}
> +	}
> +
>  	return spi_nor_spimem_xfer_data(nor, &op);
>  }
>  
> @@ -404,6 +436,30 @@ static int read_sr(struct spi_nor *nor)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
>  

This is not based on the latest tree.

> +		if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
> +			op.cmd.buswidth = 8;
> +			op.addr.buswidth = 8;
> +			op.dummy.buswidth = 8;
> +			op.data.buswidth = 8;
> +			op.cmd.nbytes = 2;
> +			op.addr.nbytes = 4;
> +			op.dummy.nbytes = 4;
> +			op.addr.val = 0;

This is not scalable... There will be bunch of if...else ladders when we
want to support other X-X-X modes... Can't these be derived from
nor->reg_proto? And then borrow the logic from spi_nor_spimem_read_data()?


Could you have a look at Boris's initial submission to add 8-8-8 mode at
https://patchwork.kernel.org/cover/10638055/ ?
You could use that series as the base for your changes/additions.

I am fine if you want to call 2nd byte of opcode as ext_opcode

Regards
Vignesh

> +
> +			if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> +				op.cmd.ext_opcode = ~SPINOR_OP_RDSR;
> +			else
> +				op.cmd.ext_opcode = SPINOR_OP_RDSR;
> +
> +			if (spi_nor_protocol_is_8D_8D_8D(nor->read_proto)) {
> +				op.dummy.nbytes *= 2;
> +				op.cmd.dtr = true;
> +				op.addr.dtr = true;
> +				op.dummy.dtr = true;
> +				op.data.dtr = true;
> +			}
> +		}
> +
>  		ret = spi_mem_exec_op(nor->spimem, &op);
>  	} else {
>  		ret = nor->read_reg(nor, SPINOR_OP_RDSR, nor->bouncebuf, 1);
> @@ -508,6 +564,19 @@ static int write_enable(struct spi_nor *nor)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_NO_DATA);
>  
> +		if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
> +			op.cmd.buswidth = 8;
> +			op.cmd.nbytes = 2;
> +
> +			if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto))
> +				op.cmd.dtr = true;
> +
> +			if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> +				op.cmd.ext_opcode = ~SPINOR_OP_WREN;
> +			else
> +				op.cmd.ext_opcode = SPINOR_OP_WREN;
> +		}
> +
>  		return spi_mem_exec_op(nor->spimem, &op);
>  	}
>  
> @@ -526,12 +595,65 @@ static int write_disable(struct spi_nor *nor)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_NO_DATA);
>  
> +		if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
> +			op.cmd.buswidth = 8;
> +			op.cmd.nbytes = 2;
> +
> +			if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto))
> +				op.cmd.dtr = true;
> +
> +			if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> +				op.cmd.ext_opcode = ~SPINOR_OP_WRDI;
> +			else
> +				op.cmd.ext_opcode = SPINOR_OP_WRDI;
> +		}
> +
>  		return spi_mem_exec_op(nor->spimem, &op);
>  	}
>  
>  	return nor->write_reg(nor, SPINOR_OP_WRDI, NULL, 0);
>  }
>  
> +/*
> + * Write configuration register 2 one byte
> + * Returns negative if error occurred.
> + */
> +static int write_cr2(struct spi_nor *nor, u32 addr, u8 val)
> +{
> +	write_enable(nor);
> +
> +	nor->bouncebuf[0] = val;
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRCR2, 1),
> +				   SPI_MEM_OP_ADDR(4, addr, 1),
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1));
> +
> +		if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
> +			op.cmd.buswidth = 8;
> +			op.addr.buswidth = 8;
> +			op.data.buswidth = 8;
> +			op.cmd.nbytes = 2;
> +
> +			if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto)) {
> +				op.cmd.dtr = true;
> +				op.addr.dtr = true;
> +				op.data.dtr = true;
> +			}
> +
> +			if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> +				op.cmd.ext_opcode = ~SPINOR_OP_WRCR2;
> +			else
> +				op.cmd.ext_opcode = SPINOR_OP_WRCR2;
> +		}
> +
> +		return spi_mem_exec_op(nor->spimem, &op);
> +	}
> +
> +	return -ENOTSUPP;
> +}
> +
>  static struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  {
>  	return mtd->priv;
> @@ -868,6 +990,19 @@ static int erase_chip(struct spi_nor *nor)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_NO_DATA);
>  
> +		if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
> +			op.cmd.buswidth = 8;
> +			op.cmd.nbytes = 2;
> +
> +			if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto))
> +				op.cmd.dtr = true;
> +
> +			if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> +				op.cmd.ext_opcode = ~SPINOR_OP_CHIP_ERASE;
> +			else
> +				op.cmd.ext_opcode = SPINOR_OP_CHIP_ERASE;
> +		}
> +
>  		return spi_mem_exec_op(nor->spimem, &op);
>  	}
>  
> @@ -945,6 +1080,22 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_NO_DATA);
>  
> +		if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
> +			op.cmd.buswidth = 8;
> +			op.addr.buswidth = 8;
> +			op.cmd.nbytes = 2;
> +
> +			if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto)) {
> +				op.cmd.dtr = true;
> +				op.addr.dtr = true;
> +			}
> +
> +			if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> +				op.cmd.ext_opcode = ~nor->erase_opcode;
> +			else
> +				op.cmd.ext_opcode = nor->erase_opcode;
> +		}
> +
>  		return spi_mem_exec_op(nor->spimem, &op);
>  	}
>  
> @@ -2825,6 +2976,7 @@ static int spi_nor_hwcaps_read2cmd(u32 hwcaps)
>  		{ SNOR_HWCAPS_READ_1_8_8,	SNOR_CMD_READ_1_8_8 },
>  		{ SNOR_HWCAPS_READ_8_8_8,	SNOR_CMD_READ_8_8_8 },
>  		{ SNOR_HWCAPS_READ_1_8_8_DTR,	SNOR_CMD_READ_1_8_8_DTR },
> +		{ SNOR_HWCAPS_READ_8_8_8_DTR,	SNOR_CMD_READ_8D_8D_8D },
>  	};
>  
>  	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd,
> @@ -2841,6 +2993,7 @@ static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
>  		{ SNOR_HWCAPS_PP_1_1_8,		SNOR_CMD_PP_1_1_8 },
>  		{ SNOR_HWCAPS_PP_1_8_8,		SNOR_CMD_PP_1_8_8 },
>  		{ SNOR_HWCAPS_PP_8_8_8,		SNOR_CMD_PP_8_8_8 },
> +		{ SNOR_HWCAPS_PP_8_8_8_DTR,	SNOR_CMD_PP_8D_8D_8D },
>  	};
>  
>  	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd,
> @@ -3010,12 +3163,6 @@ static int spi_nor_spimem_check_pp(struct spi_nor *nor,
>  	struct spi_nor_flash_parameter *params =  &nor->params;
>  	unsigned int cap;
>  
> -	/* DTR modes are not supported yet, mask them all. */
> -	*hwcaps &= ~SNOR_HWCAPS_DTR;
> -
> -	/* X-X-X modes are not supported yet, mask them all. */
> -	*hwcaps &= ~SNOR_HWCAPS_X_X_X;
> -
>  	for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
>  		int rdidx, ppidx;
>  
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index fc0b4b1..2e720ca 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -84,6 +84,9 @@
>  #define SPINOR_OP_BE_4K_4B	0x21	/* Erase 4KiB block */
>  #define SPINOR_OP_BE_32K_4B	0x5c	/* Erase 32KiB block */
>  #define SPINOR_OP_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
> +#define SPINOR_OP_READ_8_8_8	SPINOR_OP_READ_1_4_4_4B
> +#define SPINOR_OP_PP_8_8_8	SPINOR_OP_PP_4B
> +#define SPINOR_OP_PP_8D_8D_8D	SPINOR_OP_PP_4B
>  
>  /* Double Transfer Rate opcodes - defined in JEDEC JESD216B. */
>  #define SPINOR_OP_READ_1_1_1_DTR	0x0d
> @@ -93,6 +96,7 @@
>  #define SPINOR_OP_READ_1_1_1_DTR_4B	0x0e
>  #define SPINOR_OP_READ_1_2_2_DTR_4B	0xbe
>  #define SPINOR_OP_READ_1_4_4_DTR_4B	0xee
> +#define SPINOR_OP_READ_8D_8D_8D		SPINOR_OP_READ_1_4_4_DTR_4B
>  
>  /* Used for SST flashes only. */
>  #define SPINOR_OP_BP		0x02	/* Byte program */
> @@ -107,6 +111,8 @@
>  #define XSR_PAGESIZE		BIT(0)	/* Page size in Po2 or Linear */
>  #define XSR_RDY			BIT(7)	/* Ready */
>  
> +/* Write CFG Reg 2 - defined in JEDEC JESD216C. */
> +#define SPINOR_OP_WRCR2		0x72	/* Write configuration register 2 */
>  
>  /* Used for Macronix and Winbond flashes. */
>  #define SPINOR_OP_EN4B		0xb7	/* Enter 4-byte mode */
> @@ -150,6 +156,13 @@
>  /* Status Register 2 bits. */
>  #define SR2_QUAD_EN_BIT7	BIT(7)
>  
> +/* Configuration register 2, offset 0 */
> +#define CR2_REG0		0x0
> +#define CR2_REG0_MODE_MASK	GENMASK(1, 0)
> +#define CR2_REG0_MODE_SPI	0
> +#define CR2_REG0_MODE_OPI_STR	1
> +#define CR2_REG0_MODE_OPI_DTR	2
> +
>  /* Supported SPI protocols */
>  #define SNOR_PROTO_INST_MASK	GENMASK(23, 16)
>  #define SNOR_PROTO_INST_SHIFT	16
> @@ -170,6 +183,7 @@
>  	 SNOR_PROTO_DATA_MASK)
>  
>  #define SNOR_PROTO_IS_DTR	BIT(24)	/* Double Transfer Rate */
> +#define SNOR_PROTO_IS_8D_8D_8D	BIT(25) /* Full Octal DTR */
>  
>  #define SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits)	\
>  	(SNOR_PROTO_INST(_inst_nbits) |				\
> @@ -179,6 +193,10 @@
>  	(SNOR_PROTO_IS_DTR |					\
>  	 SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits))
>  
> +#define SNOR_PROTO_8D_8D_8D(_nbits)				\
> +	(SNOR_PROTO_IS_8D_8D_8D |				\
> +	 SNOR_PROTO_STR(_nbits, _nbits, _nbits))
> +
>  enum spi_nor_protocol {
>  	SNOR_PROTO_1_1_1 = SNOR_PROTO_STR(1, 1, 1),
>  	SNOR_PROTO_1_1_2 = SNOR_PROTO_STR(1, 1, 2),
> @@ -195,6 +213,7 @@ enum spi_nor_protocol {
>  	SNOR_PROTO_1_2_2_DTR = SNOR_PROTO_DTR(1, 2, 2),
>  	SNOR_PROTO_1_4_4_DTR = SNOR_PROTO_DTR(1, 4, 4),
>  	SNOR_PROTO_1_8_8_DTR = SNOR_PROTO_DTR(1, 8, 8),
> +	SNOR_PROTO_8_8_8_DTR = SNOR_PROTO_8D_8D_8D(8),
>  };
>  
>  static inline bool spi_nor_protocol_is_dtr(enum spi_nor_protocol proto)
> @@ -202,6 +221,16 @@ static inline bool spi_nor_protocol_is_dtr(enum spi_nor_protocol proto)
>  	return !!(proto & SNOR_PROTO_IS_DTR);
>  }
>  
> +static inline bool spi_nor_protocol_is_8_8_8(enum spi_nor_protocol proto)
> +{
> +	return !!(proto & SNOR_PROTO_8_8_8);
> +}
> +
> +static inline bool spi_nor_protocol_is_8D_8D_8D(enum spi_nor_protocol proto)
> +{
> +	return !!(proto & SNOR_PROTO_IS_8D_8D_8D);
> +}
> +
>  static inline u8 spi_nor_get_protocol_inst_nbits(enum spi_nor_protocol proto)
>  {
>  	return ((unsigned long)(proto & SNOR_PROTO_INST_MASK)) >>
> @@ -349,7 +378,7 @@ struct spi_nor_hwcaps {
>   * then Quad SPI protocols before Dual SPI protocols, Fast Read and lastly
>   * (Slow) Read.
>   */
> -#define SNOR_HWCAPS_READ_MASK		GENMASK(14, 0)
> +#define SNOR_HWCAPS_READ_MASK		GENMASK(15, 0)
>  #define SNOR_HWCAPS_READ		BIT(0)
>  #define SNOR_HWCAPS_READ_FAST		BIT(1)
>  #define SNOR_HWCAPS_READ_1_1_1_DTR	BIT(2)
> @@ -366,11 +395,12 @@ struct spi_nor_hwcaps {
>  #define SNOR_HWCAPS_READ_4_4_4		BIT(9)
>  #define SNOR_HWCAPS_READ_1_4_4_DTR	BIT(10)
>  
> -#define SNOR_HWCAPS_READ_OCTAL		GENMASK(14, 11)
> +#define SNOR_HWCAPS_READ_OCTAL		GENMASK(15, 11)
>  #define SNOR_HWCAPS_READ_1_1_8		BIT(11)
>  #define SNOR_HWCAPS_READ_1_8_8		BIT(12)
>  #define SNOR_HWCAPS_READ_8_8_8		BIT(13)
>  #define SNOR_HWCAPS_READ_1_8_8_DTR	BIT(14)
> +#define SNOR_HWCAPS_READ_8_8_8_DTR	BIT(15)
>  
>  /*
>   * Page Program capabilities.
> @@ -381,7 +411,7 @@ struct spi_nor_hwcaps {
>   * JEDEC/SFDP standard to define them. Also at this moment no SPI flash memory
>   * implements such commands.
>   */
> -#define SNOR_HWCAPS_PP_MASK	GENMASK(22, 16)
> +#define SNOR_HWCAPS_PP_MASK	GENMASK(23, 16)
>  #define SNOR_HWCAPS_PP		BIT(16)
>  
>  #define SNOR_HWCAPS_PP_QUAD	GENMASK(19, 17)
> @@ -389,10 +419,17 @@ struct spi_nor_hwcaps {
>  #define SNOR_HWCAPS_PP_1_4_4	BIT(18)
>  #define SNOR_HWCAPS_PP_4_4_4	BIT(19)
>  
> -#define SNOR_HWCAPS_PP_OCTAL	GENMASK(22, 20)
> +#define SNOR_HWCAPS_PP_OCTAL	GENMASK(23, 20)
>  #define SNOR_HWCAPS_PP_1_1_8	BIT(20)
>  #define SNOR_HWCAPS_PP_1_8_8	BIT(21)
>  #define SNOR_HWCAPS_PP_8_8_8	BIT(22)
> +#define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
> +
> +#define SNOR_HWCAPS_OPI_FULL_STR	(SNOR_HWCAPS_READ_8_8_8 | \
> +					 SNOR_HWCAPS_PP_8_8_8)
> +
> +#define SNOR_HWCAPS_OPI_FULL_DTR	(SNOR_HWCAPS_READ_8_8_8_DTR | \
> +					 SNOR_HWCAPS_PP_8_8_8_DTR)
>  
>  #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
>  				 SNOR_HWCAPS_READ_4_4_4 |	\
> @@ -403,7 +440,9 @@ struct spi_nor_hwcaps {
>  #define SNOR_HWCAPS_DTR		(SNOR_HWCAPS_READ_1_1_1_DTR |	\
>  				 SNOR_HWCAPS_READ_1_2_2_DTR |	\
>  				 SNOR_HWCAPS_READ_1_4_4_DTR |	\
> -				 SNOR_HWCAPS_READ_1_8_8_DTR)
> +				 SNOR_HWCAPS_READ_1_8_8_DTR |	\
> +				 SNOR_HWCAPS_READ_8_8_8_DTR |	\
> +				 SNOR_HWCAPS_PP_8_8_8_DTR)
>  
>  #define SNOR_HWCAPS_ALL		(SNOR_HWCAPS_READ_MASK |	\
>  				 SNOR_HWCAPS_PP_MASK)
> @@ -442,6 +481,7 @@ enum spi_nor_read_command_index {
>  	SNOR_CMD_READ_1_8_8,
>  	SNOR_CMD_READ_8_8_8,
>  	SNOR_CMD_READ_1_8_8_DTR,
> +	SNOR_CMD_READ_8D_8D_8D,
>  
>  	SNOR_CMD_READ_MAX
>  };
> @@ -458,6 +498,7 @@ enum spi_nor_pp_command_index {
>  	SNOR_CMD_PP_1_1_8,
>  	SNOR_CMD_PP_1_8_8,
>  	SNOR_CMD_PP_8_8_8,
> +	SNOR_CMD_PP_8D_8D_8D,
>  
>  	SNOR_CMD_PP_MAX
>  };
> @@ -528,6 +569,11 @@ struct spi_nor_flash_parameter {
>   */
>  struct flash_info;
>  
> +enum extension_cmd_mode {
> +	EXT_CMD_IS_CMD,
> +	EXT_CMD_IS_INVERSE,
> +};
> +
>  /**
>   * struct spi_nor - Structure for defining a the SPI NOR layer
>   * @mtd:		point to a mtd_info structure
> @@ -537,6 +583,7 @@ struct spi_nor_flash_parameter {
>   * @bouncebuf:		bounce buffer used when the buffer passed by the MTD
>   *                      layer is not DMA-able
>   * @bouncebuf_size:	size of the bounce buffer
> + * @ext_cmd_mode:	extension command mode, 0: same, 1: inverse
>   * @info:		spi-nor part JDEC MFR id and other info
>   * @page_size:		the page size of the SPI NOR
>   * @addr_width:		number of address bytes
> @@ -575,6 +622,7 @@ struct spi_nor {
>  	struct spi_mem		*spimem;
>  	u8			*bouncebuf;
>  	size_t			bouncebuf_size;
> +	enum extension_cmd_mode	ext_cmd_mode;
>  	const struct flash_info	*info;
>  	u32			page_size;
>  	u8			addr_width;
>

Hi Vignesh,

> 
> On 15/11/19 2:28 pm, Mason Yang wrote:
> > According to JESD216C (JEDEC Basic Flash Parameter Table 18th DWORD)
> > Octal DTR(8D-8D-8D) command and command extension (00b: same, 01b: 
inverse)
> > to add extension command mode in spi_nor struct and to add write_cr2
> > (Write CFG Reg 2) for 8-8-8/8D-8D-8D mode sequences enable.
> > 
> 
> But I don't see any code setting "nor->ext_cmd_mode" based on BFPT?
> 
> Any new feature that we add to spi-nor should make use of autodiscovery
> feature made possible by SFDP tables. Could you modify the patch to
> discover capabilities supported by flash and opcodes to be used from
> SFDP table?

Got it but our device will return a empty SFDP table.

> 
> 
> > Define the relevant macrons and enum to add such modes and make sure
> > op->xxx.dtr fields, command nbytes and extension command are properly
> > filled and unmask DTR and X-X-X modes in 
spi_nor_spimem_adjust_hwcaps()
> > so that DTR and X-X-X support detection is done through
> > spi_mem_supports_op().
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 159 
++++++++++++++++++++++++++++++++++++++++--
> >  include/linux/mtd/spi-nor.h   |  58 +++++++++++++--
> >  2 files changed, 206 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c 
b/drivers/mtd/spi-nor/spi-nor.c
> > index 7acf4a9..4cdf52d 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -320,6 +320,23 @@ static ssize_t spi_nor_spimem_read_data(struct 
spi_nor 
> *nor, loff_t from,
> >     /* convert the dummy cycles to the number of bytes */
> >     op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> > 
> > +   if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
> > +      op.cmd.nbytes = 2;
> 
> Can we get this info from SFDP too?
> 
> > +
> > +      if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> > +         op.cmd.ext_opcode = ~nor->read_opcode;
> > +      else
> > +         op.cmd.ext_opcode = nor->read_opcode;
> > +
> > +      if (spi_nor_protocol_is_8D_8D_8D(nor->read_proto)) {
> > +         op.dummy.nbytes *= 2;
> 
> > +         op.cmd.dtr = true;
> > +         op.addr.dtr = true;
> > +         op.dummy.dtr = true;
> > +         op.data.dtr = true;
> 
> Can we have a macro for this initialization?

okay, will fix it.

> 
> 
> > +      }
> > +   }
> > +
> >     return spi_nor_spimem_xfer_data(nor, &op);
> >  }
> > 
> > @@ -367,6 +384,21 @@ static ssize_t spi_nor_spimem_write_data(struct 
spi_nor
> *nor, loff_t to,
> >     if (nor->program_opcode == SPINOR_OP_AAI_WP && 
nor->sst_write_second)
> >        op.addr.nbytes = 0;
> > 
> > +   if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
> > +      op.cmd.nbytes = 2;
> > +
> > +      if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE)
> > +         op.cmd.ext_opcode = ~nor->program_opcode;
> > +      else
> > +         op.cmd.ext_opcode = nor->program_opcode;
> > +
> > +      if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto)) {
> > +         op.cmd.dtr = true;
> > +         op.addr.dtr = true;
> > +         op.data.dtr = true;
> > +      }
> > +   }
> > +
> >     return spi_nor_spimem_xfer_data(nor, &op);
> >  }
> > 
> > @@ -404,6 +436,30 @@ static int read_sr(struct spi_nor *nor)
> >                 SPI_MEM_OP_NO_DUMMY,
> >                 SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
> > 
> 
> This is not based on the latest tree.
> 
> > +      if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
> > +         op.cmd.buswidth = 8;
> > +         op.addr.buswidth = 8;
> > +         op.dummy.buswidth = 8;
> > +         op.data.buswidth = 8;
> > +         op.cmd.nbytes = 2;
> > +         op.addr.nbytes = 4;
> > +         op.dummy.nbytes = 4;
> > +         op.addr.val = 0;
> 
> This is not scalable... There will be bunch of if...else ladders when we
> want to support other X-X-X modes... Can't these be derived from
> nor->reg_proto? And then borrow the logic from 
spi_nor_spimem_read_data()?
> 

Got it !

> 
> Could you have a look at Boris's initial submission to add 8-8-8 mode at
> https://patchwork.kernel.org/cover/10638055/ ?
> You could use that series as the base for your changes/additions.

Got it.
My idea is to support 8D-8D-8D mode with a minimum patches because 
there is no define for 1D-1D-1D, 2D-2D-2D and 4D-4D-4D mode in JEDEC 
if I am right.

> 
> I am fine if you want to call 2nd byte of opcode as ext_opcode
> 
> Regards
> Vignesh
> 

thanks for your time & comments.
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

Hi,

On 09/12/19 1:26 pm, masonccyang@mxic.com.tw wrote:
> 
> Hi Vignesh,
> 
>>
>> On 15/11/19 2:28 pm, Mason Yang wrote:
>>> According to JESD216C (JEDEC Basic Flash Parameter Table 18th DWORD)
>>> Octal DTR(8D-8D-8D) command and command extension (00b: same, 01b: 
> inverse)
>>> to add extension command mode in spi_nor struct and to add write_cr2
>>> (Write CFG Reg 2) for 8-8-8/8D-8D-8D mode sequences enable.
>>>
>>
>> But I don't see any code setting "nor->ext_cmd_mode" based on BFPT?
>>
>> Any new feature that we add to spi-nor should make use of autodiscovery
>> feature made possible by SFDP tables. Could you modify the patch to
>> discover capabilities supported by flash and opcodes to be used from
>> SFDP table?
> 
> Got it but our device will return a empty SFDP table.
> 

If flash you tested on does not support JEDEC 216C then don't mention
about it. Above commit message gives an impression that flash in JEDEC
216C compliant.

>>
>>
>>> Define the relevant macrons and enum to add such modes and make sure
>>> op->xxx.dtr fields, command nbytes and extension command are properly
>>> filled and unmask DTR and X-X-X modes in 
> spi_nor_spimem_adjust_hwcaps()
>>> so that DTR and X-X-X support detection is done through
>>> spi_mem_supports_op().
>>>
[...]
>>> @@ -404,6 +436,30 @@ static int read_sr(struct spi_nor *nor)
>>>                 SPI_MEM_OP_NO_DUMMY,
>>>                 SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
>>>
>>
>> This is not based on the latest tree.
>>
>>> +      if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
>>> +         op.cmd.buswidth = 8;
>>> +         op.addr.buswidth = 8;
>>> +         op.dummy.buswidth = 8;
>>> +         op.data.buswidth = 8;
>>> +         op.cmd.nbytes = 2;
>>> +         op.addr.nbytes = 4;
>>> +         op.dummy.nbytes = 4;
>>> +         op.addr.val = 0;
>>
>> This is not scalable... There will be bunch of if...else ladders when we
>> want to support other X-X-X modes... Can't these be derived from
>> nor->reg_proto? And then borrow the logic from 
> spi_nor_spimem_read_data()?
>>
> 
> Got it !
> 
>>
>> Could you have a look at Boris's initial submission to add 8-8-8 mode at
>> https://patchwork.kernel.org/cover/10638055/ ?
>> You could use that series as the base for your changes/additions.
> 
> Got it.
> My idea is to support 8D-8D-8D mode with a minimum patches because 
> there is no define for 1D-1D-1D, 2D-2D-2D and 4D-4D-4D mode in JEDEC 
> if I am right.
> 

JESD251-A1 does talk about 4S-4D-4D right? Also none of the JEDEC
standards prohibit flash vendors from supporting other X-X-X modes.

I think you haven't thought about bigger picture here. Flash devices
that support other mode exist today and we would need the framework to
be built such that these modes can be added in future.

I suggest you start with Boris's series [1] as base and port it to
latest kernel. Isn't that series alone enough to support Macronix Octal
flashes at least?
If required, you could also always include additional patches adding new
features.

[1] https://patchwork.kernel.org/cover/10638055/

Hi Vignesh,


> >> On 15/11/19 2:28 pm, Mason Yang wrote:
> >>> According to JESD216C (JEDEC Basic Flash Parameter Table 18th DWORD)
> >>> Octal DTR(8D-8D-8D) command and command extension (00b: same, 01b: 
> > inverse)
> >>> to add extension command mode in spi_nor struct and to add write_cr2
> >>> (Write CFG Reg 2) for 8-8-8/8D-8D-8D mode sequences enable.
> >>>
> >>
> >> But I don't see any code setting "nor->ext_cmd_mode" based on BFPT?
> >>
> >> Any new feature that we add to spi-nor should make use of 
autodiscovery
> >> feature made possible by SFDP tables. Could you modify the patch to
> >> discover capabilities supported by flash and opcodes to be used from
> >> SFDP table?
> > 
> > Got it but our device will return a empty SFDP table.
> > 
> 
> If flash you tested on does not support JEDEC 216C then don't mention
> about it. Above commit message gives an impression that flash in JEDEC
> 216C compliant.
> 

okay, got it.

> >>
> >>
> >>> Define the relevant macrons and enum to add such modes and make sure
> >>> op->xxx.dtr fields, command nbytes and extension command are 
properly
> >>> filled and unmask DTR and X-X-X modes in 
> > spi_nor_spimem_adjust_hwcaps()
> >>> so that DTR and X-X-X support detection is done through
> >>> spi_mem_supports_op().
> >>>
> [...]
> >>> @@ -404,6 +436,30 @@ static int read_sr(struct spi_nor *nor)
> >>>                 SPI_MEM_OP_NO_DUMMY,
> >>>                 SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
> >>>
> >>
> >> This is not based on the latest tree.
> >>
> >>> +      if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
> >>> +         op.cmd.buswidth = 8;
> >>> +         op.addr.buswidth = 8;
> >>> +         op.dummy.buswidth = 8;
> >>> +         op.data.buswidth = 8;
> >>> +         op.cmd.nbytes = 2;
> >>> +         op.addr.nbytes = 4;
> >>> +         op.dummy.nbytes = 4;
> >>> +         op.addr.val = 0;
> >>
> >> This is not scalable... There will be bunch of if...else ladders when 
we
> >> want to support other X-X-X modes... Can't these be derived from
> >> nor->reg_proto? And then borrow the logic from 
> > spi_nor_spimem_read_data()?
> >>
> > 
> > Got it !
> > 
> >>
> >> Could you have a look at Boris's initial submission to add 8-8-8 mode 
at
> >> https://patchwork.kernel.org/cover/10638055/ ?
> >> You could use that series as the base for your changes/additions.
> > 
> > Got it.
> > My idea is to support 8D-8D-8D mode with a minimum patches because 
> > there is no define for 1D-1D-1D, 2D-2D-2D and 4D-4D-4D mode in JEDEC 
> > if I am right.
> > 
> 
> JESD251-A1 does talk about 4S-4D-4D right? Also none of the JEDEC
> standards prohibit flash vendors from supporting other X-X-X modes.
> 
> I think you haven't thought about bigger picture here. Flash devices
> that support other mode exist today and we would need the framework to
> be built such that these modes can be added in future.
> 
> I suggest you start with Boris's series [1] as base and port it to
> latest kernel. Isn't that series alone enough to support Macronix Octal
> flashes at least?
> If required, you could also always include additional patches adding new
> features.

okay.

> 
> [1] https://patchwork.kernel.org/cover/10638055/
> 
> -- 
> Regards
> Vignesh

thanks for your time & comments.
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================