On 2018-04-10 14:29, Miquel Raynal wrote:
Hi Abhishek,
On Wed, 4 Apr 2018 18:12:19 +0530, Abhishek Sahu
<absahu@xxxxxxxxxxxxxx> wrote:
The NAND flash controller generates ECC uncorrectable error
first in case of completely erased page. Currently driver
applies the erased page detection logic for other operation
errors also so fix this and return EIO for other operational
errors.
I am sorry I don't understand very well what is the purpose of this
patch, could you please explain it again?
Do you mean that you want to avoid having rising ECC errors when you
read erased pages?
Thanks Miquel for your review.
QCOM NAND flash controller has in built erased page
detection HW.
Following 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 HW checks if
all the bytes in page are 0xff and then it updates the
status in separate register NAND_ERASED_CW_DETECT_STATUS
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 register was being
checked.
Signed-off-by: Abhishek Sahu <absahu@xxxxxxxxxxxxxx>
---
drivers/mtd/nand/qcom_nandc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/qcom_nandc.c
b/drivers/mtd/nand/qcom_nandc.c
index 17321fc..57c16a6 100644
--- a/drivers/mtd/nand/qcom_nandc.c
+++ b/drivers/mtd/nand/qcom_nandc.c
@@ -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,7 +1600,7 @@ 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)) {
+ if ((flash & FS_OP_ERR) && (buffer & BS_UNCORRECTABLE_BIT)) {
And later you have another "if (buffer & BS_UNCORRECTABLE_BIT)" which
is then redundant, unless that is not what you actually want to do?
Yes. That check seems to be redundant. I will fix that.
Maybe you can add comments before the if ()/ else if () to explain in
which case you enter each branch.
Sure. That would be better. Will add the same in next patch set.
bool erased;
/* ignore erased codeword errors */
@@ -1641,6 +1642,8 @@ static int parse_read_errors(struct
qcom_nand_host *host, u8 *data_buf,
max_t(unsigned int, max_bitflips, ret);
}
}
+ } else if (flash & (FS_OP_ERR | FS_MPU_ERR)) {
+ flash_op_err = true;
} else {
unsigned int stat;
@@ -1654,6 +1657,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;
+
In you are propagating an error related to the controller, this is
fine, but I think you just want to raise the fact that a NAND
uncorrectable error occurred, in this case you should just increment
mtd->ecc_stats.failed and return 0 (returning max_bitflips here would
be
fine too has it would be 0 too).
The flash_op_err will be for other operational errors only (like
timeout,
MPU error, device failure etc). For correctable errors,
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;
Already, it is incrementing mtd->ecc_stats.failed
Thanks,
Abhishek
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
