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

[Abhishek Sahu <absahu@codeaurora.org>]

On 2018-04-23 12:35, Miquel Raynal wrote:
> Hi Abhishek,
> 
> Reduced the cc: list.
> 
> On Mon, 23 Apr 2018 12:14:32 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> On 2018-04-22 22:04, Miquel Raynal wrote:
>> > Hi Abhishek,
>> > > On Thu, 12 Apr 2018 15:29:48 +0530, Abhishek Sahu
>> > <absahu@codeaurora.org> wrote:
>> > >> On 2018-04-10 13:37, Boris Brezillon wrote:
>> >> > 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!
>> >> >>   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)
> 
> This is already the case, right? You use the DT to give the desired
> strength. As discussed earlier, let's forget about this option and
> focus on 2/ and 3/.
> 
>>   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.
> 
> Both of them are already handled. A lot of controller drivers rely on
> this logic already. Remove both ECC strength and size DT properties and
> add traces in the core to figure out why it rejected your chip.
> 
> We might help you if you provide more information.
> 
> Regards,
> Miquèl
> 

  Thanks a lot for your help Miquel!!!

  I got the required functions which we need to invoke inside
  our driver

  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c8f8afa7f92acb07641bf95b940d384ed1d0294

  I will do the changes accordingly.

  Regards,
  Abhishek

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