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

On 13/08/2026 at 18:30:28 +09, Dominique Martinet <[email protected]> wrote:

> Thanks for the feedback!
>
> Miquel Raynal wrote on Thu, Aug 13, 2026 at 10:19:08AM +0200:
>> > 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).
>
> Yeah, I was expecting UBI to use it too but I looked it up after posting
> and saw it doesn't, so I agree it doesn't matter much for us either way.
>
>> > 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.
>
> The layout is similar (each section has 2 bytes for BB marker / 2 bytes
> user data II / 12 bytes user data I), but the number of sections per
> erase block is different (went from 4 to 8)
> For ECC it's the same (4 to 8 sections, and offset changed from 64 to
> 128 accordingly because there were more data sections before)
>
> I see similar helpers in other drivers (e.g. alliancememory.c) use
> mtd->oobsize; so if you prefer I can reuse the existing
> w25n02kv_ooblayout* helpers based on this?

Yes, that would be ideal.

> (
>   Technically could go as far as checking the ecc strength[1] and also
>   merge in w25n01kv_ooblayout_ecc (variable ecc length/offset step),
>   then if we're greedy also handle oobsize == 64 to replace
>   w25m02gv_ooblayout_ecc/w25m02gv_ooblayout_free, but that's starting to
>   make the functions more complex than I'm confident in... :)
>   [1] mtd_to_nanddev(mtd)->eccreq.strength
> )

Up to you :)

> (And in that case I'll happily give up on the non-ECC distinction and
> keep the current behavior that exposes the non-protected bits)
>
>> > 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.
>
> There's also the 12 bytes in "user data I", but I agree it's probably
> fine to ignore here.
> nandbiterrs passes, so I'll leave it as is.

https://elixir.bootlin.com/linux/v7.1.8/source/include/linux/mtd/nand.h#L187
It is not very clear, but this macro is about the main data ("bytes per step").

>> > - 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.
>
> Ok, so I ought to check that e.g.
> SPINAND_PAGE_READ_FROM_CACHE_1S_4S_4S_OP still needs 2 dummy op
> delay?
> (I couldn't find the _1S_4S_4S_ meaning explained anywhere, but looking
> at the commands used the number is the number of lines used for the
> command instruction itself, the column address and the data itself?

Yes.

Dummy width is the same as address width BTW.

> the datasheet doesn't seem to handle any command with a *D so I'll
> ignore the S/D...)

SDR vs DTR. You chip may or may not support it.

Good luck!
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/