Re: [PATCH v2 09/41] mtd: spi-nor: push 4k SE handling into spi_nor_select_uniform_erase()

[Tudor Ambarus]

On 8/22/23 08:09, Michael Walle wrote:
> 4k sector erase sizes are only a thing with uniform erase types. Push
> the "we want 4k erase sizes" handling into spi_nor_select_uniform_erase().
> 
> One might wonder why the former sector_size isn't used anymore. It is
> because we either search for the largest erase size or if selected
> through kconfig, the 4k erase size. Now, why is that correct? For this,
> we have to differentiate between (1) flashes with SFDP and (2) without
> SFDP. For (1), we just set one (or two if SECT_4K is set) erase types
> and wanted_size is exactly one of these.
> 
> For (2) things are a bit more complicated. For flashes which we don't

:)

> have in our flash_info database, the generic driver is used and
> sector_size was already 0, which in turn selected the largest erase
> size. For flashes which had SFDP and an entry in flash_info, sector_size
> was always the largest sector and thus the largest erase type.
> 
> Signed-off-by: Michael Walle <mwalle@xxxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/core.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 68baf6032639..c84be791341e 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2512,13 +2512,6 @@ static int spi_nor_select_pp(struct spi_nor *nor,
>  /**
>   * spi_nor_select_uniform_erase() - select optimum uniform erase type
>   * @map:		the erase map of the SPI NOR
> - * @wanted_size:	the erase type size to search for. Contains the value of
> - *			info->sector_size, the "small sector" size in case
> - *			CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined or 0 if
> - *			there is no information about the sector size. The
> - *			latter is the case if the flash parameters are parsed
> - *			solely by SFDP, then the largest supported erase type
> - *			is selected.
>   *
>   * Once the optimum uniform sector erase command is found, disable all the
>   * other.
> @@ -2526,13 +2519,16 @@ static int spi_nor_select_pp(struct spi_nor *nor,
>   * Return: pointer to erase type on success, NULL otherwise.
>   */
>  static const struct spi_nor_erase_type *
> -spi_nor_select_uniform_erase(struct spi_nor_erase_map *map,
> -			     const u32 wanted_size)
> +spi_nor_select_uniform_erase(struct spi_nor_erase_map *map)
>  {
>  	const struct spi_nor_erase_type *tested_erase, *erase = NULL;
>  	int i;
>  	u8 uniform_erase_type = map->uniform_erase_type;
>  
> +	/*
> +	 * Search for the biggest erase size, except for when compiled
> +	 * to use 4k erases.
> +	 */
>  	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>  		if (!(uniform_erase_type & BIT(i)))
>  			continue;
> @@ -2544,10 +2540,11 @@ spi_nor_select_uniform_erase(struct spi_nor_erase_map *map,
>  			continue;
>  
>  		/*
> -		 * If the current erase size is the one, stop here:
> +		 * If the current erase size is the 4k one, stop here,
>  		 * we have found the right uniform Sector Erase command.
>  		 */
> -		if (tested_erase->size == wanted_size) {
> +		if (IS_ENABLED(CONFIG_MTD_SPI_NOR_USE_4K_SECTORS) &&

one difference is that the is enabled check may now be evaluated for
each erase type, but looks cleaner and I can live with that as this is
set at probe time anyway.

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

> +		    tested_erase->size == SZ_4K) {
>  			erase = tested_erase;
>  			break;
>  		}
> @@ -2575,7 +2572,6 @@ static int spi_nor_select_erase(struct spi_nor *nor)
>  	struct spi_nor_erase_map *map = &nor->params->erase_map;
>  	const struct spi_nor_erase_type *erase = NULL;
>  	struct mtd_info *mtd = &nor->mtd;
> -	u32 wanted_size = nor->info->sector_size;
>  	int i;
>  
>  	/*
> @@ -2586,13 +2582,8 @@ static int spi_nor_select_erase(struct spi_nor *nor)
>  	 * manage the SPI flash memory as uniform with a single erase sector
>  	 * size, when possible.
>  	 */
> -#ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
> -	/* prefer "small sector" erase if possible */
> -	wanted_size = 4096u;
> -#endif
> -
>  	if (spi_nor_has_uniform_erase(nor)) {
> -		erase = spi_nor_select_uniform_erase(map, wanted_size);
> +		erase = spi_nor_select_uniform_erase(map);
>  		if (!erase)
>  			return -EINVAL;
>  		nor->erase_opcode = erase->opcode;
>