[2/2] mtd: spi-nor: Always use biggest erase size

Message ID 20190124112014.20167-3-alexander.sverdlin@nokia.com
State Changes Requested
Delegated to: Ambarus Tudor
Headers show
Series
  • spi-nor: Drop uniform erase
Related show

Commit Message

Sverdlin, Alexander (Nokia - DE/Ulm) Jan. 24, 2019, 11:21 a.m.
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(-)

Comments

Ambarus Tudor Feb. 28, 2019, 5 p.m. | #1
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
Sverdlin, Alexander (Nokia - DE/Ulm) March 1, 2019, 9:50 a.m. | #2
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 :)

Patch

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);