Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220413083824.247136-1-gch981213@gmail.com> <20220413145843.46a3d9b5@xps13>
In-Reply-To: <20220413145843.46a3d9b5@xps13>
From:   Chuanhong Guo <gch981213@gmail.com>
Date:   Wed, 13 Apr 2022 22:50:43 +0800
Message-ID: <CAJsYDV+3J0ipbR+N-xE=DH-WXsjierdHe_pJtKf1Xbt7fdaiWw@mail.gmail.com>
Subject: Re: [PATCH v2] mtd: spinand: add support for ESMT F50x1G41LB
To:     Miquel Raynal <miquel.raynal@bootlin.com>
Cc:     linux-mtd@lists.infradead.org, Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Patrice Chotard <patrice.chotard@foss.st.com>,
        Boris Brezillon <boris.brezillon@collabora.com>,
        Christophe Kerello <christophe.kerello@foss.st.com>,
        Mark Brown <broonie@kernel.org>,
        Daniel Palmer <daniel@0x0f.com>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi!

On Wed, Apr 13, 2022 at 8:58 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> [...]
> > This patch is made purely based on datasheet info without testing
> > on any actual chips.
>
> Do you plan to get one of these any time soon?

No. I already have way more spi-nand chips than I possibly
need due to my last GigaDevice submission :)

I need to replace the spi-nand driver for mediatek in OpenWrt
with my recent submission, and this chip is used in one of the
currently supported router. So I made this patch from
datasheet for it.

BTW Rockchip added identical chip support in their u-boot.[0]
I assume they've tested it.

> I am not really confident merging a 100% non-tested driver :)

I can understand that.
I'll roll this patch out in OpenWrt anyway. I can wait for a test
there and resubmit piled downstream patches for chip supports
after next OpenWrt stable release.

>
> [...]
> > +
> > +/* ESMT uses GigaDevice 0xc8 JECDEC ID on some SPI NANDs */
> > +#define SPINAND_MFR_ESMT_C8                  0xc8
>
> What happens if the gigadevice driver probes first?

Their device ID aren't conflicting yet, so nothing will happen
at the moment.

There is a solution for future conflict: Recent SPI-NAND chips
contain a parameter page which has the exact chip vendor
and model. We can do one more detection with the parameter
page content.
Winbond W25N01KV is a 2k+96 SPI-NAND with 4-bit ECC.
It uses the exact same chip id as the current W25N01GV
(2k+64 1-bit ECC). We need to support detection using
parameter page for this crazy decision by Winbond anyway.
I'll try to code something for this with my free time.

My current idea is: We first do a detection based on chip id.
If that failed, try to read the parameter page. If we got a
valid one, match the chip vendor and model string.

Any thoughts?

(BTW this ESMT chip is POWERCHIP PSU1GS20DX
according to the parameter page in their datasheet.
But I can't find a datasheet for this model number.)

>
> > +
> > +#define F50L2G41XA_ECC_STATUS_MASK           GENMASK(6, 4)
> > +#define F50L2G41XA_STATUS_ECC_1_3_BITFLIPS   (1 << 4)
> > +#define F50L2G41XA_STATUS_ECC_4_6_BITFLIPS   (3 << 4)
> > +#define F50L2G41XA_STATUS_ECC_7_8_BITFLIPS   (5 << 4)

Oops. These are left-over defines when I discovered that ESMT 2G/4G
SPI-NANDs are repackaged micron parts and dropped their support
from this file.
A v3 is needed anyway. I'll wait for a test before submitting it.

[0]: https://github.com/rockchip-linux/u-boot/commit/52b0060178285488737854a48ddecd381f8b236e
-- 
Regards,
Chuanhong Guo