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

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 1/12/2024 4:14 PM, Takahiro Kuwano wrote:
> 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.
> 
I tried to implement this, but found mtd->erasesize is set in some fixup hooks
in manufacturer driver and need to think carefully about changing them.
Let me do this later in another series of patches.

Thanks,
Takahiro

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