[PATCH 2/2] mtd: spi-nor: spansion: uses CLSR command in S25FL{064|128|256}L chips
Yaliang.Wang
Yaliang.Wang at windriver.com
Thu Apr 1 19:44:27 BST 2021
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 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?
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
More information about the linux-mtd
mailing list