[PATCH v4 6/9] mtd: spi-nor-core: Add overlaid sector erase feature

[Takahiro Kuwano]

Hi Pratyush,

On 2/2/2021 3:56 AM, Pratyush Yadav wrote:
> Hi Takahiro,
> 
> On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano at 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. This patch adds an erase hook that emulates
>> uniform sector layout.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 48 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 1c0ba5abf9..70da0081b6 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -788,6 +788,54 @@ erase_err:
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/*
>> + * Erase for Spansion/Cypress Flash devices that has overlaid 4KB sectors at
>> + * the top and/or bottom.
>> + */
>> +static int spansion_overlaid_erase(struct mtd_info *mtd,
>> +				   struct erase_info *instr)
>> +{
>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +	struct erase_info instr_4k;
>> +	u8 opcode;
>> +	u32 erasesize;
>> +	int ret;
>> +
>> +	/* Perform default erase operation (non-overlaid portion is erased) */
>> +	ret = spi_nor_erase(mtd, instr);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Backup default erase opcode and size */
>> +	opcode = nor->erase_opcode;
>> +	erasesize = mtd->erasesize;
>> +
>> +	/*
>> +	 * Erase 4KB sectors. Use the possible max length of 4KB sector region.
>> +	 * The Flash just ignores the command if the address is not configured
>> +	 * as 4KB sector and reports ready status immediately.
>> +	 */
>> +	instr_4k.len = SZ_128K;
>> +	nor->erase_opcode = SPINOR_OP_BE_4K_4B;
>> +	mtd->erasesize = SZ_4K;
>> +	if (instr->addr == 0) {
>> +		instr_4k.addr = 0;
>> +		ret = spi_nor_erase(mtd, &instr_4k);
>> +	}
>> +	if (!ret && instr->addr + instr->len == mtd->size) {
>> +		instr_4k.addr = mtd->size - instr_4k.len;
>> +		ret = spi_nor_erase(mtd, &instr_4k);
>> +	}
> 
> This feels like a hack to me. Does the flash datasheet explicitly say 
> that erasing the overlaid area with the "normal" erase opcode is a 
> no-op?
> 
Sorry, I have noticed the datasheet I mentioned in the cover letter is a
summary version. Here is the link to he full version, but you need
registration to access.
https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522

So, let me quote the description about erasing overlaid area:

"If a sector erase transaction is applied to a 256 KB sector that is
overlaid by 4 KB sectors, the overlaid 4 KB sectors are not affected
by the erase. Only the visible (non-overlaid) portion of the 128 KB
or 192 KB sector is erased."


And about 4 KB sector erase:

"This transaction is ignored when the device is configured for uniform
sector only (CFR3V[3] = 1). If the Erase 4K B sector transaction is issued
to a non-4 KB sector address, the device will abort the operation and will
not set the ERSERR status fail bit."


> I don't see a big reason to run this hack. You are already in a 
> flash-specific erase hook. Why not just directly issue the correct erase 
> commands to the sectors? That is, why not issue 4k erase to overlaid 
> sectors and normal erase to the rest? Why do you need to emulate uniform 
> erase?
> 
Thanks for pointing this out. I should probably hook nor->erase() instead
of mtd->_erase(), then issue 4k or normal erase depending on the address.
I will introduce that in v5. 

>> +
>> +	/* Restore erase opcode and size */
>> +	nor->erase_opcode = opcode;
>> +	mtd->erasesize = erasesize;
>> +
>> +	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
>>
> 

Best Regards,
Takahiro