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

[Tudor Ambarus]

On 1/19/24 06:29, Takahiro Kuwano wrote:
> 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.
> 

Do you mean xilinx? That's a dead weight, I haven't seen patches for it
since its introduction. I'm thinking of getting rid of the xilinx s3an
flashes from the SPI NOR. Michael, Pratyush?

Cheers,
ta