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, and for contributing a clean-up patch to this series.

This series applies and tested on v4.1-rc1.

This is version 2 of the patch series, addressing 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

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

Signed-off-by Baruch Siach <[email protected]>

---
v2:
Initialize eccbytes

---
 drivers/mtd/nand/mxc_nand.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

drivers/mtd/nand/mxc_nand.c +0 -0

index 010be8aa41d4..68aec99f0d4b 100644

drivers/mtd/nand/mxc_nand.c +20 -1

+{
+	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(this->ecc.layout);
+	}
 
 	/*
 	 * Experimentation shows that i.MX NFC can only handle up to 218 oob

Signature · 1 line

2.1.4

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.

Quoted 4 lines · 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.

Did you measure this performance difference? I doubt it's worth the complexity given that we're talking about buffers with a size of up to 26 bytes.

Quoted 18 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, and for contributing a clean-up patch to this series.

This series applies and tested on v4.1-rc1.

This is version 2 of the patch series, addressing 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

Quoted 2 lines

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

I looked at this version and have only found some minor coding style violations (spaces missing on both sides of operators).

Reviewed-by Sascha Hauer <[email protected]>

Hi Uwe,

Quoted 5 lines · Uwe Kleine-K?nig

Quoted 4 lines · 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.

Quoted 3 lines

Did you measure this performance difference? I doubt it's worth the complexity given that we're talking about buffers with a size of up to 26 bytes.

We'll need both memcpy16_{to,from}io and memcpy32_{to,from}io anyway. So by "complexity" you refer to the additional alignment check and memcpy32 fallback?

Quoted 10 lines · Baruch Siach

Quoted 8 lines · Uwe Kleine-K?nig

Quoted 4 lines · 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.

Did you measure this performance difference? I doubt it's worth the complexity given that we're talking about buffers with a size of up to 26 bytes.

Quoted 3 lines

We'll need both memcpy16_{to,from}io and memcpy32_{to,from}io anyway. So by "complexity" you refer to the additional alignment check and memcpy32 fallback?

I thought we could get rid of the memcpy32 variants. Where do we need memcpy32_* where memcpy16 wouldn't work?

Quoted 2 lines

I thought we could get rid of the memcpy32 variants. Where do we need memcpy32_* where memcpy16 wouldn't work?

memcpy16 should work, but would take twice as much IO/memory accesses. That would definitely affect performance, as this is the flash data read/write hot path. I didn't test, though.

Are you sure we want to do that?

Quoted 8 lines · Baruch Siach

Quoted 2 lines

I thought we could get rid of the memcpy32 variants. Where do we need memcpy32_* where memcpy16 wouldn't work?

memcpy16 should work, but would take twice as much IO/memory accesses. That would definitely affect performance, as this is the flash data read/write hot path. I didn't test, though.

Are you sure we want to do that?

no, I'm not sure. But I think it's worth to test how much performance degrades.

Hi Uwe,

Quoted 8 lines · Uwe Kleine-K?nig

Quoted 7 lines · Baruch Siach

Quoted 2 lines

I thought we could get rid of the memcpy32 variants. Where do we need memcpy32_* where memcpy16 wouldn't work?

memcpy16 should work, but would take twice as much IO/memory accesses. That would definitely affect performance, as this is the flash data read/write hot path. I didn't test, though.

Quoted 3 lines

Quoted 1 line

Are you sure we want to do that?

no, I'm not sure. But I think it's worth to test how much performance degrades.

That will have to wait a few weeks as I don't have the hardware handy at the moment.

Quoted 9 lines · Baruch Siach

Quoted 8 lines · Uwe Kleine-K?nig

Quoted 7 lines · Baruch Siach

Quoted 2 lines

I thought we could get rid of the memcpy32 variants. Where do we need memcpy32_* where memcpy16 wouldn't work?

memcpy16 should work, but would take twice as much IO/memory accesses. That would definitely affect performance, as this is the flash data read/write hot path. I didn't test, though.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Are you sure we want to do that?

no, I'm not sure. But I think it's worth to test how much performance degrades.

That will have to wait a few weeks as I don't have the hardware handy at the moment.

In the hope the test will not be forgotten I consider it ok to take this series without the test (with keeping the memcpy32 that is).

Hi Uwe,

Quoted 10 lines · Uwe Kleine-K?nig

Quoted 9 lines · Baruch Siach

Quoted 8 lines · Uwe Kleine-K?nig

Quoted 7 lines · Baruch Siach

Quoted 2 lines

I thought we could get rid of the memcpy32 variants. Where do we need memcpy32_* where memcpy16 wouldn't work?

memcpy16 should work, but would take twice as much IO/memory accesses. That would definitely affect performance, as this is the flash data read/write hot path. I didn't test, though.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Are you sure we want to do that?

no, I'm not sure. But I think it's worth to test how much performance degrades.

That will have to wait a few weeks as I don't have the hardware handy at the moment.

In the hope the test will not be forgotten I consider it ok to take this series without the test (with keeping the memcpy32 that is).

Agreed.

I'll respin this series with the ecc_8bit_layout name change as you suggested.

May I have your Reviewed-by/Acked-by for the patches you are not the author of?