[PATCH v2 0/4] mtd: mxc_nand: fix 8 bit ECC and large oob

[PATCH v2 0/4] mtd: mxc_nand: fix 8 bit ECC and large oob

[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

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(-)

-- 
2.1.4

[PATCH v2 1/4] mtd: nand: mxc_nand: cleanup copy_spare function

[Baruch Siach]

From: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

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.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
[baruch: declare oob_chunk_size; update comments; reword commit log]
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/mtd/nand/mxc_nand.c | 46 ++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 372e0e38f59b..33b22b9c0b30 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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);
 	}
 }
 
-- 
2.1.4

[PATCH v2 2/4] mtd: mxc_nand: limit the size of used oob

[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.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
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(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 33b22b9c0b30..51c1600d7eb9 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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;
-- 
2.1.4

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[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.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2:
   * Use memcpy16_{to,from}io (Uwe Kleine-K?nig)
---
 drivers/mtd/nand/mxc_nand.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 51c1600d7eb9..010be8aa41d4 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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);
 	}
-- 
2.1.4

[PATCH v2 4/4] mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

[Baruch Siach]

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 <baruch@tkos.co.il>
---
v2:
   Initialize eccbytes
---
 drivers/mtd/nand/mxc_nand.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 010be8aa41d4..68aec99f0d4b 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -960,6 +960,23 @@ static int get_eccsize(struct mtd_info *mtd)
 		return 8;
 }
 
+static void ecc_8bit_layout(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(this->ecc.layout);
+	}
 
 	/*
 	 * Experimentation shows that i.MX NFC can only handle up to 218 oob
-- 
2.1.4

[PATCH v2 0/4] mtd: mxc_nand: fix 8 bit ECC and large oob

[Sascha Hauer]

On Sun, May 03, 2015 at 10:18:50AM +0300, Baruch Siach wrote:
> 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

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 <s.hauer@pengutronix.de>

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 4/4] mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

[Uwe Kleine-König]

On Sun, May 03, 2015 at 10:18:54AM +0300, Baruch Siach wrote:
> 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 <baruch@tkos.co.il>
A nice possible follow-up for this patch would be to substitute all
layouts by a function.

> ---
> v2:
>    Initialize eccbytes
> ---
>  drivers/mtd/nand/mxc_nand.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 010be8aa41d4..68aec99f0d4b 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -960,6 +960,23 @@ static int get_eccsize(struct mtd_info *mtd)
>  		return 8;
>  }
>  
> +static void ecc_8bit_layout(struct nand_ecclayout *layout)
As this only applies to 4k nand chips I'd suggest to add 4k somewhere in
the name of this function.

> +{
> +	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
> -- 
> 2.1.4
> 
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

On Sun, May 03, 2015 at 10:18:53AM +0300, Baruch Siach wrote:
> 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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Baruch Siach]

Hi Uwe,

On Fri, May 08, 2015 at 09:24:32AM +0200, Uwe Kleine-K?nig wrote:
> On Sun, May 03, 2015 at 10:18:53AM +0300, Baruch Siach wrote:
> > 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.

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?

baruch

-- 
     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 -

[PATCH v2 0/4] mtd: mxc_nand: fix 8 bit ECC and large oob

[Baruch Siach]

Hi Sascha,

On Fri, May 08, 2015 at 07:15:38AM +0200, Sascha Hauer wrote:
> On Sun, May 03, 2015 at 10:18:50AM +0300, Baruch Siach wrote:
> > 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
> 
> I looked at this version and have only found some minor coding style
> violations (spaces missing on both sides of operators).

Documentation/CodingStyle mentions it but checkpatch.pl doesn't warn. I find 
it more readable to omit spaces around the * operator when mixing with +. 

	layout->eccpos[i*18 + j] = i*26 + j + 7;

> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

Thanks for reviewing,
baruch

-- 
     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 -

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

On Wed, May 13, 2015 at 08:12:02AM +0300, Baruch Siach wrote:
> Hi Uwe,
> 
> On Fri, May 08, 2015 at 09:24:32AM +0200, Uwe Kleine-K?nig wrote:
> > On Sun, May 03, 2015 at 10:18:53AM +0300, Baruch Siach wrote:
> > > 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.
> 
> 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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Baruch Siach]

Hi Uwe,

On Wed, May 13, 2015 at 08:39:03AM +0200, Uwe Kleine-K?nig wrote:
> On Wed, May 13, 2015 at 08:12:02AM +0300, Baruch Siach wrote:
> > On Fri, May 08, 2015 at 09:24:32AM +0200, Uwe Kleine-K?nig wrote:
> > > On Sun, May 03, 2015 at 10:18:53AM +0300, Baruch Siach wrote:
> > > > 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.
> > 
> > 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?

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?

baruch

-- 
     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 -

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

Hello Baruch,

On Wed, May 13, 2015 at 09:44:04AM +0300, Baruch Siach wrote:
> > 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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Baruch Siach]

Hi Uwe,

On Wed, May 13, 2015 at 08:47:34AM +0200, Uwe Kleine-K?nig wrote:
> On Wed, May 13, 2015 at 09:44:04AM +0300, Baruch Siach wrote:
> > > 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.

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

baruch

-- 
     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 -

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

On Wed, May 13, 2015 at 09:59:22AM +0300, Baruch Siach wrote:
> Hi Uwe,
> 
> On Wed, May 13, 2015 at 08:47:34AM +0200, Uwe Kleine-K?nig wrote:
> > On Wed, May 13, 2015 at 09:44:04AM +0300, Baruch Siach wrote:
> > > > 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.
> 
> 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).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Baruch Siach]

Hi Uwe,

On Wed, May 13, 2015 at 09:01:40AM +0200, Uwe Kleine-K?nig wrote:
> On Wed, May 13, 2015 at 09:59:22AM +0300, Baruch Siach wrote:
> > On Wed, May 13, 2015 at 08:47:34AM +0200, Uwe Kleine-K?nig wrote:
> > > On Wed, May 13, 2015 at 09:44:04AM +0300, Baruch Siach wrote:
> > > > > 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.
> > 
> > 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?

baruch

-- 
     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 -

[PATCH v2 3/4] mtd: mxc_nand: fix truncate of unaligned oob copying

[Uwe Kleine-König]

On Wed, May 13, 2015 at 10:12:01AM +0300, Baruch Siach wrote:
> Hi Uwe,
> 
> On Wed, May 13, 2015 at 09:01:40AM +0200, Uwe Kleine-K?nig wrote:
> > On Wed, May 13, 2015 at 09:59:22AM +0300, Baruch Siach wrote:
> > > On Wed, May 13, 2015 at 08:47:34AM +0200, Uwe Kleine-K?nig wrote:
> > > > On Wed, May 13, 2015 at 09:44:04AM +0300, Baruch Siach wrote:
> > > > > > 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.
> > > 
> > > 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?
Sure, you can add an Acked-by for patches 2 and 3 for your resent.  Note
that I prefer to have spaces around operators, too. That shouldn't be a
show stopper though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |