[PATCH v5 1/5] mtd: nand: Detect Micron flash with on-die ECC (aka "internal ECC") enabled.

[PATCH v5 1/5] mtd: nand: Detect Micron flash with on-die ECC (aka "internal ECC") enabled.

[David Mosberger]

Signed-off-by: David Mosberger <davidm@egauge.net>
---
 drivers/mtd/nand/nand_base.c |   27 ++++++++++++++++++++++++---
 include/linux/mtd/nand.h     |    4 ++++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 50a6e07..7d1ec81 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3058,19 +3058,40 @@ static int nand_setup_read_retry_micron(struct mtd_info *mtd, int retry_mode)
 			feature);
 }
 
+static void nand_onfi_detect_micron_on_die_ecc(struct mtd_info *mtd)
+{
+	u8 features[ONFI_SUBFEATURE_PARAM_LEN];
+	struct nand_chip *chip = mtd->priv;
+
+	if (chip->onfi_get_features(mtd, chip, ONFI_FEATURE_ADDR_ARRAY_OP_MODE,
+				    features) < 0)
+		return;
+
+	if (features[0] & ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC) {
+		/*
+		 * If the chip has on-die ECC enabled, we kind of have
+		 * to do the same...
+		 */
+		pr_info("Using on-die ECC\n");
+	}
+}
+
 /*
  * Configure chip properties from Micron vendor-specific ONFI table
  */
-static void nand_onfi_detect_micron(struct nand_chip *chip,
-		struct nand_onfi_params *p)
+static void nand_onfi_detect_micron(struct mtd_info *mtd,
+				    struct nand_onfi_params *p)
 {
 	struct nand_onfi_vendor_micron *micron = (void *)p->vendor;
+	struct nand_chip *chip = mtd->priv;
 
 	if (le16_to_cpu(p->vendor_revision) < 1)
 		return;
 
 	chip->read_retries = micron->read_retry_options;
 	chip->setup_read_retry = nand_setup_read_retry_micron;
+
+	nand_onfi_detect_micron_on_die_ecc(mtd);
 }
 
 /*
@@ -3172,7 +3193,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	}
 
 	if (p->jedec_id == NAND_MFR_MICRON)
-		nand_onfi_detect_micron(chip, p);
+		nand_onfi_detect_micron(mtd, p);
 
 	return 1;
 }
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index cd38bba..27b01cc 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -225,6 +225,10 @@ struct nand_chip;
 /* Vendor-specific feature address (Micron) */
 #define ONFI_FEATURE_ADDR_READ_RETRY	0x89
 
+/* Vendor-specific array operation mode (Micron) */
+#define ONFI_FEATURE_ADDR_ARRAY_OP_MODE	0x90
+#define ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC		0x08
+
 /* ONFI subfeature parameters length */
 #define ONFI_SUBFEATURE_PARAM_LEN	4
 
-- 
1.7.9.5

[PATCH v5 2/5] mtd: nand: Add NAND_ECC_HW_ON_DIE ECC-mode.

[David Mosberger]

Some Micron chips such as the MT29F4G16ABADAWP have an internal
(on-die) ECC-controller that is useful when the host-platform doesn't
have hardware-support for multi-bit ECC.  This patch adds support for
such chips.  The patch is safe to apply since (a) it only detects that
on-die ECC is in use (it doesn't enable it on its own accord) and (b)
if an older kernel had been booted on such a system, there would have
been two conflicting ECC modes in use, which would have caused ECC/oob
corruption.

This patch has been tested with Micron MT29F4G16ABADAWP and should
work for any other Micron-chip with the same ECC layout, including at
least MT29F4G08AB{A,B}DA{H4,WP,HC}, MT29F4G16AB{A,B}DA{H4,WP,HC},
MT29F8G{08,16}AD{A,B}DAH4, and MT29F16G08AJADAWP.

Signed-off-by: David Mosberger <davidm@egauge.net>
---
 drivers/mtd/nand/nand_base.c |   88 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |    2 +
 2 files changed, 90 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7d1ec81..997d7f1 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -78,6 +78,20 @@ static struct nand_ecclayout nand_oob_64 = {
 		 .length = 38} }
 };
 
+static struct nand_ecclayout nand_oob_64_on_die = {
+	.eccbytes = 32,
+	.eccpos = {
+		    8,  9, 10, 11, 12, 13, 14, 15,
+		   24, 25, 26, 27, 28, 29, 30, 31,
+		   40, 41, 42, 43, 44, 45, 46, 47,
+		   56, 57, 58, 59, 60, 61, 62, 63},
+	.oobfree = {
+		{.offset =  4,	 .length = 4},
+		{.offset = 20,	 .length = 4},
+		{.offset = 36,	 .length = 4},
+		{.offset = 52,	 .length = 4}}
+};
+
 static struct nand_ecclayout nand_oob_128 = {
 	.eccbytes = 48,
 	.eccpos = {
@@ -1262,6 +1276,59 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	return max_bitflips;
 }
 
+static int check_read_status_on_die(struct mtd_info *mtd, int page)
+{
+	struct nand_chip *chip = mtd->priv;
+	int max_bitflips = 0;
+	uint8_t status;
+
+	/* Check ECC status of page just transferred into NAND's page buffer: */
+	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
+	status = chip->read_byte(mtd);
+
+	/* Switch back to data reading: */
+	chip->cmdfunc(mtd, NAND_CMD_READMODE, -1, -1);
+
+	if (status & NAND_STATUS_FAIL) {
+		/* Page has invalid ECC. */
+		mtd->ecc_stats.failed++;
+	} else if (status & NAND_STATUS_REWRITE) {
+		/*
+		 * Simple but suboptimal: any page with a single stuck
+		 * bit will be unusable since it'll be rewritten on
+		 * each read...
+		 */
+		max_bitflips = mtd->bitflip_threshold;
+	}
+	return max_bitflips;
+}
+
+/**
+ * nand_read_page_on_die - [INTERN] read raw page data without ecc
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @buf: buffer to store read data
+ * @oob_required: caller requires OOB data read to chip->oob_poi
+ * @page: page number to read
+ */
+static int nand_read_page_on_die(struct mtd_info *mtd, struct nand_chip *chip,
+				 uint8_t *buf, int oob_required, int page)
+{
+	uint32_t failed;
+	int ret;
+
+	failed = mtd->ecc_stats.failed;
+
+	ret = check_read_status_on_die(mtd, page);
+	if (ret < 0 || mtd->ecc_stats.failed != failed)
+		return ret;
+
+	chip->read_buf(mtd, buf, mtd->writesize);
+	if (oob_required)
+		chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+	return ret;
+}
+
 /**
  * nand_read_page_hwecc - [REPLACEABLE] hardware ECC based page read function
  * @mtd: mtd info structure
@@ -3072,6 +3139,7 @@ static void nand_onfi_detect_micron_on_die_ecc(struct mtd_info *mtd)
 		 * If the chip has on-die ECC enabled, we kind of have
 		 * to do the same...
 		 */
+		chip->ecc.mode = NAND_ECC_HW_ON_DIE;
 		pr_info("Using on-die ECC\n");
 	}
 }
@@ -3989,6 +4057,26 @@ int nand_scan_tail(struct mtd_info *mtd)
 		ecc->strength = ecc->bytes * 8 / fls(8 * ecc->size);
 		break;
 
+	case NAND_ECC_HW_ON_DIE:
+		/*
+		 * nand_bbt.c by default puts the BBT marker at
+		 * offset 8 in OOB, which is used for ECC (see
+		 * nand_oob_64_on_die).
+		 * Fixed by not using OOB for BBT marker.
+		 */
+		chip->bbt_options |= NAND_BBT_NO_OOB;
+		chip->ecc.layout = &nand_oob_64_on_die;
+		chip->ecc.read_page = nand_read_page_on_die;
+		chip->ecc.write_page = nand_write_page_raw;
+		chip->ecc.read_oob = nand_read_oob_std;
+		chip->ecc.read_page_raw = nand_read_page_raw;
+		chip->ecc.write_page_raw = nand_write_page_raw;
+		chip->ecc.write_oob = nand_write_oob_std;
+		chip->ecc.size = 512;
+		chip->ecc.bytes = 8;
+		chip->ecc.strength = 4;
+		break;
+
 	case NAND_ECC_NONE:
 		pr_warn("NAND_ECC_NONE selected by board driver. "
 			   "This is not recommended!\n");
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 27b01cc..d09f0a0 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -112,6 +112,7 @@ extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 /* Status bits */
 #define NAND_STATUS_FAIL	0x01
 #define NAND_STATUS_FAIL_N1	0x02
+#define NAND_STATUS_REWRITE	0x08
 #define NAND_STATUS_TRUE_READY	0x20
 #define NAND_STATUS_READY	0x40
 #define NAND_STATUS_WP		0x80
@@ -126,6 +127,7 @@ typedef enum {
 	NAND_ECC_HW_SYNDROME,
 	NAND_ECC_HW_OOB_FIRST,
 	NAND_ECC_SOFT_BCH,
+	NAND_ECC_HW_ON_DIE,
 } nand_ecc_modes_t;
 
 /*
-- 
1.7.9.5

[PATCH v5 3/5] mtd: nand: Enable subpage-reads on flashes with on-die ECC enabled.

[David Mosberger]

The Micron chips supported through the on-die ECC all have subpage capability,
so enable that.

Signed-off-by: David Mosberger <davidm@egauge.net>
---
 drivers/mtd/nand/nand_base.c |   56 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 997d7f1..7092875 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1304,6 +1304,52 @@ static int check_read_status_on_die(struct mtd_info *mtd, int page)
 }
 
 /**
+ * nand_read_subpage_on_die - [REPLACEABLE] raw sub-page read function
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @data_offs: offset of requested data within the page
+ * @readlen: data length
+ * @bufpoi: buffer to store read data
+ */
+static int nand_read_subpage_on_die(struct mtd_info *mtd,
+				    struct nand_chip *chip,
+				    uint32_t data_offs, uint32_t readlen,
+				    uint8_t *bufpoi, int page)
+{
+	int start_step, end_step, num_steps, ret;
+	int data_col_addr;
+	int datafrag_len;
+	uint32_t failed;
+	uint8_t *p;
+
+	/* Column address within the page aligned to ECC size */
+	start_step = data_offs / chip->ecc.size;
+	end_step = (data_offs + readlen - 1) / chip->ecc.size;
+	num_steps = end_step - start_step + 1;
+
+	/* Data size aligned to ECC ecc.size */
+	datafrag_len = num_steps * chip->ecc.size;
+	data_col_addr = start_step * chip->ecc.size;
+	p = bufpoi + data_col_addr;
+
+	failed = mtd->ecc_stats.failed;
+
+	ret = check_read_status_on_die(mtd, page);
+	if (ret < 0 || mtd->ecc_stats.failed != failed) {
+		memset(p, 0, datafrag_len);
+		return ret;
+	}
+
+	/* If we read not a page aligned data */
+	if (data_col_addr != 0)
+		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_col_addr, -1);
+
+	chip->read_buf(mtd, p, datafrag_len);
+
+	return ret;
+}
+
+/**
  * nand_read_page_on_die - [INTERN] read raw page data without ecc
  * @mtd: mtd info structure
  * @chip: nand chip info structure
@@ -4067,6 +4113,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		chip->bbt_options |= NAND_BBT_NO_OOB;
 		chip->ecc.layout = &nand_oob_64_on_die;
 		chip->ecc.read_page = nand_read_page_on_die;
+		chip->ecc.read_subpage = nand_read_subpage_on_die;
 		chip->ecc.write_page = nand_write_page_raw;
 		chip->ecc.read_oob = nand_read_oob_std;
 		chip->ecc.read_page_raw = nand_read_page_raw;
@@ -4144,8 +4191,13 @@ int nand_scan_tail(struct mtd_info *mtd)
 	/* Invalidate the pagebuffer reference */
 	chip->pagebuf = -1;
 
-	/* Large page NAND with SOFT_ECC should support subpage reads */
-	if ((ecc->mode == NAND_ECC_SOFT) && (chip->page_shift > 9))
+	/*
+	 * Large page NAND with SOFT_ECC or on-die ECC should support
+	 * subpage reads.
+	 */
+	if (((ecc->mode == NAND_ECC_SOFT)
+	     || (chip->ecc.mode == NAND_ECC_HW_ON_DIE))
+	    && (chip->page_shift > 9))
 		chip->options |= NAND_SUBPAGE_READ;
 
 	/* Fill in remaining MTD driver data */
-- 
1.7.9.5

[PATCH v5 4/5] mtd: nand: Allocate extra buffers needed for on-die ECC controller.

[David Mosberger]

To avoid unnecessary rewrites, it is necessary to count the number of
actual bitflips that occurred when the NAND_STATUS_REWRITE bit is set.
This patch introduces the extra buffers needed to implement that
counting.  The actual counting is in the next patch.

Signed-off-by: David Mosberger <davidm@egauge.net>
---
 drivers/mtd/nand/nand_base.c |   13 ++++++++++++-
 include/linux/mtd/nand.h     |    2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7092875..84409db 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3939,13 +3939,24 @@ int nand_scan_tail(struct mtd_info *mtd)
 			!(chip->bbt_options & NAND_BBT_USE_FLASH));
 
 	if (!(chip->options & NAND_OWN_BUFFERS)) {
+		size_t on_die_bufsz = 0;
+
+		if (chip->ecc.mode == NAND_ECC_HW_ON_DIE)
+			on_die_bufsz = 2*(mtd->writesize + mtd->oobsize);
+
 		nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
-				+ mtd->oobsize * 3, GFP_KERNEL);
+				+ mtd->oobsize * 3 + on_die_bufsz, GFP_KERNEL);
 		if (!nbuf)
 			return -ENOMEM;
 		nbuf->ecccalc = (uint8_t *)(nbuf + 1);
 		nbuf->ecccode = nbuf->ecccalc + mtd->oobsize;
 		nbuf->databuf = nbuf->ecccode + mtd->oobsize;
+		if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) {
+			nbuf->chkbuf = (nbuf->databuf + mtd->writesize
+					+ mtd->oobsize);
+			nbuf->rawbuf = (nbuf->chkbuf + mtd->writesize
+					+ mtd->oobsize);
+		}
 
 		chip->buffers = nbuf;
 	} else {
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index d09f0a0..7725bbc 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -533,6 +533,8 @@ struct nand_buffers {
 	uint8_t	*ecccalc;
 	uint8_t	*ecccode;
 	uint8_t *databuf;
+	uint8_t *chkbuf;
+	uint8_t *rawbuf;
 };
 
 /**
-- 
1.7.9.5

Re: [PATCH v5 0/5] mtd: nand: Add on-die ECC support

[Gerhard Sittig]

On Mon, 2014-04-14 at 13:35 -0600, David Mosberger wrote:
> 
> This series adds support for Micron on-die ECC.
> The patches are designed to go on top of Gerhard Sittig's patches
> to introduce a READMODE command.
> 
> Changes in v5:
>  - Fixed some formatting issues.
>  - Wherever possible, new functions take only mtd argument (not also chip).
>  - Eliminated bitdiff() function.
>  - Clarified/shortened some comments.

nit: please supply the change log for past revisions as well if
you resubmit, reviewers should not have to do the book keeping or
archeology for you if they want to know whether some aspect has
been addressed that was raised before

some readers prefer or even demand that individual patches have
their change log, because cover letters are optional, and most of
all cover letters won't end up in trackers while patches do


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH v5 1/5] mtd: nand: Detect Micron flash with on-die ECC (aka "internal ECC") enabled.

[Gerhard Sittig]

On Mon, 2014-04-14 at 13:35 -0600, David Mosberger wrote:
> 
> Signed-off-by: David Mosberger <davidm@egauge.net>

commit message is missing

> ---
>  drivers/mtd/nand/nand_base.c |   27 ++++++++++++++++++++++++---
>  include/linux/mtd/nand.h     |    4 ++++
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 50a6e07..7d1ec81 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3058,19 +3058,40 @@ static int nand_setup_read_retry_micron(struct mtd_info *mtd, int retry_mode)
>  			feature);
>  }
>  
> +static void nand_onfi_detect_micron_on_die_ecc(struct mtd_info *mtd)
> +{
> +	u8 features[ONFI_SUBFEATURE_PARAM_LEN];
> +	struct nand_chip *chip = mtd->priv;
> +
> +	if (chip->onfi_get_features(mtd, chip, ONFI_FEATURE_ADDR_ARRAY_OP_MODE,
> +				    features) < 0)
> +		return;
> +
> +	if (features[0] & ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC) {
> +		/*
> +		 * If the chip has on-die ECC enabled, we kind of have
> +		 * to do the same...
> +		 */
> +		pr_info("Using on-die ECC\n");
> +	}

the comment and the code don't match -- at this point it's mere
detection, no action is taken nor is the detection result stored
nor communicated

> +}
> +
>  /*
>   * Configure chip properties from Micron vendor-specific ONFI table
>   */


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH v5 2/5] mtd: nand: Add NAND_ECC_HW_ON_DIE ECC-mode.

[Gerhard Sittig]

On Mon, 2014-04-14 at 13:35 -0600, David Mosberger wrote:
> 
> @@ -1262,6 +1276,59 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
>  	return max_bitflips;
>  }
>  
> +static int check_read_status_on_die(struct mtd_info *mtd, int page)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	int max_bitflips = 0;
> +	uint8_t status;
> +
> +	/* Check ECC status of page just transferred into NAND's page buffer: */
> +	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> +	status = chip->read_byte(mtd);
> +
> +	/* Switch back to data reading: */
> +	chip->cmdfunc(mtd, NAND_CMD_READMODE, -1, -1);
> +
> +	if (status & NAND_STATUS_FAIL) {
> +		/* Page has invalid ECC. */
> +		mtd->ecc_stats.failed++;
> +	} else if (status & NAND_STATUS_REWRITE) {
> +		/*
> +		 * Simple but suboptimal: any page with a single stuck
> +		 * bit will be unusable since it'll be rewritten on
> +		 * each read...
> +		 */
> +		max_bitflips = mtd->bitflip_threshold;
> +	}

should the comment mention that a simple assumption is encoded
that the maximum number of detectable errors has occured if the
chip flags that _some_ error was detected? while this pessimistic
assumption results in suboptimal behaviour for specific error
situations like stuck bits

somehow I feel that the comment "has a gap" which the reader
needs to fill in, but this might just be me

> +	return max_bitflips;
> +}
> +
> +/**
> + * nand_read_page_on_die - [INTERN] read raw page data without ecc
> + * @mtd: mtd info structure
> + * @chip: nand chip info structure
> + * @buf: buffer to store read data
> + * @oob_required: caller requires OOB data read to chip->oob_poi
> + * @page: page number to read
> + */
> +static int nand_read_page_on_die(struct mtd_info *mtd, struct nand_chip *chip,
> +				 uint8_t *buf, int oob_required, int page)
> +{
> +	uint32_t failed;
> +	int ret;
> +
> +	failed = mtd->ecc_stats.failed;
> +
> +	ret = check_read_status_on_die(mtd, page);
> +	if (ret < 0 || mtd->ecc_stats.failed != failed)
> +		return ret;

the empty line suggests that storing the previous value and
checking for changes after calling a routine which potentially
does updates are two separate actions, while it's actually all
the same logical group -- I'd suggest to remove this empty line
(here and in other locations which follow this pattern)

> +
> +	chip->read_buf(mtd, buf, mtd->writesize);
> +	if (oob_required)
> +		chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +	return ret;
> +}
> +
>  /**
>   * nand_read_page_hwecc - [REPLACEABLE] hardware ECC based page read function
>   * @mtd: mtd info structure


> @@ -3072,6 +3139,7 @@ static void nand_onfi_detect_micron_on_die_ecc(struct mtd_info *mtd)
>  		 * If the chip has on-die ECC enabled, we kind of have
>  		 * to do the same...
>  		 */
> +		chip->ecc.mode = NAND_ECC_HW_ON_DIE;
>  		pr_info("Using on-die ECC\n");
>  	}
>  }

this is the place where the comment "becomes correct", because
the code follows the chip's status; please make sure that each
patch is consistent in itself, regardless of whether they are
submitted in one sequence

still a short discussion could be helpful that this is an obvious
yet simple approach, assuming that the user _wants_ to blindly
follow the chip's state; alternative approaches would be to
apply user supplied settings, or to only accept on-die-ECC if
nothing better is available by other means (you could suggest
these as future options, not that they need to get implemented
right now)

> @@ -3989,6 +4057,26 @@ int nand_scan_tail(struct mtd_info *mtd)
>  		ecc->strength = ecc->bytes * 8 / fls(8 * ecc->size);
>  		break;
>  
> +	case NAND_ECC_HW_ON_DIE:
> +		/*
> +		 * nand_bbt.c by default puts the BBT marker at
> +		 * offset 8 in OOB, which is used for ECC (see
> +		 * nand_oob_64_on_die).
> +		 * Fixed by not using OOB for BBT marker.
> +		 */
> +		chip->bbt_options |= NAND_BBT_NO_OOB;
> +		chip->ecc.layout = &nand_oob_64_on_die;
> +		chip->ecc.read_page = nand_read_page_on_die;
> +		chip->ecc.write_page = nand_write_page_raw;
> +		chip->ecc.read_oob = nand_read_oob_std;
> +		chip->ecc.read_page_raw = nand_read_page_raw;
> +		chip->ecc.write_page_raw = nand_write_page_raw;
> +		chip->ecc.write_oob = nand_write_oob_std;
> +		chip->ecc.size = 512;
> +		chip->ecc.bytes = 8;
> +		chip->ecc.strength = 4;
> +		break;
> +
>  	case NAND_ECC_NONE:
>  		pr_warn("NAND_ECC_NONE selected by board driver. "
>  			   "This is not recommended!\n");

former feedback suggested that the OOB layout might depend on the
specific chip, the ECC size/bytes/strength parameters might
depend on chips as well -- can these get looked up in or derived
from other identification data that is available from the chip?

this implementation assumes specific settings for the initial
chip that motivated development of the feature, at least a big
red warning might be due


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH v5 3/5] mtd: nand: Enable subpage-reads on flashes with on-die ECC enabled.

[Gerhard Sittig]

On Mon, 2014-04-14 at 13:35 -0600, David Mosberger wrote:
> 
> The Micron chips supported through the on-die ECC all have subpage capability,
> so enable that.
> 
> [ ... ]
> @@ -4144,8 +4191,13 @@ int nand_scan_tail(struct mtd_info *mtd)
>  	/* Invalidate the pagebuffer reference */
>  	chip->pagebuf = -1;
>  
> -	/* Large page NAND with SOFT_ECC should support subpage reads */
> -	if ((ecc->mode == NAND_ECC_SOFT) && (chip->page_shift > 9))
> +	/*
> +	 * Large page NAND with SOFT_ECC or on-die ECC should support
> +	 * subpage reads.
> +	 */
> +	if (((ecc->mode == NAND_ECC_SOFT)
> +	     || (chip->ecc.mode == NAND_ECC_HW_ON_DIE))
> +	    && (chip->page_shift > 9))
>  		chip->options |= NAND_SUBPAGE_READ;
>  
>  	/* Fill in remaining MTD driver data */

the commit message suggests that it's an assumption for current
Micron chips, the code implements an unconditional subpage
enabling for any chip that happens to have on-die-ECC -- is there
some ID data that more reliably reflects whether support for
subpages exists?


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH v5 4/5] mtd: nand: Allocate extra buffers needed for on-die ECC controller.

[Gerhard Sittig]

On Mon, 2014-04-14 at 13:35 -0600, David Mosberger wrote:
> 
> To avoid unnecessary rewrites, it is necessary to count the number of
> actual bitflips that occurred when the NAND_STATUS_REWRITE bit is set.
> This patch introduces the extra buffers needed to implement that
> counting.  The actual counting is in the next patch.

I'd suggest to s/the next patch/a subsequent patch/ -- the order
of commits in the mainline repo need not strictly follow
arbitrary submission sequences

> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3939,13 +3939,24 @@ int nand_scan_tail(struct mtd_info *mtd)
>  			!(chip->bbt_options & NAND_BBT_USE_FLASH));
>  
>  	if (!(chip->options & NAND_OWN_BUFFERS)) {
> +		size_t on_die_bufsz = 0;
> +
> +		if (chip->ecc.mode == NAND_ECC_HW_ON_DIE)
> +			on_die_bufsz = 2*(mtd->writesize + mtd->oobsize);
> +
>  		nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
> -				+ mtd->oobsize * 3, GFP_KERNEL);
> +				+ mtd->oobsize * 3 + on_die_bufsz, GFP_KERNEL);
>  		if (!nbuf)
>  			return -ENOMEM;
>  		nbuf->ecccalc = (uint8_t *)(nbuf + 1);
>  		nbuf->ecccode = nbuf->ecccalc + mtd->oobsize;
>  		nbuf->databuf = nbuf->ecccode + mtd->oobsize;
> +		if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) {
> +			nbuf->chkbuf = (nbuf->databuf + mtd->writesize
> +					+ mtd->oobsize);
> +			nbuf->rawbuf = (nbuf->chkbuf + mtd->writesize
> +					+ mtd->oobsize);
> +		}
>  
>  		chip->buffers = nbuf;
>  	} else {

you introduce the 'on_die_bufsz' variable, can you use it (under
a more appropriate name then) to first determine the required
amount of memory and then have a kzalloc() call with a size spec
that is not as complex as the current implementation?

independent from this optional style improvement, you should
fixup the whitespace issues around operators, and remove the
unnecessary parentheses; having the "writesize + oobsize" in
another variable might help eliminate more duplication and trim
line length, and better reflect how the size calculation and the
pointer calculation match each other


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH v5 5/5] mtd: nand: Improve bitflip detection for on-die ECC scheme.

[Gerhard Sittig]

On Mon, 2014-04-14 at 13:35 -0600, David Mosberger wrote:
> 
> @@ -1276,6 +1276,64 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
>  	return max_bitflips;
>  }
>  
> +static int set_on_die_ecc(struct mtd_info *mtd, int on)
> +{
> +	u8 data[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
> +	struct nand_chip *chip = mtd->priv;
> +
> +	if (on)
> +		data[0] = ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC;
> +
> +	return chip->onfi_set_features(mtd, chip,
> +				       ONFI_FEATURE_ADDR_ARRAY_OP_MODE, data);
> +}

I'd suggest to extend this, and check whether this operation
actually did enable the wanted mode (the ONFI set feature request
is a prerequisite but need not be sufficient, you might want to
re-get the feature and test for success)

> +
> +static int check_for_bitflips(struct mtd_info *mtd, int page)
> +{
> +	int flips = 0, max_bitflips = 0, i, j, read_size;

it's been elsewhere too, but here it really bothers me that the
"initial" value and subsequent access to variables is spread so
far across the code -- can you put seeding and updating those
variables closer together, such that their logical correspondence
is better reflected in the implementation?  this would save
mental resources on the reader's / maintainer's side and free
them for more demanding tasks

> +	uint8_t *chkbuf, *rawbuf, *chkoob, *rawoob;
> +	struct nand_chip *chip = mtd->priv;
> +	uint32_t *eccpos;
> +
> +	chkbuf = chip->buffers->chkbuf;
> +	rawbuf = chip->buffers->rawbuf;
> +	read_size = mtd->writesize + mtd->oobsize;
> +
> +	/* Read entire page w/OOB area with on-die ECC on: */
> +	chip->read_buf(mtd, chkbuf, read_size);

has this very variant (potentially fixed up data) not already
been read in an immediately preceeding step in the logic?  would
an explicit comment about the context or preconditions of this
routine help?

> +
> +	/* Re-read page with on-die ECC off: */
> +	set_on_die_ecc(mtd, 0);
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> +	chip->read_buf(mtd, rawbuf, read_size);
> +	set_on_die_ecc(mtd, 1);

these calls ignore the return code of a routine which may fail

> +
> +	chkoob = chkbuf + mtd->writesize;
> +	rawoob = rawbuf + mtd->writesize;
> +	eccpos = chip->ecc.layout->eccpos;
> +	for (i = 0; i < chip->ecc.steps; ++i) {
> +		/* Count bit flips in the actual data area: */
> +		flips = 0;
> +		for (j = 0; j < chip->ecc.size; ++j)
> +			flips += hweight8(chkbuf[j] ^ rawbuf[j]);
> +		/* Count bit flips in the ECC bytes: */
> +		for (j = 0; j < chip->ecc.bytes; ++j) {
> +			flips += hweight8(chkoob[*eccpos] ^ rawoob[*eccpos]);
> +			++eccpos;
> +		}
> +		if (flips > 0)
> +			mtd->ecc_stats.corrected += flips;
> +		max_bitflips = max_t(int, max_bitflips, flips);
> +		chkbuf += chip->ecc.size;
> +		rawbuf += chip->ecc.size;
> +	}
> +
> +	/* Re-issue the READ command for the actual data read that follows.  */
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);

former review feedback suggested looking into whether the
on-die-ECC support could better match what happens for the other
ECC methods:  reading array data first, checking and potentially
fixing up ECC afterwards

it's understood that this specific chip requires fetching the
status byte with the "summary" ECC error flags between filling
the chip's caches and fetching the cached data, still processing
this information might be moved to some "post processing"
routine, instead of being done in the middle of data
communication

> +
> +	return max_bitflips;
> +}
> +
>  static int check_read_status_on_die(struct mtd_info *mtd, int page)
>  {
>  	struct nand_chip *chip = mtd->priv;
> @@ -1294,11 +1352,12 @@ static int check_read_status_on_die(struct mtd_info *mtd, int page)
>  		mtd->ecc_stats.failed++;
>  	} else if (status & NAND_STATUS_REWRITE) {
>  		/*
> -		 * Simple but suboptimal: any page with a single stuck
> -		 * bit will be unusable since it'll be rewritten on
> -		 * each read...
> +		 * Micron on-die ECC doesn't report the number of
> +		 * bitflips, so we have to count them ourself to see
> +		 * if the error rate is too high.  This is
> +		 * particularly important for pages with stuck bits.
>  		 */
> -		max_bitflips = mtd->bitflip_threshold;
> +		max_bitflips = check_for_bitflips(mtd, page);
>  	}
>  	return max_bitflips;
>  }


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de