Re: [PATCH v4 14/15] mtd: rawnand: qcom: erased page bitflips detection

Miquel Raynal
2018-06-26T18:02:37Z

Hi Abhishek,

On Wed, 20 Jun 2018 12:57:41 +0530, Abhishek Sahu
<absahu@codeaurora.org> wrote:

> NAND parts can have bitflips in an erased page due to the
> process technology used. In this case, QCOM 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 cases, 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. Do raw read for all the CW's which generated the uncorrectable
> error.
> 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.
>
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
> * Changes from v3:
>
> 1. Major changes in erased codeword detection for
> raw read function

I really prefer this version, much more readable from my point of view!

>
> * Changes from v2:
> NONE
>
> * 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 | 127 +++++++++++++++++++++++++++-----------
> 1 file changed, 90 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 160acdf..e34edf1 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -1656,20 +1656,95 @@ 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_cws,
> + int page, unsigned int max_bitflips)
> +{
> + struct nand_chip *chip = &host->chip;
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int cw, data_size, oob_size, ret = 0;
> +
> + if (!data_buf) {
> + data_buf = chip->data_buf;
> + chip->pagebuf = -1;
> + }
> +
> + if (!oob_buf) {
> + oob_buf = chip->oob_poi;
> + chip->pagebuf = -1;
> + }
> +
> + for (cw = 0; cw < ecc->steps && uncorrectable_cws; cw++) {

Last nitpick:

Could you have a look to bitmap.c and bitops.h and use a
for_each_set_bit() loop?

No need to resend all the patches, you can send a v5 just for this
patch, the others are fine for me.

Thanks,
Miquèl