Re: [PATCH RFC v2] mtd: spinand: winbond: add support for W25N04LW

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Dominique,

A bit more feedback...

On 12/08/2026 at 04:14:02 GMT, Dominique Martinet <dominique.martinet@atmark-techno.com> wrote:

> Add support for Winbond W25N04LW 4Gbit SPI-NAND:
>> 1.8V 4G-bit
>> Raw serial SLC NAND flash memory
>> Dual/Quad SPI with 104MHz
>> buffer read, continuous read & sequential read
>
> Highlights copied from datasheet:
> – Page size: 4,352 Bytes (4096 + 256 Bytes)
> – Block size: 64 pages (256K + 16K Bytes)
> - Built-in 8-Bit ECC for memory array
> – ECC status bits indicate ECC results
> – Bad Block Management and LUT access
> – Software and Hardware Write-Protect
> – Power Supply Lock-Down and OTP protection
> – Unique ID and Parameter page
> – Ten 4KB OTP pages per die
> – Read Level Setting for Read Retry
>
> The 4096+256 byte page layout is as follow:
> - 4096 bytes ECC protected "main memory array", subdivided in 8
>   512 bytes sectors
> - 128 bytes spare array, subdivided in 8 16 bytes sectors as follow:
>   - 2 bytes bad block marker (no ECC protection)
>   - 2 bytes "user data II" (no ECC protection)
>   - 12 bytes "user data I" (ECC protected)
> - 128 bytes parity array, subdivided in 8 16 bytes sectors:
>   - 13 bytes ECC for main data and user data
>   - 3 unused bytes
>
> This commit does not support most advanced features such as continuous
> read, LUT management and OTP pages, and only provides basic usage.
>
> OOB layout was defined to only show ECC-protected "user data I", leaving
> "user data II" unavailable.

Fine, there is not strong rule, and anyway this is only useful for non
UBI users (jffs2, typically).

> Signed-off-by: Daisuke Mizobuchi <mizo@atmark-techno.com>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
> Changes in v2:
> - Link to v1: https://patch.msgid.link/ansYUoNKJyDQ+6f2@hu-mdalam-blr.qualcomm.com
> - fixed oob layout
> - fixed NAND_MEMORG oob size
>
> I've tested the NAND with various mtd test utils (mtd_oobtest,
> mtd_pagetest, mtd_subpagetest, mtd_nandbiterrs and some mtd-utils
> userspace counterparts) with no problem, but I'm not sure what to do
> with the ooblayout free() informations: as written in the commit
> message, this chip has 2+12 bytes of user data per sector where the
> first two are not ECC protected and the later 12 are.
>
> I believe user should make the choice of which oob bytes they want to
> use, but afaik we can't say "this is not protected", so we can't let
> them choose and must either say both are free or only provide one like I
> did here.

Indeed.

> As an extra data point, this "user area I and II" distinction is the same
> in W25N04KW (same 2+12), but w25n02kv_ooblayout_free() use there returns
> the whole 14 bytes as a single chunk, so I explicitly made a different
> choice here.
> (I believe that's not something that can be changed easily, so we should
> discuss this before merging)

If it's the same layout, I would go for keeping the existing one and
avoid the proliferation of these helpers.

> That aside:
> - Alam, would you like your name somewhere in the commit? I didn't keep
>   anything from your commit because I already had one, but happy to add
>   a Co-developed-by or something
> - I kept NAND_ECCREQ(8, 512) like W25N04KW but the datasheet says it's
>   based on 8-bits/544-bytes ECC, so I should set it as (8, 544)?

No, we are protecting 512 bytes of data. 544 is the area including bytes
used for the ECC operation itself and we are not interested by those in
this field.

>   (since it's protecting 512+12 bytes for up to 8 bits corruption)
>   In practice ECC is done by hardware and looks like it is correctly
>   reported (e.g. manually overwriting a couple of bits with nandwrite
>   and checking with nanddump properly corrects and reports number of
>   corrected bits), but might as well get this right, even if as far as I
>   understand nothing actually uses the ECC oob data to double-check
>   hardware status?

No we do not. If nandbiterrs pass, normally it's all fine.

> - I'll shamefully admit I do not understand the read / write /
>   update_cache_variants() I copied from KW,
>   Alam had the same but it'd be great to confirm using the same
>   callbacks makes sense?

There are various operations available for I/Os. Depending on the spi
controller capability and the routing at the board level, you might want
to use single, dual, quad, dtr or not transfers. And depending on the
speed you may need extra dummy cycles. All of this is captured in this
array.

> - (I wrote about OTP in the commit message but have no plan of
>   implementing it at this point as we have no use for it, I still
>   intend to look at continuous read after some other work)

Fine!

Thanks,
Miquèl