mtd: spinand: Add Octal DTR SPI (8D-8D-8D) mode support

Setting dtr field of spi_mem_op is useful when creating templates for DTR ops in spinand.h. Also, 2 bytes cmd phases are required when operating in Octal DTR SPI mode.

Setting dtr field of spi_mem_op is useful when creating templates for DTR ops in spinand.h. Also, 2 bytes cmd phases are required when operating in Octal DTR SPI mode.

Create new templates for dtr mode cmd, address, dummy and data phase in spi_mem_op, which set the dtr field to 1 and also allow passing the nbytes for the cmd phase.

Signed-off-by Apurva Nandan <[email protected]>

---
 include/linux/spi/spi-mem.h | 87 ++++++++++++++++++++++++++-----------
 1 file changed, 61 insertions(+), 26 deletions(-)

include/linux/spi/spi-mem.h +6 -5

index 85e2ff7b840d..73e52a3ecf66 100644
@@ -13,46 +13,81 @@
 
 #include <linux/spi/spi.h>
 
-#define SPI_MEM_OP_CMD(__opcode, __buswidth)			\
-	{							\
-		.buswidth = __buswidth,				\
-		.opcode = __opcode,				\
-		.nbytes = 1,					\
+#define SPI_MEM_OP_CMD_ALL_ARGS(__nbytes, __opcode, __buswidth, __dtr)	\
+	{								\
+		.buswidth = __buswidth,					\
+		.opcode = __opcode,					\
+		.nbytes = __nbytes,					\
+		.dtr = __dtr,						\
 	}
 

Currently, the op macros in spinand.h don't give the option to setup any non-array access instructions for Dual/Quad/Octal DTR SPI bus. Having a function that setups the op based on reg_proto would be better than trying to write all the setup logic in op macros.

Create a spimem_setup_op() that would setup cmd, addr, dummy and data phase of the spi_mem op, for the given spinand->reg_proto. And hence, call the spimem_setup_op() before executing any spi_mem op.

Note: In this commit, spimem_setup_op() isn't called in the read_reg_op(), write_reg_op() and wait() functions, as they need modifications in address value and data nbytes when in Octal DTR mode. This will be fixed in a later commit.

Signed-off-by Apurva Nandan <[email protected]>

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:28 +0000:

Quoted 5 lines

Currently, the op macros in spinand.h don't give the option to setup any non-array access instructions for Dual/Quad/Octal DTR SPI bus. Having a function that setups the op based on reg_proto would be better than trying to write all the setup logic in op macros.

Quoted 8 lines

Create a spimem_setup_op() that would setup cmd, addr, dummy and data phase of the spi_mem op, for the given spinand->reg_proto. And hence, call the spimem_setup_op() before executing any spi_mem op.

Note: In this commit, spimem_setup_op() isn't called in the read_reg_op(), write_reg_op() and wait() functions, as they need modifications in address value and data nbytes when in Octal DTR mode. This will be fixed in a later commit.

Thanks for this series!

So far I am fine with your changes, but I don't like the setup_op() naming much. I don't yet have a better idea, could you propose something more meaningful?

Quoted 1 line

Signed-off-by Apurva Nandan <[email protected]>

Thanks,
Miquèl

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

Hi Miquèl,

Quoted 19 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:28 +0000:

Quoted 5 lines

Currently, the op macros in spinand.h don't give the option to setup any non-array access instructions for Dual/Quad/Octal DTR SPI bus. Having a function that setups the op based on reg_proto would be better than trying to write all the setup logic in op macros.

Quoted 8 lines

Create a spimem_setup_op() that would setup cmd, addr, dummy and data phase of the spi_mem op, for the given spinand->reg_proto. And hence, call the spimem_setup_op() before executing any spi_mem op.

Note: In this commit, spimem_setup_op() isn't called in the read_reg_op(), write_reg_op() and wait() functions, as they need modifications in address value and data nbytes when in Octal DTR mode. This will be fixed in a later commit.

Quoted 6 lines

Thanks for this series!

So far I am fine with your changes, but I don't like the setup_op() naming much. I don't yet have a better idea, could you propose something more meaningful?

I made this similar to the spi_nor_spimem_setup_op(), which essentially does the same task as this in the spi-nor core.

Other names that I can think of are:

- config_op(), adjust_op(), amend_op(), patch_op()

or

- handle_op_variations(), apply_op_variations()

What do you suggest?

Quoted 5 lines

Quoted 1 line

Signed-off-by Apurva Nandan <[email protected]>

Thanks,
Miquèl

Quoted 4 lines

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

Thanks,
Apurva Nandan

Hi Apurva,

Boris, you might have a good idea for the naming discussed below?

Quoted 2 lines · On Fri, 20 Aug 2021 14:08:40 +0200 Miquel Raynal <[email protected]>…

Hi Apurva,

Quoted 1 line

Boris, you might have a good idea for the naming discussed below?

{patch,adjust,tweak}_op() all sound good to me. This being said, I'm a bit concerned about doing this op tweaking in the hot-path. Looks like something that should be done at probe or when switching to 8D mode, and kept around. The alternative would be to have per-mode op tables, which I think would be clearer.

In Octal DTR SPI mode, 2 bytes of data gets transmitted over one clock cycle, and half-cycle instruction phases aren't supported yet. So, every DTR spi_mem_op needs to have even nbytes in all phases for non-erratic behaviour from the SPI controller.

The odd length cmd and dummy phases get handled by spimem_setup_op() but the odd length address and data phases need to be handled according to the use case. For example in Octal DTR mode, read register operation has one byte long address and data phase. So it needs to extend it by adding a suitable extra byte in addr and reading 2 bytes of data, discarding the second byte.

Handle address and data phases for Octal DTR mode in read/write register and write volatile configuration register operations by adding a suitable extra byte in the address and data phase.

Create spimem_setup_reg_op() helper function to ease setting up read/write register operations in other functions, e.g. wait().

Signed-off-by Apurva Nandan <[email protected]>

---
 drivers/mtd/nand/spi/core.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

drivers/mtd/nand/spi/core.c +0 -0

index 2e59faecc8f5..a5334ad34f96 100644

drivers/mtd/nand/spi/core.c +2 -1

-	*spinand->scratchbuf = val;
+	spinand_setup_reg_op(spinand, &op);
+	memset(spinand->scratchbuf, val, op.data.nbytes);
 	return spi_mem_exec_op(spinand->spimem, &op);
 }
 

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:29 +0000:

Quoted 32 lines

In Octal DTR SPI mode, 2 bytes of data gets transmitted over one clock cycle, and half-cycle instruction phases aren't supported yet. So, every DTR spi_mem_op needs to have even nbytes in all phases for non-erratic behaviour from the SPI controller.

The odd length cmd and dummy phases get handled by spimem_setup_op() but the odd length address and data phases need to be handled according to the use case. For example in Octal DTR mode, read register operation has one byte long address and data phase. So it needs to extend it by adding a suitable extra byte in addr and reading 2 bytes of data, discarding the second byte.

Handle address and data phases for Octal DTR mode in read/write register and write volatile configuration register operations by adding a suitable extra byte in the address and data phase.

Create spimem_setup_reg_op() helper function to ease setting up read/write register operations in other functions, e.g. wait().

Signed-off-by Apurva Nandan <[email protected]>

---
  drivers/mtd/nand/spi/core.c | 26 +++++++++++++++++++++-----
  1 file changed, 21 insertions(+), 5 deletions(-)

drivers/mtd/nand/spi/core.c +0 -0

index 2e59faecc8f5..a5334ad34f96 100644
@@ -65,12 +65,27 @@ static void spinand_setup_op(const struct spinand_device *spinand,
  	}
  }
 

drivers/mtd/nand/spi/core.c static void spinand_setup_op(const struct spinand_device *spinand, · line 69

Quoted 2 lines

drivers/mtd/nand/spi/core.c +2 -0

+static void spinand_setup_reg_op(const struct spinand_device *spinand,
+				 struct spi_mem_op *op)

Same remark about the naming. In fact I believe we could have this logic in _setup_op() (or whatever its name) and add a specific parameter for it?

drivers/mtd/nand/spi/core.c static void spinand_setup_op(const struct spinand_device *spinand, · line 74

Quoted 5 lines

drivers/mtd/nand/spi/core.c +5 -0

+{
+	if (spinand->reg_proto == SPINAND_OCTAL_DTR) {
+		/*
+		 * Assigning same first and second byte will result in constant
+		 * bits on ths SPI bus between positive and negative clock edges

the

drivers/mtd/nand/spi/core.c static void spinand_setup_op(const struct spinand_device *spinand, · line 76

Quoted 2 lines

drivers/mtd/nand/spi/core.c +2 -0

+		 */
+		op->addr.val = (op->addr.val << 8) | op->addr.val;

I am not sure to understand what you do here?

drivers/mtd/nand/spi/core.c static void spinand_setup_op(const struct spinand_device *spinand, · line 79

Quoted 3 lines

drivers/mtd/nand/spi/core.c +3 -0

+		op->addr.nbytes = 2;
+		op->data.nbytes = 2;
+	}

Space

drivers/mtd/nand/spi/core.c static void spinand_setup_op(const struct spinand_device *spinand, · line 85

Quoted 8 lines

drivers/mtd/nand/spi/core.c +4 -2

+	spinand_setup_op(spinand, op);
+}
+
  static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
  {
-	struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg,
-						      spinand->scratchbuf);
+	struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg, spinand->scratchbuf);

You can do this, but in a different commit.

Quoted 8 lines

drivers/mtd/nand/spi/core.c +1 -0

  	int ret;
 
+	spinand_setup_reg_op(spinand, &op);
  	ret = spi_mem_exec_op(spinand->spimem, &op);
  	if (ret)
  		return ret;

@@ -81,10 +96,10 @@ static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
 

drivers/mtd/nand/spi/core.c static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val) · line 99

Quoted 5 lines

drivers/mtd/nand/spi/core.c +1 -2

  static int spinand_write_reg_op(struct spinand_device *spinand, u8 reg, u8 val)
  {
-	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg,
-						      spinand->scratchbuf);
+	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg, spinand->scratchbuf);

Same here.

Quoted 8 lines

drivers/mtd/nand/spi/core.c +2 -1

-	*spinand->scratchbuf = val;
+	spinand_setup_reg_op(spinand, &op);
+	memset(spinand->scratchbuf, val, op.data.nbytes);
  	return spi_mem_exec_op(spinand->spimem, &op);
  }
 

drivers/mtd/nand/spi/core.c static int spinand_wait(struct spinand_device *spinand, · line 568

Quoted 7 lines

drivers/mtd/nand/spi/core.c +1 -0

@@ -547,6 +562,7 @@ static int spinand_wait(struct spinand_device *spinand,
  	u8 status;
  	int ret;
 
+	spinand_setup_reg_op(spinand, &op);
  	ret = spi_mem_poll_status(spinand->spimem, &op, STATUS_BUSY, 0,
  				  initial_delay_us,
  				  poll_delay_us,

Thanks,
Miquèl

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

Hi Miquèl,

Quoted 29 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:29 +0000:

Quoted 24 lines

In Octal DTR SPI mode, 2 bytes of data gets transmitted over one clock cycle, and half-cycle instruction phases aren't supported yet. So, every DTR spi_mem_op needs to have even nbytes in all phases for non-erratic behaviour from the SPI controller.

The odd length cmd and dummy phases get handled by spimem_setup_op() but the odd length address and data phases need to be handled according to the use case. For example in Octal DTR mode, read register operation has one byte long address and data phase. So it needs to extend it by adding a suitable extra byte in addr and reading 2 bytes of data, discarding the second byte.

Handle address and data phases for Octal DTR mode in read/write register and write volatile configuration register operations by adding a suitable extra byte in the address and data phase.

Create spimem_setup_reg_op() helper function to ease setting up read/write register operations in other functions, e.g. wait().

Signed-off-by Apurva Nandan <[email protected]>

---
  drivers/mtd/nand/spi/core.c | 26 +++++++++++++++++++++-----
  1 file changed, 21 insertions(+), 5 deletions(-)

Quoted 14 lines

drivers/mtd/nand/spi/core.c static void spinand_setup_op(const struct spinand_device *spinand, · line 68

Quoted 9 lines

drivers/mtd/nand/spi/core.c +1 -0

index 2e59faecc8f5..a5334ad34f96 100644
@@ -65,12 +65,27 @@ static void spinand_setup_op(const struct spinand_device *spinand,
  	}
  }  
  >> +static void spinand_setup_reg_op(const struct spinand_device *spinand,  
+				 struct spi_mem_op *op)  

Same remark about the naming. In fact I believe we could have this logic in _setup_op() (or whatever its name) and add a specific parameter for it?

Okay, I will add a parameter in argument and include this logic in _setup_op().

Quoted 8 lines

drivers/mtd/nand/spi/core.c static void spinand_setup_op(const struct spinand_device *spinand, · line 74

Quoted 5 lines

drivers/mtd/nand/spi/core.c +5 -0

+{
+	if (spinand->reg_proto == SPINAND_OCTAL_DTR) {
+		/*
+		 * Assigning same first and second byte will result in constant
+		 * bits on ths SPI bus between positive and negative clock edges  

the

Ok.

Quoted 5 lines

drivers/mtd/nand/spi/core.c static void spinand_setup_op(const struct spinand_device *spinand, · line 76

Quoted 2 lines

drivers/mtd/nand/spi/core.c +2 -0

+		 */
+		op->addr.val = (op->addr.val << 8) | op->addr.val;  

I am not sure to understand what you do here?

In Octal DTR mode, 2 bytes of data are sent in a clock cycle. So, we need to append one extra byte when sending a single byte. This extra byte would be sent on the negative edge.

It will make sense to keep both the bytes same, as when it will be set on the SPI pins, the bits on the SPI IO ports will remain constant between the positive and negative edges (as 1 complete byte is set in one clock edge in MSB order). There are no restrictions by the manufacturers on this, the relevant address byte needs to be on positive edge and second byte on negative edge is don't care.

Define new PAGE_READ_FROM_CACHE and PROG_LOAD op templates for Octal DTR SPI mode. These templates would used in op_variants and

op_templates for defining Octal DTR read from cache and write to cache operations.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---
 include/linux/mtd/spinand.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

include/linux/mtd/spinand.h +6 -0

index ebb19b2cec84..35816b8cfe81 100644
@@ -122,6 +122,12 @@
 		   SPI_MEM_OP_DUMMY(ndummy, 4),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 4))
 
+#define SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(addr, ndummy, buf, len) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9d9d, 8),			\
+		   SPI_MEM_OP_ADDR_DTR(2, addr, 8),			\
+		   SPI_MEM_OP_DUMMY_DTR(ndummy, 8),			\
+		   SPI_MEM_OP_DATA_IN_DTR(len, buf, 8))
+
 #define SPINAND_PROG_EXEC_OP(addr)					\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(0x10, 1),				\
 		   SPI_MEM_OP_ADDR(3, addr, 1),				\

@@ -140,6 +146,12 @@
 		   SPI_MEM_OP_NO_DUMMY,					\
 		   SPI_MEM_OP_DATA_OUT(len, buf, 4))

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:31 +0000:

Quoted 2 lines

Define new PAGE_READ_FROM_CACHE and PROG_LOAD op templates for Octal DTR SPI mode. These templates would used in op_variants and

will be

Quoted 30 lines

op_templates for defining Octal DTR read from cache and write to cache operations.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---
  include/linux/mtd/spinand.h | 12 ++++++++++++
  1 file changed, 12 insertions(+)

include/linux/mtd/spinand.h +6 -0

index ebb19b2cec84..35816b8cfe81 100644
@@ -122,6 +122,12 @@
  		   SPI_MEM_OP_DUMMY(ndummy, 4),				\
  		   SPI_MEM_OP_DATA_IN(len, buf, 4))
 
+#define SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(addr, ndummy, buf, len) \
+	SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9d9d, 8),			\
+		   SPI_MEM_OP_ADDR_DTR(2, addr, 8),			\
+		   SPI_MEM_OP_DUMMY_DTR(ndummy, 8),			\
+		   SPI_MEM_OP_DATA_IN_DTR(len, buf, 8))
+
  #define SPINAND_PROG_EXEC_OP(addr)					\
  	SPI_MEM_OP(SPI_MEM_OP_CMD(0x10, 1),				\
  		   SPI_MEM_OP_ADDR(3, addr, 1),				\

@@ -140,6 +146,12 @@
  		   SPI_MEM_OP_NO_DUMMY,					\
  		   SPI_MEM_OP_DATA_OUT(len, buf, 4))

include/linux/mtd/spinand.h line 157

Quoted 10 lines

include/linux/mtd/spinand.h +6 -0

 
+#define SPINAND_PROG_LOAD_OCTALIO_DTR(reset, addr, buf, len)		\
+	SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, reset ? 0x0202 : 0x8484, 8),	\
+		   SPI_MEM_OP_ADDR_DTR(2, addr, 8),			\
+		   SPI_MEM_OP_NO_DUMMY,					\
+		   SPI_MEM_OP_DATA_OUT_DTR(len, buf, 8))
+
  #define SPINAND_PROTO_BUSWIDTH_MASK	GENMASK(6, 0)
  #define SPINAND_PROTO_DTR_BIT		BIT(7)
 

Thanks,
Miquèl

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

Enable Octal DTR SPI mode, i.e. 8D-8D-8D mode, if the SPI NAND flash device supports it. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes (1S-1D-8D), etc. aren't supported yet.

The method to switch to Octal DTR SPI mode may vary across manufacturers. For example, for Winbond, it is enabled by writing values to the volatile configuration register. So, let the manufacturer's code have their own implementation for switching to Octal DTR SPI mode. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes (1S-1D-8D), etc. aren't supported yet.

Check for the SPI NAND device's support for Octal DTR mode using spinand flags, and if the op_templates allow 8D-8D-8D, call

octal_dtr_enable() manufacturer op. If the SPI controller doesn't supports these modes, the selected op_templates would prevent switching

to the Octal DTR mode. And finally update the spinand reg_proto if success.

Signed-off-by Apurva Nandan <[email protected]>

---
 drivers/mtd/nand/spi/core.c | 46 +++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h |  3 +++
 2 files changed, 49 insertions(+)

drivers/mtd/nand/spi/core.c +13 -0

index 1e619b6d777f..19d8affac058 100644
@@ -256,6 +256,48 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
 			       enable ? CFG_QUAD_ENABLE : 0);
 }
 
+static bool spinand_op_is_octal_dtr(const struct spi_mem_op *op)
+{
+	return  op->cmd.buswidth == 8 && op->cmd.dtr &&
+		op->addr.buswidth == 8 && op->addr.dtr &&
+		op->data.buswidth == 8 && op->data.dtr;
+}
+
+static int spinand_init_octal_dtr_enable(struct spinand_device *spinand)
+{
+	struct device *dev = &spinand->spimem->spi->dev;
+	int ret;
+
+	if (!(spinand->flags & SPINAND_HAS_OCTAL_DTR_BIT))

drivers/mtd/nand/spi/core.c +23 -0

+		return -EINVAL;
+	}
+
+	ret = spinand->manufacturer->ops->octal_dtr_enable(spinand);
+	if (ret) {
+		dev_err(dev,
+			"Failed to enable Octal DTR SPI mode (err = %d)\n",
+			ret);
+		return ret;
+	}
+
+	spinand->reg_proto = SPINAND_OCTAL_DTR;
+
+	dev_dbg(dev,
+		"%s SPI NAND switched to Octal DTR SPI (8D-8D-8D) mode\n",
+		spinand->manufacturer->name);
+	return 0;
+}
+
 static int spinand_ecc_enable(struct spinand_device *spinand,
 			      bool enable)
 {

@@ -1189,6 +1231,10 @@ static int spinand_init_flash(struct spinand_device *spinand)
 	if (ret)
 		return ret;
 
+	ret = spinand_init_octal_dtr_enable(spinand);
+	if (ret)
+		return ret;
+
 	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
 	if (ret)
 		return ret;

include/linux/mtd/spinand.h +2 -0

index 35816b8cfe81..daa2ac5c3110 100644
@@ -271,6 +271,7 @@ struct spinand_devid {
  * @init: initialize a SPI NAND device
  * @adjust_op: modify the ops for any variation in their cmd, address, dummy or
  *	       data phase by the manufacturer
+ * @octal_dtr_enable: switch the SPI NAND flash into Octal DTR SPI mode
  * @cleanup: cleanup a SPI NAND device
  *
  * Each SPI NAND manufacturer driver should implement this interface so that

@@ -280,6 +281,7 @@ struct spinand_manufacturer_ops {
 	int (*init)(struct spinand_device *spinand);
 	void (*adjust_op)(struct spi_mem_op *op,
 			  const enum spinand_proto reg_proto);
+	int (*octal_dtr_enable)(struct spinand_device *spinand);
 	void (*cleanup)(struct spinand_device *spinand);
 };
 

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:32 +0000:

Quoted 10 lines

Enable Octal DTR SPI mode, i.e. 8D-8D-8D mode, if the SPI NAND flash device supports it. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes (1S-1D-8D), etc. aren't supported yet.

The method to switch to Octal DTR SPI mode may vary across manufacturers. For example, for Winbond, it is enabled by writing values to the volatile configuration register. So, let the manufacturer's code have their own implementation for switching to Octal DTR SPI mode. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes (1S-1D-8D), etc. aren't supported yet.

You can drop the final sentence which is a repetition of the previous paragraph.

Quoted 2 lines

Check for the SPI NAND device's support for Octal DTR mode using spinand flags, and if the op_templates allow 8D-8D-8D, call

allows

Quoted 2 lines

octal_dtr_enable() manufacturer op. If the SPI controller doesn't supports these modes, the selected op_templates would prevent switching

will

Quoted 2 lines

to the Octal DTR mode. And finally update the spinand reg_proto if success.

on

Quoted 33 lines

Signed-off-by Apurva Nandan <[email protected]>

---
  drivers/mtd/nand/spi/core.c | 46 +++++++++++++++++++++++++++++++++++++
  include/linux/mtd/spinand.h |  3 +++
  2 files changed, 49 insertions(+)

drivers/mtd/nand/spi/core.c +18 -0

index 1e619b6d777f..19d8affac058 100644
@@ -256,6 +256,48 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
  			       enable ? CFG_QUAD_ENABLE : 0);
  }
 
+static bool spinand_op_is_octal_dtr(const struct spi_mem_op *op)
+{
+	return  op->cmd.buswidth == 8 && op->cmd.dtr &&
+		op->addr.buswidth == 8 && op->addr.dtr &&
+		op->data.buswidth == 8 && op->data.dtr;
+}
+
+static int spinand_init_octal_dtr_enable(struct spinand_device *spinand)
+{
+	struct device *dev = &spinand->spimem->spi->dev;
+	int ret;
+
+	if (!(spinand->flags & SPINAND_HAS_OCTAL_DTR_BIT))
+		return 0;
+
+	if (!(spinand_op_is_octal_dtr(spinand->op_templates.read_cache) &&
+	      spinand_op_is_octal_dtr(spinand->op_templates.write_cache) &&
+	      spinand_op_is_octal_dtr(spinand->op_templates.update_cache)))

drivers/mtd/nand/spi/core.c static int spinand_init_quad_enable(struct spinand_device *spinand) · line 281

Quoted 5 lines

drivers/mtd/nand/spi/core.c +5 -0

+		return 0;
+
+	if (!spinand->manufacturer->ops->octal_dtr_enable) {
+		dev_err(dev,
+			"Missing ->octal_dtr_enable(), unable to switch mode\n");

I don't think we want an error here. Perhaps a debug or info call, but no more.

Quoted 54 lines

drivers/mtd/nand/spi/core.c +23 -0

+		return -EINVAL;
+	}
+
+	ret = spinand->manufacturer->ops->octal_dtr_enable(spinand);
+	if (ret) {
+		dev_err(dev,
+			"Failed to enable Octal DTR SPI mode (err = %d)\n",
+			ret);
+		return ret;
+	}
+
+	spinand->reg_proto = SPINAND_OCTAL_DTR;
+
+	dev_dbg(dev,
+		"%s SPI NAND switched to Octal DTR SPI (8D-8D-8D) mode\n",
+		spinand->manufacturer->name);
+	return 0;
+}
+
  static int spinand_ecc_enable(struct spinand_device *spinand,
  			      bool enable)
  {

@@ -1189,6 +1231,10 @@ static int spinand_init_flash(struct spinand_device *spinand)
  	if (ret)
  		return ret;
 
+	ret = spinand_init_octal_dtr_enable(spinand);
+	if (ret)
+		return ret;
+
  	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
  	if (ret)
  		return ret;

include/linux/mtd/spinand.h +2 -0

index 35816b8cfe81..daa2ac5c3110 100644
@@ -271,6 +271,7 @@ struct spinand_devid {
   * @init: initialize a SPI NAND device
   * @adjust_op: modify the ops for any variation in their cmd, address, dummy or
   *	       data phase by the manufacturer
+ * @octal_dtr_enable: switch the SPI NAND flash into Octal DTR SPI mode
   * @cleanup: cleanup a SPI NAND device
   *
   * Each SPI NAND manufacturer driver should implement this interface so that

@@ -280,6 +281,7 @@ struct spinand_manufacturer_ops {
  	int (*init)(struct spinand_device *spinand);
  	void (*adjust_op)(struct spi_mem_op *op,
  			  const enum spinand_proto reg_proto);
+	int (*octal_dtr_enable)(struct spinand_device *spinand);
  	void (*cleanup)(struct spinand_device *spinand);
  };
 

include/linux/mtd/spinand.h struct spinand_ecc_info { · line 356

Quoted 7 lines

include/linux/mtd/spinand.h +1 -0

@@ -348,6 +350,7 @@ struct spinand_ecc_info {
 
  #define SPINAND_HAS_QE_BIT		BIT(0)
  #define SPINAND_HAS_CR_FEAT_BIT		BIT(1)
+#define SPINAND_HAS_OCTAL_DTR_BIT	BIT(2)
 
  /**
   * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure

Thanks,
Miquèl

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

The SPI NAND core doesn't know how to switch the flash to Octal DTR mode (i.e. which operations to perform). If the manufacturer hasn't implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core wouldn't be able to switch to 8D-8D-8D mode and will also not be able to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write cache op_templates.

So, avoid choosing a Octal DTR SPI op_template for read_cache, write_cache and update_cache operations, if the manufacturer_op octal_dtr_enable() is missing.

Signed-off-by Apurva Nandan <[email protected]>

---
 drivers/mtd/nand/spi/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/mtd/nand/spi/core.c +2 -1

index 19d8affac058..8711e887b795 100644
@@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
 		if (id[0] != manufacturer->id)
 			continue;
 
+		spinand->manufacturer = manufacturer;
+
 		ret = spinand_match_and_init(spinand,
 					     manufacturer->chips,
 					     manufacturer->nchips,

@@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
 		if (ret < 0)
 			continue;
 
-		spinand->manufacturer = manufacturer;
 		return 0;
 	}
 	return -ENOTSUPP;

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33 +0000:

Quoted 7 lines

The SPI NAND core doesn't know how to switch the flash to Octal DTR mode (i.e. which operations to perform). If the manufacturer hasn't implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core wouldn't be able to switch to 8D-8D-8D mode and will also not be able to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write cache op_templates.

Quoted 3 lines

So, avoid choosing a Octal DTR SPI op_template for read_cache, write_cache and update_cache operations, if the manufacturer_op octal_dtr_enable() is missing.

After looking at your previous commit I don't see why this patch would be needed. octal_dtr_enable() only updates the mode when it succeeds so I don't think this patch is really needed.

Quoted 28 lines

Signed-off-by Apurva Nandan <[email protected]>

---
  drivers/mtd/nand/spi/core.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/mtd/nand/spi/core.c +2 -1

index 19d8affac058..8711e887b795 100644
@@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
  		if (id[0] != manufacturer->id)
  			continue;
 
+		spinand->manufacturer = manufacturer;
+
  		ret = spinand_match_and_init(spinand,
  					     manufacturer->chips,
  					     manufacturer->nchips,

@@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
  		if (ret < 0)
  			continue;
 
-		spinand->manufacturer = manufacturer;
  		return 0;
  	}
  	return -ENOTSUPP;

drivers/mtd/nand/spi/core.c spinand_select_op_variant(struct spinand_device *spinand, · line 1107

Quoted 10 lines

drivers/mtd/nand/spi/core.c +4 -0

@@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
  		unsigned int nbytes;
  		int ret;
 
+		if (spinand_op_is_octal_dtr(&op) &&
+		    !spinand->manufacturer->ops->octal_dtr_enable)
+			continue;
+
  		nbytes = nanddev_per_page_oobsize(nand) +
  			 nanddev_page_size(nand);
 

Thanks,
Miquèl

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

Quoted 16 lines

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33 +0000:

Quoted 10 lines

The SPI NAND core doesn't know how to switch the flash to Octal DTR mode (i.e. which operations to perform). If the manufacturer hasn't implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core wouldn't be able to switch to 8D-8D-8D mode and will also not be able to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write cache op_templates.

So, avoid choosing a Octal DTR SPI op_template for read_cache, write_cache and update_cache operations, if the manufacturer_op octal_dtr_enable() is missing.

Quoted 4 lines

After looking at your previous commit I don't see why this patch would be needed. octal_dtr_enable() only updates the mode when it succeeds so I don't think this patch is really needed.

I added it to prevent any errors happening dues to a missing implementation of octal_dtr_enable() from manufacturer driver side. So, if the manufacturers skips the octal_dtr_enable() implementation, we want the spinand core to run in 1s-1s-1s mode.

Read/write/update op variant selection happens in select_op_variant(), much before octal_dtr_enable(). So just check if there is a definition of octal_dtr_enable in manufacturer ops and then only use 8D op variants.

Removing this wouldn't break anything in the current implementation. Do you think we should drop this?

Quoted 37 lines

Quoted 26 lines

Signed-off-by Apurva Nandan <[email protected]>

---
  drivers/mtd/nand/spi/core.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/mtd/nand/spi/core.c +1 -0

index 19d8affac058..8711e887b795 100644
@@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
  		if (id[0] != manufacturer->id)
  			continue;  
  >> +		spinand->manufacturer = manufacturer;  
+
  		ret = spinand_match_and_init(spinand,
  					     manufacturer->chips,
  					     manufacturer->nchips,

@@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
  		if (ret < 0)
  			continue;  
  >> -		spinand->manufacturer = manufacturer;  
  		return 0;
  	}
  	return -ENOTSUPP;

drivers/mtd/nand/spi/core.c spinand_select_op_variant(struct spinand_device *spinand, · line 1106

Quoted 9 lines

drivers/mtd/nand/spi/core.c +3 -0

@@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
  		unsigned int nbytes;
  		int ret;  
  >> +		if (spinand_op_is_octal_dtr(&op) &&  
+		    !spinand->manufacturer->ops->octal_dtr_enable)
+			continue;
+
  		nbytes = nanddev_per_page_oobsize(nand) +
  			 nanddev_page_size(nand);  
  > > Thanks,  

Miquèl

Quoted 4 lines

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

Thanks,
Apurva Nandan

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 16:56:50 +0530:

Quoted 22 lines · Miquel Raynal

Quoted 20 lines

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33 +0000:

Quoted 10 lines

The SPI NAND core doesn't know how to switch the flash to Octal DTR mode (i.e. which operations to perform). If the manufacturer hasn't implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core wouldn't be able to switch to 8D-8D-8D mode and will also not be able to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write cache op_templates.

So, avoid choosing a Octal DTR SPI op_template for read_cache, write_cache and update_cache operations, if the manufacturer_op octal_dtr_enable() is missing.

After looking at your previous commit I don't see why this patch would be needed. octal_dtr_enable() only updates the mode when it succeeds so I don't think this patch is really needed.

Quoted 2 lines

I added it to prevent any errors happening dues to a missing implementation of octal_dtr_enable() from manufacturer driver side. So, if the manufacturers skips the octal_dtr_enable() implementation, we want the spinand core to run in 1s-1s-1s mode.

I still don't get the point: you fail the probe if the octal bit is enabled but the manufacturer did not implement octal_dtr_enable(), so how could we have issues? Maybe I am overlooking something though, but this seemed completely redundant to my eyes so far.

Quoted 48 lines

Read/write/update op variant selection happens in select_op_variant(), much before octal_dtr_enable(). So just check if there is a definition of octal_dtr_enable in manufacturer ops and then only use 8D op variants.

Removing this wouldn't break anything in the current implementation. Do you think we should drop this?

Quoted 41 lines

Quoted 26 lines

Signed-off-by Apurva Nandan <[email protected]>

---
   drivers/mtd/nand/spi/core.c | 7 ++++++-
   1 file changed, 6 insertions(+), 1 deletion(-)

drivers/mtd/nand/spi/core.c +1 -0

index 19d8affac058..8711e887b795 100644
@@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
   		if (id[0] != manufacturer->id)
   			continue;
   >> +		spinand->manufacturer = manufacturer;
+
   		ret = spinand_match_and_init(spinand,
   					     manufacturer->chips,
   					     manufacturer->nchips,

@@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
   		if (ret < 0)
   			continue;
   >> -		spinand->manufacturer = manufacturer;
   		return 0;
   	}
   	return -ENOTSUPP;

drivers/mtd/nand/spi/core.c spinand_select_op_variant(struct spinand_device *spinand, · line 1106

Quoted 9 lines

drivers/mtd/nand/spi/core.c +3 -0

@@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
   		unsigned int nbytes;
   		int ret;
   >> +		if (spinand_op_is_octal_dtr(&op) &&
+		    !spinand->manufacturer->ops->octal_dtr_enable)
+			continue;
+
   		nbytes = nanddev_per_page_oobsize(nand) +
   			 nanddev_page_size(nand);
   > > Thanks,

Miquèl

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

Quoted 2 lines

Thanks,
Apurva Nandan

Thanks,
Miquèl

Hi Miquèl,

Quoted 25 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 16:56:50 +0530:

Quoted 20 lines · Miquel Raynal

Quoted 19 lines

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33 +0000:

Quoted 10 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

The SPI NAND core doesn't know how to switch the flash to Octal DTR

mode (i.e. which operations to perform). If the manufacturer hasn't implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core wouldn't be able to switch to 8D-8D-8D mode and will also not be able to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write cache op_templates.

So, avoid choosing a Octal DTR SPI op_template for read_cache, write_cache and update_cache operations, if the manufacturer_op octal_dtr_enable() is missing.

After looking at your previous commit I don't see why this patch would be needed. octal_dtr_enable() only updates the mode when it succeeds so I don't think this patch is really needed.

Quoted 1 line

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

I added it to prevent any errors happening dues to a missing implementation of octal_dtr_enable() from manufacturer driver side. So, if the manufacturers skips the octal_dtr_enable() implementation, we want the spinand core to run in 1s-1s-1s mode.

I still don't get the point: you fail the probe if the octal bit is enabled but the manufacturer did not implement octal_dtr_enable(), so how could we have issues? Maybe I am overlooking something though, but this seemed completely redundant to my eyes so far.

Okay, I feel this may be redundant. This is for the case when the manufacturer has added Octal DTR read/write/update cache variants but hasn't implemented the octal_dtr_enable() method.

Without this patch, the probe would fail, if the manufacturer did not implement octal_dtr_enable(). But after using this patch, spinand can still use the chip in 1s-1s-1s mode in that case and just skip the Octal DTR op variants during the selection. And also the probe would succeed.

Quoted 54 lines

Quoted 50 lines

Read/write/update op variant selection happens in select_op_variant(), much before octal_dtr_enable(). So just check if there is a definition of octal_dtr_enable in manufacturer ops and then only use 8D op variants.

Removing this wouldn't break anything in the current implementation. Do you think we should drop this?

Quoted 37 lines

Quoted 26 lines

Signed-off-by Apurva Nandan <[email protected]>

---
   drivers/mtd/nand/spi/core.c | 7 ++++++-
   1 file changed, 6 insertions(+), 1 deletion(-)

drivers/mtd/nand/spi/core.c +1 -0

index 19d8affac058..8711e887b795 100644
@@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
   		if (id[0] != manufacturer->id)
   			continue;  
   >> +		spinand->manufacturer = manufacturer;  
+
   		ret = spinand_match_and_init(spinand,
   					     manufacturer->chips,
   					     manufacturer->nchips,

@@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
   		if (ret < 0)
   			continue;  
   >> -		spinand->manufacturer = manufacturer;  
   		return 0;
   	}
   	return -ENOTSUPP;

drivers/mtd/nand/spi/core.c spinand_select_op_variant(struct spinand_device *spinand, · line 1106

Quoted 9 lines

drivers/mtd/nand/spi/core.c +3 -0

@@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
   		unsigned int nbytes;
   		int ret;  
   >> +		if (spinand_op_is_octal_dtr(&op) &&  
+		    !spinand->manufacturer->ops->octal_dtr_enable)
+			continue;
+
   		nbytes = nanddev_per_page_oobsize(nand) +
   			 nanddev_page_size(nand);  
   > > Thanks,  

Miquèl

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

Thanks,
Apurva Nandan

Quoted 3 lines

Thanks,
Miquèl

Thanks,
Apurva Nandan

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 19:24:34 +0530:

Quoted 37 lines

Hi Miquèl,

Quoted 25 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 16:56:50 +0530:

Quoted 20 lines · Miquel Raynal

Quoted 19 lines

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33 +0000:

Quoted 10 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

The SPI NAND core doesn't know how to switch the flash to Octal DTR

mode (i.e. which operations to perform). If the manufacturer hasn't implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core wouldn't be able to switch to 8D-8D-8D mode and will also not be able to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write cache op_templates.

So, avoid choosing a Octal DTR SPI op_template for read_cache, write_cache and update_cache operations, if the manufacturer_op octal_dtr_enable() is missing.

After looking at your previous commit I don't see why this patch would be needed. octal_dtr_enable() only updates the mode when it succeeds so I don't think this patch is really needed.

Quoted 1 line

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

I added it to prevent any errors happening dues to a missing implementation of octal_dtr_enable() from manufacturer driver side. So, if the manufacturers skips the octal_dtr_enable() implementation, we want the spinand core to run in 1s-1s-1s mode.

I still don't get the point: you fail the probe if the octal bit is enabled but the manufacturer did not implement octal_dtr_enable(), so how could we have issues? Maybe I am overlooking something though, but this seemed completely redundant to my eyes so far.

Quoted 3 lines

Okay, I feel this may be redundant. This is for the case when the manufacturer has added Octal DTR read/write/update cache variants but hasn't implemented the octal_dtr_enable() method.

Without this patch, the probe would fail, if the manufacturer did not implement octal_dtr_enable(). But after using this patch, spinand can still use the chip in 1s-1s-1s mode in that case and just skip the Octal DTR op variants during the selection. And also the probe would succeed.

Unless I am overlooking something with this series applied (with or without this patch) the possibilities are:
- no octal bit -> continue as before
- octal bit and vendor callback -> uses octal mode
- octal bit and no vendor callback -> will return an error from spinand_init_octal_dtr_enable() which will fail the probe (patch 7)

Anyway we have a choice:
- Either we consider the tables describing chips as pure descriptions and we can support these chips in mode 1-1-1 (will require changes in your series as this is not what you support as far as I understand the code)
- Or we consider these tables as "what is currently supported" and in this case we just fail if one adds the octal bit without any callback implementation.

I think the latter is better for now. We can update this choice later if needed anyway.

Quoted 58 lines

Quoted 57 lines

Quoted 44 lines

Read/write/update op variant selection happens in select_op_variant(), much before octal_dtr_enable(). So just check if there is a definition of octal_dtr_enable in manufacturer ops and then only use 8D op variants.

Removing this wouldn't break anything in the current implementation. Do you think we should drop this?

Quoted 37 lines

Quoted 26 lines

Signed-off-by Apurva Nandan <[email protected]>

---
    drivers/mtd/nand/spi/core.c | 7 ++++++-
    1 file changed, 6 insertions(+), 1 deletion(-)

drivers/mtd/nand/spi/core.c +1 -0

index 19d8affac058..8711e887b795 100644
@@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
    		if (id[0] != manufacturer->id)
    			continue;
    >> +		spinand->manufacturer = manufacturer;
+
    		ret = spinand_match_and_init(spinand,
    					     manufacturer->chips,
    					     manufacturer->nchips,

@@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
    		if (ret < 0)
    			continue;
    >> -		spinand->manufacturer = manufacturer;
    		return 0;
    	}
    	return -ENOTSUPP;

drivers/mtd/nand/spi/core.c spinand_select_op_variant(struct spinand_device *spinand, · line 1106

Quoted 9 lines

drivers/mtd/nand/spi/core.c +3 -0

@@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
    		unsigned int nbytes;
    		int ret;
    >> +		if (spinand_op_is_octal_dtr(&op) &&
+		    !spinand->manufacturer->ops->octal_dtr_enable)
+			continue;
+
    		nbytes = nanddev_per_page_oobsize(nand) +
    			 nanddev_page_size(nand);
    > > Thanks,

Miquèl

Quoted 6 lines

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

Thanks,
Apurva Nandan

Thanks,
Miquèl

Quoted 2 lines

Thanks,
Apurva Nandan

Thanks,
Miquèl

Volatile configuration register are a different set of configuration registers, i.e. they differ from the status registers. A different SPI instruction is required to write to these registers. Any changes to the Volatile Configuration Register get transferred directly to the Internal Configuration Register and instantly reflect on the device operation.

In Winbond W35N01JW, these volatile configuration register must be configured in order to switch to Octal DTR SPI mode.

Add support for writing to volatile configuration registers using a new WRITE_VCR_OP template.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---
 drivers/mtd/nand/spi/core.c    |  2 +-
 drivers/mtd/nand/spi/winbond.c | 28 ++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h    |  1 +
 3 files changed, 30 insertions(+), 1 deletion(-)

drivers/mtd/nand/spi/core.c +1 -1

index 8711e887b795..f577e72da2c4 100644
@@ -442,7 +442,7 @@ static void spinand_ondie_ecc_save_status(struct nand_device *nand, u8 status)
 		engine_conf->status = status;
 }
 
-static int spinand_write_enable_op(struct spinand_device *spinand)
+int spinand_write_enable_op(struct spinand_device *spinand)
 {
 	struct spi_mem_op op = SPINAND_WR_EN_DIS_OP(true);
 

drivers/mtd/nand/spi/winbond.c +1 -0

index 76684428354e..a7052a9ca171 100644
@@ -7,6 +7,7 @@
  *	Boris Brezillon <[email protected]>
  */
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/mtd/spinand.h>

drivers/mtd/nand/spi/winbond.c +11 -0

+{
+	int ret;
+	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x81, 1),
+					  SPI_MEM_OP_ADDR(3, reg, 1),
+					  SPI_MEM_OP_NO_DUMMY,
+					  SPI_MEM_OP_DATA_OUT(1, spinand->scratchbuf, 1));
+
+	*spinand->scratchbuf = val;
+
+	ret = spinand_write_enable_op(spinand);
+	if (ret)

drivers/mtd/nand/spi/winbond.c +5 -0

+	 */
+	ndelay(150);
+	return 0;
+}
+
 static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
 	.init = winbond_spinand_init,
 };

include/linux/mtd/spinand.h +0 -0

index daa2ac5c3110..21a4e5adcd59 100644

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:34 +0000:

Quoted 51 lines

Volatile configuration register are a different set of configuration registers, i.e. they differ from the status registers. A different SPI instruction is required to write to these registers. Any changes to the Volatile Configuration Register get transferred directly to the Internal Configuration Register and instantly reflect on the device operation.

In Winbond W35N01JW, these volatile configuration register must be configured in order to switch to Octal DTR SPI mode.

Add support for writing to volatile configuration registers using a new WRITE_VCR_OP template.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---
  drivers/mtd/nand/spi/core.c    |  2 +-
  drivers/mtd/nand/spi/winbond.c | 28 ++++++++++++++++++++++++++++
  include/linux/mtd/spinand.h    |  1 +
  3 files changed, 30 insertions(+), 1 deletion(-)

drivers/mtd/nand/spi/core.c +1 -1

index 8711e887b795..f577e72da2c4 100644
@@ -442,7 +442,7 @@ static void spinand_ondie_ecc_save_status(struct nand_device *nand, u8 status)
  		engine_conf->status = status;
  }
 
-static int spinand_write_enable_op(struct spinand_device *spinand)
+int spinand_write_enable_op(struct spinand_device *spinand)
  {
  	struct spi_mem_op op = SPINAND_WR_EN_DIS_OP(true);
 

drivers/mtd/nand/spi/winbond.c +1 -0

index 76684428354e..a7052a9ca171 100644
@@ -7,6 +7,7 @@
   *	Boris Brezillon <[email protected]>
   */
 
+#include <linux/delay.h>
  #include <linux/device.h>
  #include <linux/kernel.h>
  #include <linux/mtd/spinand.h>

@@ -114,6 +115,33 @@ static int winbond_spinand_init(struct spinand_device *spinand)
  	return 0;
  }
 

drivers/mtd/nand/spi/winbond.c static int winbond_spinand_init(struct spinand_device *spinand) · line 118

Quoted 1 line

drivers/mtd/nand/spi/winbond.c +1 -0

+static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)

Maybe a comment to tell people what vcr is?

Quoted 15 lines

drivers/mtd/nand/spi/winbond.c +15 -0

+{
+	int ret;
+	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x81, 1),
+					  SPI_MEM_OP_ADDR(3, reg, 1),
+					  SPI_MEM_OP_NO_DUMMY,
+					  SPI_MEM_OP_DATA_OUT(1, spinand->scratchbuf, 1));
+
+	*spinand->scratchbuf = val;
+
+	ret = spinand_write_enable_op(spinand);
+	if (ret)
+		return ret;
+
+	ret = spi_mem_exec_op(spinand->spimem, &op);
+	if (ret)

drivers/mtd/nand/spi/winbond.c static int winbond_spinand_init(struct spinand_device *spinand) · line 139

Quoted 6 lines

drivers/mtd/nand/spi/winbond.c +6 -0

+		return ret;
+
+	/*
+	 * Write VCR operation doesn't set the busy bit in SR, so can't perform
+	 * a status poll. Minimum time of 50ns is needed to complete the write.
+	 * So, give thrice the minimum required delay.

Isn't there an official maximum time?

Quoted 13 lines

drivers/mtd/nand/spi/winbond.c +5 -0

+	 */
+	ndelay(150);
+	return 0;
+}
+
  static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
  	.init = winbond_spinand_init,
  };

include/linux/mtd/spinand.h +0 -0

index daa2ac5c3110..21a4e5adcd59 100644

include/linux/mtd/spinand.h int spinand_match_and_init(struct spinand_device *spinand, · line 565

Quoted 6 lines

include/linux/mtd/spinand.h +1 -0

@@ -560,5 +560,6 @@ int spinand_match_and_init(struct spinand_device *spinand,
 
  int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
  int spinand_select_target(struct spinand_device *spinand, unsigned int target);
+int spinand_write_enable_op(struct spinand_device *spinand);
 
  #endif /* __LINUX_MTD_SPINAND_H */

Thanks,
Miquèl

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

Add implementation of octal_dtr_enable() manufacturer_ops for Winbond. To switch to Ocatl DTR mode, setting programmable dummy cycles and SPI IO mode using the volatile configuration register is required. To function at max 120MHz SPI clock in Octal DTR mode, 12 programmable dummy clock cycle setting is required. (Default number of dummy cycle are 8 clocks)

Set the programmable dummy cycle to 12 clocks, and SPI IO mode to Octal DTR with Data Strobe in the VCR. Also, perform a READ ID operation in Octal DTR SPI mode to ensure the switch was successful.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---
 drivers/mtd/nand/spi/winbond.c | 42 ++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

drivers/mtd/nand/spi/winbond.c +37 -0

index a7052a9ca171..58cda07c15a0 100644
@@ -16,6 +16,14 @@
 
 #define WINBOND_CFG_BUF_READ		BIT(3)
 
+/* Octal DTR SPI mode (8D-8D-8D) with Data Strobe output*/
+#define WINBOND_IO_MODE_VCR_OCTAL_DTR	0xE7
+#define WINBOND_IO_MODE_VCR_ADDR	0x00
+
+/* Use 12 dummy clk cycles for using Octal DTR SPI at max 120MHZ */
+#define WINBOND_DUMMY_CLK_COUNT		12
+#define WINBOND_DUMMY_CLK_VCR_ADDR	0x01
+
 static SPINAND_OP_VARIANTS(read_cache_variants,
 		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),

@@ -142,8 +150,42 @@ static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)
 	return 0;
 }
 
+static int winbond_spinand_octal_dtr_enable(struct spinand_device *spinand)
+{
+	int ret;
+	struct spi_mem_op op;
+
+	ret = winbond_write_vcr_op(spinand, WINBOND_DUMMY_CLK_VCR_ADDR,
+				   WINBOND_DUMMY_CLK_COUNT);
+	if (ret)
+		return ret;
+
+	ret = winbond_write_vcr_op(spinand, WINBOND_IO_MODE_VCR_ADDR,
+				   WINBOND_IO_MODE_VCR_OCTAL_DTR);
+	if (ret)
+		return ret;
+
+	/* Read flash ID to make sure the switch was successful. */
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9f9f, 8),
+			   SPI_MEM_OP_NO_ADDR,
+			   SPI_MEM_OP_DUMMY_DTR(16, 8),
+			   SPI_MEM_OP_DATA_IN_DTR(SPINAND_MAX_ID_LEN,
+						  spinand->scratchbuf, 8));
+
+	ret = spi_mem_exec_op(spinand->spimem, &op);
+	if (ret)
+		return ret;
+
+	if (memcmp(spinand->scratchbuf, spinand->id.data, SPINAND_MAX_ID_LEN))
+		return -EINVAL;

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:35 +0000:

Quoted 7 lines

Add implementation of octal_dtr_enable() manufacturer_ops for Winbond. To switch to Ocatl DTR mode, setting programmable dummy cycles and SPI IO mode using the volatile configuration register is required. To function at max 120MHz SPI clock in Octal DTR mode, 12 programmable dummy clock cycle setting is required. (Default number of dummy cycle are 8 clocks)

Quoted 3 lines

Set the programmable dummy cycle to 12 clocks, and SPI IO mode to Octal DTR with Data Strobe in the VCR. Also, perform a READ ID operation in Octal DTR SPI mode to ensure the switch was successful.

Commit title should contain "winbond:" (same for the previous patch and possibly next ones as well).

Quoted 59 lines

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---
  drivers/mtd/nand/spi/winbond.c | 42 ++++++++++++++++++++++++++++++++++
  1 file changed, 42 insertions(+)

drivers/mtd/nand/spi/winbond.c +37 -0

index a7052a9ca171..58cda07c15a0 100644
@@ -16,6 +16,14 @@
 
  #define WINBOND_CFG_BUF_READ		BIT(3)
 
+/* Octal DTR SPI mode (8D-8D-8D) with Data Strobe output*/
+#define WINBOND_IO_MODE_VCR_OCTAL_DTR	0xE7
+#define WINBOND_IO_MODE_VCR_ADDR	0x00
+
+/* Use 12 dummy clk cycles for using Octal DTR SPI at max 120MHZ */
+#define WINBOND_DUMMY_CLK_COUNT		12
+#define WINBOND_DUMMY_CLK_VCR_ADDR	0x01
+
  static SPINAND_OP_VARIANTS(read_cache_variants,
  		SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),

@@ -142,8 +150,42 @@ static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)
  	return 0;
  }
 
+static int winbond_spinand_octal_dtr_enable(struct spinand_device *spinand)
+{
+	int ret;
+	struct spi_mem_op op;
+
+	ret = winbond_write_vcr_op(spinand, WINBOND_DUMMY_CLK_VCR_ADDR,
+				   WINBOND_DUMMY_CLK_COUNT);
+	if (ret)
+		return ret;
+
+	ret = winbond_write_vcr_op(spinand, WINBOND_IO_MODE_VCR_ADDR,
+				   WINBOND_IO_MODE_VCR_OCTAL_DTR);
+	if (ret)
+		return ret;
+
+	/* Read flash ID to make sure the switch was successful. */
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9f9f, 8),
+			   SPI_MEM_OP_NO_ADDR,
+			   SPI_MEM_OP_DUMMY_DTR(16, 8),
+			   SPI_MEM_OP_DATA_IN_DTR(SPINAND_MAX_ID_LEN,
+						  spinand->scratchbuf, 8));
+
+	ret = spi_mem_exec_op(spinand->spimem, &op);
+	if (ret)
+		return ret;
+
+	if (memcmp(spinand->scratchbuf, spinand->id.data, SPINAND_MAX_ID_LEN))
+		return -EINVAL;

drivers/mtd/nand/spi/winbond.c static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val) · line 191

Quoted 10 lines

drivers/mtd/nand/spi/winbond.c +5 -0

+
+	return 0;
+}
+
  static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
  	.init = winbond_spinand_init,
+	.octal_dtr_enable = winbond_spinand_octal_dtr_enable,
  };
 
  const struct spinand_manufacturer winbond_spinand_manufacturer = {

Thanks,
Miquèl

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

Manufacturers like Gigadevice and Winbond are adding Power-on-Reset functionality in their SPI NAND flash chips. PoR instruction consists of a 66h command followed by 99h command, and is different from the FFh reset. The reset command FFh just clears the status only registers, while the PoR command erases all the configurations written to the flash and is equivalent to a power-down -> power-up cycle.

Add support for the Power-on-Reset command for any flash that provides this feature.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---

include/linux/mtd/spinand.h +2 -0

  */
 #define SPINAND_READ_INITIAL_DELAY_US	6
 #define SPINAND_READ_POLL_DELAY_US	5

@@ -227,6 +241,8 @@ struct spinand_device;
 #define SPINAND_WRITE_POLL_DELAY_US	15
 #define SPINAND_ERASE_INITIAL_DELAY_US	250
 #define SPINAND_ERASE_POLL_DELAY_US	50
+#define SPINAND_POR_MIN_DELAY_US	1000
+#define SPINAND_POR_MAX_DELAY_US	1200
 
 #define SPINAND_WAITRDY_TIMEOUT_MS	400
 

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36 +0000:

Quoted 14 lines

Manufacturers like Gigadevice and Winbond are adding Power-on-Reset functionality in their SPI NAND flash chips. PoR instruction consists of a 66h command followed by 99h command, and is different from the FFh reset. The reset command FFh just clears the status only registers, while the PoR command erases all the configurations written to the flash and is equivalent to a power-down -> power-up cycle.

Add support for the Power-on-Reset command for any flash that provides this feature.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---

[...]

				\

include/linux/mtd/spinand.h struct spinand_device; · line 233

Quoted 5 lines

include/linux/mtd/spinand.h +1 -0

@@ -218,6 +230,8 @@ struct spinand_device;
   * reading/programming/erasing when the RESET occurs. Since we always
   * issue a RESET when the device is IDLE, 5us is selected for both initial
   * and poll delay.
+ * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us

s/max upto/up to/

include/linux/mtd/spinand.h struct spinand_device; · line 234

Quoted 1 line

include/linux/mtd/spinand.h +1 -0

+ * to 1200 us safely.

I don't really get why, if the maximum is 500, then let's wait for 500us.

Quoted 13 lines

include/linux/mtd/spinand.h +2 -0

   */
  #define SPINAND_READ_INITIAL_DELAY_US	6
  #define SPINAND_READ_POLL_DELAY_US	5

@@ -227,6 +241,8 @@ struct spinand_device;
  #define SPINAND_WRITE_POLL_DELAY_US	15
  #define SPINAND_ERASE_INITIAL_DELAY_US	250
  #define SPINAND_ERASE_POLL_DELAY_US	50
+#define SPINAND_POR_MIN_DELAY_US	1000
+#define SPINAND_POR_MAX_DELAY_US	1200
 
  #define SPINAND_WAITRDY_TIMEOUT_MS	400
 

include/linux/mtd/spinand.h struct spinand_ecc_info { · line 373

Quoted 7 lines

include/linux/mtd/spinand.h +1 -0

@@ -351,6 +367,7 @@ struct spinand_ecc_info {
  #define SPINAND_HAS_QE_BIT		BIT(0)
  #define SPINAND_HAS_CR_FEAT_BIT		BIT(1)
  #define SPINAND_HAS_OCTAL_DTR_BIT	BIT(2)
+#define SPINAND_HAS_POR_CMD_BIT		BIT(3)
 
  /**
   * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure

Thanks,
Miquèl

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

Hi Miquèl,

Quoted 21 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36 +0000:

Quoted 14 lines

Manufacturers like Gigadevice and Winbond are adding Power-on-Reset functionality in their SPI NAND flash chips. PoR instruction consists of a 66h command followed by 99h command, and is different from the FFh reset. The reset command FFh just clears the status only registers, while the PoR command erases all the configurations written to the flash and is equivalent to a power-down -> power-up cycle.

Add support for the Power-on-Reset command for any flash that provides this feature.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---

[...]

Quoted 9 lines

				\  

include/linux/mtd/spinand.h struct spinand_device; · line 233

Quoted 5 lines

include/linux/mtd/spinand.h +1 -0

@@ -218,6 +230,8 @@ struct spinand_device;
   * reading/programming/erasing when the RESET occurs. Since we always
   * issue a RESET when the device is IDLE, 5us is selected for both initial
   * and poll delay.
+ * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us  

s/max upto/up to/

Okay!

Quoted 5 lines

include/linux/mtd/spinand.h struct spinand_device; · line 234

Quoted 1 line

include/linux/mtd/spinand.h +1 -0

+ * to 1200 us safely.  

I don't really get why, if the maximum is 500, then let's wait for 500us.

Generally we keep some margin from the maximum time, no?

Quoted 25 lines

Quoted 16 lines

include/linux/mtd/spinand.h +2 -0

   */
  #define SPINAND_READ_INITIAL_DELAY_US	6
  #define SPINAND_READ_POLL_DELAY_US	5

@@ -227,6 +241,8 @@ struct spinand_device;
  #define SPINAND_WRITE_POLL_DELAY_US	15
  #define SPINAND_ERASE_INITIAL_DELAY_US	250
  #define SPINAND_ERASE_POLL_DELAY_US	50
+#define SPINAND_POR_MIN_DELAY_US	1000
+#define SPINAND_POR_MAX_DELAY_US	1200  
  >>   #define SPINAND_WAITRDY_TIMEOUT_MS	400
  >> @@ -351,6 +367,7 @@ struct spinand_ecc_info {  
  #define SPINAND_HAS_QE_BIT		BIT(0)

#define SPINAND_HAS_CR_FEAT_BIT BIT(1)
#define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
+#define SPINAND_HAS_POR_CMD_BIT BIT(3)

Quoted 2 lines

Quoted 1 line

Quoted 1 line

/**

* struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure

Thanks,
Miquèl

Quoted 4 lines

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

Thanks,
Apurva Nandan

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:09:07 +0530:

Quoted 42 lines

Hi Miquèl,

Quoted 21 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36 +0000:

Quoted 14 lines

Manufacturers like Gigadevice and Winbond are adding Power-on-Reset functionality in their SPI NAND flash chips. PoR instruction consists of a 66h command followed by 99h command, and is different from the FFh reset. The reset command FFh just clears the status only registers, while the PoR command erases all the configurations written to the flash and is equivalent to a power-down -> power-up cycle.

Add support for the Power-on-Reset command for any flash that provides this feature.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---

[...]

Quoted 9 lines

				\

include/linux/mtd/spinand.h struct spinand_device; · line 233

Quoted 5 lines

include/linux/mtd/spinand.h +1 -0

@@ -218,6 +230,8 @@ struct spinand_device;
    * reading/programming/erasing when the RESET occurs. Since we always
    * issue a RESET when the device is IDLE, 5us is selected for both initial
    * and poll delay.
+ * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us

s/max upto/up to/

Okay!

Quoted 5 lines

include/linux/mtd/spinand.h struct spinand_device; · line 234

Quoted 1 line

include/linux/mtd/spinand.h +1 -0

+ * to 1200 us safely.

I don't really get why, if the maximum is 500, then let's wait for 500us.

Quoted 1 line

Generally we keep some margin from the maximum time, no?

Well, yes and no.

If you know that an operation will last Xms and have nothing else to do, then you can take some margin if you are in a probe (called once) but definitely not if you are in a fast path.

Otherwise the best is to have some kind of signaling but I'm not sure you'll have one for the reset op...

Quoted 30 lines

Quoted 29 lines

Quoted 15 lines

include/linux/mtd/spinand.h +2 -0

    */
   #define SPINAND_READ_INITIAL_DELAY_US	6
   #define SPINAND_READ_POLL_DELAY_US	5

@@ -227,6 +241,8 @@ struct spinand_device;
   #define SPINAND_WRITE_POLL_DELAY_US	15
   #define SPINAND_ERASE_INITIAL_DELAY_US	250
   #define SPINAND_ERASE_POLL_DELAY_US	50
+#define SPINAND_POR_MIN_DELAY_US	1000
+#define SPINAND_POR_MAX_DELAY_US	1200
   >>   #define SPINAND_WAITRDY_TIMEOUT_MS	400
   >> @@ -351,6 +367,7 @@ struct spinand_ecc_info {
   #define SPINAND_HAS_QE_BIT		BIT(0)

   #define SPINAND_HAS_CR_FEAT_BIT		BIT(1)
   #define SPINAND_HAS_OCTAL_DTR_BIT	BIT(2)

Quoted 3 lines

+#define SPINAND_HAS_POR_CMD_BIT BIT(3)

Quoted 1 line

Quoted 1 line

/**

* struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure

Thanks,
Miquèl

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

Quoted 2 lines

Thanks,
Apurva Nandan

Thanks,
Miquèl

Hi Miquèl,

Quoted 44 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:09:07 +0530:

Quoted 39 lines

Hi Miquèl,

Quoted 20 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36 +0000:

Quoted 14 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Manufacturers like Gigadevice and Winbond are adding Power-on-Reset

functionality in their SPI NAND flash chips. PoR instruction consists of a 66h command followed by 99h command, and is different from the FFh reset. The reset command FFh just clears the status only registers, while the PoR command erases all the configurations written to the flash and is equivalent to a power-down -> power-up cycle.

Add support for the Power-on-Reset command for any flash that provides this feature.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---

[...]

Quoted 9 lines

				\  

include/linux/mtd/spinand.h struct spinand_device; · line 233

Quoted 5 lines

include/linux/mtd/spinand.h +1 -0

@@ -218,6 +230,8 @@ struct spinand_device;
    * reading/programming/erasing when the RESET occurs. Since we always
    * issue a RESET when the device is IDLE, 5us is selected for both initial
    * and poll delay.
+ * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us  

s/max upto/up to/

Quoted 1 line

Quoted 1 line

Okay!

Quoted 5 lines

Quoted 1 line

+ * to 1200 us safely.

I don't really get why, if the maximum is 500, then let's wait for 500us.

Quoted 1 line

Quoted 1 line

Quoted 8 lines

Quoted 1 line

Generally we keep some margin from the maximum time, no?

Well, yes and no.

If you know that an operation will last Xms and have nothing else to do, then you can take some margin if you are in a probe (called once) but definitely not if you are in a fast path.

I think as PoR reset would be called at every mtd_suspend() call, so we can reduce the delay. And we would be expecting some time gap before the next mtd_resume() call.

Quoted 3 lines

Otherwise the best is to have some kind of signaling but I'm not sure you'll have one for the reset op...

According to public datasheet, it doesn't set the busy bit during reset.

So do you suggest in the favor of removing the delay margin?

Quoted 33 lines

Quoted 32 lines

Quoted 26 lines

Quoted 16 lines

 
    */
   #define SPINAND_READ_INITIAL_DELAY_US	6
   #define SPINAND_READ_POLL_DELAY_US	5

include/linux/mtd/spinand.h +2 -0

@@ -227,6 +241,8 @@ struct spinand_device;
   #define SPINAND_WRITE_POLL_DELAY_US	15
   #define SPINAND_ERASE_INITIAL_DELAY_US	250
   #define SPINAND_ERASE_POLL_DELAY_US	50
+#define SPINAND_POR_MIN_DELAY_US	1000
+#define SPINAND_POR_MAX_DELAY_US	1200  
   >>   #define SPINAND_WAITRDY_TIMEOUT_MS	400
   >> @@ -351,6 +367,7 @@ struct spinand_ecc_info {  
   #define SPINAND_HAS_QE_BIT		BIT(0)

   #define SPINAND_HAS_CR_FEAT_BIT		BIT(1)
   #define SPINAND_HAS_OCTAL_DTR_BIT	BIT(2)

Quoted 3 lines

+#define SPINAND_HAS_POR_CMD_BIT BIT(3)

Quoted 1 line

Quoted 1 line

/**

* struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure

Thanks,
Miquèl

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

Thanks,
Apurva Nandan

Quoted 3 lines

Thanks,
Miquèl

Thanks,
Apurva Nandan

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 19:11:58 +0530:

Quoted 62 lines

Hi Miquèl,

Quoted 44 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:09:07 +0530:

Quoted 39 lines

Hi Miquèl,

Quoted 20 lines · Miquel Raynal

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36 +0000:

Quoted 14 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Manufacturers like Gigadevice and Winbond are adding Power-on-Reset

functionality in their SPI NAND flash chips. PoR instruction consists of a 66h command followed by 99h command, and is different from the FFh reset. The reset command FFh just clears the status only registers, while the PoR command erases all the configurations written to the flash and is equivalent to a power-down -> power-up cycle.

Add support for the Power-on-Reset command for any flash that provides this feature.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by Apurva Nandan <[email protected]>

---

[...]

Quoted 9 lines

				\

include/linux/mtd/spinand.h struct spinand_device; · line 233

Quoted 5 lines

include/linux/mtd/spinand.h +1 -0

@@ -218,6 +230,8 @@ struct spinand_device;
     * reading/programming/erasing when the RESET occurs. Since we always
     * issue a RESET when the device is IDLE, 5us is selected for both initial
     * and poll delay.
+ * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us

s/max upto/up to/

Quoted 1 line

Quoted 1 line

Okay!

Quoted 5 lines

Quoted 1 line

+ * to 1200 us safely.

I don't really get why, if the maximum is 500, then let's wait for 500us.

Quoted 1 line

Quoted 1 line

Quoted 8 lines

Quoted 1 line

Generally we keep some margin from the maximum time, no?

Well, yes and no.

If you know that an operation will last Xms and have nothing else to do, then you can take some margin if you are in a probe (called once) but definitely not if you are in a fast path.

I think as PoR reset would be called at every mtd_suspend() call, so we can reduce the delay. And we would be expecting some time gap before the next mtd_resume() call.

Quoted 3 lines

Otherwise the best is to have some kind of signaling but I'm not sure you'll have one for the reset op...

Quoted 3 lines

According to public datasheet, it doesn't set the busy bit during reset.

So do you suggest in the favor of removing the delay margin?

Well, it's microseconds, maybe you can reduce it a little bit but that will be ok.

Quoted 37 lines

Quoted 36 lines

Quoted 26 lines

Quoted 26 lines

Quoted 16 lines

  
     */
    #define SPINAND_READ_INITIAL_DELAY_US	6
    #define SPINAND_READ_POLL_DELAY_US	5

include/linux/mtd/spinand.h +2 -0

@@ -227,6 +241,8 @@ struct spinand_device;
    #define SPINAND_WRITE_POLL_DELAY_US	15
    #define SPINAND_ERASE_INITIAL_DELAY_US	250
    #define SPINAND_ERASE_POLL_DELAY_US	50
+#define SPINAND_POR_MIN_DELAY_US	1000
+#define SPINAND_POR_MAX_DELAY_US	1200
    >>   #define SPINAND_WAITRDY_TIMEOUT_MS	400
    >> @@ -351,6 +367,7 @@ struct spinand_ecc_info {
    #define SPINAND_HAS_QE_BIT		BIT(0)

    #define SPINAND_HAS_CR_FEAT_BIT		BIT(1)
    #define SPINAND_HAS_OCTAL_DTR_BIT	BIT(2)

Quoted 3 lines

+#define SPINAND_HAS_POR_CMD_BIT BIT(3)

Quoted 1 line

Quoted 1 line

/**

* struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure

Thanks,
Miquèl

Quoted 6 lines

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

Thanks,
Apurva Nandan

Thanks,
Miquèl

Quoted 2 lines

Thanks,
Apurva Nandan

Thanks,
Miquèl

A soft reset using FFh command doesn't erase the flash's configuration and doesn't reset the SPI IO mode also. This can result in the flash being in a different SPI IO mode, e.g. Octal DTR, when resuming from sleep. This would render the flash in an unusable state.

Perform a Power-on-Reset (PoR), if available in the flash, when suspending the device by runtime_pm. This would set the flash to clean

state for reinitialization during resume and would also ensure that it is in standard SPI IO mode (1S-1S-1S) before the resume begins.

Signed-off-by Apurva Nandan <[email protected]>

---
 drivers/mtd/nand/spi/core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/mtd/nand/spi/core.c +15 -0

index 608f4eb85b0a..6fb3aa6af540 100644
@@ -1329,6 +1329,21 @@ static void spinand_mtd_resume(struct mtd_info *mtd)
 	spinand_ecc_enable(spinand, false);
 }
 
+static int spinand_mtd_suspend(struct mtd_info *mtd)
+{
+	struct spinand_device *spinand = mtd_to_spinand(mtd);
+	int ret;
+
+	if (!(spinand->flags & SPINAND_HAS_POR_CMD_BIT))
+		return 0;
+
+	ret = spinand_power_on_rst_op(spinand);
+	if (ret)
+		dev_err(&spinand->spimem->spi->dev, "suspend() failed\n");
+
+	return ret;
+}
+
 static int spinand_init(struct spinand_device *spinand)
 {
 	struct device *dev = &spinand->spimem->spi->dev;

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:37 +0000:

Quoted 4 lines

A soft reset using FFh command doesn't erase the flash's configuration and doesn't reset the SPI IO mode also. This can result in the flash being in a different SPI IO mode, e.g. Octal DTR, when resuming from sleep. This would render the flash in an unusable state.

could put the falsh in?

Quoted 2 lines

Perform a Power-on-Reset (PoR), if available in the flash, when suspending the device by runtime_pm. This would set the flash to clean

I think runtime_pm is something else.

Quoted 2 lines

state for reinitialization during resume and would also ensure that it is in standard SPI IO mode (1S-1S-1S) before the resume begins.

Please add a comment about this to explain why we don't do this reset at resume time.

Quoted 33 lines

Signed-off-by Apurva Nandan <[email protected]>

---
  drivers/mtd/nand/spi/core.c | 16 ++++++++++++++++
  1 file changed, 16 insertions(+)

drivers/mtd/nand/spi/core.c +15 -0

index 608f4eb85b0a..6fb3aa6af540 100644
@@ -1329,6 +1329,21 @@ static void spinand_mtd_resume(struct mtd_info *mtd)
  	spinand_ecc_enable(spinand, false);
  }
 
+static int spinand_mtd_suspend(struct mtd_info *mtd)
+{
+	struct spinand_device *spinand = mtd_to_spinand(mtd);
+	int ret;
+
+	if (!(spinand->flags & SPINAND_HAS_POR_CMD_BIT))
+		return 0;
+
+	ret = spinand_power_on_rst_op(spinand);
+	if (ret)
+		dev_err(&spinand->spimem->spi->dev, "suspend() failed\n");
+
+	return ret;
+}
+
  static int spinand_init(struct spinand_device *spinand)
  {
  	struct device *dev = &spinand->spimem->spi->dev;

drivers/mtd/nand/spi/core.c static int spinand_init(struct spinand_device *spinand) · line 1422

Quoted 7 lines

drivers/mtd/nand/spi/core.c +1 -0

@@ -1401,6 +1416,7 @@ static int spinand_init(struct spinand_device *spinand)
  	mtd->_erase = spinand_mtd_erase;
  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
  	mtd->_resume = spinand_mtd_resume;
+	mtd->_suspend = spinand_mtd_suspend;
 
  	if (nand->ecc.engine) {
  		ret = mtd_ooblayout_count_freebytes(mtd);

Thanks,
Miquèl

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