Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim

[Michael Walle]

Hi Tudor,

Am 2020-01-11 15:19, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
> Hi, Michael,
>
> On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote:
> > Add support for the Winbond W25QnnJW-IM flashes. These have a
> > programmable QE bit. There are also the W25QnnJW-IQ variant which 
> > shares
> > the ID with the W25QnnFW parts. These have the QE bit hard strapped to
> > 1, thus don't support hardware write protection.
>
> There are few flavors of hw write protection supported by this flash, 
> the Q
> version does not disable them all. How about saying just that the /HOLD
> function is disabled?

I don't get your point here ;) My understanding is that HOLD# and WP# 
will
be disabled. Thus there is no "hardware write protection". What other hw
write protection do you have in mind?

> When we receive new flash id patches, we ask the contributors to 
> specify if
> they test the flash, in which modes (single, quad), and with which 
> controller.
> Ideally all the flash's flags should be tested, but there are cases in 
> which
> the controllers do not support quad read for example, and we accept the
> patches even if tested in single read mode. SPI_NOR_HAS_LOCK and
> SPI_NOR_HAS_TB must be tested as well.
>
> Even if the patches are rather simple, we ask for this to be sure that 
> we
> don't add a flash that is broken from day one. So, would you please 
> tell us
> what flashes did you test, what flags, and with which controller?

Ok will add that to the commit message. Just to make sure. I've only 
tested the
32mbit part. So is it still ok to include all other flashes of this 
family?

For now. tested with the NXP FlexSPI, single and dual (no quad since we 
are
using the write protection feature and IO2 and IO3 are not connected to 
the
CPU). So write protection is also tested. I will retest the TB bit.

> > Signed-off-by: Michael Walle <michael@xxxxxxxx>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c 
> > b/drivers/mtd/spi-nor/spi-nor.c
> > index addb6319fcbb..3fa8a81bdab0 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = 
> > {
> >  			SECT_4K | SPI_NOR_DUAL_READ |
> SPI_NOR_QUAD_READ |
> >  			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> >  	},
> > +	{
> > +		"w25q16jwim", INFO(0xef8015, 0, 64 * 1024,  32,
>
> "i" is for the temperature range, which is not a fixed characteristic. 
> Usually
> there are flashes with the same jedec-id, but with different 
> temperature
> ranges. Let's drop the "i" and rename it to "w25q16jwm"

Only that there is no flash with that part name :( according to the 
datasheet
there is only this one temp range available. From what I've seen for now 
(esp
looking at the macronix parts) it seems to first come first serve ;)
That being said, I don't insist on keeping that name, I'm fine with any 
name,
since I've learned you cannot rely on it in any way. Eg. the w25q32jwiq 
will
be discovered as w25q32dw. Or some Macronix flashes will be discovered 
as
ancient ones.

Btw. is renaming the flashes also considered a backwards incomaptible 
change?
And can there be two flashes with the same name? Because IMHO it would 
be
better to just have the name "w25q16" regardless whether its an FW/JW/JV 
etc.
It's better to show an ambiguous name than a wrong name.

-michael