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

From: Tudor.Ambarus
Date: Mon Jan 13 2020 - 04:06:51 EST


Hi, Michael,

On Sunday, January 12, 2020 1:16:12 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> 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?

Time delay write disable after Power-up for example.

>
> > 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?

No, just the ones that you can test please.

>
> 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.

Great, thanks.

>
> >> 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

The datasheet describes the W25Q32JW flash (check the first page of the
datasheet). There are two flavors of this flash, each with its own jedec-id: Q
version uses 156016h, M 158016h. We should name this flashes as "w25q32jwq"
and "w25q32jwm". Please notice that I skipped intentionally the "i" that
stands for temperature range. Manufacturers can provide better temperature
ranges for the same flash without changing the jedec-id. See this datasheet:

https://ro.mouser.com/datasheet/2/949/w25q128jv_revf_03272018_plus-1489608.pdf

> 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,

you should be fine just with the name that best describes the flash :)

> 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.

Would you please study what's wrong with these names and provide a patch to
fix them?

>
> Btw. is renaming the flashes also considered a backwards incomaptible
> change?

No, we can fix the names.

> And can there be two flashes with the same name? Because IMHO it would
> be

I would prefer that we don't. Why would you have two different jedec-ids with
the same name?

Cheers,
ta

> 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