Re: [PATCH v2] mtd: rawnand: xway: No hardcoded ECC engine, use device tree setting

[Kestrel seventyfour]

Hi Miquèl,

Am Di., 24. Aug. 2021 um 19:22 Uhr schrieb Miquel Raynal
<miquel.raynal@xxxxxxxxxxx>:
>
> Hello,
>
> Kestrel seventyfour <kestrelseventyfour@xxxxxxxxx> wrote on Tue, 24 Aug
> 2021 09:15:49 +0200:
>
> > Hi Miquèl,
> >
> > Am Mo., 23. Aug. 2021 um 17:24 Uhr schrieb Miquel Raynal
> > <miquel.raynal@xxxxxxxxxxx>:
> > >
> > > Hi Kestrel,
> > >
> > > Kestrel seventyfour <kestrelseventyfour@xxxxxxxxx> wrote on Mon, 23 Aug
> > > 2021 13:19:43 +0200:
> > >
> > > > Hi Miquèl,
> > > >
> > > > Am Do., 19. Aug. 2021 um 10:03 Uhr schrieb Miquel Raynal
> > > > <miquel.raynal@xxxxxxxxxxx>:
> > ...
> > > >
> > > > thank you for your response.
> > > > If I remove the nand-ecc-xxx properties in the device tree, the device with
> > > > the Toshiba NAND chip is working. However, the device with the Micron
> > > > NAND fails with NO ECC functions supplied; hardware ECC not possible,
> > > > seems to be at line 5367 or equivalent.
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L5367
> > > >
> > > > It looks like the micron nand driver supports on die only if its
> > > > specified int the
> > > > Device tree:
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_micron.c#L511
> > > > The Micron NAND driver probably needs to set the ECC type to ON DIE if the
> > > > variable ondie contains the supported attribute?!
> > >
> > > You're right but I don't see any easy upstream-able solution here.
> > > Changing the behavior in the Xway driver would certainly break users,
> > > changing the behavior in the Micron driver would certainly break even
> > > more users. The root cause being an absence of proper description (the
> > > integration changed). Honestly I feel stuck, maybe you can try to
> > > register your device, if it fails, change the integration in the driver
> > > (to an ondie ecc engine) then retry?
> > >
> > > Thanks,
> > > Miquèl
> >
> > Do you think adding something like below at the following location
> > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/xway_nand.c#L223
> > would be upstreamable (with or without device tree property?)?
> >
> >         err = nand_scan(&data->chip, 1);
> >         if (err /* && of_property_read_bool(np, "lantiq,retry-on-die") */) {
> >                 data->chip.ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_DIE;
> >                 err = nand_scan(&data->chip, 1);
> >                 if (err) return err;
> >         }
> >
> > It still throws the kernel warning on first try, but the second try then works.
>
> Can you please remind me what is xway/lantiq/your setup/how public it
> is/who's using this driver?
>
> Thanks,
> Miquèl

Its for Openwrt, I would like to add support for 3 more devices
AVM fritzbox 3490/5490 and 7490. They all have varying NAND chips.
I have initially created a PR to have my initial patch tested:
https://github.com/openwrt/openwrt/pull/4426
There is already one device supported which has two DTBs one for
Micron and one for non Micron (3370), but its not very straight forward.
Without having this issue solved, flashing those devices would be
possibly having issues depending on NAND chip or the awkward
workaround of flashing one image and if it does not boot, boot the
other one. Without self soldered serial console, it would not very
easy to figure out the NAND manufacturer.
The AVM stock firmware is old kernel and does not use device
tree for NAND, they just query all possible manufacturers and set
up NAND based on manufacturer query.

Thanks, Daniel.