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

[Nicolas Ferre <nicolas.ferre@microchip.com>]

On 21/09/2023 at 09:14, Tudor Ambarus wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

Absolutely, here is what I'm experiencing:
# no_sfdp_flags not set / linux-next / sama5d27 wlsom1 ek
# CONFIG_MTD_SPI_NOR_USE_4K_SECTORS=y
root@sama5d27-wlsom1-ek-sd:~# mtdinfo /dev/mtd0
mtd0
Name:                           spi0.1
Type:                           nor
Eraseblock size:                4096 bytes, 4.0 KiB
Amount of eraseblocks:          1024 (4194304 bytes, 4.0 MiB)
Minimum input/output unit size: 1 byte
Sub-page size:                  1 byte
Character device major/minor:   90:0
Bad blocks are allowed:         false
Device is writable:             true

# no_sfdp_flags not set / linux-next / sama5d27 wlsom1 ek
# # CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not set
root@sama5d27-wlsom1-ek-sd:~# mtdinfo /dev/mtd0
mtd0
Name:                           spi0.1
Type:                           nor
Eraseblock size:                65536 bytes, 64.0 KiB
Amount of eraseblocks:          64 (4194304 bytes, 4.0 MiB)
Minimum input/output unit size: 1 byte
Sub-page size:                  1 byte
Character device major/minor:   90:0
Bad blocks are allowed:         false
Device is writable:             true

>>> +               .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.

Sure, I'm on it ;-)

>> Thanks for the heads-up on this update that I hadn't noticed. Best regards,
>>    Nicolas
>>
> you're welcome! Cheers!

Best regards,
  Nicolas