[PATCH v2 1/2] mtd: spi-nor: core: rework struct spi_nor_erase_region
Takahiro Kuwano
tkuw584924 at gmail.com
Sun Feb 4 20:21:51 PST 2024
Hi Michael,
On 2/2/2024 1:13 AM, Michael Walle wrote:
> Hi,
>
> Thanks for working on this! This is going to look better now.
>
>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>
>> Encoding bitmask flags into offset worsen the code readability. The
>> erase type mask and flags should be stored in dedicated members. Also,
>> erase_map.uniform_erase_type can be removed as it is redundant.
>
> Nice!
>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> ---
>> drivers/mtd/spi-nor/core.c | 35 +++++++++++++++--------------------
>> drivers/mtd/spi-nor/core.h | 28 ++++++++++------------------
>> drivers/mtd/spi-nor/sfdp.c | 30 +++++++++++++-----------------
>
> I think debugfs.c is missing here and needs to be changed, too.
>
>> 3 files changed, 38 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 1b0c6770c14e..5b2f13d0888e 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1150,7 +1150,7 @@ static u8 spi_nor_convert_3to4_erase(u8 opcode)
>>
>> static bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>> {
>> - return !!nor->params->erase_map.uniform_erase_type;
>> + return !!nor->params->erase_map.uniform_region.erase_mask;
>
> I you are going to change this line, please drop the unneeded "!!".
>
>> }
>>
>> static void spi_nor_set_4byte_opcodes(struct spi_nor *nor)
>> @@ -1534,7 +1534,6 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>> const struct spi_nor_erase_type *erase;
>> u32 rem;
>> int i;
>> - u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>
>> /*
>> * Erase types are ordered by size, with the smallest erase type at
>> @@ -1542,7 +1541,7 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>> */
>> for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>> /* Does the erase region support the tested erase type? */
>> - if (!(erase_mask & BIT(i)))
>> + if (!(region->erase_mask & BIT(i)))
>> continue;
>>
>> erase = &map->erase_type[i];
>> @@ -1550,7 +1549,7 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>> continue;
>>
>> /* Alignment is not mandatory for overlaid regions */
>> - if (region->offset & SNOR_OVERLAID_REGION &&
>> + if (region->flags & SNOR_OVERLAID_REGION &&
>> region->size <= len)
>> return erase;
>>
>> @@ -1568,12 +1567,12 @@ spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>
>> static u64 spi_nor_region_is_last(const struct spi_nor_erase_region *region)
>> {
>> - return region->offset & SNOR_LAST_REGION;
>> + return region->flags & SNOR_LAST_REGION;
>> }
>
> Do you think we can get rid of this, too? Just save the number
> of erase regions instead of having this flag. IIRC Tudor had also
> suggested this.
>
Yes, I will remove this and rework related functions.
If we remove this flag, the SNOR_OVERLAID_REGION will be only one flag.
How about removing the SNOR_OVERLAID_REGION as well, and change 'region->flags'
to a bool value like 'region->overlaid'?
>>
>> static u64 spi_nor_region_end(const struct spi_nor_erase_region *region)
>> {
>> - return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>> + return region->offset + region->size;
>> }
>>
>> /**
>> @@ -1604,16 +1603,14 @@ static struct spi_nor_erase_region *
>> spi_nor_find_erase_region(const struct spi_nor_erase_map *map, u64 addr)
>> {
>> struct spi_nor_erase_region *region = map->regions;
>> - u64 region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
>> - u64 region_end = region_start + region->size;
>> + u64 region_end = spi_nor_region_end(region);
>>
>> - while (addr < region_start || addr >= region_end) {
>> + while (addr < region->offset || addr >= region_end) {
>> region = spi_nor_region_next(region);
>> if (!region)
>> return ERR_PTR(-EINVAL);
>>
>> - region_start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
>> - region_end = region_start + region->size;
>> + region_end = spi_nor_region_end(region);
>> }
>>
>> return region;
>> @@ -1641,7 +1638,7 @@ spi_nor_init_erase_cmd(const struct spi_nor_erase_region *region,
>> cmd->opcode = erase->opcode;
>> cmd->count = 1;
>>
>> - if (region->offset & SNOR_OVERLAID_REGION)
>> + if (region->flags & SNOR_OVERLAID_REGION)
>> cmd->size = region->size;
>> else
>> cmd->size = erase->size;
>> @@ -1700,7 +1697,7 @@ static int spi_nor_init_erase_cmd_list(struct spi_nor *nor,
>>
>> if (prev_erase != erase ||
>> erase->size != cmd->size ||
>> - region->offset & SNOR_OVERLAID_REGION) {
>> + region->flags & SNOR_OVERLAID_REGION) {
>> cmd = spi_nor_init_erase_cmd(region, erase);
>> if (IS_ERR(cmd)) {
>> ret = PTR_ERR(cmd);
>> @@ -2439,12 +2436,11 @@ void spi_nor_mask_erase_type(struct spi_nor_erase_type *erase)
>> void spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>> u8 erase_mask, u64 flash_size)
>> {
>> - /* Offset 0 with erase_mask and SNOR_LAST_REGION bit set */
>> - map->uniform_region.offset = (erase_mask & SNOR_ERASE_TYPE_MASK) |
>> - SNOR_LAST_REGION;
>> + map->uniform_region.offset = 0;
>> map->uniform_region.size = flash_size;
>> + map->uniform_region.erase_mask = erase_mask;
>> + map->uniform_region.flags = SNOR_LAST_REGION;
>> map->regions = &map->uniform_region;
>> - map->uniform_erase_type = erase_mask;
>> }
>>
>> int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>> @@ -2539,7 +2535,7 @@ 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;
>> + u8 uniform_erase_type = map->uniform_region.erase_mask;
>>
>> for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>> if (!(uniform_erase_type & BIT(i)))
>> @@ -2573,8 +2569,7 @@ spi_nor_select_uniform_erase(struct spi_nor_erase_map *map,
>> return NULL;
>>
>> /* Disable all other Sector Erase commands. */
>> - map->uniform_erase_type &= ~SNOR_ERASE_TYPE_MASK;
>> - map->uniform_erase_type |= BIT(erase - map->erase_type);
>> + map->uniform_region.erase_mask = BIT(erase - map->erase_type);
>> return erase;
>> }
>>
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 9217379b9cfe..e002de22b18a 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -233,27 +233,25 @@ struct spi_nor_erase_command {
>> /**
>> * struct spi_nor_erase_region - Structure to describe a SPI NOR erase region
>> * @offset: the offset in the data array of erase region start.
>> - * LSB bits are used as a bitmask encoding flags to
>> - * determine if this region is overlaid, if this region is
>> - * the last in the SPI NOR flash memory and to indicate
>> - * all the supported erase commands inside this region.
>> - * The erase types are sorted in ascending order with the
>> - * smallest Erase Type size being at BIT(0).
>> * @size: the size of the region in bytes.
>> + * @erase_mask: bitmask to indicate all the supported erase commands
>> + * inside this region. The erase types are sorted in
>> + * ascending order with the smallest Erase Type size being
>> + * at BIT(0).
>> + * @flags: flags to determine if this region is overlaid, if this
>> + * region is the last in the SPI NOR flash memory
>> */
>> struct spi_nor_erase_region {
>> u64 offset;
>> u64 size;
>> + u8 erase_mask;
>> + u8 flags;
>> };
>>
>> #define SNOR_ERASE_TYPE_MAX 4
>> -#define SNOR_ERASE_TYPE_MASK GENMASK_ULL(SNOR_ERASE_TYPE_MAX - 1, 0)
>>
>> -#define SNOR_LAST_REGION BIT(4)
>> -#define SNOR_OVERLAID_REGION BIT(5)
>> -
>> -#define SNOR_ERASE_FLAGS_MAX 6
>> -#define SNOR_ERASE_FLAGS_MASK GENMASK_ULL(SNOR_ERASE_FLAGS_MAX - 1, 0)
>> +#define SNOR_LAST_REGION BIT(0)
>> +#define SNOR_OVERLAID_REGION BIT(1)
>>
>> /**
>> * struct spi_nor_erase_map - Structure to describe the SPI NOR erase map
>> @@ -266,17 +264,11 @@ struct spi_nor_erase_region {
>> * The erase types are sorted in ascending order, with the
>> * smallest Erase Type size being the first member in the
>> * erase_type array.
>> - * @uniform_erase_type: bitmask encoding erase types that can erase the
>> - * entire memory. This member is completed at init by
>> - * uniform and non-uniform SPI NOR flash memories if they
>> - * support at least one erase type that can erase the
>> - * entire memory.
>> */
>> struct spi_nor_erase_map {
>> struct spi_nor_erase_region *regions;
>> struct spi_nor_erase_region uniform_region;
>> struct spi_nor_erase_type erase_type[SNOR_ERASE_TYPE_MAX];
>> - u8 uniform_erase_type;
>> };
>>
>> /**
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index b3b11dfed789..c8d8b4e5b7e6 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -389,17 +389,14 @@ static u8 spi_nor_sort_erase_mask(struct spi_nor_erase_map *map, u8 erase_mask)
>> static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>> {
>> struct spi_nor_erase_region *region = map->regions;
>> - u8 region_erase_mask, sorted_erase_mask;
>> + u8 sorted_erase_mask;
>>
>> while (region) {
>> - region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>> -
>> sorted_erase_mask = spi_nor_sort_erase_mask(map,
>> - region_erase_mask);
>> + region->erase_mask);
>>
>> /* Overwrite erase mask. */
>> - region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>> - sorted_erase_mask;
>> + region->erase_mask = sorted_erase_mask;
>>
>> region = spi_nor_region_next(region);
>> }
>> @@ -553,8 +550,6 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>> * selecting the uniform erase.
>> */
>> spi_nor_regions_sort_erase_types(map);
>> - map->uniform_erase_type = map->uniform_region.offset &
>> - SNOR_ERASE_TYPE_MASK;
>>
>> /* Stop here if not JESD216 rev A or later. */
>> if (bfpt_header->length == BFPT_DWORD_MAX_JESD216)
>> @@ -781,12 +776,12 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>
>> static void spi_nor_region_mark_end(struct spi_nor_erase_region *region)
>> {
>> - region->offset |= SNOR_LAST_REGION;
>> + region->flags |= SNOR_LAST_REGION;
>> }
>
> I'd assume this will go away if you have some kind of .n_erase_maps
>
>>
>> static void spi_nor_region_mark_overlay(struct spi_nor_erase_region *region)
>> {
>> - region->offset |= SNOR_OVERLAID_REGION;
>> + region->flags |= SNOR_OVERLAID_REGION;
>
> And this I'd drop, too. No need for a function to set just a flag.
>
> -michael
>
>> }
>>
>> /**
>> @@ -848,9 +843,10 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
>> /* Populate regions. */
>> for (i = 0; i < region_count; i++) {
>> j = i + 1; /* index for the region dword */
>> + region[i].offset = offset;
>> region[i].size = SMPT_MAP_REGION_SIZE(smpt[j]);
>> erase_type = SMPT_MAP_REGION_ERASE_TYPE(smpt[j]);
>> - region[i].offset = offset | erase_type;
>> + region[i].erase_mask = erase_type;
>>
>> spi_nor_region_check_overlay(®ion[i], erase, erase_type);
>>
>> @@ -866,21 +862,21 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
>> */
>> regions_erase_type |= erase_type;
>>
>> - offset = (region[i].offset & ~SNOR_ERASE_FLAGS_MASK) +
>> - region[i].size;
>> + offset = region[i].offset + region[i].size;
>> }
>> spi_nor_region_mark_end(®ion[i - 1]);
>>
>> - save_uniform_erase_type = map->uniform_erase_type;
>> - map->uniform_erase_type = spi_nor_sort_erase_mask(map,
>> - uniform_erase_type);
>> + save_uniform_erase_type = map->uniform_region.erase_mask;
>> + map->uniform_region.erase_mask =
>> + spi_nor_sort_erase_mask(map,
>> + uniform_erase_type);
>>
>> if (!regions_erase_type) {
>> /*
>> * Roll back to the previous uniform_erase_type mask, SMPT is
>> * broken.
>> */
>> - map->uniform_erase_type = save_uniform_erase_type;
>> + map->uniform_region.erase_mask = save_uniform_erase_type;
>> return -EINVAL;
>> }
More information about the linux-mtd
mailing list