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

[Abhishek Sahu]

On 2018-04-22 22:04, Miquel Raynal wrote:
> Hi Abhishek,
>
> On Thu, 12 Apr 2018 15:29:48 +0530, Abhishek Sahu
> <absahu@xxxxxxxxxxxxxx> wrote:
>
> > On 2018-04-10 13:37, Boris Brezillon wrote:
> > > On Tue, 10 Apr 2018 09:55:58 +0200
> > > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> > > >> > Hi Abhishek,
> > >> >
> > >> > On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
> > >> > <absahu@xxxxxxxxxxxxxx> wrote:
> > >> >
> > >> > > On 2018-04-06 18:01, Miquel Raynal wrote:
> > >> > > > Hi Abhishek,
> > >> > > >
> > >> > > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> > >> > > > <absahu@xxxxxxxxxxxxxx> 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!
> >
> >   Thanks Boris and Miquel for your inputs.
> >
> >   Just want to confirm if already its implemented in core layer
> >   or shall I explore regrading this option.
> >
> >   I checked by removing this property alone from dtsi and it was
> >   failing with
> >
> >   "Driver must set ecc.strength when using hardware ECC"
> >
> >   I checked the code in nand_base.c also but couldn't get
> >   anything related with this.
>
> I don't know exactly what you did but you should have a look at what
> lead you to this path:
> https://elixir.bootlin.com/linux/v4.17-rc1/source/drivers/mtd/nand/raw/nand_base.c#L6421

 Our driver supports both ECC strength 4 bits and 8 bits
 and normally till now, we need to specify the ecc strength in device 
tree.

 Now, since same board can have different ECC strength chip so we
 can't fix the ecc strength in device tree and we need to look
 the required correction in ONFI param.

 We can have some code in generic layer which

 1. Provides the way to specify the supported strength in DT by NAND
    controller (for our case, it is 4 and 8)
 2. Read the chip ONFI/JEDEC Param and choose the configure to use
    controller strength according to its requirement.
 3. For Non ONFI/JEDEC devices, choose the maximum strength according
    to OOB bytes.

 I just want to check if we have something like this already in place
 or I can add the same in generic code so that this can be used by
 other drivers also.

 Thanks,
 Abhishek