[PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map

[Tudor Ambarus]

On 12/25/23 08:03, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> 
> Some of Infineon SPI NOR flash devices support hybrid sector layout that
> overlays 4KB sectors on a 256KB sector and SPI NOR framework recognizes
> that by parsing SMPT and construct params->erase_map. The hybrid sector
> layout is similar to CFI flash devices that have small sectors on top
> and/or bottom address. In case of CFI flash devices, the erase map
> information is parsed through CFI table and populated into
> mtd->eraseregions so that users can create MTD partitions that aligned with
> small sector boundaries. This patch provides the same capability to SPI
> NOR flash devices that have non-uniform erase map.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> ---
> The s25hs512t has 32 x 4KB, 1 x 128KB, and 255 x 256KB sectors by default.
> Let's think about creating three MTD partitions that align to region
> boundary.
> 
> BEFORE applying this patch, 1st and 2nd region are forced to read-only:
> 
> ---kernel log---
> ...
> spi-nor spi0.0: s25hs512t (65536 Kbytes)
> 3 fixed-partitions partitions found on MTD device spi0.0
> Creating 3 MTD partitions on "spi0.0":
> 0x000000000000-0x000000020000 : "4KB x 32"
> mtd: partition "4KB x 32" doesn't end on an erase/write block -- force read-only
> 0x000000020000-0x000000040000 : "128KB x 1"
> mtd: partition "128KB x 1" doesn't start on an erase/write block boundary -- force read-only
> 0x000000040000-0x000004000000 : "256KB x 255"
> ...
> 
> ---MTD Info---
> zynq> mtd_debug info /dev/mtd0
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_ROM
> mtd.size = 131072 (128K)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
> 
> zynq> mtd_debug info /dev/mtd1
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_ROM
> mtd.size = 131072 (128K)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
> 
> zynq> mtd_debug info /dev/mtd2
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 66846720 (63M)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
> 
> 
> 
> AFTER applying this patch, erasesize is correctly informed and no read-only:
> 
> ---kernel log---
> ...
> spi-nor spi0.0: s25hs512t (65536 Kbytes)
> 3 fixed-partitions partitions found on MTD device spi0.0
> Creating 3 MTD partitions on "spi0.0":
> 0x000000000000-0x000000020000 : "4KB x 32"
> 0x000000020000-0x000000040000 : "128KB x 1"
> 0x000000040000-0x000004000000 : "256KB x 255"
> ...
> 
> ---MTD Info---
> zynq> mtd_debug info /dev/mtd0
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 131072 (128K)
> mtd.erasesize = 4096 (4K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
> 
> zynq> mtd_debug info /dev/mtd1
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 131072 (128K)
> mtd.erasesize = 131072 (128K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
> 
> zynq> mtd_debug info /dev/mtd2
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 66846720 (63M)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
> 

nice!

> ---
>  drivers/mtd/spi-nor/core.c | 55 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1b0c6770c14e..e512491733a8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3408,7 +3408,51 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>  	return info;
>  }
>  
> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
> +{
> +	struct spi_nor_erase_map *map = &nor->params->erase_map;
> +	struct spi_nor_erase_region *region = map->regions;

shall we have some consts above?

> +	struct mtd_info *mtd = &nor->mtd;

some prefer a reverse xmas tree, thus put the definition from above
below the declaration from below.

> +	struct mtd_erase_region_info *mtd_region;
> +	u32 erase_size;
> +	u8 erase_mask;

put the u8 last to avoid stack padding

> +	int n_regions, i, j;

unsigned int

> +
> +	for (i = 0; !spi_nor_region_is_last(&region[i]); i++)
> +		;
> +
> +	n_regions = i + 1;

all this just to get the number of regions? how about saving the number
of regions somewhere and use it everywhere?

> +	mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
> +				  GFP_KERNEL);
> +	if (!mtd_region)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n_regions; i++) {
> +		if (region[i].offset & SNOR_OVERLAID_REGION) {
> +			erase_size = region[i].size;
> +		} else {
> +			erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
> +
> +			for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
> +				if (erase_mask & BIT(j)) {
> +					erase_size = map->erase_type[j].size;
> +					break;
> +				}
> +			}

this for, determining the erase size, deserves a dedicated method. Too
many indentation levels.

> +		}
> +		mtd_region[i].erasesize = erase_size;
> +		mtd_region[i].numblocks = div64_ul(region[i].size, erase_size);
> +		mtd_region[i].offset = region[i].offset &
> +				       ~SNOR_ERASE_FLAGS_MASK;
> +	}
> +
> +	mtd->numeraseregions = n_regions;
> +	mtd->eraseregions = mtd_region;
> +
> +	return 0;
> +}
> +
> +static int spi_nor_set_mtd_info(struct spi_nor *nor)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
>  	struct device *dev = nor->dev;
> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
>  	mtd->_resume = spi_nor_resume;
>  	mtd->_get_device = spi_nor_get_device;
>  	mtd->_put_device = spi_nor_put_device;
> +
> +	if (spi_nor_has_uniform_erase(nor))
> +		return 0;
> +
> +	return spi_nor_set_mtd_eraseregions(nor);
>  }
>  
>  static int spi_nor_hw_reset(struct spi_nor *nor)
> @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  		return ret;
>  
>  	/* No mtd_info fields should be used up to this point. */
> -	spi_nor_set_mtd_info(nor);
> +	ret = spi_nor_set_mtd_info(nor);
> +	if (ret)
> +		return ret;
>  
>  	dev_info(dev, "%s (%lld Kbytes)\n", info->name,
>  			(long long)mtd->size >> 10);