[PATCH 01/14] mtd: nand: brcm: switch to mtd_ooblayout_ops

[Michael Nazzareno Trimarchi]

Hi William

On Thu, Jan 26, 2023 at 6:39 PM William Zhang
<william.zhang at broadcom.com> wrote:
>
>
>
> On 01/26/2023 12:43 AM, Linus Walleij wrote:
> > On Thu, Jan 26, 2023 at 2:02 AM William Zhang
> > <william.zhang at broadcom.com> wrote:
> >

Can you add your review-by?

Michael

> >> Unfortunately the u-boot nand base code still uses nand_ecclayout
> >> structure because it was based on old kernel nand driver.  Your change
> >> cause bugcheck in the nand_scan_tail at line 4978 when mtd->oobsize is
> >> not one of the default size (i.e. some large nand with BCH-8 ecc
> >> requirement and has 224 bytes oobsize per 4K page) because ecc->layout
> >> is never set. Also certainly any data built based on nand_cclayout like
> >> mtd->oobavail will not be correct.
> >>
> >> I actually converted back to nand_ecclayout structure from mtd_ooblayout
> >> with this fix to solve the above issues.
> >> Fixes: e365de90517b ("drivers: nand: brcmnand: fix nand_chip ecc layout
> >> structure")
> >
> > Argh yeah I see. Let's hold this series off then.
> > It was worth a try!
> >
> >> I can see your point to get the latest from the brcmnand linux kernel
> >> driver but this requires updating the u-boot nand base driver to use
> >> mtd_ooblayout as well and all others nand controller drivers too. I am
> >> not sure if this is something you want to tackle right now.
> >
> > No I can't do that I do not have a big enough experience with NAND
> > flash and no testbed for it either.
> >
> >> As far as I can see, all other oob/ecc layout setting patches you back
> >> ported in this series are not in the original brcmstb_choose_ecc_layout
> >> code you replaced. So I am not worried about that we must switch to
> >> mtd_ooblayout_ops at this point.  If indeed there is bug in
> >> brcmstb_choose_ecc_layout, we can port and convert the fix to
> >> nand_ecclayout structure from kernel code.
> >
> > OK no they seemed to be mostly improvements of the algorithm so we
> > can certainly live without.
> >
> >> Was the bug you were hunting down in the code related to this patch?
> >
> > Actually not, I think, it's one of the other patches I sent, the one enabling
> > BCH-1 by reading the proper ECC properties from the device tree.
> > That made it finally work.
>  >
> > The iproc NAND driver I sent should also work pretty much as-is, nothing
> > depends on these backports.
> >
> Yes the patches that are not relate to the ooblayout should still be
> good to have.
>
> > Yours,
> > Linus Walleij
> >



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael at amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info at amarulasolutions.com
www.amarulasolutions.com