[PATCH v4 2/5] mtd: nand: Add NAND_ECC_HW_ON_DIE ECC-mode.
Brian Norris
computersforpeace at gmail.com
Tue Apr 1 00:24:01 PDT 2014
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 <davidm at egauge.net>
> ---
> 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
More information about the linux-mtd
mailing list