[PATCH v3 1/5] mtd: nand: localize ECC failures per page

[PATCH v3 1/5] mtd: nand: localize ECC failures per page

[Brian Norris]

ECC failures can be tracked at the page level, not the do_read_ops level
(i.e., a potentially multi-page transaction).

This helps prepare for READ RETRY support.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
No changes since v1

 drivers/mtd/nand/nand_base.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 9b3bb3c519e9..e85b07f4293d 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1422,7 +1422,6 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 {
 	int chipnr, page, realpage, col, bytes, aligned, oob_required;
 	struct nand_chip *chip = mtd->priv;
-	struct mtd_ecc_stats stats;
 	int ret = 0;
 	uint32_t readlen = ops->len;
 	uint32_t oobreadlen = ops->ooblen;
@@ -1432,7 +1431,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	uint8_t *bufpoi, *oob, *buf;
 	unsigned int max_bitflips = 0;
 
-	stats = mtd->ecc_stats;
+	bool ecc_fail = false;
 
 	chipnr = (int)(from >> chip->chip_shift);
 	chip->select_chip(mtd, chipnr);
@@ -1447,6 +1446,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	oob_required = oob ? 1 : 0;
 
 	while (1) {
+		unsigned int ecc_failures = mtd->ecc_stats.failed;
 		bytes = min(mtd->writesize - col, readlen);
 		aligned = (bytes == mtd->writesize);
 
@@ -1483,7 +1483,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 			/* Transfer not aligned data */
 			if (!aligned) {
 				if (!NAND_HAS_SUBPAGE_READ(chip) && !oob &&
-				    !(mtd->ecc_stats.failed - stats.failed) &&
+				    !(mtd->ecc_stats.failed - ecc_failures) &&
 				    (ops->mode != MTD_OPS_RAW)) {
 					chip->pagebuf = realpage;
 					chip->pagebuf_bitflips = ret;
@@ -1513,6 +1513,9 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				else
 					nand_wait_ready(mtd);
 			}
+
+			if (mtd->ecc_stats.failed - ecc_failures)
+				ecc_fail = true;
 		} else {
 			memcpy(buf, chip->buffers->databuf + col, bytes);
 			buf += bytes;
@@ -1547,7 +1550,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	if (ret < 0)
 		return ret;
 
-	if (mtd->ecc_stats.failed - stats.failed)
+	if (ecc_fail)
 		return -EBADMSG;
 
 	return max_bitflips;
-- 
1.8.3.2

[PATCH v3 2/5] mtd: nand: add ONFI vendor block for Micron

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
No changes since v1

 include/linux/mtd/nand.h | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index f3ea8daf08ee..029fe5948dc4 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -285,7 +285,8 @@ struct nand_onfi_params {
 	u8 reserved4[7];
 
 	/* vendor */
-	u8 reserved5[90];
+	__le16 vendor_revision;
+	u8 vendor[88];
 
 	__le16 crc;
 } __attribute__((packed));
@@ -326,6 +327,26 @@ struct onfi_ext_param_page {
 	 */
 } __packed;
 
+struct nand_onfi_vendor_micron {
+	u8 two_plane_read;
+	u8 read_cache;
+	u8 read_unique_id;
+	u8 dq_imped;
+	u8 dq_imped_num_settings;
+	u8 dq_imped_feat_addr;
+	u8 rb_pulldown_strength;
+	u8 rb_pulldown_strength_feat_addr;
+	u8 rb_pulldown_strength_num_settings;
+	u8 otp_mode;
+	u8 otp_page_start;
+	u8 otp_data_prot_addr;
+	u8 otp_num_pages;
+	u8 otp_feat_addr;
+	u8 read_retry_options;
+	u8 reserved[72];
+	u8 param_revision;
+} __packed;
+
 /**
  * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
  * @lock:               protection lock
-- 
1.8.3.2

[PATCH v3 3/5] mtd: nand: add generic READ RETRY support

[Brian Norris]

Modern MLC (and even SLC?) NAND can experience a large number of
bitflips (beyond the recommended correctability capacity) due to drifts
in the voltage threshold (Vt). These bitflips can cause ECC errors to
occur well within the expected lifetime of the flash. To account for
this, some manufacturers provide a mechanism for shifting the Vt
threshold after a corrupted read.

The generic pattern seems to be that a particular flash has N read retry
modes (where N = 0, traditionally), and after an ECC failure, the host
should reconfigure the flash to use the next available mode, then retry
the read operation. This process repeats until all bitfips can be
corrected or until the host has tried all available retry modes.

This patch adds the infrastructure support for a
vendor-specific/flash-specific callback, used for setting the read-retry
mode (i.e., voltage threshold).

For now, this patch always returns the flash to mode 0 (the default
mode) after a successful read-retry, according to the flowchart found in
Micron's datasheets. This may need to change in the future if it is
determined that eventually, mode 0 is insufficient for the majority of
the flash cells (and so for performance reasons, we should leave the
flash in mode 1, 2, etc.).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v1 -> v2: fix a logic error when incrementing retry_mode, which caused -EINVAL
          failures on flash that didn't need READ RETRY

v2 -> v3: split out the generic callback support as a separate patch; adjust #
          of retry modes bounds check

 drivers/mtd/nand/nand_base.c | 56 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/mtd/nand.h     |  6 +++++
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index e85b07f4293d..d47c5bbca2b3 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1410,6 +1410,28 @@ static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob,
 }
 
 /**
+ * nand_set_read_retry - [INTERN] Set the READ RETRY mode
+ * @mtd: MTD device structure
+ * @retry_mode: the retry mode to use
+ *
+ * Some vendors supply a special command to shift the Vt threshold, to be used
+ * when there are too many bitflips in a page (i.e., ECC error). After setting
+ * a new threshold, the host should retry reading the page.
+ */
+static int nand_set_read_retry(struct mtd_info *mtd, int retry_mode)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	if (retry_mode >= chip->read_retries)
+		return -EINVAL;
+
+	if (!chip->set_read_retry)
+		return -EOPNOTSUPP;
+
+	return chip->set_read_retry(mtd, retry_mode);
+}
+
+/**
  * nand_do_read_ops - [INTERN] Read data with ECC
  * @mtd: MTD device structure
  * @from: offset to read from
@@ -1431,6 +1453,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	uint8_t *bufpoi, *oob, *buf;
 	unsigned int max_bitflips = 0;
 
+	int retry_mode = 0;
 	bool ecc_fail = false;
 
 	chipnr = (int)(from >> chip->chip_shift);
@@ -1494,8 +1517,6 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				memcpy(buf, chip->buffers->databuf + col, bytes);
 			}
 
-			buf += bytes;
-
 			if (unlikely(oob)) {
 				int toread = min(oobreadlen, max_oobsize);
 
@@ -1514,8 +1535,27 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 					nand_wait_ready(mtd);
 			}
 
-			if (mtd->ecc_stats.failed - ecc_failures)
-				ecc_fail = true;
+			if (mtd->ecc_stats.failed - ecc_failures) {
+				if (retry_mode + 1 <= chip->read_retries) {
+					retry_mode++;
+					pr_debug("ECC error; performing READ RETRY %d\n",
+							retry_mode);
+
+					ret = nand_set_read_retry(mtd,
+							retry_mode);
+					if (ret < 0)
+						break;
+
+					/* Reset failures */
+					mtd->ecc_stats.failed = ecc_failures;
+					continue;
+				} else {
+					/* No more retry modes; real failure */
+					ecc_fail = true;
+				}
+			}
+
+			buf += bytes;
 		} else {
 			memcpy(buf, chip->buffers->databuf + col, bytes);
 			buf += bytes;
@@ -1525,6 +1565,14 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 
 		readlen -= bytes;
 
+		/* Reset to retry mode 0 */
+		if (retry_mode) {
+			ret = nand_set_read_retry(mtd, 0);
+			if (ret < 0)
+				break;
+			retry_mode = 0;
+		}
+
 		if (!readlen)
 			break;
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 029fe5948dc4..ef70505dade1 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -472,6 +472,8 @@ struct nand_buffers {
  *			commands to the chip.
  * @waitfunc:		[REPLACEABLE] hardwarespecific function for wait on
  *			ready.
+ * @set_read_retry:	[FLASHSPECIFIC] flash (vendor) specific function for
+ *			setting the read-retry mode. Mostly needed for MLC NAND.
  * @ecc:		[BOARDSPECIFIC] ECC control structure
  * @buffers:		buffer structure for read/write
  * @hwcontrol:		platform-specific hardware control structure
@@ -518,6 +520,7 @@ struct nand_buffers {
  *			non 0 if ONFI supported.
  * @onfi_params:	[INTERN] holds the ONFI page parameter when ONFI is
  *			supported, 0 otherwise.
+ * @read_retries:	[INTERN] the number of read retry modes supported
  * @onfi_set_features:	[REPLACEABLE] set the features for ONFI nand
  * @onfi_get_features:	[REPLACEABLE] get the features for ONFI nand
  * @bbt:		[INTERN] bad block table pointer
@@ -565,6 +568,7 @@ struct nand_chip {
 			int feature_addr, uint8_t *subfeature_para);
 	int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
 			int feature_addr, uint8_t *subfeature_para);
+	int (*set_read_retry)(struct mtd_info *mtd, int retry_mode);
 
 	int chip_delay;
 	unsigned int options;
@@ -589,6 +593,8 @@ struct nand_chip {
 	int onfi_version;
 	struct nand_onfi_params	onfi_params;
 
+	int read_retries;
+
 	flstate_t state;
 
 	uint8_t *oob_poi;
-- 
1.8.3.2

[PATCH v3 4/5] mtd: nand: support Micron READ RETRY

[Brian Norris]

Micron provides READ RETRY support via the ONFI vendor-specific
parameter block (to indicate how many read-retry modes are available)
and the ONFI {GET,SET}_FEATURES commands with a vendor-specific feature
address (to support reading/switching the current read-retry mode).

The recommended sequence is as follows:

  1. Perform PAGE_READ operation
  2. If no ECC error, we are done
  3. Run SET_FEATURES with feature address 89h, mode 1
  4. Retry PAGE_READ operation
  5. If ECC error and there are remaining supported modes, increment the
     mode and return to step 3. Otherwise, this is a true ECC error.
  6. Run SET_FEATURES with feature address 89h, mode 0, to return to the
     default state.

This patch implements the chip->set_read_retry() callback for
Micron and fills in the chip->read_retries.

Tested on Micron MT29F32G08CBADA, which supports 8 read-retry modes.

The Micron vendor-specific table was checked against the datasheets for
the following Micron NAND:

Needs retry   Cell-type    Part number          Vendor revision    Byte 180
-----------   ---------    ----------------     ---------------    ------------
No            SLC          MT29F16G08ABABA      1                  Reserved (0)
No            MLC          MT29F32G08CBABA      1                  Reserved (0)
No            SLC          MT29F1G08AACWP       1                  0
Yes           MLC          MT29F32G08CBADA      1                  08h
Yes           MLC          MT29F64G08CBABA      2                  08h

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2 -> v3: - split out from the previous patch
	  - support Micron via replaceable vendor-specific/flash-specific
	    callback
	  - add vendor-revision check (I've only seen Micron with vendor
	    revision >= 1)

Note that I intentionally *haven't* accomodated a DMA-able buffer for the ONFI
set-features command, so GPMI NAND may have problems with this.

 drivers/mtd/nand/nand_base.c | 27 +++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d47c5bbca2b3..df20c43f186a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2979,6 +2979,30 @@ ext_out:
 	return ret;
 }
 
+static int nand_set_read_retry_micron(struct mtd_info *mtd, int retry_mode)
+{
+	struct nand_chip *chip = mtd->priv;
+	uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {retry_mode};
+
+	return chip->onfi_set_features(mtd, chip, ONFI_FEATURE_ADDR_READ_RETRY,
+			feature);
+}
+
+/*
+ * Configure chip properties from Micron vendor-specific ONFI table
+ */
+static void nand_onfi_detect_micron(struct nand_chip *chip,
+		struct nand_onfi_params *p)
+{
+	struct nand_onfi_vendor_micron *micron = (void *)p->vendor;
+
+	if (le16_to_cpu(p->vendor_revision) < 1)
+		return;
+
+	chip->read_retries = micron->read_retry_options;
+	chip->set_read_retry = nand_set_read_retry_micron;
+}
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -3085,6 +3109,9 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 		pr_warn("Could not retrieve ONFI ECC requirements\n");
 	}
 
+	if (p->jedec_id == NAND_MFR_MICRON)
+		nand_onfi_detect_micron(chip, p);
+
 	return 1;
 }
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index ef70505dade1..e3621aba1417 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -219,6 +219,9 @@ struct nand_chip;
 /* ONFI feature address */
 #define ONFI_FEATURE_ADDR_TIMING_MODE	0x1
 
+/* Vendor-specific feature address (Micron) */
+#define ONFI_FEATURE_ADDR_READ_RETRY	0x89
+
 /* ONFI subfeature parameters length */
 #define ONFI_SUBFEATURE_PARAM_LEN	4
 
-- 
1.8.3.2

[PATCH v3 5/5] mtd: nand: use __packed shorthand

[Brian Norris]

To be consistent with the rest of include/linux/mtd/nand.h, we should
use the __packed shorthand instead of __attribute__((packed)).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
No change since v1

 include/linux/mtd/nand.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index e3621aba1417..2413bac095ae 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -292,7 +292,7 @@ struct nand_onfi_params {
 	u8 vendor[88];
 
 	__le16 crc;
-} __attribute__((packed));
+} __packed;
 
 #define ONFI_CRC_BASE	0x4F4E
 
-- 
1.8.3.2

Re: [PATCH v3 4/5] mtd: nand: support Micron READ RETRY

[Huang Shijie]

On Fri, Jan 03, 2014 at 04:37:07PM -0800, Brian Norris wrote:
> 
> Note that I intentionally *haven't* accomodated a DMA-able buffer for the ONFI
> set-features command, so GPMI NAND may have problems with this.

I have fixed it in another patch set:
http://lists.infradead.org/pipermail/linux-mtd/2013-December/051085.html

http://lists.infradead.org/pipermail/linux-mtd/2013-December/051086.html

thanks
Huang Shijie

Re: [PATCH v3 2/5] mtd: nand: add ONFI vendor block for Micron

[Huang Shijie]

On Fri, Jan 03, 2014 at 04:37:05PM -0800, Brian Norris wrote:
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> No changes since v1
> 
>  include/linux/mtd/nand.h | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index f3ea8daf08ee..029fe5948dc4 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -285,7 +285,8 @@ struct nand_onfi_params {
>  	u8 reserved4[7];
>  
>  	/* vendor */
> -	u8 reserved5[90];
> +	__le16 vendor_revision;
> +	u8 vendor[88];
>  
>  	__le16 crc;
>  } __attribute__((packed));
> @@ -326,6 +327,26 @@ struct onfi_ext_param_page {
>  	 */
>  } __packed;
>  
> +struct nand_onfi_vendor_micron {
> +	u8 two_plane_read;
> +	u8 read_cache;
> +	u8 read_unique_id;
> +	u8 dq_imped;
> +	u8 dq_imped_num_settings;
> +	u8 dq_imped_feat_addr;
> +	u8 rb_pulldown_strength;
> +	u8 rb_pulldown_strength_feat_addr;
> +	u8 rb_pulldown_strength_num_settings;
> +	u8 otp_mode;
> +	u8 otp_page_start;
> +	u8 otp_data_prot_addr;
> +	u8 otp_num_pages;
> +	u8 otp_feat_addr;
> +	u8 read_retry_options;
> +	u8 reserved[72];
> +	u8 param_revision;
> +} __packed;
> +
>  /**
>   * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
>   * @lock:               protection lock
> -- 
> 1.8.3.2
> 
> 
> 
Acked-by: Huang Shijie <b32955@freescale.com>

Re: [PATCH v3 5/5] mtd: nand: use __packed shorthand

[Huang Shijie]

On Fri, Jan 03, 2014 at 04:37:08PM -0800, Brian Norris wrote:
> To be consistent with the rest of include/linux/mtd/nand.h, we should
> use the __packed shorthand instead of __attribute__((packed)).
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> No change since v1
> 
>  include/linux/mtd/nand.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index e3621aba1417..2413bac095ae 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -292,7 +292,7 @@ struct nand_onfi_params {
>  	u8 vendor[88];
>  
>  	__le16 crc;
> -} __attribute__((packed));
> +} __packed;
>  
>  #define ONFI_CRC_BASE	0x4F4E
>  
> -- 
> 1.8.3.2
> 
> 
> 
Acked-by: Huang Shijie <b32955@freescale.com>

Re: [PATCH v3 3/5] mtd: nand: add generic READ RETRY support

[Huang Shijie]

On Fri, Jan 03, 2014 at 04:37:06PM -0800, Brian Norris wrote:
> +/**
>   * nand_do_read_ops - [INTERN] Read data with ECC
>   * @mtd: MTD device structure
>   * @from: offset to read from
> @@ -1431,6 +1453,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  	uint8_t *bufpoi, *oob, *buf;
>  	unsigned int max_bitflips = 0;
>  
> +	int retry_mode = 0;
>  	bool ecc_fail = false;
>  
>  	chipnr = (int)(from >> chip->chip_shift);
> @@ -1494,8 +1517,6 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  				memcpy(buf, chip->buffers->databuf + col, bytes);
>  			}
>  
> -			buf += bytes;
> -
>  			if (unlikely(oob)) {
>  				int toread = min(oobreadlen, max_oobsize);
>  
> @@ -1514,8 +1535,27 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  					nand_wait_ready(mtd);
>  			}
>  
> -			if (mtd->ecc_stats.failed - ecc_failures)
> -				ecc_fail = true;
> +			if (mtd->ecc_stats.failed - ecc_failures) {
> +				if (retry_mode + 1 <= chip->read_retries) {
> +					retry_mode++;
> +					pr_debug("ECC error; performing READ RETRY %d\n",
> +							retry_mode);

you can move this pr_debug into the nand_set_read_retry().


> +
> +					ret = nand_set_read_retry(mtd,
> +							retry_mode);
> +					if (ret < 0)
> +						break;
> +
> +					/* Reset failures */
> +					mtd->ecc_stats.failed = ecc_failures;
> +					continue;
> +				} else {
> +					/* No more retry modes; real failure */
> +					ecc_fail = true;
> +				}
> +			}
> +
> +			buf += bytes;
>  		} else {
>  			memcpy(buf, chip->buffers->databuf + col, bytes);
>  			buf += bytes;
> @@ -1525,6 +1565,14 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  
>  		readlen -= bytes;
>  
> +		/* Reset to retry mode 0 */
> +		if (retry_mode) {
> +			ret = nand_set_read_retry(mtd, 0);
> +			if (ret < 0)
> +				break;
> +			retry_mode = 0;
> +		}
> +
>  		if (!readlen)
>  			break;
>  
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 029fe5948dc4..ef70505dade1 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -472,6 +472,8 @@ struct nand_buffers {
>   *			commands to the chip.
>   * @waitfunc:		[REPLACEABLE] hardwarespecific function for wait on
>   *			ready.
> + * @set_read_retry:	[FLASHSPECIFIC] flash (vendor) specific function for
> + *			setting the read-retry mode. Mostly needed for MLC NAND.

why not use the name "read_retry"?
i think it is more clear.

thanks
Huang Shijie

Re: [PATCH v3 4/5] mtd: nand: support Micron READ RETRY

[Huang Shijie]

On Fri, Jan 03, 2014 at 04:37:07PM -0800, Brian Norris wrote:
> +static int nand_set_read_retry_micron(struct mtd_info *mtd, int retry_mode)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {retry_mode};
> +
> +	return chip->onfi_set_features(mtd, chip, ONFI_FEATURE_ADDR_READ_RETRY,
> +			feature);
> +}
> +
> +/*
> + * Configure chip properties from Micron vendor-specific ONFI table
> + */
> +static void nand_onfi_detect_micron(struct nand_chip *chip,
> +		struct nand_onfi_params *p)
> +{
> +	struct nand_onfi_vendor_micron *micron = (void *)p->vendor;
> +
> +	if (le16_to_cpu(p->vendor_revision) < 1)
> +		return;
> +
> +	chip->read_retries = micron->read_retry_options;
> +	chip->set_read_retry = nand_set_read_retry_micron;
Besides the name, i am okay with this patch.

thanks
Huang Shijie

Re: [PATCH v3 1/5] mtd: nand: localize ECC failures per page

[Huang Shijie]

On Fri, Jan 03, 2014 at 04:37:04PM -0800, Brian Norris wrote:
> ECC failures can be tracked at the page level, not the do_read_ops level
> (i.e., a potentially multi-page transaction).
> 
> This helps prepare for READ RETRY support.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> No changes since v1
> 
>  drivers/mtd/nand/nand_base.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 9b3bb3c519e9..e85b07f4293d 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1422,7 +1422,6 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  {
>  	int chipnr, page, realpage, col, bytes, aligned, oob_required;
>  	struct nand_chip *chip = mtd->priv;
> -	struct mtd_ecc_stats stats;
>  	int ret = 0;
>  	uint32_t readlen = ops->len;
>  	uint32_t oobreadlen = ops->ooblen;
> @@ -1432,7 +1431,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  	uint8_t *bufpoi, *oob, *buf;
>  	unsigned int max_bitflips = 0;
>  
> -	stats = mtd->ecc_stats;
> +	bool ecc_fail = false;

Please do not put this line here, put it in the above lines.

>  
>  	chipnr = (int)(from >> chip->chip_shift);
>  	chip->select_chip(mtd, chipnr);
> @@ -1447,6 +1446,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  	oob_required = oob ? 1 : 0;
>  
>  	while (1) {
> +		unsigned int ecc_failures = mtd->ecc_stats.failed;
please add a white line here.


thanks
Huang Shijie

Re: [PATCH v3 3/5] mtd: nand: add generic READ RETRY support

[Huang Shijie]

On Fri, Jan 03, 2014 at 04:37:06PM -0800, Brian Norris wrote:
> Modern MLC (and even SLC?) NAND can experience a large number of
> bitflips (beyond the recommended correctability capacity) due to drifts
> in the voltage threshold (Vt). These bitflips can cause ECC errors to
> occur well within the expected lifetime of the flash. To account for
> this, some manufacturers provide a mechanism for shifting the Vt
> threshold after a corrupted read.
> 
> The generic pattern seems to be that a particular flash has N read retry
> modes (where N = 0, traditionally), and after an ECC failure, the host
> should reconfigure the flash to use the next available mode, then retry
> the read operation. This process repeats until all bitfips can be
> corrected or until the host has tried all available retry modes.
> 
> This patch adds the infrastructure support for a
> vendor-specific/flash-specific callback, used for setting the read-retry
> mode (i.e., voltage threshold).
> 
> For now, this patch always returns the flash to mode 0 (the default
> mode) after a successful read-retry, according to the flowchart found in
> Micron's datasheets. This may need to change in the future if it is
> determined that eventually, mode 0 is insufficient for the majority of
> the flash cells (and so for performance reasons, we should leave the
> flash in mode 1, 2, etc.).
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> v1 -> v2: fix a logic error when incrementing retry_mode, which caused -EINVAL
>           failures on flash that didn't need READ RETRY
> 
> v2 -> v3: split out the generic callback support as a separate patch; adjust #
>           of retry modes bounds check
> 
>  drivers/mtd/nand/nand_base.c | 56 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/mtd/nand.h     |  6 +++++
>  2 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index e85b07f4293d..d47c5bbca2b3 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1410,6 +1410,28 @@ static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob,
>  }
>  
>  /**
> + * nand_set_read_retry - [INTERN] Set the READ RETRY mode
> + * @mtd: MTD device structure
> + * @retry_mode: the retry mode to use
> + *
> + * Some vendors supply a special command to shift the Vt threshold, to be used
> + * when there are too many bitflips in a page (i.e., ECC error). After setting
> + * a new threshold, the host should retry reading the page.
> + */
> +static int nand_set_read_retry(struct mtd_info *mtd, int retry_mode)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +
> +	if (retry_mode >= chip->read_retries)
> +		return -EINVAL;
> +
> +	if (!chip->set_read_retry)
> +		return -EOPNOTSUPP;
> +
> +	return chip->set_read_retry(mtd, retry_mode);
> +}
> +
> +/**
>   * nand_do_read_ops - [INTERN] Read data with ECC
>   * @mtd: MTD device structure
>   * @from: offset to read from
> @@ -1431,6 +1453,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  	uint8_t *bufpoi, *oob, *buf;
>  	unsigned int max_bitflips = 0;
>  
> +	int retry_mode = 0;
>  	bool ecc_fail = false;
>  
>  	chipnr = (int)(from >> chip->chip_shift);
> @@ -1494,8 +1517,6 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  				memcpy(buf, chip->buffers->databuf + col, bytes);
>  			}
>  
> -			buf += bytes;
> -
>  			if (unlikely(oob)) {
>  				int toread = min(oobreadlen, max_oobsize);
>  
> @@ -1514,8 +1535,27 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  					nand_wait_ready(mtd);
>  			}
>  
> -			if (mtd->ecc_stats.failed - ecc_failures)
> -				ecc_fail = true;
> +			if (mtd->ecc_stats.failed - ecc_failures) {
> +				if (retry_mode + 1 <= chip->read_retries) {
> +					retry_mode++;
> +					pr_debug("ECC error; performing READ RETRY %d\n",
> +							retry_mode);
> +
> +					ret = nand_set_read_retry(mtd,
> +							retry_mode);
> +					if (ret < 0)
> +						break;
> +
> +					/* Reset failures */
> +					mtd->ecc_stats.failed = ecc_failures;
> +					continue;
IMHO, use a "goto" here makes it more readable.
and the "goto" makes the code runs faster.

such as:

-------------------------------------------------------------------------

+read_retry:
			chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);

			/*
			 * Now read the page into the buffer.  Absent an error,
			 * the read methods return max bitflips per ecc step.
			 */
			if (unlikely(ops->mode == MTD_OPS_RAW))
				ret = chip->ecc.read_page_raw(mtd, chip, bufpoi,
							      oob_required,
							      page);
			else if (!aligned && NAND_HAS_SUBPAGE_READ(chip) &&
				 !oob)
				ret = chip->ecc.read_subpage(mtd, chip,
							col, bytes, bufpoi,
							page);
			else
				ret = chip->ecc.read_page(mtd, chip, bufpoi,
							  oob_required, page);
                        ....................
+			if (mtd->ecc_stats.failed - ecc_failures) {
+				if (retry_mode + 1 <= chip->read_retries) {
+					retry_mode++;
+					pr_debug("ECC error; performing READ RETRY %d\n",
+							retry_mode);
+
+					ret = nand_set_read_retry(mtd,
+							retry_mode);
+					if (ret < 0)
+						break;
+
+					/* Reset failures */
+					mtd->ecc_stats.failed = ecc_failures;
+					goto read_retry;

--------------------------------------------------------------------


thanks
Huang Shijie

Re: [PATCH v3 3/5] mtd: nand: add generic READ RETRY support

[Huang Shijie]

On Mon, Jan 13, 2014 at 12:04:00AM -0800, Brian Norris wrote:
> 
> It's not actually performing any "read" or a "retry"; the function is
> just configuring the device for a retry. So I don't think read_retry is
> correct. For my name, I was abbreviating "set read retry mode". How do
> you like "setup_read_retry" or "set_read_retry_mode"?
I prefer to the setup_read_retry :)

thanks
Huang Shijie

Re: [PATCH v3 3/5] mtd: nand: add generic READ RETRY support

[Brian Norris]

On Tue, Jan 07, 2014 at 02:17:34PM +0800, Huang Shijie wrote:
> On Fri, Jan 03, 2014 at 04:37:06PM -0800, Brian Norris wrote:
> > @@ -1514,8 +1535,27 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
> >  					nand_wait_ready(mtd);
> >  			}
> >  
> > -			if (mtd->ecc_stats.failed - ecc_failures)
> > -				ecc_fail = true;
> > +			if (mtd->ecc_stats.failed - ecc_failures) {
> > +				if (retry_mode + 1 <= chip->read_retries) {
> > +					retry_mode++;
> > +					pr_debug("ECC error; performing READ RETRY %d\n",
> > +							retry_mode);
> 
> you can move this pr_debug into the nand_set_read_retry().

OK.

> > +
> > +					ret = nand_set_read_retry(mtd,
> > +							retry_mode);
> > +					if (ret < 0)
> > +						break;
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 029fe5948dc4..ef70505dade1 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -472,6 +472,8 @@ struct nand_buffers {
> >   *			commands to the chip.
> >   * @waitfunc:		[REPLACEABLE] hardwarespecific function for wait on
> >   *			ready.
> > + * @set_read_retry:	[FLASHSPECIFIC] flash (vendor) specific function for
> > + *			setting the read-retry mode. Mostly needed for MLC NAND.
> 
> why not use the name "read_retry"?
> i think it is more clear.

It's not actually performing any "read" or a "retry"; the function is
just configuring the device for a retry. So I don't think read_retry is
correct. For my name, I was abbreviating "set read retry mode". How do
you like "setup_read_retry" or "set_read_retry_mode"?

Brian