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
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 <[email protected]>
---
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
Implement raw OOB access functions to retrieve OOB bytes when accessing the
NAND in raw mode.
Signed-off-by: Boris Brezillon <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Mon, Oct 20, 2014 at 10:46:14AM +0200, Boris Brezillon wrote:
> 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 <[email protected]>
> ---
> 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
>
thanks for this patch.
hi Brian, what's your opinion about this patch?
thanks
Huang Shijie
On Sat, Oct 25, 2014 at 11:55:27AM +0800, Huang Shijie wrote:
> On Mon, Oct 20, 2014 at 10:46:14AM +0200, Boris Brezillon wrote:
> > 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 <[email protected]>
> > ---
> > 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
> >
> thanks for this patch.
>
> hi Brian, what's your opinion about this patch?
I like the concept. It might be more succinctly described along the
lines of "the same layout as the non-raw versions (i.e., .read_page /
.write_page)". But the extra words may help for being more explicit.
I haven't taken a closer look at the other patches yet.
Brian
On Mon, Oct 20, 2014 at 10:46:14AM +0200, Boris Brezillon wrote:
> 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 <[email protected]>
Pushed this patch to l2-mtd.git. Thanks!
Brian
On Mon, Oct 20, 2014 at 10:46:16AM +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 <[email protected]>
> ---
> 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)
I think I follow what this function is doing, and gpmi-nand notes the
ECC layout elsewhere in the driver, but can you put a few comments at
above this function to describe what it's doing? Refer to existing
comments as needed. And maybe note the tricky parts inline with the
code.
> +{
> + 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)
Can buf ever be zero here?
> + 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);
So you're manufacturing a few 0 bits here, right? Is that safe? Would we
prefer to manufacture 1 bits, as if they are "erased"?
> +
> + oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8);
> +
> + if (oob_byte_off < mtd->oobsize)
Extra whitespace before '<'.
> + 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)
Same comment applies, about commenting the function.
> +{
> + 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)
Can buf be zero?
> + 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)
Again, can buf be zero?
> + 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];
Brian
On Mon, Oct 20, 2014 at 10:46:15AM +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.
>
> Signed-off-by: Boris Brezillon <[email protected]>
> ---
> 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)
Two things:
1) Yikes! This function is a little hairy.
2) This function really deserves a full comment header (kerneldoc?); it
needs to have clearly-documented high-level semantics.
I'm not sure how to address #1, as the complexity is necessary. Did you
run this through some unit tests, at least?
> +{
[snip]
> +}
Brian
On Mon, Oct 20, 2014 at 10:46:13AM +0200, Boris Brezillon wrote:
> 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.
Can I get any ack's, tested-by's, or reviewed-by's?
Brian
Hi Brian,
On Thu, 20 Nov 2014 01:08:07 -0800
Brian Norris <[email protected]> wrote:
> On Mon, Oct 20, 2014 at 10:46:16AM +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 <[email protected]>
> > ---
> > 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)
>
> I think I follow what this function is doing, and gpmi-nand notes the
> ECC layout elsewhere in the driver, but can you put a few comments at
> above this function to describe what it's doing? Refer to existing
> comments as needed. And maybe note the tricky parts inline with the
> code.
Sure, I'll add some comments.
>
> > +{
> > + 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)
>
> Can buf ever be zero here?
Actually, I call this function with a NULL buf in my 4th patch (to dump
the oob area).
>
> > + 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);
>
> So you're manufacturing a few 0 bits here, right? Is that safe? Would we
> prefer to manufacture 1 bits, as if they are "erased"?
AFAIR this is what the controller is doing (but I'll have to re-check
that one).
>
> > +
> > + oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8);
> > +
> > + if (oob_byte_off < mtd->oobsize)
>
> Extra whitespace before '<'.
I'll Fix that.
Thanks for the review.
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Thu, 20 Nov 2014 01:22:09 -0800
Brian Norris <[email protected]> wrote:
> On Mon, Oct 20, 2014 at 10:46:15AM +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.
> >
> > Signed-off-by: Boris Brezillon <[email protected]>
> > ---
> > 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)
>
> Two things:
>
> 1) Yikes! This function is a little hairy.
Yes I know, and if you see a much simpler algorithm to do that, I'm
really interested :-).
>
> 2) This function really deserves a full comment header (kerneldoc?); it
> needs to have clearly-documented high-level semantics.
I'll add a kernel doc header.
>
> I'm not sure how to address #1, as the complexity is necessary. Did you
> run this through some unit tests, at least?
No, but I did test it with several ECC configs.
Anyway, if I develop such unit tests, do you want me to put them in the
driver code (under an #ifdef section) ?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Thu, Nov 20, 2014 at 10:42:53AM +0100, Boris Brezillon wrote:
> On Thu, 20 Nov 2014 01:22:09 -0800 Brian Norris <[email protected]> wrote:
> > On Mon, Oct 20, 2014 at 10:46:15AM +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.
> > >
> > > Signed-off-by: Boris Brezillon <[email protected]>
> > > ---
> > > 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)
> >
> > Two things:
> >
> > 1) Yikes! This function is a little hairy.
>
> Yes I know, and if you see a much simpler algorithm to do that, I'm
> really interested :-).
No ideas at the moment :)
> > 2) This function really deserves a full comment header (kerneldoc?); it
> > needs to have clearly-documented high-level semantics.
>
> I'll add a kernel doc header.
>
> >
> > I'm not sure how to address #1, as the complexity is necessary. Did you
> > run this through some unit tests, at least?
>
> No, but I did test it with several ECC configs.
> Anyway, if I develop such unit tests, do you want me to put them in the
> driver code (under an #ifdef section) ?
I dunno, that seems like it might just clutter the file more, and I'm
not sure if anyone is likely to run them. I was mostly curious how you've
verified it.
Brian
On Thu, Nov 20, 2014 at 12:06:27AM -0800, Brian Norris wrote:
> On Mon, Oct 20, 2014 at 10:46:14AM +0200, Boris Brezillon wrote:
> > 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 <[email protected]>
>
> Pushed this patch to l2-mtd.git. Thanks!
thanks.
I am really happy you can merge this patch.
Huang Shijie
On Thu, Nov 20, 2014 at 01:23:36AM -0800, Brian Norris wrote:
> On Mon, Oct 20, 2014 at 10:46:13AM +0200, Boris Brezillon wrote:
> > 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.
>
> Can I get any ack's, tested-by's, or reviewed-by's?
>
I will add my tested-by in the Boris's next release.
thanks
Huang Shijie