Skip to contentSkip to conversation

mtd: mxc_nand: fix 8 bit ECC and large oob

PATCH New Baruch Siach 3 participants 10 messages May 13, 2015 – May 21, 2015

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v3 cover mtd: mxc_nand: fix 8 bit ECC and large oob

This patch series fixes mxc_nand bugs that I encountered while trying to use 8 bit ECC mode on i.MX25 with the Micron MT29F8G08ABABA flash chip.

Many thanks to Uwe Kleine-K?nig for guiding me through the process of debugging and fixing, for contributing a clean-up patch to this series, and for reviewing the rest.

This series applies on v4.1-rc1.

This is version 3 of the patch series, adding Reviewed-by and Acked-by tags from Sascha Hauer and Uwe Kleine-K?nig. Patch 4/4 renamed a function as suggested by Uwe Kleine-K?nig.

Version 2 of the patch series
(http://thread.gmane.org/gmane.linux.drivers.mtd/58687), addressed the comments and suggestion of Uwe Kleine-K?nig on the first series
(http://thread.gmane.org/gmane.linux.ports.arm.kernel/408635). See each patch for detailed per-patch changelog.

Baruch Siach (3):
mtd: mxc_nand: limit the size of used oob
mtd: mxc_nand: fix truncate of unaligned oob copying
mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

Uwe Kleine-K?nig (1):
mtd: nand: mxc_nand: cleanup copy_spare function

 drivers/mtd/nand/mxc_nand.c | 110 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 94 insertions(+), 16 deletions(-)
Signature · 1 line
2.1.4
#1 lore raw

PATCH v3 1/4 mtd: nand: mxc_nand: cleanup copy_spare function

From: Uwe Kleine-K?nig <[email protected]>

To give people without the reference manual at hand a chance to understand how spare area is handled in the i.MX nand controller, improve commenting, naming of variables and coding style.

No functional change intended.

[baruch: declare oob_chunk_size; update comments; reword commit log]
---
 drivers/mtd/nand/mxc_nand.c | 46 ++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)
drivers/mtd/nand/mxc_nand.c +31 -15
index 372e0e38f59b..33b22b9c0b30 100644
@@ -807,32 +807,48 @@ static void mxc_nand_select_chip_v2(struct mtd_info *mtd, int chip)
 }
 
 /*
- * Function to transfer data to/from spare area.
+ * The controller splits a page into data chunks of 512 bytes + partial oob.
+ * There are writesize / 512 such chunks, the size of the partial oob parts is
+ * oobsize / #chunks rounded down to a multiple of 2. The last oob chunk then
+ * contains additionally the byte lost by rounding (if any).
+ * This function handles the needed shuffling between host->data_buf (which
+ * holds a page in natural order, i.e. writesize bytes data + oobsize bytes
+ * spare) and the NFC buffer.
  */
 static void copy_spare(struct mtd_info *mtd, bool bfrom)
 {
 	struct nand_chip *this = mtd->priv;
 	struct mxc_nand_host *host = this->priv;
-	u16 i, j;
-	u16 n = mtd->writesize >> 9;
+	u16 i, oob_chunk_size;
+	u16 num_chunks = mtd->writesize / 512;
+
 	u8 *d = host->data_buf + mtd->writesize;
 	u8 __iomem *s = host->spare0;
-	u16 t = host->devtype_data->spare_len;
+	u16 sparebuf_size = host->devtype_data->spare_len;
 
-	j = (mtd->oobsize / n >> 1) << 1;
+	/* size of oob chunk for all but possibly the last one */
+	oob_chunk_size = (mtd->oobsize / num_chunks) & ~1;
 
 	if (bfrom) {
-		for (i = 0; i < n - 1; i++)
-			memcpy32_fromio(d + i * j, s + i * t, j);
-
-		/* the last section */
-		memcpy32_fromio(d + i * j, s + i * t, mtd->oobsize - i * j);
+		for (i = 0; i < num_chunks - 1; i++)
+			memcpy32_fromio(d + i * oob_chunk_size,
+					s + i * sparebuf_size,
+					oob_chunk_size);
+
+		/* the last chunk */
+		memcpy32_fromio(d + i * oob_chunk_size,
+				s + i * sparebuf_size,
+				mtd->oobsize - i * oob_chunk_size);
 	} else {
-		for (i = 0; i < n - 1; i++)
-			memcpy32_toio(&s[i * t], &d[i * j], j);
-
-		/* the last section */
-		memcpy32_toio(&s[i * t], &d[i * j], mtd->oobsize - i * j);
+		for (i = 0; i < num_chunks - 1; i++)
+			memcpy32_toio(&s[i * sparebuf_size],
+				      &d[i * oob_chunk_size],
+				      oob_chunk_size);
+
+		/* the last chunk */
+		memcpy32_toio(&s[oob_chunk_size * sparebuf_size],
+			      &d[i * oob_chunk_size],
+			      mtd->oobsize - i * oob_chunk_size);
 	}
 }
 
Signature · 1 line
2.1.4
#2 lore raw

PATCH v3 2/4 mtd: mxc_nand: limit the size of used oob

In reply to Baruch Siach

For 4k pages the i.MX NFC hardware uses no more than 218 bytes for 8bit ECC data. Larger oobsize confuses the logic of copy_spare(). Limit the size of used oob size to avoid that.

---
v2:
* Move used_oobsize into struct mxc_nand_host, and initialize it from

     _probe() (Uwe Kleine-K?nig)
   * Add a more details to in-code comment
---
 drivers/mtd/nand/mxc_nand.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/mtd/nand/mxc_nand.c +13 -3
index 33b22b9c0b30..51c1600d7eb9 100644
@@ -189,6 +189,7 @@ struct mxc_nand_host {
 	int			clk_act;
 	int			irq;
 	int			eccsize;
+	int			used_oobsize;
 	int			active_cs;
 
 	struct completion	op_completion;
@@ -827,7 +828,7 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
 	u16 sparebuf_size = host->devtype_data->spare_len;
 
 	/* size of oob chunk for all but possibly the last one */
-	oob_chunk_size = (mtd->oobsize / num_chunks) & ~1;
+	oob_chunk_size = (host->used_oobsize / num_chunks) & ~1;
 
 	if (bfrom) {
 		for (i = 0; i < num_chunks - 1; i++)
@@ -838,7 +839,7 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
 		/* the last chunk */
 		memcpy32_fromio(d + i * oob_chunk_size,
 				s + i * sparebuf_size,
-				mtd->oobsize - i * oob_chunk_size);
+				host->used_oobsize - i * oob_chunk_size);
 	} else {
 		for (i = 0; i < num_chunks - 1; i++)
 			memcpy32_toio(&s[i * sparebuf_size],
@@ -848,7 +849,7 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
 		/* the last chunk */
 		memcpy32_toio(&s[oob_chunk_size * sparebuf_size],
 			      &d[i * oob_chunk_size],
-			      mtd->oobsize - i * oob_chunk_size);
+			      host->used_oobsize - i * oob_chunk_size);
 	}
 }
 
@@ -1606,6 +1607,15 @@ static int mxcnd_probe(struct platform_device *pdev)
 	else if (mtd->writesize == 4096)
 		this->ecc.layout = host->devtype_data->ecclayout_4k;
 
+	/*
+	 * Experimentation shows that i.MX NFC can only handle up to 218 oob
+	 * bytes. Limit used_oobsize to 218 so as to not confuse copy_spare()
+	 * into copying invalid data to/from the spare IO buffer, as this
+	 * might cause ECC data corruption when doing sub-page write to a
+	 * partially written page.
+	 */
+	host->used_oobsize = min(mtd->oobsize, 218U);
+
 	if (this->ecc.mode == NAND_ECC_HW) {
 		if (is_imx21_nfc(host) || is_imx27_nfc(host))
 			this->ecc.strength = 1;
Signature · 1 line
2.1.4
#3 lore raw

PATCH v3 3/4 mtd: mxc_nand: fix truncate of unaligned oob copying

In reply to Baruch Siach

Copy to/from oob io area might not be aligned to 4 bytes. When 8 bit ECC is used, the buffer size is 26. Add memcpy16_{to,from}io, and use them to avoid truncating the buffer. Prefer memcpy32_{to,from}io when the buffer is properly aligned for better performance.

---
v2:
* Use memcpy16_{to,from}io (Uwe Kleine-K?nig)

---
 drivers/mtd/nand/mxc_nand.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)
drivers/mtd/nand/mxc_nand.c +36 -4
index 51c1600d7eb9..010be8aa41d4 100644
@@ -281,12 +281,44 @@ static void memcpy32_fromio(void *trg, const void __iomem  *src, size_t size)
 		*t++ = __raw_readl(s++);
 }
 
+static void memcpy16_fromio(void *trg, const void __iomem  *src, size_t size)
+{
+	int i;
+	u16 *t = trg;
+	const __iomem u16 *s = src;
+
+	/* We assume that src (IO) is always 32bit aligned */
+	if (PTR_ALIGN(trg, 4) == trg && IS_ALIGNED(size, 4)) {
+		memcpy32_fromio(trg, src, size);
+		return;
+	}
+
+	for (i = 0; i < (size >> 1); i++)
+		*t++ = __raw_readw(s++);
+}
+
 static inline void memcpy32_toio(void __iomem *trg, const void *src, int size)
 {
 	/* __iowrite32_copy use 32bit size values so divide by 4 */
 	__iowrite32_copy(trg, src, size / 4);
 }
 
+static void memcpy16_toio(void __iomem *trg, const void *src, int size)
+{
+	int i;
+	__iomem u16 *t = trg;
+	const u16 *s = src;
+
+	/* We assume that trg (IO) is always 32bit aligned */
+	if (PTR_ALIGN(src, 4) == src && IS_ALIGNED(size, 4)) {
+		memcpy32_toio(trg, src, size);
+		return;
+	}
+
+	for (i = 0; i < (size >> 1); i++)
+		__raw_writew(*s++, t++);
+}
+
 static int check_int_v3(struct mxc_nand_host *host)
 {
 	uint32_t tmp;
@@ -832,22 +864,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
 
 	if (bfrom) {
 		for (i = 0; i < num_chunks - 1; i++)
-			memcpy32_fromio(d + i * oob_chunk_size,
+			memcpy16_fromio(d + i * oob_chunk_size,
 					s + i * sparebuf_size,
 					oob_chunk_size);
 
 		/* the last chunk */
-		memcpy32_fromio(d + i * oob_chunk_size,
+		memcpy16_fromio(d + i * oob_chunk_size,
 				s + i * sparebuf_size,
 				host->used_oobsize - i * oob_chunk_size);
 	} else {
 		for (i = 0; i < num_chunks - 1; i++)
-			memcpy32_toio(&s[i * sparebuf_size],
+			memcpy16_toio(&s[i * sparebuf_size],
 				      &d[i * oob_chunk_size],
 				      oob_chunk_size);
 
 		/* the last chunk */
-		memcpy32_toio(&s[oob_chunk_size * sparebuf_size],
+		memcpy16_toio(&s[oob_chunk_size * sparebuf_size],
 			      &d[i * oob_chunk_size],
 			      host->used_oobsize - i * oob_chunk_size);
 	}
Signature · 1 line
2.1.4
#4 lore raw

PATCH v3 4/4 mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

In reply to Baruch Siach

Hardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet another static struct nand_ecclayout, generate it in code.

---
v3:
Rename ecc_8bit_layout to ecc_8bit_layout_4k (Uwe Kleine-K?nig)

v2:
Initialize eccbytes

---
 drivers/mtd/nand/mxc_nand.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxc_nand.c +21 -1
index 010be8aa41d4..25759563bf11 100644
@@ -960,6 +960,23 @@ static int get_eccsize(struct mtd_info *mtd)
 		return 8;
 }
 
+static void ecc_8bit_layout_4k(struct nand_ecclayout *layout)
+{
+	int i, j;
+
+	layout->eccbytes = 8*18;
+	for (i = 0; i < 8; i++)
+		for (j = 0; j < 18; j++)
+			layout->eccpos[i*18 + j] = i*26 + j + 7;
+
+	layout->oobfree[0].offset = 2;
+	layout->oobfree[0].length = 4;
+	for (i = 1; i < 8; i++) {
+		layout->oobfree[i].offset = i*26;
+		layout->oobfree[i].length = 7;
+	}
+}
+
 static void preset_v1(struct mtd_info *mtd)
 {
 	struct nand_chip *nand_chip = mtd->priv;
@@ -1636,8 +1653,11 @@ static int mxcnd_probe(struct platform_device *pdev)
 
 	if (mtd->writesize == 2048)
 		this->ecc.layout = host->devtype_data->ecclayout_2k;
-	else if (mtd->writesize == 4096)
+	else if (mtd->writesize == 4096) {
 		this->ecc.layout = host->devtype_data->ecclayout_4k;
+		if (get_eccsize(mtd) == 8)
+			ecc_8bit_layout_4k(this->ecc.layout);
+	}
 
 	/*
 	 * Experimentation shows that i.MX NFC can only handle up to 218 oob
Signature · 1 line
2.1.4
#5 lore raw

PATCH v3 4/4 mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

Quoted 5 lines · Baruch Siach

Hardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet another static struct nand_ecclayout, generate it in code.

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#6 lore raw

PATCH v3 4/4 mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

In reply to Baruch Siach
Quoted 2 lines · Baruch Siach

Hardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet another static struct nand_ecclayout, generate it in code.

I like the idea of not adding more static declarations. But I think you have a potential problem below.

Quoted 41 lines

---
v3:
Rename ecc_8bit_layout to ecc_8bit_layout_4k (Uwe Kleine-K?nig)

v2:
Initialize eccbytes

---
 drivers/mtd/nand/mxc_nand.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxc_nand.c +17 -0
index 010be8aa41d4..25759563bf11 100644
@@ -960,6 +960,23 @@ static int get_eccsize(struct mtd_info *mtd)
 		return 8;
 }
 
+static void ecc_8bit_layout_4k(struct nand_ecclayout *layout)
+{
+	int i, j;
+
+	layout->eccbytes = 8*18;
+	for (i = 0; i < 8; i++)
+		for (j = 0; j < 18; j++)
+			layout->eccpos[i*18 + j] = i*26 + j + 7;
+
+	layout->oobfree[0].offset = 2;
+	layout->oobfree[0].length = 4;
+	for (i = 1; i < 8; i++) {
+		layout->oobfree[i].offset = i*26;
+		layout->oobfree[i].length = 7;
+	}
+}
+
 static void preset_v1(struct mtd_info *mtd)
 {
 	struct nand_chip *nand_chip = mtd->priv;

drivers/mtd/nand/mxc_nand.c static int mxcnd_probe(struct platform_device *pdev) · line 1659

Quoted 8 lines
drivers/mtd/nand/mxc_nand.c +3 -1
@@ -1636,8 +1653,11 @@ static int mxcnd_probe(struct platform_device *pdev)
 
 	if (mtd->writesize == 2048)
 		this->ecc.layout = host->devtype_data->ecclayout_2k;
-	else if (mtd->writesize == 4096)
+	else if (mtd->writesize == 4096) {
 		this->ecc.layout = host->devtype_data->ecclayout_4k;
+		if (get_eccsize(mtd) == 8)
+			ecc_8bit_layout_4k(this->ecc.layout);

So you're overwriting an existing layout (e.g., nandv2_hw_eccoob_4k). What if you have more than one NAND chip? You might do better by dynamically allocating the memory.

drivers/mtd/nand/mxc_nand.c static int mxcnd_probe(struct platform_device *pdev) · line 1663

Quoted 4 lines
drivers/mtd/nand/mxc_nand.c +1 -0
+	}
 
 	/*
 	 * Experimentation shows that i.MX NFC can only handle up to 218 oob

Brian

#7 lore raw

PATCH v3 4/4 mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

Hi Brian,

Quoted 7 lines · Brian Norris

drivers/mtd/nand/mxc_nand.c static int mxcnd_probe(struct platform_device *pdev) · line 1659

Quoted 2 lines · Baruch Siach
drivers/mtd/nand/mxc_nand.c +2 -0
+		if (get_eccsize(mtd) == 8)
+			ecc_8bit_layout_4k(this->ecc.layout);

So you're overwriting an existing layout (e.g., nandv2_hw_eccoob_4k). What if you have more than one NAND chip? You might do better by dynamically allocating the memory.

It would take a quite a bit more code changes then that to have the mxc_nand driver support more than one NAND chip, not to mention the DT binding. As Uwe has indicated on a previous version of this series, ecclayout handling in this driver could use some cleanup. This patch just fixes bug, trying to break anything else while doing so.

Thanks for reviewing, and for applying the rest of this series.

baruch

Signature · 3 lines
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
Applied
#8 lore raw

PATCH v3 4/4 mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

Quoted 11 lines · Baruch Siach
Quoted 7 lines · Brian Norris

drivers/mtd/nand/mxc_nand.c static int mxcnd_probe(struct platform_device *pdev) · line 1659

Quoted 2 lines · Baruch Siach
drivers/mtd/nand/mxc_nand.c +2 -0
+		if (get_eccsize(mtd) == 8)
+			ecc_8bit_layout_4k(this->ecc.layout);

So you're overwriting an existing layout (e.g., nandv2_hw_eccoob_4k). What if you have more than one NAND chip? You might do better by dynamically allocating the memory.

It would take a quite a bit more code changes then that to have the mxc_nand driver support more than one NAND chip, not to mention the DT binding. As Uwe

Right. I guess there's also the case that you have more than one instance of this controller / driver. But I assume that's pretty unlikely?

Quoted 3 lines

has indicated on a previous version of this series, ecclayout handling in this driver could use some cleanup. This patch just fixes bug, trying to break anything else while doing so.

Yeah, OK. Then I'll apply this patch anyway, and the rest could be worked out later if this driver ever supports more cases.

Quoted 1 line

Thanks for reviewing, and for applying the rest of this series.

Applied, thanks.

Brian

#9 lore raw

v3 cover mtd: mxc_nand: fix 8 bit ECC and large oob

In reply to Baruch Siach
Quoted 13 lines · Baruch Siach

This patch series fixes mxc_nand bugs that I encountered while trying to use 8 bit ECC mode on i.MX25 with the Micron MT29F8G08ABABA flash chip.

Many thanks to Uwe Kleine-K?nig for guiding me through the process of debugging and fixing, for contributing a clean-up patch to this series, and for reviewing the rest.

This series applies on v4.1-rc1.

This is version 3 of the patch series, adding Reviewed-by and Acked-by tags from Sascha Hauer and Uwe Kleine-K?nig. Patch 4/4 renamed a function as suggested by Uwe Kleine-K?nig.

Quoted 5 lines

Version 2 of the patch series
(http://thread.gmane.org/gmane.linux.drivers.mtd/58687), addressed the comments and suggestion of Uwe Kleine-K?nig on the first series
(http://thread.gmane.org/gmane.linux.ports.arm.kernel/408635). See each patch for detailed per-patch changelog.

Pushed the first 3 to l2-mtd.git. Thanks all!

I have one comment on the 4th.

Brian