Re: (EXT) Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To: Michael Walle <michael@walle.cc>
Cc: Tudor Ambarus <tudor.ambarus@microchip.com>,
	Pratyush Yadav <p.yadav@ti.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: (EXT) Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q
Date: Wed, 06 Oct 2021 14:32:11 +0200	[thread overview]
Message-ID: <3258026683c916a3a42e98ba76628228cddacb23.camel@ew.tq-group.com> (raw)
In-Reply-To: <f3dbab898e9f1946129e5733095bdf3c@walle.cc>

On Tue, 2021-07-27 at 09:09 +0200, Michael Walle wrote:
> Am 2021-07-23 13:27, schrieb Matthias Schiffer:
> > All mt25q variants have the same features.
> > 
> > Unlike the smaller variants, no n25q with 2G exists, so we don't need 
> > to
> > match on the extended ID to distinguish n25q and mt25q series for these
> > models.
> 
> But why shouldn't we? What if there will be another flash with
> the same first three id bytes?

How do you suggest we proceed here? At the moment there are entries
matching on 0x20b[ab]22 (ignoring the extended ID) with the name
mt25q[lu]02g.

Should I change these entries to match on on the extended ID
0x20b[ab]22 / 0x104400 instead when I add the bits for the features
specific to the variant, removing support for other 0x20b[ab]22
variants that may or may not actually exist? Keeping both entries (with
and without extended ID match) would preserve compatiblity with such
variants, but this approach seems problematic to me as well, as I can't
even give a name to the more generic entries (and there is no natural
extension of the n25q naming scheme to a 2G variant).

> 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/mtd/spi-nor/micron-st.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/micron-st.c 
> > b/drivers/mtd/spi-nor/micron-st.c
> > index c224e59820a1..d5baa8762c8d 100644
> > --- a/drivers/mtd/spi-nor/micron-st.c
> > +++ b/drivers/mtd/spi-nor/micron-st.c
> > @@ -181,11 +181,11 @@ static const struct flash_info st_parts[] = {
> >  			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> >  			      NO_CHIP_ERASE) },
> >  	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
> > -			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> > -			      NO_CHIP_ERASE) },
> 
> This bothers me. I'm not sure how this will work. I see that
> chip erase is command 0xc7, but both the new and the old flash
> just supports 0xc3 (DIE ERASE). Did you test these changes?

I finally got my hands on hardware with this flash again (well, a
mt25qu01g, I don't think we have the 2G variants anywhere) and I can
confirm that the chip erase does not work with my patch. I will send an
updated version that keeps NO_CHIP_ERASE.

> 
> > +			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> > +			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> >  	{ "mt25qu02g",   INFO(0x20bb22, 0, 64 * 1024, 4096,
> >  			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> > -			      SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> > +			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> > 
> >  	{ "m25p05",  INFO(0x202010,  0,  32 * 1024,   2, 0) },
> >  	{ "m25p10",  INFO(0x202011,  0,  32 * 1024,   4, 0) },
> 
> -michael