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

[Nicolas Ferre]

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@xxxxxxxxxxxxx wrote:
> > > > From: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxx>
> > > > ---
> >
> > [..]
> >
> > > >    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