Re: [PATCH 1/9] mtd: nand: qcom: use the ecc strength from device parameter

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

On Tue, 10 Apr 2018 09:55:58 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> > Hi Abhishek,
> > 
> > On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
> > <absahu@codeaurora.org> wrote:
> >   
> > > On 2018-04-06 18:01, Miquel Raynal wrote:    
> > > > Hi Abhishek,
> > > > 
> > > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> > > > <absahu@codeaurora.org> wrote:
> > > >       
> > > >> Currently the driver uses the ECC strength specified in
> > > >> device tree. The ONFI or JEDEC device parameter page
> > > >> contains the ‘ECC correctability’ field which indicates the
> > > >> number of bits that the host should be able to correct per
> > > >> 512 bytes of data.      
> > > > 
> > > > This is misleading. This field is not about the controller but rather
> > > > the chip requirements in terms of minimal strength for nominal use.
> > > >       
> > > 
> > >   Thanks Miquel.
> > > 
> > >   Yes. Its NAND chip requirement. I have used the description for
> > >   NAND ONFI param page
> > > 
> > >   5.6.1.24. Byte 112: Number of bits ECC correctability
> > >   This field indicates the number of bits that the host should be
> > >   able to correct per 512 bytes of data.
> > >     
> > > >> The ecc correctability is assigned in
> > > >> chip parameter during device probe time. QPIC/EBI2 NAND
> > > >> supports 4/8-bit ecc correction. The Same kind of board
> > > >> can have different NAND parts so use the ecc strength
> > > >> from device parameter (if its non zero) instead of
> > > >> device tree.      
> > > > 
> > > > That is not what you do.
> > > > 
> > > > What you do is forcing the strength to be 8-bit per ECC chunk if the
> > > > NAND chip requires at least 8-bit/chunk strength.
> > > > 
> > > > The DT property is here to force a strength. Otherwise, Linux will
> > > > propose to the NAND controller to use the minimum strength required by
> > > > the chip (from either the ONFI/JEDEC parameter page or from a static
> > > > table).
> > > >       
> > > 
> > >   The main problem is that the same kind of boards can have different
> > >   NAND parts.
> > > 
> > >   Lets assume, we have following 2 cases.
> > > 
> > >   1. Non ONFI/JEDEC device for which chip->ecc_strength_ds
> > >      will be zero. In this case, the ecc->strength from DT
> > >      will be used    
> > 
> > No, the strength from DT will always be used if the property is
> > present, no matter the type of chip.
> >   
> > >   2. ONFI/JEDEC device for which chip->ecc_strength_ds > 8.
> > >      Since QCOM nand controller can not support
> > >      ECC correction greater than 8 bits so we can use 8 bit ECC
> > >      itself instead of failing NAND boot completely.    
> > 
> > I understand that. But this is still not what you do.
> >   
> > >     
> > > > IMHO, you have two solutions:
> > > > 1/ Remove these properties from the board DT (breaks DT backward
> > > > compatibility though);      
> > > 
> > >   - nand-ecc-strength: This is optional property in nand.txt and
> > >     Required property in qcom_nandc.txt.    
> > 
> > Well, this property is not controller specific but chip specific. The
> > controller driver does not rely on it, so this property should not be
> > required.
> >   
> > > We can't remove since
> > >     if the device is Non ONFI/JEDEC, then ecc strength will come
> > >     from DT only.    
> > 
> > We can remove it and let the core handle this (as this is generic to
> > all raw NANDs and not specific to this controller driver). Try it out!
> > 
> > However if the defaults value do not match your expectations, I think
> > you can add your non-ONFI/JEDEC chip in 'nand_ids.c', this should fill
> > your strength_ds field and let you avoid using these properties.  
> 
> Actually nand_ids.c should not be filled anymore, instead you can
> implement this detection thanks to the part full name in the vendor
> code nand_samsung.c, nand_micron.c, nand_macronix.c, nand_hynix.c, etc.

Usually you don't have to go that far, and the ECC requirements are
encoded somewhere in the ID (after byte 2). When that's not the
case, you may have to check the full ID.

> Depending on what part you are using, it might already work.

Yep.