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

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

2018-04-23 Thread Abhishek Sahu


On 2018-04-22 22:04, Miquel Raynal wrote:

Hi Abhishek,

On Thu, 12 Apr 2018 15:29:48 +0530, Abhishek Sahu
 wrote:


On 2018-04-10 13:37, Boris Brezillon wrote:
> On Tue, 10 Apr 2018 09:55:58 +0200
> Miquel Raynal  wrote:
> >> > Hi Abhishek,
>> >
>> > On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
>> >  wrote:
>> >
>> > > On 2018-04-06 18:01, Miquel Raynal wrote:
>> > > > Hi Abhishek,
>> > > >
>> > > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
>> > > >  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

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

2018-04-23 Thread Abhishek Sahu


On 2018-04-22 22:04, Miquel Raynal wrote:

Hi Abhishek,

On Thu, 12 Apr 2018 15:29:48 +0530, Abhishek Sahu
 wrote:


On 2018-04-10 13:37, Boris Brezillon wrote:
> On Tue, 10 Apr 2018 09:55:58 +0200
> Miquel Raynal  wrote:
> >> > Hi Abhishek,
>> >
>> > On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
>> >  wrote:
>> >
>> > > On 2018-04-06 18:01, Miquel Raynal wrote:
>> > > > Hi Abhishek,
>> > > >
>> > > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
>> > > >  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,

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

2018-04-22 Thread Miquel Raynal

Hi Abhishek,

On Thu, 12 Apr 2018 15:29:48 +0530, Abhishek Sahu
 wrote:

> On 2018-04-10 13:37, Boris Brezillon wrote:
> > On Tue, 10 Apr 2018 09:55:58 +0200
> > Miquel Raynal  wrote:  
> > >> > Hi Abhishek,  
> >> >
> >> > On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
> >> >  wrote:
> >> >  
> >> > > On 2018-04-06 18:01, Miquel Raynal wrote:  
> >> > > > Hi Abhishek,
> >> > > >
> >> > > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> >> > > >  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

Thanks,
Miquèl

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

2018-04-22 Thread Miquel Raynal

Hi Abhishek,

On Thu, 12 Apr 2018 15:29:48 +0530, Abhishek Sahu
 wrote:

> On 2018-04-10 13:37, Boris Brezillon wrote:
> > On Tue, 10 Apr 2018 09:55:58 +0200
> > Miquel Raynal  wrote:  
> > >> > Hi Abhishek,  
> >> >
> >> > On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
> >> >  wrote:
> >> >  
> >> > > On 2018-04-06 18:01, Miquel Raynal wrote:  
> >> > > > Hi Abhishek,
> >> > > >
> >> > > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> >> > > >  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

Thanks,
Miquèl

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

2018-04-12 Thread Abhishek Sahu


On 2018-04-10 13:37, Boris Brezillon wrote:

On Tue, 10 Apr 2018 09:55:58 +0200
Miquel Raynal  wrote:


> Hi Abhishek,
>
> On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
>  wrote:
>
> > On 2018-04-06 18:01, Miquel Raynal wrote:
> > > Hi Abhishek,
> > >
> > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> > >  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.

 Thanks,
 Abhishek


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

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

2018-04-12 Thread Abhishek Sahu


On 2018-04-10 13:37, Boris Brezillon wrote:

On Tue, 10 Apr 2018 09:55:58 +0200
Miquel Raynal  wrote:


> Hi Abhishek,
>
> On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
>  wrote:
>
> > On 2018-04-06 18:01, Miquel Raynal wrote:
> > > Hi Abhishek,
> > >
> > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> > >  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.

 Thanks,
 Abhishek


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

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

2018-04-10 Thread Boris Brezillon

On Tue, 10 Apr 2018 09:55:58 +0200
Miquel Raynal  wrote:

> > Hi Abhishek,
> > 
> > On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
> >  wrote:
> >   
> > > On 2018-04-06 18:01, Miquel Raynal wrote:
> > > > Hi Abhishek,
> > > > 
> > > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> > > >  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.

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

2018-04-10 Thread Boris Brezillon

On Tue, 10 Apr 2018 09:55:58 +0200
Miquel Raynal  wrote:

> > Hi Abhishek,
> > 
> > On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
> >  wrote:
> >   
> > > On 2018-04-06 18:01, Miquel Raynal wrote:
> > > > Hi Abhishek,
> > > > 
> > > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> > > >  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.

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

2018-04-10 Thread Miquel Raynal


> Hi Abhishek,
> 
> On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
>  wrote:
> 
> > On 2018-04-06 18:01, Miquel Raynal wrote:  
> > > Hi Abhishek,
> > > 
> > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> > >  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.
Depending on what part you are using, it might already work.

> 
> >   
> > > 2/ Create another DT for the board.
> > > 
> > 
> >   Its not about board but about part. We have IPQ8074 HK01 board
> >   with 4 bit ECC chip/8 bit ECC chip/non ONFI/JEDEC chip.
> >   
> > > However, there is something to do in this driver: the strength chosen
> > > should be limited to the controller capabilities (8-bit/512B if I
> > > understand correctly). In this case you have two options: either you
> > > limit the strength like the size [1] if (ecc->strength > 8);
> > 
> >   Limiting the strength will make all the boards with ecc strength > 8
> >   to fail completely
> >   
> > > just limit the maximum strength to 8 like this [2] and the core will
> > > spawn a warning in the dmesg telling that the ECC strength used is
> > > below the NAND chip requirements.
> > 
> >   Yes its good idea. I can update the patch with dmesg warning.
> > 
> >   Thanks,
> >   Abhishek
> >   
> > > 
> > > Thanks,
> > > Miquèl
> > > 
> > > [1]
> > > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2332
> > > [2] http://code.bulix.org/nyf63w-315268
> > > 
> > > 
> > >> 
> > >> Signed-off-by: Abhishek Sahu 
> > >> ---
> > >>  drivers/mtd/nand/qcom_nandc.c | 8 
> > >>  1 file changed, 8 insertions(+)
> > >>

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

2018-04-10 Thread Miquel Raynal


> Hi Abhishek,
> 
> On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
>  wrote:
> 
> > On 2018-04-06 18:01, Miquel Raynal wrote:  
> > > Hi Abhishek,
> > > 
> > > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> > >  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.
Depending on what part you are using, it might already work.

> 
> >   
> > > 2/ Create another DT for the board.
> > > 
> > 
> >   Its not about board but about part. We have IPQ8074 HK01 board
> >   with 4 bit ECC chip/8 bit ECC chip/non ONFI/JEDEC chip.
> >   
> > > However, there is something to do in this driver: the strength chosen
> > > should be limited to the controller capabilities (8-bit/512B if I
> > > understand correctly). In this case you have two options: either you
> > > limit the strength like the size [1] if (ecc->strength > 8);
> > 
> >   Limiting the strength will make all the boards with ecc strength > 8
> >   to fail completely
> >   
> > > just limit the maximum strength to 8 like this [2] and the core will
> > > spawn a warning in the dmesg telling that the ECC strength used is
> > > below the NAND chip requirements.
> > 
> >   Yes its good idea. I can update the patch with dmesg warning.
> > 
> >   Thanks,
> >   Abhishek
> >   
> > > 
> > > Thanks,
> > > Miquèl
> > > 
> > > [1]
> > > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2332
> > > [2] http://code.bulix.org/nyf63w-315268
> > > 
> > > 
> > >> 
> > >> Signed-off-by: Abhishek Sahu 
> > >> ---
> > >>  drivers/mtd/nand/qcom_nandc.c | 8 
> > >>  1 file changed, 8 insertions(+)
> > >> 
> > >> diff --git a/drivers/mtd/nand/qcom_nandc.c 
> > >>

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

2018-04-10 Thread Miquel Raynal

Hi Abhishek,

On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
 wrote:

> On 2018-04-06 18:01, Miquel Raynal wrote:
> > Hi Abhishek,
> > 
> > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> >  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.

> 
> > 2/ Create another DT for the board.
> >   
> 
>   Its not about board but about part. We have IPQ8074 HK01 board
>   with 4 bit ECC chip/8 bit ECC chip/non ONFI/JEDEC chip.
> 
> > However, there is something to do in this driver: the strength chosen
> > should be limited to the controller capabilities (8-bit/512B if I
> > understand correctly). In this case you have two options: either you
> > limit the strength like the size [1] if (ecc->strength > 8);  
> 
>   Limiting the strength will make all the boards with ecc strength > 8
>   to fail completely
> 
> > just limit the maximum strength to 8 like this [2] and the core will
> > spawn a warning in the dmesg telling that the ECC strength used is
> > below the NAND chip requirements.  
> 
>   Yes its good idea. I can update the patch with dmesg warning.
> 
>   Thanks,
>   Abhishek
> 
> > 
> > Thanks,
> > Miquèl
> > 
> > [1]
> > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2332
> > [2] http://code.bulix.org/nyf63w-315268
> > 
> >   
> >> 
> >> Signed-off-by: Abhishek Sahu 
> >> ---
> >>  drivers/mtd/nand/qcom_nandc.c | 8 
> >>  1 file changed, 8 insertions(+)
> >> 
> >> diff --git a/drivers/mtd/nand/qcom_nandc.c 
> >> b/drivers/mtd/nand/qcom_nandc.c
> >> index 563b759..8dd40de 100644
> >> --- a/drivers/mtd/nand/qcom_nandc.c
> >> +++ b/drivers/mtd/nand/qcom_nandc.c
> >> @@ -2334,6 +2334,14 @@ static int qcom_nand_host_setup(struct 
> >> qcom_nand_host *host)
> >>return -EINVAL;
> >>}
> >> 
> >> +  /*
> >> +   * Read the required ecc strength from NAND device and overwrite
> >> +   * the device tree ecc strength for devices which require
> >> +   * ecc correctability bits >= 8
> >> +   */
> >> +  if

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

2018-04-10 Thread Miquel Raynal

Hi Abhishek,

On Tue, 10 Apr 2018 11:39:35 +0530, Abhishek Sahu
 wrote:

> On 2018-04-06 18:01, Miquel Raynal wrote:
> > Hi Abhishek,
> > 
> > On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
> >  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.

> 
> > 2/ Create another DT for the board.
> >   
> 
>   Its not about board but about part. We have IPQ8074 HK01 board
>   with 4 bit ECC chip/8 bit ECC chip/non ONFI/JEDEC chip.
> 
> > However, there is something to do in this driver: the strength chosen
> > should be limited to the controller capabilities (8-bit/512B if I
> > understand correctly). In this case you have two options: either you
> > limit the strength like the size [1] if (ecc->strength > 8);  
> 
>   Limiting the strength will make all the boards with ecc strength > 8
>   to fail completely
> 
> > just limit the maximum strength to 8 like this [2] and the core will
> > spawn a warning in the dmesg telling that the ECC strength used is
> > below the NAND chip requirements.  
> 
>   Yes its good idea. I can update the patch with dmesg warning.
> 
>   Thanks,
>   Abhishek
> 
> > 
> > Thanks,
> > Miquèl
> > 
> > [1]
> > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2332
> > [2] http://code.bulix.org/nyf63w-315268
> > 
> >   
> >> 
> >> Signed-off-by: Abhishek Sahu 
> >> ---
> >>  drivers/mtd/nand/qcom_nandc.c | 8 
> >>  1 file changed, 8 insertions(+)
> >> 
> >> diff --git a/drivers/mtd/nand/qcom_nandc.c 
> >> b/drivers/mtd/nand/qcom_nandc.c
> >> index 563b759..8dd40de 100644
> >> --- a/drivers/mtd/nand/qcom_nandc.c
> >> +++ b/drivers/mtd/nand/qcom_nandc.c
> >> @@ -2334,6 +2334,14 @@ static int qcom_nand_host_setup(struct 
> >> qcom_nand_host *host)
> >>return -EINVAL;
> >>}
> >> 
> >> +  /*
> >> +   * Read the required ecc strength from NAND device and overwrite
> >> +   * the device tree ecc strength for devices which require
> >> +   * ecc correctability bits >= 8
> >> +   */
> >> +  if (chip->ecc_strength_ds >= 8)
> >> +  ecc->strength = 8;
> >> +
> >>

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

2018-04-10 Thread Abhishek Sahu


On 2018-04-06 18:01, Miquel Raynal wrote:

Hi Abhishek,

On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
 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
 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.


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. We can't remove since
   if the device is Non ONFI/JEDEC, then ecc strength will come
   from DT only.


2/ Create another DT for the board.



 Its not about board but about part. We have IPQ8074 HK01 board
 with 4 bit ECC chip/8 bit ECC chip/non ONFI/JEDEC chip.


However, there is something to do in this driver: the strength chosen
should be limited to the controller capabilities (8-bit/512B if I
understand correctly). In this case you have two options: either you
limit the strength like the size [1] if (ecc->strength > 8);


 Limiting the strength will make all the boards with ecc strength > 8
 to fail completely


just limit the maximum strength to 8 like this [2] and the core will
spawn a warning in the dmesg telling that the ECC strength used is
below the NAND chip requirements.


 Yes its good idea. I can update the patch with dmesg warning.

 Thanks,
 Abhishek



Thanks,
Miquèl

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2332
[2] http://code.bulix.org/nyf63w-315268




Signed-off-by: Abhishek Sahu 
---
 drivers/mtd/nand/qcom_nandc.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/qcom_nandc.c 
b/drivers/mtd/nand/qcom_nandc.c

index 563b759..8dd40de 100644
--- a/drivers/mtd/nand/qcom_nandc.c
+++ b/drivers/mtd/nand/qcom_nandc.c
@@ -2334,6 +2334,14 @@ static int qcom_nand_host_setup(struct 
qcom_nand_host *host)

return -EINVAL;
}

+   /*
+* Read the required ecc strength from NAND device and overwrite
+* the device tree ecc strength for devices which require
+* ecc correctability bits >= 8
+*/
+   if (chip->ecc_strength_ds >= 8)
+   ecc->strength = 8;
+
wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;

if (ecc->strength >= 8) {

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

2018-04-10 Thread Abhishek Sahu


On 2018-04-06 18:01, Miquel Raynal wrote:

Hi Abhishek,

On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
 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
 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.


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. We can't remove since
   if the device is Non ONFI/JEDEC, then ecc strength will come
   from DT only.


2/ Create another DT for the board.



 Its not about board but about part. We have IPQ8074 HK01 board
 with 4 bit ECC chip/8 bit ECC chip/non ONFI/JEDEC chip.


However, there is something to do in this driver: the strength chosen
should be limited to the controller capabilities (8-bit/512B if I
understand correctly). In this case you have two options: either you
limit the strength like the size [1] if (ecc->strength > 8);


 Limiting the strength will make all the boards with ecc strength > 8
 to fail completely


just limit the maximum strength to 8 like this [2] and the core will
spawn a warning in the dmesg telling that the ECC strength used is
below the NAND chip requirements.


 Yes its good idea. I can update the patch with dmesg warning.

 Thanks,
 Abhishek



Thanks,
Miquèl

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2332
[2] http://code.bulix.org/nyf63w-315268




Signed-off-by: Abhishek Sahu 
---
 drivers/mtd/nand/qcom_nandc.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/qcom_nandc.c 
b/drivers/mtd/nand/qcom_nandc.c

index 563b759..8dd40de 100644
--- a/drivers/mtd/nand/qcom_nandc.c
+++ b/drivers/mtd/nand/qcom_nandc.c
@@ -2334,6 +2334,14 @@ static int qcom_nand_host_setup(struct 
qcom_nand_host *host)

return -EINVAL;
}

+   /*
+* Read the required ecc strength from NAND device and overwrite
+* the device tree ecc strength for devices which require
+* ecc correctability bits >= 8
+*/
+   if (chip->ecc_strength_ds >= 8)
+   ecc->strength = 8;
+
wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;

if (ecc->strength >= 8) {

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

2018-04-06 Thread Miquel Raynal

Hi Abhishek,

On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
 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.

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

IMHO, you have two solutions:
1/ Remove these properties from the board DT (breaks DT backward
compatibility though);
2/ Create another DT for the board.

However, there is something to do in this driver: the strength chosen
should be limited to the controller capabilities (8-bit/512B if I
understand correctly). In this case you have two options: either you
limit the strength like the size [1] if (ecc->strength > 8); or you
just limit the maximum strength to 8 like this [2] and the core will
spawn a warning in the dmesg telling that the ECC strength used is
below the NAND chip requirements.

Thanks,
Miquèl

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2332
[2] http://code.bulix.org/nyf63w-315268

> 
> Signed-off-by: Abhishek Sahu 
> ---
>  drivers/mtd/nand/qcom_nandc.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c
> index 563b759..8dd40de 100644
> --- a/drivers/mtd/nand/qcom_nandc.c
> +++ b/drivers/mtd/nand/qcom_nandc.c
> @@ -2334,6 +2334,14 @@ static int qcom_nand_host_setup(struct qcom_nand_host 
> *host)
>   return -EINVAL;
>   }
>  
> + /*
> +  * Read the required ecc strength from NAND device and overwrite
> +  * the device tree ecc strength for devices which require
> +  * ecc correctability bits >= 8
> +  */
> + if (chip->ecc_strength_ds >= 8)
> + ecc->strength = 8;
> +
>   wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
>  
>   if (ecc->strength >= 8) {

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

2018-04-06 Thread Miquel Raynal

Hi Abhishek,

On Wed,  4 Apr 2018 18:12:17 +0530, Abhishek Sahu
 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.

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

IMHO, you have two solutions:
1/ Remove these properties from the board DT (breaks DT backward
compatibility though);
2/ Create another DT for the board.

However, there is something to do in this driver: the strength chosen
should be limited to the controller capabilities (8-bit/512B if I
understand correctly). In this case you have two options: either you
limit the strength like the size [1] if (ecc->strength > 8); or you
just limit the maximum strength to 8 like this [2] and the core will
spawn a warning in the dmesg telling that the ECC strength used is
below the NAND chip requirements.

Thanks,
Miquèl

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2332
[2] http://code.bulix.org/nyf63w-315268

> 
> Signed-off-by: Abhishek Sahu 
> ---
>  drivers/mtd/nand/qcom_nandc.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c
> index 563b759..8dd40de 100644
> --- a/drivers/mtd/nand/qcom_nandc.c
> +++ b/drivers/mtd/nand/qcom_nandc.c
> @@ -2334,6 +2334,14 @@ static int qcom_nand_host_setup(struct qcom_nand_host 
> *host)
>   return -EINVAL;
>   }
>  
> + /*
> +  * Read the required ecc strength from NAND device and overwrite
> +  * the device tree ecc strength for devices which require
> +  * ecc correctability bits >= 8
> +  */
> + if (chip->ecc_strength_ds >= 8)
> + ecc->strength = 8;
> +
>   wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
>  
>   if (ecc->strength >= 8) {

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

16 matches

Site Navigation

Mail list logo

Footer information