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

[Dominique Martinet <dominique.martinet@atmark-techno.com>]

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?
----(untested)

static int w25n02kv_ooblayout_ecc(struct mtd_info *mtd, int section,
                                  struct mtd_oob_region *region)
{
	/* 4 sections for oobsize=128, 8 for oobsize=256 */
        if (section >= mtd->oobsize / 32)
                return -ERANGE;

        region->offset = (mtd->oobsize / 2) + (16 * section);
        region->length = 13;

        return 0;
}

static int w25n02kv_ooblayout_free(struct mtd_info *mtd, int section,
                                   struct mtd_oob_region *region)
{
        if (section >= mtd->oobsize / 32)
                return -ERANGE;

        region->offset = (16 * section) + 2;
        region->length = 14;

        return 0;
}
----

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


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

> > - 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?
the datasheet doesn't seem to handle any command with a *D so I'll
ignore the S/D...)

Ah, okay, the dummy delay is in byte and looking at the datasheet I need
one byte worth for 0x6b (1S_1S_4S, 8 cycles at 1 line) and two bytes
worth for 0xeb (1S_4S_4S, 4 cycles at 4 lines) so this makes sense, I'll
take some time to check the other ops before submitting v3.


(And I guess that's where I'll need to add stuff for continuous read
too, it doesn't look like I can reuse the
cont_read_cache_dual_quad_dtr_variants as the ops you added don't seem
to exist here, if I understood it right I *think* I need to set some
state register then with the same commands keeping CS down/clock ticking
should keep providing data...? Well; I'll look it up some more too
before bothering you again)

Thank you again,
-- 
Dominique



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