Discussion:
[PATCH v4 0/4] mtd: nand: gpmi: add proper raw access support
Boris Brezillon
2014-10-20 08:46:13 UTC
Permalink
Hello,

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

I also updated the nand_ecc_ctrl struct documentation to clearly state that
specific layouts should be hidden when accessing the NAND chip in raw mode.

Best Regards,

Boris

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 (4):
mtd: nand: provide detailed description for raw read/write page
methods
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 | 129 +++++++++++++++++++++++++++++
drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 146 +++++++++++++++++++++++++++++++++
drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 6 ++
include/linux/mtd/nand.h | 17 +++-
4 files changed, 296 insertions(+), 2 deletions(-)
--
1.9.1
Boris Brezillon
2014-10-20 08:46:17 UTC
Permalink
Implement raw OOB access functions to retrieve OOB bytes when accessing the
NAND in raw mode.

Signed-off-by: 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 bd4dedc..d7c483a 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1473,6 +1473,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;
@@ -1792,6 +1808,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
Boris Brezillon
2014-10-20 08:46:14 UTC
Permalink
read_page_raw and write_page_raw method description is not clear enough.
It clearly specifies that ECC correction should not be involved but does
not talk about specific layout (by layout I mean where in-band and
out-of-band data are stored on the NAND media) used by NAND/ECC
controllers.

Those specific layouts might impact MTD users and thus should be hidden (as
already done in the standard NAND_ECC_HW_SYNDROME implementation).

Clearly state this constraint in the nand_ecc_ctrl struct documentation.

Signed-off-by: Boris Brezillon <***@free-electrons.com>
---
include/linux/mtd/nand.h | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index e4d451e..b14d190 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -455,8 +455,21 @@ struct nand_hw_control {
* be provided if an hardware ECC is available
* @calculate: function for ECC calculation or readback from ECC hardware
* @correct: function for ECC correction, matching to ECC generator (sw/hw)
- * @read_page_raw: function to read a raw page without ECC
- * @write_page_raw: function to write a raw page without ECC
+ * @read_page_raw: function to read a raw page without ECC. This function
+ * should hide the specific layout used by the ECC
+ * controller and always return contiguous in-band and
+ * out-of-band data even if they're not stored
+ * contiguously on the NAND chip (e.g.
+ * NAND_ECC_HW_SYNDROME interleaves in-band and
+ * out-of-band data).
+ * @write_page_raw: function to write a raw page without ECC. This function
+ * should hide the specific layout used by the ECC
+ * controller and consider the passed data as contiguous
+ * in-band and out-of-band data. ECC controller is
+ * responsible for doing the appropriate transformations
+ * to adapt to its specific layout (e.g.
+ * NAND_ECC_HW_SYNDROME interleaves in-band and
+ * out-of-band data).
* @read_page: function to read a page according to the ECC generator
* requirements; returns maximum number of bitflips corrected in
* any single ECC step, 0 if bitflips uncorrectable, -EIO hw error
--
1.9.1
Boris Brezillon
2014-10-20 08:46:15 UTC
Permalink
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 <***@free-electrons.com>
---
drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 129 +++++++++++++++++++++++++++++++++
drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 4 +
2 files changed, 133 insertions(+)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 87e658c..5d4f140 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -1353,3 +1353,132 @@ 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. */
+ 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 before extracting a byte
+ * to store in dst.
+ */
+ for (i = 0; i < nbytes; i++) {
+ src_byte |= src[i] << (8 - src_bit_off);
+ dst[i] = src_byte;
+ src_byte >>= 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 && !src_bit_off)
+ return;
+
+ /* Copy the remaining bits to src_byte */
+ if (nbits)
+ src_byte |= (*src & GENMASK(nbits - 1, 0)) <<
+ ((8 - src_bit_off) % 8);
+ nbits += (8 - src_bit_off) % 8;
+
+ /*
+ * In case there were not enough bits to get a byte aligned dst buffer
+ * prepare the src_byte variable to match the dst organization (shift
+ * src_byte by dst_bit_off and retrieve the 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 dst */
+ 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
Boris Brezillon
2014-10-20 08:46:16 UTC
Permalink
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 <***@free-electrons.com>
---
drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 128 +++++++++++++++++++++++++++++++++
drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 2 +
2 files changed, 130 insertions(+)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 959cb9b..bd4dedc 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,128 @@ 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) {
+ if (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_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 +1790,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
Loading...