Re: [PATCH] mtd: spi-nor: atmel: add at25ff321a entry

[Tudor Ambarus <tudor.ambarus@linaro.org>]

On 9/20/23 17:16, Nicolas Ferre wrote:
> On 19/09/2023 at 18:12, Tudor Ambarus wrote:
>> On 08.09.2023 18:14, nicolas.ferre@microchip.com wrote:
>>> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>>>
>>> Add the at25ff321a 4MB SPI flash which is able to provide
>>> SFDP informations.
>>> Datasheet:
>>> https://www.renesas.com/us/en/document/dst/at25ff321a-datasheet
>>>
>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>>> ---
> 
> [..]
> 
>>>   drivers/mtd/spi-nor/atmel.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>>> index 58968c1e7d2f..c94d52951481 100644
>>> --- a/drivers/mtd/spi-nor/atmel.c
>>> +++ b/drivers/mtd/spi-nor/atmel.c
>>> @@ -184,6 +184,10 @@ static const struct flash_info atmel_nor_parts[]
>>> = {
>>>                FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
>>>                NO_SFDP_FLAGS(SECT_4K)
>>>                .fixups = &atmel_nor_global_protection_fixups },
>>> +     { "at25ff321a", INFO(0x1f4708, 0, 64 * 1024,  64)
>>> +             FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
>>> +             PARSE_SFDP
>>> +             .fixups = &atmel_nor_global_protection_fixups },
>>
>> We have recently changed how the flash entries are defined. Would you
>> please try the following changes instead?
>>
>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>> index 95f0e139284e..44218716d81e 100644
>> --- a/drivers/mtd/spi-nor/atmel.c
>> +++ b/drivers/mtd/spi-nor/atmel.c
>> @@ -213,6 +213,12 @@ static const struct flash_info atmel_nor_parts[] = {
>>                  .flags = SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE,
>>                  .no_sfdp_flags = SECT_4K,
>>                  .fixups = &atmel_nor_global_protection_fixups
>> +       }, {
>> +               .id = SNOR_ID(0x1f, 0x47, 0x08),
>> +               .name = "at25ff321a",
>> +               .flags = SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE,
> 
> Here, I added:
> 
>                .no_sfdp_flags = SECT_4K,
> 
> to match the other devices of the family... I checked on the datasheet,
> 4K sectors are okay, but I don't know exactly if this is eligible to the
> "no_sfdb_flags" property... forgive me, I didn't check further knowing
> that you might have better view on this than me ;-)

yeah, no worries. You should not specify 4k here, as it should already
be set when parsing SFDP. You can use mtdinfo /dev/mtdx to check what
erase size gets set. In the core we set the maximum supported erase size
for speed considerations and making ubifs happy, so you'll probably see
64k when checking with mtdinfo. But you can try 4k erases as well by
setting MTD_SPI_NOR_USE_4K_SECTORS.

> 
>> +               .fixups = &atmel_nor_global_protection_fixups
>> +       }, {
>>          }, {
>>                  .id = SNOR_ID(0x1f, 0x48, 0x00),
>>                  .name = "at25df641",
> 
> That works perfectly well (I can re-post the test results as my previous
> patch if needed): do you want me to send the updated patch with your
> Suggested-by tag or you can send yours, tell me what you prefer.

Send a new patch please, it was just a suggestion to speed the things up.
> 
> Thanks for the heads-up on this update that I hadn't noticed. Best regards,
>   Nicolas
> 
you're welcome! Cheers!
ta