[PATCH 1/2] mtd: spi-nor: core: move Spansion SR ready codes out of core

[PATCH 1/2] mtd: spi-nor: core: move Spansion SR ready codes out of core

[yaliang.wang]

From: Yaliang Wang <Yaliang.Wang@windriver.com>

To move manufacture specific checking SR ready codes out of core, set up
an extra method "sr_ready" in struct spi_nor_fixups, by doing this, the
manufactures can fill in and initialize the method it in their own
files, and leaves the core a relatively clean place.

Spansion's checking flash chip ready codes are moved out by applying
forementioned "sr_ready" method. There will be several following-up
patches for moving "xsr" and "fsr" related codes out of core.

Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
---
 drivers/mtd/spi-nor/core.c     | 84 ++++++++++------------------------
 drivers/mtd/spi-nor/core.h     |  8 ++++
 drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  1 -
 4 files changed, 101 insertions(+), 62 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 20df44b753da..74729eb89947 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -157,7 +157,7 @@ static int spi_nor_spimem_exec_op(struct spi_nor *nor, struct spi_mem_op *op)
 	return spi_mem_exec_op(nor->spimem, op);
 }
 
-static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
+int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
 					   u8 *buf, size_t len)
 {
 	if (spi_nor_protocol_is_dtr(nor->reg_proto))
@@ -166,7 +166,7 @@ static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
 	return nor->controller_ops->read_reg(nor, opcode, buf, len);
 }
 
-static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
+int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
 					    const u8 *buf, size_t len)
 {
 	if (spi_nor_protocol_is_dtr(nor->reg_proto))
@@ -175,7 +175,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
 	return nor->controller_ops->write_reg(nor, opcode, buf, len);
 }
 
-static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
+int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
 {
 	if (spi_nor_protocol_is_dtr(nor->write_proto))
 		return -EOPNOTSUPP;
@@ -649,32 +649,7 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
 	return !!(nor->bouncebuf[0] & XSR_RDY);
 }
 
-/**
- * spi_nor_clear_sr() - Clear the Status Register.
- * @nor:	pointer to 'struct spi_nor'.
- */
-static void spi_nor_clear_sr(struct spi_nor *nor)
-{
-	int ret;
-
-	if (nor->spimem) {
-		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
-				   SPI_MEM_OP_NO_ADDR,
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_NO_DATA);
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
-						       NULL, 0);
-	}
-
-	if (ret)
-		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
-}
+static const struct flash_info *spi_nor_read_id(struct spi_nor *nor);
 
 /**
  * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
@@ -690,28 +665,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
 	if (ret)
 		return ret;
 
-	if (nor->flags & SNOR_F_USE_CLSR &&
-	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
-		if (nor->bouncebuf[0] & SR_E_ERR)
-			dev_err(nor->dev, "Erase Error occurred\n");
-		else
-			dev_err(nor->dev, "Programming Error occurred\n");
-
-		spi_nor_clear_sr(nor);
-
-		/*
-		 * WEL bit remains set to one when an erase or page program
-		 * error occurs. Issue a Write Disable command to protect
-		 * against inadvertent writes that can possibly corrupt the
-		 * contents of the memory.
-		 */
-		ret = spi_nor_write_disable(nor);
-		if (ret)
-			return ret;
-
-		return -EIO;
-	}
-
 	return !(nor->bouncebuf[0] & SR_WIP);
 }
 
@@ -792,18 +745,27 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
  */
 static int spi_nor_ready(struct spi_nor *nor)
 {
-	int sr, fsr;
+	const struct flash_info *info = nor->info ? nor->info : spi_nor_read_id(nor);
+
+	if (IS_ERR_OR_NULL(info))
+		return -ENOENT;
+
+	/*Chip specific method for querying whether the flash is ready*/
+	if (info->fixups && info->fixups->sr_ready)
+		return info->fixups->sr_ready(nor);
+
+	/*Manufacture specific method for querying whether the flash is ready*/
+	if (nor->manufacturer && nor->manufacturer->fixups &&
+	    nor->manufacturer->fixups->sr_ready)
+		return nor->manufacturer->fixups->sr_ready(nor);
 
 	if (nor->flags & SNOR_F_READY_XSR_RDY)
-		sr = spi_nor_xsr_ready(nor);
-	else
-		sr = spi_nor_sr_ready(nor);
-	if (sr < 0)
-		return sr;
-	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
-	if (fsr < 0)
-		return fsr;
-	return sr && fsr;
+		return spi_nor_xsr_ready(nor);
+	if (nor->flags & SNOR_F_USE_FSR)
+		return spi_nor_fsr_ready(nor);
+
+	/*Common method for querying whether the flash is ready*/
+	return spi_nor_sr_ready(nor);
 }
 
 /**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index d631ee299de3..36356f27ee92 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -253,6 +253,7 @@ struct spi_nor_flash_parameter {
  *             parameters that could not be extracted by other means (i.e.
  *             when information provided by the SFDP/flash_info tables are
  *             incomplete or wrong).
+ * @sr_ready: special method for querying whether a flash chip is ready.
  *
  * Those hooks can be used to tweak the SPI NOR configuration when the SFDP
  * table is broken or not available.
@@ -264,6 +265,7 @@ struct spi_nor_fixups {
 			 const struct sfdp_bfpt *bfpt,
 			 struct spi_nor_flash_parameter *params);
 	void (*post_sfdp)(struct spi_nor *nor);
+	int (*sr_ready)(struct spi_nor *nor);
 };
 
 struct flash_info {
@@ -471,6 +473,12 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
 			     const struct sfdp_bfpt *bfpt,
 			     struct spi_nor_flash_parameter *params);
 
+int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
+					   u8 *buf, size_t len);
+int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
+					    const u8 *buf, size_t len);
+int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs);
+
 static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
 {
 	return mtd->priv;
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index b0c5521c1e27..67619b64c148 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -18,6 +18,7 @@
 #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN	0x3
 #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
 #define SPINOR_OP_CYPRESS_RD_FAST		0xee
+#define SPINOR_OP_SPANSION_CLSR			0x30
 
 /**
  * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
@@ -175,6 +176,74 @@ static struct spi_nor_fixups s28hs512t_fixups = {
 	.post_bfpt = s28hs512t_post_bfpt_fixup,
 };
 
+/**
+ * spansion_clear_sr() - Clear the Status Register.
+ * @nor:	pointer to 'struct spi_nor'.
+ */
+static void spansion_clear_sr(struct spi_nor *nor)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SPANSION_CLSR, 0),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_NO_DATA);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_SPANSION_CLSR,
+						       NULL, 0);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
+}
+
+/**
+ * spansion_sr_ready() - Spansion specific method for querying the flash to
+ * see if it is ready for new commands.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int spansion_sr_ready(struct spi_nor *nor)
+{
+	u8 *sr = nor->bouncebuf;
+	int ret;
+
+	ret = spi_nor_read_sr(nor, sr);
+	if (ret)
+		return ret;
+
+	if (nor->flags & SNOR_F_USE_CLSR &&
+	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
+		if (nor->bouncebuf[0] & SR_E_ERR)
+			dev_err(nor->dev, "Erase Error occurred\n");
+		else
+			dev_err(nor->dev, "Programming Error occurred\n");
+
+		spansion_clear_sr(nor);
+
+		/*
+		 * WEL bit remains set to one when an erase or page program
+		 * error occurs. Issue a Write Disable command to protect
+		 * against inadvertent writes that can possibly corrupt the
+		 * contents of the memory.
+		 */
+		ret = spi_nor_write_disable(nor);
+		if (ret)
+			return ret;
+
+		return -EIO;
+	}
+
+	return !(sr[0] & SR_WIP);
+}
+
 static int
 s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
 			 const struct sfdp_parameter_header *bfpt_header,
@@ -291,6 +360,7 @@ static void spansion_post_sfdp_fixups(struct spi_nor *nor)
 
 static const struct spi_nor_fixups spansion_fixups = {
 	.post_sfdp = spansion_post_sfdp_fixups,
+	.sr_ready = spansion_sr_ready,
 };
 
 const struct spi_nor_manufacturer spi_nor_spansion = {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index d13958de6d8a..43bd66204fdf 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -100,7 +100,6 @@
 
 /* Used for Spansion flashes only. */
 #define SPINOR_OP_BRWR		0x17	/* Bank register write */
-#define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
 
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
-- 
2.25.1


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

[PATCH 2/2] mtd: spi-nor: spansion: uses CLSR command in S25FL{064|128|256}L chips

[yaliang.wang]

From: Yaliang Wang <Yaliang.Wang@windriver.com>

S25FL{064|128|256}L chips can't recover from errors when program error
or erase error occurs, P_ERR or E_ERR bit will be set to one, WIP bit
will remain set to one, A Clear Status Register command must be sent to
return the device to STANDBY state.

First record of the can't recover error is in commit <c4b3eacc1dfe>
("mtd: spi-nor: Recover from Spansion/Cypress errors"). The error has
been fixed by the commit, however, S25FL-L chips shifted P_ERR and E_ERR
bits from SR1 to SR2, which makes the previous fix no longer work.

Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
---
 drivers/mtd/spi-nor/spansion.c | 96 ++++++++++++++++++++++++++++++++--
 1 file changed, 93 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 67619b64c148..4ed98cb969a5 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -19,7 +19,10 @@
 #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
 #define SPINOR_OP_CYPRESS_RD_FAST		0xee
 #define SPINOR_OP_SPANSION_CLSR			0x30
+#define SPINOR_OP_SPANSION_SR2			0x07
 
+#define S25FL_L_SR2_P_ERR BIT(5)
+#define S25FL_L_SR2_E_ERR BIT(6)
 /**
  * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
@@ -203,6 +206,38 @@ static void spansion_clear_sr(struct spi_nor *nor)
 		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
 }
 
+/**
+ * spansion_read_sr2() - Read the Status Register 2 using the
+ * SPINOR_OP_SPANSION_SR2 (07h) command.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @sr2:	pointer to DMA-able buffer where the value of the
+ *		Status Register 2 will be written.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_read_sr2(struct spi_nor *nor, u8 *sr2)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SPANSION_SR2, 1),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_IN(1, sr2, 1));
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_SPANSION_SR2,
+						    sr2, 1);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d reading SR2\n", ret);
+
+	return ret;
+}
+
 /**
  * spansion_sr_ready() - Spansion specific method for querying the flash to
  * see if it is ready for new commands.
@@ -244,6 +279,55 @@ static int spansion_sr_ready(struct spi_nor *nor)
 	return !(sr[0] & SR_WIP);
 }
 
+/**
+ * s25fl_l_sr_ready() - s25fl_l family specific method for querying the flash
+ * to see if it is ready for new commands.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int s25fl_l_sr_ready(struct spi_nor *nor)
+{
+	u8 *sr = nor->bouncebuf;
+	int ret;
+
+	ret = spi_nor_read_sr(nor, sr);
+	if (ret)
+		return ret;
+
+	ret = spansion_read_sr2(nor, &sr[1]);
+	if (ret)
+		return ret;
+
+	if (nor->flags & SNOR_F_USE_CLSR &&
+			sr[1] & (S25FL_L_SR2_E_ERR | S25FL_L_SR2_P_ERR)) {
+		if (sr[1] & S25FL_L_SR2_E_ERR)
+			dev_err(nor->dev, "Erase Error occurred\n");
+		else
+			dev_err(nor->dev, "Programming Error occurred\n");
+
+		spansion_clear_sr(nor);
+
+		/*
+		 * WEL bit remains set to one when an erase or page program
+		 * error occurs. Issue a Write Disable command to protect
+		 * against inadvertent writes that can possibly corrupt the
+		 * contents of the memory.
+		 */
+		ret = spi_nor_write_disable(nor);
+		if (ret)
+			return ret;
+
+		return -EIO;
+	}
+
+	return !(sr[0] & SR_WIP);
+}
+
+static struct spi_nor_fixups s25fl_l_fixups = {
+	.sr_ready = s25fl_l_sr_ready,
+};
+
 static int
 s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
 			 const struct sfdp_parameter_header *bfpt_header,
@@ -331,13 +415,19 @@ static const struct flash_info spansion_parts[] = {
 			     SECT_4K | SPI_NOR_DUAL_READ) },
 	{ "s25fl064l",  INFO(0x016017,      0,  64 * 1024, 128,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-			     SPI_NOR_4B_OPCODES) },
+			     SPI_NOR_4B_OPCODES | USE_CLSR)
+		.fixups = &s25fl_l_fixups,
+	},
 	{ "s25fl128l",  INFO(0x016018,      0,  64 * 1024, 256,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-			     SPI_NOR_4B_OPCODES) },
+			     SPI_NOR_4B_OPCODES | USE_CLSR)
+		.fixups = &s25fl_l_fixups,
+	},
 	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-			     SPI_NOR_4B_OPCODES) },
+			     SPI_NOR_4B_OPCODES | USE_CLSR)
+		.fixups = &s25fl_l_fixups,
+	},
 	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
 			      SPI_NOR_NO_ERASE) },
 	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,
-- 
2.25.1


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

Re: [PATCH 1/2] mtd: spi-nor: core: move Spansion SR ready codes out of core

[Pratyush Yadav]

On 01/03/21 10:28PM, yaliang.wang@windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang@windriver.com>
> 
> To move manufacture specific checking SR ready codes out of core, set up
> an extra method "sr_ready" in struct spi_nor_fixups, by doing this, the
> manufactures can fill in and initialize the method it in their own
> files, and leaves the core a relatively clean place.
> 
> Spansion's checking flash chip ready codes are moved out by applying
> forementioned "sr_ready" method. There will be several following-up
> patches for moving "xsr" and "fsr" related codes out of core.
> 
> Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
> ---
>  drivers/mtd/spi-nor/core.c     | 84 ++++++++++------------------------
>  drivers/mtd/spi-nor/core.h     |  8 ++++
>  drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |  1 -
>  4 files changed, 101 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 20df44b753da..74729eb89947 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -157,7 +157,7 @@ static int spi_nor_spimem_exec_op(struct spi_nor *nor, struct spi_mem_op *op)
>  	return spi_mem_exec_op(nor->spimem, op);
>  }
>  
> -static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
> +int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
>  					   u8 *buf, size_t len)
>  {
>  	if (spi_nor_protocol_is_dtr(nor->reg_proto))
> @@ -166,7 +166,7 @@ static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
>  	return nor->controller_ops->read_reg(nor, opcode, buf, len);
>  }
>  
> -static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> +int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
>  					    const u8 *buf, size_t len)

The second line is not aligned with the open parenthesis in the above 3 
functions. You can pass "--strict" to checkpatch.pl to check for this.

>  {
>  	if (spi_nor_protocol_is_dtr(nor->reg_proto))
> @@ -175,7 +175,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
>  	return nor->controller_ops->write_reg(nor, opcode, buf, len);
>  }
>  
> -static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
> +int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
>  {
>  	if (spi_nor_protocol_is_dtr(nor->write_proto))
>  		return -EOPNOTSUPP;
> @@ -649,32 +649,7 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
>  	return !!(nor->bouncebuf[0] & XSR_RDY);
>  }
>  
> -/**
> - * spi_nor_clear_sr() - Clear the Status Register.
> - * @nor:	pointer to 'struct spi_nor'.
> - */
> -static void spi_nor_clear_sr(struct spi_nor *nor)
> -{
> -	int ret;
> -
> -	if (nor->spimem) {
> -		struct spi_mem_op op =
> -			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> -				   SPI_MEM_OP_NO_ADDR,
> -				   SPI_MEM_OP_NO_DUMMY,
> -				   SPI_MEM_OP_NO_DATA);
> -
> -		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> -
> -		ret = spi_mem_exec_op(nor->spimem, &op);
> -	} else {
> -		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> -						       NULL, 0);
> -	}
> -
> -	if (ret)
> -		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> -}
> +static const struct flash_info *spi_nor_read_id(struct spi_nor *nor);

This forward declaration is not needed. More on this below.

>  
>  /**
>   * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
> @@ -690,28 +665,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
>  	if (ret)
>  		return ret;
>  
> -	if (nor->flags & SNOR_F_USE_CLSR &&
> -	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> -		if (nor->bouncebuf[0] & SR_E_ERR)
> -			dev_err(nor->dev, "Erase Error occurred\n");
> -		else
> -			dev_err(nor->dev, "Programming Error occurred\n");
> -
> -		spi_nor_clear_sr(nor);
> -
> -		/*
> -		 * WEL bit remains set to one when an erase or page program
> -		 * error occurs. Issue a Write Disable command to protect
> -		 * against inadvertent writes that can possibly corrupt the
> -		 * contents of the memory.
> -		 */
> -		ret = spi_nor_write_disable(nor);
> -		if (ret)
> -			return ret;
> -
> -		return -EIO;
> -	}
> -
>  	return !(nor->bouncebuf[0] & SR_WIP);
>  }
>  
> @@ -792,18 +745,27 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>   */
>  static int spi_nor_ready(struct spi_nor *nor)
>  {
> -	int sr, fsr;
> +	const struct flash_info *info = nor->info ? nor->info : spi_nor_read_id(nor);
> +
> +	if (IS_ERR_OR_NULL(info))
> +		return -ENOENT;

nor->info can't be NULL or invalid. The probe would fail in that case 
and this function would never get executed. No need to check anything 
before using nor->info.


> +
> +	/*Chip specific method for querying whether the flash is ready*/

Nitpick: Please add a space after the '*' when starting a comment and 
before it when ending a comment. Same for all other comments.

> +	if (info->fixups && info->fixups->sr_ready)
> +		return info->fixups->sr_ready(nor);
> +
> +	/*Manufacture specific method for querying whether the flash is ready*/

s/Manufacture/Manufacturer/

> +	if (nor->manufacturer && nor->manufacturer->fixups &&
> +	    nor->manufacturer->fixups->sr_ready)
> +		return nor->manufacturer->fixups->sr_ready(nor);

I don't think nor->info->fixups is the correct place for this hook. 
Those should be about fixing up incorrect or missing information about 
the flash. It should instead be placed in nor->params.

This eliminates the need for the call to 
nor->manufacturer->fixups->sr_ready. Now you can simply call 
nor->params->sr_ready(). The fixup hooks will take care of populating it 
with the correct function based on the flash or manufacturer.

>  
>  	if (nor->flags & SNOR_F_READY_XSR_RDY)
> -		sr = spi_nor_xsr_ready(nor);
> -	else
> -		sr = spi_nor_sr_ready(nor);
> -	if (sr < 0)
> -		return sr;
> -	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
> -	if (fsr < 0)
> -		return fsr;
> -	return sr && fsr;
> +		return spi_nor_xsr_ready(nor);
> +	if (nor->flags & SNOR_F_USE_FSR)
> +		return spi_nor_fsr_ready(nor);
> +
> +	/*Common method for querying whether the flash is ready*/
> +	return spi_nor_sr_ready(nor);

You are no longer taking into account the FSR status correctly. If a 
flash specifies the USE_FSR flag, then spi_nor_fsr_ready() directly 
returns and spi_nor_sr_ready() is not consulted at all. But if the flash 
then populates the sr_ready() hook then it directly the result of 
sr_ready() and spi_nor_fsr_ready() is not consulted at all.

>  }
>  
>  /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index d631ee299de3..36356f27ee92 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -253,6 +253,7 @@ struct spi_nor_flash_parameter {
>   *             parameters that could not be extracted by other means (i.e.
>   *             when information provided by the SFDP/flash_info tables are
>   *             incomplete or wrong).
> + * @sr_ready: special method for querying whether a flash chip is ready.

There is nothing "special" about this. Simply saying "queries whether a 
flash chip is ready or not" should be fine.

>   *
>   * Those hooks can be used to tweak the SPI NOR configuration when the SFDP
>   * table is broken or not available.
> @@ -264,6 +265,7 @@ struct spi_nor_fixups {
>  			 const struct sfdp_bfpt *bfpt,
>  			 struct spi_nor_flash_parameter *params);
>  	void (*post_sfdp)(struct spi_nor *nor);
> +	int (*sr_ready)(struct spi_nor *nor);
>  };
>  
>  struct flash_info {
> @@ -471,6 +473,12 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>  			     const struct sfdp_bfpt *bfpt,
>  			     struct spi_nor_flash_parameter *params);
>  
> +int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
> +					   u8 *buf, size_t len);
> +int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> +					    const u8 *buf, size_t len);
> +int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs);
> +
>  static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
>  {
>  	return mtd->priv;
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index b0c5521c1e27..67619b64c148 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -18,6 +18,7 @@
>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN	0x3
>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
>  #define SPINOR_OP_CYPRESS_RD_FAST		0xee
> +#define SPINOR_OP_SPANSION_CLSR			0x30
>  
>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
> @@ -175,6 +176,74 @@ static struct spi_nor_fixups s28hs512t_fixups = {
>  	.post_bfpt = s28hs512t_post_bfpt_fixup,
>  };
>  
> +/**
> + * spansion_clear_sr() - Clear the Status Register.
> + * @nor:	pointer to 'struct spi_nor'.
> + */
> +static void spansion_clear_sr(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SPANSION_CLSR, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_NO_DATA);
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_SPANSION_CLSR,
> +						       NULL, 0);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> +}

Ok.

> +
> +/**
> + * spansion_sr_ready() - Spansion specific method for querying the flash to
> + * see if it is ready for new commands.
> + * @nor:	pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spansion_sr_ready(struct spi_nor *nor)
> +{
> +	u8 *sr = nor->bouncebuf;
> +	int ret;
> +
> +	ret = spi_nor_read_sr(nor, sr);
> +	if (ret)
> +		return ret;
> +
> +	if (nor->flags & SNOR_F_USE_CLSR &&
> +	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> +		if (nor->bouncebuf[0] & SR_E_ERR)
> +			dev_err(nor->dev, "Erase Error occurred\n");
> +		else
> +			dev_err(nor->dev, "Programming Error occurred\n");
> +
> +		spansion_clear_sr(nor);
> +
> +		/*
> +		 * WEL bit remains set to one when an erase or page program
> +		 * error occurs. Issue a Write Disable command to protect
> +		 * against inadvertent writes that can possibly corrupt the
> +		 * contents of the memory.
> +		 */
> +		ret = spi_nor_write_disable(nor);
> +		if (ret)
> +			return ret;
> +
> +		return -EIO;
> +	}
> +
> +	return !(sr[0] & SR_WIP);
> +}
> +

Ok.

>  static int
>  s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
>  			 const struct sfdp_parameter_header *bfpt_header,
> @@ -291,6 +360,7 @@ static void spansion_post_sfdp_fixups(struct spi_nor *nor)
>  
>  static const struct spi_nor_fixups spansion_fixups = {
>  	.post_sfdp = spansion_post_sfdp_fixups,
> +	.sr_ready = spansion_sr_ready,
>  };
>  
>  const struct spi_nor_manufacturer spi_nor_spansion = {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index d13958de6d8a..43bd66204fdf 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -100,7 +100,6 @@
>  
>  /* Used for Spansion flashes only. */
>  #define SPINOR_OP_BRWR		0x17	/* Bank register write */
> -#define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
>  
>  /* Used for Micron flashes only. */
>  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 1/2] mtd: spi-nor: core: move Spansion SR ready codes out of core

[Vignesh Raghavendra]

Hi,

On 3/1/21 7:58 PM, yaliang.wang@windriver.com wrote:
> +/**
> + * spansion_clear_sr() - Clear the Status Register.
> + * @nor:	pointer to 'struct spi_nor'.
> + */
> +static void spansion_clear_sr(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SPANSION_CLSR, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_NO_DATA);
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_SPANSION_CLSR,
> +						       NULL, 0);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> +}
> +

No, instead introduce a helper in core.c which can handle both
spi_mem_exec_op() and spi_nor_controller_ops_write_reg().

Regards
Vignesh

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

Re: [PATCH 2/2] mtd: spi-nor: spansion: uses CLSR command in S25FL{064|128|256}L chips

[Pratyush Yadav]

On 01/03/21 10:28PM, yaliang.wang@windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang@windriver.com>
> 
> S25FL{064|128|256}L chips can't recover from errors when program error
> or erase error occurs, P_ERR or E_ERR bit will be set to one, WIP bit
> will remain set to one, A Clear Status Register command must be sent to
> return the device to STANDBY state.
> 
> First record of the can't recover error is in commit <c4b3eacc1dfe>
> ("mtd: spi-nor: Recover from Spansion/Cypress errors"). The error has
> been fixed by the commit, however, S25FL-L chips shifted P_ERR and E_ERR
> bits from SR1 to SR2, which makes the previous fix no longer work.
> 
> Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
> ---
>  drivers/mtd/spi-nor/spansion.c | 96 ++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 67619b64c148..4ed98cb969a5 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -19,7 +19,10 @@
>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
>  #define SPINOR_OP_CYPRESS_RD_FAST		0xee
>  #define SPINOR_OP_SPANSION_CLSR			0x30
> +#define SPINOR_OP_SPANSION_SR2			0x07
>  
> +#define S25FL_L_SR2_P_ERR BIT(5)
> +#define S25FL_L_SR2_E_ERR BIT(6)

Nitpick: Align the BIT() part with the lines above so it all looks nice.

>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:		pointer to a 'struct spi_nor'
> @@ -203,6 +206,38 @@ static void spansion_clear_sr(struct spi_nor *nor)
>  		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
>  }
>  
> +/**
> + * spansion_read_sr2() - Read the Status Register 2 using the
> + * SPINOR_OP_SPANSION_SR2 (07h) command.
> + * @nor:	pointer to 'struct spi_nor'.
> + * @sr2:	pointer to DMA-able buffer where the value of the
> + *		Status Register 2 will be written.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_read_sr2(struct spi_nor *nor, u8 *sr2)
> +{
> +	int ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SPANSION_SR2, 1),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_IN(1, sr2, 1));

The datasheet says that this command is not available in QPI mode. The 
flashes set SPI_NOR_QUAD_READ. So they will be put into QPI mode and 
will be unable to execute this, right? Why does this work then?

> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_SPANSION_SR2,
> +						    sr2, 1);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d reading SR2\n", ret);
> +
> +	return ret;
> +}
> +
>  /**
>   * spansion_sr_ready() - Spansion specific method for querying the flash to
>   * see if it is ready for new commands.
> @@ -244,6 +279,55 @@ static int spansion_sr_ready(struct spi_nor *nor)
>  	return !(sr[0] & SR_WIP);
>  }
>  
> +/**
> + * s25fl_l_sr_ready() - s25fl_l family specific method for querying the flash
> + * to see if it is ready for new commands.
> + * @nor:	pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int s25fl_l_sr_ready(struct spi_nor *nor)
> +{
> +	u8 *sr = nor->bouncebuf;
> +	int ret;
> +
> +	ret = spi_nor_read_sr(nor, sr);
> +	if (ret)
> +		return ret;
> +
> +	ret = spansion_read_sr2(nor, &sr[1]);
> +	if (ret)
> +		return ret;

Since SR2 is only used when SNOR_F_USE_CLSR is set, only run 
spansion_read_sr2() when the flag is set to avoid running it for flashes 
that don't need it.

On that note, since this function is called s25fl_l_sr_ready(), you 
already know this flash can use CLSR. So why even check for the flag at 
all?

> +
> +	if (nor->flags & SNOR_F_USE_CLSR &&
> +			sr[1] & (S25FL_L_SR2_E_ERR | S25FL_L_SR2_P_ERR)) {

Align with open parenthesis.

> +		if (sr[1] & S25FL_L_SR2_E_ERR)
> +			dev_err(nor->dev, "Erase Error occurred\n");
> +		else
> +			dev_err(nor->dev, "Programming Error occurred\n");
> +
> +		spansion_clear_sr(nor);
> +
> +		/*
> +		 * WEL bit remains set to one when an erase or page program
> +		 * error occurs. Issue a Write Disable command to protect
> +		 * against inadvertent writes that can possibly corrupt the
> +		 * contents of the memory.
> +		 */
> +		ret = spi_nor_write_disable(nor);
> +		if (ret)
> +			return ret;
> +
> +		return -EIO;
> +	}
> +
> +	return !(sr[0] & SR_WIP);
> +}
> +
> +static struct spi_nor_fixups s25fl_l_fixups = {
> +	.sr_ready = s25fl_l_sr_ready,
> +};
> +
>  static int
>  s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
>  			 const struct sfdp_parameter_header *bfpt_header,
> @@ -331,13 +415,19 @@ static const struct flash_info spansion_parts[] = {
>  			     SECT_4K | SPI_NOR_DUAL_READ) },
>  	{ "s25fl064l",  INFO(0x016017,      0,  64 * 1024, 128,
>  			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -			     SPI_NOR_4B_OPCODES) },
> +			     SPI_NOR_4B_OPCODES | USE_CLSR)
> +		.fixups = &s25fl_l_fixups,
> +	},
>  	{ "s25fl128l",  INFO(0x016018,      0,  64 * 1024, 256,
>  			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -			     SPI_NOR_4B_OPCODES) },
> +			     SPI_NOR_4B_OPCODES | USE_CLSR)
> +		.fixups = &s25fl_l_fixups,
> +	},
>  	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
>  			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -			     SPI_NOR_4B_OPCODES) },
> +			     SPI_NOR_4B_OPCODES | USE_CLSR)
> +		.fixups = &s25fl_l_fixups,
> +	},
>  	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
>  			      SPI_NOR_NO_ERASE) },
>  	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 1/2] mtd: spi-nor: core: move Spansion SR ready codes out of core

[Tudor.Ambarus]

On 3/1/21 9:08 PM, Pratyush Yadav wrote:
>> +     if (nor->manufacturer && nor->manufacturer->fixups &&
>> +         nor->manufacturer->fixups->sr_ready)
>> +             return nor->manufacturer->fixups->sr_ready(nor);
> I don't think nor->info->fixups is the correct place for this hook.
> Those should be about fixing up incorrect or missing information about
> the flash. It should instead be placed in nor->params.
> 

Pratyush is right.

> This eliminates the need for the call to
> nor->manufacturer->fixups->sr_ready. Now you can simply call
> nor->params->sr_ready(). The fixup hooks will take care of populating it
> with the correct function based on the flash or manufacturer.
> 

For operations that require a different sequence of opcodes issued,
or different checks needed, I would use something like from below.

struct spi_nor_ops {
	int (*ready)(struct spi_nor *nor);
};

struct spi_nor_flash_parameter {
	...
	struct spi_nor_erase_map        erase_map;
	struct spi_nor_ops		ops;
	...
};

You'll have to get rid of the SNOR_F_USE_FSR, SNOR_F_READY_XSR_RDY and
SNOR_F_USE_CLSR. You'll init the default spi_nor_ready() in
spi_nor_info_init_params(), and you will overwrite it for the 3 cases
from above in the manufacturer or flash default_init() hook.
You'll then use across the core nor->params->ops.ready().

Not related to your patch, but related with the overall architecture:
there is another case, where the just the opcode differs, where manufacturers
use different hex opcodes for the same command (ex. some manufacturers use 0x35
for reading CR, others use 0x15). That will require a struct spi_nor_opcodes.

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

Re: [PATCH 2/2] mtd: spi-nor: spansion: uses CLSR command in S25FL{064|128|256}L chips

[Yaliang.Wang]

Hi, Pratyush

Appreciate for your carefully review.

On 3/2/21 7:15 PM, Pratyush Yadav wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On 01/03/21 10:28PM,yaliang.wang@windriver.com  wrote:
>> From: Yaliang Wang<Yaliang.Wang@windriver.com>
>>
>> S25FL{064|128|256}L chips can't recover from errors when program error
>> or erase error occurs, P_ERR or E_ERR bit will be set to one, WIP bit
>> will remain set to one, A Clear Status Register command must be sent to
>> return the device to STANDBY state.
>>
>> First record of the can't recover error is in commit <c4b3eacc1dfe>
>> ("mtd: spi-nor: Recover from Spansion/Cypress errors"). The error has
>> been fixed by the commit, however, S25FL-L chips shifted P_ERR and E_ERR
>> bits from SR1 to SR2, which makes the previous fix no longer work.
>>
>> Signed-off-by: Yaliang Wang<Yaliang.Wang@windriver.com>
>> ---
>>   drivers/mtd/spi-nor/spansion.c | 96 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 93 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 67619b64c148..4ed98cb969a5 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -19,7 +19,10 @@
>>   #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS  0
>>   #define SPINOR_OP_CYPRESS_RD_FAST            0xee
>>   #define SPINOR_OP_SPANSION_CLSR                      0x30
>> +#define SPINOR_OP_SPANSION_SR2                       0x07
>>
>> +#define S25FL_L_SR2_P_ERR BIT(5)
>> +#define S25FL_L_SR2_E_ERR BIT(6)
> Nitpick: Align the BIT() part with the lines above so it all looks nice.
>
>>   /**
>>    * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>    * @nor:             pointer to a 'struct spi_nor'
>> @@ -203,6 +206,38 @@ static void spansion_clear_sr(struct spi_nor *nor)
>>                dev_dbg(nor->dev, "error %d clearing SR\n", ret);
>>   }
>>
>> +/**
>> + * spansion_read_sr2() - Read the Status Register 2 using the
>> + * SPINOR_OP_SPANSION_SR2 (07h) command.
>> + * @nor:     pointer to 'struct spi_nor'.
>> + * @sr2:     pointer to DMA-able buffer where the value of the
>> + *           Status Register 2 will be written.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_read_sr2(struct spi_nor *nor, u8 *sr2)
>> +{
>> +     int ret;
>> +
>> +     if (nor->spimem) {
>> +             struct spi_mem_op op =
>> +                     SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SPANSION_SR2, 1),
>> +                                SPI_MEM_OP_NO_ADDR,
>> +                                SPI_MEM_OP_NO_DUMMY,
>> +                                SPI_MEM_OP_DATA_IN(1, sr2, 1));
> The datasheet says that this command is not available in QPI mode. The
> flashes set SPI_NOR_QUAD_READ. So they will be put into QPI mode and
> will be unable to execute this, right? Why does this work then?
Didn't aware of that before, the correct opcode should be RDAR(0x65), 
will correct it in later patches.
>> +
>> +             ret = spi_mem_exec_op(nor->spimem, &op);
>> +     } else {
>> +             ret = nor->controller_ops->read_reg(nor, SPINOR_OP_SPANSION_SR2,
>> +                                                 sr2, 1);
>> +     }
>> +
>> +     if (ret)
>> +             dev_dbg(nor->dev, "error %d reading SR2\n", ret);
>> +
>> +     return ret;
>> +}
>> +
>>   /**
>>    * spansion_sr_ready() - Spansion specific method for querying the flash to
>>    * see if it is ready for new commands.
>> @@ -244,6 +279,55 @@ static int spansion_sr_ready(struct spi_nor *nor)
>>        return !(sr[0] & SR_WIP);
>>   }
>>
>> +/**
>> + * s25fl_l_sr_ready() - s25fl_l family specific method for querying the flash
>> + * to see if it is ready for new commands.
>> + * @nor:     pointer to 'struct spi_nor'.
>> + *
>> + * Return: 1 if ready, 0 if not ready, -errno on errors.
>> + */
>> +static int s25fl_l_sr_ready(struct spi_nor *nor)
>> +{
>> +     u8 *sr = nor->bouncebuf;
>> +     int ret;
>> +
>> +     ret = spi_nor_read_sr(nor, sr);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = spansion_read_sr2(nor, &sr[1]);
>> +     if (ret)
>> +             return ret;
> Since SR2 is only used when SNOR_F_USE_CLSR is set, only run
> spansion_read_sr2() when the flag is set to avoid running it for flashes
> that don't need it.
>
> On that note, since this function is called s25fl_l_sr_ready(), you
> already know this flash can use CLSR. So why even check for the flag at
> all?
You are right, the check can be eliminated.
>> +
>> +     if (nor->flags & SNOR_F_USE_CLSR &&
>> +                     sr[1] & (S25FL_L_SR2_E_ERR | S25FL_L_SR2_P_ERR)) {
> Align with open parenthesis.
>
>> +             if (sr[1] & S25FL_L_SR2_E_ERR)
>> +                     dev_err(nor->dev, "Erase Error occurred\n");
>> +             else
>> +                     dev_err(nor->dev, "Programming Error occurred\n");
>> +
>> +             spansion_clear_sr(nor);
>> +
>> +             /*
>> +              * WEL bit remains set to one when an erase or page program
>> +              * error occurs. Issue a Write Disable command to protect
>> +              * against inadvertent writes that can possibly corrupt the
>> +              * contents of the memory.
>> +              */
>> +             ret = spi_nor_write_disable(nor);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             return -EIO;
>> +     }
>> +
>> +     return !(sr[0] & SR_WIP);
>> +}
>> +
>> +static struct spi_nor_fixups s25fl_l_fixups = {
>> +     .sr_ready = s25fl_l_sr_ready,
>> +};
>> +
>>   static int
>>   s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
>>                         const struct sfdp_parameter_header *bfpt_header,
>> @@ -331,13 +415,19 @@ static const struct flash_info spansion_parts[] = {
>>                             SECT_4K | SPI_NOR_DUAL_READ) },
>>        { "s25fl064l",  INFO(0x016017,      0,  64 * 1024, 128,
>>                             SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>> -                          SPI_NOR_4B_OPCODES) },
>> +                          SPI_NOR_4B_OPCODES | USE_CLSR)
>> +             .fixups = &s25fl_l_fixups,
>> +     },
>>        { "s25fl128l",  INFO(0x016018,      0,  64 * 1024, 256,
>>                             SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>> -                          SPI_NOR_4B_OPCODES) },
>> +                          SPI_NOR_4B_OPCODES | USE_CLSR)
>> +             .fixups = &s25fl_l_fixups,
>> +     },
>>        { "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
>>                             SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>> -                          SPI_NOR_4B_OPCODES) },
>> +                          SPI_NOR_4B_OPCODES | USE_CLSR)
>> +             .fixups = &s25fl_l_fixups,
>> +     },
>>        { "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
>>                              SPI_NOR_NO_ERASE) },
>>        { "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
Best Regards,
Yaliang

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