Hi Michael Walle,
Thanks for the feedback.
> Subject: Re: [PATCH] mtd: spi-nor: Add Renesas AT25QL128A serial nor
> flash
>
> Hi,
>
> Am 2022-05-02 20:56, schrieb Biju Das:
> > Add support for Renesas AT25QL128A serial nor flash.
> > Details of flash chip can be found here [1]
> >
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > $ xxd -p sfdp
> > 53464450060101ff00060110300000ff1f00010280000001ffffffffffff
> > ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
> > 083b80bbfeffffffffff00ffffff42eb0c200f5210d800ff3362d5008429
> > 01ceeca1073d7a757a75f7a2d55c19f61cffe810c080ffffffffffffffff
> > ffffffffffffffff501650190000ffff
>
> Thanks!
>
> > :~# md5sum
> > /sys/devices/platform/soc/10060000.spi/rpc-if-spi/spi_master/spi1/spi1
> > .0/spi-nor/sfdp
> > 23e3ec56b5b8f986d0488ba4727239dd
> >
> > ~# cat
> > /sys/devices/platform/soc/10060000.spi/rpc-if-spi/spi_master/spi1/spi1
> > .0/spi-nor/jedec_id
> > 1f4218
> >
> > ~# cat
> > /sys/devices/platform/soc/10060000.spi/rpc-if-spi/spi_master/spi1/spi1
> > .0/spi-nor/partname
> > at25ql128a
> >
> > ~# cat
> > /sys/devices/platform/soc/10060000.spi/rpc-if-spi/spi_master/spi1/spi1
> > .0/spi-nor/manufacturer
> > renesas
> >
> > RFC->v1:
> > * Moved the flash definitions to atmel.c
> > * Remove macro related to locking as it is untested.
> > * Replaced INFO macro with SNOR_ID3
> >
> > RFC:
> > *
> > ---
> > drivers/mtd/spi-nor/atmel.c | 55
> > +++++++++++++++++++++++++++++++++++++
> > drivers/mtd/spi-nor/core.c | 1 +
> > drivers/mtd/spi-nor/core.h | 1 +
> > 3 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
> > index 656dd80a0be7..61cff6de7db2 100644
> > --- a/drivers/mtd/spi-nor/atmel.c
> > +++ b/drivers/mtd/spi-nor/atmel.c
> > @@ -10,6 +10,15 @@
> >
> > #define ATMEL_SR_GLOBAL_PROTECT_MASK GENMASK(5, 2)
> >
> > +#define SNOR_ID3(_jedec_id) \
> > + .id = { \
> > + ((_jedec_id) >> 16) & 0xff, \
> > + ((_jedec_id) >> 8) & 0xff, \
> > + (_jedec_id) & 0xff, \
> > + }, \
> > + .id_len = 3, \
> > + .parse_sfdp = true
>
> Put that into core.h below the INFO macros.
OK. Agreed.
>
> > +
> > /*
> > * The Atmel AT25FS010/AT25FS040 parts have some weird configuration
> > for the
> > * block protection bits. We don't support them. But legacy behavior
> > in linux @@ -204,6 +213,52 @@ static const struct flash_info
> > atmel_nor_parts[] = {
> > NO_SFDP_FLAGS(SECT_4K) },
> > };
> >
> > +static const struct flash_info renesas_nor_parts[] = {
> > + { "at25ql128a", SNOR_ID3(0x1f4218) },
>
> Put that into the atmel list, no need for an own list.
OK.
>
> > +};
> > +
> > +/**
> > + * renesas_nor_set_4byte_addr_mode() - Set 4-byte address mode for
> > Renesas
> > + * flashes.
> > + * @nor: pointer to 'struct spi_nor'.
> > + * @enable: true to enter the 4-byte address mode, false to exit
> the
> > 4-byte
> > + * address mode.
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +static int renesas_nor_set_4byte_addr_mode(struct spi_nor *nor, bool
> > enable)
>
> Please expand the SFDP parser. This should be contained in 16th dword,
> there.
OK, Got it.
>
> > +{
> > + int ret;
> > +
> > + ret = spi_nor_write_enable(nor);
> > + if (ret)
> > + return ret;
> > +
> > + ret = spi_nor_set_4byte_addr_mode(nor, enable);
> > + if (ret)
> > + return ret;
> > +
> > + return spi_nor_write_disable(nor);
> > +}
> > +
> > +static void renesas_nor_default_init(struct spi_nor *nor)
> > +{
> > + nor->flags &= ~SNOR_F_HAS_16BIT_SR;
>
> Why is that?
>
> > + nor->params->quad_enable = NULL;
> > + nor->params->set_4byte_addr_mode = renesas_nor_set_4byte_addr_mode;
> > +}
> > +
> > +static const struct spi_nor_fixups renesas_nor_fixups = {
> > + .default_init = renesas_nor_default_init,
> > +};
> > +
> > +const struct spi_nor_manufacturer spi_nor_renesas = {
> > + .name = "renesas",
> > + .parts = renesas_nor_parts,
> > + .nparts = ARRAY_SIZE(renesas_nor_parts),
> > + .fixups = &renesas_nor_fixups,
> > +};
>
> Not needed.
OK, will remove this and send V2.
Cheers,
Biju
>
> > +
> > const struct spi_nor_manufacturer spi_nor_atmel = {
> > .name = "atmel",
> > .parts = atmel_nor_parts,
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index b4f141ad9c9c..ba9f222da00b 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -1621,6 +1621,7 @@ static const struct spi_nor_manufacturer
> > *manufacturers[] = {
> > &spi_nor_issi,
> > &spi_nor_macronix,
> > &spi_nor_micron,
> > + &spi_nor_renesas,
> > &spi_nor_st,
> > &spi_nor_spansion,
> > &spi_nor_sst,
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index b7fd760e3b47..3d2e39329079 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -511,6 +511,7 @@ extern const struct spi_nor_manufacturer
> > spi_nor_intel;
> > extern const struct spi_nor_manufacturer spi_nor_issi;
> > extern const struct spi_nor_manufacturer spi_nor_macronix;
> > extern const struct spi_nor_manufacturer spi_nor_micron;
> > +extern const struct spi_nor_manufacturer spi_nor_renesas;
> > extern const struct spi_nor_manufacturer spi_nor_st;
> > extern const struct spi_nor_manufacturer spi_nor_spansion;
> > extern const struct spi_nor_manufacturer spi_nor_sst;
