Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Stefan Agner]

On 27.05.2018 18:30, Boris Brezillon wrote:
> On Sun, 27 May 2018 17:54:03 +0200
> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> 
>> Hi Boris,
>>
>> On Sun, 27 May 2018 17:13:37 +0200, Boris Brezillon
>> <boris.brezillon@xxxxxxxxxxx> wrote:
>>
>> > On Sun, 27 May 2018 16:18:32 +0200
>> > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>> >
>> > > Hi Stefan,
>> > >
>> > > On Thu, 24 May 2018 14:19:18 +0200, Stefan Agner <stefan@xxxxxxxx>
>> > > wrote:
>> > >
>> > > > On 24.05.2018 13:53, Boris Brezillon wrote:
>> > > > > Hi Benjamin,
>> > > > >
>> > > > > On Thu, 24 May 2018 13:30:14 +0200
>> > > > > Benjamin Lindqvist <benjamin.lindqvist@xxxxxxxxx> wrote:
>> > > > >
>> > > > >> Hi Stefan,
>> > > > >>
>> > > > >> It seems to me that a probe similar to what the BootROM does shouldn't
>> > > > >> be awfully complicated to implement - just cycle through the switch
>> > > > >> cases in case of an ECC error. But I guess that's more of an idea for
>> > > > >> further improvements rather than a comment to the patch set under
>> > > > >> review.
>> > > > >
>> > > > > Nope, not really an option, because you're not guaranteed that the NAND
>> > > > > will be used as a boot media, and the first page or first set of pages
>> > > > > might just be erased.
>> > > > >
>> > > >
>> > > > Yeah I did not meant probing like the Boot ROM does.
>> > > >
>> > > > What I meant was using only the ECC modes which are supported by the
>> > > > Boot ROM when the driver tries to choose a viable mode. So that would
>> > > > be:
>> > > > - RS t=4
>> > > > - BCH t=8
>> > > > - BCH t=16
>> > > >
>> > > > Maybe we could add a property to enable that behavior:
>> > > >
>> > > > tegra,use-bootable-ecc-only;
>> > >
>> > > I'm not sure a property is needed.
>> > >
>> > > As there is currently no official user of this driver, why not turning
>> > > mandatory the nand-ecc-xxx properties?
>> >
>> > Not a big fan of this solution. We already have a few cases where the
>> > NAND part was changed on a design and the new NAND had different ECC
>> > requirements, With your suggestion, that means creating a new .dts file
>> > for each possible NAND part.
>> >
>> > Note that having a solution that picks the best ECC config based on
>> > chip->ecc_xxx_ds should be the preferred approach. nand-ecc- props are
>> > mainly here to address the case where you need/want to assign a config
>> > that does not match the ECC requirements exposed by the chip.
>>
>> Ok, that's right it's a problem.
>>
>> But then the driver has to choose a default algorithm if none is given.
> 
> Yep.
> 

Our design currently uses a chip which requires 8-bit per 512 byte. But
the downstream BSP currently uses the maximum possible strength, BCH 16.

I guess we could create two algorithms, one which tries to maximize ECC
strength and one which tries to match the required strength. Both with
and without the boot ROM restrictions...

But then, newer chips mostly require higher ECC strength. Given that we
already use the maximum, I think for that particular case just selecting
BCH 16 would be fine. Therefor I planned for v2 to just implement manual
selection...

>> In this case, should we select the one that fits best the NAND chip
>> requirements, or shall we limit to the ones supported by the BootRom?
> 
> We should limit to the one used by the BootROM only if the NAND is used
> as a boot medium.
> 
>>
>> The underlying question is: will we add a tegra,use-bootable-ecc-only
>> property?
> 
> I guess this one is fine, because it's only adding a constraint on the
> possible ECC modes that can be used, it's not forcing a specific ECC
> strength.
> 
> Note that if we want to make this property generic we could name it
> nand-is-boot-medium.

Sounds sensible. Although, only because the boot loader needs to be
written in that mode does not necessarily requires the rootfs to be in
the same mode.. But it is clearly preferable.

--
Stefan