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

[Takahiro Kuwano]

On 1/19/2024 3:55 PM, Tudor Ambarus wrote:
> 
> 
> 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?
> 
spansion also. Changing it may impact to some older parts. I need time to test
and debug with different Flash parts.

> Cheers,
> ta