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

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Sun, 27 May 2018 17:54:03 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> On Sun, 27 May 2018 17:13:37 +0200, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> 
> > On Sun, 27 May 2018 16:18:32 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hi Stefan,
> > > 
> > > On Thu, 24 May 2018 14:19:18 +0200, Stefan Agner <stefan@agner.ch>
> > > 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@endian.se> 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.

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