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

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 1/5/2024 9:23 PM, Michael Walle wrote:
> Hi,
> 
>> -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;
>> +    struct mtd_info *mtd = &nor->mtd;
>> +    struct mtd_erase_region_info *mtd_region;
>> +    u32 erase_size;
>> +    u8 erase_mask;
>> +    int n_regions, i, j;
>> +
>> +    for (i = 0; !spi_nor_region_is_last(&region[i]); i++)
>> +        ;
> 
> Please put that into a helper which returns the number of regions.
> 
Yes, I will do it.

> FWIW, I really dislike the magic around encoding all sorts of stuff
> into the offset. It makes the code just hard to read.
> 
> 
>> +
>> +    n_regions = i + 1;
>> +    mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
>> +                  GFP_KERNEL);
> 
> Who's the owner? mtd->dev or nor->dev?
> 
I think it should be nor->dev.
The mtd device is not yet registered at this point.

>> +    if (!mtd_region)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < n_regions; i++) {
>> +        if (region[i].offset & SNOR_OVERLAID_REGION) {
> 
> Btw. what is an overlaid region? I couldn't find any comment
> about it.
> 
It is the remaining part of regular sector that overlaid by 4KB sectors.
In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on
bottom address, 128KB is overlaid region. The erase opcode for this region is
same as 256KB sectors.

>> +            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;
>> +                }
>> +            }
>> +        }
>> +        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);
> 
> mtd->erasesize is set somewhere else, please move it into this
> function, because it will also have a special case for the
> non_uniform flashes. Maybe we'll need our own erasesize stored
> together with the opcode.
> 
Let me introduce params->erasesize which set through SFDP parse, then

    mtd->erasesize = nor->params->erasesize;

like as other 'size' params.

> Also this should be written as
> 
> if (!spi_nor_has_uniform_erase(nor))
>   spi_nor_set_mtd_eraseregions(nor);
> 
> return 0;
> 
> -michael
> 
OK, thanks!

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

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