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

[Miquel Raynal]

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

The underlying question is: will we add a tegra,use-bootable-ecc-only
property?

> 
> > In the documentation you can add
> > a note saying that using other algorithms than the three above is not
> > supported by the BootROM.
> >   
> > >     
> > > >>
> > > >> However, I think that allowing for an override of the oobsize
> > > >> inference would be a good idea before merging, no? This could just be
> > > >> a trivial #ifdef (at least temporarily). If you agree but don't feel
> > > >> like doing it yourself, I'd be happy to pitch in. Let me know.      
> > > > 
> > > > That's why we have nand-ecc-xxx properties in the DT.
> > > >       
> > > 
> > > Yes, nand-ecc-strength is the first thing I plan to implement, that way
> > > strength can be defined in dt.
> > > 
> > > --
> > > Stefan    
> > 
> > Thanks,
> > MiquÃl  
>