[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Boris BREZILLON]

Hello Huang, Brian,

This is just a new proposal to support raw accesses in a more standard way
in the GPMI driver.
This series has been tested on an imx28 board.

Any suggestions are welcome.

Best Regards,

Boris

Changes since v2:
 - fixed a bug in gpmi_move_bits
 - add a raw_buffer field to be used when using raw access methods
   (experienced memory corruptions when directly using page_buffer_virt
   buffer)
 - add raw OOB access functions

Boris BREZILLON (3):
  mtd: nand: gpmi: add gpmi_move_bits function
  mtd: nand: gpmi: add proper raw access support
  mtd: nand: gpmi: add raw oob access functions

 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |  88 ++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 144 +++++++++++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   6 ++
 3 files changed, 238 insertions(+)

-- 
1.9.1

[PATCH v3 1/3] mtd: nand: gpmi: add gpmi_move_bits function

[Boris BREZILLON]

Add a new function to move bits (not bytes) from a memory region to
another one.
This function is similar to memmove except it acts at bit level.
This function is needed to implement GPMI raw access functions, given the
fact that ECC engine does not pad ECC bits to the next byte boundary.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 88 ++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  4 ++
 2 files changed, 92 insertions(+)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 87e658c..e2f706a 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this,
 	set_dma_type(this, DMA_FOR_READ_ECC_PAGE);
 	return start_dma_with_bch_irq(this, desc);
 }
+
+void gpmi_move_bits(u8 *dst, size_t dst_bit_off,
+		    const u8 *src, size_t src_bit_off,
+		    size_t nbits)
+{
+	size_t i;
+	size_t nbytes;
+	u32 src_byte = 0;
+
+	src += src_bit_off / 8;
+	src_bit_off %= 8;
+
+	dst += dst_bit_off / 8;
+	dst_bit_off %= 8;
+
+	if (src_bit_off) {
+		src_byte = src[0] >> src_bit_off;
+		nbits -= 8 - src_bit_off;
+		src++;
+	}
+
+	nbytes = nbits / 8;
+
+	if (dst_bit_off) {
+		if (src_bit_off <= dst_bit_off) {
+			dst[0] &= GENMASK(dst_bit_off - 1, 0);
+			dst[0] |= src_byte << dst_bit_off;
+			src_bit_off += (8 - dst_bit_off);
+			src_byte >>= (8 - dst_bit_off);
+			dst_bit_off = 0;
+			dst++;
+		} else if (nbytes) {
+			src_byte |= src[0] << (8 - src_bit_off);
+			dst[0] &= GENMASK(dst_bit_off - 1, 0);
+			dst[0] |= src_byte << dst_bit_off;
+			src_bit_off += dst_bit_off;
+			src_byte >>= (8 - dst_bit_off);
+			dst_bit_off = 0;
+			dst++;
+			nbytes--;
+			src++;
+			if (src_bit_off > 7) {
+				src_bit_off -= 8;
+				dst[0] = src_byte;
+				dst++;
+				src_byte >>= 8;
+			}
+		}
+	}
+
+	if (!src_bit_off && !dst_bit_off) {
+		if (nbytes)
+			memcpy(dst, src, nbytes);
+	} else {
+		for (i = 0; i < nbytes; i++) {
+			src_byte |= src[i] << (8 - src_bit_off);
+			dst[i] = src_byte;
+			src_byte >>= 8;
+		}
+	}
+
+	dst += nbytes;
+	src += nbytes;
+	nbits %= 8;
+
+	if (!nbits && !src_bit_off)
+		return;
+
+	if (nbits)
+		src_byte |= (*src & GENMASK(nbits - 1, 0)) <<
+			    ((8 - src_bit_off) % 8);
+	nbits += (8 - src_bit_off) % 8;
+
+	if (dst_bit_off)
+		src_byte = (src_byte << dst_bit_off) |
+			   (*dst & GENMASK(dst_bit_off - 1, 0));
+	nbits += dst_bit_off;
+
+	if (nbits % 8)
+		src_byte |= (dst[nbits / 8] & GENMASK(7, nbits % 8)) <<
+			    (nbits / 8);
+
+	nbytes = DIV_ROUND_UP(nbits, 8);
+	for (i = 0; i < nbytes; i++) {
+		dst[i] = src_byte;
+		src_byte >>= 8;
+	}
+}
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 32c6ba4..17d0736 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -290,6 +290,10 @@ extern int gpmi_send_page(struct gpmi_nand_data *,
 extern int gpmi_read_page(struct gpmi_nand_data *,
 			dma_addr_t payload, dma_addr_t auxiliary);
 
+void gpmi_move_bits(u8 *dst, size_t dst_bit_off,
+		    const u8 *src, size_t src_bit_off,
+		    size_t nbits);
+
 /* BCH : Status Block Completion Codes */
 #define STATUS_GOOD		0x00
 #define STATUS_ERASED		0xff
-- 
1.9.1

[PATCH v3 1/3] mtd: nand: gpmi: add gpmi_move_bits function

[Huang Shijie]

On Tue, Sep 23, 2014 at 04:07:34PM +0200, Boris BREZILLON wrote:
> Add a new function to move bits (not bytes) from a memory region to
> another one.
> This function is similar to memmove except it acts at bit level.
> This function is needed to implement GPMI raw access functions, given the
> fact that ECC engine does not pad ECC bits to the next byte boundary.
sorry for not comment your v2 patch set.

> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 88 ++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  4 ++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> index 87e658c..e2f706a 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this,
>  	set_dma_type(this, DMA_FOR_READ_ECC_PAGE);
>  	return start_dma_with_bch_irq(this, desc);
>  }
> +
> +void gpmi_move_bits(u8 *dst, size_t dst_bit_off,
> +		    const u8 *src, size_t src_bit_off,
> +		    size_t nbits)
we can simplify the code.

We could use the bytes to replace the @nbits.

The chunk data is always byte aligned.


Btw: please add more comments in this function.

thanks
Huang Shijie

[PATCH v3 1/3] mtd: nand: gpmi: add gpmi_move_bits function

[Boris BREZILLON]

On Tue, 23 Sep 2014 22:54:15 +0800
Huang Shijie <shijie8@gmail.com> wrote:

> On Tue, Sep 23, 2014 at 04:07:34PM +0200, Boris BREZILLON wrote:
> > Add a new function to move bits (not bytes) from a memory region to
> > another one.
> > This function is similar to memmove except it acts at bit level.
> > This function is needed to implement GPMI raw access functions, given the
> > fact that ECC engine does not pad ECC bits to the next byte boundary.
> sorry for not comment your v2 patch set.
> 
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 88 ++++++++++++++++++++++++++++++++++
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  4 ++
> >  2 files changed, 92 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > index 87e658c..e2f706a 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this,
> >  	set_dma_type(this, DMA_FOR_READ_ECC_PAGE);
> >  	return start_dma_with_bch_irq(this, desc);
> >  }
> > +
> > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off,
> > +		    const u8 *src, size_t src_bit_off,
> > +		    size_t nbits)
> we can simplify the code.

Any suggestions ?

> 
> We could use the bytes to replace the @nbits.
> 
> The chunk data is always byte aligned.

This function is also used to store ECC bits in the OOB buffer and
these chunk of data are not byte aligned :-).

> 
> 
> Btw: please add more comments in this function.

Yep, I should definitely comment it, and I'll do.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 1/3] mtd: nand: gpmi: add gpmi_move_bits function

[Huang Shijie]

On Tue, Sep 23, 2014 at 04:58:22PM +0200, Boris BREZILLON wrote:
> On Tue, 23 Sep 2014 22:54:15 +0800
> Huang Shijie <shijie8@gmail.com> wrote:
> 
> > On Tue, Sep 23, 2014 at 04:07:34PM +0200, Boris BREZILLON wrote:
> > > Add a new function to move bits (not bytes) from a memory region to
> > > another one.
> > > This function is similar to memmove except it acts at bit level.
> > > This function is needed to implement GPMI raw access functions, given the
> > > fact that ECC engine does not pad ECC bits to the next byte boundary.
> > sorry for not comment your v2 patch set.
> > 
> > > 
> > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > ---
> > >  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 88 ++++++++++++++++++++++++++++++++++
> > >  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  4 ++
> > >  2 files changed, 92 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > > index 87e658c..e2f706a 100644
> > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > > @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this,
> > >  	set_dma_type(this, DMA_FOR_READ_ECC_PAGE);
> > >  	return start_dma_with_bch_irq(this, desc);
> > >  }
> > > +
> > > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off,
> > > +		    const u8 *src, size_t src_bit_off,
> > > +		    size_t nbits)
> > we can simplify the code.
> 
> Any suggestions ?
> 
> > 
> > We could use the bytes to replace the @nbits.
> > 
> > The chunk data is always byte aligned.
> 
> This function is also used to store ECC bits in the OOB buffer and
> these chunk of data are not byte aligned :-).
yes. you are right. I missed it.

could you also comment these two hooks in the patch set.

I hope Brian also can check it, and we can make it more clear about how
to implement them.


thanks
Huang Shijie

[PATCH v3 1/3] mtd: nand: gpmi: add gpmi_move_bits function

[Huang Shijie]

On Tue, Sep 23, 2014 at 11:04:28PM +0800, Huang Shijie wrote:
> On Tue, Sep 23, 2014 at 04:58:22PM +0200, Boris BREZILLON wrote:
> > On Tue, 23 Sep 2014 22:54:15 +0800
> > Huang Shijie <shijie8@gmail.com> wrote:
> > 
> > > On Tue, Sep 23, 2014 at 04:07:34PM +0200, Boris BREZILLON wrote:
> > > > Add a new function to move bits (not bytes) from a memory region to
> > > > another one.
> > > > This function is similar to memmove except it acts at bit level.
> > > > This function is needed to implement GPMI raw access functions, given the
> > > > fact that ECC engine does not pad ECC bits to the next byte boundary.
> > > sorry for not comment your v2 patch set.
> > > 
> > > > 
> > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > > ---
> > > >  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 88 ++++++++++++++++++++++++++++++++++
> > > >  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  4 ++
> > > >  2 files changed, 92 insertions(+)
> > > > 
> > > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > > > index 87e658c..e2f706a 100644
> > > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > > > @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this,
> > > >  	set_dma_type(this, DMA_FOR_READ_ECC_PAGE);
> > > >  	return start_dma_with_bch_irq(this, desc);
> > > >  }
> > > > +
> > > > +void gpmi_move_bits(u8 *dst, size_t dst_bit_off,
> > > > +		    const u8 *src, size_t src_bit_off,
> > > > +		    size_t nbits)
> > > we can simplify the code.
> > 
> > Any suggestions ?
> > 
> > > 
> > > We could use the bytes to replace the @nbits.
> > > 
> > > The chunk data is always byte aligned.
> > 
> > This function is also used to store ECC bits in the OOB buffer and
> > these chunk of data are not byte aligned :-).
> yes. you are right. I missed it.
> 
> could you also comment these two hooks in the patch set.
I mean the @ecc->read_page_raw and @ecc->write_page_raw.

thanks
Huang Shijie

[PATCH v3 1/3] mtd: nand: gpmi: add gpmi_move_bits function

[Boris BREZILLON]

Hi Huang,

I've added some code comments inline (and I'll squash them in my next
version).

On Tue, 23 Sep 2014 16:07:34 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> Add a new function to move bits (not bytes) from a memory region to
> another one.
> This function is similar to memmove except it acts at bit level.
> This function is needed to implement GPMI raw access functions, given the
> fact that ECC engine does not pad ECC bits to the next byte boundary.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 88 ++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  4 ++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> index 87e658c..e2f706a 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> @@ -1353,3 +1353,91 @@ int gpmi_read_page(struct gpmi_nand_data *this,
>  	set_dma_type(this, DMA_FOR_READ_ECC_PAGE);
>  	return start_dma_with_bch_irq(this, desc);
>  }
> +
> +void gpmi_move_bits(u8 *dst, size_t dst_bit_off,
> +		    const u8 *src, size_t src_bit_off,
> +		    size_t nbits)
> +{
> +	size_t i;
> +	size_t nbytes;
> +	u32 src_byte = 0;
> +


	/*
	 * Move src and dst pointers to the closest byte pointer and
	 * store bit offsets within a byte.
	 */

> +	src += src_bit_off / 8;
> +	src_bit_off %= 8;
> +
> +	dst += dst_bit_off / 8;
> +	dst_bit_off %= 8;
> +
	/*
	 * Initialize the src_byte value with bits available in the
	 * first byte of data so that we end up with a byte aligned
	 * src pointer.
	 */
> +	if (src_bit_off) {
> +		src_byte = src[0] >> src_bit_off;
> +		nbits -= 8 - src_bit_off;
> +		src++;
> +	}
> +
	/*
	 * Calculate the number of bytes that can be copied from src to
	 * dst.
	 */
> +	nbytes = nbits / 8;
> +

	/*
	 * Try to align dst to a byte boundary by peeking some bits
	 * from the source.
	 */

> +	if (dst_bit_off) {
> +		if (src_bit_off <= dst_bit_off) {
> +			dst[0] &= GENMASK(dst_bit_off - 1, 0);
> +			dst[0] |= src_byte << dst_bit_off;
> +			src_bit_off += (8 - dst_bit_off);
> +			src_byte >>= (8 - dst_bit_off);
> +			dst_bit_off = 0;
> +			dst++;
> +		} else if (nbytes) {
> +			src_byte |= src[0] << (8 - src_bit_off);
> +			dst[0] &= GENMASK(dst_bit_off - 1, 0);
> +			dst[0] |= src_byte << dst_bit_off;
> +			src_bit_off += dst_bit_off;
> +			src_byte >>= (8 - dst_bit_off);
> +			dst_bit_off = 0;
> +			dst++;
> +			nbytes--;
> +			src++;
> +			if (src_bit_off > 7) {
> +				src_bit_off -= 8;
> +				dst[0] = src_byte;
> +				dst++;
> +				src_byte >>= 8;
> +			}
> +		}
> +	}
> +
> +	if (!src_bit_off && !dst_bit_off) {
		/*
		 * Both src and dst pointers are byte aligned, thus we
		 * can just use the optimized memcpy function
		 */
> +		if (nbytes)
> +			memcpy(dst, src, nbytes);
> +	} else {

		/*
		 * src buffer is not byte aligned, hence we have to copy
		 * each src byte to the src_byte variable (after
		 * applying the appropriate shift depending on the
		 * src bit offset).
		 * We still try to work on bytes until there's not
		 * enough available data in the src buffer.
		 */

> +		for (i = 0; i < nbytes; i++) {
> +			src_byte |= src[i] << (8 - src_bit_off);
> +			dst[i] = src_byte;
> +			src_byte >>= 8;
> +		}
> +	}
> +
	/* move dst and src buffers */
> +	dst += nbytes;
> +	src += nbytes;

	/*
	 * nbits is the number of remaining bits. It should not exceed
	 * 8 as we've already worked on bytes as much as possible.
	 */


> +	nbits %= 8;
> +

	/*
	 * if there's no more bits to copy to the destination and src
	 * buffer was already byte aligned, then we're done.
	 */
> +	if (!nbits && !src_bit_off)
> +		return;
> +
	/* Copy the remaining bits to the src_byte variable */
> +	if (nbits)
> +		src_byte |= (*src & GENMASK(nbits - 1, 0)) <<
> +			    ((8 - src_bit_off) % 8);
> +	nbits += (8 - src_bit_off) % 8;
> +

	/*
	 * There were not enough bits to copy from src to dst to get a
	 * byte aligned dst buffer. In this case prepare the src_byte
	 * variable to match the dst organization (just shift src_byte
	 * by dst bit offset and retrieve least significant bits from
	 * dst).
	 */

> +	if (dst_bit_off)
> +		src_byte = (src_byte << dst_bit_off) |
> +			   (*dst & GENMASK(dst_bit_off - 1, 0));
> +	nbits += dst_bit_off;
> +

	/*
	 * Keep most significant bits from dst if we end up with an
	 * unaligned number bits.
	 */

> +	if (nbits % 8)
> +		src_byte |= (dst[nbits / 8] & GENMASK(7, nbits % 8)) <<
> +			    (nbits / 8);
> +

	/* Copy the remaining bytes to the destination */

> +	nbytes = DIV_ROUND_UP(nbits, 8);
> +	for (i = 0; i < nbytes; i++) {
> +		dst[i] = src_byte;
> +		src_byte >>= 8;
> +	}
> +}
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index 32c6ba4..17d0736 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> @@ -290,6 +290,10 @@ extern int gpmi_send_page(struct gpmi_nand_data *,
>  extern int gpmi_read_page(struct gpmi_nand_data *,
>  			dma_addr_t payload, dma_addr_t auxiliary);
>  
> +void gpmi_move_bits(u8 *dst, size_t dst_bit_off,
> +		    const u8 *src, size_t src_bit_off,
> +		    size_t nbits);
> +
>  /* BCH : Status Block Completion Codes */
>  #define STATUS_GOOD		0x00
>  #define STATUS_ERASED		0xff



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support

[Boris BREZILLON]

Several MTD users (either in user or kernel space) expect a valid raw
access support to NAND chip devices.
This is particularly true for testing tools which are often touching the
data stored in a NAND chip in raw mode to artificially generate errors.

The GPMI drivers do not implemenent raw access functions, and thus rely on
default HW_ECC scheme implementation.
The default implementation consider the data and OOB area as properly
separated in their respective NAND section, which is not true for the GPMI
controller.
In this driver/controller some OOB data are stored at the beginning of the
NAND data area (these data are called metadata in the driver), then ECC
bytes are interleaved with data chunk (which is similar to the
HW_ECC_SYNDROME scheme), and eventually the remaining bytes are used as
OOB data.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 126 +++++++++++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   2 +
 2 files changed, 128 insertions(+)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 959cb9b..7921ba7 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -791,6 +791,7 @@ static void gpmi_free_dma_buffer(struct gpmi_nand_data *this)
 					this->page_buffer_phys);
 	kfree(this->cmd_buffer);
 	kfree(this->data_buffer_dma);
+	kfree(this->raw_buffer);
 
 	this->cmd_buffer	= NULL;
 	this->data_buffer_dma	= NULL;
@@ -837,6 +838,9 @@ static int gpmi_alloc_dma_buffer(struct gpmi_nand_data *this)
 	if (!this->page_buffer_virt)
 		goto error_alloc;
 
+	this->raw_buffer = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
+	if (!this->raw_buffer)
+		goto error_alloc;
 
 	/* Slice up the page buffer. */
 	this->payload_virt = this->page_buffer_virt;
@@ -1347,6 +1351,126 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
 	return status & NAND_STATUS_FAIL ? -EIO : 0;
 }
 
+static int gpmi_ecc_read_page_raw(struct mtd_info *mtd,
+				  struct nand_chip *chip, uint8_t *buf,
+				  int oob_required, int page)
+{
+	struct gpmi_nand_data *this = chip->priv;
+	struct bch_geometry *nfc_geo = &this->bch_geometry;
+	int eccsize = nfc_geo->ecc_chunk_size;
+	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
+	u8 *tmp_buf = this->raw_buffer;
+	size_t src_bit_off;
+	size_t oob_bit_off;
+	size_t oob_byte_off;
+	uint8_t *oob = chip->oob_poi;
+	int step;
+
+	chip->read_buf(mtd, tmp_buf,
+		       mtd->writesize + mtd->oobsize);
+
+	if (this->swap_block_mark) {
+		u8 swap = tmp_buf[0];
+
+		tmp_buf[0] = tmp_buf[mtd->writesize];
+		tmp_buf[mtd->writesize] = swap;
+	}
+
+	if (oob_required)
+		memcpy(oob, tmp_buf, nfc_geo->metadata_size);
+
+	oob_bit_off = nfc_geo->metadata_size * 8;
+	src_bit_off = oob_bit_off;
+
+	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
+		if (buf)
+			gpmi_move_bits(buf, step * eccsize * 8,
+				       tmp_buf, src_bit_off,
+				       eccsize * 8);
+		src_bit_off += eccsize * 8;
+
+		if (oob_required)
+			gpmi_move_bits(oob, oob_bit_off,
+				       tmp_buf, src_bit_off,
+				       eccbits);
+
+		src_bit_off += eccbits;
+		oob_bit_off += eccbits;
+	}
+
+	if (oob_required && oob_bit_off % 8)
+		oob[oob_bit_off / 8] &= GENMASK(oob_bit_off - 1, 0);
+
+	oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8);
+
+	if (oob_required && oob_byte_off  < mtd->oobsize)
+		memcpy(oob + oob_byte_off,
+		       tmp_buf + mtd->writesize + oob_byte_off,
+		       mtd->oobsize - oob_byte_off);
+
+	return 0;
+}
+
+static int gpmi_ecc_write_page_raw(struct mtd_info *mtd,
+				   struct nand_chip *chip,
+				   const uint8_t *buf,
+				   int oob_required)
+{
+	struct gpmi_nand_data *this = chip->priv;
+	struct bch_geometry *nfc_geo = &this->bch_geometry;
+	int eccsize = nfc_geo->ecc_chunk_size;
+	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
+	u8 *tmp_buf = this->raw_buffer;
+	uint8_t *oob = chip->oob_poi;
+	size_t dst_bit_off;
+	size_t oob_bit_off;
+	size_t oob_byte_off;
+	int step;
+
+	if (!buf || !oob_required)
+		memset(tmp_buf, 0xff, mtd->writesize + mtd->oobsize);
+
+	memcpy(tmp_buf, oob, nfc_geo->metadata_size);
+	oob_bit_off = nfc_geo->metadata_size * 8;
+	dst_bit_off = oob_bit_off;
+
+	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
+		if (buf)
+			gpmi_move_bits(tmp_buf, dst_bit_off,
+				       buf, step * eccsize * 8, eccsize * 8);
+		dst_bit_off += eccsize * 8;
+
+		/* Pad last ECC block to align following data on a byte */
+		if (step == nfc_geo->ecc_chunk_count - 1 &&
+		    (oob_bit_off + eccbits) % 8)
+			eccbits += 8 - ((oob_bit_off + eccbits) % 8);
+
+		if (oob_required)
+			gpmi_move_bits(tmp_buf, dst_bit_off,
+				       oob, oob_bit_off, eccbits);
+
+		dst_bit_off += eccbits;
+		oob_bit_off += eccbits;
+	}
+
+	oob_byte_off = oob_bit_off / 8;
+
+	if (oob_required && oob_byte_off < mtd->oobsize)
+		memcpy(tmp_buf + mtd->writesize + oob_byte_off,
+		       oob + oob_byte_off, mtd->oobsize - oob_byte_off);
+
+	if (this->swap_block_mark) {
+		u8 swap = tmp_buf[0];
+
+		tmp_buf[0] = tmp_buf[mtd->writesize];
+		tmp_buf[mtd->writesize] = swap;
+	}
+
+	chip->write_buf(mtd, tmp_buf, mtd->writesize + mtd->oobsize);
+
+	return 0;
+}
+
 static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
 {
 	struct nand_chip *chip = mtd->priv;
@@ -1664,6 +1788,8 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
 	ecc->write_page	= gpmi_ecc_write_page;
 	ecc->read_oob	= gpmi_ecc_read_oob;
 	ecc->write_oob	= gpmi_ecc_write_oob;
+	ecc->read_page_raw = gpmi_ecc_read_page_raw;
+	ecc->write_page_raw = gpmi_ecc_write_page_raw;
 	ecc->mode	= NAND_ECC_HW;
 	ecc->size	= bch_geo->ecc_chunk_size;
 	ecc->strength	= bch_geo->ecc_strength;
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 17d0736..89ab5c8 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -189,6 +189,8 @@ struct gpmi_nand_data {
 	void			*auxiliary_virt;
 	dma_addr_t		auxiliary_phys;
 
+	void			*raw_buffer;
+
 	/* DMA channels */
 #define DMA_CHANS		8
 	struct dma_chan		*dma_chans[DMA_CHANS];
-- 
1.9.1

[PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support

[Huang Shijie]

On Tue, Sep 23, 2014 at 04:07:35PM +0200, Boris BREZILLON wrote:
> Several MTD users (either in user or kernel space) expect a valid raw
> access support to NAND chip devices.
> This is particularly true for testing tools which are often touching the
> data stored in a NAND chip in raw mode to artificially generate errors.
> 
> The GPMI drivers do not implemenent raw access functions, and thus rely on
> default HW_ECC scheme implementation.
> The default implementation consider the data and OOB area as properly
> separated in their respective NAND section, which is not true for the GPMI
> controller.
> In this driver/controller some OOB data are stored at the beginning of the
> NAND data area (these data are called metadata in the driver), then ECC
> bytes are interleaved with data chunk (which is similar to the
> HW_ECC_SYNDROME scheme), and eventually the remaining bytes are used as
> OOB data.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 126 +++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   2 +
>  2 files changed, 128 insertions(+)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 959cb9b..7921ba7 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -791,6 +791,7 @@ static void gpmi_free_dma_buffer(struct gpmi_nand_data *this)
>  					this->page_buffer_phys);
>  	kfree(this->cmd_buffer);
>  	kfree(this->data_buffer_dma);
> +	kfree(this->raw_buffer);
>  
>  	this->cmd_buffer	= NULL;
>  	this->data_buffer_dma	= NULL;
> @@ -837,6 +838,9 @@ static int gpmi_alloc_dma_buffer(struct gpmi_nand_data *this)
>  	if (!this->page_buffer_virt)
>  		goto error_alloc;
>  
> +	this->raw_buffer = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
why add this buffer?

did you meet some data overlapped?


> +	if (!this->raw_buffer)
> +		goto error_alloc;
>  
>  	/* Slice up the page buffer. */
>  	this->payload_virt = this->page_buffer_virt;
> @@ -1347,6 +1351,126 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
>  	return status & NAND_STATUS_FAIL ? -EIO : 0;
>  }
>  
> +static int gpmi_ecc_read_page_raw(struct mtd_info *mtd,
> +				  struct nand_chip *chip, uint8_t *buf,
> +				  int oob_required, int page)
> +{
> +	struct gpmi_nand_data *this = chip->priv;
> +	struct bch_geometry *nfc_geo = &this->bch_geometry;
> +	int eccsize = nfc_geo->ecc_chunk_size;
> +	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
> +	u8 *tmp_buf = this->raw_buffer;
> +	size_t src_bit_off;
> +	size_t oob_bit_off;
> +	size_t oob_byte_off;
> +	uint8_t *oob = chip->oob_poi;
> +	int step;
> +
> +	chip->read_buf(mtd, tmp_buf,
> +		       mtd->writesize + mtd->oobsize);
> +
> +	if (this->swap_block_mark) {
> +		u8 swap = tmp_buf[0];
> +
> +		tmp_buf[0] = tmp_buf[mtd->writesize];
> +		tmp_buf[mtd->writesize] = swap;
> +	}
> +
> +	if (oob_required)
> +		memcpy(oob, tmp_buf, nfc_geo->metadata_size);
> +
> +	oob_bit_off = nfc_geo->metadata_size * 8;
> +	src_bit_off = oob_bit_off;
> +
> +	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
> +		if (buf)
	could this @buf become NULL?


> +			gpmi_move_bits(buf, step * eccsize * 8,
> +				       tmp_buf, src_bit_off,
> +				       eccsize * 8);
> +		src_bit_off += eccsize * 8;
> +
> +		if (oob_required)
> +			gpmi_move_bits(oob, oob_bit_off,
> +				       tmp_buf, src_bit_off,
> +				       eccbits);
> +
> +		src_bit_off += eccbits;
> +		oob_bit_off += eccbits;
> +	}
> +
> +	if (oob_required && oob_bit_off % 8)
> +		oob[oob_bit_off / 8] &= GENMASK(oob_bit_off - 1, 0);
> +
> +	oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8);
> +
> +	if (oob_required && oob_byte_off  < mtd->oobsize)
> +		memcpy(oob + oob_byte_off,
> +		       tmp_buf + mtd->writesize + oob_byte_off,
> +		       mtd->oobsize - oob_byte_off);

For the above 9 lines, we'd better add a condition check here to make code more clear:
	if (oob_required) {

		....

	}

thanks
Huang Shijie
> +
> +	return 0;
> +}
> +
> +static int gpmi_ecc_write_page_raw(struct mtd_info *mtd,
> +				   struct nand_chip *chip,
> +				   const uint8_t *buf,
> +				   int oob_required)
> +{
> +	struct gpmi_nand_data *this = chip->priv;
> +	struct bch_geometry *nfc_geo = &this->bch_geometry;
> +	int eccsize = nfc_geo->ecc_chunk_size;
> +	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
> +	u8 *tmp_buf = this->raw_buffer;
> +	uint8_t *oob = chip->oob_poi;
> +	size_t dst_bit_off;
> +	size_t oob_bit_off;
> +	size_t oob_byte_off;
> +	int step;
> +
> +	if (!buf || !oob_required)
> +		memset(tmp_buf, 0xff, mtd->writesize + mtd->oobsize);
> +
> +	memcpy(tmp_buf, oob, nfc_geo->metadata_size);
> +	oob_bit_off = nfc_geo->metadata_size * 8;
> +	dst_bit_off = oob_bit_off;
> +
> +	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
> +		if (buf)
> +			gpmi_move_bits(tmp_buf, dst_bit_off,
> +				       buf, step * eccsize * 8, eccsize * 8);
> +		dst_bit_off += eccsize * 8;
> +
> +		/* Pad last ECC block to align following data on a byte */
> +		if (step == nfc_geo->ecc_chunk_count - 1 &&
> +		    (oob_bit_off + eccbits) % 8)
> +			eccbits += 8 - ((oob_bit_off + eccbits) % 8);
> +
> +		if (oob_required)
> +			gpmi_move_bits(tmp_buf, dst_bit_off,
> +				       oob, oob_bit_off, eccbits);
> +
> +		dst_bit_off += eccbits;
> +		oob_bit_off += eccbits;
> +	}
> +
> +	oob_byte_off = oob_bit_off / 8;
> +
> +	if (oob_required && oob_byte_off < mtd->oobsize)
> +		memcpy(tmp_buf + mtd->writesize + oob_byte_off,
> +		       oob + oob_byte_off, mtd->oobsize - oob_byte_off);
> +
> +	if (this->swap_block_mark) {
> +		u8 swap = tmp_buf[0];
> +
> +		tmp_buf[0] = tmp_buf[mtd->writesize];
> +		tmp_buf[mtd->writesize] = swap;
> +	}
> +
> +	chip->write_buf(mtd, tmp_buf, mtd->writesize + mtd->oobsize);
> +
> +	return 0;
> +}
> +
>  static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  {
>  	struct nand_chip *chip = mtd->priv;
> @@ -1664,6 +1788,8 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
>  	ecc->write_page	= gpmi_ecc_write_page;
>  	ecc->read_oob	= gpmi_ecc_read_oob;
>  	ecc->write_oob	= gpmi_ecc_write_oob;
> +	ecc->read_page_raw = gpmi_ecc_read_page_raw;
> +	ecc->write_page_raw = gpmi_ecc_write_page_raw;
>  	ecc->mode	= NAND_ECC_HW;
>  	ecc->size	= bch_geo->ecc_chunk_size;
>  	ecc->strength	= bch_geo->ecc_strength;
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index 17d0736..89ab5c8 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> @@ -189,6 +189,8 @@ struct gpmi_nand_data {
>  	void			*auxiliary_virt;
>  	dma_addr_t		auxiliary_phys;
>  
> +	void			*raw_buffer;
> +
>  	/* DMA channels */
>  #define DMA_CHANS		8
>  	struct dma_chan		*dma_chans[DMA_CHANS];
> -- 
> 1.9.1
> 

[PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support

[Boris BREZILLON]

On Tue, 23 Sep 2014 23:17:41 +0800
Huang Shijie <shijie8@gmail.com> wrote:

> On Tue, Sep 23, 2014 at 04:07:35PM +0200, Boris BREZILLON wrote:
> > Several MTD users (either in user or kernel space) expect a valid raw
> > access support to NAND chip devices.
> > This is particularly true for testing tools which are often touching the
> > data stored in a NAND chip in raw mode to artificially generate errors.
> > 
> > The GPMI drivers do not implemenent raw access functions, and thus rely on
> > default HW_ECC scheme implementation.
> > The default implementation consider the data and OOB area as properly
> > separated in their respective NAND section, which is not true for the GPMI
> > controller.
> > In this driver/controller some OOB data are stored at the beginning of the
> > NAND data area (these data are called metadata in the driver), then ECC
> > bytes are interleaved with data chunk (which is similar to the
> > HW_ECC_SYNDROME scheme), and eventually the remaining bytes are used as
> > OOB data.
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 126 +++++++++++++++++++++++++++++++++
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   2 +
> >  2 files changed, 128 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > index 959cb9b..7921ba7 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -791,6 +791,7 @@ static void gpmi_free_dma_buffer(struct gpmi_nand_data *this)
> >  					this->page_buffer_phys);
> >  	kfree(this->cmd_buffer);
> >  	kfree(this->data_buffer_dma);
> > +	kfree(this->raw_buffer);
> >  
> >  	this->cmd_buffer	= NULL;
> >  	this->data_buffer_dma	= NULL;
> > @@ -837,6 +838,9 @@ static int gpmi_alloc_dma_buffer(struct gpmi_nand_data *this)
> >  	if (!this->page_buffer_virt)
> >  		goto error_alloc;
> >  
> > +	this->raw_buffer = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
> why add this buffer?

I don't why, but I experienced memory corruptions (triggering kernel
panics) when using the page_buffer_virt, even though I had resized it
according to the NAND writesize and oobsize (see my previous version).

Do you see anything that could generate an overflow ?

It seems to work when I allocate my own buffer...

> 
> did you meet some data overlapped?
> 
> 
> > +	if (!this->raw_buffer)
> > +		goto error_alloc;
> >  
> >  	/* Slice up the page buffer. */
> >  	this->payload_virt = this->page_buffer_virt;
> > @@ -1347,6 +1351,126 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
> >  	return status & NAND_STATUS_FAIL ? -EIO : 0;
> >  }
> >  
> > +static int gpmi_ecc_read_page_raw(struct mtd_info *mtd,
> > +				  struct nand_chip *chip, uint8_t *buf,
> > +				  int oob_required, int page)
> > +{
> > +	struct gpmi_nand_data *this = chip->priv;
> > +	struct bch_geometry *nfc_geo = &this->bch_geometry;
> > +	int eccsize = nfc_geo->ecc_chunk_size;
> > +	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
> > +	u8 *tmp_buf = this->raw_buffer;
> > +	size_t src_bit_off;
> > +	size_t oob_bit_off;
> > +	size_t oob_byte_off;
> > +	uint8_t *oob = chip->oob_poi;
> > +	int step;
> > +
> > +	chip->read_buf(mtd, tmp_buf,
> > +		       mtd->writesize + mtd->oobsize);
> > +
> > +	if (this->swap_block_mark) {
> > +		u8 swap = tmp_buf[0];
> > +
> > +		tmp_buf[0] = tmp_buf[mtd->writesize];
> > +		tmp_buf[mtd->writesize] = swap;
> > +	}
> > +
> > +	if (oob_required)
> > +		memcpy(oob, tmp_buf, nfc_geo->metadata_size);
> > +
> > +	oob_bit_off = nfc_geo->metadata_size * 8;
> > +	src_bit_off = oob_bit_off;
> > +
> > +	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
> > +		if (buf)
> 	could this @buf become NULL?

It's just a check to later support raw OOB accesses (see patch
3) ;-).

> 
> 
> > +			gpmi_move_bits(buf, step * eccsize * 8,
> > +				       tmp_buf, src_bit_off,
> > +				       eccsize * 8);
> > +		src_bit_off += eccsize * 8;
> > +
> > +		if (oob_required)
> > +			gpmi_move_bits(oob, oob_bit_off,
> > +				       tmp_buf, src_bit_off,
> > +				       eccbits);
> > +
> > +		src_bit_off += eccbits;
> > +		oob_bit_off += eccbits;
> > +	}
> > +
> > +	if (oob_required && oob_bit_off % 8)
> > +		oob[oob_bit_off / 8] &= GENMASK(oob_bit_off - 1, 0);
> > +
> > +	oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8);
> > +
> > +	if (oob_required && oob_byte_off  < mtd->oobsize)
> > +		memcpy(oob + oob_byte_off,
> > +		       tmp_buf + mtd->writesize + oob_byte_off,
> > +		       mtd->oobsize - oob_byte_off);
> 
> For the above 9 lines, we'd better add a condition check here to make code more clear:
> 	if (oob_required) {
> 
> 		....
> 
> 	}

Absolutely, I'll change that.

Thanks,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support

[Huang Shijie]

On Tue, Sep 23, 2014 at 04:07:35PM +0200, Boris BREZILLON wrote:
> Several MTD users (either in user or kernel space) expect a valid raw
> access support to NAND chip devices.
> This is particularly true for testing tools which are often touching the
> data stored in a NAND chip in raw mode to artificially generate errors.
> 
> The GPMI drivers do not implemenent raw access functions, and thus rely on
> default HW_ECC scheme implementation.
> The default implementation consider the data and OOB area as properly
> separated in their respective NAND section, which is not true for the GPMI
> controller.
> In this driver/controller some OOB data are stored at the beginning of the
> NAND data area (these data are called metadata in the driver), then ECC
> bytes are interleaved with data chunk (which is similar to the
> HW_ECC_SYNDROME scheme), and eventually the remaining bytes are used as
> OOB data.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 126 +++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   2 +
>  2 files changed, 128 insertions(+)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 959cb9b..7921ba7 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -791,6 +791,7 @@ static void gpmi_free_dma_buffer(struct gpmi_nand_data *this)
>  					this->page_buffer_phys);
>  	kfree(this->cmd_buffer);
>  	kfree(this->data_buffer_dma);
> +	kfree(this->raw_buffer);
>  
>  	this->cmd_buffer	= NULL;
>  	this->data_buffer_dma	= NULL;
> @@ -837,6 +838,9 @@ static int gpmi_alloc_dma_buffer(struct gpmi_nand_data *this)
>  	if (!this->page_buffer_virt)
>  		goto error_alloc;
>  
> +	this->raw_buffer = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
> +	if (!this->raw_buffer)
> +		goto error_alloc;
>  
>  	/* Slice up the page buffer. */
>  	this->payload_virt = this->page_buffer_virt;
> @@ -1347,6 +1351,126 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
>  	return status & NAND_STATUS_FAIL ? -EIO : 0;
>  }
>  
> +static int gpmi_ecc_read_page_raw(struct mtd_info *mtd,
> +				  struct nand_chip *chip, uint8_t *buf,
> +				  int oob_required, int page)

	In actually, i suggest to call the ecc->read_page() in this
	hook. And after the ecc->read_page(), copy the relative data to
	the relative buffers. Is my suggestion right? please correct me.



> +{
> +	struct gpmi_nand_data *this = chip->priv;
> +	struct bch_geometry *nfc_geo = &this->bch_geometry;
> +	int eccsize = nfc_geo->ecc_chunk_size;
> +	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
> +	u8 *tmp_buf = this->raw_buffer;
> +	size_t src_bit_off;
> +	size_t oob_bit_off;
> +	size_t oob_byte_off;
> +	uint8_t *oob = chip->oob_poi;
> +	int step;
> +
> +	chip->read_buf(mtd, tmp_buf,
> +		       mtd->writesize + mtd->oobsize);
> +
> +	if (this->swap_block_mark) {
> +		u8 swap = tmp_buf[0];
> +
> +		tmp_buf[0] = tmp_buf[mtd->writesize];
> +		tmp_buf[mtd->writesize] = swap;
> +	}
> +
> +	if (oob_required)
> +		memcpy(oob, tmp_buf, nfc_geo->metadata_size);
> +
> +	oob_bit_off = nfc_geo->metadata_size * 8;
> +	src_bit_off = oob_bit_off;
> +
> +	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
> +		if (buf)
> +			gpmi_move_bits(buf, step * eccsize * 8,
> +				       tmp_buf, src_bit_off,
> +				       eccsize * 8);
> +		src_bit_off += eccsize * 8;
> +
> +		if (oob_required)
> +			gpmi_move_bits(oob, oob_bit_off,
> +				       tmp_buf, src_bit_off,
> +				       eccbits);
> +
> +		src_bit_off += eccbits;
> +		oob_bit_off += eccbits;
> +	}
> +
> +	if (oob_required && oob_bit_off % 8)
> +		oob[oob_bit_off / 8] &= GENMASK(oob_bit_off - 1, 0);
> +
> +	oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8);
> +
> +	if (oob_required && oob_byte_off  < mtd->oobsize)
> +		memcpy(oob + oob_byte_off,
> +		       tmp_buf + mtd->writesize + oob_byte_off,
> +		       mtd->oobsize - oob_byte_off);
> +
> +	return 0;
> +}
> +
> +static int gpmi_ecc_write_page_raw(struct mtd_info *mtd,
> +				   struct nand_chip *chip,
> +				   const uint8_t *buf,
> +				   int oob_required)
> +{


give me more time to think over this hook :(

sorry.

thanks
Huang Shijie
> +	struct gpmi_nand_data *this = chip->priv;
> +	struct bch_geometry *nfc_geo = &this->bch_geometry;
> +	int eccsize = nfc_geo->ecc_chunk_size;
> +	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
> +	u8 *tmp_buf = this->raw_buffer;
> +	uint8_t *oob = chip->oob_poi;
> +	size_t dst_bit_off;
> +	size_t oob_bit_off;
> +	size_t oob_byte_off;
> +	int step;
> +
> +	if (!buf || !oob_required)
> +		memset(tmp_buf, 0xff, mtd->writesize + mtd->oobsize);
> +
> +	memcpy(tmp_buf, oob, nfc_geo->metadata_size);
> +	oob_bit_off = nfc_geo->metadata_size * 8;
> +	dst_bit_off = oob_bit_off;
> +
> +	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
> +		if (buf)
> +			gpmi_move_bits(tmp_buf, dst_bit_off,
> +				       buf, step * eccsize * 8, eccsize * 8);
> +		dst_bit_off += eccsize * 8;
> +
> +		/* Pad last ECC block to align following data on a byte */
> +		if (step == nfc_geo->ecc_chunk_count - 1 &&
> +		    (oob_bit_off + eccbits) % 8)
> +			eccbits += 8 - ((oob_bit_off + eccbits) % 8);
> +
> +		if (oob_required)
> +			gpmi_move_bits(tmp_buf, dst_bit_off,
> +				       oob, oob_bit_off, eccbits);
> +
> +		dst_bit_off += eccbits;
> +		oob_bit_off += eccbits;
> +	}
> +
> +	oob_byte_off = oob_bit_off / 8;
> +
> +	if (oob_required && oob_byte_off < mtd->oobsize)
> +		memcpy(tmp_buf + mtd->writesize + oob_byte_off,
> +		       oob + oob_byte_off, mtd->oobsize - oob_byte_off);
> +
> +	if (this->swap_block_mark) {
> +		u8 swap = tmp_buf[0];
> +
> +		tmp_buf[0] = tmp_buf[mtd->writesize];
> +		tmp_buf[mtd->writesize] = swap;
> +	}
> +
> +	chip->write_buf(mtd, tmp_buf, mtd->writesize + mtd->oobsize);
> +
> +	return 0;
> +}
> +
>  static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  {
>  	struct nand_chip *chip = mtd->priv;
> @@ -1664,6 +1788,8 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
>  	ecc->write_page	= gpmi_ecc_write_page;
>  	ecc->read_oob	= gpmi_ecc_read_oob;
>  	ecc->write_oob	= gpmi_ecc_write_oob;
> +	ecc->read_page_raw = gpmi_ecc_read_page_raw;
> +	ecc->write_page_raw = gpmi_ecc_write_page_raw;
>  	ecc->mode	= NAND_ECC_HW;
>  	ecc->size	= bch_geo->ecc_chunk_size;
>  	ecc->strength	= bch_geo->ecc_strength;
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index 17d0736..89ab5c8 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> @@ -189,6 +189,8 @@ struct gpmi_nand_data {
>  	void			*auxiliary_virt;
>  	dma_addr_t		auxiliary_phys;
>  
> +	void			*raw_buffer;
> +
>  	/* DMA channels */
>  #define DMA_CHANS		8
>  	struct dma_chan		*dma_chans[DMA_CHANS];
> -- 
> 1.9.1
> 

[PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support

[Boris BREZILLON]

On Wed, 24 Sep 2014 00:10:44 +0800
Huang Shijie <shijie8@gmail.com> wrote:

> On Tue, Sep 23, 2014 at 04:07:35PM +0200, Boris BREZILLON wrote:
> > Several MTD users (either in user or kernel space) expect a valid raw
> > access support to NAND chip devices.
> > This is particularly true for testing tools which are often touching the
> > data stored in a NAND chip in raw mode to artificially generate errors.
> > 
> > The GPMI drivers do not implemenent raw access functions, and thus rely on
> > default HW_ECC scheme implementation.
> > The default implementation consider the data and OOB area as properly
> > separated in their respective NAND section, which is not true for the GPMI
> > controller.
> > In this driver/controller some OOB data are stored at the beginning of the
> > NAND data area (these data are called metadata in the driver), then ECC
> > bytes are interleaved with data chunk (which is similar to the
> > HW_ECC_SYNDROME scheme), and eventually the remaining bytes are used as
> > OOB data.
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 126 +++++++++++++++++++++++++++++++++
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   2 +
> >  2 files changed, 128 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > index 959cb9b..7921ba7 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -791,6 +791,7 @@ static void gpmi_free_dma_buffer(struct gpmi_nand_data *this)
> >  					this->page_buffer_phys);
> >  	kfree(this->cmd_buffer);
> >  	kfree(this->data_buffer_dma);
> > +	kfree(this->raw_buffer);
> >  
> >  	this->cmd_buffer	= NULL;
> >  	this->data_buffer_dma	= NULL;
> > @@ -837,6 +838,9 @@ static int gpmi_alloc_dma_buffer(struct gpmi_nand_data *this)
> >  	if (!this->page_buffer_virt)
> >  		goto error_alloc;
> >  
> > +	this->raw_buffer = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
> > +	if (!this->raw_buffer)
> > +		goto error_alloc;
> >  
> >  	/* Slice up the page buffer. */
> >  	this->payload_virt = this->page_buffer_virt;
> > @@ -1347,6 +1351,126 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
> >  	return status & NAND_STATUS_FAIL ? -EIO : 0;
> >  }
> >  
> > +static int gpmi_ecc_read_page_raw(struct mtd_info *mtd,
> > +				  struct nand_chip *chip, uint8_t *buf,
> > +				  int oob_required, int page)
> 
> 	In actually, i suggest to call the ecc->read_page() in this
> 	hook. And after the ecc->read_page(), copy the relative data to
> 	the relative buffers. Is my suggestion right? please correct me.

Unfortunately it's not. The user expect that ECC correction is not
involved when accessing the NAND in raw mode, which is not the case in
your read_page implementation.
This is particularly useful when one want to see the real page status
(including bitflips).

Moreover, I like to see the generated ECC bytes/bits when using raw
access (but I'm not sure this is a requirement). This will help
deducing the BCH algorithm and/or XOR mask applied after producing
these bits if someone ever want to spend some time reverse engineering
it.

> 
> 
> 
> > +{
> > +	struct gpmi_nand_data *this = chip->priv;
> > +	struct bch_geometry *nfc_geo = &this->bch_geometry;
> > +	int eccsize = nfc_geo->ecc_chunk_size;
> > +	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
> > +	u8 *tmp_buf = this->raw_buffer;
> > +	size_t src_bit_off;
> > +	size_t oob_bit_off;
> > +	size_t oob_byte_off;
> > +	uint8_t *oob = chip->oob_poi;
> > +	int step;
> > +
> > +	chip->read_buf(mtd, tmp_buf,
> > +		       mtd->writesize + mtd->oobsize);
> > +
> > +	if (this->swap_block_mark) {
> > +		u8 swap = tmp_buf[0];
> > +
> > +		tmp_buf[0] = tmp_buf[mtd->writesize];
> > +		tmp_buf[mtd->writesize] = swap;
> > +	}
> > +
> > +	if (oob_required)
> > +		memcpy(oob, tmp_buf, nfc_geo->metadata_size);
> > +
> > +	oob_bit_off = nfc_geo->metadata_size * 8;
> > +	src_bit_off = oob_bit_off;
> > +
> > +	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
> > +		if (buf)
> > +			gpmi_move_bits(buf, step * eccsize * 8,
> > +				       tmp_buf, src_bit_off,
> > +				       eccsize * 8);
> > +		src_bit_off += eccsize * 8;
> > +
> > +		if (oob_required)
> > +			gpmi_move_bits(oob, oob_bit_off,
> > +				       tmp_buf, src_bit_off,
> > +				       eccbits);
> > +
> > +		src_bit_off += eccbits;
> > +		oob_bit_off += eccbits;
> > +	}
> > +
> > +	if (oob_required && oob_bit_off % 8)
> > +		oob[oob_bit_off / 8] &= GENMASK(oob_bit_off - 1, 0);
> > +
> > +	oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8);
> > +
> > +	if (oob_required && oob_byte_off  < mtd->oobsize)
> > +		memcpy(oob + oob_byte_off,
> > +		       tmp_buf + mtd->writesize + oob_byte_off,
> > +		       mtd->oobsize - oob_byte_off);
> > +
> > +	return 0;
> > +}
> > +
> > +static int gpmi_ecc_write_page_raw(struct mtd_info *mtd,
> > +				   struct nand_chip *chip,
> > +				   const uint8_t *buf,
> > +				   int oob_required)
> > +{
> 
> 
> give me more time to think over this hook :(

Sure, no problem.

Actually it's doing the exact same thing read_page_raw is doing expect
it's applying on a write access.
I copy the provided data (in-band and out-of-band is required) into a
temporary buffer to match the GPMI controller layout, then write it
without involving the BCH block (which means no ECC bits generation).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support

[Boris BREZILLON]

On Tue, 23 Sep 2014 19:16:19 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> On Wed, 24 Sep 2014 00:10:44 +0800
> Huang Shijie <shijie8@gmail.com> wrote:
> 
> > On Tue, Sep 23, 2014 at 04:07:35PM +0200, Boris BREZILLON wrote:
> > > Several MTD users (either in user or kernel space) expect a valid raw
> > > access support to NAND chip devices.
> > > This is particularly true for testing tools which are often touching the
> > > data stored in a NAND chip in raw mode to artificially generate errors.
> > > 
> > > The GPMI drivers do not implemenent raw access functions, and thus rely on
> > > default HW_ECC scheme implementation.
> > > The default implementation consider the data and OOB area as properly
> > > separated in their respective NAND section, which is not true for the GPMI
> > > controller.
> > > In this driver/controller some OOB data are stored at the beginning of the
> > > NAND data area (these data are called metadata in the driver), then ECC
> > > bytes are interleaved with data chunk (which is similar to the
> > > HW_ECC_SYNDROME scheme), and eventually the remaining bytes are used as
> > > OOB data.
> > > 
> > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > ---
> > >  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 126 +++++++++++++++++++++++++++++++++
> > >  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   2 +
> > >  2 files changed, 128 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > > index 959cb9b..7921ba7 100644
> > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > > @@ -791,6 +791,7 @@ static void gpmi_free_dma_buffer(struct gpmi_nand_data *this)
> > >  					this->page_buffer_phys);
> > >  	kfree(this->cmd_buffer);
> > >  	kfree(this->data_buffer_dma);
> > > +	kfree(this->raw_buffer);
> > >  
> > >  	this->cmd_buffer	= NULL;
> > >  	this->data_buffer_dma	= NULL;
> > > @@ -837,6 +838,9 @@ static int gpmi_alloc_dma_buffer(struct gpmi_nand_data *this)
> > >  	if (!this->page_buffer_virt)
> > >  		goto error_alloc;
> > >  
> > > +	this->raw_buffer = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
> > > +	if (!this->raw_buffer)
> > > +		goto error_alloc;
> > >  
> > >  	/* Slice up the page buffer. */
> > >  	this->payload_virt = this->page_buffer_virt;
> > > @@ -1347,6 +1351,126 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
> > >  	return status & NAND_STATUS_FAIL ? -EIO : 0;
> > >  }
> > >  
> > > +static int gpmi_ecc_read_page_raw(struct mtd_info *mtd,
> > > +				  struct nand_chip *chip, uint8_t *buf,
> > > +				  int oob_required, int page)
> > 
> > 	In actually, i suggest to call the ecc->read_page() in this
> > 	hook. And after the ecc->read_page(), copy the relative data to
> > 	the relative buffers. Is my suggestion right? please correct me.
> 
> Unfortunately it's not. The user expect that ECC correction is not
> involved when accessing the NAND in raw mode, which is not the case in
> your read_page implementation.
> This is particularly useful when one want to see the real page status
> (including bitflips).
> 
> Moreover, I like to see the generated ECC bytes/bits when using raw
> access (but I'm not sure this is a requirement). This will help
> deducing the BCH algorithm and/or XOR mask applied after producing
> these bits if someone ever want to spend some time reverse engineering
> it.
> 
> > 
> > 
> > 
> > > +{
> > > +	struct gpmi_nand_data *this = chip->priv;
> > > +	struct bch_geometry *nfc_geo = &this->bch_geometry;
> > > +	int eccsize = nfc_geo->ecc_chunk_size;
> > > +	int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len;
> > > +	u8 *tmp_buf = this->raw_buffer;
> > > +	size_t src_bit_off;
> > > +	size_t oob_bit_off;
> > > +	size_t oob_byte_off;
> > > +	uint8_t *oob = chip->oob_poi;
> > > +	int step;
> > > +
> > > +	chip->read_buf(mtd, tmp_buf,
> > > +		       mtd->writesize + mtd->oobsize);
> > > +
> > > +	if (this->swap_block_mark) {
> > > +		u8 swap = tmp_buf[0];
> > > +
> > > +		tmp_buf[0] = tmp_buf[mtd->writesize];
> > > +		tmp_buf[mtd->writesize] = swap;
> > > +	}
> > > +
> > > +	if (oob_required)
> > > +		memcpy(oob, tmp_buf, nfc_geo->metadata_size);
> > > +
> > > +	oob_bit_off = nfc_geo->metadata_size * 8;
> > > +	src_bit_off = oob_bit_off;
> > > +
> > > +	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
> > > +		if (buf)
> > > +			gpmi_move_bits(buf, step * eccsize * 8,
> > > +				       tmp_buf, src_bit_off,
> > > +				       eccsize * 8);
> > > +		src_bit_off += eccsize * 8;
> > > +
> > > +		if (oob_required)
> > > +			gpmi_move_bits(oob, oob_bit_off,
> > > +				       tmp_buf, src_bit_off,
> > > +				       eccbits);
> > > +
> > > +		src_bit_off += eccbits;
> > > +		oob_bit_off += eccbits;
> > > +	}
> > > +
> > > +	if (oob_required && oob_bit_off % 8)
> > > +		oob[oob_bit_off / 8] &= GENMASK(oob_bit_off - 1, 0);
> > > +
> > > +	oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8);
> > > +
> > > +	if (oob_required && oob_byte_off  < mtd->oobsize)
> > > +		memcpy(oob + oob_byte_off,
> > > +		       tmp_buf + mtd->writesize + oob_byte_off,
> > > +		       mtd->oobsize - oob_byte_off);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int gpmi_ecc_write_page_raw(struct mtd_info *mtd,
> > > +				   struct nand_chip *chip,
> > > +				   const uint8_t *buf,
> > > +				   int oob_required)
> > > +{
> > 
> > 
> > give me more time to think over this hook :(
> 
> Sure, no problem.
> 
> Actually it's doing the exact same thing read_page_raw is doing expect
> it's applying on a write access.
> I copy the provided data (in-band and out-of-band is required) into a
						    ^ if

> temporary buffer to match the GPMI controller layout, then write it
> without involving the BCH block (which means no ECC bits generation).
> 
> Best Regards,
> 
> Boris
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 3/3] mtd: nand: gpmi: add raw oob access functions

[Boris BREZILLON]

Implement raw OOB access functions to retrieve OOB bytes when accessing the
NAND in raw mode.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 7921ba7..850356a 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1471,6 +1471,22 @@ static int gpmi_ecc_write_page_raw(struct mtd_info *mtd,
 	return 0;
 }
 
+static int gpmi_ecc_read_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+				 int page)
+{
+	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
+
+	return gpmi_ecc_read_page_raw(mtd, chip, NULL, 1, page);
+}
+
+static int gpmi_ecc_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+				 int page)
+{
+	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0, page);
+
+	return gpmi_ecc_write_page_raw(mtd, chip, NULL, 1);
+}
+
 static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
 {
 	struct nand_chip *chip = mtd->priv;
@@ -1790,6 +1806,8 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
 	ecc->write_oob	= gpmi_ecc_write_oob;
 	ecc->read_page_raw = gpmi_ecc_read_page_raw;
 	ecc->write_page_raw = gpmi_ecc_write_page_raw;
+	ecc->read_oob_raw = gpmi_ecc_read_oob_raw;
+	ecc->write_oob_raw = gpmi_ecc_write_oob_raw;
 	ecc->mode	= NAND_ECC_HW;
 	ecc->size	= bch_geo->ecc_chunk_size;
 	ecc->strength	= bch_geo->ecc_strength;
-- 
1.9.1

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Boris Brezillon]

Hi,

On Tue, 23 Sep 2014 16:07:33 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> Hello Huang, Brian,
> 
> This is just a new proposal to support raw accesses in a more standard way
> in the GPMI driver.
> This series has been tested on an imx28 board.
> 
> Any suggestions are welcome.

Brian, any chance you could take a look at this series and give your
opinion and/or suggest a new approach ?

Huang, did you have time to think about a better way to implement these
raw access functions ?

Best Regards,

Boris
-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Huang Shijie]

On Tue, Sep 30, 2014 at 10:07:20AM +0200, Boris Brezillon wrote:
> Hi,
> 
> On Tue, 23 Sep 2014 16:07:33 +0200
> Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:
> 
> > Hello Huang, Brian,
> > 
> > This is just a new proposal to support raw accesses in a more standard way
> > in the GPMI driver.
> > This series has been tested on an imx28 board.
> > 
> > Any suggestions are welcome.
> 
> Brian, any chance you could take a look at this series and give your
> opinion and/or suggest a new approach ?
> 
> Huang, did you have time to think about a better way to implement these
> raw access functions ?
> 
sorry for the later reply. I was busy recently.
You can send out the new version. Please do not forget add a patch to
add
more comments for these hooks.

I will test your patch(or your new patch set) on Oct 7 and Oct 8.

thanks
Huang Shijie

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Huang Shijie]

On Tue, Sep 23, 2014 at 04:07:33PM +0200, Boris BREZILLON wrote:
> Hello Huang, Brian,
> 
> This is just a new proposal to support raw accesses in a more standard way
> in the GPMI driver.
> This series has been tested on an imx28 board.
> 
> Any suggestions are welcome.
> 
> Best Regards,
> 
> Boris
> 
> Changes since v2:
>  - fixed a bug in gpmi_move_bits
>  - add a raw_buffer field to be used when using raw access methods
>    (experienced memory corruptions when directly using page_buffer_virt
>    buffer)
>  - add raw OOB access functions

I tested this patch set today with the imx6dl-sabreauto board.

    NAND: Micron MT29F64G08CBABAWP
	8192MiB, MLC, page size: 8192, OOB size: 744

    ECC: 40bit

The result:

[ 3672.779009] ==================================================
[ 3672.784974] mtd_nandbiterrs: MTD device: 0
[ 3672.789480] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
[ 3672.798169] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
[ 3672.804554] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
[ 3672.812497] mtd_nandbiterrs: incremental biterrors test
[ 3672.818688] mtd_nandbiterrs: write_page
[ 3672.825529] mtd_nandbiterrs: rewrite page
[ 3672.837290] mtd_nandbiterrs: read_page
[ 3672.848407] mtd_nandbiterrs: error: read failed at 0x0
[ 3672.853644] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
[ 3672.862932] mtd_nandbiterrs: finished successfully.
[ 3672.867837] ==================================================

[ 3745.282368] ==================================================
[ 3745.288227] mtd_nandbiterrs: MTD device: 0
[ 3745.292913] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
[ 3745.301897] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
[ 3745.308023] mtd_nandbiterrs: Using page=1, offset=8192, eraseblock=0
[ 3745.316778] mtd_nandbiterrs: incremental biterrors test
[ 3745.323017] mtd_nandbiterrs: write_page
[ 3745.328616] mtd_nandbiterrs: rewrite page
[ 3745.334191] mtd_nandbiterrs: read_page
[ 3745.346878] mtd_nandbiterrs: error: read failed at 0x2000
[ 3745.352352] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
[ 3745.361281] mtd_nandbiterrs: finished successfully.
[ 3745.366173] ==================================================


Is this okay? 

thanks
Huang Shijie

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Boris Brezillon]

On Wed, 8 Oct 2014 22:24:40 +0800
Huang Shijie <shijie8@gmail.com> wrote:

> On Tue, Sep 23, 2014 at 04:07:33PM +0200, Boris BREZILLON wrote:
> > Hello Huang, Brian,
> > 
> > This is just a new proposal to support raw accesses in a more standard way
> > in the GPMI driver.
> > This series has been tested on an imx28 board.
> > 
> > Any suggestions are welcome.
> > 
> > Best Regards,
> > 
> > Boris
> > 
> > Changes since v2:
> >  - fixed a bug in gpmi_move_bits
> >  - add a raw_buffer field to be used when using raw access methods
> >    (experienced memory corruptions when directly using page_buffer_virt
> >    buffer)
> >  - add raw OOB access functions
> 
> I tested this patch set today with the imx6dl-sabreauto board.
> 
>     NAND: Micron MT29F64G08CBABAWP
> 	8192MiB, MLC, page size: 8192, OOB size: 744
> 
>     ECC: 40bit
> 
> The result:
> 
> [ 3672.779009] ==================================================
> [ 3672.784974] mtd_nandbiterrs: MTD device: 0
> [ 3672.789480] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
> [ 3672.798169] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
> [ 3672.804554] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
> [ 3672.812497] mtd_nandbiterrs: incremental biterrors test
> [ 3672.818688] mtd_nandbiterrs: write_page
> [ 3672.825529] mtd_nandbiterrs: rewrite page
> [ 3672.837290] mtd_nandbiterrs: read_page
> [ 3672.848407] mtd_nandbiterrs: error: read failed at 0x0
> [ 3672.853644] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
> [ 3672.862932] mtd_nandbiterrs: finished successfully.
> [ 3672.867837] ==================================================
> 
> [ 3745.282368] ==================================================
> [ 3745.288227] mtd_nandbiterrs: MTD device: 0
> [ 3745.292913] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
> [ 3745.301897] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
> [ 3745.308023] mtd_nandbiterrs: Using page=1, offset=8192, eraseblock=0
> [ 3745.316778] mtd_nandbiterrs: incremental biterrors test
> [ 3745.323017] mtd_nandbiterrs: write_page
> [ 3745.328616] mtd_nandbiterrs: rewrite page
> [ 3745.334191] mtd_nandbiterrs: read_page
> [ 3745.346878] mtd_nandbiterrs: error: read failed at 0x2000
> [ 3745.352352] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
> [ 3745.361281] mtd_nandbiterrs: finished successfully.
> [ 3745.366173] ==================================================
> 
> 
> Is this okay?

No, it doesn't seem to be correct.
But it's an MLC flash, so you'll most probably need to apply this patch
to nandbiterrs testsuite:

http://code.bulix.org/f69wuu-87021

This patch is flashing the block between each bitflip insertion to
avoid multiple write without erasure (which, AFAIK, is not supported
by MLC flashes).

Can you also try to write zeros to a NAND page and then dump it in
raw mode (and provide me with the resulted /tmp/dump file or an
hexdump output of this file) ?

flash_erase /dev/mtdX 0 1
dd if=/dev/zero of=/tmp/zero bs=8k count=1
nandwrite /dev/mtdX /tmp/zero
nanddump -n -o -l 8192 -f /tmp/dump /dev/mtdX 

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Huang Shijie]

On Wed, Oct 08, 2014 at 05:10:34PM +0200, Boris Brezillon wrote:
> On Wed, 8 Oct 2014 22:24:40 +0800
> Huang Shijie <shijie8@gmail.com> wrote:
> 
> > On Tue, Sep 23, 2014 at 04:07:33PM +0200, Boris BREZILLON wrote:
> > > Hello Huang, Brian,
> > > 
> > > This is just a new proposal to support raw accesses in a more standard way
> > > in the GPMI driver.
> > > This series has been tested on an imx28 board.
> > > 
> > > Any suggestions are welcome.
> > > 
> > > Best Regards,
> > > 
> > > Boris
> > > 
> > > Changes since v2:
> > >  - fixed a bug in gpmi_move_bits
> > >  - add a raw_buffer field to be used when using raw access methods
> > >    (experienced memory corruptions when directly using page_buffer_virt
> > >    buffer)
> > >  - add raw OOB access functions
> > 
> > I tested this patch set today with the imx6dl-sabreauto board.
> > 
> >     NAND: Micron MT29F64G08CBABAWP
> > 	8192MiB, MLC, page size: 8192, OOB size: 744
> > 
> >     ECC: 40bit
> > 
> > The result:
> > 
> > [ 3672.779009] ==================================================
> > [ 3672.784974] mtd_nandbiterrs: MTD device: 0
> > [ 3672.789480] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
> > [ 3672.798169] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
> > [ 3672.804554] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
> > [ 3672.812497] mtd_nandbiterrs: incremental biterrors test
> > [ 3672.818688] mtd_nandbiterrs: write_page
> > [ 3672.825529] mtd_nandbiterrs: rewrite page
> > [ 3672.837290] mtd_nandbiterrs: read_page
> > [ 3672.848407] mtd_nandbiterrs: error: read failed at 0x0
> > [ 3672.853644] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
> > [ 3672.862932] mtd_nandbiterrs: finished successfully.
> > [ 3672.867837] ==================================================
> > 
> > [ 3745.282368] ==================================================
> > [ 3745.288227] mtd_nandbiterrs: MTD device: 0
> > [ 3745.292913] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
> > [ 3745.301897] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
> > [ 3745.308023] mtd_nandbiterrs: Using page=1, offset=8192, eraseblock=0
> > [ 3745.316778] mtd_nandbiterrs: incremental biterrors test
> > [ 3745.323017] mtd_nandbiterrs: write_page
> > [ 3745.328616] mtd_nandbiterrs: rewrite page
> > [ 3745.334191] mtd_nandbiterrs: read_page
> > [ 3745.346878] mtd_nandbiterrs: error: read failed at 0x2000
> > [ 3745.352352] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
> > [ 3745.361281] mtd_nandbiterrs: finished successfully.
> > [ 3745.366173] ==================================================
> > 
> > 
> > Is this okay?
> 
> No, it doesn't seem to be correct.
> But it's an MLC flash, so you'll most probably need to apply this patch
> to nandbiterrs testsuite:
> 
> http://code.bulix.org/f69wuu-87021
> 
> This patch is flashing the block between each bitflip insertion to
> avoid multiple write without erasure (which, AFAIK, is not supported
> by MLC flashes).
After I applied this patch. It seems ok now.

The test result:
[  244.789900] ==================================================
[  244.795826] mtd_nandbiterrs: MTD device: 1
[  244.799974] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
[  244.808563] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
[  244.814697] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
[  244.822622] mtd_nandbiterrs: incremental biterrors test
[  244.828146] mtd_nandbiterrs: write_page
[  244.833341] mtd_nandbiterrs: fill page
[  244.838831] mtd_nandbiterrs: rewrite page
[  244.844353] mtd_nandbiterrs: read_page
[  244.848585] mtd_nandbiterrs: verify_page
[  244.852915] mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage
[  244.859973] mtd_nandbiterrs: Inserted biterror @ 0/5
[  244.866316] mtd_nandbiterrs: rewrite page
[  244.871629] mtd_nandbiterrs: read_page
[  244.875864] mtd_nandbiterrs: verify_page
[  244.880161] mtd_nandbiterrs: Successfully corrected 1 bit errors per subpage
[  244.887247] mtd_nandbiterrs: Inserted biterror @ 0/2
[  244.893499] mtd_nandbiterrs: rewrite page
[  244.898766] mtd_nandbiterrs: read_page
[  244.903071] mtd_nandbiterrs: verify_page
[  244.907369] mtd_nandbiterrs: Successfully corrected 2 bit errors per subpage
[  244.914463] mtd_nandbiterrs: Inserted biterror @ 0/0
[  244.920701] mtd_nandbiterrs: rewrite page
[  244.926109] mtd_nandbiterrs: read_page
[  244.930343] mtd_nandbiterrs: verify_page
[  244.934672] mtd_nandbiterrs: Successfully corrected 3 bit errors per subpage
[  244.941752] mtd_nandbiterrs: Inserted biterror @ 1/7
[  244.947988] mtd_nandbiterrs: rewrite page
[  244.953408] mtd_nandbiterrs: read_page
[  244.957641] mtd_nandbiterrs: verify_page
[  244.961970] mtd_nandbiterrs: Successfully corrected 4 bit errors per subpage
[  244.969029] mtd_nandbiterrs: Inserted biterror @ 1/5
[  244.975357] mtd_nandbiterrs: rewrite page
[  244.980627] mtd_nandbiterrs: read_page
[  244.984931] mtd_nandbiterrs: verify_page
[  244.989229] mtd_nandbiterrs: Successfully corrected 5 bit errors per subpage
[  244.996318] mtd_nandbiterrs: Inserted biterror @ 1/2
[  245.002576] mtd_nandbiterrs: rewrite page
[  245.007843] mtd_nandbiterrs: read_page
[  245.012139] mtd_nandbiterrs: verify_page
[  245.016439] mtd_nandbiterrs: Successfully corrected 6 bit errors per subpage
[  245.023526] mtd_nandbiterrs: Inserted biterror @ 1/0
[  245.029755] mtd_nandbiterrs: rewrite page
[  245.035149] mtd_nandbiterrs: read_page
[  245.039378] mtd_nandbiterrs: verify_page
[  245.043726] mtd_nandbiterrs: Successfully corrected 7 bit errors per subpage
[  245.050787] mtd_nandbiterrs: Inserted biterror @ 2/6
[  245.057123] mtd_nandbiterrs: rewrite page
[  245.062423] mtd_nandbiterrs: read_page
[  245.066654] mtd_nandbiterrs: verify_page
[  245.071001] mtd_nandbiterrs: Successfully corrected 8 bit errors per subpage
[  245.078060] mtd_nandbiterrs: Inserted biterror @ 2/5
[  245.084398] mtd_nandbiterrs: rewrite page
[  245.089673] mtd_nandbiterrs: read_page
[  245.093978] mtd_nandbiterrs: verify_page
[  245.098275] mtd_nandbiterrs: Successfully corrected 9 bit errors per subpage
[  245.105360] mtd_nandbiterrs: Inserted biterror @ 2/2
[  245.111622] mtd_nandbiterrs: rewrite page
[  245.116899] mtd_nandbiterrs: read_page
[  245.121188] mtd_nandbiterrs: verify_page
[  245.125485] mtd_nandbiterrs: Successfully corrected 10 bit errors per subpage
[  245.132658] mtd_nandbiterrs: Inserted biterror @ 2/0
[  245.138895] mtd_nandbiterrs: rewrite page
[  245.144294] mtd_nandbiterrs: read_page
[  245.148524] mtd_nandbiterrs: verify_page
[  245.152853] mtd_nandbiterrs: Successfully corrected 11 bit errors per subpage
[  245.159998] mtd_nandbiterrs: Inserted biterror @ 3/7
[  245.166326] mtd_nandbiterrs: rewrite page
[  245.171620] mtd_nandbiterrs: read_page
[  245.175849] mtd_nandbiterrs: verify_page
[  245.180146] mtd_nandbiterrs: Successfully corrected 12 bit errors per subpage
[  245.187314] mtd_nandbiterrs: Inserted biterror @ 3/6
[  245.193603] mtd_nandbiterrs: rewrite page
[  245.198874] mtd_nandbiterrs: read_page
[  245.203183] mtd_nandbiterrs: verify_page
[  245.207480] mtd_nandbiterrs: Successfully corrected 13 bit errors per subpage
[  245.214657] mtd_nandbiterrs: Inserted biterror @ 3/5
[  245.220958] mtd_nandbiterrs: rewrite page
[  245.226235] mtd_nandbiterrs: read_page
[  245.230462] mtd_nandbiterrs: verify_page
[  245.234787] mtd_nandbiterrs: Successfully corrected 14 bit errors per subpage
[  245.241965] mtd_nandbiterrs: Inserted biterror @ 3/2
[  245.248210] mtd_nandbiterrs: rewrite page
[  245.253614] mtd_nandbiterrs: read_page
[  245.257846] mtd_nandbiterrs: verify_page
[  245.262177] mtd_nandbiterrs: Successfully corrected 15 bit errors per subpage
[  245.269322] mtd_nandbiterrs: Inserted biterror @ 3/0
[  245.275670] mtd_nandbiterrs: rewrite page
[  245.280965] mtd_nandbiterrs: read_page
[  245.285202] mtd_nandbiterrs: verify_page
[  245.289501] mtd_nandbiterrs: Successfully corrected 16 bit errors per subpage
[  245.296677] mtd_nandbiterrs: Inserted biterror @ 4/2
[  245.302952] mtd_nandbiterrs: rewrite page
[  245.308237] mtd_nandbiterrs: read_page
[  245.312548] mtd_nandbiterrs: verify_page
[  245.316846] mtd_nandbiterrs: Successfully corrected 17 bit errors per subpage
[  245.324018] mtd_nandbiterrs: Inserted biterror @ 4/0
[  245.330264] mtd_nandbiterrs: rewrite page
[  245.335672] mtd_nandbiterrs: read_page
[  245.339895] mtd_nandbiterrs: verify_page
[  245.344222] mtd_nandbiterrs: Successfully corrected 18 bit errors per subpage
[  245.351399] mtd_nandbiterrs: Inserted biterror @ 5/7
[  245.357673] mtd_nandbiterrs: rewrite page
[  245.363085] mtd_nandbiterrs: read_page
[  245.367316] mtd_nandbiterrs: verify_page
[  245.371646] mtd_nandbiterrs: Successfully corrected 19 bit errors per subpage
[  245.378792] mtd_nandbiterrs: Inserted biterror @ 5/2
[  245.385136] mtd_nandbiterrs: rewrite page
[  245.390402] mtd_nandbiterrs: read_page
[  245.394704] mtd_nandbiterrs: verify_page
[  245.399002] mtd_nandbiterrs: Successfully corrected 20 bit errors per subpage
[  245.406178] mtd_nandbiterrs: Inserted biterror @ 5/0
[  245.412440] mtd_nandbiterrs: rewrite page
[  245.417711] mtd_nandbiterrs: read_page
[  245.422027] mtd_nandbiterrs: verify_page
[  245.426326] mtd_nandbiterrs: Successfully corrected 21 bit errors per subpage
[  245.433515] mtd_nandbiterrs: Inserted biterror @ 6/6
[  245.439795] mtd_nandbiterrs: rewrite page
[  245.445203] mtd_nandbiterrs: read_page
[  245.449432] mtd_nandbiterrs: verify_page
[  245.453762] mtd_nandbiterrs: Successfully corrected 22 bit errors per subpage
[  245.460927] mtd_nandbiterrs: Inserted biterror @ 6/2
[  245.467160] mtd_nandbiterrs: rewrite page
[  245.472558] mtd_nandbiterrs: read_page
[  245.476788] mtd_nandbiterrs: verify_page
[  245.481133] mtd_nandbiterrs: Successfully corrected 23 bit errors per subpage
[  245.488279] mtd_nandbiterrs: Inserted biterror @ 6/0
[  245.494610] mtd_nandbiterrs: rewrite page
[  245.499888] mtd_nandbiterrs: read_page
[  245.504194] mtd_nandbiterrs: verify_page
[  245.508490] mtd_nandbiterrs: Successfully corrected 24 bit errors per subpage
[  245.515676] mtd_nandbiterrs: Inserted biterror @ 7/7
[  245.522001] mtd_nandbiterrs: rewrite page
[  245.527275] mtd_nandbiterrs: read_page
[  245.531577] mtd_nandbiterrs: verify_page
[  245.535874] mtd_nandbiterrs: Successfully corrected 25 bit errors per subpage
[  245.543049] mtd_nandbiterrs: Inserted biterror @ 7/6
[  245.549291] mtd_nandbiterrs: rewrite page
[  245.554685] mtd_nandbiterrs: read_page
[  245.558916] mtd_nandbiterrs: verify_page
[  245.563252] mtd_nandbiterrs: Successfully corrected 26 bit errors per subpage
[  245.570397] mtd_nandbiterrs: Inserted biterror @ 7/2
[  245.576733] mtd_nandbiterrs: rewrite page
[  245.582124] mtd_nandbiterrs: read_page
[  245.586355] mtd_nandbiterrs: verify_page
[  245.590650] mtd_nandbiterrs: Successfully corrected 27 bit errors per subpage
[  245.597836] mtd_nandbiterrs: Inserted biterror @ 7/0
[  245.604124] mtd_nandbiterrs: rewrite page
[  245.609392] mtd_nandbiterrs: read_page
[  245.613704] mtd_nandbiterrs: verify_page
[  245.618005] mtd_nandbiterrs: Successfully corrected 28 bit errors per subpage
[  245.625179] mtd_nandbiterrs: Inserted biterror @ 8/7
[  245.631417] mtd_nandbiterrs: rewrite page
[  245.636685] mtd_nandbiterrs: read_page
[  245.641007] mtd_nandbiterrs: verify_page
[  245.645308] mtd_nandbiterrs: Successfully corrected 29 bit errors per subpage
[  245.652482] mtd_nandbiterrs: Inserted biterror @ 8/5
[  245.658726] mtd_nandbiterrs: rewrite page
[  245.664125] mtd_nandbiterrs: read_page
[  245.668355] mtd_nandbiterrs: verify_page
[  245.672697] mtd_nandbiterrs: Successfully corrected 30 bit errors per subpage
[  245.679842] mtd_nandbiterrs: Inserted biterror @ 8/4
[  245.686178] mtd_nandbiterrs: rewrite page
[  245.691492] mtd_nandbiterrs: read_page
[  245.695722] mtd_nandbiterrs: verify_page
[  245.700019] mtd_nandbiterrs: Successfully corrected 31 bit errors per subpage
[  245.707193] mtd_nandbiterrs: Inserted biterror @ 8/2
[  245.713444] mtd_nandbiterrs: rewrite page
[  245.718720] mtd_nandbiterrs: read_page
[  245.723022] mtd_nandbiterrs: verify_page
[  245.727319] mtd_nandbiterrs: Successfully corrected 32 bit errors per subpage
[  245.734489] mtd_nandbiterrs: Inserted biterror @ 8/0
[  245.740728] mtd_nandbiterrs: rewrite page
[  245.746126] mtd_nandbiterrs: read_page
[  245.750356] mtd_nandbiterrs: verify_page
[  245.754694] mtd_nandbiterrs: Successfully corrected 33 bit errors per subpage
[  245.761861] mtd_nandbiterrs: Inserted biterror @ 9/5
[  245.768127] mtd_nandbiterrs: rewrite page
[  245.773537] mtd_nandbiterrs: read_page
[  245.777776] mtd_nandbiterrs: verify_page
[  245.782109] mtd_nandbiterrs: Successfully corrected 34 bit errors per subpage
[  245.789253] mtd_nandbiterrs: Inserted biterror @ 9/4
[  245.795584] mtd_nandbiterrs: rewrite page
[  245.800900] mtd_nandbiterrs: read_page
[  245.805137] mtd_nandbiterrs: verify_page
[  245.809434] mtd_nandbiterrs: Successfully corrected 35 bit errors per subpage
[  245.816606] mtd_nandbiterrs: Inserted biterror @ 9/2
[  245.822860] mtd_nandbiterrs: rewrite page
[  245.828129] mtd_nandbiterrs: read_page
[  245.832428] mtd_nandbiterrs: verify_page
[  245.836727] mtd_nandbiterrs: Successfully corrected 36 bit errors per subpage
[  245.843914] mtd_nandbiterrs: Inserted biterror @ 9/0
[  245.850158] mtd_nandbiterrs: rewrite page
[  245.855565] mtd_nandbiterrs: read_page
[  245.859799] mtd_nandbiterrs: verify_page
[  245.864127] mtd_nandbiterrs: Successfully corrected 37 bit errors per subpage
[  245.871294] mtd_nandbiterrs: Inserted biterror @ 10/7
[  245.877618] mtd_nandbiterrs: rewrite page
[  245.883018] mtd_nandbiterrs: read_page
[  245.887249] mtd_nandbiterrs: verify_page
[  245.891575] mtd_nandbiterrs: Successfully corrected 38 bit errors per subpage
[  245.898721] mtd_nandbiterrs: Inserted biterror @ 10/6
[  245.905141] mtd_nandbiterrs: rewrite page
[  245.910420] mtd_nandbiterrs: read_page
[  245.914738] mtd_nandbiterrs: verify_page
[  245.919035] mtd_nandbiterrs: Successfully corrected 39 bit errors per subpage
[  245.926210] mtd_nandbiterrs: Inserted biterror @ 10/5
[  245.932549] mtd_nandbiterrs: rewrite page
[  245.937820] mtd_nandbiterrs: read_page
[  245.942120] mtd_nandbiterrs: Read reported 40 corrected bit errors
[  245.948311] mtd_nandbiterrs: verify_page
[  245.952639] mtd_nandbiterrs: Successfully corrected 40 bit errors per subpage
[  245.959784] mtd_nandbiterrs: Inserted biterror @ 10/4
[  245.966204] mtd_nandbiterrs: rewrite page
[  245.971508] mtd_nandbiterrs: read_page
[  245.981085] mtd_nandbiterrs: error: fill page failed at 0x0 err = -74
[  245.987538] mtd_nandbiterrs: After 41 biterrors per subpage, read reported error -74
[  245.996662] mtd_nandbiterrs: finished successfully.
[  246.001578] ==================================================

I think you can send out the new version.

thanks
Huang Shijie

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Boris Brezillon]

On Fri, 10 Oct 2014 22:42:51 +0800
Huang Shijie <shijie8@gmail.com> wrote:

> On Wed, Oct 08, 2014 at 05:10:34PM +0200, Boris Brezillon wrote:
> > On Wed, 8 Oct 2014 22:24:40 +0800
> > Huang Shijie <shijie8@gmail.com> wrote:
> > 
> > > On Tue, Sep 23, 2014 at 04:07:33PM +0200, Boris BREZILLON wrote:
> > > > Hello Huang, Brian,
> > > > 
> > > > This is just a new proposal to support raw accesses in a more standard way
> > > > in the GPMI driver.
> > > > This series has been tested on an imx28 board.
> > > > 
> > > > Any suggestions are welcome.
> > > > 
> > > > Best Regards,
> > > > 
> > > > Boris
> > > > 
> > > > Changes since v2:
> > > >  - fixed a bug in gpmi_move_bits
> > > >  - add a raw_buffer field to be used when using raw access methods
> > > >    (experienced memory corruptions when directly using page_buffer_virt
> > > >    buffer)
> > > >  - add raw OOB access functions
> > > 
> > > I tested this patch set today with the imx6dl-sabreauto board.
> > > 
> > >     NAND: Micron MT29F64G08CBABAWP
> > > 	8192MiB, MLC, page size: 8192, OOB size: 744
> > > 
> > >     ECC: 40bit
> > > 
> > > The result:
> > > 
> > > [ 3672.779009] ==================================================
> > > [ 3672.784974] mtd_nandbiterrs: MTD device: 0
> > > [ 3672.789480] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
> > > [ 3672.798169] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
> > > [ 3672.804554] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
> > > [ 3672.812497] mtd_nandbiterrs: incremental biterrors test
> > > [ 3672.818688] mtd_nandbiterrs: write_page
> > > [ 3672.825529] mtd_nandbiterrs: rewrite page
> > > [ 3672.837290] mtd_nandbiterrs: read_page
> > > [ 3672.848407] mtd_nandbiterrs: error: read failed at 0x0
> > > [ 3672.853644] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
> > > [ 3672.862932] mtd_nandbiterrs: finished successfully.
> > > [ 3672.867837] ==================================================
> > > 
> > > [ 3745.282368] ==================================================
> > > [ 3745.288227] mtd_nandbiterrs: MTD device: 0
> > > [ 3745.292913] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
> > > [ 3745.301897] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
> > > [ 3745.308023] mtd_nandbiterrs: Using page=1, offset=8192, eraseblock=0
> > > [ 3745.316778] mtd_nandbiterrs: incremental biterrors test
> > > [ 3745.323017] mtd_nandbiterrs: write_page
> > > [ 3745.328616] mtd_nandbiterrs: rewrite page
> > > [ 3745.334191] mtd_nandbiterrs: read_page
> > > [ 3745.346878] mtd_nandbiterrs: error: read failed at 0x2000
> > > [ 3745.352352] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
> > > [ 3745.361281] mtd_nandbiterrs: finished successfully.
> > > [ 3745.366173] ==================================================
> > > 
> > > 
> > > Is this okay?
> > 
> > No, it doesn't seem to be correct.
> > But it's an MLC flash, so you'll most probably need to apply this patch
> > to nandbiterrs testsuite:
> > 
> > http://code.bulix.org/f69wuu-87021
> > 
> > This patch is flashing the block between each bitflip insertion to
> > avoid multiple write without erasure (which, AFAIK, is not supported
> > by MLC flashes).
> After I applied this patch. It seems ok now.
> 
> The test result:
> [  244.789900] ==================================================
> [  244.795826] mtd_nandbiterrs: MTD device: 1
> [  244.799974] mtd_nandbiterrs: MTD device size 16777216, eraseblock=2097152, page=8192, oob=744
> [  244.808563] mtd_nandbiterrs: Device uses 1 subpages of 8192 bytes
> [  244.814697] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
> [  244.822622] mtd_nandbiterrs: incremental biterrors test
> [  244.828146] mtd_nandbiterrs: write_page
> [  244.833341] mtd_nandbiterrs: fill page
> [  244.838831] mtd_nandbiterrs: rewrite page
> [  244.844353] mtd_nandbiterrs: read_page
> [  244.848585] mtd_nandbiterrs: verify_page
> [  244.852915] mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage
> [  244.859973] mtd_nandbiterrs: Inserted biterror @ 0/5
> [  244.866316] mtd_nandbiterrs: rewrite page
> [  244.871629] mtd_nandbiterrs: read_page
> [  244.875864] mtd_nandbiterrs: verify_page
> [  244.880161] mtd_nandbiterrs: Successfully corrected 1 bit errors per subpage
> [  244.887247] mtd_nandbiterrs: Inserted biterror @ 0/2
> [  244.893499] mtd_nandbiterrs: rewrite page
> [  244.898766] mtd_nandbiterrs: read_page
> [  244.903071] mtd_nandbiterrs: verify_page
> [  244.907369] mtd_nandbiterrs: Successfully corrected 2 bit errors per subpage
> [  244.914463] mtd_nandbiterrs: Inserted biterror @ 0/0
> [  244.920701] mtd_nandbiterrs: rewrite page
> [  244.926109] mtd_nandbiterrs: read_page
> [  244.930343] mtd_nandbiterrs: verify_page
> [  244.934672] mtd_nandbiterrs: Successfully corrected 3 bit errors per subpage
> [  244.941752] mtd_nandbiterrs: Inserted biterror @ 1/7
> [  244.947988] mtd_nandbiterrs: rewrite page
> [  244.953408] mtd_nandbiterrs: read_page
> [  244.957641] mtd_nandbiterrs: verify_page
> [  244.961970] mtd_nandbiterrs: Successfully corrected 4 bit errors per subpage
> [  244.969029] mtd_nandbiterrs: Inserted biterror @ 1/5
> [  244.975357] mtd_nandbiterrs: rewrite page
> [  244.980627] mtd_nandbiterrs: read_page
> [  244.984931] mtd_nandbiterrs: verify_page
> [  244.989229] mtd_nandbiterrs: Successfully corrected 5 bit errors per subpage
> [  244.996318] mtd_nandbiterrs: Inserted biterror @ 1/2
> [  245.002576] mtd_nandbiterrs: rewrite page
> [  245.007843] mtd_nandbiterrs: read_page
> [  245.012139] mtd_nandbiterrs: verify_page
> [  245.016439] mtd_nandbiterrs: Successfully corrected 6 bit errors per subpage
> [  245.023526] mtd_nandbiterrs: Inserted biterror @ 1/0
> [  245.029755] mtd_nandbiterrs: rewrite page
> [  245.035149] mtd_nandbiterrs: read_page
> [  245.039378] mtd_nandbiterrs: verify_page
> [  245.043726] mtd_nandbiterrs: Successfully corrected 7 bit errors per subpage
> [  245.050787] mtd_nandbiterrs: Inserted biterror @ 2/6
> [  245.057123] mtd_nandbiterrs: rewrite page
> [  245.062423] mtd_nandbiterrs: read_page
> [  245.066654] mtd_nandbiterrs: verify_page
> [  245.071001] mtd_nandbiterrs: Successfully corrected 8 bit errors per subpage
> [  245.078060] mtd_nandbiterrs: Inserted biterror @ 2/5
> [  245.084398] mtd_nandbiterrs: rewrite page
> [  245.089673] mtd_nandbiterrs: read_page
> [  245.093978] mtd_nandbiterrs: verify_page
> [  245.098275] mtd_nandbiterrs: Successfully corrected 9 bit errors per subpage
> [  245.105360] mtd_nandbiterrs: Inserted biterror @ 2/2
> [  245.111622] mtd_nandbiterrs: rewrite page
> [  245.116899] mtd_nandbiterrs: read_page
> [  245.121188] mtd_nandbiterrs: verify_page
> [  245.125485] mtd_nandbiterrs: Successfully corrected 10 bit errors per subpage
> [  245.132658] mtd_nandbiterrs: Inserted biterror @ 2/0
> [  245.138895] mtd_nandbiterrs: rewrite page
> [  245.144294] mtd_nandbiterrs: read_page
> [  245.148524] mtd_nandbiterrs: verify_page
> [  245.152853] mtd_nandbiterrs: Successfully corrected 11 bit errors per subpage
> [  245.159998] mtd_nandbiterrs: Inserted biterror @ 3/7
> [  245.166326] mtd_nandbiterrs: rewrite page
> [  245.171620] mtd_nandbiterrs: read_page
> [  245.175849] mtd_nandbiterrs: verify_page
> [  245.180146] mtd_nandbiterrs: Successfully corrected 12 bit errors per subpage
> [  245.187314] mtd_nandbiterrs: Inserted biterror @ 3/6
> [  245.193603] mtd_nandbiterrs: rewrite page
> [  245.198874] mtd_nandbiterrs: read_page
> [  245.203183] mtd_nandbiterrs: verify_page
> [  245.207480] mtd_nandbiterrs: Successfully corrected 13 bit errors per subpage
> [  245.214657] mtd_nandbiterrs: Inserted biterror @ 3/5
> [  245.220958] mtd_nandbiterrs: rewrite page
> [  245.226235] mtd_nandbiterrs: read_page
> [  245.230462] mtd_nandbiterrs: verify_page
> [  245.234787] mtd_nandbiterrs: Successfully corrected 14 bit errors per subpage
> [  245.241965] mtd_nandbiterrs: Inserted biterror @ 3/2
> [  245.248210] mtd_nandbiterrs: rewrite page
> [  245.253614] mtd_nandbiterrs: read_page
> [  245.257846] mtd_nandbiterrs: verify_page
> [  245.262177] mtd_nandbiterrs: Successfully corrected 15 bit errors per subpage
> [  245.269322] mtd_nandbiterrs: Inserted biterror @ 3/0
> [  245.275670] mtd_nandbiterrs: rewrite page
> [  245.280965] mtd_nandbiterrs: read_page
> [  245.285202] mtd_nandbiterrs: verify_page
> [  245.289501] mtd_nandbiterrs: Successfully corrected 16 bit errors per subpage
> [  245.296677] mtd_nandbiterrs: Inserted biterror @ 4/2
> [  245.302952] mtd_nandbiterrs: rewrite page
> [  245.308237] mtd_nandbiterrs: read_page
> [  245.312548] mtd_nandbiterrs: verify_page
> [  245.316846] mtd_nandbiterrs: Successfully corrected 17 bit errors per subpage
> [  245.324018] mtd_nandbiterrs: Inserted biterror @ 4/0
> [  245.330264] mtd_nandbiterrs: rewrite page
> [  245.335672] mtd_nandbiterrs: read_page
> [  245.339895] mtd_nandbiterrs: verify_page
> [  245.344222] mtd_nandbiterrs: Successfully corrected 18 bit errors per subpage
> [  245.351399] mtd_nandbiterrs: Inserted biterror @ 5/7
> [  245.357673] mtd_nandbiterrs: rewrite page
> [  245.363085] mtd_nandbiterrs: read_page
> [  245.367316] mtd_nandbiterrs: verify_page
> [  245.371646] mtd_nandbiterrs: Successfully corrected 19 bit errors per subpage
> [  245.378792] mtd_nandbiterrs: Inserted biterror @ 5/2
> [  245.385136] mtd_nandbiterrs: rewrite page
> [  245.390402] mtd_nandbiterrs: read_page
> [  245.394704] mtd_nandbiterrs: verify_page
> [  245.399002] mtd_nandbiterrs: Successfully corrected 20 bit errors per subpage
> [  245.406178] mtd_nandbiterrs: Inserted biterror @ 5/0
> [  245.412440] mtd_nandbiterrs: rewrite page
> [  245.417711] mtd_nandbiterrs: read_page
> [  245.422027] mtd_nandbiterrs: verify_page
> [  245.426326] mtd_nandbiterrs: Successfully corrected 21 bit errors per subpage
> [  245.433515] mtd_nandbiterrs: Inserted biterror @ 6/6
> [  245.439795] mtd_nandbiterrs: rewrite page
> [  245.445203] mtd_nandbiterrs: read_page
> [  245.449432] mtd_nandbiterrs: verify_page
> [  245.453762] mtd_nandbiterrs: Successfully corrected 22 bit errors per subpage
> [  245.460927] mtd_nandbiterrs: Inserted biterror @ 6/2
> [  245.467160] mtd_nandbiterrs: rewrite page
> [  245.472558] mtd_nandbiterrs: read_page
> [  245.476788] mtd_nandbiterrs: verify_page
> [  245.481133] mtd_nandbiterrs: Successfully corrected 23 bit errors per subpage
> [  245.488279] mtd_nandbiterrs: Inserted biterror @ 6/0
> [  245.494610] mtd_nandbiterrs: rewrite page
> [  245.499888] mtd_nandbiterrs: read_page
> [  245.504194] mtd_nandbiterrs: verify_page
> [  245.508490] mtd_nandbiterrs: Successfully corrected 24 bit errors per subpage
> [  245.515676] mtd_nandbiterrs: Inserted biterror @ 7/7
> [  245.522001] mtd_nandbiterrs: rewrite page
> [  245.527275] mtd_nandbiterrs: read_page
> [  245.531577] mtd_nandbiterrs: verify_page
> [  245.535874] mtd_nandbiterrs: Successfully corrected 25 bit errors per subpage
> [  245.543049] mtd_nandbiterrs: Inserted biterror @ 7/6
> [  245.549291] mtd_nandbiterrs: rewrite page
> [  245.554685] mtd_nandbiterrs: read_page
> [  245.558916] mtd_nandbiterrs: verify_page
> [  245.563252] mtd_nandbiterrs: Successfully corrected 26 bit errors per subpage
> [  245.570397] mtd_nandbiterrs: Inserted biterror @ 7/2
> [  245.576733] mtd_nandbiterrs: rewrite page
> [  245.582124] mtd_nandbiterrs: read_page
> [  245.586355] mtd_nandbiterrs: verify_page
> [  245.590650] mtd_nandbiterrs: Successfully corrected 27 bit errors per subpage
> [  245.597836] mtd_nandbiterrs: Inserted biterror @ 7/0
> [  245.604124] mtd_nandbiterrs: rewrite page
> [  245.609392] mtd_nandbiterrs: read_page
> [  245.613704] mtd_nandbiterrs: verify_page
> [  245.618005] mtd_nandbiterrs: Successfully corrected 28 bit errors per subpage
> [  245.625179] mtd_nandbiterrs: Inserted biterror @ 8/7
> [  245.631417] mtd_nandbiterrs: rewrite page
> [  245.636685] mtd_nandbiterrs: read_page
> [  245.641007] mtd_nandbiterrs: verify_page
> [  245.645308] mtd_nandbiterrs: Successfully corrected 29 bit errors per subpage
> [  245.652482] mtd_nandbiterrs: Inserted biterror @ 8/5
> [  245.658726] mtd_nandbiterrs: rewrite page
> [  245.664125] mtd_nandbiterrs: read_page
> [  245.668355] mtd_nandbiterrs: verify_page
> [  245.672697] mtd_nandbiterrs: Successfully corrected 30 bit errors per subpage
> [  245.679842] mtd_nandbiterrs: Inserted biterror @ 8/4
> [  245.686178] mtd_nandbiterrs: rewrite page
> [  245.691492] mtd_nandbiterrs: read_page
> [  245.695722] mtd_nandbiterrs: verify_page
> [  245.700019] mtd_nandbiterrs: Successfully corrected 31 bit errors per subpage
> [  245.707193] mtd_nandbiterrs: Inserted biterror @ 8/2
> [  245.713444] mtd_nandbiterrs: rewrite page
> [  245.718720] mtd_nandbiterrs: read_page
> [  245.723022] mtd_nandbiterrs: verify_page
> [  245.727319] mtd_nandbiterrs: Successfully corrected 32 bit errors per subpage
> [  245.734489] mtd_nandbiterrs: Inserted biterror @ 8/0
> [  245.740728] mtd_nandbiterrs: rewrite page
> [  245.746126] mtd_nandbiterrs: read_page
> [  245.750356] mtd_nandbiterrs: verify_page
> [  245.754694] mtd_nandbiterrs: Successfully corrected 33 bit errors per subpage
> [  245.761861] mtd_nandbiterrs: Inserted biterror @ 9/5
> [  245.768127] mtd_nandbiterrs: rewrite page
> [  245.773537] mtd_nandbiterrs: read_page
> [  245.777776] mtd_nandbiterrs: verify_page
> [  245.782109] mtd_nandbiterrs: Successfully corrected 34 bit errors per subpage
> [  245.789253] mtd_nandbiterrs: Inserted biterror @ 9/4
> [  245.795584] mtd_nandbiterrs: rewrite page
> [  245.800900] mtd_nandbiterrs: read_page
> [  245.805137] mtd_nandbiterrs: verify_page
> [  245.809434] mtd_nandbiterrs: Successfully corrected 35 bit errors per subpage
> [  245.816606] mtd_nandbiterrs: Inserted biterror @ 9/2
> [  245.822860] mtd_nandbiterrs: rewrite page
> [  245.828129] mtd_nandbiterrs: read_page
> [  245.832428] mtd_nandbiterrs: verify_page
> [  245.836727] mtd_nandbiterrs: Successfully corrected 36 bit errors per subpage
> [  245.843914] mtd_nandbiterrs: Inserted biterror @ 9/0
> [  245.850158] mtd_nandbiterrs: rewrite page
> [  245.855565] mtd_nandbiterrs: read_page
> [  245.859799] mtd_nandbiterrs: verify_page
> [  245.864127] mtd_nandbiterrs: Successfully corrected 37 bit errors per subpage
> [  245.871294] mtd_nandbiterrs: Inserted biterror @ 10/7
> [  245.877618] mtd_nandbiterrs: rewrite page
> [  245.883018] mtd_nandbiterrs: read_page
> [  245.887249] mtd_nandbiterrs: verify_page
> [  245.891575] mtd_nandbiterrs: Successfully corrected 38 bit errors per subpage
> [  245.898721] mtd_nandbiterrs: Inserted biterror @ 10/6
> [  245.905141] mtd_nandbiterrs: rewrite page
> [  245.910420] mtd_nandbiterrs: read_page
> [  245.914738] mtd_nandbiterrs: verify_page
> [  245.919035] mtd_nandbiterrs: Successfully corrected 39 bit errors per subpage
> [  245.926210] mtd_nandbiterrs: Inserted biterror @ 10/5
> [  245.932549] mtd_nandbiterrs: rewrite page
> [  245.937820] mtd_nandbiterrs: read_page
> [  245.942120] mtd_nandbiterrs: Read reported 40 corrected bit errors
> [  245.948311] mtd_nandbiterrs: verify_page
> [  245.952639] mtd_nandbiterrs: Successfully corrected 40 bit errors per subpage
> [  245.959784] mtd_nandbiterrs: Inserted biterror @ 10/4
> [  245.966204] mtd_nandbiterrs: rewrite page
> [  245.971508] mtd_nandbiterrs: read_page
> [  245.981085] mtd_nandbiterrs: error: fill page failed at 0x0 err = -74
> [  245.987538] mtd_nandbiterrs: After 41 biterrors per subpage, read reported error -74
> [  245.996662] mtd_nandbiterrs: finished successfully.
> [  246.001578] ==================================================
> 
> I think you can send out the new version.

That's great news!

Thanks for testing it.

I'll send a new version soon.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Iwo Mergler]

>
> No, it doesn't seem to be correct.
> But it's an MLC flash, so you'll most probably need to apply this patch
> to nandbiterrs testsuite:
>
> http://code.bulix.org/f69wuu-87021
>
> This patch is flashing the block between each bitflip insertion to
> avoid multiple write without erasure (which, AFAIK, is not supported
> by MLC flashes).

Hi Huang,


just out of interest, have you tried this on the MLC NAND without the patch?

I'm aware that MLC says you shouldn't write multiple times, but that is
with a view towards specified data endurance. I would only expect a few
additional bit errors during the test.

Did you try the overwrite test?

I'm curious how MLC NAND does when subjected to multiple writes.


Best regards,

Iwo

______________________________________________________________________
This communication contains information which may be confidential or privileged. The information is intended solely for the use of the individual or entity named above.  If you are not the intended recipient, be aware that any disclosure, copying, distribution or use of the contents of this information is prohibited.  If you have received this communication in error, please notify me by telephone immediately.
______________________________________________________________________

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Huang Shijie]

On Tue, Oct 14, 2014 at 04:50:27PM +1100, Iwo Mergler wrote:
> 
> >
> > No, it doesn't seem to be correct.
> > But it's an MLC flash, so you'll most probably need to apply this patch
> > to nandbiterrs testsuite:
> >
> > http://code.bulix.org/f69wuu-87021
> >
> > This patch is flashing the block between each bitflip insertion to
> > avoid multiple write without erasure (which, AFAIK, is not supported
> > by MLC flashes).
> 
> Hi Huang,
> 
> 
> just out of interest, have you tried this on the MLC NAND without the patch?

yes. I tried. As i posted, it will failed.

> 
> I'm aware that MLC says you shouldn't write multiple times, but that is
> with a view towards specified data endurance. I would only expect a few
> additional bit errors during the test.
> 
> Did you try the overwrite test?
not yet. I can test it tomorrow.

> 
> I'm curious how MLC NAND does when subjected to multiple writes.
We should not do the multiple writes to the MLC nand. 
We can do so with the SLC nand.

thank
Huang Shijie

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Huang Shijie]

On Tue, Oct 14, 2014 at 04:50:27PM +1100, Iwo Mergler wrote:
> 
> >
> > No, it doesn't seem to be correct.
> > But it's an MLC flash, so you'll most probably need to apply this patch
> > to nandbiterrs testsuite:
> >
> > http://code.bulix.org/f69wuu-87021
> >
> > This patch is flashing the block between each bitflip insertion to
> > avoid multiple write without erasure (which, AFAIK, is not supported
> > by MLC flashes).
> 
> Hi Huang,
> 
> 
> just out of interest, have you tried this on the MLC NAND without the patch?
> 
> I'm aware that MLC says you shouldn't write multiple times, but that is
> with a view towards specified data endurance. I would only expect a few
> additional bit errors during the test.
> 
> Did you try the overwrite test?
> 
The following is the test result for overwrite with this MLC patch:

root at imx6qdlsolo:~# insmod mtd_nandbiterrs.ko dev=1 mode=1
[  762.534714] 
[  762.536259] ==================================================
[  762.542115] mtd_nandbiterrs: MTD device: 1
[  762.546326] mtd_nandbiterrs: MTD device size 16777216,
	eraseblock=2097152, page=8192, oob=744
	[  762.554937] mtd_nandbiterrs: Device uses 1 subpages of 8192
	bytes
	[  762.561059] mtd_nandbiterrs: Using page=0, offset=0,
	eraseblock=0
	[  762.571333] mtd_nandbiterrs: overwrite biterrors test
	[  762.576715] mtd_nandbiterrs: write_page
	[  762.590448] mtd_nandbiterrs: error: read failed at 0x0
	[  762.595650] mtd_nandbiterrs: Read reported error -74
	[  762.600625] mtd_nandbiterrs: Bit error histogram (0
			operations total):
	[  762.608586] mtd_nandbiterrs: finished successfully.
	[  762.613501]
	==================================================
	insmod: ERROR: could not insert module mtd_nandbiterrs.ko:
	Input/output error


thanks	   

Huang Shijie

[PATCH v3 0/3] mtd: nand: gpmi: add proper raw access support

[Iwo Mergler]

On Sun, 19 Oct 2014 13:20:38 +1100 Huang Shijie <shijie8@gmail.com> wrote:
> On Tue, Oct 14, 2014 at 04:50:27PM +1100, Iwo Mergler wrote:
> > 
> > >
> > > No, it doesn't seem to be correct.
> > > But it's an MLC flash, so you'll most probably need to apply this
> > > patch to nandbiterrs testsuite:
> > >
> > > http://code.bulix.org/f69wuu-87021
> > >
> > > This patch is flashing the block between each bitflip insertion to
> > > avoid multiple write without erasure (which, AFAIK, is not
> > > supported by MLC flashes).
> > 
> > Hi Huang,
> > 
> > 
> > just out of interest, have you tried this on the MLC NAND without
> > the patch?
> > 
> > I'm aware that MLC says you shouldn't write multiple times, but
> > that is with a view towards specified data endurance. I would only
> > expect a few additional bit errors during the test.
> > 
> > Did you try the overwrite test?
> > 
> The following is the test result for overwrite with this MLC patch:
> 
> root at imx6qdlsolo:~# insmod mtd_nandbiterrs.ko dev=1 mode=1
> [  762.534714] 
> [  762.536259] ==================================================
> [  762.542115] mtd_nandbiterrs: MTD device: 1
> [  762.546326] mtd_nandbiterrs: MTD device size 16777216,
> 	eraseblock=2097152, page=8192, oob=744
> 	[  762.554937] mtd_nandbiterrs: Device uses 1 subpages of 8192
> 	bytes
> 	[  762.561059] mtd_nandbiterrs: Using page=0, offset=0,
> 	eraseblock=0
> 	[  762.571333] mtd_nandbiterrs: overwrite biterrors test
> 	[  762.576715] mtd_nandbiterrs: write_page
> 	[  762.590448] mtd_nandbiterrs: error: read failed at 0x0
> 	[  762.595650] mtd_nandbiterrs: Read reported error -74
> 	[  762.600625] mtd_nandbiterrs: Bit error histogram (0
> 			operations total):
> 	[  762.608586] mtd_nandbiterrs: finished successfully.
> 	[  762.613501]
> 	==================================================
> 	insmod: ERROR: could not insert module mtd_nandbiterrs.ko:
> 	Input/output error
> 

Thanks. If I read this correctly, this means that writing the same data twice,
already generates more bit errors than the ECC can fix.


Best regards,

Iwo

______________________________________________________________________
This communication contains information which may be confidential or privileged. The information is intended solely for the use of the individual or entity named above.  If you are not the intended recipient, be aware that any disclosure, copying, distribution or use of the contents of this information is prohibited.  If you have received this communication in error, please notify me by telephone immediately.
______________________________________________________________________