[PATCH 0/2] spi-nor: Drop uniform erase

[PATCH 0/2] spi-nor: Drop uniform erase

[Sverdlin, Alexander (Nokia - DE/Ulm)]

A series which initially started as n25q128a13 erase time regression
analysis but ended up in removing uniform erase completely.

It has been tested on the following name/ID combinations:

n25q128a13:
JEDEC ID 20 ba 18 10 00 00
JEDEC ID 20 ba 18 10 40 00
JEDEC ID 20 ba 18 10 44 00

n25q128a11:
JEDEC ID 20 bb 18 10 44 00

s25fl129p1:
JEDEC ID 01 20 18 4d 01 81

Alexander Sverdlin (2):
  mtd: spi-nor: Fix comment of spi_nor_find_best_erase_type()
  mtd: spi-nor: Always use biggest erase size

 drivers/mtd/spi-nor/spi-nor.c | 48 ++++++-----------------------------
 1 file changed, 8 insertions(+), 40 deletions(-)

-- 
2.19.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] mtd: spi-nor: Fix comment of spi_nor_find_best_erase_type()

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Erase types are sorted *smallest* type first, refer to
spi_nor_sort_erase_mask().

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 6e13bbd1aaa5..c3598f0571cc 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -739,7 +739,7 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
 	u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
 
 	/*
-	 * Erase types are ordered by size, with the biggest erase type at
+	 * Erase types are ordered by size, with the smallest erase type at
 	 * index 0.
 	 */
 	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
-- 
2.19.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Sverdlin, Alexander (Nokia - DE/Ulm)]

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

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Tudor.Ambarus]

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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Sverdlin, Alexander (Nokia - DE/Ulm)]

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

-- 
Best regards,
Alexander Sverdlin.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: Fix comment of spi_nor_find_best_erase_type()

[Tudor.Ambarus]

On 01/24/2019 01:21 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> Erase types are sorted *smallest* type first, refer to
> spi_nor_sort_erase_mask().
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Please fix your author name to match your S-o-b tag. See
https://lkml.org/lkml/2019/2/8/556 for a workaround.

I'll add my R-b after this change.

I didn't forget about patch 2/2, I just want to check the upper mtd layers.

Thanks,
ta
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 6e13bbd1aaa5..c3598f0571cc 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -739,7 +739,7 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>  	u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>  
>  	/*
> -	 * Erase types are ordered by size, with the biggest erase type at
> +	 * Erase types are ordered by size, with the smallest erase type at
>  	 * index 0.
>  	 */
>  	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 0/2] spi-nor: Drop uniform erase

[Sverdlin, Alexander (Nokia - DE/Ulm)]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

A series which initially started as n25q128a13 erase time regression
analysis but ended up in removing uniform erase completely.

It has been tested on the following name/ID combinations:

n25q128a13:
JEDEC ID 20 ba 18 10 00 00
JEDEC ID 20 ba 18 10 40 00
JEDEC ID 20 ba 18 10 44 00

n25q128a11:
JEDEC ID 20 bb 18 10 44 00

s25fl129p1:
JEDEC ID 01 20 18 4d 01 81

Alexander Sverdlin (2):
  mtd: spi-nor: Fix comment of spi_nor_find_best_erase_type()
  mtd: spi-nor: Always use biggest erase size

 drivers/mtd/spi-nor/spi-nor.c | 48 ++++++-----------------------------
 1 file changed, 8 insertions(+), 40 deletions(-)

-- 
2.19.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] mtd: spi-nor: Fix comment of spi_nor_find_best_erase_type()

[Sverdlin, Alexander (Nokia - DE/Ulm)]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Erase types are sorted *smallest* type first, refer to
spi_nor_sort_erase_mask().

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 6e13bbd1aaa5..c3598f0571cc 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -739,7 +739,7 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
 	u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
 
 	/*
-	 * Erase types are ordered by size, with the biggest erase type at
+	 * Erase types are ordered by size, with the smallest erase type at
 	 * index 0.
 	 */
 	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
-- 
2.19.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Sverdlin, Alexander (Nokia - DE/Ulm)]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

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

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: Fix comment of spi_nor_find_best_erase_type()

[Tudor.Ambarus]

On 03/19/2019 06:57 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> External E-Mail
> 
> 
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Erase types are sorted *smallest* type first, refer to
> spi_nor_sort_erase_mask().
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 6e13bbd1aaa5..c3598f0571cc 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -739,7 +739,7 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>  	u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>  
>  	/*
> -	 * Erase types are ordered by size, with the biggest erase type at
> +	 * Erase types are ordered by size, with the smallest erase type at
>  	 * index 0.
>  	 */
>  	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: spi-nor: Fix comment of spi_nor_find_best_erase_type()

[Tudor.Ambarus]

On 03/19/2019 06:57 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Erase types are sorted *smallest* type first, refer to
> spi_nor_sort_erase_mask().
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to http://git.infradead.org/linux-mtd.git, spi-nor/next. Thanks.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Alexander Sverdlin]

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?

-- 
Best regards,
Alexander Sverdlin.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/