[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 at windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang at 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 at 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.