Date:   Mon, 13 Jan 2020 11:07:39 +0100
From:   Michael Walle <michael@...le.cc>
To:     Tudor.Ambarus@...rochip.com
Cc:     linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        richard@....at, vigneshr@...com, miquel.raynal@...tlin.com
Subject: Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim

Hi Tudor,

Am 2020-01-13 10:06, schrieb Tudor.Ambarus@...rochip.com:
> 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@...rochip.com:
>> > 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.

That is the usual "write enable" mechanism. while marketing may seem to 
see
that as write protection, I do not, esp. not hardware write protection.

What about changing it to the following?
"These have the QE bit hard strapped to 1, thus don't the /HOLD and /WP
pins".

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

ok

>> 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@...le.cc>
>> >> ---
>> >>
>> >>  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".

You mean ef6016 and ef8016, yes that is correct. My point was there is 
no
"w25q15jwm": If linux kernel messages write "detected w25q32jwm" nobody 
will
know what that part is, because there is no such part. There is a 
w25q32jwim
or maybe w25q32jwXm but no w25q32jwm.

And naming the Q version w25q32jwq is not possible, because the id is 
shared
with the w25q32dw, which is already in spi-nor.c.

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

I know that and get your point. But I fear that this will confuse 
others.

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

Well, I've did that last week. But TBH I don't know if I want to go down 
that
road. Its not only the names, its also the flags. There is a mix of old 
and
new flashes in spi-nor.c; for example the newer ones supports dual and 
quad
mode. But the crux is, Macronix shares the same id over different 
generations
of the flash chip. For example look at the MX25L8005 (as it is supported 
in
the kernel) [1]. It doesn't support dual I/O mode. But the newer 
generation
MX25L8006E, which has the same id, supports it. So even I could come up 
with
something to fix it, I doubt it will be accepted without testing.


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

Because as pointed out in the Winbond example you cannot distiguish 
between
W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 
and
MX25L8006E. Thus my reasoning was to show only the common part, ie 
W25Q32
or MX25L80 which should be the same for this particular ID. Like I said, 
I'd
prefer showing an ambiguous name instead of a wrong one. But then you 
may
have different IDs with the same ambiguous name.

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

-michael

[1] 
http://web.archive.org/web/20180712194807/https://www.mct.net/download/macronix/mx25l8005.pdf

Powered by blists - more mailing lists