| Message ID | 20190124112014.20167-3-alexander.sverdlin@nokia.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | spi-nor: Drop uniform erase | expand |
Hi, Alexander, On 01/24/2019 01:21 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote: > Optimize erase time by always using biggest erase size for given erase > request. Do it by removing "sector"-at-a-time erase code. > spi_nor_erase_multi_sectors() seems to be mature enough to handle all > the cases better. Did you check if the upper mtd layers are affected by this change? > > For the above to work backwards-compatible regarding 4-bytes commands > spi_nor_set_4byte_opcodes() has to prepare them always, independent of > spi_nor_has_uniform_erase() flag. > > Remainder check in spi_nor_erase() becomes superfluous because > spi_nor_erase_multi_sectors() performs it anyway. > > The trigger for this change was n25q128a13: enabling SECT_4K increased > erase time of 128k block from 1.763s to 11.335s. When needed, one can disable the 4K sectors with the MTD_SPI_NOR_USE_4K_SECTORS kconfig option. Does this help you? Cheers, ta
Hi Tudor, On 28/02/2019 18:00, Tudor.Ambarus@microchip.com wrote: > On 01/24/2019 01:21 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote: >> Optimize erase time by always using biggest erase size for given erase >> request. Do it by removing "sector"-at-a-time erase code. >> spi_nor_erase_multi_sectors() seems to be mature enough to handle all >> the cases better. > > Did you check if the upper mtd layers are affected by this change? For now I just change static function, I didn't want to remote spi_nor_has_uniform_erase() infrastructure in the first step. So I personally see no problems with my patch. It's also quite well tested up to now. >> For the above to work backwards-compatible regarding 4-bytes commands >> spi_nor_set_4byte_opcodes() has to prepare them always, independent of >> spi_nor_has_uniform_erase() flag. >> >> Remainder check in spi_nor_erase() becomes superfluous because >> spi_nor_erase_multi_sectors() performs it anyway. >> >> The trigger for this change was n25q128a13: enabling SECT_4K increased >> erase time of 128k block from 1.763s to 11.335s. > > When needed, one can disable the 4K sectors with the MTD_SPI_NOR_USE_4K_SECTORS > kconfig option. Does this help you? This very same kernel runs on different boards with different flashes. Seems that more modern flashes are not so slow when 4K sectors are used. Moreover, I truly believe this was a dead code and want to be convinced otherwise before real problems start :)
Hello Tudor, On 01/03/2019 10:50, Alexander Sverdlin wrote: >>> Optimize erase time by always using biggest erase size for given erase >>> request. Do it by removing "sector"-at-a-time erase code. >>> spi_nor_erase_multi_sectors() seems to be mature enough to handle all >>> the cases better. >> Did you check if the upper mtd layers are affected by this change? > For now I just change static function, I didn't want to remote > spi_nor_has_uniform_erase() infrastructure in the first step. > > So I personally see no problems with my patch. It's also quite well > tested up to now. > >>> For the above to work backwards-compatible regarding 4-bytes commands >>> spi_nor_set_4byte_opcodes() has to prepare them always, independent of >>> spi_nor_has_uniform_erase() flag. >>> >>> Remainder check in spi_nor_erase() becomes superfluous because >>> spi_nor_erase_multi_sectors() performs it anyway. >>> >>> The trigger for this change was n25q128a13: enabling SECT_4K increased >>> erase time of 128k block from 1.763s to 11.335s. >> When needed, one can disable the 4K sectors with the MTD_SPI_NOR_USE_4K_SECTORS >> kconfig option. Does this help you? > This very same kernel runs on different boards with different flashes. > Seems that more modern flashes are not so slow when 4K sectors are used. > > Moreover, I truly believe this was a dead code and want to be convinced > otherwise before real problems start :) in the past 10 months I didn't encounter any problems with this patch. Could you take another look on it?
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index c3598f0571cc..a25324a52eae 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -434,6 +434,10 @@ static u8 spi_nor_convert_3to4_erase(u8 opcode) static void spi_nor_set_4byte_opcodes(struct spi_nor *nor) { + struct spi_nor_erase_map *map = &nor->erase_map; + struct spi_nor_erase_type *erase; + int i; + /* Do some manufacturer fixups first */ switch (JEDEC_MFR(nor->info)) { case SNOR_MFR_SPANSION: @@ -450,16 +454,9 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor) nor->program_opcode = spi_nor_convert_3to4_program(nor->program_opcode); nor->erase_opcode = spi_nor_convert_3to4_erase(nor->erase_opcode); - if (!spi_nor_has_uniform_erase(nor)) { - struct spi_nor_erase_map *map = &nor->erase_map; - struct spi_nor_erase_type *erase; - int i; - - for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) { - erase = &map->erase_type[i]; - erase->opcode = - spi_nor_convert_3to4_erase(erase->opcode); - } + for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) { + erase = &map->erase_type[i]; + erase->opcode = spi_nor_convert_3to4_erase(erase->opcode); } } @@ -978,18 +975,11 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { struct spi_nor *nor = mtd_to_spi_nor(mtd); u32 addr, len; - uint32_t rem; int ret; dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr, (long long)instr->len); - if (spi_nor_has_uniform_erase(nor)) { - div_u64_rem(instr->len, mtd->erasesize, &rem); - if (rem) - return -EINVAL; - } - addr = instr->addr; len = instr->len; @@ -1021,28 +1011,6 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) if (ret) goto erase_err; - /* REVISIT in some cases we could speed up erasing large regions - * by using SPINOR_OP_SE instead of SPINOR_OP_BE_4K. We may have set up - * to use "small sector erase", but that's not always optimal. - */ - - /* "sector"-at-a-time erase */ - } else if (spi_nor_has_uniform_erase(nor)) { - while (len) { - write_enable(nor); - - ret = spi_nor_erase_sector(nor, addr); - if (ret) - goto erase_err; - - addr += mtd->erasesize; - len -= mtd->erasesize; - - ret = spi_nor_wait_till_ready(nor); - if (ret) - goto erase_err; - } - /* erase multiple sectors */ } else { ret = spi_nor_erase_multi_sectors(nor, addr, len);
Optimize erase time by always using biggest erase size for given erase request. Do it by removing "sector"-at-a-time erase code. spi_nor_erase_multi_sectors() seems to be mature enough to handle all the cases better. For the above to work backwards-compatible regarding 4-bytes commands spi_nor_set_4byte_opcodes() has to prepare them always, independent of spi_nor_has_uniform_erase() flag. Remainder check in spi_nor_erase() becomes superfluous because spi_nor_erase_multi_sectors() performs it anyway. The trigger for this change was n25q128a13: enabling SECT_4K increased erase time of 128k block from 1.763s to 11.335s. Fixes: 4607777c71 ("mtd: spi-nor: add subsector flag to n25q128a") Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> --- drivers/mtd/spi-nor/spi-nor.c | 46 ++++++----------------------------- 1 file changed, 7 insertions(+), 39 deletions(-)