On Tue, 23 Sep 2014 19:16:19 +0200 Boris BREZILLON <[email protected]> wrote:
> On Wed, 24 Sep 2014 00:10:44 +0800 > Huang Shijie <[email protected]> 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 <[email protected]> > > > --- > > > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 126 > > > +++++++++++++++++++++++++++++++++ > > > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 2 + > > > 2 files changed, 128 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > > index 959cb9b..7921ba7 100644 > > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > > @@ -791,6 +791,7 @@ static void gpmi_free_dma_buffer(struct > > > gpmi_nand_data *this) > > > this->page_buffer_phys); > > > kfree(this->cmd_buffer); > > > kfree(this->data_buffer_dma); > > > + kfree(this->raw_buffer); > > > > > > this->cmd_buffer = NULL; > > > this->data_buffer_dma = NULL; > > > @@ -837,6 +838,9 @@ static int gpmi_alloc_dma_buffer(struct > > > gpmi_nand_data *this) > > > if (!this->page_buffer_virt) > > > goto error_alloc; > > > > > > + this->raw_buffer = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL); > > > + if (!this->raw_buffer) > > > + goto error_alloc; > > > > > > /* Slice up the page buffer. */ > > > this->payload_virt = this->page_buffer_virt; > > > @@ -1347,6 +1351,126 @@ gpmi_ecc_write_oob(struct mtd_info *mtd, struct > > > nand_chip *chip, int page) > > > return status & NAND_STATUS_FAIL ? -EIO : 0; > > > } > > > > > > +static int gpmi_ecc_read_page_raw(struct mtd_info *mtd, > > > + struct nand_chip *chip, uint8_t *buf, > > > + int oob_required, int page) > > > > In actually, i suggest to call the ecc->read_page() in this > > hook. And after the ecc->read_page(), copy the relative data to > > the relative buffers. Is my suggestion right? please correct me. > > Unfortunately it's not. The user expect that ECC correction is not > involved when accessing the NAND in raw mode, which is not the case in > your read_page implementation. > This is particularly useful when one want to see the real page status > (including bitflips). > > Moreover, I like to see the generated ECC bytes/bits when using raw > access (but I'm not sure this is a requirement). This will help > deducing the BCH algorithm and/or XOR mask applied after producing > these bits if someone ever want to spend some time reverse engineering > it. > > > > > > > > > > +{ > > > + struct gpmi_nand_data *this = chip->priv; > > > + struct bch_geometry *nfc_geo = &this->bch_geometry; > > > + int eccsize = nfc_geo->ecc_chunk_size; > > > + int eccbits = nfc_geo->ecc_strength * nfc_geo->gf_len; > > > + u8 *tmp_buf = this->raw_buffer; > > > + size_t src_bit_off; > > > + size_t oob_bit_off; > > > + size_t oob_byte_off; > > > + uint8_t *oob = chip->oob_poi; > > > + int step; > > > + > > > + chip->read_buf(mtd, tmp_buf, > > > + mtd->writesize + mtd->oobsize); > > > + > > > + if (this->swap_block_mark) { > > > + u8 swap = tmp_buf[0]; > > > + > > > + tmp_buf[0] = tmp_buf[mtd->writesize]; > > > + tmp_buf[mtd->writesize] = swap; > > > + } > > > + > > > + if (oob_required) > > > + memcpy(oob, tmp_buf, nfc_geo->metadata_size); > > > + > > > + oob_bit_off = nfc_geo->metadata_size * 8; > > > + src_bit_off = oob_bit_off; > > > + > > > + for (step = 0; step < nfc_geo->ecc_chunk_count; step++) { > > > + if (buf) > > > + gpmi_move_bits(buf, step * eccsize * 8, > > > + tmp_buf, src_bit_off, > > > + eccsize * 8); > > > + src_bit_off += eccsize * 8; > > > + > > > + if (oob_required) > > > + gpmi_move_bits(oob, oob_bit_off, > > > + tmp_buf, src_bit_off, > > > + eccbits); > > > + > > > + src_bit_off += eccbits; > > > + oob_bit_off += eccbits; > > > + } > > > + > > > + if (oob_required && oob_bit_off % 8) > > > + oob[oob_bit_off / 8] &= GENMASK(oob_bit_off - 1, 0); > > > + > > > + oob_byte_off = DIV_ROUND_UP(oob_bit_off, 8); > > > + > > > + if (oob_required && oob_byte_off < mtd->oobsize) > > > + memcpy(oob + oob_byte_off, > > > + tmp_buf + mtd->writesize + oob_byte_off, > > > + mtd->oobsize - oob_byte_off); > > > + > > > + return 0; > > > +} > > > + > > > +static int gpmi_ecc_write_page_raw(struct mtd_info *mtd, > > > + struct nand_chip *chip, > > > + const uint8_t *buf, > > > + int oob_required) > > > +{ > > > > > > give me more time to think over this hook :( > > Sure, no problem. > > Actually it's doing the exact same thing read_page_raw is doing expect > it's applying on a write access. > I copy the provided data (in-band and out-of-band is required) into a ^ if > temporary buffer to match the GPMI controller layout, then write it > without involving the BCH block (which means no ECC bits generation). > > Best Regards, > > Boris > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
