[v5,07/10] mtd: spi-nor-core: Add non-uniform erase for Spansion/Cypress

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
bottom, depending on the device configuration, while U-Boot supports
uniform sector layout only.

The spansion_erase_non_uniform() erases overlaid 4KB sectors,
non-overlaid portion of normal sector, and remaining normal sectors, by
selecting correct erase command and size based on the address to erase
and size of overlaid portion in parameters.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v5:
  - New in v5, introduce spansion_erase_non_uniform() as a replacement
    for spansion_overlaid_erase() in v4

 drivers/mtd/spi/spi-nor-core.c | 72 ++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

On 19/02/21 10:56AM, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
> bottom, depending on the device configuration, while U-Boot supports
> uniform sector layout only.
> 
> The spansion_erase_non_uniform() erases overlaid 4KB sectors,
> non-overlaid portion of normal sector, and remaining normal sectors, by
> selecting correct erase command and size based on the address to erase
> and size of overlaid portion in parameters.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v5:
>   - New in v5, introduce spansion_erase_non_uniform() as a replacement
>     for spansion_overlaid_erase() in v4
> 
>  drivers/mtd/spi/spi-nor-core.c | 72 ++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index e5fc0e7965..46948ed41b 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -793,6 +793,78 @@ erase_err:
>  	return ret;
>  }
>  
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +/**
> + * spansion_erase_non_uniform() - erase non-uniform sectors for Spansion/Cypress
> + *                                chips
> + * @mtd:	pointer to a 'struct mtd_info'
> + * @instr:	pointer to a 'struct erase_info'
> + * @ovlsz_top:	size of overlaid portion at the top address
> + * @ovlsz_btm:	size of overlaid portion at the bottom address
> + *
> + * Erase an address range on the nor chip that can contain 4KB sectors overlaid
> + * on top and/or bottom. The appropriate erase opcode and size are chosen by
> + * address to erase and size of overlaid portion.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_erase_non_uniform(struct mtd_info *mtd,
> +				      struct erase_info *instr, u32 ovlsz_top,
> +				      u32 ovlsz_btm)

Is there any reason why you are not using the nor->erase() hook? As far 
as I can see it should also be able to perform the same erase while 
avoiding all the boilerplate needed for keeping track of address, 
remaining erase, write enable, error handling, etc.

One problem I can see is that you don't always increment address by 
nor->erasesize. That can change depending on which sector got erased. 
spi_nor_erase() can't currently handle that. But IMO a reasonable fix 
for this would be to return the size actually erased in nor->erase(), 
like how it is done for the unix read() and write() system calls. A 
negative value would still mean an error but now a positive return value 
will tell the caller how much data was actually erased.

I think it is a relatively easy refactor and worth doing.

> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	struct spi_mem_op op =
> +		SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
> +			   SPI_MEM_OP_ADDR(nor->addr_width, instr->addr, 1),
> +			   SPI_MEM_OP_NO_DUMMY,
> +			   SPI_MEM_OP_NO_DATA);
> +	u32 len = instr->len;
> +	u32 erasesize;
> +	int ret;

spi_nor_erase() does some sanity checking for instr->len. Even more 
reason to use nor->erase() so we don't have to duplicate that code.

> +
> +	while (len) {
> +		/* 4KB sectors */
> +		if (op.addr.val < ovlsz_btm ||
> +		    op.addr.val >= mtd->size - ovlsz_top) {
> +			op.cmd.opcode = SPINOR_OP_BE_4K;
> +			erasesize = SZ_4K;

Ok.

> +
> +		/* Non-overlaid portion in the normal sector at the bottom */
> +		} else if (op.addr.val == ovlsz_btm) {
> +			op.cmd.opcode = nor->erase_opcode;
> +			erasesize = mtd->erasesize - ovlsz_btm;
> +
> +		/* Non-overlaid portion in the normal sector at the top */
> +		} else if (op.addr.val == mtd->size - mtd->erasesize) {
> +			op.cmd.opcode = nor->erase_opcode;
> +			erasesize = mtd->erasesize - ovlsz_top;

Patch 9/10 does not check for uniform sector size configuration. But if 
the check is to be added later, passing 0 to ovlsz_top and ovlsz_btm 
will do the right thing because erasesize will end up equal to 
mtd->erasesize in both cases. Neat!

> +
> +		/* Normal sectors */
> +		} else {
> +			op.cmd.opcode = nor->erase_opcode;
> +			erasesize = mtd->erasesize;
> +		}

Ok.

> +
> +		write_enable(nor);
> +
> +		ret = spi_mem_exec_op(nor->spi, &op);
> +		if (ret)
> +			break;
> +
> +		op.addr.val += erasesize;
> +		len -= erasesize;

I recall a patch for Linux by Tudor recently that moved some code like 
this to after spi_nor_wait_till_ready(). Let's do so here as well. Of 
course, this will not matter if you are using nor->erase() instead. The 
problem will still be there since spi_nor_erase() also does this but 
that is a separate fix.

> +
> +		ret = spi_nor_wait_till_ready(nor);
> +		if (ret)
> +			break;
> +	}
> +
> +	write_disable(nor);
> +
> +	return ret;
> +}
> +#endif
> +
>  #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
>  /* Write status register and ensure bits in mask match written values */
>  static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
> -- 
> 2.25.1
>

Hi Pratyush,

On 2/24/2021 9:05 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, tkuw584924@gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
>> bottom, depending on the device configuration, while U-Boot supports
>> uniform sector layout only.
>>
>> The spansion_erase_non_uniform() erases overlaid 4KB sectors,
>> non-overlaid portion of normal sector, and remaining normal sectors, by
>> selecting correct erase command and size based on the address to erase
>> and size of overlaid portion in parameters.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v5:
>>   - New in v5, introduce spansion_erase_non_uniform() as a replacement
>>     for spansion_overlaid_erase() in v4
>>
>>  drivers/mtd/spi/spi-nor-core.c | 72 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index e5fc0e7965..46948ed41b 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -793,6 +793,78 @@ erase_err:
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/**
>> + * spansion_erase_non_uniform() - erase non-uniform sectors for Spansion/Cypress
>> + *                                chips
>> + * @mtd:	pointer to a 'struct mtd_info'
>> + * @instr:	pointer to a 'struct erase_info'
>> + * @ovlsz_top:	size of overlaid portion at the top address
>> + * @ovlsz_btm:	size of overlaid portion at the bottom address
>> + *
>> + * Erase an address range on the nor chip that can contain 4KB sectors overlaid
>> + * on top and/or bottom. The appropriate erase opcode and size are chosen by
>> + * address to erase and size of overlaid portion.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_erase_non_uniform(struct mtd_info *mtd,
>> +				      struct erase_info *instr, u32 ovlsz_top,
>> +				      u32 ovlsz_btm)
> 
> Is there any reason why you are not using the nor->erase() hook? As far 
> as I can see it should also be able to perform the same erase while 
> avoiding all the boilerplate needed for keeping track of address, 
> remaining erase, write enable, error handling, etc.
> 
I tried to use the nor-erase() hook, but I saw the erasesize problem
you mentioned below and didn't think about changing the return value of
existing function.

> One problem I can see is that you don't always increment address by 
> nor->erasesize. That can change depending on which sector got erased. 
> spi_nor_erase() can't currently handle that. But IMO a reasonable fix 
> for this would be to return the size actually erased in nor->erase(), 
> like how it is done for the unix read() and write() system calls. A 
> negative value would still mean an error but now a positive return value 
> will tell the caller how much data was actually erased.
> 
> I think it is a relatively easy refactor and worth doing.
> 
I agree. Let me introduce it in v6.

>> +{
>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +	struct spi_mem_op op =
>> +		SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
>> +			   SPI_MEM_OP_ADDR(nor->addr_width, instr->addr, 1),
>> +			   SPI_MEM_OP_NO_DUMMY,
>> +			   SPI_MEM_OP_NO_DATA);
>> +	u32 len = instr->len;
>> +	u32 erasesize;
>> +	int ret;
> 
> spi_nor_erase() does some sanity checking for instr->len. Even more 
> reason to use nor->erase() so we don't have to duplicate that code.
> 
>> +
>> +	while (len) {
>> +		/* 4KB sectors */
>> +		if (op.addr.val < ovlsz_btm ||
>> +		    op.addr.val >= mtd->size - ovlsz_top) {
>> +			op.cmd.opcode = SPINOR_OP_BE_4K;
>> +			erasesize = SZ_4K;
> 
> Ok.
> 
>> +
>> +		/* Non-overlaid portion in the normal sector at the bottom */
>> +		} else if (op.addr.val == ovlsz_btm) {
>> +			op.cmd.opcode = nor->erase_opcode;
>> +			erasesize = mtd->erasesize - ovlsz_btm;
>> +
>> +		/* Non-overlaid portion in the normal sector at the top */
>> +		} else if (op.addr.val == mtd->size - mtd->erasesize) {
>> +			op.cmd.opcode = nor->erase_opcode;
>> +			erasesize = mtd->erasesize - ovlsz_top;
> 
> Patch 9/10 does not check for uniform sector size configuration. But if 
> the check is to be added later, passing 0 to ovlsz_top and ovlsz_btm 
> will do the right thing because erasesize will end up equal to 
> mtd->erasesize in both cases. Neat!
> 
>> +
>> +		/* Normal sectors */
>> +		} else {
>> +			op.cmd.opcode = nor->erase_opcode;
>> +			erasesize = mtd->erasesize;
>> +		}
> 
> Ok.
> 
>> +
>> +		write_enable(nor);
>> +
>> +		ret = spi_mem_exec_op(nor->spi, &op);
>> +		if (ret)
>> +			break;
>> +
>> +		op.addr.val += erasesize;
>> +		len -= erasesize;
> 
> I recall a patch for Linux by Tudor recently that moved some code like 
> this to after spi_nor_wait_till_ready(). Let's do so here as well. Of 
> course, this will not matter if you are using nor->erase() instead. The 
> problem will still be there since spi_nor_erase() also does this but 
> that is a separate fix.
> 
>> +
>> +		ret = spi_nor_wait_till_ready(nor);
>> +		if (ret)
>> +			break;
>> +	}
>> +
>> +	write_disable(nor);
>> +
>> +	return ret;
>> +}
>> +#endif
>> +
>>  #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
>>  /* Write status register and ensure bits in mask match written values */
>>  static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
>> -- 
>> 2.25.1
>>
>