Update for QCOM NAND driver
Subsystems (6)
v2 cover Update for QCOM NAND driver
* v2:
1. Addressed all review comments in v1.
1. Make the generic helper function for NAND ECC parameters setup and used this helper function for QCOM and Denali nand driver for ECC setup.
2. Modified commit message for some of the patches and added more comments.
3. Added new patch for fixing ‘return 0’ for raw read.
4. Removed the read last codeword part for nand oob write.
5. Reorganized bad block check function and removed the read_last_cw function completely.
* v1:
This patch series mainly deals with error handling and erased page bitflip detection for QCOM NAND driver.
1. The error handling was missing for some of the cases so fixed the same.
2. Add the support for taking ECC strength from ONFI parameter. The earlier QCOM boards were coming with 4-bit ECC chip but now the same boards are coming with 8-bit ECC chip since the earlier 4-bit parts are obsolete from some vendors.
3. We got few issues related with NAND erased page bitflips. The QCOM NAND controller can’t detect the bitflip in completely erased page so added the support to detect the same. It implemented the logic mentioned in patch [1] which didn’t go in mainline and later the generic functions were provided [2] to count the number of bitflips and make all 0xff. This patch series did some optimization logic to prevent the unnecessary full page raw read and data copy from QCOM NAND controller to DMA.
4. Following are the testing done for these patches in QCOM IPQ8074 HK01 (4-bit and 8-bit ECC chip) and IPQ806x AP148 boards. a. Run all mtd test and check if it passes
b. Introduce custom bitflips in erased page and check if it
returns no error/EUCLEAN/EBADMSG depending upon number of bitflips and position.
c. Introduce failure condition for operational failure and
check if it detects the same.
[1]: https://patchwork.ozlabs.org/patch/328994/
[2]: https://patchwork.ozlabs.org/patch/509970/
Abhishek Sahu (14):
mtd: rawnand: helper function for setting up ECC parameters
mtd: rawnand: denali: use helper function for ecc setup
dt-bindings: qcom_nandc: make nand-ecc-strength optional
mtd: rawnand: qcom: use the ecc strength from device parameter
mtd: rawnand: qcom: wait for desc completion in all BAM channels
mtd: rawnand: qcom: erased page detection for uncorrectable errors
only
mtd: rawnand: qcom: fix null pointer access for erased page detection
mtd: rawnand: qcom: parse read errors for read oob also
mtd: rawnand: qcom: modify write_oob to remove read codeword part
mtd: rawnand: qcom: fix return value for raw page read
mtd: rawnand: qcom: minor code reorganization for bad block check
mtd: rawnand: qcom: check for operation errors in case of raw read
mtd: rawnand: qcom: helper function for raw read
mtd: rawnand: qcom: erased page bitflips detection
.../devicetree/bindings/mtd/qcom_nandc.txt | 4 +- drivers/mtd/nand/raw/denali.c | 30 +- drivers/mtd/nand/raw/nand_base.c | 42 ++ drivers/mtd/nand/raw/qcom_nandc.c | 622 ++++++++++++++------- include/linux/mtd/rawnand.h | 3 + 5 files changed, 461 insertions(+), 240 deletions(-)
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v2 1/14 mtd: rawnand: helper function for setting up ECC parameters
commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check, match, maximize ECC settings") provides generic helpers which drivers can use for setting up ECC parameters.
Since same board can have different ECC strength nand chips so following is the logic for setting up ECC strength and ECC step size, which can be used by most of the drivers.
1. If both ECC step size and ECC strength are already set (usually by DT) then just check whether this setting
is supported by NAND controller.
2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest to the chip's requirement. If available OOB size can't fit the chip requirement then select maximum ECC strength which can be fit with available OOB size with warning.
This patch introduces nand_ecc_param_setup function which calls the required helper functions for the above logic. The drivers can use this single function instead of calling the 3 helper functions individually.
---
* Changes from v1:
NEW PATCH
drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 3 +++ 2 files changed, 45 insertions(+)
drivers/mtd/nand/raw/nand_base.c +42 -0
@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip, }EXPORT_SYMBOL_GPL (nand_maximize_ecc); +/** +* nand_ecc_param_setup - Set the ECC strength and ECC step size +* @chip: nand chip info structure +* @caps: ECC engine caps info structure +* @oobavail: OOB size that the ECC engine can use +* +* Choose the ECC strength according to following logic +* +* 1. If both ECC step size and ECC strength are already set (usually by DT) +* then check if it is supported by this controller. +* 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. +* 3. Otherwise, try to match the ECC step size and ECC strength closest +* to the chip's requirement. If available OOB size can't fit the chip +* requirement then fallback to the maximum ECC step size and ECC strength +* and print the warning. +* +* On success, the chosen ECC settings are set. +*/ +int nand_ecc_param_setup (struct nand_chip *chip, +const struct nand_ecc_caps *caps,int oobavail) +{ +int ret; + +if (chip->ecc.size && chip->ecc.strength) +return nand_check_ecc_caps (chip, caps, oobavail); + +if (chip->ecc.options &NAND_ECC_MAXIMIZE ) +return nand_maximize_ecc (chip, caps, oobavail); + +if (!nand_match_ecc_req (chip, caps, oobavail)) +return 0 ; + + ret =nand_maximize_ecc (chip, caps, oobavail); +if (!ret) +pr_warn ("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n" , + chip->ecc_step_ds, chip->ecc_strength_ds, + chip->ecc.size, chip->ecc.strength); + +return ret; +} +EXPORT_SYMBOL_GPL (nand_ecc_param_setup); +/* * Check if the chip configuration meet the datasheet requirements.
include/linux/mtd/rawnand.h +3 -0
@@ -1627,6 +1627,9 @@ int nand_match_ecc_req(struct nand_chip *chip,int nand_maximize_ecc (struct nand_chip *chip,const struct nand_ecc_caps *caps,int oobavail); +int nand_ecc_param_setup (struct nand_chip *chip, +const struct nand_ecc_caps *caps,int oobavail); +/* Default write_oob implementation */ int nand_write_oob_std (struct mtd_info *mtd,struct nand_chip *chip,int page);
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
2018-05-03 21:20 GMT+09:00 Abhishek Sahu <[email protected]>:
Quoted 65 lines
commit
2c8f8afa7f92("mtd: nand: add generic helpers to check, match, maximize ECC settings") provides generic helpers which drivers can use for setting up ECC parameters.Since same board can have different ECC strength nand chips so following is the logic for setting up ECC strength and ECC step size, which can be used by most of the drivers.
1. If both ECC step size and ECC strength are already set (usually by DT) then just check whether this setting
is supported by NAND controller.
2. IfNAND_ECC_MAXIMIZEis set, then select maximum ECC strength supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest to the chip's requirement. If available OOB size can't fit the chip requirement then select maximum ECC strength which can be fit with available OOB size with warning.This patch introduces
nand_ecc_param_setupfunction which calls the required helper functions for the above logic. The drivers can use this single function instead of calling the 3 helper functions individually.CC Masahiro Yamada <[email protected]>Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW PATCH
drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 3 +++ 2 files changed, 45 insertions(+)drivers/mtd/nand/raw/nand_base.c +23
-0@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip, }EXPORT_SYMBOL_GPL (nand_maximize_ecc); +/** +* nand_ecc_param_setup - Set the ECC strength and ECC step size +* @chip: nand chip info structure +* @caps: ECC engine caps info structure +* @oobavail: OOB size that the ECC engine can use +* +* Choose the ECC strength according to following logic +* +* 1. If both ECC step size and ECC strength are already set (usually by DT) +* then check if it is supported by this controller. +* 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. +* 3. Otherwise, try to match the ECC step size and ECC strength closest +* to the chip's requirement. If available OOB size can't fit the chip +* requirement then fallback to the maximum ECC step size and ECC strength +* and print the warning. +* +* On success, the chosen ECC settings are set. +*/ +int nand_ecc_param_setup (struct nand_chip *chip, +const struct nand_ecc_caps *caps,int oobavail) +{ +int ret; +
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6284
Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +10
-0+if (chip->ecc.size && chip->ecc.strength) +return nand_check_ecc_caps (chip, caps, oobavail); + +if (chip->ecc.options &NAND_ECC_MAXIMIZE ) +return nand_maximize_ecc (chip, caps, oobavail); + +if (!nand_match_ecc_req (chip, caps, oobavail)) +return 0 ; + + ret =nand_maximize_ecc (chip, caps, oobavail);
Why two calls for nand_maximize_ecc()?
My code is simpler, and does not display
false-positive warning.
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6288
Quoted 4 lines
drivers/mtd/nand/raw/nand_base.c +4
-0+if (!ret) +pr_warn ("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n" , + chip->ecc_step_ds, chip->ecc_strength_ds, + chip->ecc.size, chip->ecc.strength);
This is annoying.
{ecc_step_ds, ecc_strength_ds} are not provided by Non-ONFi devices.
So,
ECC (step, strength) = (0, 0) not supported on this controller.
will be always displayed.
The strength will be checked by nand_ecc_strength_good() anyway.
Quoted 4 lines
drivers/mtd/nand/raw/nand_base.c +4
-0+ +return ret; +} +EXPORT_SYMBOL_GPL (nand_ecc_param_setup);
Signature · 2 lines
Best Regards Masahiro Yamada
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
Quoted 86 lines · Masahiro Yamada
2018-05-03 21:20 GMT+09:00 Abhishek Sahu <[email protected]>:
Quoted 71 lines
commit
2c8f8afa7f92("mtd: nand: add generic helpers to check, match, maximize ECC settings") provides generic helpers which drivers can use for setting up ECC parameters.Since same board can have different ECC strength nand chips so following is the logic for setting up ECC strength and ECC step size, which can be used by most of the drivers.
1. If both ECC step size and ECC strength are already set (usually by DT) then just check whether this setting
is supported by NAND controller.
2. IfNAND_ECC_MAXIMIZEis set, then select maximum ECC strength supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest to the chip's requirement. If available OOB size can't fit the chip requirement then select maximum ECC strength which can be fit with available OOB size with warning.This patch introduces
nand_ecc_param_setupfunction which calls the required helper functions for the above logic. The drivers can use this single function instead of calling the 3 helper functions individually.CC Masahiro Yamada <[email protected]>Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW PATCH
drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 3 +++ 2 files changed, 45 insertions(+)drivers/mtd/nand/raw/nand_base.c +12
-0@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip, }EXPORT_SYMBOL_GPL (nand_maximize_ecc); +/** +* nand_ecc_param_setup - Set the ECC strength and ECC step size +* @chip: nand chip info structure +* @caps: ECC engine caps info structure +* @oobavail: OOB size that the ECC engine can use +* +* Choose the ECC strength according to following logic +* +* 1. If both ECC step size and ECC strength are already set (usually by DT) +* then check if it is supported by this controller. +* 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. +* 3. Otherwise, try to match the ECC step size and ECC strength closest
+ * to the chip's requirement. If available OOB size can't fit the
chip
+ * requirement then fallback to the maximum ECC step size and ECC
strength
+ * and print the warning.
+ *
+ * On success, the chosen ECC settings are set.
+ */drivers/mtd/nand/raw/nand_base.c +2
-0+int nand_ecc_param_setup (struct nand_chip *chip, + const struct nand_ecc_caps *caps, intoobavail)
drivers/mtd/nand/raw/nand_base.c +3
-0+{ +int ret; +drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6284
Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +10
-0+if (chip->ecc.size && chip->ecc.strength) +return nand_check_ecc_caps (chip, caps, oobavail); + +if (chip->ecc.options &NAND_ECC_MAXIMIZE ) +return nand_maximize_ecc (chip, caps, oobavail); + +if (!nand_match_ecc_req (chip, caps, oobavail)) +return 0 ; + + ret =nand_maximize_ecc (chip, caps, oobavail);Why two calls for
nand_maximize_ecc()?
Quoted 3 lines
My code is simpler, and does not display
false-positive warning.
Thanks Masahiro.
Since, Now this is in moved to generic layer that's why I put this warning that this function is falling back to some other ECC settings which is not recommend by chip.
If this warning seems unnecessary then I can remove this and then directly your code changes can be put here instead of calling nand_maximize_ecc 2 times.
Quoted 12 lines
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6288
Quoted 6 lines
drivers/mtd/nand/raw/nand_base.c +2
-0+if (!ret) + pr_warn("ECC (step, strength) = (%d, %d) not supportedon this controller. Fallback to (%d, %d)\n",
drivers/mtd/nand/raw/nand_base.c +2
-0+ chip->ecc_step_ds, chip->ecc_strength_ds, + chip->ecc.size, chip->ecc.strength);This is annoying.
{
ecc_step_ds,ecc_strength_ds} are not provided by Non-ONFi devices.
Quoted 8 lines
So,
ECC (step, strength) = (0, 0) not supported on this controller.will be always displayed.
The strength will be checked by
nand_ecc_strength_good()anyway.
But for most of the non ONFI devices also, this is being calculated by ID.
You can get some background for this in
http://lists.infradead.org/pipermail/linux-mtd/2018-April/080193.html
Regards,
Abhishek
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
In reply to Masahiro YamadaQuoted 71 lines · On Mon, 7 May 2018 12:40:39 +0900 Masahiro Yamada…
2018-05-03 21:20 GMT+09:00 Abhishek Sahu <[email protected]>:
Quoted 70 lines
commit
2c8f8afa7f92("mtd: nand: add generic helpers to check, match, maximize ECC settings") provides generic helpers which drivers can use for setting up ECC parameters.Since same board can have different ECC strength nand chips so following is the logic for setting up ECC strength and ECC step size, which can be used by most of the drivers.
1. If both ECC step size and ECC strength are already set (usually by DT) then just check whether this setting
is supported by NAND controller.
2. IfNAND_ECC_MAXIMIZEis set, then select maximum ECC strength supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest to the chip's requirement. If available OOB size can't fit the chip requirement then select maximum ECC strength which can be fit with available OOB size with warning.This patch introduces
nand_ecc_param_setupfunction which calls the required helper functions for the above logic. The drivers can use this single function instead of calling the 3 helper functions individually.CC Masahiro Yamada <[email protected]>Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW PATCH
drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 3 +++ 2 files changed, 45 insertions(+)drivers/mtd/nand/raw/nand_base.c +28
-0@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip, }EXPORT_SYMBOL_GPL (nand_maximize_ecc); +/** +* nand_ecc_param_setup - Set the ECC strength and ECC step size +* @chip: nand chip info structure +* @caps: ECC engine caps info structure +* @oobavail: OOB size that the ECC engine can use +* +* Choose the ECC strength according to following logic +* +* 1. If both ECC step size and ECC strength are already set (usually by DT) +* then check if it is supported by this controller. +* 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. +* 3. Otherwise, try to match the ECC step size and ECC strength closest +* to the chip's requirement. If available OOB size can't fit the chip +* requirement then fallback to the maximum ECC step size and ECC strength +* and print the warning. +* +* On success, the chosen ECC settings are set. +*/ +int nand_ecc_param_setup (struct nand_chip *chip, +const struct nand_ecc_caps *caps,int oobavail) +{ +int ret; + +if (chip->ecc.size && chip->ecc.strength) +return nand_check_ecc_caps (chip, caps, oobavail); + +if (chip->ecc.options &NAND_ECC_MAXIMIZE ) +return nand_maximize_ecc (chip, caps, oobavail);
Quoted 8 lines
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6284
Quoted 5 lines
drivers/mtd/nand/raw/nand_base.c +5
-0+ +if (!nand_match_ecc_req (chip, caps, oobavail)) +return 0 ; + + ret =nand_maximize_ecc (chip, caps, oobavail);Why two calls for
nand_maximize_ecc()?
As long as the code does the same thing, I don't care that much.
Quoted 2 lines
My code is simpler,
and I don't see how your code is simpler. Mainly a matter of taste AFAICS.
Quoted 2 lines
and does not display
false-positive warning.
I agree on the false-positive warning though, this should be avoided.
Quoted 8 lines
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6288
Quoted 6 lines
drivers/mtd/nand/raw/nand_base.c +4
-0+if (!ret) +pr_warn ("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n" , + chip->ecc_step_ds, chip->ecc_strength_ds, + chip->ecc.size, chip->ecc.strength);
Quoted 6 lines
This is annoying.
{
ecc_step_ds,ecc_strength_ds} are not provided by Non-ONFi devices.So,
ECC (step, strength) = (0, 0) not supported on this controller.
Well, if you have a chip that requires ECC but exposes 0bits/0bytes then this should be fixed. 0,0 should only be valid when the chip does not require ECC at all (so, only really old chips). For all other chips, including non-ONFI ones, we should have a valid value here.
Quoted 5 lines
will be always displayed.
The strength will be checked by
nand_ecc_strength_good()anyway.
True. So, I agree that the pr_warn() is unneeded, but I still think we should fix all cases where ECC reqs are missing, so if you have such a setup, please add some code to nand_<vendor>.c to initialize ->ecc_xxx_ds properly.
Thanks,
Boris
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
2018-05-07 16:39 GMT+09:00 Boris Brezillon <[email protected]>:
Quoted 108 lines
Quoted 71 lines · On Mon, 7 May 2018 12:40:39 +0900 Masahiro Yamada…
2018-05-03 21:20 GMT+09:00 Abhishek Sahu <[email protected]>:
Quoted 70 lines
commit
2c8f8afa7f92("mtd: nand: add generic helpers to check, match, maximize ECC settings") provides generic helpers which drivers can use for setting up ECC parameters.Since same board can have different ECC strength nand chips so following is the logic for setting up ECC strength and ECC step size, which can be used by most of the drivers.
1. If both ECC step size and ECC strength are already set (usually by DT) then just check whether this setting
is supported by NAND controller.
2. IfNAND_ECC_MAXIMIZEis set, then select maximum ECC strength supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest to the chip's requirement. If available OOB size can't fit the chip requirement then select maximum ECC strength which can be fit with available OOB size with warning.This patch introduces
nand_ecc_param_setupfunction which calls the required helper functions for the above logic. The drivers can use this single function instead of calling the 3 helper functions individually.CC Masahiro Yamada <[email protected]>Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW PATCH
drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 3 +++ 2 files changed, 45 insertions(+)drivers/mtd/nand/raw/nand_base.c +28
-0@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip, }EXPORT_SYMBOL_GPL (nand_maximize_ecc); +/** +* nand_ecc_param_setup - Set the ECC strength and ECC step size +* @chip: nand chip info structure +* @caps: ECC engine caps info structure +* @oobavail: OOB size that the ECC engine can use +* +* Choose the ECC strength according to following logic +* +* 1. If both ECC step size and ECC strength are already set (usually by DT) +* then check if it is supported by this controller. +* 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. +* 3. Otherwise, try to match the ECC step size and ECC strength closest +* to the chip's requirement. If available OOB size can't fit the chip +* requirement then fallback to the maximum ECC step size and ECC strength +* and print the warning. +* +* On success, the chosen ECC settings are set. +*/ +int nand_ecc_param_setup (struct nand_chip *chip, +const struct nand_ecc_caps *caps,int oobavail) +{ +int ret; + +if (chip->ecc.size && chip->ecc.strength) +return nand_check_ecc_caps (chip, caps, oobavail); + +if (chip->ecc.options &NAND_ECC_MAXIMIZE ) +return nand_maximize_ecc (chip, caps, oobavail);Quoted 8 lines
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6284
Quoted 5 lines
drivers/mtd/nand/raw/nand_base.c +5
-0+ +if (!nand_match_ecc_req (chip, caps, oobavail)) +return 0 ; + + ret =nand_maximize_ecc (chip, caps, oobavail);Why two calls for
nand_maximize_ecc()?As long as the code does the same thing, I don't care that much.
Quoted 2 lines
My code is simpler,
and I don't see how your code is simpler. Mainly a matter of taste AFAICS.
Quoted 2 lines
and does not display
false-positive warning.I agree on the false-positive warning though, this should be avoided.
Quoted 12 lines
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6288
Quoted 6 lines
drivers/mtd/nand/raw/nand_base.c +4
-0+if (!ret) +pr_warn ("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n" , + chip->ecc_step_ds, chip->ecc_strength_ds, + chip->ecc.size, chip->ecc.strength);This is annoying.
{
ecc_step_ds,ecc_strength_ds} are not provided by Non-ONFi devices.
Quoted 7 lines
Quoted 2 lines
So,
ECC (step, strength) = (0, 0) not supported on this controller.Well, if you have a chip that requires ECC but exposes 0bits/0bytes then this should be fixed. 0,0 should only be valid when the chip does not require ECC at all (so, only really old chips). For all other chips, including non-ONFI ones, we should have a valid value here.
Sorry, it was my misunderstanding.
My NAND chip is Toshiba.
If I remember correctly, Toshiba chips were not set with ECC requirements in the past,
but as far as I tested the latest kernel now, the ECC requirement was set by drivers/mtd/nand/raw/nand_toshiba.c
Quoted 11 lines
Quoted 5 lines
will be always displayed.
The strength will be checked by
nand_ecc_strength_good()anyway.True. So, I agree that the
pr_warn()is unneeded, but I still think we should fix all cases where ECC reqs are missing, so if you have such a setup, please add some code to nand_<vendor>.c to initialize ->ecc_xxx_ds properly.
If we decide to not display pr_warn(),
I think the code like denali_ecc_setup() should work, and simple.
Signature · 2 lines
Best Regards Masahiro Yamada
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
Quoted 138 lines · Masahiro Yamada
2018-05-07 16:39 GMT+09:00 Boris Brezillon <[email protected]>:
Quoted 108 lines
Quoted 71 lines · On Mon, 7 May 2018 12:40:39 +0900 Masahiro Yamada…
2018-05-03 21:20 GMT+09:00 Abhishek Sahu <[email protected]>:
Quoted 70 lines
commit
2c8f8afa7f92("mtd: nand: add generic helpers to check, match, maximize ECC settings") provides generic helpers which drivers can use for setting up ECC parameters.Since same board can have different ECC strength nand chips so following is the logic for setting up ECC strength and ECC step size, which can be used by most of the drivers.
1. If both ECC step size and ECC strength are already set (usually by DT) then just check whether this setting
is supported by NAND controller.
2. IfNAND_ECC_MAXIMIZEis set, then select maximum ECC strength supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest to the chip's requirement. If available OOB size can't fit the chip requirement then select maximum ECC strength which can be fit with available OOB size with warning.This patch introduces
nand_ecc_param_setupfunction which calls the required helper functions for the above logic. The drivers can use this single function instead of calling the 3 helper functions individually.CC Masahiro Yamada <[email protected]>Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW PATCH
drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 3 +++ 2 files changed, 45 insertions(+)drivers/mtd/nand/raw/nand_base.c +28
-0@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip, }EXPORT_SYMBOL_GPL (nand_maximize_ecc); +/** +* nand_ecc_param_setup - Set the ECC strength and ECC step size +* @chip: nand chip info structure +* @caps: ECC engine caps info structure +* @oobavail: OOB size that the ECC engine can use +* +* Choose the ECC strength according to following logic +* +* 1. If both ECC step size and ECC strength are already set (usually by DT) +* then check if it is supported by this controller. +* 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. +* 3. Otherwise, try to match the ECC step size and ECC strength closest +* to the chip's requirement. If available OOB size can't fit the chip +* requirement then fallback to the maximum ECC step size and ECC strength +* and print the warning. +* +* On success, the chosen ECC settings are set. +*/ +int nand_ecc_param_setup (struct nand_chip *chip, +const struct nand_ecc_caps *caps,int oobavail) +{ +int ret; + +if (chip->ecc.size && chip->ecc.strength) +return nand_check_ecc_caps (chip, caps, oobavail); + +if (chip->ecc.options &NAND_ECC_MAXIMIZE ) +return nand_maximize_ecc (chip, caps, oobavail);Quoted 8 lines
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6284
Quoted 5 lines
drivers/mtd/nand/raw/nand_base.c +5
-0+ +if (!nand_match_ecc_req (chip, caps, oobavail)) +return 0 ; + + ret =nand_maximize_ecc (chip, caps, oobavail);Why two calls for
nand_maximize_ecc()?As long as the code does the same thing, I don't care that much.
Quoted 2 lines
My code is simpler,
and I don't see how your code is simpler. Mainly a matter of taste AFAICS.
Quoted 2 lines
and does not display
false-positive warning.I agree on the false-positive warning though, this should be avoided.
Quoted 12 lines
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6288
Quoted 6 lines
drivers/mtd/nand/raw/nand_base.c +4
-0+if (!ret) +pr_warn ("ECC (step, strength) = (%d, %d) not supported on this controller. Fallback to (%d, %d)\n" , + chip->ecc_step_ds, chip->ecc_strength_ds, + chip->ecc.size, chip->ecc.strength);This is annoying.
{
ecc_step_ds,ecc_strength_ds} are not provided by Non-ONFi devices.Quoted 7 lines
Quoted 2 lines
So,
ECC (step, strength) = (0, 0) not supported on this controller.Well, if you have a chip that requires ECC but exposes 0bits/0bytes then this should be fixed. 0,0 should only be valid when the chip does not require ECC at all (so, only really old chips). For all other chips, including non-ONFI ones, we should have a valid value here.
Sorry, it was my misunderstanding.
My NAND chip is Toshiba.
If I remember correctly, Toshiba chips were not set with ECC requirements in the past,
but as far as I tested the latest kernel now, the ECC requirement was set bydrivers/mtd/nand/raw/nand_toshiba.cQuoted 6 lines
Quoted 5 lines
will be always displayed.
The strength will be checked by
nand_ecc_strength_good()anyway.
Quoted 8 lines
Quoted 5 lines
True. So, I agree that the
pr_warn()is unneeded, but I still think we should fix all cases where ECC reqs are missing, so if you have such a setup, please add some code to nand_<vendor>.c to initialize ->ecc_xxx_ds properly.If we decide to not display
pr_warn(),
I think the code likedenali_ecc_setup()should work, and simple.
Thanks Boris and Masahiro.
I will remove this print and then we can use code likedenali_ecc_setup.
Regards,
Abhishek
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
In reply to Abhishek SahuQuoted 52 lines · On Thu, 3 May 2018 17:50:28 +0530 Abhishek Sahu <[email protected]> wrote:
commit
2c8f8afa7f92("mtd: nand: add generic helpers to check, match, maximize ECC settings") provides generic helpers which drivers can use for setting up ECC parameters.Since same board can have different ECC strength nand chips so following is the logic for setting up ECC strength and ECC step size, which can be used by most of the drivers.
1. If both ECC step size and ECC strength are already set (usually by DT) then just check whether this setting
is supported by NAND controller.
2. IfNAND_ECC_MAXIMIZEis set, then select maximum ECC strength supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest to the chip's requirement. If available OOB size can't fit the chip requirement then select maximum ECC strength which can be fit with available OOB size with warning.This patch introduces
nand_ecc_param_setupfunction which calls the required helper functions for the above logic. The drivers can use this single function instead of calling the 3 helper functions individually.CC Masahiro Yamada <[email protected]>Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW PATCH
drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 3 +++ 2 files changed, 45 insertions(+)drivers/mtd/nand/raw/nand_base.c +10
-0@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip, }EXPORT_SYMBOL_GPL (nand_maximize_ecc); +/** +* nand_ecc_param_setup - Set the ECC strength and ECC step size +* @chip: nand chip info structure +* @caps: ECC engine caps info structure +* @oobavail: OOB size that the ECC engine can use +* +* Choose the ECC strength according to following logic +* +* 1. If both ECC step size and ECC strength are already set (usually by DT) +* then check if it is supported by this controller.
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6271
Quoted 10 lines
drivers/mtd/nand/raw/nand_base.c +10
-0+* 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. +* 3. Otherwise, try to match the ECC step size and ECC strength closest +* to the chip's requirement. If available OOB size can't fit the chip +* requirement then fallback to the maximum ECC step size and ECC strength +* and print the warning. +* +* On success, the chosen ECC settings are set. +*/ +int nand_ecc_param_setup (struct nand_chip *chip, +const struct nand_ecc_caps *caps,int oobavail)
Can we rename this function "nand_ecc_choose_conf()".
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
Quoted 64 lines · Boris Brezillon
Quoted 61 lines · On Thu, 3 May 2018 17:50:28 +0530 Abhishek Sahu <[email protected]> wrote:
commit
2c8f8afa7f92("mtd: nand: add generic helpers to check, match, maximize ECC settings") provides generic helpers which drivers can use for setting up ECC parameters.Since same board can have different ECC strength nand chips so following is the logic for setting up ECC strength and ECC step size, which can be used by most of the drivers.
1. If both ECC step size and ECC strength are already set (usually by DT) then just check whether this setting
is supported by NAND controller.
2. IfNAND_ECC_MAXIMIZEis set, then select maximum ECC strength supported by NAND controller.
3. Otherwise, try to match the ECC step size and ECC strength closest to the chip's requirement. If available OOB size can't fit the chip requirement then select maximum ECC strength which can be fit with available OOB size with warning.This patch introduces
nand_ecc_param_setupfunction which calls the required helper functions for the above logic. The drivers can use this single function instead of calling the 3 helper functions individually.CC Masahiro Yamada <[email protected]>Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW PATCH
drivers/mtd/nand/raw/nand_base.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/rawnand.h | 3 +++ 2 files changed, 45 insertions(+)drivers/mtd/nand/raw/nand_base.c +12
-0@@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip, }EXPORT_SYMBOL_GPL (nand_maximize_ecc); +/** +* nand_ecc_param_setup - Set the ECC strength and ECC step size +* @chip: nand chip info structure +* @caps: ECC engine caps info structure +* @oobavail: OOB size that the ECC engine can use +* +* Choose the ECC strength according to following logic +* +* 1. If both ECC step size and ECC strength are already set (usually by DT) +* then check if it is supported by this controller. +* 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. +* 3. Otherwise, try to match the ECC step size and ECC strength closest
+ * to the chip's requirement. If available OOB size can't fit the
chip
+ * requirement then fallback to the maximum ECC step size and ECC
strength
Quoted 8 lines
drivers/mtd/nand/raw/nand_base.c int nand_maximize_ecc(struct nand_chip *chip, · line 6271
Quoted 6 lines
+ * and print the warning.
+ *
+ * On success, the chosen ECC settings are set.
+ */drivers/mtd/nand/raw/nand_base.c +2
-0+int nand_ecc_param_setup (struct nand_chip *chip, +const struct nand_ecc_caps *caps,int oobavail)Can we rename this function "
nand_ecc_choose_conf()".
Thanks Boris.
I will rename this function.
Regards,
Abhishek
PATCH v2 2/14 mtd: rawnand: denali: use helper function for ecc setup
In reply to Abhishek SahuNow, the NAND base layer has helper function for ecc parameters setup which does the same thing.
---
* Changes from v1:
NEW PATCH
drivers/mtd/nand/raw/denali.c | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-)
drivers/mtd/nand/raw/denali.c +2 -28
@@ -1120,33 +1120,6 @@ int denali_calc_ecc_bytes(int step_size, int strength) }EXPORT_SYMBOL (denali_calc_ecc_bytes); -static int denali_ecc_setup (struct mtd_info *mtd,struct nand_chip *chip, -struct denali_nand_info *denali) -{ -int oobavail = mtd->oobsize - denali->oob_skip_bytes; -int ret; - -/* -* If .size and .strength are already set (usually by DT), -* check if they are supported by this controller. -*/ -if (chip->ecc.size && chip->ecc.strength) -return nand_check_ecc_caps (chip, denali->ecc_caps, oobavail); - -/* -* We want .size and .strength closest to the chip's requirement -* unless NAND_ECC_MAXIMIZE is requested. -*/ -if (!(chip->ecc.options &NAND_ECC_MAXIMIZE )) { - ret =nand_match_ecc_req (chip, denali->ecc_caps, oobavail); -if (!ret) -return 0 ; - } - -/* Max ECC strength is the last thing we can do */ -return nand_maximize_ecc (chip, denali->ecc_caps, oobavail); -} -static int denali_ooblayout_ecc (struct mtd_info *mtd,int section,struct mtd_oob_region *oobregion) {
@@ -1317,7 +1290,8 @@ int denali_init(struct denali_nand_info *denali) chip->ecc.mode =NAND_ECC_HW_SYNDROME ; chip->options |=NAND_NO_SUBPAGE_WRITE ; - ret =denali_ecc_setup (mtd, chip, denali); + ret =nand_ecc_param_setup (chip, denali->ecc_caps, + mtd->oobsize - denali->oob_skip_bytes);if (ret) {dev_err (denali->dev,"Failed to setup ECC settings.\n" ); goto disable_irq;
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v2 02/14] mtd: rawnand: denali: use helper function for ecc setup
Hi Abhishek,
Quoted 2 lines · Abhishek Sahu
Now, the NAND base layer has helper function for ecc parameters setup which does the same thing.
Even if this message has a meaning in the series, I would prefer something more generic like:
"Use the NAND core helper function xxx() to tune the ECC parameters instead of the function locally defined."
With this change:
Thanks,
Miquèl
Re: [PATCH v2 02/14] mtd: rawnand: denali: use helper function for ecc setup
Quoted 5 lines · Miquel Raynal
Hi Abhishek,
On Thu, 3 May 2018 17:50:29 +0530, Abhishek Sahu
<[email protected]> wrote:
Quoted 9 lines
Quoted 2 lines
Now, the NAND base layer has helper function for ecc parameters setup which does the same thing.
Even if this message has a meaning in the series, I would prefer something more generic like:
"Use the NAND core helper function
xxx()to tune the ECC parameters instead of the function locally defined."
Sure Miquel. I will change the commit message.
Thanks,
Abhishek
Quoted 6 lines
With this change:
Acked-by Miquel Raynal <[email protected]>Thanks,
Miquèl
PATCH v2 3/14 dt-bindings: qcom_nandc: make nand-ecc-strength optional
In reply to Abhishek SahuNow, nand-ecc-strength is optional. If specified in DT, then controller will use this ECC strength otherwise ECC strength will be calculated according to chip requirement and available OOB size.
---
* Changes from v1:
NEW PATCH
Documentation/devicetree/bindings/mtd/qcom_nandc.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mtd/qcom_nandc.txt +3 -1
@@ -45,11 +45,13 @@ Required properties: number (e.g., 0, 1, 2, etc.) - #address-cells: see partition.txt - #size-cells: see partition.txt -- nand-ecc-strength: see nand.txt - nand-ecc-step-size: must be 512. see nand.txt for more details. Optional properties: - nand-bus-width: see nand.txt +- nand-ecc-strength: see nand.txt. If not specified, then ECC strength will + be used according to chip requirement and available + OOB size. Each nandcs device node may optionally contain a 'partitions' sub-node, which further contains sub-nodes describing the flash partition mapping. See
Signature · 7 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Re: [PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional
Quoted 18 lines · On Thu, 3 May 2018 17:50:30 +0530 Abhishek Sahu <[email protected]> wrote:
Now, nand-ecc-strength is optional. If specified in DT, then controller will use this ECC strength otherwise ECC strength will be calculated according to chip requirement and available OOB size.
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW PATCH
Documentation/devicetree/bindings/mtd/qcom_nandc.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/mtd/qcom_nandc.txt +0
-0
Documentation/devicetree/bindings/mtd/qcom_nandc.txt Required properties: · line 48
Quoted 5 lines
Documentation/devicetree/bindings/mtd/qcom_nandc.txt +0
-1@@ -45,11 +45,13 @@ Required properties: number (e.g., 0, 1, 2, etc.) - #address-cells: see partition.txt - #size-cells: see partition.txt -- nand-ecc-strength: see nand.txt - nand-ecc-step-size: must be 512. see nand.txt for more details.
As mentioned in my other review, no need to specify nand-ecc-step-size if you don't have a choice. You can remove the property and say that nand-ecc-strength encodes the ECC strength for 512 bytes chunks.
Quoted 9 lines
Documentation/devicetree/bindings/mtd/qcom_nandc.txt +3
-0Optional properties: - nand-bus-width: see nand.txt +- nand-ecc-strength: see nand.txt. If not specified, then ECC strength will + be used according to chip requirement and available + OOB size. Each nandcs device node may optionally contain a 'partitions' sub-node, which further contains sub-nodes describing the flash partition mapping. See
Miquel Raynal
maintainerRe: [PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional
In reply to Abhishek SahuHi Abhishek,
Quoted 3 lines · Abhishek Sahu
Now, nand-ecc-strength is optional. If specified in DT, then controller will use this ECC strength otherwise ECC strength will be calculated according to chip requirement and available OOB size.
Same comment as before: don't start the commit log with "now,".
Thanks,
Miquèl
Re: [PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional
Quoted 5 lines · Miquel Raynal
Hi Abhishek,
On Thu, 3 May 2018 17:50:30 +0530, Abhishek Sahu
<[email protected]> wrote:
Quoted 8 lines
Quoted 3 lines
Now, nand-ecc-strength is optional. If specified in DT, then controller will use this ECC strength otherwise ECC strength will be calculated according to chip requirement and available OOB size.
Same comment as before: don't start the commit log with "now,".
Thanks,
Miquèl
Thanks Miquel.
I will change the commit message.
Regards,
Abhishek
PATCH v2 4/14 mtd: rawnand: qcom: use the ecc strength from device parameter
In reply to Abhishek SahuCurrently the driver uses the ECC strength specified in DT. The QPIC/EBI2 NAND supports 4 or 8-bit ECC correction. The same kind of board can have different NAND parts so use the ECC strength from device parameters if it is not specified in DT.
---
* Changes from v1:
1. Removed the custom logic and used the helper fuction.
drivers/mtd/nand/raw/qcom_nandc.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +22 -9
@@ -2315,13 +2315,21 @@ static int qcom_nand_ooblayout_free(struct mtd_info *mtd, int section, .free = qcom_nand_ooblayout_free, }; +static int +qcom_nandc_calc_ecc_bytes (int step_size,int strength) +{ +return strength ==4 ?12 :16 ; +} +NAND_ECC_CAPS_SINGLE (qcom_nandc_ecc_caps, qcom_nandc_calc_ecc_bytes, +NANDC_STEP_SIZE ,4 ,8 ); +static int qcom_nand_host_setup (struct qcom_nand_host *host) {struct nand_chip *chip = &host->chip;struct mtd_info *mtd =nand_to_mtd (chip);struct nand_ecc_ctrl *ecc = &chip->ecc;struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip); -int cwperpage, bad_block_byte; +int cwperpage, bad_block_byte, ret;bool wide_bus;int ecc_mode =1 ;
@@ -2334,8 +2342,20 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)return -EINVAL ; } + cwperpage = mtd->writesize / ecc->size; + +/* +* Each CW has 4 available OOB bytes which will be protected with ECC +* so remaining bytes can be used for ECC. +*/ + ret =nand_ecc_param_setup (chip, &qcom_nandc_ecc_caps, + mtd->oobsize - (cwperpage <<2 )); +if (ret) { +dev_err (nandc->dev,"No valid ecc settings possible\n" ); +return ret; + } + wide_bus = chip->options &NAND_BUSWIDTH_16 ? true : false; -if (ecc->strength >=8 ) {/* 8 bit ECC defaults to BCH ECC on all platforms */ host->bch_enabled = true;
@@ -2403,7 +2423,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)mtd_set_ooblayout (mtd, &qcom_nand_ooblayout_ops); - cwperpage = mtd->writesize / ecc->size; nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage, cwperpage);
@@ -2419,12 +2438,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)* for 8 bit ECC */ host->cw_size = host->cw_data + ecc->bytes; - -if (ecc->bytes * (mtd->writesize / ecc->size) > mtd->oobsize) { -dev_err (nandc->dev,"ecc data doesn't fit in OOB area\n" ); -return -EINVAL ; - } - bad_block_byte = mtd->writesize - host->cw_size * (cwperpage -1 ) +1 ; host->cfg0 = (cwperpage -1 ) <<CW_PER_PAGE
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v2 04/14] mtd: rawnand: qcom: use the ecc strength from device parameter
Quoted 42 lines · On Thu, 3 May 2018 17:50:31 +0530 Abhishek Sahu <[email protected]> wrote:
Currently the driver uses the ECC strength specified in DT. The QPIC/EBI2 NAND supports 4 or 8-bit ECC correction. The same kind of board can have different NAND parts so use the ECC strength from device parameters if it is not specified in DT.
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:1. Removed the custom logic and used the helper fuction.
drivers/mtd/nand/raw/qcom_nandc.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)drivers/mtd/nand/raw/qcom_nandc.c +9
-1@@ -2315,13 +2315,21 @@ static int qcom_nand_ooblayout_free(struct mtd_info *mtd, int section, .free = qcom_nand_ooblayout_free, }; +static int +qcom_nandc_calc_ecc_bytes (int step_size,int strength) +{ +return strength ==4 ?12 :16 ; +} +NAND_ECC_CAPS_SINGLE (qcom_nandc_ecc_caps, qcom_nandc_calc_ecc_bytes, +NANDC_STEP_SIZE ,4 ,8 ); +static int qcom_nand_host_setup (struct qcom_nand_host *host) {struct nand_chip *chip = &host->chip;struct mtd_info *mtd =nand_to_mtd (chip);struct nand_ecc_ctrl *ecc = &chip->ecc;struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip); -int cwperpage, bad_block_byte; +int cwperpage, bad_block_byte, ret;bool wide_bus;int ecc_mode =1 ;
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nand_host_setup(struct qcom_nand_host *host) · line 2345
Quoted 4 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-0@@ -2334,8 +2342,20 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)return -EINVAL ; } + cwperpage = mtd->writesize / ecc->size;
Looks like you're still expecting nand-ecc-step-size to be defined in the DT, which does not really make sense since you only support one
size: NANDC_STEP_SIZE.
You should remove the
if (ecc->size != NANDC_STEP_SIZE) {
dev_err(nandc->dev, "invalid ecc size\n");
return -EINVAL;
}block, then do:
cwperpage = mtd->writesize / NANDC_STEP_SIZE;
and finally let the nand_ecc_param_setup() function choose the best config for you.
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nand_host_setup(struct qcom_nand_host *host) · line 2352
Quoted 7 lines
drivers/mtd/nand/raw/qcom_nandc.c +7
-0+ +/* +* Each CW has 4 available OOB bytes which will be protected with ECC +* so remaining bytes can be used for ECC. +*/ + ret =nand_ecc_param_setup (chip, &qcom_nandc_ecc_caps, + mtd->oobsize - (cwperpage <<2 ));
Please stop doing useless optimizations like. That brings nothing and obfuscates the code a bit more. You say in the comment that each codeword has 4 protected OOB bytes that can be used by the upper layer, so just do (cwperpage * 4) and let gcc optimize that for you.
Quoted 31 lines
drivers/mtd/nand/raw/qcom_nandc.c +5
-8+if (ret) { +dev_err (nandc->dev,"No valid ecc settings possible\n" ); +return ret; + } + wide_bus = chip->options &NAND_BUSWIDTH_16 ? true : false; -if (ecc->strength >=8 ) {/* 8 bit ECC defaults to BCH ECC on all platforms */ host->bch_enabled = true;@@ -2403,7 +2423,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)mtd_set_ooblayout (mtd, &qcom_nand_ooblayout_ops); - cwperpage = mtd->writesize / ecc->size; nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage, cwperpage);@@ -2419,12 +2438,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)* for 8 bit ECC */ host->cw_size = host->cw_data + ecc->bytes; - -if (ecc->bytes * (mtd->writesize / ecc->size) > mtd->oobsize) { -dev_err (nandc->dev,"ecc data doesn't fit in OOB area\n" ); -return -EINVAL ; - } - bad_block_byte = mtd->writesize - host->cw_size * (cwperpage -1 ) +1 ; host->cfg0 = (cwperpage -1 ) <<CW_PER_PAGE
Re: [PATCH v2 04/14] mtd: rawnand: qcom: use the ecc strength from device parameter
Quoted 64 lines · Boris Brezillon
Quoted 44 lines · On Thu, 3 May 2018 17:50:31 +0530 Abhishek Sahu <[email protected]> wrote:
Currently the driver uses the ECC strength specified in DT. The QPIC/EBI2 NAND supports 4 or 8-bit ECC correction. The same kind of board can have different NAND parts so use the ECC strength from device parameters if it is not specified in DT.
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:1. Removed the custom logic and used the helper fuction.
drivers/mtd/nand/raw/qcom_nandc.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)drivers/mtd/nand/raw/qcom_nandc.c +9
-1@@ -2315,13 +2315,21 @@ static int qcom_nand_ooblayout_free(struct mtd_info *mtd, int section, .free = qcom_nand_ooblayout_free, }; +static int +qcom_nandc_calc_ecc_bytes (int step_size,int strength) +{ +return strength ==4 ?12 :16 ; +} +NAND_ECC_CAPS_SINGLE (qcom_nandc_ecc_caps, qcom_nandc_calc_ecc_bytes, +NANDC_STEP_SIZE ,4 ,8 ); +static int qcom_nand_host_setup (struct qcom_nand_host *host) {struct nand_chip *chip = &host->chip;struct mtd_info *mtd =nand_to_mtd (chip);struct nand_ecc_ctrl *ecc = &chip->ecc;struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip); -int cwperpage, bad_block_byte; +int cwperpage, bad_block_byte, ret;bool wide_bus;int ecc_mode =1 ;drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nand_host_setup(struct · line 2346
Quoted 5 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-0@@ -2334,8 +2342,20 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)return -EINVAL ; } + cwperpage = mtd->writesize / ecc->size;Looks like you're still expecting nand-ecc-step-size to be defined in the DT, which does not really make sense since you only support one
size:NANDC_STEP_SIZE.You should remove the
if (ecc->size != NANDC_STEP_SIZE) { dev_err(nandc->dev, "invalid ecc size\n"); return -EINVAL; }
Quoted 7 lines
block, then do:
cwperpage = mtd->writesize / NANDC_STEP_SIZE;and finally let the
nand_ecc_param_setup()function choose the best config for you.
Correct Boris.
It only supports one step size so we can remove this DT property. I will make the changes.
Quoted 5 lines
Quoted 5 lines
drivers/mtd/nand/raw/qcom_nandc.c +3
-0+ +/* +* Each CW has 4 available OOB bytes which will be protected with ECC
drivers/mtd/nand/raw/qcom_nandc.c +1
-0+* so remaining bytes can be used for ECC.
Quoted 9 lines
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nand_host_setup(struct qcom_nand_host *host) · line 2352
Quoted 3 lines
drivers/mtd/nand/raw/qcom_nandc.c +3
-0+*/ + ret =nand_ecc_param_setup (chip, &qcom_nandc_ecc_caps, + mtd->oobsize - (cwperpage <<2 ));Please stop doing useless optimizations like. That brings nothing and obfuscates the code a bit more. You say in the comment that each codeword has 4 protected OOB bytes that can be used by the upper layer, so just do (cwperpage * 4) and let gcc optimize that for you.
Sure. I will change it.
Thanks,
Abhishek
Quoted 34 lines
Quoted 34 lines
drivers/mtd/nand/raw/qcom_nandc.c +5
-8+if (ret) { +dev_err (nandc->dev,"No valid ecc settings possible\n" ); +return ret; + } + wide_bus = chip->options &NAND_BUSWIDTH_16 ? true : false; -if (ecc->strength >=8 ) {/* 8 bit ECC defaults to BCH ECC on all platforms */ host->bch_enabled = true;@@ -2403,7 +2423,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host) mtd_set_ooblayout(mtd, &qcom_nand_ooblayout_ops); - cwperpage = mtd->writesize / ecc->size; nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage, cwperpage);@@ -2419,12 +2438,6 @@ static int qcom_nand_host_setup(structqcom_nand_host *host) * for 8 bit ECC */ host->cw_size = host->cw_data + ecc->bytes; - -if (ecc->bytes * (mtd->writesize / ecc->size) > mtd->oobsize) { -dev_err (nandc->dev,"ecc data doesn't fit in OOB area\n" ); -return -EINVAL ; - } - bad_block_byte = mtd->writesize - host->cw_size * (cwperpage -1 ) +1 ;
host->cfg0= (cwperpage - 1) <<CW_PER_PAGE
PATCH v2 5/14 mtd: rawnand: qcom: wait for desc completion in all BAM channels
In reply to Abhishek SahuThe BAM has 3 channels - tx, rx and command. command channel is used for register read/writes, tx channel for data writes and rx channel for data reads. Currently, the driver assumes the transfer completion once it gets all the command descriptor completed. Sometimes, there is race condition in data channel (tx/rx) and command channel completion and in these cases, the data in buffer is not valid during the small window between command descriptor completion and data descriptor completion.
Now, the changes have been made to assign the callback for channel's final descriptor. The DMA will generate the callback when all the descriptors have completed in that channel. The NAND transfer will be completed only when all required DMA channels have generated the completion callback.
---
* Changes from v1:
NONE
1. Removed the custom logic and used the helper fuction.
drivers/mtd/nand/raw/qcom_nandc.c | 55 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-)
drivers/mtd/nand/raw/qcom_nandc.c +54 -1
@@ -213,6 +213,8 @@#define QPIC_PER_CW_CMD_SGL 32#define QPIC_PER_CW_DATA_SGL 8 +#define QPIC_NAND_COMPLETION_TIMEOUT msecs_to_jiffies(2000) +/* * Flags used in DMA descriptor preparation helper functions * (i.e. read_reg_dma/write_reg_dma/read_data_dma/write_data_dma)
@@ -245,6 +247,11 @@* @tx_sgl_start - start index in data sgl for tx. * @rx_sgl_pos - current index in data sgl for rx. * @rx_sgl_start - start index in data sgl for rx. +* @first_chan_done - if current transfer already has got first channel +* DMA desc completion. +* @txn_done - completion for nand transfer. +* @last_data_desc - last DMA desc in data channel (tx/rx). +* @last_cmd_desc - last DMA desc in command channel. */ struct bam_transaction {struct bam_cmd_element *bam_ce;
@@ -258,6 +265,10 @@ struct bam_transaction {u32 tx_sgl_start;u32 rx_sgl_pos;u32 rx_sgl_start; +bool first_chan_done; +struct completion txn_done; +struct dma_async_tx_descriptor *last_data_desc; +struct dma_async_tx_descriptor *last_cmd_desc; };/*
@@ -504,6 +515,8 @@ static void free_bam_transaction(struct qcom_nand_controller *nandc) bam_txn->data_sgl = bam_txn_buf; +init_completion (&bam_txn->txn_done); +return bam_txn; }
@@ -523,11 +536,36 @@ static void clear_bam_transaction(struct qcom_nand_controller *nandc) bam_txn->tx_sgl_start =0 ; bam_txn->rx_sgl_pos =0 ; bam_txn->rx_sgl_start =0 ; + bam_txn->last_data_desc =NULL ; + bam_txn->first_chan_done = false;sg_init_table (bam_txn->cmd_sgl, nandc->max_cwperpage *QPIC_PER_CW_CMD_SGL );sg_init_table (bam_txn->data_sgl, nandc->max_cwperpage *QPIC_PER_CW_DATA_SGL ); + +reinit_completion (&bam_txn->txn_done); +} + +/* Callback for DMA descriptor completion */ +static void qpic_bam_dma_done (void *data) +{ +struct bam_transaction *bam_txn = data; + +/* +* In case of data transfer with NAND, 2 callbacks will be generated. +* One for command channel and another one for data channel. +* If current transaction has data descriptors then check if its +* already got one DMA channel completion callback. In this case +* make the NAND transfer complete otherwise mark first_chan_done true +* and wait for next channel DMA completion callback. +*/ +if (bam_txn->last_data_desc && !bam_txn->first_chan_done) { + bam_txn->first_chan_done = true; +return ; + } + +complete (&bam_txn->txn_done); }static inline struct qcom_nand_host *to_qcom_nand_host (struct nand_chip *chip)
@@ -756,6 +794,12 @@ static int prepare_bam_async_desc(struct qcom_nand_controller *nandc, desc->dma_desc = dma_desc; +/* update last data/command descriptor */ +if (chan == nandc->cmd_chan) + bam_txn->last_cmd_desc = dma_desc; +else + bam_txn->last_data_desc = dma_desc; +list_add_tail (&desc->node, &nandc->desc_list);return 0 ;
@@ -1273,10 +1317,19 @@ static int submit_descs(struct qcom_nand_controller *nandc) cookie =dmaengine_submit (desc->dma_desc);if (nandc->props->is_bam) { + bam_txn->last_cmd_desc->callback = qpic_bam_dma_done; + bam_txn->last_cmd_desc->callback_param = bam_txn; +if (bam_txn->last_data_desc) { + bam_txn->last_data_desc->callback = qpic_bam_dma_done; + bam_txn->last_data_desc->callback_param = bam_txn; + } +dma_async_issue_pending (nandc->tx_chan);dma_async_issue_pending (nandc->rx_chan); +dma_async_issue_pending (nandc->cmd_chan); -if (dma_sync_wait (nandc->cmd_chan, cookie) !=DMA_COMPLETE ) +if (!wait_for_completion_timeout (&bam_txn->txn_done, +QPIC_NAND_COMPLETION_TIMEOUT ))return -ETIMEDOUT ; }else {if (dma_sync_wait (nandc->chan, cookie) !=DMA_COMPLETE )
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Miquel Raynal
maintainerRe: [PATCH v2 05/14] mtd: rawnand: qcom: wait for desc completion in all BAM channels
Hi Abhishek,
Quoted 5 lines · Abhishek Sahu
The BAM has 3 channels - tx, rx and command. command channel is used for register read/writes, tx channel for data writes and rx channel for data reads. Currently, the driver assumes the transfer completion once it gets all the command descriptor completed. Sometimes, there is race condition in data channel
"Sometimes, there is a race condition between the data channel (rx/tx) and the command channel completion. In these cases, ..."
Quoted 2 lines
(tx/rx) and command channel completion and in these cases, the data in buffer is not valid during the small window between
^ present in the buffer ?
Quoted 3 lines
command descriptor completion and data descriptor completion.
Now, the changes have been made to assign the callback for
It is preferable to use a descriptive tense when you expose what the patch does. Something like "Change <this> to assign ..."
Quoted 5 lines
channel's final descriptor. The DMA will generate the callback when all the descriptors have completed in that channel. The NAND transfer will be completed only when all required DMA channels have generated the completion callback.
It looks like this is a fix that is a good candidate for stable trees, you might want to add the relevant tags.
Quoted 15 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NONE
1. Removed the custom logic and used the helper fuction.
drivers/mtd/nand/raw/qcom_nandc.c | 55 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-)drivers/mtd/nand/raw/qcom_nandc.c +0
-0
drivers/mtd/nand/raw/qcom_nandc.c line 216
Quoted 4 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-0@@ -213,6 +213,8 @@#define QPIC_PER_CW_CMD_SGL 32#define QPIC_PER_CW_DATA_SGL 8 +#define QPIC_NAND_COMPLETION_TIMEOUT msecs_to_jiffies(2000)
That's huge, but why not, it's a timeout anyway.
drivers/mtd/nand/raw/qcom_nandc.c line 252
Quoted 11 lines
drivers/mtd/nand/raw/qcom_nandc.c +4
-0+/* * Flags used in DMA descriptor preparation helper functions * (i.e. read_reg_dma/write_reg_dma/read_data_dma/write_data_dma) @@ -245,6 +247,11 @@* @tx_sgl_start - start index in data sgl for tx. * @rx_sgl_pos - current index in data sgl for rx. * @rx_sgl_start - start index in data sgl for rx. +* @first_chan_done - if current transfer already has got first channel +* DMA desc completion. +* @txn_done - completion for nand transfer.
s/nand/NAND/
Quoted 26 lines
drivers/mtd/nand/raw/qcom_nandc.c +8
-0+* @last_data_desc - last DMA desc in data channel (tx/rx). +* @last_cmd_desc - last DMA desc in command channel. */ struct bam_transaction {struct bam_cmd_element *bam_ce;@@ -258,6 +265,10 @@ struct bam_transaction {u32 tx_sgl_start;u32 rx_sgl_pos;u32 rx_sgl_start; +bool first_chan_done; +struct completion txn_done; +struct dma_async_tx_descriptor *last_data_desc; +struct dma_async_tx_descriptor *last_cmd_desc; };/* @@ -504,6 +515,8 @@ static void free_bam_transaction(struct qcom_nand_controller *nandc) bam_txn->data_sgl = bam_txn_buf; +init_completion (&bam_txn->txn_done); +return bam_txn; }
drivers/mtd/nand/raw/qcom_nandc.c static void clear_bam_transaction(struct qcom_nand_controller *nandc) · line 540
Quoted 5 lines
drivers/mtd/nand/raw/qcom_nandc.c +2
-0@@ -523,11 +536,36 @@ static void clear_bam_transaction(struct qcom_nand_controller *nandc) bam_txn->tx_sgl_start =0 ; bam_txn->rx_sgl_pos =0 ; bam_txn->rx_sgl_start =0 ; + bam_txn->last_data_desc =NULL ; + bam_txn->first_chan_done = false;
Are you sure you don't want to reinit last_cmd_desc here?
Quoted 16 lines
drivers/mtd/nand/raw/qcom_nandc.c +11
-0sg_init_table (bam_txn->cmd_sgl, nandc->max_cwperpage *QPIC_PER_CW_CMD_SGL );sg_init_table (bam_txn->data_sgl, nandc->max_cwperpage *QPIC_PER_CW_DATA_SGL ); + +reinit_completion (&bam_txn->txn_done); +} + +/* Callback for DMA descriptor completion */ +static void qpic_bam_dma_done (void *data) +{ +struct bam_transaction *bam_txn = data; + +/* +* In case of data transfer with NAND, 2 callbacks will be generated.
drivers/mtd/nand/raw/qcom_nandc.c static void clear_bam_transaction(struct qcom_nand_controller *nandc) · line 566
Quoted 10 lines
drivers/mtd/nand/raw/qcom_nandc.c +10
-0+* One for command channel and another one for data channel. +* If current transaction has data descriptors then check if its +* already got one DMA channel completion callback. In this case +* make the NAND transfer complete otherwise mark first_chan_done true +* and wait for next channel DMA completion callback. +*/ +if (bam_txn->last_data_desc && !bam_txn->first_chan_done) { + bam_txn->first_chan_done = true; +return ; + }
There is a lot of new variables just to wait for two bam_dma_done(). Why not just creating a boolean like "wait_second completion", initialize it in prepare_bam_async_desc to true when needed and complete txn_done when it's false, that's all:
if (bam_txn->wait_second_completion) {
bam_txn->wait_second_completion = false;
return;
}Quoted 6 lines
drivers/mtd/nand/raw/qcom_nandc.c +2
-0+ +complete (&bam_txn->txn_done); }static inline struct qcom_nand_host *to_qcom_nand_host (struct nand_chip *chip)
drivers/mtd/nand/raw/qcom_nandc.c static int prepare_bam_async_desc(struct qcom_nand_controller *nandc, · line 802
Quoted 9 lines
drivers/mtd/nand/raw/qcom_nandc.c +6
-0@@ -756,6 +794,12 @@ static int prepare_bam_async_desc(struct qcom_nand_controller *nandc, desc->dma_desc = dma_desc; +/* update last data/command descriptor */ +if (chan == nandc->cmd_chan) + bam_txn->last_cmd_desc = dma_desc; +else + bam_txn->last_data_desc = dma_desc; +
Is there a reason for the "last_" prefix? why not current_*_desc or just *_desc? (this is a real question :) ). Correct me if I'm wrong but you have a scatter-gather list of DMA transfers that are mapped to form one DMA descriptor, so there is no "last" descriptor, right?
Otherwise, as I told you above, why not just a:
if (chan == nandc->data_chan)
bam_txn->wait_second_completion = true;drivers/mtd/nand/raw/qcom_nandc.c static int submit_descs(struct qcom_nand_controller *nandc) · line 1325
Quoted 13 lines
drivers/mtd/nand/raw/qcom_nandc.c +6
-0list_add_tail (&desc->node, &nandc->desc_list);return 0 ;@@ -1273,10 +1317,19 @@ static int submit_descs(struct qcom_nand_controller *nandc) cookie =dmaengine_submit (desc->dma_desc);if (nandc->props->is_bam) { + bam_txn->last_cmd_desc->callback = qpic_bam_dma_done; + bam_txn->last_cmd_desc->callback_param = bam_txn; +if (bam_txn->last_data_desc) { + bam_txn->last_data_desc->callback = qpic_bam_dma_done; + bam_txn->last_data_desc->callback_param = bam_txn; + }
Why don't you do this directly in prepare_bam_async_desc?
With:
dma_desc->callback = ...
dma_desc->callback_param = ...Quoted 11 lines
drivers/mtd/nand/raw/qcom_nandc.c +4
-1+dma_async_issue_pending (nandc->tx_chan);dma_async_issue_pending (nandc->rx_chan); +dma_async_issue_pending (nandc->cmd_chan); -if (dma_sync_wait (nandc->cmd_chan, cookie) !=DMA_COMPLETE ) +if (!wait_for_completion_timeout (&bam_txn->txn_done, +QPIC_NAND_COMPLETION_TIMEOUT ))return -ETIMEDOUT ; }else {if (dma_sync_wait (nandc->chan, cookie) !=DMA_COMPLETE )
Signature · 3 lines
Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v2 05/14] mtd: rawnand: qcom: wait for desc completion in all BAM channels
Quoted 17 lines · Miquel Raynal
Hi Abhishek,
Quoted 5 lines · Abhishek Sahu
The BAM has 3 channels - tx, rx and command. command channel is used for register read/writes, tx channel for data writes and rx channel for data reads. Currently, the driver assumes the transfer completion once it gets all the command descriptor completed. Sometimes, there is race condition in data channel
"Sometimes, there is a race condition between the data channel (rx/tx) and the command channel completion. In these cases, ..."
Quoted 2 lines
(tx/rx) and command channel completion and in these cases, the data in buffer is not valid during the small window between
Quoted 9 lines
^ present in the buffer ?
Quoted 3 lines
command descriptor completion and data descriptor completion.
Now, the changes have been made to assign the callback for
It is preferable to use a descriptive tense when you expose what the patch does. Something like "Change <this> to assign ..."
Thanks Miquel for your review.
I will change the sentence.
Quoted 8 lines
Quoted 5 lines
channel's final descriptor. The DMA will generate the callback when all the descriptors have completed in that channel. The NAND transfer will be completed only when all required DMA channels have generated the completion callback.
It looks like this is a fix that is a good candidate for stable trees, you might want to add the relevant tags.
Sure. I will add the relevant tags.
Quoted 20 lines
Quoted 20 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NONE
1. Removed the custom logic and used the helper fuction.
drivers/mtd/nand/raw/qcom_nandc.c | 55 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-)drivers/mtd/nand/raw/qcom_nandc.c +0
-0@@ -213,6 +213,8 @@#define QPIC_PER_CW_CMD_SGL 32#define QPIC_PER_CW_DATA_SGL 8
Quoted 4 lines
drivers/mtd/nand/raw/qcom_nandc.c line 216
Quoted 1 line
drivers/mtd/nand/raw/qcom_nandc.c +1
-0+#define QPIC_NAND_COMPLETION_TIMEOUT msecs_to_jiffies(2000)That's huge, but why not, it's a timeout anyway.
Correct. This timeout will never happen in normal case. It will hit if something bad happened in the board.
Quoted 41 lines
Quoted 9 lines
drivers/mtd/nand/raw/qcom_nandc.c +2
-0+/* * Flags used in DMA descriptor preparation helper functions * (i.e. read_reg_dma/write_reg_dma/read_data_dma/write_data_dma) @@ -245,6 +247,11 @@* @tx_sgl_start - start index in data sgl for tx. * @rx_sgl_pos - current index in data sgl for rx. * @rx_sgl_start - start index in data sgl for rx. +* @first_chan_done - if current transfer already has got first Quoted 3 lines
channel
+ * DMA desc completion.
+ * @txn_done - completion for nand transfer.s/nand/NAND/
Quoted 26 lines
+ * @last_data_desc - last DMA desc in data channel (tx/rx).
+ * @last_cmd_desc - last DMA desc in command channel. */
structbam_transaction{drivers/mtd/nand/raw/qcom_nandc.c +6
-0struct bam_cmd_element *bam_ce;@@ -258,6 +265,10 @@ struct bam_transaction {u32 tx_sgl_start;u32 rx_sgl_pos;u32 rx_sgl_start; +bool first_chan_done; +struct completion txn_done; +struct dma_async_tx_descriptor *last_data_desc; +struct dma_async_tx_descriptor *last_cmd_desc; };/* @@ -504,6 +515,8 @@ static void free_bam_transaction(struct qcom_nand_controller *nandc) bam_txn->data_sgl = bam_txn_buf; +init_completion (&bam_txn->txn_done); +return bam_txn; }
Quoted 9 lines
drivers/mtd/nand/raw/qcom_nandc.c static void clear_bam_transaction(struct · line 541
Quoted 7 lines
drivers/mtd/nand/raw/qcom_nandc.c +2
-0@@ -523,11 +536,36 @@ static void clear_bam_transaction(struct qcom_nand_controller *nandc) bam_txn->tx_sgl_start = 0; bam_txn->rx_sgl_pos =0 ; bam_txn->rx_sgl_start =0 ; + bam_txn->last_data_desc =NULL ; + bam_txn->first_chan_done = false;Are you sure you don't want to reinit
last_cmd_deschere?
Each NAND data transfer will definitely have at least one command desc so that reinit is redundant.
But some of the NAND transfers can have only command descriptors (i.e. no data desc) so, we need to reinit last_data_desc.
Quoted 51 lines
Quoted 23 lines
drivers/mtd/nand/raw/qcom_nandc.c +11
-0sg_init_table (bam_txn->cmd_sgl, nandc->max_cwperpage *QPIC_PER_CW_CMD_SGL );sg_init_table (bam_txn->data_sgl, nandc->max_cwperpage *QPIC_PER_CW_DATA_SGL ); + +reinit_completion (&bam_txn->txn_done); +} + +/* Callback for DMA descriptor completion */ +static void qpic_bam_dma_done (void *data) +{ +struct bam_transaction *bam_txn = data; + +/* +* In case of data transfer with NAND, 2 callbacks will be generated.
drivers/mtd/nand/raw/qcom_nandc.c +4
-0+* One for command channel and another one for data channel. +* If current transaction has data descriptors then check if its +* already got one DMA channel completion callback. In this case +* make the NAND transfer complete otherwise mark first_chan_done true
drivers/mtd/nand/raw/qcom_nandc.c static void clear_bam_transaction(struct qcom_nand_controller *nandc) · line 566
Quoted 6 lines
drivers/mtd/nand/raw/qcom_nandc.c +6
-0+* and wait for next channel DMA completion callback. +*/ +if (bam_txn->last_data_desc && !bam_txn->first_chan_done) { + bam_txn->first_chan_done = true; +return ; + }There is a lot of new variables just to wait for two
bam_dma_done(). Why not just creating a boolean like "wait_secondcompletion", initialize it inprepare_bam_async_descto true when needed and completetxn_donewhen it's false, that's all:if (bam_txn->wait_second_completion) { bam_txn->wait_second_completion = false; return; }Quoted 11 lines
drivers/mtd/nand/raw/qcom_nandc.c +2
-0+ +complete (&bam_txn->txn_done); } static inline struct qcom_nand_host *to_qcom_nand_host(struct
nand_chip*chip)drivers/mtd/nand/raw/qcom_nandc.c +0
-0@@ -756,6 +794,12 @@ static int prepare_bam_async_desc(struct qcom_nand_controller *nandc, desc->dma_desc = dma_desc;
Quoted 12 lines
drivers/mtd/nand/raw/qcom_nandc.c static int prepare_bam_async_desc(struct qcom_nand_controller *nandc, · line 802
Quoted 6 lines
drivers/mtd/nand/raw/qcom_nandc.c +6
-0+/* update last data/command descriptor */ +if (chan == nandc->cmd_chan) + bam_txn->last_cmd_desc = dma_desc; +else + bam_txn->last_data_desc = dma_desc; +Is there a reason for the "
last_" prefix? why not current_*_desc or just*_desc? (this is a real question :) ). Correct me if I'm wrong but you have a scatter-gather list of DMA transfers that are mapped to form one DMA descriptor, so there is no "last" descriptor, right?
We have 3 DMA channels (tx/rx and command) and each channel has
multiple
DMA descriptors. The callback needs to be set for last descriptor only for that channel.
Quoted 5 lines
Otherwise, as I told you above, why not just a:
if (chan == nandc->data_chan) bam_txn->wait_second_completion = true;
This is nice idea.
I will change the implementation accordingly.
Quoted 17 lines
Quoted 4 lines
drivers/mtd/nand/raw/qcom_nandc.c +0
-0list_add_tail (&desc->node, &nandc->desc_list);drivers/mtd/nand/raw/qcom_nandc.c +0
-0return 0 ;drivers/mtd/nand/raw/qcom_nandc.c static int submit_descs(struct · line 1326
Quoted 10 lines
drivers/mtd/nand/raw/qcom_nandc.c +6
-0@@ -1273,10 +1317,19 @@ static int submit_descs(struct qcom_nand_controller *nandc) cookie =dmaengine_submit (desc->dma_desc);if (nandc->props->is_bam) { + bam_txn->last_cmd_desc->callback = qpic_bam_dma_done; + bam_txn->last_cmd_desc->callback_param = bam_txn; +if (bam_txn->last_data_desc) { + bam_txn->last_data_desc->callback = qpic_bam_dma_done; + bam_txn->last_data_desc->callback_param = bam_txn; + }Why don't you do this directly in
prepare_bam_async_desc?
Quoted 5 lines
With:
dma_desc->callback = ... dma_desc->callback_param = ...
prepare_bam_async_desc can be called multiple times since each channel can have list of DMA descriptors. We want to set callback only for last DMA descriptor in that channel.
CMD desc1 -> Data desc1 -> Data desc2-> CMD desc2 -> CMD desc3
In the above case, the callback should be set for Data desc2 and CMD desc3.
Thanks,
Abhishek
Quoted 11 lines
Quoted 11 lines
drivers/mtd/nand/raw/qcom_nandc.c +4
-1+dma_async_issue_pending (nandc->tx_chan);dma_async_issue_pending (nandc->rx_chan); +dma_async_issue_pending (nandc->cmd_chan); -if (dma_sync_wait (nandc->cmd_chan, cookie) !=DMA_COMPLETE ) +if (!wait_for_completion_timeout (&bam_txn->txn_done, +QPIC_NAND_COMPLETION_TIMEOUT ))return -ETIMEDOUT ; }else {if (
dma_sync_wait(nandc->chan, cookie)!=DMA_COMPLETE)
PATCH v2 6/14 mtd: rawnand: qcom: erased page detection for uncorrectable errors only
In reply to Abhishek SahuFollowing is the flow in the HW if controller tries to read erased page
1. First ECC uncorrectable error will be generated from ECC engine since ECC engine first calculates the ECC with all 0xff and match the calculated ECC with ECC code in OOB (which is again all 0xff).
2. After getting ECC error, erased CW detection logic will be applied which is different for BCH and RS ECC
a. For BCH, HW checks if all the bytes in page are 0xff and then
it updates the status in separate register NAND_ERASED_CW_DETECT_STATUS.
b. For RS ECC, the HW reports the same error when reading an
erased CW, but it notifies that it is an erased CW by placing special characters at certain offsets in the buffer.
So the erased CW detect status should be checked only if ECC engine generated the uncorrectable error.
Currently for all other operational errors also (like TIMEOUT, MPU errors, etc.), the erased CW detect logic is being applied so fix this and return EIO for other operational errors.
---
* Changes from v1:
1. Added more detail in commit message
2. Added comment before each if/else
3. Removed redundant check for BS_UNCORRECTABLE_BIT
drivers/mtd/nand/raw/qcom_nandc.c | 65 ++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 22 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +43 -22
@@ -1578,6 +1578,7 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,struct nand_ecc_ctrl *ecc = &chip->ecc;unsigned int max_bitflips =0 ;struct read_stats *buf; +bool flash_op_err = false;int i; buf = (struct read_stats *)nandc->reg_read_buf;
@@ -1599,8 +1600,18 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf, buffer =le32_to_cpu (buf->buffer); erased_cw =le32_to_cpu (buf->erased_cw); -if (flash & (FS_OP_ERR |FS_MPU_ERR )) { +/* +* Check ECC failure for each codeword. ECC failure can +* happen in either of the following conditions +* 1. If number of bitflips are greater than ECC engine +* capability. +* 2. If this codeword contains all 0xff for which erased +* codeword detection check will be done. +*/ +if ((flash &FS_OP_ERR ) && (buffer &BS_UNCORRECTABLE_BIT )) {bool erased; +int ret, ecclen, extraooblen; +void *eccbuf;/* ignore erased codeword errors */ if (host->bch_enabled) {
@@ -1618,29 +1629,36 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,continue ; } -if (buffer &BS_UNCORRECTABLE_BIT ) { -int ret, ecclen, extraooblen; -void *eccbuf; + eccbuf = oob_buf ? oob_buf + oob_len :NULL ; + ecclen = oob_buf ? host->ecc_bytes_hw :0 ; + extraooblen = oob_buf ? oob_len :0 ; - eccbuf = oob_buf ? oob_buf + oob_len :NULL ; - ecclen = oob_buf ? host->ecc_bytes_hw :0 ; - extraooblen = oob_buf ? oob_len :0 ; - -/* -* make sure it isn't an erased page reported -* as not-erased by HW because of a few bitflips -*/ - ret =nand_check_erased_ecc_chunk (data_buf, - data_len, eccbuf, ecclen, oob_buf, - extraooblen, ecc->strength); -if (ret <0 ) { - mtd->ecc_stats.failed++; - }else { - mtd->ecc_stats.corrected += ret; - max_bitflips = - max_t(unsigned int, max_bitflips, ret); - } +/* +* make sure it isn't an erased page reported +* as not-erased by HW because of a few bitflips +*/ + ret =nand_check_erased_ecc_chunk (data_buf, + data_len, eccbuf, ecclen, oob_buf, + extraooblen, ecc->strength); +if (ret <0 ) { + mtd->ecc_stats.failed++; + }else { + mtd->ecc_stats.corrected += ret; + max_bitflips = + max_t(unsigned int, max_bitflips, ret); } +/* +* Check if MPU or any other operational error (timeout, +* device failure, etc.) happened for this codeword and +* make flash_op_err true. If flash_op_err is set, then +* EIO will be returned for page read. +*/ + }else if (flash & (FS_OP_ERR |FS_MPU_ERR )) { + flash_op_err = true; +/* +* No ECC or operational errors happened. Check the number of +* bits corrected and update the ecc_stats.corrected. +*/ }else {unsigned int stat;
@@ -1654,6 +1672,9 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf, oob_buf += oob_len + ecc->bytes; } +if (flash_op_err) +return -EIO ; +return max_bitflips; }
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v2 06/14] mtd: rawnand: qcom: erased page detection for uncorrectable errors only
Hi Abhishek,
Quoted 2 lines · Abhishek Sahu
Following is the flow in the HW if controller tries to read erased page
Nit: ^ missing ':'
Quoted 21 lines
1. First ECC uncorrectable error will be generated from ECC engine since ECC engine first calculates the ECC with all
0xffand match the calculated ECC with ECC code in OOB (which is again all0xff).
2. After getting ECC error, erased CW detection logic will be applied which is different for BCH and RS ECC
a. For BCH, HW checks if all the bytes in page are0xffand then
it updates the status in separate registerNAND_ERASED_CW_DETECT_STATUS.
b. For RS ECC, the HW reports the same error when reading an
erased CW, but it notifies that it is an erased CW by placing special characters at certain offsets in the buffer.So the erased CW detect status should be checked only if ECC engine generated the uncorrectable error.
Currently for all other operational errors also (like TIMEOUT, MPU errors, etc.), the erased CW detect logic is being applied so fix this and return EIO for other operational errors.
Quoted 6 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:1. Added more detail in commit message
2. Added comment before each if/else
Thanks for that, it's much more ease to review :)
Quoted 5 lines
3. Removed redundant check for
BS_UNCORRECTABLE_BITdrivers/mtd/nand/raw/qcom_nandc.c | 65 ++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 22 deletions(-)
Thanks,
Miquèl
Re: [PATCH v2 06/14] mtd: rawnand: qcom: erased page detection for uncorrectable errors only
Quoted 10 lines · Miquel Raynal
Hi Abhishek,
Quoted 2 lines · Abhishek Sahu
Following is the flow in the HW if controller tries to read erased page
Nit: ^ missing ':'
Sure. I will fix this.
Quoted 21 lines
Quoted 21 lines
1. First ECC uncorrectable error will be generated from ECC engine since ECC engine first calculates the ECC with all
0xffand match the calculated ECC with ECC code in OOB (which is again all0xff).
2. After getting ECC error, erased CW detection logic will be applied which is different for BCH and RS ECC
a. For BCH, HW checks if all the bytes in page are0xffand then
it updates the status in separate registerNAND_ERASED_CW_DETECT_STATUS.
b. For RS ECC, the HW reports the same error when reading an
erased CW, but it notifies that it is an erased CW by placing special characters at certain offsets in the buffer.So the erased CW detect status should be checked only if ECC engine generated the uncorrectable error.
Currently for all other operational errors also (like TIMEOUT, MPU errors, etc.), the erased CW detect logic is being applied so fix this and return EIO for other operational errors.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:1. Added more detail in commit message
2. Added comment before each if/elseThanks for that, it's much more ease to review :)
Thanks Miquel for your review.
Regards,
Abhishek
Quoted 12 lines
Quoted 7 lines
3. Removed redundant check for
BS_UNCORRECTABLE_BITdrivers/mtd/nand/raw/qcom_nandc.c | 65 ++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 22 deletions(-)Acked-by Miquel Raynal <[email protected]>Thanks,
Miquèl
PATCH v2 7/14 mtd: rawnand: qcom: fix null pointer access for erased page detection
In reply to Abhishek Sahuparse_read_errors can be called with only oob_buf in which case data_buf will be NULL. If data_buf is NULL, then don’t treat this page as completely erased in case of ECC uncorrectable error for RS ECC. For BCH ECC, the controller itself tells regarding erased page in status register.
---
* Changes from v1:
1. Added more detail in commit message
2. Added comment before each if/else
drivers/mtd/nand/raw/qcom_nandc.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +15 -3
@@ -1613,13 +1613,24 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,int ret, ecclen, extraooblen;void *eccbuf; -/* ignore erased codeword errors */ +/* +* For BCH ECC, ignore erased codeword errors, if +* ERASED_CW bits are set. +*/ if (host->bch_enabled) { erased = (erased_cw &ERASED_CW ) ==ERASED_CW ? true : false; - }else { +/* +* For RS ECC, HW reports the erased CW by placing +* special characters at certain offsets in the buffer. +* These special characters will be valid only if +* complete page is read i.e. data_buf is not NULL. +*/ + }else if (data_buf) { erased =erased_chunk_check_and_fixup (data_buf, data_len); + }else { + erased = false; }if (erased) {
@@ -1667,7 +1678,8 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf, max_bitflips =max (max_bitflips, stat); } - data_buf += data_len; +if (data_buf) + data_buf += data_len;if (oob_buf) oob_buf += oob_len + ecc->bytes; }
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v2 07/14] mtd: rawnand: qcom: fix null pointer access for erased page detection
Hi Abhishek,
Quoted 6 lines · Abhishek Sahu
parse_read_errorscan be called with onlyoob_bufin which casedata_bufwill be NULL. Ifdata_bufis NULL, then don’t treat this page as completely erased in case of ECC uncorrectable error for RS ECC. For BCH ECC, the controller itself tells regarding erased page in status register.
Quoted 6 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:1. Added more detail in commit message
2. Added comment before each if/else
Again, thanks for that.
Quoted 3 lines
drivers/mtd/nand/raw/qcom_nandc.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
Signature · 3 lines
Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v2 07/14] mtd: rawnand: qcom: fix null pointer access for erased page detection
Quoted 18 lines · Miquel Raynal
Hi Abhishek,
Quoted 6 lines · Abhishek Sahu
parse_read_errorscan be called with onlyoob_bufin which casedata_bufwill be NULL. Ifdata_bufis NULL, then don’t treat this page as completely erased in case of ECC uncorrectable error for RS ECC. For BCH ECC, the controller itself tells regarding erased page in status register.Quoted 6 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:1. Added more detail in commit message
2. Added comment before each if/else
Quoted 7 lines
Again, thanks for that.
Quoted 3 lines
drivers/mtd/nand/raw/qcom_nandc.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)Acked-by Miquel Raynal <[email protected]>
Thanks Miquel for your review.
Regards,
Abhishek
PATCH v2 8/14 mtd: rawnand: qcom: parse read errors for read oob also
In reply to Abhishek Sahuread_page and read_oob both calls the read_page_ecc function. The QCOM NAND controller protect the OOB available bytes with ECC so read errors should be checked for read_oob also. Now this patch moves the error checking code inside read_page_ecc so caller does not have to check explicitly for read errors.
---
* Changes from v1:
1. Minor code change for return early in case of error
drivers/mtd/nand/raw/qcom_nandc.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +9 -17
@@ -1700,6 +1700,7 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,struct nand_chip *chip = &host->chip;struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip);struct nand_ecc_ctrl *ecc = &chip->ecc; +u8 *data_buf_start = data_buf, *oob_buf_start = oob_buf;int i, ret;config_nand_page_read (nandc);
@@ -1760,12 +1761,14 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf, } ret =submit_descs (nandc); -if (ret) +free_descs (nandc); + +if (ret) {dev_err (nandc->dev,"failure to read page/oob\n" ); +return ret; + } -free_descs (nandc); - -return ret; +return parse_read_errors (host, data_buf_start, oob_buf_start); }/*
@@ -1810,20 +1813,14 @@ static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,struct qcom_nand_host *host =to_qcom_nand_host (chip);struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip);u8 *data_buf, *oob_buf =NULL ; -int ret;nand_read_page_op (chip, page,0 ,NULL ,0 ); data_buf = buf; oob_buf = oob_required ? chip->oob_poi :NULL ;clear_bam_transaction (nandc); - ret =read_page_ecc (host, data_buf, oob_buf); -if (ret) { -dev_err (nandc->dev,"failure to read page\n" ); -return ret; - } -return parse_read_errors (host, data_buf, oob_buf); +return read_page_ecc (host, data_buf, oob_buf); }/* implements ecc->read_page_raw() */
@@ -1913,7 +1910,6 @@ static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,struct qcom_nand_host *host =to_qcom_nand_host (chip);struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip);struct nand_ecc_ctrl *ecc = &chip->ecc; -int ret;clear_read_regs (nandc);clear_bam_transaction (nandc);
@@ -1922,11 +1918,7 @@ static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,set_address (host,0 , page);update_rw_regs (host, ecc->steps, true); - ret =read_page_ecc (host,NULL , chip->oob_poi); -if (ret) -dev_err (nandc->dev,"failure to read oob\n" ); - -return ret; +return read_page_ecc (host,NULL , chip->oob_poi); }/* implements ecc->write_page() */
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v2 9/14 mtd: rawnand: qcom: modify write_oob to remove read codeword part
In reply to Abhishek SahuQCOM NAND layout protect available OOB data bytes with ECC also so when ecc->write_oob (qcom_nandc_write_oob) is being called then it can't update just OOB bytes. Currently, it first reads the last codeword which includes old OOB bytes. Then it updates the old OOB bytes with new one and then again writes the codeword back. The reading codeword is unnecessary since all the other bytes should be 0xff only.
This patch removes the read part and updates the oob bytes with all other data bytes as 0xff.
---
* Changes from v1:
NEW CHANGE
drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +5 -12
@@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd,* implements ecc->write_oob() * * the NAND controller cannot write only data or only oob within a codeword, -* since ecc is calculated for the combined codeword. we first copy the -* entire contents for the last codeword(data + oob), replace the old oob -* with the new one in chip->oob_poi, and then write the entire codeword. -* this read-copy-write operation results in a slight performance loss. +* since ecc is calculated for the combined codeword. So make all the data +* bytes as 0xff and update the oob from chip->oob_poi, and then write +* the entire codeword again. */ static int qcom_nandc_write_oob (struct mtd_info *mtd,struct nand_chip *chip,int page)
@@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,int data_size, oob_size;int ret; - host->use_ecc = true; - -clear_bam_transaction (nandc); - ret =copy_last_cw (host, page); -if (ret) -return ret; - -clear_read_regs (nandc);clear_bam_transaction (nandc);/* calculate the data and oob size for the last codeword/step */ data_size = ecc->size - ((ecc->steps -1 ) <<2 ); oob_size = mtd->oobavail; + host->use_ecc = true; +memset (nandc->data_buffer,0xff , host->cw_data);/* override new oob content to last codeword */ mtd_ooblayout_get_databytes (mtd, nandc->data_buffer + data_size, oob,0 , mtd->oobavail);
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v2 09/14] mtd: rawnand: qcom: modify write_oob to remove read codeword part
Hi Abhishek,
Some nitpicking below.
Quoted 1 line · Abhishek Sahu
QCOM NAND layout protect available OOB data bytes with ECC also so
^controller
Quoted 1 line
when
ecc->write_oob(qcom_nandc_write_oob) is being called then it
You can just state "->write_oob()"
Quoted 3 lines
can't update just OOB bytes. Currently, it first reads the last codeword which includes old OOB bytes. Then it updates the old OOB bytes with new one and then again writes the codeword back.
ones?
Quoted 2 lines
The reading codeword is unnecessary since all the other bytes should be
0xffonly.
since the user is responsible to
have these bytes cleared to 0xFF.
Quoted 2 lines
This patch removes the read part and updates the oob bytes with
s/oob/OOB/
Quoted 1 line
all other data bytes as
0xff.
The end of the sentence is not clear for me. Do you mean that padding with 0xFF is realized before write?
Quoted 15 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW CHANGE
drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)drivers/mtd/nand/raw/qcom_nandc.c +0
-0
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_write_page_raw(struct mtd_info *mtd, · line 2069
Quoted 3 lines
drivers/mtd/nand/raw/qcom_nandc.c +0
-0@@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd,* implements ecc->write_oob() * * the NAND controller cannot write only data or only oob within a codeword,
s/oob/OOB/
Remove the trailing ','
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_write_page_raw(struct mtd_info *mtd, · line 2070
Quoted 5 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-4-* since ecc is calculated for the combined codeword. we first copy the -* entire contents for the last codeword(data + oob), replace the old oob -* with the new one in chip->oob_poi, and then write the entire codeword. -* this read-copy-write operation results in a slight performance loss. +* since ecc is calculated for the combined codeword. So make all the data
s/ecc/ECC/
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_write_page_raw(struct mtd_info *mtd, · line 2072
Quoted 2 lines
drivers/mtd/nand/raw/qcom_nandc.c +2
-0+* bytes as 0xff and update the oob from chip->oob_poi, and then write +* the entire codeword again.
What about "Pad the data area with OxFF before writing."?
Quoted 11 lines
drivers/mtd/nand/raw/qcom_nandc.c +0
-4*/ static int qcom_nandc_write_oob (struct mtd_info *mtd,struct nand_chip *chip,int page)@@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,int data_size, oob_size;int ret; - host->use_ecc = true; - -clear_bam_transaction (nandc); - ret =copy_last_cw (host, page);
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, · line 2089
Quoted 10 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-4-if (ret) -return ret; - -clear_read_regs (nandc);clear_bam_transaction (nandc);/* calculate the data and oob size for the last codeword/step */ data_size = ecc->size - ((ecc->steps -1 ) <<2 ); oob_size = mtd->oobavail; + host->use_ecc = true;
You don't need to move this line here, do you?
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, · line 2094
Quoted 5 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-0+memset (nandc->data_buffer,0xff , host->cw_data);/* override new oob content to last codeword */ mtd_ooblayout_get_databytes (mtd, nandc->data_buffer + data_size, oob,0 , mtd->oobavail);
Once fixed, you can add my:
Thanks,
Miquèl
Re: [PATCH v2 09/14] mtd: rawnand: qcom: modify write_oob to remove read codeword part
Quoted 108 lines · Miquel Raynal
Hi Abhishek,
Some nitpicking below.
Quoted 1 line · Abhishek Sahu
QCOM NAND layout protect available OOB data bytes with ECC also so
^controller
Quoted 1 line
when
ecc->write_oob(qcom_nandc_write_oob) is being called then itYou can just state "->write_oob()"
Quoted 3 lines
can't update just OOB bytes. Currently, it first reads the last codeword which includes old OOB bytes. Then it updates the old OOB bytes with new one and then again writes the codeword back.
ones?
Quoted 2 lines
The reading codeword is unnecessary since all the other bytes should be
0xffonly.since the user is responsible to
have these bytes cleared to0xFF.Quoted 2 lines
This patch removes the read part and updates the oob bytes with
s/oob/OOB/
Quoted 1 line
all other data bytes as
0xff.The end of the sentence is not clear for me. Do you mean that padding with
0xFFis realized before write?Quoted 15 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW CHANGE
drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)drivers/mtd/nand/raw/qcom_nandc.c +0
-0drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_write_page_raw(struct · line 2071
Quoted 5 lines
drivers/mtd/nand/raw/qcom_nandc.c +0
-0@@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(structmtd_info *mtd, * implements ecc->write_oob() * * the NAND controller cannot write only data or only oob within a codeword,s/oob/OOB/
Remove the trailing ','
Quoted 10 lines
drivers/mtd/nand/raw/qcom_nandc.c +0
-1-* since ecc is calculated for the combined codeword. we first copy the
- * entire contents for the last codeword(data + oob), replace the old oob
- * with the new one inchip->oob_poi, and then write the entire codeword.
- * this read-copy-write operation results in a slight performance loss.
+ * since ecc is calculated for the combined codeword. So make all the datas/ecc/ECC/
Quoted 3 lines
+ * bytes as
0xffand update the oob fromchip->oob_poi, and then write
+ * the entire codeword again.What about "Pad the data area with OxFF before writing."?
Quoted 13 lines
*/
static int qcom_nandc_write_oob(structmtd_info*mtd, structnand_chip*chip,drivers/mtd/nand/raw/qcom_nandc.c +0
-4int page)@@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int data_size, oob_size;int ret; - host->use_ecc = true; - -clear_bam_transaction (nandc); - ret =copy_last_cw (host, page);drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_write_oob(struct · line 2090
Quoted 10 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-4-if (ret) -return ret; - -clear_read_regs (nandc);clear_bam_transaction (nandc);/* calculate the data and oob size for the last codeword/step */ data_size = ecc->size - ((ecc->steps -1 ) <<2 ); oob_size = mtd->oobavail; + host->use_ecc = true;You don't need to move this line here, do you?
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-0+memset (nandc->data_buffer,0xff , host->cw_data);/* override new oob content to last codeword */ mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size,oob,
0,mtd->oobavail);Once fixed, you can add my:
Thanks Miquel.
I will fix all these and update the patch.
Regards,
Abhishek
Quoted 5 lines
Acked-by Miquel Raynal <[email protected]>Thanks,
Miquèl
PATCH v2 10/14 mtd: rawnand: qcom: fix return value for raw page read
In reply to Abhishek SahuCurrently zero is being returned for all raw page read so fix the same.
---
* Changes from v1:
NEW CHANGE
drivers/mtd/nand/raw/qcom_nandc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/nand/raw/qcom_nandc.c +1 -1
@@ -1900,7 +1900,7 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,free_descs (nandc); -return 0 ; +return ret; }/* implements ecc->read_oob() */
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Miquel Raynal
maintainerRe: [PATCH v2 10/14] mtd: rawnand: qcom: fix return value for raw page read
Hi Abhishek,
Quoted 2 lines · Abhishek Sahu
Currently zero is being returned for all raw page read so fix the same.
What about "Fix value returned by ->read_page_raw() to be the actual operation status, instead of always 0."?
Quoted 15 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW CHANGE
drivers/mtd/nand/raw/qcom_nandc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/mtd/nand/raw/qcom_nandc.c +0
-0
drivers/mtd/nand/raw/qcom_nandc.c static int qcom_nandc_read_page_raw(struct mtd_info *mtd, · line 1906
Quoted 8 lines
drivers/mtd/nand/raw/qcom_nandc.c +1
-1@@ -1900,7 +1900,7 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,free_descs (nandc); -return 0 ; +return ret; }/* implements ecc->read_oob() */
Thanks,
Miquèl
Signature · 3 lines
Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v2 10/14] mtd: rawnand: qcom: fix return value for raw page read
Quoted 11 lines · Miquel Raynal
Hi Abhishek,
Quoted 2 lines · Abhishek Sahu
Currently zero is being returned for all raw page read so fix the same.
What about "Fix value returned by ->read_page_raw() to be the actual operation status, instead of always 0."?
Sure Miquel. It looks better.
I will change this.
Thanks,
Abhishek
Quoted 27 lines
Quoted 23 lines
Signed-off-by Abhishek Sahu <[email protected]>---
* Changes from v1:NEW CHANGE
drivers/mtd/nand/raw/qcom_nandc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/mtd/nand/raw/qcom_nandc.c +1
-1@@ -1900,7 +1900,7 @@ static int qcom_nandc_read_page_raw(structmtd_info *mtd,free_descs (nandc ); -return 0 ; +return ret; }Quoted 1 line
/* implements
ecc->read_oob()*/Thanks,
Miquèl
PATCH v2 11/14 mtd: rawnand: qcom: minor code reorganization for bad block check
In reply to Abhishek SahuThe QCOM NAND layout is such that, the bad block byte offset for last codeword will come to first byte in spare area. Currently, the raw read for last codeword is being done with copy_last_cw function. It does following 2 things
1. Read the last codeword bytes from NAND chip to NAND controller internal HW buffer.
2. Copy all these bytes from this HW buffer to driver RAM buffer.
For bad block check, maximum two bytes are required so instead of copying the complete bytes in step 2, only those bbm_size bytes can be copied.
This patch does minor code reorganization for the same. After this, copy_last_cw function won’t be required.
---
* Changes from v1:
NEW CHANGE
drivers/mtd/nand/raw/qcom_nandc.c | 66 +++++++++++++++------------------------ 1 file changed, 25 insertions(+), 41 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +25 -41
@@ -1771,41 +1771,6 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,return parse_read_errors (host, data_buf_start, oob_buf_start); } -/* -* a helper that copies the last step/codeword of a page (containing free oob) -* into our local buffer -*/ -static int copy_last_cw (struct qcom_nand_host *host,int page) -{ -struct nand_chip *chip = &host->chip; -struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip); -struct nand_ecc_ctrl *ecc = &chip->ecc; -int size; -int ret; - -clear_read_regs (nandc); - - size = host->use_ecc ? host->cw_data : host->cw_size; - -/* prepare a clean read buffer */ -memset (nandc->data_buffer,0xff , size); - -set_address (host, host->cw_size * (ecc->steps -1 ), page); -update_rw_regs (host,1 , true); - -config_nand_single_cw_page_read (nandc); - -read_data_dma (nandc,FLASH_BUF_ACC , nandc->data_buffer, size,0 ); - - ret =submit_descs (nandc); -if (ret) -dev_err (nandc->dev,"failed to copy last codeword\n" ); - -free_descs (nandc); - -return ret; -} -/* implements ecc->read_page() */ static int qcom_nandc_read_page (struct mtd_info *mtd,struct nand_chip *chip,uint8_t *buf,int oob_required,int page)
@@ -2121,6 +2086,7 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)struct nand_ecc_ctrl *ecc = &chip->ecc;int page, ret, bbpos, bad =0 ;u32 flash_status; +u8 *bbm_bytes_buf = chip->data_buf; page = (int )(ofs >> chip->page_shift) & chip->pagemask;
@@ -2131,11 +2097,31 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)* that contains the BBM */ host->use_ecc = false; + bbpos = mtd->writesize - host->cw_size * (ecc->steps -1 );clear_bam_transaction (nandc); - ret =copy_last_cw (host, page); -if (ret) +clear_read_regs (nandc); + +set_address (host, host->cw_size * (ecc->steps -1 ), page); +update_rw_regs (host,1 , true); + +/* +* The last codeword data will be copied from NAND device to NAND +* controller internal HW buffer. Copy only required BBM size bytes +* from this HW buffer to bbm_bytes_buf which is present at +* bbpos offset. +*/ +nandc_set_read_loc (nandc,0 , bbpos, host->bbm_size,1 ); +config_nand_single_cw_page_read (nandc); +read_data_dma (nandc,FLASH_BUF_ACC + bbpos, bbm_bytes_buf, + host->bbm_size,0 ); + + ret =submit_descs (nandc); +free_descs (nandc); +if (ret) { +dev_err (nandc->dev,"failed to copy bad block bytes\n" ); goto err; + } flash_status =le32_to_cpu (nandc->reg_read_buf[0 ]);
@@ -2144,12 +2130,10 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) goto err; } - bbpos = mtd->writesize - host->cw_size * (ecc->steps -1 ); - - bad = nandc->data_buffer[bbpos] !=0xff ; + bad = bbm_bytes_buf[0 ] !=0xff ;if (chip->options &NAND_BUSWIDTH_16 ) - bad = bad || (nandc->data_buffer[bbpos +1 ] !=0xff ); + bad = bad || (bbm_bytes_buf[1 ] !=0xff ); err:return bad; }
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v2 12/14 mtd: rawnand: qcom: check for operation errors in case of raw read
In reply to Abhishek SahuCurrently there is no error checking for raw read. For raw reads, there won’t be any ECC failure but the operational failures are possible so schedule the NAND_FLASH_STATUS read after each codeword.
---
* Changes from v1:
1. Removed the code for copy_last_cw.
drivers/mtd/nand/raw/qcom_nandc.c | 58 +++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 18 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +40 -18
@@ -1099,7 +1099,8 @@ static void config_nand_page_read(struct qcom_nand_controller *nandc)* Helper to prepare DMA descriptors for configuring registers * before reading each codeword in NAND page. */ -static void config_nand_cw_read (struct qcom_nand_controller *nandc) +static void +config_nand_cw_read (struct qcom_nand_controller *nandc,bool use_ecc) {if (nandc->props->is_bam) write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
@@ -1108,19 +1109,25 @@ static void config_nand_cw_read(struct qcom_nand_controller *nandc)write_reg_dma (nandc,NAND_FLASH_CMD ,1 ,NAND_BAM_NEXT_SGL );write_reg_dma (nandc,NAND_EXEC_CMD ,1 ,NAND_BAM_NEXT_SGL ); -read_reg_dma (nandc,NAND_FLASH_STATUS ,2 ,0 ); -read_reg_dma (nandc,NAND_ERASED_CW_DETECT_STATUS ,1 , -NAND_BAM_NEXT_SGL ); +if (use_ecc) { +read_reg_dma (nandc,NAND_FLASH_STATUS ,2 ,0 ); +read_reg_dma (nandc,NAND_ERASED_CW_DETECT_STATUS ,1 , +NAND_BAM_NEXT_SGL ); + }else { +read_reg_dma (nandc,NAND_FLASH_STATUS ,1 ,NAND_BAM_NEXT_SGL ); + } }/* * Helper to prepare dma descriptors to configure registers needed for reading a * single codeword in page */ -static void config_nand_single_cw_page_read (struct qcom_nand_controller *nandc) +static void +config_nand_single_cw_page_read (struct qcom_nand_controller *nandc, +bool use_ecc) {config_nand_page_read (nandc); -config_nand_cw_read (nandc); +config_nand_cw_read (nandc, use_ecc); }/*
@@ -1201,7 +1208,7 @@ static int nandc_param(struct qcom_nand_host *host) nandc->buf_count =512 ;memset (nandc->data_buffer,0xff , nandc->buf_count); -config_nand_single_cw_page_read (nandc); +config_nand_single_cw_page_read (nandc, false);read_data_dma (nandc,FLASH_BUF_ACC , nandc->data_buffer, nandc->buf_count,0 );
@@ -1565,6 +1572,23 @@ struct read_stats {__le32 erased_cw; }; +/* reads back FLASH_STATUS register set by the controller */ +static int check_flash_errors (struct qcom_nand_host *host,int cw_cnt) +{ +struct nand_chip *chip = &host->chip; +struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip); +int i; + +for (i =0 ; i < cw_cnt; i++) { +u32 flash =le32_to_cpu (nandc->reg_read_buf[i]); + +if (flash & (FS_OP_ERR |FS_MPU_ERR )) +return -EIO ; + } + +return 0 ; +} +/* * reads back status registers set by the controller to notify page read * errors. this is equivalent to what 'ecc->correct()' would do.
@@ -1731,7 +1755,7 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf, } } -config_nand_cw_read (nandc); +config_nand_cw_read (nandc, true);if (data_buf) read_data_dma(nandc, FLASH_BUF_ACC, data_buf,
@@ -1841,7 +1865,7 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,nandc_set_read_loc (nandc,3 , read_loc, oob_size2,1 ); } -config_nand_cw_read (nandc); +config_nand_cw_read (nandc, false);read_data_dma (nandc, reg_off, data_buf, data_size1,0 ); reg_off += data_size1;
@@ -1860,12 +1884,13 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd, } ret =submit_descs (nandc); -if (ret) +free_descs (nandc); +if (ret) {dev_err (nandc->dev,"failure to read raw page\n" ); +return ret; + } -free_descs (nandc); - -return ret; +return check_flash_errors (host, ecc->steps); }/* implements ecc->read_oob() */
@@ -2085,7 +2110,6 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip);struct nand_ecc_ctrl *ecc = &chip->ecc;int page, ret, bbpos, bad =0 ; -u32 flash_status;u8 *bbm_bytes_buf = chip->data_buf; page = (int )(ofs >> chip->page_shift) & chip->pagemask;
@@ -2112,7 +2136,7 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)* bbpos offset. */ nandc_set_read_loc (nandc,0 , bbpos, host->bbm_size,1 ); -config_nand_single_cw_page_read (nandc); +config_nand_single_cw_page_read (nandc, host->use_ecc);read_data_dma (nandc,FLASH_BUF_ACC + bbpos, bbm_bytes_buf, host->bbm_size,0 );
@@ -2123,9 +2147,7 @@ static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs) goto err; } - flash_status =le32_to_cpu (nandc->reg_read_buf[0 ]); - -if (flash_status & (FS_OP_ERR |FS_MPU_ERR )) { +if (check_flash_errors (host,1 )) {dev_warn (nandc->dev,"error when trying to read BBM\n" ); goto err; }
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v2 13/14 mtd: rawnand: qcom: helper function for raw read
In reply to Abhishek SahuThis patch does minor code reorganization for raw reads. Currently the raw read is required for complete page but for subsequent patches related with erased codeword bit flips detection, only few CW should be read. So, this patch adds helper function and introduces the read CW bitmask which specifies which CW reads are required in complete page.
---
* Changes from v1:
1. Included more detail in function comment
drivers/mtd/nand/raw/qcom_nandc.c | 197 ++++++++++++++++++++++++-------------- 1 file changed, 123 insertions(+), 74 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +123 -74
@@ -1590,6 +1590,127 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt) }/* +* Helper to perform the page raw read operation. The read_cw_mask will be +* used to specify the codewords (CW) for which the data should be read. The +* single page contains multiple CW. +* +* Normally other NAND controllers store the data in main area and +* ecc bytes in oob area. So, if page size is 2048+64 then 2048 +* data bytes will go in main area followed by ECC bytes. The QCOM NAND +* controller follows different layout in which the data+oob is internally +* divided in 528/532 bytes CW and each CW contains 516 bytes followed by +* ECC parity bytes for that CW. By this, 4 available oob bytes per CW +* will also be protected with ECC. +* +* For each CW read, following are the 2 steps +* 1. Read the codeword bytes from NAND chip to NAND controller internal HW +* buffer. +* 2. Copy all these bytes from this HW buffer to driver RAM buffer. +* +* Sometime, only few CW data is required in complete page. The read_cw_mask +* specifies which CW in a page needs to be read. Start address will be +* determined with this CW mask to skip unnecessary data copy from NAND +* flash device. Then, actual data copy from NAND controller HW internal buffer +* to data buffer will be done only for the CWs, which have the mask set. +*/ +static int +nandc_read_page_raw (struct mtd_info *mtd,struct nand_chip *chip, +u8 *data_buf,u8 *oob_buf, +int page,unsigned long read_cw_mask) +{ +struct qcom_nand_host *host =to_qcom_nand_host (chip); +struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip); +struct nand_ecc_ctrl *ecc = &chip->ecc; +int i, ret; +int read_loc, start_step, last_step; + +nand_read_page_op (chip, page,0 ,NULL ,0 ); + + host->use_ecc = false; + start_step =ffs (read_cw_mask) -1 ; + last_step =fls (read_cw_mask); + +clear_bam_transaction (nandc); +set_address (host, host->cw_size * start_step, page); +update_rw_regs (host, last_step - start_step, true); +config_nand_page_read (nandc); + +for (i = start_step; i < last_step; i++) { +int data_size1, data_size2, oob_size1, oob_size2; +int reg_off =FLASH_BUF_ACC ; + + data_size1 = mtd->writesize - host->cw_size * (ecc->steps -1 ); + oob_size1 = host->bbm_size; + +if (i == (ecc->steps -1 )) { + data_size2 = ecc->size - data_size1 - + ((ecc->steps -1 ) <<2 ); + oob_size2 = (ecc->steps <<2 ) + host->ecc_bytes_hw + + host->spare_bytes; + }else { + data_size2 = host->cw_data - data_size1; + oob_size2 = host->ecc_bytes_hw + host->spare_bytes; + } + +/* +* Don't perform actual data copy from NAND controller internal +* HW buffer to data buffer through DMA for this codeword. +*/ +if (!(read_cw_mask &BIT (i))) { +if (nandc->props->is_bam) +nandc_set_read_loc (nandc,0 ,0 ,0 ,1 ); + +config_nand_cw_read (nandc, false); + + data_buf += data_size1 + data_size2; + oob_buf += oob_size1 + oob_size2; + +continue ; + } + +if (nandc->props->is_bam) { + read_loc =0 ; +nandc_set_read_loc (nandc,0 , read_loc, data_size1,0 ); + read_loc += data_size1; + +nandc_set_read_loc (nandc,1 , read_loc, oob_size1,0 ); + read_loc += oob_size1; + +nandc_set_read_loc (nandc,2 , read_loc, data_size2,0 ); + read_loc += data_size2; + +nandc_set_read_loc (nandc,3 , read_loc, oob_size2,1 ); + } + +config_nand_cw_read (nandc, false); + +read_data_dma (nandc, reg_off, data_buf, data_size1,0 ); + reg_off += data_size1; + data_buf += data_size1; + +read_data_dma (nandc, reg_off, oob_buf, oob_size1,0 ); + reg_off += oob_size1; + oob_buf += oob_size1; + +read_data_dma (nandc, reg_off, data_buf, data_size2,0 ); + reg_off += data_size2; + data_buf += data_size2; + +read_data_dma (nandc, reg_off, oob_buf, oob_size2,0 ); + oob_buf += oob_size2; + } + + ret =submit_descs (nandc); +free_descs (nandc); +if (ret) { +dev_err (nandc->dev,"failure to read raw page\n" ); +return ret; + } + +return check_flash_errors (host, ecc->steps); +} + +/* * reads back status registers set by the controller to notify page read * errors. this is equivalent to what 'ecc->correct()' would do. */
@@ -1817,80 +1938,8 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page) { - struct qcom_nand_host *host = to_qcom_nand_host(chip); -struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip); -u8 *data_buf, *oob_buf; -struct nand_ecc_ctrl *ecc = &chip->ecc; -int i, ret; -int read_loc; - -nand_read_page_op (chip, page,0 ,NULL ,0 ); - data_buf = buf; - oob_buf = chip->oob_poi; - - host->use_ecc = false; - -clear_bam_transaction (nandc); -update_rw_regs (host, ecc->steps, true); -config_nand_page_read (nandc); - -for (i =0 ; i < ecc->steps; i++) { -int data_size1, data_size2, oob_size1, oob_size2; -int reg_off =FLASH_BUF_ACC ; - - data_size1 = mtd->writesize - host->cw_size * (ecc->steps -1 ); - oob_size1 = host->bbm_size; - -if (i == (ecc->steps -1 )) { - data_size2 = ecc->size - data_size1 - - ((ecc->steps -1 ) <<2 ); - oob_size2 = (ecc->steps <<2 ) + host->ecc_bytes_hw + - host->spare_bytes; - }else { - data_size2 = host->cw_data - data_size1; - oob_size2 = host->ecc_bytes_hw + host->spare_bytes; - } - -if (nandc->props->is_bam) { - read_loc =0 ; -nandc_set_read_loc (nandc,0 , read_loc, data_size1,0 ); - read_loc += data_size1; - -nandc_set_read_loc (nandc,1 , read_loc, oob_size1,0 ); - read_loc += oob_size1; - -nandc_set_read_loc (nandc,2 , read_loc, data_size2,0 ); - read_loc += data_size2; - -nandc_set_read_loc (nandc,3 , read_loc, oob_size2,1 ); - } - -config_nand_cw_read (nandc, false); - -read_data_dma (nandc, reg_off, data_buf, data_size1,0 ); - reg_off += data_size1; - data_buf += data_size1; - -read_data_dma (nandc, reg_off, oob_buf, oob_size1,0 ); - reg_off += oob_size1; - oob_buf += oob_size1; - -read_data_dma (nandc, reg_off, data_buf, data_size2,0 ); - reg_off += data_size2; - data_buf += data_size2; - -read_data_dma (nandc, reg_off, oob_buf, oob_size2,0 ); - oob_buf += oob_size2; - } - - ret =submit_descs (nandc); -free_descs (nandc); -if (ret) { -dev_err (nandc->dev,"failure to read raw page\n" ); -return ret; - } - -return check_flash_errors (host, ecc->steps); + return nandc_read_page_raw(mtd, chip, buf, chip->oob_poi, page, + BIT(chip->ecc.steps) - 1); }/* implements ecc->read_oob() */
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v2 14/14 mtd: rawnand: qcom: erased page bitflips detection
In reply to Abhishek SahuNAND parts can have bitflips in an erased page due to the process technology used. In this case, qpic nand controller is not able to identify that page as an erased page. Currently the driver calls nand_check_erased_ecc_chunk for identifying the erased pages but this won’t work always since the checking is being with ECC engine returned data. In case of bitflips, the ECC engine tries to correct the data and then it generates the uncorrectable error. Now, this data is not equal to original raw data. For erased CW identification, the raw data should be read again from NAND device and this nand_check_erased_ecc_chunk function should be called for raw data only.
Now following logic is being added to identify the erased codeword bitflips.
1. In most of the case, not all the codewords will have bitflips and only single CW will have bitflips. So, there is no need to read the complete raw page data. The NAND raw read can be scheduled for any CW in page. The NAND controller works on CW basis and it will update the status register after each CW read. Maintain the bitmask for the CW which generated the uncorrectable error.
2. Schedule the raw flash read from NAND flash device to NAND controller buffer for all these CWs between first and last uncorrectable errors CWs. Copy the content from NAND controller internal HW buffer to actual data buffer only for the uncorrectable errors CWs so that other CW data content won’t be affected, and unnecessary data copy can be avoided.
3. Both DATA and OOB need to be checked for number of 0. The top-level API can be called with only data buf or oob buf so use chip->databuf if data buf is null and chip->oob_poi if oob buf is null for copying the raw bytes temporarily.
4. For each CW, check the number of 0 in cw_data and usable oob bytes, The bbm and spare (unused) bytes bit flip won’t affect the ECC so don’t check the number of bitflips in this area.
---
* Changes from v1:
1. Minor change in commit message
2. invalidate pagebuf if databuf or oob_poi is used
drivers/mtd/nand/raw/qcom_nandc.c | 135 +++++++++++++++++++++++++++----------- 1 file changed, 98 insertions(+), 37 deletions(-)
drivers/mtd/nand/raw/qcom_nandc.c +98 -37
@@ -1711,20 +1711,103 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt) }/* +* Bitflips can happen in erased codewords also so this function counts the +* number of 0 in each CW for which ECC engine returns the uncorrectable +* error. The page will be assumed as erased if this count is less than or +* equal to the ecc->strength for each CW. +* +* 1. Both DATA and OOB need to be checked for number of 0. The +* top-level API can be called with only data buf or oob buf so use +* chip->data_buf if data buf is null and chip->oob_poi if oob buf +* is null for copying the raw bytes. +* 2. Perform raw read for all the CW which has uncorrectable errors. +* 3. For each CW, check the number of 0 in cw_data and usable oob bytes. +* The bbm and spare bytes bit flip won’t affect the ECC so don’t check +* the number of bitflips in this area. +*/ +static int +check_for_erased_page (struct qcom_nand_host *host,u8 *data_buf, +u8 *oob_buf,unsigned long uncorrectable_err_cws, +int page,unsigned int max_bitflips) +{ +struct nand_chip *chip = &host->chip; +struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip); +struct mtd_info *mtd =nand_to_mtd (chip); +struct nand_ecc_ctrl *ecc = &chip->ecc; +int i, start_step, last_step, ret =0 ; + + start_step =ffs (uncorrectable_err_cws) -1 ; + last_step =fls (uncorrectable_err_cws); + +if (!data_buf) { + data_buf = chip->data_buf; + chip->pagebuf =-1 ; + } + +if (!oob_buf) { + oob_buf = chip->oob_poi; + chip->pagebuf =-1 ; + } + + data_buf += start_step * host->cw_data; + oob_buf += start_step * ecc->bytes; + +clear_read_regs (nandc); +nandc_read_page_raw (mtd, chip, data_buf, oob_buf, page, + uncorrectable_err_cws); + +for (i = start_step; i < last_step; i++) { +int data_size, oob_size; + +if (i == (ecc->steps -1 )) { + data_size = ecc->size - ((ecc->steps -1 ) <<2 ); + oob_size = (ecc->steps <<2 ) + host->ecc_bytes_hw; + }else { + data_size = host->cw_data; + oob_size = host->ecc_bytes_hw; + } + +if (uncorrectable_err_cws &BIT (i)) { +/* +* make sure it isn't an erased page reported +* as not-erased by HW because of a few bitflips +*/ + ret =nand_check_erased_ecc_chunk (data_buf, + data_size, oob_buf + host->bbm_size, + oob_size,NULL , +0 , ecc->strength); +if (ret <0 ) { + mtd->ecc_stats.failed++; + }else { + mtd->ecc_stats.corrected += ret; + max_bitflips = + max_t(unsigned int, max_bitflips, ret); + } + } + + data_buf += data_size; + oob_buf += ecc->bytes; + } + +return max_bitflips; +} + +/* * reads back status registers set by the controller to notify page read * errors. this is equivalent to what 'ecc->correct()' would do. */ static int parse_read_errors (struct qcom_nand_host *host,u8 *data_buf, -u8 *oob_buf) +u8 *oob_buf,int page) {struct nand_chip *chip = &host->chip;struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip);struct mtd_info *mtd =nand_to_mtd (chip);struct nand_ecc_ctrl *ecc = &chip->ecc; -unsigned int max_bitflips =0 ; +unsigned int max_bitflips =0 , uncorrectable_err_cws =0 ;struct read_stats *buf; -bool flash_op_err = false; +bool flash_op_err = false, erased;int i; +u8 *data_buf_start = data_buf, *oob_buf_start = oob_buf; buf = (struct read_stats *)nandc->reg_read_buf;nandc_read_buffer_sync (nandc, true);
@@ -1754,10 +1837,6 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,* codeword detection check will be done. */ if ((flash &FS_OP_ERR ) && (buffer &BS_UNCORRECTABLE_BIT )) { -bool erased; -int ret, ecclen, extraooblen; -void *eccbuf; -/* * For BCH ECC, ignore erased codeword errors, if * ERASED_CW bits are set.
@@ -1778,31 +1857,8 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf, erased = false; } -if (erased) { - data_buf += data_len; -if (oob_buf) - oob_buf += oob_len + ecc->bytes; -continue ; - } - - eccbuf = oob_buf ? oob_buf + oob_len :NULL ; - ecclen = oob_buf ? host->ecc_bytes_hw :0 ; - extraooblen = oob_buf ? oob_len :0 ; - -/* -* make sure it isn't an erased page reported -* as not-erased by HW because of a few bitflips -*/ - ret =nand_check_erased_ecc_chunk (data_buf, - data_len, eccbuf, ecclen, oob_buf, - extraooblen, ecc->strength); -if (ret <0 ) { - mtd->ecc_stats.failed++; - }else { - mtd->ecc_stats.corrected += ret; - max_bitflips = - max_t(unsigned int, max_bitflips, ret); - } +if (!erased) + uncorrectable_err_cws |=BIT (i);/* * Check if MPU or any other operational error (timeout, * device failure, etc.) happened for this codeword and
@@ -1832,7 +1888,12 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,if (flash_op_err)return -EIO ; -return max_bitflips; +if (!uncorrectable_err_cws) +return max_bitflips; + +return check_for_erased_page (host, data_buf_start, oob_buf_start, + uncorrectable_err_cws, page, + max_bitflips); }/*
@@ -1840,7 +1901,7 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,* ecc->read_oob() */ static int read_page_ecc (struct qcom_nand_host *host,u8 *data_buf, -u8 *oob_buf) +u8 *oob_buf,int page) {struct nand_chip *chip = &host->chip;struct qcom_nand_controller *nandc =get_qcom_nand_controller (chip);
@@ -1913,7 +1974,7 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,return ret; } -return parse_read_errors (host, data_buf_start, oob_buf_start); +return parse_read_errors (host, data_buf_start, oob_buf_start, page); }/* implements ecc->read_page() */
@@ -1930,7 +1991,7 @@ static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,clear_bam_transaction (nandc); -return read_page_ecc (host, data_buf, oob_buf); +return read_page_ecc (host, data_buf, oob_buf, page); }/* implements ecc->read_page_raw() */
@@ -1957,7 +2018,7 @@ static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,set_address (host,0 , page);update_rw_regs (host, ecc->steps, true); -return read_page_ecc (host,NULL , chip->oob_poi); +return read_page_ecc (host,NULL , chip->oob_poi, page); }/* implements ecc->write_page() */
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation