Message-ID: <CAJsYDVK9mmhBnfR1OjVbJ9RpCkU7S0wcQDYL+L043E_+mahX=Q@mail.gmail.com>
Date:   Fri, 15 Apr 2022 00:26:13 +0800
From:   Chuanhong Guo <gch981213@...il.com>
To:     Boris Brezillon <boris.brezillon@...labora.com>
Cc:     linux-mtd@...ts.infradead.org,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Patrice Chotard <patrice.chotard@...s.st.com>,
        Christophe Kerello <christophe.kerello@...s.st.com>,
        Mark Brown <broonie@...nel.org>,
        Daniel Palmer <daniel@...f.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] mtd: spinand: add support for detection with param page

Hi!

On Thu, Apr 14, 2022 at 11:06 PM Boris Brezillon
<boris.brezillon@...labora.com> wrote:
> [...]
> > 1. ESMT uses JEDEC ID from other vendors. This may collapse with future
> >    chips.
>
> Really?! So the manufacturer id matching is not even possible?

Here are some datasheets:
F50L1G41LB JEDEC ID: 0xc8 0x01
https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F50L1G41LB(2M).pdf
F50D1G41LB JEDEC ID: 0xc8 0x11
https://www.esmt.com.tw/upload/pdf/ESMT/datasheets/F50D1G41LB(2M).pdf
0xc8 is the JEDEC ID of GigaDevice.

>
> > 2. Winbond W25N01KV uses the exact same JEDEC ID as W25N01GV while
> >    having completely different chip parameters.

and datasheet for this Winbond chip is in the attachment.

> > [...]
>
> Oh come on! Looks like they never learn from their mistakes...
>
> >
> > It's common for vendors to release a series of SPI-NANDs with the same
> > SPI-NAND parameters in different voltages and/or capacities. The chip
> > table defined in this patch supports multiple model strings in one
> > entry, and multiple chip models can be covered using only one entry.
>
> That's really disappointing. And I guess the ONFI-page read commands are
> not even standard, and each vendor will come with its own sequence to
> read it.

They seem to have agreed on this already?
According to my datasheet collection:
Gigadevice since GD5FxGQ5, Macronix, ESMT, Micron, Winbond, Dosillicon
and KIOXIA can all read a parameter page with the following sequence:
1. execute WREN (0x06)
2. Set bit 6 in 0xB0 register
3. Load page (0x13) with a row address of 1
4. Use any supported Read Page Cache instruction to get 3 copies of
   ONFI data.

Step 4 is where there may be some differences, but it seems most chip
use 0x03/0x0b, 2-byte address, 1-byte dummy for single-bit spi
instructions. Even if that doesn't work, we'll simply get garbage data
and sig & crc checking won't pass.

> What's bothering me the most is the fact that ONFI parameter
> pages are not even designed for SPI NANDs. They have a bunch of
> parameters that don't really apply to SPI NANDs, and we're still
> lacking SPI-specific info, like the read/write/erase command variants,
> the maximum SPI bus width (AKA SPI modes)...

Agree. This is really annoying and it's preventing us from having
a complete auto-detected chip support mechanism.

> To sum-up, if we have to support reading the ONFI parameter page, I'd
> rather keep it vendor-private (but that's assuming the vendor ID
> detection works, and you seem to imply ESMT cheats on that too), and
> use it only as a way to distinguish between 2 chip variants.

I can't keep the ESMT one vendor-private unless i add some ESMT
hacks to gigadevice.c, or explicitly put ESMT entry above GigaDevice
and write a comment as a warning about the issue in core.c.

I think this detection is fine because these parameter page have
signature and CRC checking in place. That should keep any
misdetection from happenning.

>
> >
> > Signed-off-by: Chuanhong Guo <gch981213@...il.com>
> > ---
> >
> > I'm not brave enough to touch raw nand code without testing, so
> > sanitize_string, onfi_crc16 and nand_bit_wise_majority are
> > duplicated from raw/nand_onfi.c into spi/onfi.c
> >
> >  drivers/mtd/nand/spi/Makefile |   2 +-
> >  drivers/mtd/nand/spi/core.c   |  23 +--
> >  drivers/mtd/nand/spi/onfi.c   | 296 ++++++++++++++++++++++++++++++++++
>
> Please, no. Try to move the common code to drivers/mtd/nand/onfi.c
> instead of duplicating it. AFAICT, the only thing that's spinand
> specific is spinand_onfi_read() and the last part of
> spinand_onfi_detect(). I'm sure we can find a way to expose
> generic onfi_parsing() helpers.

The three function I mentioned are exact copies from raw nand.
Apart from that, we may be able to extract a common function for
parsing memory organization. On SPI-NANDs, the ONFI version
and feature field is 0, and the raw nand code relies on both fields
for ECC info, so this part will need to be separated.
I'll look into this. However I don't have any raw nand device to test
with atm, so the modification on raw nand side will be untested in v2.

-- 
Regards,
Chuanhong Guo

Download attachment "W25N01KVxxIR_Preliminary_Datasheet_Rev.A01_20220117.pdf" of type "application/pdf" (1128933 bytes)

Powered by blists - more mailing lists