Re: [PATCH v4 6/9] mtd: spi-nor-core: Add overlaid sector erase feature
Hi Pratyush,
On 2/2/2021 3:56 AM, Pratyush Yadav wrote:
> Hi Takahiro,
>
> On 28/01/21 01:36PM, tkuw584...@gmail.com wrote:
>> From: Takahiro Kuwano
>>
>> 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
>> ---
>> 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, _4k);
>> +}
>> +if (!ret && instr->addr + instr->len == mtd->size) {
>> +instr_4k.addr = mtd->size - instr_4k.len;
>> +ret = spi_nor_erase(mtd, _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
Re: [PATCH v4 6/9] mtd: spi-nor-core: Add overlaid sector erase feature
Hi Takahiro,
On 28/01/21 01:36PM, tkuw584...@gmail.com wrote:
> From: Takahiro Kuwano
>
> 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
> ---
> 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, _4k);
> + }
> + if (!ret && instr->addr + instr->len == mtd->size) {
> + instr_4k.addr = mtd->size - instr_4k.len;
> + ret = spi_nor_erase(mtd, _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?
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?
> +
> + /* 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
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
[PATCH v4 6/9] mtd: spi-nor-core: Add overlaid sector erase feature
From: Takahiro Kuwano
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
---
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, _4k);
+ }
+ if (!ret && instr->addr + instr->len == mtd->size) {
+ instr_4k.addr = mtd->size - instr_4k.len;
+ ret = spi_nor_erase(mtd, _4k);
+ }
+
+ /* 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
