From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pb0-x230.google.com ([2607:f8b0:400e:c01::230]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WUt3f-0000zo-VB for linux-mtd@lists.infradead.org; Tue, 01 Apr 2014 07:24:33 +0000 Received: by mail-pb0-f48.google.com with SMTP id md12so9383943pbc.21 for ; Tue, 01 Apr 2014 00:24:08 -0700 (PDT) Date: Tue, 1 Apr 2014 00:24:01 -0700 From: Brian Norris To: David Mosberger Subject: Re: [PATCH v4 2/5] mtd: nand: Add NAND_ECC_HW_ON_DIE ECC-mode. Message-ID: <20140401072401.GB6400@brian-ubuntu> References: <1396308537-16013-1-git-send-email-davidm@egauge.net> <1396308537-16013-3-git-send-email-davidm@egauge.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1396308537-16013-3-git-send-email-davidm@egauge.net> Cc: gsi@denx.de, linux-mtd@lists.infradead.org, pekon@ti.com, dedekind1@gmail.com List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Mon, Mar 31, 2014 at 05:28:54PM -0600, David Mosberger wrote: > Some Micron chips such as the MT29F4G16ABADAWP have an internal > (on-die) ECC-controller that is useful when the host-platform doesn't > have hardware-support for multi-bit ECC. This patch adds > NAND_ECC_HW_ON_DIE to support such chips when the driver detects that > the on-die ECC controller is enabled. > > Signed-off-by: David Mosberger > --- > drivers/mtd/nand/nand_base.c | 133 ++++++++++++++++++++++++++++++++++++++++++ > drivers/of/of_mtd.c | 1 + > include/linux/mtd/nand.h | 2 + > 3 files changed, 136 insertions(+) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index f145f00..7047e9f5 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -78,6 +78,20 @@ static struct nand_ecclayout nand_oob_64 = { > .length = 38} } > }; > > +static struct nand_ecclayout nand_oob_64_on_die = { This is not the only possible layout for on-die ECC. It's probably not even the only one used by Micron, is it? What about larger page sizes, for instance? > + .eccbytes = 32, > + .eccpos = { > + 8, 9, 10, 11, 12, 13, 14, 15, > + 24, 25, 26, 27, 28, 29, 30, 31, > + 40, 41, 42, 43, 44, 45, 46, 47, > + 56, 57, 58, 59, 60, 61, 62, 63}, > + .oobfree = { > + {.offset = 4, .length = 4}, > + {.offset = 20, .length = 4}, > + {.offset = 36, .length = 4}, > + {.offset = 52, .length = 4}} > +}; > + > static struct nand_ecclayout nand_oob_128 = { > .eccbytes = 48, > .eccpos = { > @@ -1258,6 +1272,105 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip, > return max_bitflips; > } > > +static int check_read_status_on_die(struct mtd_info *mtd, > + struct nand_chip *chip, int page) > +{ > + int max_bitflips = 0; > + uint8_t status; > + > + /* Check ECC status of page just transferred into NAND's page buffer: */ > + chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1); > + status = chip->read_byte(mtd); > + > + /* Switch back to data reading: */ > + chip->cmdfunc(mtd, NAND_CMD_READMODE, -1, -1); > + > + if (status & NAND_STATUS_FAIL) { > + /* Page has invalid ECC. */ > + mtd->ecc_stats.failed++; > + } else if (status & NAND_STATUS_REWRITE) { > + /* > + * Simple but suboptimal: any page with a single stuck > + * bit will be unusable since it'll be rewritten on > + * each read... > + */ > + max_bitflips = mtd->bitflip_threshold; > + } > + return max_bitflips; > +} > + > +/** > + * nand_read_subpage_on_die - [REPLACEABLE] raw sub-page read function > + * @mtd: mtd info structure > + * @chip: nand chip info structure > + * @data_offs: offset of requested data within the page > + * @readlen: data length > + * @bufpoi: buffer to store read data > + */ > +static int nand_read_subpage_on_die(struct mtd_info *mtd, > + struct nand_chip *chip, > + uint32_t data_offs, uint32_t readlen, > + uint8_t *bufpoi, int page) > +{ > + int start_step, end_step, num_steps, ret; > + int data_col_addr; > + int datafrag_len; > + uint32_t failed; > + uint8_t *p; > + > + /* Column address within the page aligned to ECC size */ > + start_step = data_offs / chip->ecc.size; > + end_step = (data_offs + readlen - 1) / chip->ecc.size; > + num_steps = end_step - start_step + 1; > + > + /* Data size aligned to ECC ecc.size */ > + datafrag_len = num_steps * chip->ecc.size; > + data_col_addr = start_step * chip->ecc.size; > + p = bufpoi + data_col_addr; > + > + failed = mtd->ecc_stats.failed; > + > + ret = check_read_status_on_die(mtd, chip, page); > + if (ret < 0 || mtd->ecc_stats.failed != failed) { > + memset(p, 0, datafrag_len); > + return ret; > + } > + > + /* If we read not a page aligned data */ > + if (data_col_addr != 0) > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_col_addr, -1); > + > + chip->read_buf(mtd, p, datafrag_len); > + > + return ret; > +} > + > +/** > + * nand_read_page_on_die - [INTERN] read raw page data without ecc > + * @mtd: mtd info structure > + * @chip: nand chip info structure > + * @buf: buffer to store read data > + * @oob_required: caller requires OOB data read to chip->oob_poi > + * @page: page number to read > + */ > +static int nand_read_page_on_die(struct mtd_info *mtd, struct nand_chip *chip, > + uint8_t *buf, int oob_required, int page) > +{ > + uint32_t failed; > + int ret; > + > + failed = mtd->ecc_stats.failed; > + > + ret = check_read_status_on_die(mtd, chip, page); > + if (ret < 0 || mtd->ecc_stats.failed != failed) > + return ret; > + > + chip->read_buf(mtd, buf, mtd->writesize); > + if (oob_required) > + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); > + return ret; > +} > + > /** > * nand_read_page_hwecc - [REPLACEABLE] hardware ECC based page read function > * @mtd: mtd info structure > @@ -3069,6 +3182,7 @@ nand_onfi_detect_micron_on_die_ecc(struct mtd_info *mtd, > * If the chip has on-die ECC enabled, we kind of have > * to do the same... > */ > + chip->ecc.mode = NAND_ECC_HW_ON_DIE; Nope. nand_base does not make choices about the ECC type to use; that's the job of the low-level / hardware driver. Refer to my comment on the first patch; drivers should opt into this somehow. > pr_info("Using on-die ECC\n"); > } > } > @@ -3985,6 +4099,25 @@ int nand_scan_tail(struct mtd_info *mtd) > ecc->strength = ecc->bytes * 8 / fls(8 * ecc->size); > break; > > + case NAND_ECC_HW_ON_DIE: > + /* nand_bbt attempts to put Bbt marker at offset 8 in s/Bbt/BBT/ Also, please see Documentation/CodingStyle regarding the proper multiline comment style. > + oob, which is used for ECC by Micron s/oob/OOB/ > + MT29F4G16ABADAWP, for example. Fixed by not using > + OOB for BBT marker. */ > + chip->bbt_options |= NAND_BBT_NO_OOB; > + chip->ecc.layout = &nand_oob_64_on_die; > + chip->ecc.read_page = nand_read_page_on_die; > + chip->ecc.read_subpage = nand_read_subpage_on_die; > + chip->ecc.write_page = nand_write_page_raw; > + chip->ecc.read_oob = nand_read_oob_std; > + chip->ecc.read_page_raw = nand_read_page_raw; > + chip->ecc.write_page_raw = nand_write_page_raw; > + chip->ecc.write_oob = nand_write_oob_std; > + chip->ecc.size = 512; > + chip->ecc.bytes = 8; > + chip->ecc.strength = 4; This is all way too specific to the particular Micron flash you're looking at. There are other vendors that use on-die ECC, and any support here should be written to accommodate future additions (by Micron or other vendors). Particularly: the ECC strength/size should be determined per vendor/flash, not here. Can you get this from ONFI or the ID? At least the ecc.{strength,bytes,size} should probably be assigned from the flash-vendor-specific callback. > + break; > + > case NAND_ECC_NONE: > pr_warn("NAND_ECC_NONE selected by board driver. " > "This is not recommended!\n"); > diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c > index b7361ed..c844c84 100644 > --- a/drivers/of/of_mtd.c > +++ b/drivers/of/of_mtd.c > @@ -23,6 +23,7 @@ static const char *nand_ecc_modes[] = { > [NAND_ECC_HW_SYNDROME] = "hw_syndrome", > [NAND_ECC_HW_OOB_FIRST] = "hw_oob_first", > [NAND_ECC_SOFT_BCH] = "soft_bch", > + [NAND_ECC_HW_ON_DIE] = "hw_on_die", > }; > > /** > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 780ab58..dbb99b3 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -112,6 +112,7 @@ extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len); > /* Status bits */ > #define NAND_STATUS_FAIL 0x01 > #define NAND_STATUS_FAIL_N1 0x02 > +#define NAND_STATUS_REWRITE 0x08 How "standard" is this? Is this a Micron-specific status bit? If so, we should note that in a comment. > #define NAND_STATUS_TRUE_READY 0x20 > #define NAND_STATUS_READY 0x40 > #define NAND_STATUS_WP 0x80 > @@ -126,6 +127,7 @@ typedef enum { > NAND_ECC_HW_SYNDROME, > NAND_ECC_HW_OOB_FIRST, > NAND_ECC_SOFT_BCH, > + NAND_ECC_HW_ON_DIE, > } nand_ecc_modes_t; > > /* Brian