[PATCH] mtd: spi-nor: Add Renesas AT25QL128A serial nor flash

[Biju Das]

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 at bp.renesas.com>
> > ---
> > $  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;