Re: [PATCH] mtd: spi-nor: spansion: add clear sr fixup for s25fl-l family

From: Takahiro Kuwano
Date: Mon Nov 28 2022 - 23:53:25 EST

Next message: Guenter Roeck: "Re: [PATCH 1/2] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks"
Previous message: Li Jinlin: "Re: [PATCH v2] blk-iocost: fix shift-out-of-bounds in iocg_hick_delay()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Yaliang and Tudor,

On 11/22/2022 7:28 PM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> + Takahiro
>
> Hi, Takahiro,
>
> Would you please review/test this spansion patch?
>
> Thanks,
> ta
>
> On 10/18/22 12:57, yaliang.wang@xxxxxxxxxxxxx wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> From: Yaliang Wang <Yaliang.Wang@xxxxxxxxxxxxx>
>>
>> Spansion S25FL-L family flashes s25fl064l/s25fl128l/s25fl256l can't
>> automatically recover from programming/erase errors, the Status Register
>> error bits inflecting the errors will not change until a Clear Status
>> Register command be executed.
>>
>> Same thing also happens on other Spansion flash families, they've
>> properly handled it, USE_CLSR manufacturer flag was introduced for this
>> purpose, but S25FL-L cannot simply reuse their work, because S25FL-L has
>> the different error bit settings. S25FL-L defines programming/erase
>> error bits in Status Register 2, whereas the other families define the
>> same error bits in Status Register 1, causing S25FL-L needs a different
>> method to handle this problem.
>>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Fixes: 0074a8f3b303 ("mtd: spi-nor: Add support for s25fl128l and s25fl256l")
>> Fixes: d8b494a32889 ("mtd: spi-nor: Add support for Spansion S25FL064L")
>> Signed-off-by: Yaliang Wang <Yaliang.Wang@xxxxxxxxxxxxx>
>> ---
>> drivers/mtd/spi-nor/spansion.c | 119 ++++++++++++++++++++++++++-------
>> 1 file changed, 93 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 0150049007be..8f353ddda5f5 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -14,6 +14,7 @@
>> #define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */
>> #define SPINOR_OP_RD_ANY_REG 0x65 /* Read any register */
>> #define SPINOR_OP_WR_ANY_REG 0x71 /* Write any register */
>> +#define SPINOR_REG_CYPRESS_SR2V 0x00800001
>> #define SPINOR_REG_CYPRESS_CFR1V 0x00800002
>> #define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN BIT(1) /* Quad Enable */
>> #define SPINOR_REG_CYPRESS_CFR2V 0x00800003
>> @@ -25,6 +26,10 @@
>> #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS 0
>> #define SPINOR_OP_CYPRESS_RD_FAST 0xee
>>
>> +/* s25fl-l family specific */
>> +#define S25FL_L_SR2V_P_ERR BIT(5) /* Programming Error Occurred */
>> +#define S25FL_L_SR2V_E_ERR BIT(6) /* Erase Error Occurred */
>> +
>> /* Cypress SPI NOR flash operations. */
>> #define CYPRESS_NOR_WR_ANY_REG_OP(naddr, addr, ndata, buf) \
>> SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 0), \
>> @@ -44,6 +49,29 @@
>> SPI_MEM_OP_NO_DUMMY, \
>> SPI_MEM_OP_NO_DATA)
>>
>> +/**
>> + * spansion_nor_clear_sr() - Clear the Status Register.
>> + * @nor: pointer to 'struct spi_nor'.
>> + */
>> +static void spansion_nor_clear_sr(struct spi_nor *nor)
>> +{
>> + int ret;
>> +
>> + if (nor->spimem) {
>> + struct spi_mem_op op = SPANSION_CLSR_OP;
>> +
>> + 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 int cypress_nor_octal_dtr_en(struct spi_nor *nor)
>> {
>> struct spi_mem_op op;
>> @@ -342,6 +370,65 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>> .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>> };
>>
>> +/**
>> + * s25fl_l_sr_ready_and_clear() - S25FL_L family flashes need to query
>> + * Status Register 1 to check if the flash is ready and clear it if
>> + * there are Programming/Erase errors in Status Register 2.
>> + * @nor: pointer to 'struct spi_nor'.
>> + *
>> + * Return: 1 if ready, 0 if not ready, -errno on errors.
>> + */
>> +static int s25fl_l_sr_ready_and_clear(struct spi_nor *nor)
>> +{
>> + int ret;
>> + u8 addr_mode_nbytes = nor->params->addr_mode_nbytes;
>> + struct spi_mem_op op =
>> + CYPRESS_NOR_RD_ANY_REG_OP(addr_mode_nbytes,
>> + SPINOR_REG_CYPRESS_SR2V,
>> + &nor->bouncebuf[1]);
>> +
RDAR in S25FL-L family requires 8 dummy cycles by default. This is one of
discrepancies in Spansion/Cypress/Infineon SPI NOR families.

In S25FL-L and S25FS-S families, number of dummy cycles in RDAR is same
as read ops (Fast read, Quad output read, etc) latency regardless of
register type, volatile or non-volatile. It is 8 by default.

In SEMPER family (S25HL/HS-T and S28HL/HS-T), number of dummy cycles in
RDAR for Non-volatile registers is same as read ops latency. For volatile
registers, it depends on CFR3[7:6] and is 0 by default.

So, we may want to let CYPRESS_NOR_RD_ANY_REG_OP macro take 'ndummy' param
to handle this...

>> + /* Read Status Register 1 */
>> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> + if (ret)
>> + return ret;
>> +
>> + /* RDSR2 command isn't available in QPI mode, use RDAR instead */
Or use RDSR2 instead. QPI (4-4-4) mode is not supported in this driver.

>> + ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
>> + if (ret)
>> + return ret;
>> +
>> + if (nor->bouncebuf[1] & (S25FL_L_SR2V_P_ERR | S25FL_L_SR2V_E_ERR)) {
>> + if (nor->bouncebuf[1] & S25FL_L_SR2V_E_ERR)
>> + dev_err(nor->dev, "Erase Error occurred\n");
>> + else
>> + dev_err(nor->dev, "Programming Error occurred\n");
>> +
>> + spansion_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);
>> +}
>> +
>> +static void s25fl_l_late_init(struct spi_nor *nor)
>> +{
>> + nor->params->ready = s25fl_l_sr_ready_and_clear;
>> +}
>> +
>> +static const struct spi_nor_fixups s25fl_l_fixups = {
>> + .late_init = s25fl_l_late_init,
>> +};
>> static const struct flash_info spansion_nor_parts[] = {
>> /* Spansion/Cypress -- single (large) sector size only, at least
>> * for the chips listed here (without boot sectors).
>> @@ -428,13 +515,16 @@ static const struct flash_info spansion_nor_parts[] = {
>> NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ) },
>> { "s25fl064l", INFO(0x016017, 0, 64 * 1024, 128)
>> NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>> - FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
>> + FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
>> + .fixups = &s25fl_l_fixups },
>> { "s25fl128l", INFO(0x016018, 0, 64 * 1024, 256)
>> NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>> - FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
>> + FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
>> + .fixups = &s25fl_l_fixups },
>> { "s25fl256l", INFO(0x016019, 0, 64 * 1024, 512)
>> NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>> - FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
>> + FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
>> + .fixups = &s25fl_l_fixups },
How about merging SR2V check into existing spansion_nor_sr_ready_and_clear()?

Introduce new MFR flag like USE_SR2V then

MFR_FLAGS(USE_CLSR|USE_SR2V)

static int spansion_nor_sr_ready_and_clear(struct spi_nor *nor)
{
int ret;

ret = spi_nor_read_sr(nor, nor->bouncebuf);
if (ret)
return ret;

if (nor->info->mfr_flags & USE_SR2V) {
/* check SR2V */
} else {
/* check SR */
}

...
}

This is just my preference. What do you think?

>> { "s25hl512t", INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256)
>> PARSE_SFDP
>> MFR_FLAGS(USE_CLSR)
>> @@ -460,29 +550,6 @@ static const struct flash_info spansion_nor_parts[] = {
>> },
>> };
>>
>> -/**
>> - * spansion_nor_clear_sr() - Clear the Status Register.
>> - * @nor: pointer to 'struct spi_nor'.
>> - */
>> -static void spansion_nor_clear_sr(struct spi_nor *nor)
>> -{
>> - int ret;
>> -
>> - if (nor->spimem) {
>> - struct spi_mem_op op = SPANSION_CLSR_OP;
>> -
>> - 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);
>> -}
>> -
>> /**
>> * spansion_nor_sr_ready_and_clear() - Query the Status Register to see if the
>> * flash is ready for new commands and clear it if there are any errors.
>> --
>> 2.34.1
>>
>

Thanks,
Takahiro

Next message: Guenter Roeck: "Re: [PATCH 1/2] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks"
Previous message: Li Jinlin: "Re: [PATCH v2] blk-iocost: fix shift-out-of-bounds in iocg_hick_delay()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]