Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 14 Apr 2022 17:06:44 +0200
From:   Boris Brezillon <boris.brezillon@collabora.com>
To:     Chuanhong Guo <gch981213@gmail.com>
Cc:     linux-mtd@lists.infradead.org,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Patrice Chotard <patrice.chotard@foss.st.com>,
        Christophe Kerello <christophe.kerello@foss.st.com>,
        Mark Brown <broonie@kernel.org>,
        Daniel Palmer <daniel@0x0f.com>,
        linux-kernel@vger.kernel.org (open list)
Subject: Re: [PATCH 1/2] mtd: spinand: add support for detection with param
 page
Message-ID: <20220414170644.4a53484f@collabora.com>
In-Reply-To: <20220414143426.723168-2-gch981213@gmail.com>
References: <20220414143426.723168-1-gch981213@gmail.com>
        <20220414143426.723168-2-gch981213@gmail.com>
Organization: Collabora
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 14 Apr 2022 22:34:25 +0800
Chuanhong Guo <gch981213@gmail.com> wrote:

> SPI-NAND detection using chip ID isn't always reliable.
> Here are two known cases:
> 1. ESMT uses JEDEC ID from other vendors. This may collapse with future
>    chips.

Really?! So the manufacturer id matching is not even possible?

> 2. Winbond W25N01KV uses the exact same JEDEC ID as W25N01GV while
>    having completely different chip parameters.
> 
> Recent SPI-NANDs have a parameter page which is stored in the same
> format as raw NAND ONFI data. There are strings for chip manufacturer
> and chip model. Chip ECC requirement and memory organization are
> available too.
> This patch adds support for detecting SPI-NANDs with the parameter page
> after ID matching failed. In this detection, memory organization and
> ECC requirements are taken from the parameter page, and other SPI-NAND
> specific parameters are obtained by matching chip model string with
> a separated table.

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

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.

> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.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.