diff mbox series

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

Message ID 20210301142844.1089385-2-yaliang.wang@windriver.com
State Changes Requested
Delegated to: Ambarus Tudor
Headers show
Series [1/2] mtd: spi-nor: core: move Spansion SR ready codes out of core | expand

Commit Message

Yaliang.Wang March 1, 2021, 2:28 p.m. UTC
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(-)

Comments

Pratyush Yadav March 2, 2021, 11:15 a.m. UTC | #1
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,
Yaliang.Wang April 1, 2021, 6:44 p.m. UTC | #2
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
diff mbox series

Patch

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,