| Message ID | 1522845745-6624-2-git-send-email-absahu@codeaurora.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Update for QCOM NAND driver | expand |
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. > 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 <absahu@codeaurora.org> > --- > 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) {
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 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 <absahu@codeaurora.org> >> --- >> 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) {
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. > > > 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 <absahu@codeaurora.org> > >> --- > >> 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) { > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ Thanks, Miquèl
> 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. 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 <absahu@codeaurora.org> > > >> --- > > >> 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) { > > > > ______________________________________________________ > > Linux MTD discussion mailing list > > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > Thanks, Miquèl
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.
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. 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.
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 Thanks, Miquèl
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) 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
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 > > 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
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
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) {
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. 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. Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> --- drivers/mtd/nand/qcom_nandc.c | 8 ++++++++ 1 file changed, 8 insertions(+)