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