[PATCH 0/3] mtd: spi-nor: Rework Octal DTR enable methods

[PATCH 0/3] mtd: spi-nor: Rework Octal DTR enable methods

[Tudor Ambarus]

There are manufacturers that use registers indexed by address. Some of
them support "read/write any register" opcodes. Provide core methods that
can be used by all manufacturers.
Update the Octal DTR enable manufacturer methods: introduce templates for
register operations, use the introduced core helpers.

The series depends on:
https://lore.kernel.org/lkml/20220209133656.374903-1-tudor.ambarus@microchip.com/

Tudor Ambarus (3):
  mtd: spi-nor: core: Add helpers to read/write any register
  mtd: spi-nor: micron-st: Rework spi_nor_micron_octal_dtr_enable()
  mtd: spi-nor: spansion: Rework spi_nor_cypress_octal_dtr_enable()

 drivers/mtd/spi-nor/core.c      |  41 ++++++++++
 drivers/mtd/spi-nor/core.h      |   4 +
 drivers/mtd/spi-nor/micron-st.c | 105 +++++++++++++------------
 drivers/mtd/spi-nor/spansion.c  | 135 +++++++++++++++++---------------
 4 files changed, 172 insertions(+), 113 deletions(-)

-- 
2.25.1

[PATCH 1/3] mtd: spi-nor: core: Add helpers to read/write any register

[Tudor Ambarus]

There are manufacturers that use registers indexed by address. Some of
them support "read/write any register" opcodes. Provide core methods that
can be used by all manufacturers. SPI NOR controller ops are intentionally
not supported as we intend to move all the SPI NOR controller drivers
under the SPI subsystem.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c | 41 ++++++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/core.h |  4 ++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 7d5e3acb0ae7..d394179689e6 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -307,6 +307,47 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
 	return nor->controller_ops->write(nor, to, len, buf);
 }
 
+/**
+ * spi_nor_read_reg() - read register to flash memory
+ * @nor:        pointer to 'struct spi_nor'.
+ * @op:		SPI memory operation. op->data.buf must be DMA-able.
+ * @proto:	SPI protocol to use for the register operation.
+ *
+ * Return: zero on success, -errno otherwise
+ */
+int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
+		     enum spi_nor_protocol proto)
+{
+	if (!nor->spimem)
+		return -EOPNOTSUPP;
+
+	spi_nor_spimem_setup_op(nor, op, proto);
+	return spi_nor_spimem_exec_op(nor, op);
+}
+
+/**
+ * spi_nor_write_reg() - write register to flash memory
+ * @nor:        pointer to 'struct spi_nor'
+ * @op:		SPI memory operation. op->data.buf must be DMA-able.
+ * @proto:	SPI protocol to use for the register operation.
+ *
+ * Return: zero on success, -errno otherwise
+ */
+int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
+		      enum spi_nor_protocol proto)
+{
+	int ret;
+
+	if (!nor->spimem)
+		return -EOPNOTSUPP;
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+	spi_nor_spimem_setup_op(nor, op, proto);
+	return spi_nor_spimem_exec_op(nor, op);
+}
+
 /**
  * spi_nor_write_enable() - Set write enable latch with Write Enable command.
  * @nor:	pointer to 'struct spi_nor'.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index cbfb4fa7647f..c728454b5424 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -578,6 +578,10 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 			  u8 *buf);
 ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
 			   const u8 *buf);
+int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
+		     enum spi_nor_protocol proto);
+int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
+		      enum spi_nor_protocol proto);
 int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
 
 int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
-- 
2.25.1

[PATCH 2/3] mtd: spi-nor: micron-st: Rework spi_nor_micron_octal_dtr_enable()

[Tudor Ambarus]

Introduce template operation to remove code duplication.
Split spi_nor_micron_octal_dtr_enable() in spi_nor_micron_octal_dtr_en()
and spi_nor_micron_octal_dtr_dis() as it no longer made sense to try to
keep everything alltogether: too many "if (enable)" throughout the code,
which made the code difficult to follow.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/micron-st.c | 105 +++++++++++++++++---------------
 1 file changed, 55 insertions(+), 50 deletions(-)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 7f66b5943ceb..013aa6a52737 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -17,73 +17,72 @@
 #define SPINOR_MT_OCT_DTR	0xe7	/* Enable Octal DTR. */
 #define SPINOR_MT_EXSPI		0xff	/* Enable Extended SPI (default) */
 
-static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
+/* Micron ST SPI NOR flash operations. */
+#define SPI_NOR_MICRON_WR_ANY_REG_OP(naddr, addr, ndata, buf)		\
+	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 0),		\
+		   SPI_MEM_OP_ADDR(naddr, addr, 0),			\
+		   SPI_MEM_OP_NO_DUMMY,					\
+		   SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
+
+static int spi_nor_micron_octal_dtr_en(struct spi_nor *nor)
 {
 	struct spi_mem_op op;
 	u8 *buf = nor->bouncebuf;
 	int ret;
 
-	if (enable) {
-		/* Use 20 dummy cycles for memory array reads. */
-		ret = spi_nor_write_enable(nor);
-		if (ret)
-			return ret;
-
-		*buf = 20;
-		op = (struct spi_mem_op)
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
-				   SPI_MEM_OP_ADDR(3, SPINOR_REG_MT_CFR1V, 1),
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-		if (ret)
-			return ret;
+	/* Use 20 dummy cycles for memory array reads. */
+	*buf = 20;
+	op = (struct spi_mem_op)
+		SPI_NOR_MICRON_WR_ANY_REG_OP(3, SPINOR_REG_MT_CFR1V, 1, buf);
+	ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret)
+		return ret;
 
-		ret = spi_nor_wait_till_ready(nor);
-		if (ret)
-			return ret;
-	}
+	buf[0] = SPINOR_MT_OCT_DTR;
+	op = (struct spi_mem_op)
+		SPI_NOR_MICRON_WR_ANY_REG_OP(3, SPINOR_REG_MT_CFR0V, 1, buf);
+	ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
 
-	ret = spi_nor_write_enable(nor);
+	/* Read flash ID to make sure the switch was successful. */
+	ret = spi_nor_read_id(nor, 0, 8, buf, SNOR_PROTO_8_8_8_DTR);
 	if (ret)
 		return ret;
 
-	if (enable) {
-		buf[0] = SPINOR_MT_OCT_DTR;
-	} else {
-		/*
-		 * The register is 1-byte wide, but 1-byte transactions are not
-		 * allowed in 8D-8D-8D mode. The next register is the dummy
-		 * cycle configuration register. Since the transaction needs to
-		 * be at least 2 bytes wide, set the next register to its
-		 * default value. This also makes sense because the value was
-		 * changed when enabling 8D-8D-8D mode, it should be reset when
-		 * disabling.
-		 */
-		buf[0] = SPINOR_MT_EXSPI;
-		buf[1] = SPINOR_REG_MT_CFR1V_DEF;
-	}
+	if (memcmp(buf, nor->info->id, nor->info->id_len))
+		return -EINVAL;
 
-	op = (struct spi_mem_op)
-		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
-			   SPI_MEM_OP_ADDR(enable ? 3 : 4,
-					   SPINOR_REG_MT_CFR0V, 1),
-			   SPI_MEM_OP_NO_DUMMY,
-			   SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
+	return 0;
+}
 
-	if (!enable)
-		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+static int spi_nor_micron_octal_dtr_dis(struct spi_nor *nor)
+{
+	struct spi_mem_op op;
+	u8 *buf = nor->bouncebuf;
+	int ret;
 
-	ret = spi_mem_exec_op(nor->spimem, &op);
+	/*
+	 * The register is 1-byte wide, but 1-byte transactions are not allowed
+	 * in 8D-8D-8D mode. The next register is the dummy cycle configuration
+	 * register. Since the transaction needs to be at least 2 bytes wide,
+	 * set the next register to its default value. This also makes sense
+	 * because the value was changed when enabling 8D-8D-8D mode, it should
+	 * be reset when disabling.
+	 */
+	buf[0] = SPINOR_MT_EXSPI;
+	buf[1] = SPINOR_REG_MT_CFR1V_DEF;
+	op = (struct spi_mem_op)
+		SPI_NOR_MICRON_WR_ANY_REG_OP(4, SPINOR_REG_MT_CFR0V, 2, buf);
+	ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
 	if (ret)
 		return ret;
 
 	/* Read flash ID to make sure the switch was successful. */
-	if (enable)
-		ret = spi_nor_read_id(nor, 0, 8, buf, SNOR_PROTO_8_8_8_DTR);
-	else
-		ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);
+	ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);
 	if (ret)
 		return ret;
 
@@ -93,6 +92,12 @@ static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
 	return 0;
 }
 
+static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+	return enable ? spi_nor_micron_octal_dtr_en(nor) :
+			spi_nor_micron_octal_dtr_dis(nor);
+}
+
 static void mt35xu512aba_default_init(struct spi_nor *nor)
 {
 	nor->params->octal_dtr_enable = spi_nor_micron_octal_dtr_enable;
-- 
2.25.1

[PATCH 3/3] mtd: spi-nor: spansion: Rework spi_nor_cypress_octal_dtr_enable()

[Tudor Ambarus]

Introduce template operation to remove code duplication.
Split spi_nor_cypress_octal_dtr_enable() in
spi_nor_cypress_octal_dtr_ena() spi_nor_cypress_octal_dtr_dis() as it no
longer made sense to try to keep everything alltogether: too many
"if (enable)" throughout the code, which made the code difficult to read.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spansion.c | 135 ++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 63 deletions(-)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index d69a569f31e4..9bb239f1e142 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -19,85 +19,78 @@
 #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
 #define SPINOR_OP_CYPRESS_RD_FAST		0xee
 
-/**
- * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
- * @nor:		pointer to a 'struct spi_nor'
- * @enable:              whether to enable or disable Octal DTR
- *
- * This also sets the memory access latency cycles to 24 to allow the flash to
- * run at up to 200MHz.
- *
- * Return: 0 on success, -errno otherwise.
- */
-static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
+/* Spansion/Cypress SPI NOR flash operations. */
+#define SPI_NOR_SPANSION_WR_ANY_REG_OP(naddr, addr, ndata, buf)		\
+	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 0),		\
+		   SPI_MEM_OP_ADDR(naddr, addr, 0),			\
+		   SPI_MEM_OP_NO_DUMMY,					\
+		   SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
+
+static int spi_nor_cypress_octal_dtr_en(struct spi_nor *nor)
 {
 	struct spi_mem_op op;
 	u8 *buf = nor->bouncebuf;
 	int ret;
 
-	if (enable) {
-		/* Use 24 dummy cycles for memory array reads. */
-		ret = spi_nor_write_enable(nor);
-		if (ret)
-			return ret;
-
-		*buf = SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24;
-		op = (struct spi_mem_op)
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
-				   SPI_MEM_OP_ADDR(3, SPINOR_REG_CYPRESS_CFR2V,
-						   1),
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-		if (ret)
-			return ret;
-
-		ret = spi_nor_wait_till_ready(nor);
-		if (ret)
-			return ret;
-
-		nor->read_dummy = 24;
-	}
-
-	/* Set/unset the octal and DTR enable bits. */
-	ret = spi_nor_write_enable(nor);
+	/* Use 24 dummy cycles for memory array reads. */
+	*buf = SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24;
+	op = (struct spi_mem_op)
+		SPI_NOR_SPANSION_WR_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR2V,
+					       1, buf);
+
+	ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
 	if (ret)
 		return ret;
 
-	if (enable) {
-		buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
-	} else {
-		/*
-		 * The register is 1-byte wide, but 1-byte transactions are not
-		 * allowed in 8D-8D-8D mode. Since there is no register at the
-		 * next location, just initialize the value to 0 and let the
-		 * transaction go on.
-		 */
-		buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
-		buf[1] = 0;
-	}
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret)
+		return ret;
+
+	nor->read_dummy = 24;
 
+	/* Set the octal and DTR enable bits. */
+	buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
 	op = (struct spi_mem_op)
-		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
-			   SPI_MEM_OP_ADDR(enable ? 3 : 4,
-					   SPINOR_REG_CYPRESS_CFR5V,
-					   1),
-			   SPI_MEM_OP_NO_DUMMY,
-			   SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
+		SPI_NOR_SPANSION_WR_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR5V,
+					       1, buf);
 
-	if (!enable)
-		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+	ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
 
-	ret = spi_mem_exec_op(nor->spimem, &op);
+	/* Read flash ID to make sure the switch was successful. */
+	ret = spi_nor_read_id(nor, 4, 3, buf, SNOR_PROTO_8_8_8_DTR);
+	if (ret)
+		return ret;
+
+	if (memcmp(buf, nor->info->id, nor->info->id_len))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int spi_nor_cypress_octal_dtr_dis(struct spi_nor *nor)
+{
+	struct spi_mem_op op;
+	u8 *buf = nor->bouncebuf;
+	int ret;
+
+	/*
+	 * The register is 1-byte wide, but 1-byte transactions are not allowed
+	 * in 8D-8D-8D mode. Since there is no register at the next location,
+	 * just initialize the value to 0 and let the transaction go on.
+	 */
+	buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
+	buf[1] = 0;
+	op = (struct spi_mem_op)
+		SPI_NOR_SPANSION_WR_ANY_REG_OP(4, SPINOR_REG_CYPRESS_CFR5V,
+					       2, buf);
+	ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
 	if (ret)
 		return ret;
 
 	/* Read flash ID to make sure the switch was successful. */
-	if (enable)
-		ret = spi_nor_read_id(nor, 4, 3, buf, SNOR_PROTO_8_8_8_DTR);
-	else
-		ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);
+	ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);
 	if (ret)
 		return ret;
 
@@ -107,6 +100,22 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
 	return 0;
 }
 
+/**
+ * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
+ * @nor:		pointer to a 'struct spi_nor'
+ * @enable:              whether to enable or disable Octal DTR
+ *
+ * This also sets the memory access latency cycles to 24 to allow the flash to
+ * run at up to 200MHz.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+	return enable ? spi_nor_cypress_octal_dtr_en(nor) :
+			spi_nor_cypress_octal_dtr_dis(nor);
+}
+
 static void s28hs512t_default_init(struct spi_nor *nor)
 {
 	nor->params->octal_dtr_enable = spi_nor_cypress_octal_dtr_enable;
-- 
2.25.1

Re: [PATCH 1/3] mtd: spi-nor: core: Add helpers to read/write any register

[Takahiro Kuwano]

On 2/10/2022 11:33 AM, Tudor Ambarus wrote:
> There are manufacturers that use registers indexed by address. Some of
> them support "read/write any register" opcodes. Provide core methods that
> can be used by all manufacturers. SPI NOR controller ops are intentionally
> not supported as we intend to move all the SPI NOR controller drivers
> under the SPI subsystem.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 41 ++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/core.h |  4 ++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 7d5e3acb0ae7..d394179689e6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -307,6 +307,47 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>  	return nor->controller_ops->write(nor, to, len, buf);
>  }
>  
> +/**
> + * spi_nor_read_reg() - read register to flash memory
> + * @nor:        pointer to 'struct spi_nor'.
> + * @op:		SPI memory operation. op->data.buf must be DMA-able.
> + * @proto:	SPI protocol to use for the register operation.
> + *
> + * Return: zero on success, -errno otherwise
> + */
> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
> +		     enum spi_nor_protocol proto)
> +{
> +	if (!nor->spimem)
> +		return -EOPNOTSUPP;
> +
> +	spi_nor_spimem_setup_op(nor, op, proto);
> +	return spi_nor_spimem_exec_op(nor, op);
> +}
> +
> +/**
> + * spi_nor_write_reg() - write register to flash memory
> + * @nor:        pointer to 'struct spi_nor'
> + * @op:		SPI memory operation. op->data.buf must be DMA-able.
> + * @proto:	SPI protocol to use for the register operation.
> + *
> + * Return: zero on success, -errno otherwise
> + */
> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
> +		      enum spi_nor_protocol proto)
> +{
> +	int ret;
> +
> +	if (!nor->spimem)
> +		return -EOPNOTSUPP;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +	spi_nor_spimem_setup_op(nor, op, proto);
> +	return spi_nor_spimem_exec_op(nor, op);
> +}
> +
>  /**
>   * spi_nor_write_enable() - Set write enable latch with Write Enable command.
>   * @nor:	pointer to 'struct spi_nor'.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index cbfb4fa7647f..c728454b5424 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -578,6 +578,10 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>  			  u8 *buf);
>  ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>  			   const u8 *buf);
> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
> +		     enum spi_nor_protocol proto);
> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
> +		      enum spi_nor_protocol proto);
>  int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
>  
>  int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);

Thank you for introducing these helpers.
I revised my S25HL/HS-T series on top of this and confirmed that is
working correctly.

Tested-By: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

Best Regards,
Takahiro

Re: [PATCH 1/3] mtd: spi-nor: core: Add helpers to read/write any register

[Pratyush Yadav]

On 10/02/22 04:33AM, Tudor Ambarus wrote:
> There are manufacturers that use registers indexed by address. Some of
> them support "read/write any register" opcodes. Provide core methods that
> can be used by all manufacturers. SPI NOR controller ops are intentionally
> not supported as we intend to move all the SPI NOR controller drivers
> under the SPI subsystem.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 41 ++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/core.h |  4 ++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 7d5e3acb0ae7..d394179689e6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -307,6 +307,47 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>  	return nor->controller_ops->write(nor, to, len, buf);
>  }
>  
> +/**
> + * spi_nor_read_reg() - read register to flash memory

Nitpick: s/to/from/ ?

> + * @nor:        pointer to 'struct spi_nor'.
> + * @op:		SPI memory operation. op->data.buf must be DMA-able.
> + * @proto:	SPI protocol to use for the register operation.
> + *
> + * Return: zero on success, -errno otherwise
> + */
> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
> +		     enum spi_nor_protocol proto)
> +{
> +	if (!nor->spimem)
> +		return -EOPNOTSUPP;
> +
> +	spi_nor_spimem_setup_op(nor, op, proto);
> +	return spi_nor_spimem_exec_op(nor, op);
> +}
> +
> +/**
> + * spi_nor_write_reg() - write register to flash memory
> + * @nor:        pointer to 'struct spi_nor'
> + * @op:		SPI memory operation. op->data.buf must be DMA-able.
> + * @proto:	SPI protocol to use for the register operation.
> + *
> + * Return: zero on success, -errno otherwise
> + */
> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
> +		      enum spi_nor_protocol proto)
> +{
> +	int ret;
> +
> +	if (!nor->spimem)
> +		return -EOPNOTSUPP;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;

Nitpick: Add a blank line here.

> +	spi_nor_spimem_setup_op(nor, op, proto);
> +	return spi_nor_spimem_exec_op(nor, op);
> +}
> +
>  /**
>   * spi_nor_write_enable() - Set write enable latch with Write Enable command.
>   * @nor:	pointer to 'struct spi_nor'.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index cbfb4fa7647f..c728454b5424 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -578,6 +578,10 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>  			  u8 *buf);
>  ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>  			   const u8 *buf);
> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
> +		     enum spi_nor_protocol proto);
> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
> +		      enum spi_nor_protocol proto);
>  int spi_nor_erase_sector(struct spi_nor *nor, u32 addr);
>  
>  int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 2/3] mtd: spi-nor: micron-st: Rework spi_nor_micron_octal_dtr_enable()

[Pratyush Yadav]

Hi Tudor,

On 10/02/22 04:33AM, Tudor Ambarus wrote:
> Introduce template operation to remove code duplication.
> Split spi_nor_micron_octal_dtr_enable() in spi_nor_micron_octal_dtr_en()
> and spi_nor_micron_octal_dtr_dis() as it no longer made sense to try to
> keep everything alltogether: too many "if (enable)" throughout the code,
> which made the code difficult to follow.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/micron-st.c | 105 +++++++++++++++++---------------
>  1 file changed, 55 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index 7f66b5943ceb..013aa6a52737 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -17,73 +17,72 @@
>  #define SPINOR_MT_OCT_DTR	0xe7	/* Enable Octal DTR. */
>  #define SPINOR_MT_EXSPI		0xff	/* Enable Extended SPI (default) */
>  
> -static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
> +/* Micron ST SPI NOR flash operations. */
> +#define SPI_NOR_MICRON_WR_ANY_REG_OP(naddr, addr, ndata, buf)		\

Should change function and variable names based on mwalle's patches 
(assuming you agree with that scheme). MICRON_NOR_WR_ANY_REG_OP?

> +	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 0),		\
> +		   SPI_MEM_OP_ADDR(naddr, addr, 0),			\
> +		   SPI_MEM_OP_NO_DUMMY,					\
> +		   SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
> +
> +static int spi_nor_micron_octal_dtr_en(struct spi_nor *nor)

micron_nor_octal_dtr_en(). Same for other functions.

>  {
>  	struct spi_mem_op op;
>  	u8 *buf = nor->bouncebuf;
>  	int ret;
>  
> -	if (enable) {
> -		/* Use 20 dummy cycles for memory array reads. */
> -		ret = spi_nor_write_enable(nor);
> -		if (ret)
> -			return ret;
> -
> -		*buf = 20;
> -		op = (struct spi_mem_op)
> -			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
> -				   SPI_MEM_OP_ADDR(3, SPINOR_REG_MT_CFR1V, 1),
> -				   SPI_MEM_OP_NO_DUMMY,
> -				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
> -
> -		ret = spi_mem_exec_op(nor->spimem, &op);
> -		if (ret)
> -			return ret;
> +	/* Use 20 dummy cycles for memory array reads. */
> +	*buf = 20;
> +	op = (struct spi_mem_op)
> +		SPI_NOR_MICRON_WR_ANY_REG_OP(3, SPINOR_REG_MT_CFR1V, 1, buf);
> +	ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
> +	if (ret)
> +		return ret;
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret)
> +		return ret;
>  
> -		ret = spi_nor_wait_till_ready(nor);
> -		if (ret)
> -			return ret;
> -	}
> +	buf[0] = SPINOR_MT_OCT_DTR;
> +	op = (struct spi_mem_op)
> +		SPI_NOR_MICRON_WR_ANY_REG_OP(3, SPINOR_REG_MT_CFR0V, 1, buf);
> +	ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
> +	if (ret)
> +		return ret;
>  
> -	ret = spi_nor_write_enable(nor);
> +	/* Read flash ID to make sure the switch was successful. */
> +	ret = spi_nor_read_id(nor, 0, 8, buf, SNOR_PROTO_8_8_8_DTR);
>  	if (ret)
>  		return ret;
>  
> -	if (enable) {
> -		buf[0] = SPINOR_MT_OCT_DTR;
> -	} else {
> -		/*
> -		 * The register is 1-byte wide, but 1-byte transactions are not
> -		 * allowed in 8D-8D-8D mode. The next register is the dummy
> -		 * cycle configuration register. Since the transaction needs to
> -		 * be at least 2 bytes wide, set the next register to its
> -		 * default value. This also makes sense because the value was
> -		 * changed when enabling 8D-8D-8D mode, it should be reset when
> -		 * disabling.
> -		 */
> -		buf[0] = SPINOR_MT_EXSPI;
> -		buf[1] = SPINOR_REG_MT_CFR1V_DEF;
> -	}
> +	if (memcmp(buf, nor->info->id, nor->info->id_len))
> +		return -EINVAL;
>  
> -	op = (struct spi_mem_op)
> -		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
> -			   SPI_MEM_OP_ADDR(enable ? 3 : 4,
> -					   SPINOR_REG_MT_CFR0V, 1),
> -			   SPI_MEM_OP_NO_DUMMY,
> -			   SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
> +	return 0;
> +}
>  
> -	if (!enable)
> -		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +static int spi_nor_micron_octal_dtr_dis(struct spi_nor *nor)
> +{
> +	struct spi_mem_op op;
> +	u8 *buf = nor->bouncebuf;
> +	int ret;
>  
> -	ret = spi_mem_exec_op(nor->spimem, &op);
> +	/*
> +	 * The register is 1-byte wide, but 1-byte transactions are not allowed
> +	 * in 8D-8D-8D mode. The next register is the dummy cycle configuration
> +	 * register. Since the transaction needs to be at least 2 bytes wide,
> +	 * set the next register to its default value. This also makes sense
> +	 * because the value was changed when enabling 8D-8D-8D mode, it should
> +	 * be reset when disabling.
> +	 */
> +	buf[0] = SPINOR_MT_EXSPI;
> +	buf[1] = SPINOR_REG_MT_CFR1V_DEF;
> +	op = (struct spi_mem_op)
> +		SPI_NOR_MICRON_WR_ANY_REG_OP(4, SPINOR_REG_MT_CFR0V, 2, buf);
> +	ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
>  	if (ret)
>  		return ret;
>  
>  	/* Read flash ID to make sure the switch was successful. */
> -	if (enable)
> -		ret = spi_nor_read_id(nor, 0, 8, buf, SNOR_PROTO_8_8_8_DTR);
> -	else
> -		ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);
> +	ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);

nor->reg_proto is not updated yet. It will be updated _after_ this 
function completes. So you would end up calling read ID in 8D-8D-8D 
mode, which would be bogus.

I tried with Micron MT35XU512ABA. Enable works fine, but disable fails 
(it succeeds in reality but the function is unable to verify that) 
because of this. Changing nor->reg_proto to SNOR_PROTO_1_1_1 fixes it.

Looks like the problem is not introduced by this patch though. It seems 
to come from patch 5 of your mx66 series. I see the same with the 
Cypress flash too but I have not tested it yet.

>  	if (ret)
>  		return ret;
>  
> @@ -93,6 +92,12 @@ static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
>  	return 0;
>  }
>  
> +static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
> +{
> +	return enable ? spi_nor_micron_octal_dtr_en(nor) :
> +			spi_nor_micron_octal_dtr_dis(nor);
> +}
> +
>  static void mt35xu512aba_default_init(struct spi_nor *nor)
>  {
>  	nor->params->octal_dtr_enable = spi_nor_micron_octal_dtr_enable;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 3/3] mtd: spi-nor: spansion: Rework spi_nor_cypress_octal_dtr_enable()

[Pratyush Yadav]

On 10/02/22 04:33AM, Tudor Ambarus wrote:
> Introduce template operation to remove code duplication.
> Split spi_nor_cypress_octal_dtr_enable() in
> spi_nor_cypress_octal_dtr_ena() spi_nor_cypress_octal_dtr_dis() as it no
> longer made sense to try to keep everything alltogether: too many
> "if (enable)" throughout the code, which made the code difficult to read.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/spansion.c | 135 ++++++++++++++++++---------------
>  1 file changed, 72 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index d69a569f31e4..9bb239f1e142 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -19,85 +19,78 @@
>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
>  #define SPINOR_OP_CYPRESS_RD_FAST		0xee
>  
> -/**
> - * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
> - * @nor:		pointer to a 'struct spi_nor'
> - * @enable:              whether to enable or disable Octal DTR
> - *
> - * This also sets the memory access latency cycles to 24 to allow the flash to
> - * run at up to 200MHz.
> - *
> - * Return: 0 on success, -errno otherwise.
> - */
> -static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
> +/* Spansion/Cypress SPI NOR flash operations. */
> +#define SPI_NOR_SPANSION_WR_ANY_REG_OP(naddr, addr, ndata, buf)		\

Same comments as before for variable and function names.

> +	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 0),		\
> +		   SPI_MEM_OP_ADDR(naddr, addr, 0),			\
> +		   SPI_MEM_OP_NO_DUMMY,					\
> +		   SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
> +
> +static int spi_nor_cypress_octal_dtr_en(struct spi_nor *nor)
>  {
>  	struct spi_mem_op op;
>  	u8 *buf = nor->bouncebuf;
>  	int ret;
>  
> -	if (enable) {
> -		/* Use 24 dummy cycles for memory array reads. */
> -		ret = spi_nor_write_enable(nor);
> -		if (ret)
> -			return ret;
> -
> -		*buf = SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24;
> -		op = (struct spi_mem_op)
> -			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
> -				   SPI_MEM_OP_ADDR(3, SPINOR_REG_CYPRESS_CFR2V,
> -						   1),
> -				   SPI_MEM_OP_NO_DUMMY,
> -				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
> -
> -		ret = spi_mem_exec_op(nor->spimem, &op);
> -		if (ret)
> -			return ret;
> -
> -		ret = spi_nor_wait_till_ready(nor);
> -		if (ret)
> -			return ret;
> -
> -		nor->read_dummy = 24;
> -	}
> -
> -	/* Set/unset the octal and DTR enable bits. */
> -	ret = spi_nor_write_enable(nor);
> +	/* Use 24 dummy cycles for memory array reads. */
> +	*buf = SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24;
> +	op = (struct spi_mem_op)
> +		SPI_NOR_SPANSION_WR_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR2V,
> +					       1, buf);
> +
> +	ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
>  	if (ret)
>  		return ret;
>  
> -	if (enable) {
> -		buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
> -	} else {
> -		/*
> -		 * The register is 1-byte wide, but 1-byte transactions are not
> -		 * allowed in 8D-8D-8D mode. Since there is no register at the
> -		 * next location, just initialize the value to 0 and let the
> -		 * transaction go on.
> -		 */
> -		buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
> -		buf[1] = 0;
> -	}
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret)
> +		return ret;
> +
> +	nor->read_dummy = 24;
>  
> +	/* Set the octal and DTR enable bits. */
> +	buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
>  	op = (struct spi_mem_op)
> -		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
> -			   SPI_MEM_OP_ADDR(enable ? 3 : 4,
> -					   SPINOR_REG_CYPRESS_CFR5V,
> -					   1),
> -			   SPI_MEM_OP_NO_DUMMY,
> -			   SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
> +		SPI_NOR_SPANSION_WR_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR5V,
> +					       1, buf);
>  
> -	if (!enable)
> -		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +	ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
> +	if (ret)
> +		return ret;
>  
> -	ret = spi_mem_exec_op(nor->spimem, &op);
> +	/* Read flash ID to make sure the switch was successful. */
> +	ret = spi_nor_read_id(nor, 4, 3, buf, SNOR_PROTO_8_8_8_DTR);
> +	if (ret)
> +		return ret;
> +
> +	if (memcmp(buf, nor->info->id, nor->info->id_len))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int spi_nor_cypress_octal_dtr_dis(struct spi_nor *nor)
> +{
> +	struct spi_mem_op op;
> +	u8 *buf = nor->bouncebuf;
> +	int ret;
> +
> +	/*
> +	 * The register is 1-byte wide, but 1-byte transactions are not allowed
> +	 * in 8D-8D-8D mode. Since there is no register at the next location,
> +	 * just initialize the value to 0 and let the transaction go on.
> +	 */
> +	buf[0] = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
> +	buf[1] = 0;
> +	op = (struct spi_mem_op)
> +		SPI_NOR_SPANSION_WR_ANY_REG_OP(4, SPINOR_REG_CYPRESS_CFR5V,
> +					       2, buf);
> +	ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
>  	if (ret)
>  		return ret;
>  
>  	/* Read flash ID to make sure the switch was successful. */
> -	if (enable)
> -		ret = spi_nor_read_id(nor, 4, 3, buf, SNOR_PROTO_8_8_8_DTR);
> -	else
> -		ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);
> +	ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);

As with previous patch, this won't work.

I have not tested on this flash yet. I will wait for a re-roll.

>  	if (ret)
>  		return ret;
>  
> @@ -107,6 +100,22 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
>  	return 0;
>  }
>  
> +/**
> + * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
> + * @nor:		pointer to a 'struct spi_nor'
> + * @enable:              whether to enable or disable Octal DTR
> + *
> + * This also sets the memory access latency cycles to 24 to allow the flash to
> + * run at up to 200MHz.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
> +{
> +	return enable ? spi_nor_cypress_octal_dtr_en(nor) :
> +			spi_nor_cypress_octal_dtr_dis(nor);
> +}
> +
>  static void s28hs512t_default_init(struct spi_nor *nor)
>  {
>  	nor->params->octal_dtr_enable = spi_nor_cypress_octal_dtr_enable;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 2/3] mtd: spi-nor: micron-st: Rework spi_nor_micron_octal_dtr_enable()

[Tudor.Ambarus]

On 2/24/22 21:51, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
> On 10/02/22 04:33AM, Tudor Ambarus wrote:
>> Introduce template operation to remove code duplication.
>> Split spi_nor_micron_octal_dtr_enable() in spi_nor_micron_octal_dtr_en()
>> and spi_nor_micron_octal_dtr_dis() as it no longer made sense to try to
>> keep everything alltogether: too many "if (enable)" throughout the code,
>> which made the code difficult to follow.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/micron-st.c | 105 +++++++++++++++++---------------
>>  1 file changed, 55 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
>> index 7f66b5943ceb..013aa6a52737 100644
>> --- a/drivers/mtd/spi-nor/micron-st.c
>> +++ b/drivers/mtd/spi-nor/micron-st.c
>> @@ -17,73 +17,72 @@
>>  #define SPINOR_MT_OCT_DTR    0xe7    /* Enable Octal DTR. */
>>  #define SPINOR_MT_EXSPI              0xff    /* Enable Extended SPI (default) */
>>
>> -static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
>> +/* Micron ST SPI NOR flash operations. */
>> +#define SPI_NOR_MICRON_WR_ANY_REG_OP(naddr, addr, ndata, buf)                \
> 
> Should change function and variable names based on mwalle's patches
> (assuming you agree with that scheme). MICRON_NOR_WR_ANY_REG_OP?

yes, will do.

> 
>> +     SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 0),          \
>> +                SPI_MEM_OP_ADDR(naddr, addr, 0),                     \
>> +                SPI_MEM_OP_NO_DUMMY,                                 \
>> +                SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
>> +
>> +static int spi_nor_micron_octal_dtr_en(struct spi_nor *nor)
> 
> micron_nor_octal_dtr_en(). Same for other functions.
> 
>>  {
>>       struct spi_mem_op op;
>>       u8 *buf = nor->bouncebuf;
>>       int ret;
>>
>> -     if (enable) {
>> -             /* Use 20 dummy cycles for memory array reads. */
>> -             ret = spi_nor_write_enable(nor);
>> -             if (ret)
>> -                     return ret;
>> -
>> -             *buf = 20;
>> -             op = (struct spi_mem_op)
>> -                     SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
>> -                                SPI_MEM_OP_ADDR(3, SPINOR_REG_MT_CFR1V, 1),
>> -                                SPI_MEM_OP_NO_DUMMY,
>> -                                SPI_MEM_OP_DATA_OUT(1, buf, 1));
>> -
>> -             ret = spi_mem_exec_op(nor->spimem, &op);
>> -             if (ret)
>> -                     return ret;
>> +     /* Use 20 dummy cycles for memory array reads. */
>> +     *buf = 20;
>> +     op = (struct spi_mem_op)
>> +             SPI_NOR_MICRON_WR_ANY_REG_OP(3, SPINOR_REG_MT_CFR1V, 1, buf);
>> +     ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
>> +     if (ret)
>> +             return ret;
>> +     ret = spi_nor_wait_till_ready(nor);
>> +     if (ret)
>> +             return ret;
>>
>> -             ret = spi_nor_wait_till_ready(nor);
>> -             if (ret)
>> -                     return ret;
>> -     }
>> +     buf[0] = SPINOR_MT_OCT_DTR;
>> +     op = (struct spi_mem_op)
>> +             SPI_NOR_MICRON_WR_ANY_REG_OP(3, SPINOR_REG_MT_CFR0V, 1, buf);
>> +     ret = spi_nor_write_reg(nor, &op, nor->reg_proto);
>> +     if (ret)
>> +             return ret;
>>
>> -     ret = spi_nor_write_enable(nor);
>> +     /* Read flash ID to make sure the switch was successful. */
>> +     ret = spi_nor_read_id(nor, 0, 8, buf, SNOR_PROTO_8_8_8_DTR);
>>       if (ret)
>>               return ret;
>>
>> -     if (enable) {
>> -             buf[0] = SPINOR_MT_OCT_DTR;
>> -     } else {
>> -             /*
>> -              * The register is 1-byte wide, but 1-byte transactions are not
>> -              * allowed in 8D-8D-8D mode. The next register is the dummy
>> -              * cycle configuration register. Since the transaction needs to
>> -              * be at least 2 bytes wide, set the next register to its
>> -              * default value. This also makes sense because the value was
>> -              * changed when enabling 8D-8D-8D mode, it should be reset when
>> -              * disabling.
>> -              */
>> -             buf[0] = SPINOR_MT_EXSPI;
>> -             buf[1] = SPINOR_REG_MT_CFR1V_DEF;
>> -     }
>> +     if (memcmp(buf, nor->info->id, nor->info->id_len))
>> +             return -EINVAL;
>>
>> -     op = (struct spi_mem_op)
>> -             SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
>> -                        SPI_MEM_OP_ADDR(enable ? 3 : 4,
>> -                                        SPINOR_REG_MT_CFR0V, 1),
>> -                        SPI_MEM_OP_NO_DUMMY,
>> -                        SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
>> +     return 0;
>> +}
>>
>> -     if (!enable)
>> -             spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
>> +static int spi_nor_micron_octal_dtr_dis(struct spi_nor *nor)
>> +{
>> +     struct spi_mem_op op;
>> +     u8 *buf = nor->bouncebuf;
>> +     int ret;
>>
>> -     ret = spi_mem_exec_op(nor->spimem, &op);
>> +     /*
>> +      * The register is 1-byte wide, but 1-byte transactions are not allowed
>> +      * in 8D-8D-8D mode. The next register is the dummy cycle configuration
>> +      * register. Since the transaction needs to be at least 2 bytes wide,
>> +      * set the next register to its default value. This also makes sense
>> +      * because the value was changed when enabling 8D-8D-8D mode, it should
>> +      * be reset when disabling.
>> +      */
>> +     buf[0] = SPINOR_MT_EXSPI;
>> +     buf[1] = SPINOR_REG_MT_CFR1V_DEF;
>> +     op = (struct spi_mem_op)
>> +             SPI_NOR_MICRON_WR_ANY_REG_OP(4, SPINOR_REG_MT_CFR0V, 2, buf);
>> +     ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
>>       if (ret)
>>               return ret;
>>
>>       /* Read flash ID to make sure the switch was successful. */
>> -     if (enable)
>> -             ret = spi_nor_read_id(nor, 0, 8, buf, SNOR_PROTO_8_8_8_DTR);
>> -     else
>> -             ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);
>> +     ret = spi_nor_read_id(nor, 0, 0, buf, nor->reg_proto);
> 
> nor->reg_proto is not updated yet. It will be updated _after_ this
> function completes. So you would end up calling read ID in 8D-8D-8D
> mode, which would be bogus.
> 
> I tried with Micron MT35XU512ABA. Enable works fine, but disable fails
> (it succeeds in reality but the function is unable to verify that)
> because of this. Changing nor->reg_proto to SNOR_PROTO_1_1_1 fixes it.
> 
> Looks like the problem is not introduced by this patch though. It seems
> to come from patch 5 of your mx66 series. I see the same with the
> Cypress flash too but I have not tested it yet.

okay, thanks, I'll take a look. Thanks for doing the testing!

Cheers,
ta
> 
>>       if (ret)
>>               return ret;
>>
>> @@ -93,6 +92,12 @@ static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
>>       return 0;
>>  }
>>
>> +static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
>> +{
>> +     return enable ? spi_nor_micron_octal_dtr_en(nor) :
>> +                     spi_nor_micron_octal_dtr_dis(nor);
>> +}
>> +
>>  static void mt35xu512aba_default_init(struct spi_nor *nor)
>>  {
>>       nor->params->octal_dtr_enable = spi_nor_micron_octal_dtr_enable;
>> --
>> 2.25.1
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.