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

[Takahiro Kuwano <tkuw584924@gmail.com>]

Hi Pratyush,

On 2/24/2021 9:05 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, tkuw584924 at 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
>>
>