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

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

[Boris Brezillon]

Hello,

This series provides an implementation for raw accesses taking care of
hidding the specific layout used by the GPMI controller.

Best Regards,

Boris

Changes since v5:
 - rename gpmi_move_bits into gpmi_copy_bits

Changes since v4:
 - fixed a few corner cases in gpmi_move_bits (tested it with:
   https://github.com/bbrezillon/gpmi-move-bits-test/blob/master/gpmi-move-bits-test.c)
 - add documentation and comments for the new gpmi functions

Changes since v3:
 - add comments to the gpmi_move_bits function
 - extend raw read/write documentation
 - move last part of the raw_page_read function into a conditional block

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_copy_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  | 153 +++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 201 +++++++++++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   6 +
 3 files changed, 360 insertions(+)

-- 
1.9.1

[PATCH v6 1/3] mtd: nand: gpmi: add gpmi_copy_bits function

[Boris Brezillon]

Add a new function to copy bits (not bytes) from a memory region to
another one.
This function is similar to memcpy except it acts at bit level.
It is needed to implement GPMI raw access functions and adapt to the
hardware ECC engine which does not pad ECC bits to the next byte boundary.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Tested-by: Huang Shijie <shijie8@gmail.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 153 +++++++++++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   4 +
 2 files changed, 157 insertions(+)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 87e658c..27f272e 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -1353,3 +1353,156 @@ 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);
 }
+
+/**
+ * gpmi_copy_bits - copy bits from one memory region to another
+ * @dst: destination buffer
+ * @dst_bit_off: bit offset we're starting to write at
+ * @src: source buffer
+ * @src_bit_off: bit offset we're starting to read from
+ * @nbits: number of bits to copy
+ *
+ * This functions copies bits from one memory region to another, and is used by
+ * the GPMI driver to copy ECC sections which are not guaranteed to be byte
+ * aligned.
+ *
+ * src and dst should not overlap.
+ *
+ */
+void gpmi_copy_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_buffer = 0;
+	size_t bits_in_src_buffer = 0;
+
+	if (!nbits)
+		return;
+
+	/*
+	 * 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_buffer 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_buffer = src[0] >> src_bit_off;
+		if (nbits >= (8 - src_bit_off)) {
+			bits_in_src_buffer += 8 - src_bit_off;
+		} else {
+			src_buffer &= GENMASK(nbits - 1, 0);
+			bits_in_src_buffer += nbits;
+		}
+		nbits -= bits_in_src_buffer;
+		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. */
+	if (dst_bit_off) {
+		if (bits_in_src_buffer < (8 - dst_bit_off) && nbytes) {
+			src_buffer |= src[0] << bits_in_src_buffer;
+			bits_in_src_buffer += 8;
+			src++;
+			nbytes--;
+		}
+
+		if (bits_in_src_buffer >= (8 - dst_bit_off)) {
+			dst[0] &= GENMASK(dst_bit_off - 1, 0);
+			dst[0] |= src_buffer << dst_bit_off;
+			src_buffer >>= (8 - dst_bit_off);
+			bits_in_src_buffer -= (8 - dst_bit_off);
+			dst_bit_off = 0;
+			dst++;
+			if (bits_in_src_buffer > 7) {
+				bits_in_src_buffer -= 8;
+				dst[0] = src_buffer;
+				dst++;
+				src_buffer >>= 8;
+			}
+		}
+	}
+
+	if (!bits_in_src_buffer && !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_buffer variable before extracting a byte
+		 * to store in dst.
+		 */
+		for (i = 0; i < nbytes; i++) {
+			src_buffer |= src[i] << bits_in_src_buffer;
+			dst[i] = src_buffer;
+			src_buffer >>= 8;
+		}
+	}
+	/* Update dst and src pointers */
+	dst += nbytes;
+	src += nbytes;
+
+	/*
+	 * nbits is the number of remaining bits. It should not exceed 8 as
+	 * we've already copied as much bytes 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 && !bits_in_src_buffer)
+		return;
+
+	/* Copy the remaining bits to src_buffer */
+	if (nbits)
+		src_buffer |= (*src & GENMASK(nbits - 1, 0)) <<
+			      bits_in_src_buffer;
+	bits_in_src_buffer += nbits;
+
+	/*
+	 * In case there were not enough bits to get a byte aligned dst buffer
+	 * prepare the src_buffer variable to match the dst organization (shift
+	 * src_buffer by dst_bit_off and retrieve the least significant bits
+	 * from dst).
+	 */
+	if (dst_bit_off)
+		src_buffer = (src_buffer << dst_bit_off) |
+			     (*dst & GENMASK(dst_bit_off - 1, 0));
+	bits_in_src_buffer += dst_bit_off;
+
+	/*
+	 * Keep most significant bits from dst if we end up with an unaligned
+	 * number of bits.
+	 */
+	nbytes = bits_in_src_buffer / 8;
+	if (bits_in_src_buffer % 8) {
+		src_buffer |= (dst[nbytes] &
+			       GENMASK(7, bits_in_src_buffer % 8)) <<
+			      (nbytes * 8);
+		nbytes++;
+	}
+
+	/* Copy the remaining bytes to dst */
+	for (i = 0; i < nbytes; i++) {
+		dst[i] = src_buffer;
+		src_buffer >>= 8;
+	}
+}
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 32c6ba4..20da1f1 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_copy_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 v6 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>
Tested-by: Huang Shijie <shijie8@gmail.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 183 +++++++++++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |   2 +
 2 files changed, 185 insertions(+)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 959cb9b..987de1e 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,183 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
 	return status & NAND_STATUS_FAIL ? -EIO : 0;
 }
 
+/*
+ * This function reads a NAND page without involving the ECC engine (no HW
+ * ECC correction).
+ * The tricky part in the GPMI/BCH controller is that it stores ECC bits
+ * inline (interleaved with payload DATA), and do not align data chunk on
+ * byte boundaries.
+ * We thus need to take care moving the payload data and ECC bits stored in the
+ * page into the provided buffers, which is why we're using gpmi_copy_bits.
+ *
+ * See set_geometry_by_ecc_info inline comments to have a full description
+ * of the layout used by the GPMI controller.
+ */
+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 required, swap the bad block marker and the data stored in the
+	 * metadata section, so that we don't wrongly consider a block as bad.
+	 *
+	 * See the layout description for a detailed explanation on why this
+	 * is needed.
+	 */
+	if (this->swap_block_mark) {
+		u8 swap = tmp_buf[0];
+
+		tmp_buf[0] = tmp_buf[mtd->writesize];
+		tmp_buf[mtd->writesize] = swap;
+	}
+
+	/*
+	 * Copy the metadata section into the oob buffer (this section is
+	 * guaranteed to be aligned on a byte boundary).
+	 */
+	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;
+
+	/* Extract interleaved payload data and ECC bits */
+	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
+		if (buf)
+			gpmi_copy_bits(buf, step * eccsize * 8,
+				       tmp_buf, src_bit_off,
+				       eccsize * 8);
+		src_bit_off += eccsize * 8;
+
+		/* Align last ECC block to align a byte boundary */
+		if (step == nfc_geo->ecc_chunk_count - 1 &&
+		    (oob_bit_off + eccbits) % 8)
+			eccbits += 8 - ((oob_bit_off + eccbits) % 8);
+
+		if (oob_required)
+			gpmi_copy_bits(oob, oob_bit_off,
+				       tmp_buf, src_bit_off,
+				       eccbits);
+
+		src_bit_off += eccbits;
+		oob_bit_off += eccbits;
+	}
+
+	if (oob_required) {
+		oob_byte_off = oob_bit_off / 8;
+
+		if (oob_byte_off < mtd->oobsize)
+			memcpy(oob + oob_byte_off,
+			       tmp_buf + mtd->writesize + oob_byte_off,
+			       mtd->oobsize - oob_byte_off);
+	}
+
+	return 0;
+}
+
+/*
+ * This function writes a NAND page without involving the ECC engine (no HW
+ * ECC generation).
+ * The tricky part in the GPMI/BCH controller is that it stores ECC bits
+ * inline (interleaved with payload DATA), and do not align data chunk on
+ * byte boundaries.
+ * We thus need to take care moving the OOB area at the right place in the
+ * final page, which is why we're using gpmi_copy_bits.
+ *
+ * See set_geometry_by_ecc_info inline comments to have a full description
+ * of the layout used by the GPMI controller.
+ */
+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;
+
+	/*
+	 * Initialize all bits to 1 in case we don't have a buffer for the
+	 * payload or oob data in order to leave unspecified bits of data
+	 * to their initial state.
+	 */
+	if (!buf || !oob_required)
+		memset(tmp_buf, 0xff, mtd->writesize + mtd->oobsize);
+
+	/*
+	 * First copy the metadata section (stored in oob buffer) at the
+	 * beginning of the page, as imposed by the GPMI layout.
+	 */
+	memcpy(tmp_buf, oob, nfc_geo->metadata_size);
+	oob_bit_off = nfc_geo->metadata_size * 8;
+	dst_bit_off = oob_bit_off;
+
+	/* Interleave payload data and ECC bits */
+	for (step = 0; step < nfc_geo->ecc_chunk_count; step++) {
+		if (buf)
+			gpmi_copy_bits(tmp_buf, dst_bit_off,
+				       buf, step * eccsize * 8, eccsize * 8);
+		dst_bit_off += eccsize * 8;
+
+		/* Align last ECC block to align a byte boundary */
+		if (step == nfc_geo->ecc_chunk_count - 1 &&
+		    (oob_bit_off + eccbits) % 8)
+			eccbits += 8 - ((oob_bit_off + eccbits) % 8);
+
+		if (oob_required)
+			gpmi_copy_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 required, swap the bad block marker and the first byte of the
+	 * metadata section, so that we don't modify the bad block marker.
+	 *
+	 * See the layout description for a detailed explanation on why this
+	 * is needed.
+	 */
+	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 +1845,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 20da1f1..544062f 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 v6 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>
Tested-by: Huang Shijie <shijie8@gmail.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 987de1e..4f3851a 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1528,6 +1528,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;
@@ -1847,6 +1863,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 v6 0/3] mtd: nand: gpmi: add proper raw access support

[Brian Norris]

On Sun, Nov 30, 2014 at 07:10:27PM +0100, Boris Brezillon wrote:
> Hello,
> 
> This series provides an implementation for raw accesses taking care of
> hidding the specific layout used by the GPMI controller.
> 
> Best Regards,
> 
> Boris
> 
> Changes since v5:
>  - rename gpmi_move_bits into gpmi_copy_bits
> 
> Changes since v4:
>  - fixed a few corner cases in gpmi_move_bits (tested it with:
>    https://github.com/bbrezillon/gpmi-move-bits-test/blob/master/gpmi-move-bits-test.c)
>  - add documentation and comments for the new gpmi functions
> 
> Changes since v3:
>  - add comments to the gpmi_move_bits function
>  - extend raw read/write documentation
>  - move last part of the raw_page_read function into a conditional block
> 
> 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

Applied the series. Thanks!

Out of curiosity, what tests does gpmi-nand.c now pass/fail?

Also, is it time to yank / fixup some of these comments from
gpmi-nand.c?

 ...
 * FIXME: The following paragraph is incorrect, now that there exist
 * ecc.read_oob_raw and ecc.write_oob_raw functions.
 *
 * Since MTD assumes the OOB is not covered by ECC, there is no pair of
 * ECC-based/raw functions for reading or or writing the OOB. The fact that the
 * caller wants an ECC-based or raw view of the page is not propagated down to
 * this driver.
 */

Brian

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

[Boris Brezillon]

Hi Brian,

On Mon, 1 Dec 2014 00:47:09 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> On Sun, Nov 30, 2014 at 07:10:27PM +0100, Boris Brezillon wrote:
> > Hello,
> > 
> > This series provides an implementation for raw accesses taking care of
> > hidding the specific layout used by the GPMI controller.
> > 
> > Best Regards,
> > 
> > Boris
> > 
> > Changes since v5:
> >  - rename gpmi_move_bits into gpmi_copy_bits
> > 
> > Changes since v4:
> >  - fixed a few corner cases in gpmi_move_bits (tested it with:
> >    https://github.com/bbrezillon/gpmi-move-bits-test/blob/master/gpmi-move-bits-test.c)
> >  - add documentation and comments for the new gpmi functions
> > 
> > Changes since v3:
> >  - add comments to the gpmi_move_bits function
> >  - extend raw read/write documentation
> >  - move last part of the raw_page_read function into a conditional block
> > 
> > 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
> 
> Applied the series. Thanks!
> 
> Out of curiosity, what tests does gpmi-nand.c now pass/fail?

The oobtest is still failing. I started to debug it, but didn't have
enough time to make it work.

The nandbiterrs test is working, though I didn't manage to make the
incremental test fail (writing the same pattern 10000 times without
erasing the block between each write does not generate any bit flips) on
my SLC NAND: MT29F2G08ABAEAH4.
Can someone with another SLC NAND chip try it ?

> 
> Also, is it time to yank / fixup some of these comments from
> gpmi-nand.c?

I was asking myself the same question...

> 
>  ...
>  * FIXME: The following paragraph is incorrect, now that there exist
>  * ecc.read_oob_raw and ecc.write_oob_raw functions.
>  *
>  * Since MTD assumes the OOB is not covered by ECC, there is no pair of
>  * ECC-based/raw functions for reading or or writing the OOB. The fact that the
>  * caller wants an ECC-based or raw view of the page is not propagated down to
>  * this driver.
>  */

I guess we can remove them.
Huang can you confirm that the raw access functions introduced in this
series are covering what's described here ?

Regards,

Boris

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

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

[Huang Shijie]

On Mon, Dec 01, 2014 at 09:58:58AM +0100, Boris Brezillon wrote:
> Hi Brian,
> 
> On Mon, 1 Dec 2014 00:47:09 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> 
> > On Sun, Nov 30, 2014 at 07:10:27PM +0100, Boris Brezillon wrote:
> > > Hello,
> > > 
> > > This series provides an implementation for raw accesses taking care of
> > > hidding the specific layout used by the GPMI controller.
> > > 
> > > Best Regards,
> > > 
> > > Boris
> > > 
> > > Changes since v5:
> > >  - rename gpmi_move_bits into gpmi_copy_bits
> > > 
> > > Changes since v4:
> > >  - fixed a few corner cases in gpmi_move_bits (tested it with:
> > >    https://github.com/bbrezillon/gpmi-move-bits-test/blob/master/gpmi-move-bits-test.c)
> > >  - add documentation and comments for the new gpmi functions
> > > 
> > > Changes since v3:
> > >  - add comments to the gpmi_move_bits function
> > >  - extend raw read/write documentation
> > >  - move last part of the raw_page_read function into a conditional block
> > > 
> > > 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
> > 
> > Applied the series. Thanks!
> > 
> > Out of curiosity, what tests does gpmi-nand.c now pass/fail?
> 
> The oobtest is still failing. I started to debug it, but didn't have
> enough time to make it work.
> 
> The nandbiterrs test is working, though I didn't manage to make the
> incremental test fail (writing the same pattern 10000 times without
> erasing the block between each write does not generate any bit flips) on
> my SLC NAND: MT29F2G08ABAEAH4.
> Can someone with another SLC NAND chip try it ?
> 
> > 
> > Also, is it time to yank / fixup some of these comments from
> > gpmi-nand.c?
> 
> I was asking myself the same question...
> 
> > 
> >  ...
> >  * FIXME: The following paragraph is incorrect, now that there exist
> >  * ecc.read_oob_raw and ecc.write_oob_raw functions.
> >  *
> >  * Since MTD assumes the OOB is not covered by ECC, there is no pair of
> >  * ECC-based/raw functions for reading or or writing the OOB. The fact that the
> >  * caller wants an ECC-based or raw view of the page is not propagated down to
> >  * this driver.
> >  */
> 
> I guess we can remove them.
> Huang can you confirm that the raw access functions introduced in this
> series are covering what's described here ?
I think we can remove these comments now.

thanks
Huang Shijie

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

[Boris Brezillon]

On Tue, 2 Dec 2014 08:47:43 +0800
Huang Shijie <shijie.huang@intel.com> wrote:

> On Mon, Dec 01, 2014 at 09:58:58AM +0100, Boris Brezillon wrote:
> > Hi Brian,
> > 
> > On Mon, 1 Dec 2014 00:47:09 -0800
> > Brian Norris <computersforpeace@gmail.com> wrote:
> > 
> > > On Sun, Nov 30, 2014 at 07:10:27PM +0100, Boris Brezillon wrote:
> > > > Hello,
> > > > 
> > > > This series provides an implementation for raw accesses taking care of
> > > > hidding the specific layout used by the GPMI controller.
> > > > 
> > > > Best Regards,
> > > > 
> > > > Boris
> > > > 
> > > > Changes since v5:
> > > >  - rename gpmi_move_bits into gpmi_copy_bits
> > > > 
> > > > Changes since v4:
> > > >  - fixed a few corner cases in gpmi_move_bits (tested it with:
> > > >    https://github.com/bbrezillon/gpmi-move-bits-test/blob/master/gpmi-move-bits-test.c)
> > > >  - add documentation and comments for the new gpmi functions
> > > > 
> > > > Changes since v3:
> > > >  - add comments to the gpmi_move_bits function
> > > >  - extend raw read/write documentation
> > > >  - move last part of the raw_page_read function into a conditional block
> > > > 
> > > > 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
> > > 
> > > Applied the series. Thanks!
> > > 
> > > Out of curiosity, what tests does gpmi-nand.c now pass/fail?
> > 
> > The oobtest is still failing. I started to debug it, but didn't have
> > enough time to make it work.
> > 
> > The nandbiterrs test is working, though I didn't manage to make the
> > incremental test fail (writing the same pattern 10000 times without
> > erasing the block between each write does not generate any bit flips) on
> > my SLC NAND: MT29F2G08ABAEAH4.
> > Can someone with another SLC NAND chip try it ?
> > 
> > > 
> > > Also, is it time to yank / fixup some of these comments from
> > > gpmi-nand.c?
> > 
> > I was asking myself the same question...
> > 
> > > 
> > >  ...
> > >  * FIXME: The following paragraph is incorrect, now that there exist
> > >  * ecc.read_oob_raw and ecc.write_oob_raw functions.
> > >  *
> > >  * Since MTD assumes the OOB is not covered by ECC, there is no pair of
> > >  * ECC-based/raw functions for reading or or writing the OOB. The fact that the
> > >  * caller wants an ECC-based or raw view of the page is not propagated down to
> > >  * this driver.
> > >  */
> > 
> > I guess we can remove them.
> > Huang can you confirm that the raw access functions introduced in this
> > series are covering what's described here ?
> I think we can remove these comments now.

Brian, do you want me to send this patch or are you taking care of it ?


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

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

[Brian Norris]

On Tue, Dec 02, 2014 at 09:21:50AM +0100, Boris Brezillon wrote:
> On Tue, 2 Dec 2014 08:47:43 +0800 Huang Shijie <shijie.huang@intel.com> wrote:
> > On Mon, Dec 01, 2014 at 09:58:58AM +0100, Boris Brezillon wrote:
> > > On Mon, 1 Dec 2014 00:47:09 -0800 Brian Norris <computersforpeace@gmail.com> wrote:
> > > > Also, is it time to yank / fixup some of these comments from
> > > > gpmi-nand.c?
> > > 
> > > I was asking myself the same question...
> > > 
> > > > 
> > > >  ...
> > > >  * FIXME: The following paragraph is incorrect, now that there exist
> > > >  * ecc.read_oob_raw and ecc.write_oob_raw functions.
> > > >  *
> > > >  * Since MTD assumes the OOB is not covered by ECC, there is no pair of
> > > >  * ECC-based/raw functions for reading or or writing the OOB. The fact that the
> > > >  * caller wants an ECC-based or raw view of the page is not propagated down to
> > > >  * this driver.
> > > >  */
> > > 
> > > I guess we can remove them.
> > > Huang can you confirm that the raw access functions introduced in this
> > > series are covering what's described here ?
> > I think we can remove these comments now.
> 
> Brian, do you want me to send this patch or are you taking care of it ?

Go ahead and send a patch.

Brian