2023-09-20 02:45:24

by Tudor Ambarus

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



On 08.09.2023 18:14, [email protected] wrote:
> From: Nicolas Ferre <[email protected]>
>
> 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 <[email protected]>
> ---
> Hi,
>

Hi!

> This flash is present on the MikoE flash 10 Click board and was tested on
> sama7g5-ek at spi frequency of 80 MHz:
>
> Here is the test I ran:
> root@sama7g5ek-sd:~# dd if=/dev/urandom of=./spi_test bs=1M count=3
> 3+0 records in
> 3+0 records out
> 3145728 bytes (3.1 MB, 3.0 MiB) copied, 0.932896 s, 3.4 MB/s
> root@sama7g5ek-sd:~# mtd_debug write /dev/mtd0 0 3145728 spi_test
> Copied 3145728 bytes from spi_test to address 0x00000000 in flash
> root@sama7g5ek-sd:~# mtd_debug erase /dev/mtd0 0 3145728
> Erased 3145728 bytes from address 0x00000000 in flash
> root@sama7g5ek-sd:~# mtd_debug read /dev/mtd0 0 3145728 spi_read
> Copied 3145728 bytes from address 0x00000000 in flash to spi_read
> root@sama7g5ek-sd:~# hexdump spi_read
> 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
> *
> 0300000
> root@sama7g5ek-sd:~# mtd_debug write /dev/mtd0 0 3145728 spi_test
> Copied 3145728 bytes from spi_test to address 0x00000000 in flash
> root@sama7g5ek-sd:~# mtd_debug read /dev/mtd0 0 3145728 spi_read
> Copied 3145728 bytes from address 0x00000000 in flash to spi_read
> root@sama7g5ek-sd:~# sha1sum spi_test spi_read
> 06d5459972d51a2ff4270e612270c6519e797a0b spi_test
> 06d5459972d51a2ff4270e612270c6519e797a0b spi_read
>
> Here are the data from sysfs:
>
> root@sama7g5ek-sd:~# cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
> at25ff321a
> root@sama7g5ek-sd:~# cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
> 1f4708
> root@sama7g5ek-sd:~# cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
> atmel
> root@sama7g5ek-sd:~# hexdump -C /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 00000000 53 46 44 50 06 01 00 ff 00 06 01 10 10 00 00 ff |SFDP............|
> 00000010 e5 20 e1 ff ff ff ff 01 40 eb 08 6b 08 3b 00 ff |. [email protected].;..|
> 00000020 ee ff ff ff ff ff 00 ff ff ff 00 ff 0c 20 0f 52 |............. .R|
> 00000030 10 d8 00 ff 50 2a 2a 01 82 ff 9c d2 64 c1 08 46 |....P**.....d..F|
> 00000040 7a 75 7a 75 f7 c4 d5 5c 00 06 51 ff 88 30 00 00 |zuzu...\..Q..0..|
> 00000050
> root@sama7g5ek-sd:~# md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> d540f07cbfb7c9c19654c453b561b311 /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
>

Wonderful, thanks Nicolas!

> Best regards,
> Nicolas
>
> 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,
+ .fixups = &atmel_nor_global_protection_fixups
+ }, {
}, {
.id = SNOR_ID(0x1f, 0x48, 0x00),
.name = "at25df641",


2023-09-20 19:13:59

by Nicolas Ferre

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

On 19/09/2023 at 18:12, Tudor Ambarus wrote:
> On 08.09.2023 18:14, [email protected] wrote:
>> From: Nicolas Ferre <[email protected]>
>>
>> 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 <[email protected]>
>> ---

[..]

>> 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 ;-)

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

Thanks for the heads-up on this update that I hadn't noticed. Best regards,
Nicolas

2023-09-22 08:09:40

by Tudor Ambarus

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



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, [email protected] wrote:
>>> From: Nicolas Ferre <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>
> [..]
>
>>>   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

2023-09-26 18:05:16

by Nicolas Ferre

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

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, [email protected] wrote:
>>>> From: Nicolas Ferre <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>
>> [..]
>>
>>>> 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