Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
On Tue, 23 Sep 2014 19:16:19 +0200
Boris BREZILLON wrote:
> On Wed, 24 Sep 2014 00:10:44 +0800
> Huang Shijie 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
> > > ---
> > > 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 = >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);
> > > +
> > > +
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
On Wed, 24 Sep 2014 00:10:44 +0800
Huang Shijie 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
> > ---
> > 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 = >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);
> > +
> > +
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
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
> ---
> 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 = >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 =
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
On Tue, 23 Sep 2014 23:17:41 +0800
Huang Shijie 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
> > ---
> > 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 = >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 -
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
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
> ---
> 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 = >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 = >bch_geometry;
> + int eccsize =
[PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
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
---
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 = >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 = >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);
+
[PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
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.brezil...@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 =
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
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.brezil...@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 =
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
On Tue, 23 Sep 2014 23:17:41 +0800
Huang Shijie shij...@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.brezil...@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
--
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
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.brezil...@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
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
On Wed, 24 Sep 2014 00:10:44 +0800
Huang Shijie shij...@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.brezil...@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 +
Re: [PATCH v3 2/3] mtd: nand: gpmi: add proper raw access support
On Tue, 23 Sep 2014 19:16:19 +0200
Boris BREZILLON boris.brezil...@free-electrons.com wrote:
On Wed, 24 Sep 2014 00:10:44 +0800
Huang Shijie shij...@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.brezil...@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
