diff mbox

on-die ECC support

Message ID CACwUX0MqJwed7fGm7miN2meFikY4NhfejhW=Q_NOJ3Fj04475A@mail.gmail.com
State New, archived
Headers show

Commit Message

David Mosberger-Tang March 22, 2013, 9:14 p.m. UTC 
Wow, how did Ivan figure the ECC bit layouts?  I stared at it for a
while last night and then said: no way!

I ended up going with a brutally simple approach: read the page and if
it gets the REWRITE status bit, read it again with on-die ECC turned
off, then do a bitdiff of the data read (and the ECC bytes read) to
count the number of bitflips.  While perhaps brutish, this approach
has the advantage of only relying on documented features.  As long as
the layout described by ecc->layout is accurate, this should work
reliably.  Thus, Micron can screw around with the ECC encoding as they
wish, as long as the layout doesn't change, we're fine.

Note that the patch below doesn't check for bitflips in the OOB areas
that Micron calls "User metadata I".  We don't care about those bits
since our file system doesn't use OOB at all.

Do you have any recommendation on a good value for bitflip_threshold?
1 is obviously too small and 4 feels a bit like playing with fire, no?

  --david

On Fri, Mar 22, 2013 at 2:21 AM, Peter Horton <phorton@bitbox.co.uk> wrote:
> On 21/03/2013 20:00, David Mosberger-Tang wrote:
>>
>>
>> I missed your email until now.  You said:
>>
>>   > Be careful using the NAND status register to indicate bit flips. The
>>   > chips we were using set the status bit for any correction and this can
>>   > result in very high error counts. Some of the pages had bits which are
>>   > permanently 0 or 1 meaning they might need correction every read. We
>>   > worked around this by re-reading any page that the chip flagged as
>>   > corrected and using the software BCH to work out the actual number of
>>   > bits in error.
>>
>> Ouch.  I think you are right: it looks to me that the chip sets the
>> REWRITE_RECOMMENDED bit
>> for any bit flips.  Is the Linux BCH code compatible with the Micron code?
>>
>> Yes, using software BCH to figure out actual number of bitflips would
>> be the right approach then.
>> Is there a patch for that somewhere?
>
>
> Not a patch but the driver file is here :-
>
> http://files.bitbox.co.uk/public/nand-on-die-ecc/10CUL494_nand_2.6.37.c
>
> Credit for the Micron ECC layout goes to Ivan Djelic IIRC.
>
> P.
>

Comments

Peter Horton March 23, 2013, 7:27 p.m. UTC | #1 
On 22/03/2013 21:14, David Mosberger-Tang wrote:
>
> I ended up going with a brutally simple approach: read the page and if
> it gets the REWRITE status bit, read it again with on-die ECC turned
> off, then do a bitdiff of the data read (and the ECC bytes read) to
> count the number of bitflips.  While perhaps brutish, this approach
> has the advantage of only relying on documented features.  As long as
> the layout described by ecc->layout is accurate, this should work
> reliably.  Thus, Micron can screw around with the ECC encoding as they
> wish, as long as the layout doesn't change, we're fine.
>

I prefer this solution to ours, much simpler and probably faster too.

> Note that the patch below doesn't check for bitflips in the OOB areas
> that Micron calls "User metadata I".  We don't care about those bits
> since our file system doesn't use OOB at all.
>
> Do you have any recommendation on a good value for bitflip_threshold?
> 1 is obviously too small and 4 feels a bit like playing with fire, no?
>

IIRC we reprted only when there were three or more bit errors in a page. 
This would allow two bits with fixed states per page.

P.
diff mbox

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index fa85dd5..43d8944 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1167,7 +1167,7 @@  static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip,
  * @bufpoi: buffer to store read data
  */
 static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
-			uint32_t data_offs, uint32_t readlen, uint8_t *bufpoi)
+			uint32_t data_offs, uint32_t readlen, uint8_t *bufpoi, int page)
 {
 	int start_step, end_step, num_steps;
 	uint32_t *eccpos = chip->ecc.layout->eccpos;
@@ -1265,9 +1265,74 @@  set_on_die_ecc (struct mtd_info *mtd, struct nand_chip *chip, int on)
 	return (*chip->onfi_set_features)(mtd, chip, 0x90, data);
 }
 
-static int check_read_status_on_die (struct mtd_info *mtd, struct nand_chip *chip)
+/*
+ * 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)
 {
-	unsigned int max_bitflips = 0;
+	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);
+
+	/* Re-read page with on-die ECC off: */
+	set_on_die_ecc(mtd, chip, 0);
+	{
+		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) {
+			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);
+
+	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: */
@@ -1285,12 +1350,15 @@  static int check_read_status_on_die (struct mtd_info *mtd, struct nand_chip *chi
 		mtd->ecc_stats.failed++;
 	else if (status & NAND_STATUS_REWRITE) {
 		/*
-		 * Bit flip(s); we don't know how many, but since a rewrite is recommended
-		 * we should assume ecc.strength bitflips so that the higher-level software
-		 * is going to do something about it.
+		 * 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:
 		 */
-		mtd->ecc_stats.corrected += chip->ecc.strength;
-		max_bitflips = chip->ecc.strength;
+		max_bitflips = check_for_bitflips (mtd, chip, page);
 	}
 	return max_bitflips;
 }
@@ -1304,7 +1372,7 @@  static int check_read_status_on_die (struct mtd_info *mtd, struct nand_chip *chi
  * @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)
+			uint32_t data_offs, uint32_t readlen, uint8_t *bufpoi, int page)
 {
 	int start_step, end_step, num_steps, ret;
 	int data_col_addr;
@@ -1324,7 +1392,7 @@  static int nand_read_subpage_on_die(struct mtd_info *mtd, struct nand_chip *chip
 
 	failed = mtd->ecc_stats.failed;
 
-	ret = check_read_status_on_die (mtd, chip);
+	ret = check_read_status_on_die (mtd, chip, page);
 	if (ret < 0 || mtd->ecc_stats.failed != failed) {
 		memset (p, 0, datafrag_len);
 		return ret;
@@ -1355,7 +1423,7 @@  static int nand_read_page_on_die(struct mtd_info *mtd, struct nand_chip *chip,
 
 	failed = mtd->ecc_stats.failed;
 
-	ret = check_read_status_on_die (mtd, chip);
+	ret = check_read_status_on_die (mtd, chip, page);
 	if (ret < 0 || mtd->ecc_stats.failed != failed) {
 		memset (buf, 0, mtd->writesize);
 		if (oob_required)
@@ -1639,7 +1707,7 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 			} else if (!aligned && NAND_HAS_SUBPAGE_READ(chip) &&
 				 !oob)
 				ret = chip->ecc.read_subpage(mtd, chip,
-							col, bytes, bufpoi);
+							col, bytes, bufpoi, page);
 			else
 				ret = chip->ecc.read_page(mtd, chip, bufpoi,
 							  oob_required, page);
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 53e99c7..f086dd3 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -379,7 +379,7 @@  struct nand_ecc_ctrl {
 	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
 			uint8_t *buf, int oob_required, int page);
 	int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip,
-			uint32_t offs, uint32_t len, uint8_t *buf);
+			uint32_t offs, uint32_t len, uint8_t *buf, int page);
 	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
 			const uint8_t *buf, int oob_required);
 	int (*write_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip,
@@ -404,6 +404,8 @@  struct nand_buffers {
 	uint8_t	ecccalc[NAND_MAX_OOBSIZE];
 	uint8_t	ecccode[NAND_MAX_OOBSIZE];
 	uint8_t databuf[NAND_MAX_PAGESIZE + NAND_MAX_OOBSIZE];
+	uint8_t chkbuf[NAND_MAX_PAGESIZE + NAND_MAX_OOBSIZE];
+	uint8_t rawbuf[NAND_MAX_PAGESIZE + NAND_MAX_OOBSIZE];
 };
 
 /**