mtd: nand: Add support for Micron on-die ECC controller (rev2).

This patch enables support for on-die ECC controllers as found on
certain Micron chips, for example.  On-die ECC can be useful for
platforms lacking hardware-support for multi-bit ECC.  The patch is
safe to apply because it never turns on on-die ECC on its own (that
job is left to the bootloader).  Instead, this code simply checks
whether the on-die ECC controller is enabled and, if so, uses it.

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

Hi David,

>From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of David Mosberger
>Subject: [PATCH] mtd: nand: Add support for Micron on-die ECC controller (rev2).
>
Sorry, I looked at this patch little lately.
But good, if you can keep $subject consistent, like using
prefix "[PATCH v2]"  instead of adding suffix "(rev2)".

>This patch enables support for on-die ECC controllers as found on
>certain Micron chips, for example.  On-die ECC can be useful for
>platforms lacking hardware-support for multi-bit ECC.  The patch is
>safe to apply because it never turns on on-die ECC on its own (that
>job is left to the bootloader).  Instead, this code simply checks
>whether the on-die ECC controller is enabled and, if so, uses it.
>
Also, It would have been good if you can break this patch into two portions
because, I see this patch touching some sections of generic NAND driver
which are re-used by multiple other controller drivers, and it would be
necessary to take multiple tested-by from different owners before
accepting these changes. So, if you break your patch into
- Patch having Micron on-die controller specific changes / hooks
- Patch populating those hooks in generic NAND interface and other checks
Then it would be easy for review and fast in getting acceptance.

(Also, good to be reviewed by Brian once)


>Signed-of-by: David Mosberger <davidm@egauge.net>
>---
> drivers/mtd/nand/nand_base.c |  258 +++++++++++++++++++++++++++++++++++++++++-
> include/linux/mtd/nand.h     |    8 ++
> 2 files changed, 263 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>index 5826da3..e642d1f 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 = {
>@@ -1250,6 +1264,196 @@ 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, struct nand_chip *chip, int on)
>+{
>+	u8 data[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
>+
>+	if (chip->ecc.mode != NAND_ECC_HW_ON_DIE)
>+		return 0;
>+
>+	if (on)
>+		data[0] = ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC;
>+
>+	return chip->onfi_set_features(mtd, chip, ONFI_FEATURE_ADDR_OP_MODE,
>+				       data);
>+}
>+
>+/*
>+ * Return the number of bits that differ between buffers SRC1 and
>+ * SRC2, both of which are LEN bytes long.
>+ *
>+ * This code could be optimized for, but it only gets called on pages
>+ * with bitflips and compared to the cost of migrating an eraseblock,
>+ * the execution time here is trivial...
>+ */
>+static int
>+bitdiff(const void *s1, const void *s2, size_t len)
>+{
>+	const uint8_t *src1 = s1, *src2 = s2;
>+	int count = 0, i;
>+
>+	for (i = 0; i < len; ++i)
>+		count += hweight8(*src1++ ^ *src2++);
>+	return count;
>+}
>+
>+static int
>+check_for_bitflips(struct mtd_info *mtd, struct nand_chip *chip, int page)
>+{
>+	int flips = 0, max_bitflips = 0, i, j, read_size;
>+	uint8_t *chkbuf, *rawbuf, *chkoob, *rawoob;
>+	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);

This should have been called directly from nand_read_page_on_die()

>+
>+	/* Re-read page with on-die ECC off: */
>+	set_on_die_ecc(mtd, chip, 0);
>+	{
Why these braces {} ? Did  you miss any error-checking here ?

>+		chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
>+		chip->read_buf(mtd, rawbuf, read_size);
>+	}

>+	set_on_die_ecc(mtd, chip, 1);
>+
>+	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 = bitdiff(chkbuf, rawbuf, chip->ecc.size);
>+		/* Count bit flips in the ECC bytes: */
>+		for (j = 0; j < chip->ecc.bytes; ++j) {

You should check bit-flips in complete OOB region (mtd->oobsize) not just ecc.bytes.

>+			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);
>+

Something wrong here in the flow. (please see my comments later).

>+	return max_bitflips;
>+}
>+
>+static int check_read_status_on_die(struct mtd_info *mtd,
>+				    struct nand_chip *chip, int page)
>+{
>+	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->cmd_ctrl(mtd, NAND_CMD_READ0,
>+		       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
>+	chip->cmd_ctrl(mtd, NAND_CMD_NONE,
>+		       NAND_NCE | NAND_CTRL_CHANGE);
>+
>+	if (status & NAND_STATUS_FAIL) {
>+		/* Page has invalid ECC. */
>+		mtd->ecc_stats.failed++;
>+	} else if (status & NAND_STATUS_REWRITE) {
>+		/*
>+		 * The Micron chips turn on the REWRITE status bit for
>+		 * ANY bit flips.  Some pages have stuck bits, so we
>+		 * don't want to migrate a block just because of
>+		 * single bit errors because otherwise, that block
>+		 * would effectively become unusable.  So, work out in
>+		 * software what the max number of flipped bits is for
>+		 * all subpages in a page:
>+		 */
>+		max_bitflips = check_for_bitflips(mtd, chip, page);
>+	}
>+	return max_bitflips;
>+}
>+
>+/**
>+ * 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, chip, 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
>+ * @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;
>+

This is the entry point of chip->ecc.read_page(),  So, your first call to ecc.read_buf
should have happened here, not in check_for_bitflips(). And you should have just
passed the read_buffer to for checking of bit-flips.

>+	ret = check_read_status_on_die(mtd, chip, page);
Also this should have been part of nand_chip->ecc.correct() interface.



>+	if (ret < 0 || mtd->ecc_stats.failed != failed) {
>+		memset(buf, 0, mtd->writesize);

This is not required, because if there are ECC failure (uncorrectable bit-flips)
then, nand_do_read_ops() will automatically convert it into -EBADMSG.
And then let above layers MTD, UBI determine what to do with the corrupted data.
Un-correctable bit-flips may be in those portions of  a page which do not
contain any data. Hence, you should not reset the buffer even for ECC failures.

Example: UBIFS erase-header and volume-id-headers occupy on first few
bytes of the page. And so UBIFS uses in-band CRC checks to see if its headers
have any corruption. Now, even if the NAND page had some uncorrectable
bit-flips, but those bit-flips do not affect the data of UBIFS headers,
then UBIFS just reads those pages and scrubs the block.


>+		if (oob_required)
>+			memset(chip->oob_poi, 0, mtd->oobsize);
 -- same here --

>+		return ret;
>+	}
>+
>+	chip->read_buf(mtd, buf, mtd->writesize);

This is not correct. You first read the data, and then check for bit-flips.
You *don't* re-read the page again, because on re-read you may find
new bit-flips due to read-disturb errors, which were not accounted in
the earlier count. 

So, if you follow the below sequence..
(1) read buffer (with on-die ECC == ON)
(2) check for bit-flips (just pass the read_buffer)
(3) correct bit-flips (again just pass read_buffer)
Then probably you don't need this call, and you can re-use existing generic
NAND driver implementations.


>+	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
>@@ -3783,22 +3987,46 @@ EXPORT_SYMBOL(nand_scan_ident);
> int nand_scan_tail(struct mtd_info *mtd)
> {
> 	int i;
>+	u8 features[ONFI_SUBFEATURE_PARAM_LEN];
> 	struct nand_chip *chip = mtd->priv;
> 	struct nand_ecc_ctrl *ecc = &chip->ecc;
> 	struct nand_buffers *nbuf;
>
>+	if (chip->onfi_get_features(mtd, chip, ONFI_FEATURE_ADDR_OP_MODE,
>+				    features) >= 0) {
>+		if (features[0] & ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC) {
>+			/*
>+			 * 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");
>+		}
>+	}
>+
> 	/* New bad blocks should be marked in OOB, flash-based BBT, or both */
> 	BUG_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
> 			!(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 {
>@@ -3956,6 +4184,25 @@ 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 attempts to put Bbt marker at offset 8 in
>+		   oob, which is used for ECC by Micron
>+		   MT29F4G16ABADAWP, for example.  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.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;
>+		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");
>@@ -4023,8 +4270,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 */
>diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>index 450d61e..4514ced 100644
>--- a/include/linux/mtd/nand.h
>+++ b/include/linux/mtd/nand.h
>@@ -101,6 +101,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
>@@ -115,6 +116,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;
>
> /*
>@@ -214,6 +216,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_OP_MODE	0x90
>+#define ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC		0x08
>+
> /* ONFI subfeature parameters length */
> #define ONFI_SUBFEATURE_PARAM_LEN	4
>
>@@ -516,6 +522,8 @@ struct nand_buffers {
> 	uint8_t	*ecccalc;
> 	uint8_t	*ecccode;
> 	uint8_t *databuf;
>+	uint8_t *chkbuf;
>+	uint8_t *rawbuf;
> };
>
> /**
>--
>1.7.9.5
>
>
Also, can include a link to public copy of the Micron device datasheet
which has on-die ECC feature. It would be helpful to understand your patch


with regards, pekon

thanks for adding me to Cc:

On Thu, 2014-03-27 at 06:56 +0000, Gupta, Pekon wrote:
> 
> >From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of David Mosberger
> >Subject: [PATCH] mtd: nand: Add support for Micron on-die ECC controller (rev2).
> >
> Sorry, I looked at this patch little lately.
> But good, if you can keep $subject consistent, like using
> prefix "[PATCH v2]"  instead of adding suffix "(rev2)".

David, "PATCH v2" (can be applied with --subject-prefix in the
--format-patch invocation) has several benefits, reviewers
clearly notice it's another iteration, and the irrelevant
"(rev2)" won't clobber the commit message upon application

> >+static int
> >+check_for_bitflips(struct mtd_info *mtd, struct nand_chip *chip, int page)
> >+{
> >+	int flips = 0, max_bitflips = 0, i, j, read_size;
> >+	uint8_t *chkbuf, *rawbuf, *chkoob, *rawoob;
> >+	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);
> 
> This should have been called directly from nand_read_page_on_die()

given that there are questions and concerns raised, a comment in
the source about the program flow (in the "good" and "corrected"
and "beyond repair" cases) might be good

I'll respond to the "don't do this here" below

> >+static int check_read_status_on_die(struct mtd_info *mtd,
> >+				    struct nand_chip *chip, int page)
> >+{
> >+	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->cmd_ctrl(mtd, NAND_CMD_READ0,
> >+		       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
> >+	chip->cmd_ctrl(mtd, NAND_CMD_NONE,
> >+		       NAND_NCE | NAND_CTRL_CHANGE);
> >+
> >+	if (status & NAND_STATUS_FAIL) {
> >+		/* Page has invalid ECC. */
> >+		mtd->ecc_stats.failed++;
> >+	} else if (status & NAND_STATUS_REWRITE) {
> >+		/*
> >+		 * The Micron chips turn on the REWRITE status bit for
> >+		 * ANY bit flips.  Some pages have stuck bits, so we
> >+		 * don't want to migrate a block just because of
> >+		 * single bit errors because otherwise, that block
> >+		 * would effectively become unusable.  So, work out in
> >+		 * software what the max number of flipped bits is for
> >+		 * all subpages in a page:
> >+		 */
> >+		max_bitflips = check_for_bitflips(mtd, chip, page);
> >+	}
> >+	return max_bitflips;
> >+}

a quick search even suggests that the REWRITE status bit need not
be seen upon _any_ fixable ECC error, but might depend on an
internal threshold (i.e. when this chip or a later model thinks
that a data refresh might be due); this is an assumption, cannot
tell whether it was verified

http://lists.infradead.org/pipermail/linux-mtd/2012-November/044855.html

it's a pity that the hardware won't tell how many bit errors were
detected, and that you feel you have to do the dance of switching
modes and reading raw data just to find this detail for yourself
:(  this is a new quality in all the patches I have seen floating
around


> >+/**
> >+ * 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;
> >+
> 
> This is the entry point of chip->ecc.read_page(),  So, your first call to ecc.read_buf
> should have happened here, not in check_for_bitflips(). And you should have just
> passed the read_buffer to for checking of bit-flips.
> 
> >+	ret = check_read_status_on_die(mtd, chip, page);
> Also this should have been part of nand_chip->ecc.correct() interface.

AFAIU the sequence of commands which the NAND chip requires
doesn't quite map to the sequence of what the MTD layer does
(emit the read command, read the data, optionally check ECC)

when "on die ECC" is enabled, the chip requires a sequence of
- read page command (READ0, addresses, READSTART)
- status check (mandatory! cmd 70, to check FAIL and REWRITE bits)
- re-enter data read mode (READ0, _without_ addresses and START)
- fetch data bytes

actually the READ0 plus READSTART does a transfer from the chip's
array into the chip's caches, before data is fetched out of the
chip into the SoC; the repeated READ0 does some kind of "rewind"
from status output to data output, it doesn't re-read the array
within the chip

> 
> >+	if (ret < 0 || mtd->ecc_stats.failed != failed) {
> >+		memset(buf, 0, mtd->writesize);
> 
> This is not required, because if there are ECC failure (uncorrectable bit-flips)
> then, nand_do_read_ops() will automatically convert it into -EBADMSG.
> And then let above layers MTD, UBI determine what to do with the corrupted data.
> Un-correctable bit-flips may be in those portions of  a page which do not
> contain any data. Hence, you should not reset the buffer even for ECC failures.
> 
> Example: UBIFS erase-header and volume-id-headers occupy on first few
> bytes of the page. And so UBIFS uses in-band CRC checks to see if its headers
> have any corruption. Now, even if the NAND page had some uncorrectable
> bit-flips, but those bit-flips do not affect the data of UBIFS headers,
> then UBIFS just reads those pages and scrubs the block.

that's a good hint, one might want to check the other ECC
callback routines (I thought I saw that pattern elsewhere, but
was working on a different kernel version)

> >+		if (oob_required)
> >+			memset(chip->oob_poi, 0, mtd->oobsize);
>  -- same here --
> 
> >+		return ret;
> >+	}
> >+
> >+	chip->read_buf(mtd, buf, mtd->writesize);
> 
> This is not correct. You first read the data, and then check for bit-flips.
> You *don't* re-read the page again, because on re-read you may find
> new bit-flips due to read-disturb errors, which were not accounted in
> the earlier count. 
> 
> So, if you follow the below sequence..
> (1) read buffer (with on-die ECC == ON)
> (2) check for bit-flips (just pass the read_buffer)
> (3) correct bit-flips (again just pass read_buffer)
> Then probably you don't need this call, and you can re-use existing generic
> NAND driver implementations.

unfortunately this is not possible according to the chip's
datasheet; even if you _evaluate_ the status only later, you have
to _fetch_ it before the data, and thus do the command re-issue
dance, and cannot use nand_command_lp() since it automatically
does READSTART for READ0

adding another pseudo command with high bits set and LSB 0x00
might help (similar to deplete), then the common command routine
can get re-used

> >@@ -3956,6 +4184,25 @@ 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 attempts to put Bbt marker at offset 8 in
> >+		   oob, which is used for ECC by Micron
> >+		   MT29F4G16ABADAWP, for example.  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.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;
> >+		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;
> >+

I was wondering, have you seen an explicit discussion of how many
data bytes to read and to write in read and program requests?  I
missed this in the datasheet, neither found it in TN-29-56, but
just might have been blind ...


> >@@ -4023,8 +4270,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 */

this might apply to this specific model, but is it a general rule
that chips with on-die-ECC will have subpage support?  and that
we always want to use it?

> Also, can include a link to public copy of the Micron device datasheet
> which has on-die ECC feature. It would be helpful to understand your patch

I don't have a link at hand, but the datasheet's footer reads
"m60a_4gb_8gb_16gb_ecc_nand.pdf"

searching for implementations, there's the "sketch" or outline in
TN-29-56, but I haven't seen an official implementation from
Micron yet, and all the floating patches appear to "be inspired
by each other" if they are not identical (they all follow a
certain scheme, and even use identical identifiers) -- would be
nice to see some official reference, regardless of how much test
an "arbitrary" external implementation has received


virtually yours
Gerhard Sittig

On Thu, Mar 27, 2014 at 5:27 AM, Gerhard Sittig <gsi@denx.de> wrote:
> thanks for adding me to Cc:
>
> On Thu, 2014-03-27 at 06:56 +0000, Gupta, Pekon wrote:
>>
>> >From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of David Mosberger
>> >Subject: [PATCH] mtd: nand: Add support for Micron on-die ECC controller (rev2).
>> >
>> Sorry, I looked at this patch little lately.
>> But good, if you can keep $subject consistent, like using
>> prefix "[PATCH v2]"  instead of adding suffix "(rev2)".
>
> David, "PATCH v2" (can be applied with --subject-prefix in the
> --format-patch invocation) has several benefits, reviewers
> clearly notice it's another iteration, and the irrelevant
> "(rev2)" won't clobber the commit message upon application

OK, thanks for the tip!

> a quick search even suggests that the REWRITE status bit need not
> be seen upon _any_ fixable ECC error, but might depend on an
> internal threshold (i.e. when this chip or a later model thinks
> that a data refresh might be due); this is an assumption, cannot
> tell whether it was verified

That's correct.  Micron doesn't define that clearly, but we found that for the
chip we used (MT29F4G16ABADAWP) the bit gets set for any ECC errors
(and that behavior was confirmed by others on the linux-mtd list previously).

> http://lists.infradead.org/pipermail/linux-mtd/2012-November/044855.html
>
> it's a pity that the hardware won't tell how many bit errors were
> detected, and that you feel you have to do the dance of switching
> modes and reading raw data just to find this detail for yourself
> :(  this is a new quality in all the patches I have seen floating
> around

I agree.  I'm certainly not looking for extra work or creating needless
complexity in the driver and at first was hoping that we could get away
without having to count bitflips, but in the end, it was the only solution
that worked correctly.  Without it, you'd end up rewriting blocks
needlessly, making the problem only worse.

> AFAIU the sequence of commands which the NAND chip requires
> doesn't quite map to the sequence of what the MTD layer does
> (emit the read command, read the data, optionally check ECC)
>
> when "on die ECC" is enabled, the chip requires a sequence of
> - read page command (READ0, addresses, READSTART)
> - status check (mandatory! cmd 70, to check FAIL and REWRITE bits)
> - re-enter data read mode (READ0, _without_ addresses and START)
> - fetch data bytes
>
> actually the READ0 plus READSTART does a transfer from the chip's
> array into the chip's caches, before data is fetched out of the
> chip into the SoC; the repeated READ0 does some kind of "rewind"
> from status output to data output, it doesn't re-read the array
> within the chip

That's correct.

>> >+    if (ret < 0 || mtd->ecc_stats.failed != failed) {
>> >+            memset(buf, 0, mtd->writesize);
>>
>> This is not required, because if there are ECC failure (uncorrectable bit-flips)
>> then, nand_do_read_ops() will automatically convert it into -EBADMSG.
>> And then let above layers MTD, UBI determine what to do with the corrupted data.
>> Un-correctable bit-flips may be in those portions of  a page which do not
>> contain any data. Hence, you should not reset the buffer even for ECC failures.
>>
>> Example: UBIFS erase-header and volume-id-headers occupy on first few
>> bytes of the page. And so UBIFS uses in-band CRC checks to see if its headers
>> have any corruption. Now, even if the NAND page had some uncorrectable
>> bit-flips, but those bit-flips do not affect the data of UBIFS headers,
>> then UBIFS just reads those pages and scrubs the block.

I see.  OK, I'll fix that, thanks!

> I was wondering, have you seen an explicit discussion of how many
> data bytes to read and to write in read and program requests?  I
> missed this in the datasheet, neither found it in TN-29-56, but
> just might have been blind ...

Sorry, I'm not sure I understand the question.  Let me know if this doesn't
answer your question: the way those chips work is that when you issue a
READ command, the entire page gets transferred and ECC applied.
After that, you can transfer bytes from the read buffer at will (even
randomly).

>> >+     * 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 */
>
> this might apply to this specific model, but is it a general rule
> that chips with on-die-ECC will have subpage support?  and that
> we always want to use it?

I don't know, but obviously that can be tweaked later if it's not perfect.

>> Also, can include a link to public copy of the Micron device datasheet
>> which has on-die ECC feature. It would be helpful to understand your patch
>
> I don't have a link at hand, but the datasheet's footer reads
> "m60a_4gb_8gb_16gb_ecc_nand.pdf"
>
> searching for implementations, there's the "sketch" or outline in
> TN-29-56, but I haven't seen an official implementation from
> Micron yet, and all the floating patches appear to "be inspired
> by each other" if they are not identical (they all follow a
> certain scheme, and even use identical identifiers) -- would be
> nice to see some official reference, regardless of how much test
> an "arbitrary" external implementation has received

As far as I know, those datasheets all require an NDA.
For example, google for MT29F4G16ABADAWP and then try
to download the datasheet: you'll have to create an account and
agree to an NDA.  (This is part of the reason we don't use Micron
for our manufacturing anymore).

  --david

Hi,

>From: Gerhard Sittig [mailto:gsi@denx.de]
>>On Thu, 2014-03-27 at 06:56 +0000, Gupta, Pekon wrote:
>> >From: David Mosberger

[...]

>> >+/**
>> >+ * 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;
>> >+
>>
>> This is the entry point of chip->ecc.read_page(),  So, your first call to ecc.read_buf
>> should have happened here, not in check_for_bitflips(). And you should have just
>> passed the read_buffer to for checking of bit-flips.
>>
>> >+	ret = check_read_status_on_die(mtd, chip, page);
>> Also this should have been part of nand_chip->ecc.correct() interface.
>
>AFAIU the sequence of commands which the NAND chip requires
>doesn't quite map to the sequence of what the MTD layer does
>(emit the read command, read the data, optionally check ECC)
>
>when "on die ECC" is enabled, the chip requires a sequence of
>- read page command (READ0, addresses, READSTART)
>- status check (mandatory! cmd 70, to check FAIL and REWRITE bits)
>- re-enter data read mode (READ0, _without_ addresses and START)
>- fetch data bytes
>
I checked Micron Datasheet for "MT29F4G16ABADAWP by name
"m60a_4gb_8gb_16gb_ecc_nand.pdf" and on its 
"Figure 39: READ PAGE (00h-30h) Operation with Internal ECC Enabled"
There is no mention of above third step.

As per the "figure 39" a READ_PAGE Operation with internal ECC enabled
requires following sequence..
- [00h] <address> <address> ... <address> [30h]  /* READ_PAGE command */
- [70h] <status> [00h]        /* READ_STATUS command */
And after that data comes our serial on the bus. So in that case you can


>actually the READ0 plus READSTART does a transfer from the chip's
>array into the chip's caches, before data is fetched out of the
>chip into the SoC; the repeated READ0 does some kind of "rewind"
>from status output to data output, it doesn't re-read the array
>within the chip
>
Agree.. All I'm saying is if you merge "check_read_status_on_die()" into
nand_read_page_on_die() then this would map to normal NAND driver flow.

nand_read_page_on_die( ... ) {
 (a)	chip->cmdfunc(mtd, NAND_CMD_READ0, column, page);  /* READ_PAGE command */

 (b)	/* 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);

(c)	/* Read the data out from the NAND device */
	chip->read_buf(mtd, buf, mtd->writesize);
	if (oob_required)
		chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);

(d)	/* check for ECC errors */
	if (status & NAND_STATUS_FAIL) {
		/* Page has invalid ECC, but still pass to above layers */
		return -EBADMSG;
	} else if (status & NAND_STATUS_REWRITE) {
		bitflip_count = chip->ecc.correct( mtd, buf, , ... )
		return bitflip_count;
	} else {
		return 0;
	}
}

Where; chip->ecc.correct = check_for_bitflips();

Above implementation, as you already fetch the data from NAND device
at the start, it saves you from one extra read done in check_for_bitflips()
+	/* Read entire page w/OOB area with on-die ECC on: */
+	chip->read_buf(mtd, chkbuf, read_size);

Also, you have to return the read_data, even in case of ECC failures,
which was not earlier done in check_read_status_on_die()

[...]

>> (1) read buffer (with on-die ECC == ON)
>> (2) check for bit-flips (just pass the read_buffer)
>> (3) correct bit-flips (again just pass read_buffer)
>> Then probably you don't need this call, and you can re-use existing generic
>> NAND driver implementations.
>
>unfortunately this is not possible according to the chip's
>datasheet; even if you _evaluate_ the status only later, you have
>to _fetch_ it before the data, and thus do the command re-issue
>dance, and cannot use nand_command_lp() since it automatically
>does READSTART for READ0
>
I have broken (1) into (a) + (b) + (c) above.
And (2) + (3) = (d), where, (3) is actually chip->ecc.correct() = check_for_bitflips().

Hope that clarifies the intend..


With regards, pekon

On Fri, 2014-03-28 at 08:37 +0000, Gupta, Pekon wrote:
> 
> >From: Gerhard Sittig [mailto:gsi@denx.de]
> 
> [...]
> 
> >AFAIU the sequence of commands which the NAND chip requires
> >doesn't quite map to the sequence of what the MTD layer does
> >(emit the read command, read the data, optionally check ECC)
> >
> >when "on die ECC" is enabled, the chip requires a sequence of
> >- read page command (READ0, addresses, READSTART)
> >- status check (mandatory! cmd 70, to check FAIL and REWRITE bits)
> >- re-enter data read mode (READ0, _without_ addresses and START)
> >- fetch data bytes
> >
> I checked Micron Datasheet for "MT29F4G16ABADAWP by name
> "m60a_4gb_8gb_16gb_ecc_nand.pdf" and on its 
> "Figure 39: READ PAGE (00h-30h) Operation with Internal ECC Enabled"
> There is no mention of above third step.
> 
> As per the "figure 39" a READ_PAGE Operation with internal ECC enabled
> requires following sequence..
> - [00h] <address> <address> ... <address> [30h]  /* READ_PAGE command */
> - [70h] <status> [00h]        /* READ_STATUS command */
> And after that data comes our serial on the bus. So in that case you can

They cheat. :)  You'll only notice when you look closely, I
misread this stuff too at first.  The Micron datasheet uses
different terms than the MTD implementation in Linux does.

Their "READ_PAGE" command, referred to as 00-30, actually is in
Linux the "READ0 command (0x00) and READSTART command (0x30)"
sequence.  (I'd like to see them as sequences or aggregates of
existing commands, instead of even more commands that happen to
re-use other command's opcodes, which I feel is confusing.)

The above "70 status 00" actually is "STATUS (command 70 and a
status byte) plus READ0 (command 00)".  Not re-emitting the READ0
(this time without the addresses and READSTART) would make you
fetch more status bytes instead of the cached data bytes.

So you will find everything that is done in the implementation,
it's just that you have to "translate" between the perspectives.
That's OK, Linux is not the reference, but one has to remain
aware of this difference.


It's nice to see that apparently the "regular read with
subsequent ECC check" can be done, too.  With an option for
another "raw read and bit flip counting" should errors have
occured.


virtually yours
Gerhard Sittig

On Thu, 2014-03-27 at 13:28 -0600, David Mosberger wrote:
> 
> On Thu, Mar 27, 2014 at 5:27 AM, Gerhard Sittig <gsi@denx.de> wrote:
> 
> > I was wondering, have you seen an explicit discussion of how many
> > data bytes to read and to write in read and program requests?  I
> > missed this in the datasheet, neither found it in TN-29-56, but
> > just might have been blind ...
> 
> Sorry, I'm not sure I understand the question.  Let me know if this doesn't
> answer your question: the way those chips work is that when you issue a
> READ command, the entire page gets transferred and ECC applied.
> After that, you can transfer bytes from the read buffer at will (even
> randomly).

It's the answer to one half of what I wondered. :)

Upon read, the chip does ECC detection and correction in
transparent ways.  So you probably just fetch the status and the
page's content (the writesize count), omitting the OOB area.
What would you get when you keep reading after the page size?
Probably the OOB area.  Is it required to read this as well, to
have it around in the write case?

Upon write, you certainly have to send the page content, i.e. a
stream of bytes with a count of the page's payload size.  But I
would not be certain whether you have to send the OOB area as
well.  Even if the chip will create and store ECC data by itself,
I could not find whether you have to "clock those bytes in" to
trigger the in-chip operation, too.  Am I overly paranoid?  Am I
talking nonsense? :)  There must have been a reason for the
introduction of the "oob_required" condition -- I just understood
it was other MLC chips, not the on-die-ECC that we are talking
about here.

But _if_ you have to send the OOB data upon write, most probably
you have to read this area as well, to pass back those data bytes
within OOB that are not ECC, or else you risk losing mgmt data.

Yes, I'm aware of the READ_OOB "pseudo command", which does a
read operation yet implies a specific offset within the cache,
i.e. it "forwards the read pointer".  This is used in a different
context, as is the raw operation.


virtually yours
Gerhard Sittig

Gerhard,

On Fri, Mar 28, 2014 at 6:56 AM, Gerhard Sittig <gsi@denx.de> wrote:

> Upon read, the chip does ECC detection and correction in
> transparent ways.  So you probably just fetch the status and the
> page's content (the writesize count), omitting the OOB area.
> What would you get when you keep reading after the page size?
> Probably the OOB area.  Is it required to read this as well, to
> have it around in the write case?

Yeah, I see where the confusion comes from: ECC is not calculated
when transferring data between the chip and the host.  Instead, ECC is
calculated
when moving a page between the flash array and the chip-internal buffer.

You can transfer data between the host and the chip-internal buffer
in any way you want as it doesn't involve the ECC engine at all.

Instead, it's the READ/PAGEPROG commands that take care of checking/updating
the ECC bytes.

In the datasheet, you can see that from the fact that the chip's RDY
line will stay low
longer with internal ECC enabled: for READ, its t_R_ECC (45-70us) instead of
t_R (25us), for PROGRAM PAGE it's t_PROG_ECC (220-600us) instead of
t_PROG (200-600us).

  --david

On Thu, Mar 27, 2014 at 12:56 AM, Gupta, Pekon <pekon@ti.com> wrote:

>>+      set_on_die_ecc(mtd, chip, 1);
>>+
>>+      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 = bitdiff(chkbuf, rawbuf, chip->ecc.size);
>>+              /* Count bit flips in the ECC bytes: */
>>+              for (j = 0; j < chip->ecc.bytes; ++j) {
>
> You should check bit-flips in complete OOB region (mtd->oobsize) not just ecc.bytes.

I was under the impression that OOB data bytes cannot be assumed to be
ECC protected.
As it happens, when using Internal ECC on those Micron chips, *some*
of the OOB databytes
are ECC protected, but I didn't think it was necessary to count those
for bitflips, since OOB users
won't assume ECC protection anyhow.  Am I wrong about that?

  --david

Hi Gerhard,
[...]

>> >when "on die ECC" is enabled, the chip requires a sequence of
>> >- read page command (READ0, addresses, READSTART)
>> >- status check (mandatory! cmd 70, to check FAIL and REWRITE bits)
>> >- re-enter data read mode (READ0, _without_ addresses and START)
>> >- fetch data bytes
>> >
>> I checked Micron Datasheet for "MT29F4G16ABADAWP by name
>> "m60a_4gb_8gb_16gb_ecc_nand.pdf" and on its
>> "Figure 39: READ PAGE (00h-30h) Operation with Internal ECC Enabled"
>> There is no mention of above third step.
>>
>> As per the "figure 39" a READ_PAGE Operation with internal ECC enabled
>> requires following sequence..
>> - [00h] <address> <address> ... <address> [30h]  /* READ_PAGE command */
>> - [70h] <status> [00h]        /* READ_STATUS command */
>> And after that data comes our serial on the bus. So in that case you can
>
>They cheat. :)  You'll only notice when you look closely, I
>misread this stuff too at first.  The Micron datasheet uses
>different terms than the MTD implementation in Linux does.
>
>Their "READ_PAGE" command, referred to as 00-30, actually is in
>Linux the "READ0 command (0x00) and READSTART command (0x30)"
>sequence.  (I'd like to see them as sequences or aggregates of
>existing commands, instead of even more commands that happen to
>re-use other command's opcodes, which I feel is confusing.)
>
Yes, I think its correctly implemented in nand_command_lp()
(Kindly cross check if I misunderstood below)

For chip->cmd(mtd, NAND_CMD_READ0, column, page);

@@nand_command_lp(..)
(Step-1) : Sends READ0 == [00h]
         /* Command latch cycle */
	chip->cmd_ctrl(mtd, command, NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);

(Step-2): Sends <column-address> .. <page-address>
        if (column != -1) {
        ...
        }
        if (page != -1) {
        ...
        }

(Step-3): Send READ_START == [30h]
                switch (command) {
                ...
                case NAND_CMD_READ0:
                         chip->cmd_ctrl(mtd, NAND_CMD_READSTART,
			          NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
                         chip->cmd_ctrl(mtd, NAND_CMD_NONE,
			          NAND_NCE | NAND_CTRL_CHANGE);
                }

So, I think nand_command_lp() follows the sequence as per Micron device.
As it has been working for normal nand_reads also.


>The above "70 status 00" actually is "STATUS (command 70 and a
>status byte) plus READ0 (command 00)".  Not re-emitting the READ0
>(this time without the addresses and READSTART) would make you
>fetch more status bytes instead of the cached data bytes.
>
Yes, this is why I said ("this may require some tweaks").
So, actually you can modify nand_command_lp()
- to add new command = NAND_CMD_END_STATUS which just issues [00h],
- OR modify existing NAND_CMD_STATUS to do something similar.

But I think for most of the command sequences nand_command_lp()
can be re-used, if not then please improve it.

with regards, pekon

On Fri, Mar 28, 2014 at 09:52:37AM -0600, David Mosberger wrote:
> On Thu, Mar 27, 2014 at 12:56 AM, Gupta, Pekon <pekon@ti.com> wrote:
> 
> >>+      set_on_die_ecc(mtd, chip, 1);
> >>+
> >>+      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 = bitdiff(chkbuf, rawbuf, chip->ecc.size);
> >>+              /* Count bit flips in the ECC bytes: */
> >>+              for (j = 0; j < chip->ecc.bytes; ++j) {
> >
> > You should check bit-flips in complete OOB region (mtd->oobsize) not just ecc.bytes.
> 
> I was under the impression that OOB data bytes cannot be assumed to be
> ECC protected.
> As it happens, when using Internal ECC on those Micron chips, *some*
> of the OOB databytes
> are ECC protected, but I didn't think it was necessary to count those
> for bitflips, since OOB users
> won't assume ECC protection anyhow.  Am I wrong about that?

This is a touchy subject. Most of your comments are correct;
traditionally, ECC did not protect OOB, and some of the main users of it
(like JFFS2) assume that it isn't. This is patently false on some modern
systems, which do protect it.

But for this case, the max_bitflips count is used for determining when
the error rate is unacceptably high, not just to see whether your data
is currently corrupt. So if extra bitflips in this (otherwise
unimportant) spare area might eventually cause the ECC hardware to
report an error, then we need to count them.

Brian

On Fri, Mar 28, 2014 at 01:56:00PM +0100, Gerhard Sittig wrote:
> There must have been a reason for the
> introduction of the "oob_required" condition -- I just understood
> it was other MLC chips, not the on-die-ECC that we are talking
> about here.

'oob_required' is really only there for hardware which optimizes the
no-OOB case for program and read (I have an out-of-tree driver for
hardware whose DMA engine accelerates data-only read/program; ECC is
done on-the-fly in hardware).

Brian

[Forward as plain text.  Thanks, Gmail...]

---------- Forwarded message ----------
From: David Mosberger <davidm@egauge.net>
Date: Tue, Apr 1, 2014 at 9:09 AM
Subject: Re: [PATCH] mtd: nand: Add support for Micron on-die ECC
controller (rev2).
To: Brian Norris <computersforpeace@gmail.com>
Cc: "Gupta, Pekon" <pekon@ti.com>, "dedekind1@gmail.com"
<dedekind1@gmail.com>, "linux-mtd@lists.infradead.org"
<linux-mtd@lists.infradead.org>, Gerhard Sittig <gsi@denx.de>


On Tue, Apr 1, 2014 at 2:36 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
>
>
> But for this case, the max_bitflips count is used for determining when
> the error rate is unacceptably high, not just to see whether your data
> is currently corrupt. So if extra bitflips in this (otherwise
> unimportant) spare area might eventually cause the ECC hardware to
> report an error, then we need to count them.
>

To do this properly would require describing which OOB bytes are
included in the ECC.
I don't think that's possible with the current nand_ecclayout, no?

Also, effectively what my patch does is to make those OOB bytes not
ECC protected, which
is perfectly fine semantics as far as Linux is concerned.  So yes,
what you say would be
an improvement, but it's not strictly needed.

  --david