1.) Why add the ECC information to the nand_chip{} ?
Each nand chip has its requirement for the ECC correctability, such as
"4bit ECC for each 512Byte" or "40bit ECC for each 1024Byte".
This ECC info is very important to the nand controller, such as gpmi.
Take the Micron MT29F64G08CBABA for example, its geometry is
8k page size, 744 bytes oob size and it requires 40bit ECC per 1K bytes.
If we do not provide the ECC info to the gpmi nand driver, it has to
calculate the ECC correctability itself. The gpmi driver will gets the 56bit
ECC for per 1K bytes which is beyond its BCH's 40bit ecc capibility.
The gpmi will quits in this case. But in actually, the gpmi can supports
this nand chip if it can get the right ECC info.
2.) About the patch set:
2.1) patch 1:
This patch adds some comment for the two fields which can be
treated as the initial semantics.
2.2) patch 2 ~ patch 5:
Parse out the ecc info from the extended parameter page for
ONFI nand.
2.2) patch 6 ~ patch 8:
Add the ECC info for full-id nand.
2.3) patch 9
The gpmi uses the ecc info to set the BCH module. and with this
patch set, the gpmi can supports the MT29F64G08CBABA now.
v2 --> v3:
[0] add a new patch to define the semantics of the two fields.
[1] Use the Change Read Column command to remove the "last" argument.
[2] simplify the onfi_feature().
[3] Use kmalloc() to replace kcalloc().
[4] others.
v1 --> v2:
[0] Since the first 3 patches are accepted, I do not send them in the
version 2.
[1] add NAND_ prefix for macros used by the full-id nands.
[2] add onfi_ prefix for the extend parameter page data structures.
[3] rename the onfi_get_feature() to onfi_feature().
[4] I re-test this patch set again.
Huang Shijie (9):
mtd: add more comment for ecc_strength/ecc_size
mtd: add data structures for Extended Parameter Page
mtd: add a helper to get the supported features for ONFI nand
mtd: get the ECC info from the Extended Parameter Page
mtd: replace the hardcode with the onfi_feature()
mtd: add a new field for ecc info in the nand_flash_dev{}
mtd: parse out the ECC info for the full-id nand chips
mtd: add the ecc info for some full-id nand chips
mtd: gpmi: set the BCH's geometry with the ecc info
drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 128 +++++++++++++++++++++++++++++++-
drivers/mtd/nand/nand_base.c | 80 +++++++++++++++++++-
drivers/mtd/nand/nand_ids.c | 8 +-
include/linux/mtd/nand.h | 63 +++++++++++++++-
4 files changed, 269 insertions(+), 10 deletions(-)
Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page
to store the ECC info.
The onfi spec tells us that if the nand chip's recommended ECC codeword
size is not 512 bytes, then the @ecc_bits is 0xff. The host _SHOULD_ then
read the Extended ECC information that is part of the extended parameter
page to retrieve the ECC requirements for this device.
This patch adds
[1] the neccessary fields for nand_onfi_params{},
[2] and adds the onfi_ext_ecc_info{} for Extended ECC information,
[3] adds onfi_ext_section{} for extended sections,
[4] and adds onfi_ext_param_page{} for the Extended Parameter Page.
Signed-off-by: Huang Shijie <[email protected]>
---
include/linux/mtd/nand.h | 39 ++++++++++++++++++++++++++++++++++++++-
1 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c1cc690..3f47ad9 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -224,7 +224,10 @@ struct nand_onfi_params {
__le16 revision;
__le16 features;
__le16 opt_cmd;
- u8 reserved[22];
+ u8 reserved0[2];
+ __le16 ext_param_page_length; /* since ONFI 2.1 */
+ u8 num_of_param_pages; /* since ONFI 2.1 */
+ u8 reserved1[17];
/* manufacturer information block */
char manufacturer[12];
@@ -281,6 +284,40 @@ struct nand_onfi_params {
#define ONFI_CRC_BASE 0x4F4E
+/* Extended ECC information Block Definition (since ONFI 2.1) */
+struct onfi_ext_ecc_info {
+ u8 ecc_bits;
+ u8 codeword_size;
+ __le16 bb_per_lun;
+ __le16 block_endurance;
+ u8 reserved[2];
+} __attribute__((packed));
+
+#define ONFI_SECTION_TYPE_0 0 /* Unused section. */
+#define ONFI_SECTION_TYPE_1 1 /* for additional sections. */
+#define ONFI_SECTION_TYPE_2 2 /* for ECC information. */
+struct onfi_ext_section {
+ u8 type;
+ u8 length;
+} __attribute__((packed));
+
+#define ONFI_EXT_SECTION_MAX 8
+
+/* Extended Parameter Page Definition (since ONFI 2.1) */
+struct onfi_ext_param_page {
+ __le16 crc;
+ u8 sig[4]; /* 'E' 'P' 'P' 'S' */
+ u8 reserved0[10];
+ struct onfi_ext_section sections[ONFI_EXT_SECTION_MAX];
+
+ /*
+ * The actual size of the Extended Parameter Page is in
+ * @ext_param_page_length of nand_onfi_params{}.
+ * The following are the variable length sections.
+ * So we do not add any fields below. Please see the ONFI spec.
+ */
+} __attribute__((packed));
+
/**
* struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
* @lock: protection lock
--
1.7.1
Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page
to store the ECC info.
The onfi spec tells us that if the nand chip's recommended ECC codeword
size is not 512 bytes, then the @ecc_bits is 0xff. The host _SHOULD_ then
read the Extended ECC information that is part of the extended parameter
page to retrieve the ECC requirements for this device.
This patch implement the reading of the Extended Parameter Page, and parses
the sections for ECC type, and get the ECC info from the ECC section.
Tested this patch with Micron MT29F64G08CBABAWP.
Signed-off-by: Huang Shijie <[email protected]>
---
drivers/mtd/nand/nand_base.c | 72 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 72 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index beff911..584cb0f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2846,6 +2846,73 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
return crc;
}
+/* Parse the Extended Parameter Page. */
+static int nand_flash_detect_ext_param_page(struct mtd_info *mtd,
+ struct nand_chip *chip, struct nand_onfi_params *p)
+{
+ struct onfi_ext_param_page *ep;
+ struct onfi_ext_section *s;
+ struct onfi_ext_ecc_info *ecc;
+ uint8_t *cursor;
+ int len;
+ int ret;
+ int i;
+
+ len = le16_to_cpu(p->ext_param_page_length) * 16;
+ ep = kmalloc(len, GFP_KERNEL);
+ if (!ep) {
+ pr_debug("Out of memory.\n");
+ ret = -ENOMEM;
+ goto ext_out;
+ }
+
+ /* Send our own NAND_CMD_PARAM. */
+ chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
+
+ /*
+ * Use the Change Read Columm command to skip the ONFI Parameter Pages.
+ * The nand_command() does not support the Change Read Columm command,
+ * use the nand_command_lp() instead.
+ */
+ nand_command_lp(mtd, NAND_CMD_RNDOUT,
+ sizeof(*p) * p->num_of_param_pages , -1);
+
+ /* Read out the Extended Parameter Page. */
+ chip->read_buf(mtd, (uint8_t *)ep, len);
+ if ((onfi_crc16(ONFI_CRC_BASE, ((uint8_t *)ep) + 2, len - 2)
+ != le16_to_cpu(ep->crc)) || strncmp(ep->sig, "EPPS", 4)) {
+ pr_debug("fail in the CRC.\n");
+ ret = -EINVAL;
+ goto ext_out;
+ }
+
+ /* find the ECC section. */
+ cursor = (uint8_t *)(ep + 1);
+ for (i = 0; i < ONFI_EXT_SECTION_MAX; i++) {
+ s = ep->sections + i;
+ if (s->type == ONFI_SECTION_TYPE_2)
+ break;
+ cursor += s->length * 16;
+ }
+ if (i == ONFI_EXT_SECTION_MAX) {
+ pr_debug("We can not find the ECC section.\n");
+ ret = -EINVAL;
+ goto ext_out;
+ }
+
+ /* get the info we want. */
+ ecc = (struct onfi_ext_ecc_info *)cursor;
+ chip->ecc_strength = ecc->ecc_bits;
+ chip->ecc_size = 1 << ecc->codeword_size;
+
+ pr_info("ONFI extended param page detected.\n");
+ return 0;
+
+ext_out:
+ kfree(ep);
+ return ret;
+}
+
/*
* Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
*/
@@ -2914,6 +2981,11 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
if (p->ecc_bits != 0xff) {
chip->ecc_strength = p->ecc_bits;
chip->ecc_size = 512;
+ } else if (chip->onfi_version >= 21 &&
+ (onfi_feature(chip) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
+ /* The Extended Parameter Page is supported since ONFI 2.1. */
+ if (nand_flash_detect_ext_param_page(mtd, chip, p))
+ pr_info("Failed to detect the extended param page.\n");
}
pr_info("ONFI flash detected\n");
--
1.7.1
Add the ecc info for TC58NVG2S0F, TC58NVG3S0F, TC58NVG5D2 and TC58NVG6D2.
>From these chips' datasheets, we know that:
The TC58NVG2S0F and TC58NVG3S0F require 4bit ECC for per 512byte.
The TC58NVG5D2 and TC58NVG6D2 require 40bits ECC for per 1024byte.
Signed-off-by: Huang Shijie <[email protected]>
---
drivers/mtd/nand/nand_ids.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index 683813a..a87b0a3 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -33,16 +33,16 @@ struct nand_flash_dev nand_flash_ids[] = {
*/
{"TC58NVG2S0F 4G 3.3V 8-bit",
{ .id = {0x98, 0xdc, 0x90, 0x26, 0x76, 0x15, 0x01, 0x08} },
- SZ_4K, SZ_512, SZ_256K, 0, 8, 224},
+ SZ_4K, SZ_512, SZ_256K, 0, 8, 224, NAND_ECC_INFO(4, SZ_512) },
{"TC58NVG3S0F 8G 3.3V 8-bit",
{ .id = {0x98, 0xd3, 0x90, 0x26, 0x76, 0x15, 0x02, 0x08} },
- SZ_4K, SZ_1K, SZ_256K, 0, 8, 232},
+ SZ_4K, SZ_1K, SZ_256K, 0, 8, 232, NAND_ECC_INFO(4, SZ_512) },
{"TC58NVG5D2 32G 3.3V 8-bit",
{ .id = {0x98, 0xd7, 0x94, 0x32, 0x76, 0x56, 0x09, 0x00} },
- SZ_8K, SZ_4K, SZ_1M, 0, 8, 640},
+ SZ_8K, SZ_4K, SZ_1M, 0, 8, 640, NAND_ECC_INFO(40, SZ_1K) },
{"TC58NVG6D2 64G 3.3V 8-bit",
{ .id = {0x98, 0xde, 0x94, 0x82, 0x76, 0x56, 0x04, 0x20} },
- SZ_8K, SZ_8K, SZ_2M, 0, 8, 640},
+ SZ_8K, SZ_8K, SZ_2M, 0, 8, 640, NAND_ECC_INFO(40, SZ_1K) },
LEGACY_ID_NAND("NAND 4MiB 5V 8-bit", 0x6B, 4, SZ_8K, SP_OPTIONS),
LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 4, SZ_8K, SP_OPTIONS),
--
1.7.1
Parse out the ECC information for the full-id nand chips.
Signed-off-by: Huang Shijie <[email protected]>
---
drivers/mtd/nand/nand_base.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8b2a792..b873220 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3249,6 +3249,8 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
chip->cellinfo = id_data[2];
chip->chipsize = (uint64_t)type->chipsize << 20;
chip->options |= type->options;
+ chip->ecc_strength = NAND_ECC_STRENGTH(type->ecc_info);
+ chip->ecc_size = NAND_ECC_SIZE(type->ecc_info);
*busw = type->options & NAND_BUSWIDTH_16;
--
1.7.1
If the nand chip provides us the ECC info, we can use it firstly.
The set_geometry_by_ecc_info() will use the ECC info, and
calculate the parameters we need.
Rename the old code to lagacy_set_geometry() which will takes effect
when there is no ECC info from the nand chip or we fails in the ECC info case.
Signed-off-by: Huang Shijie <[email protected]>
Signed-off-by: Huang Shijie <[email protected]>
---
drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 128 +++++++++++++++++++++++++++++++-
1 files changed, 127 insertions(+), 1 deletions(-)
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 717881a..e085ade 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -113,7 +113,128 @@ static inline bool gpmi_check_ecc(struct gpmi_nand_data *this)
return true;
}
-int common_nfc_set_geometry(struct gpmi_nand_data *this)
+/*
+ * If we can get the ECC information from the nand chip, we do not
+ * need to calculate them ourselves.
+ *
+ * We may have available oob space in this case.
+ */
+static bool set_geometry_by_ecc_info(struct gpmi_nand_data *this)
+{
+ struct bch_geometry *geo = &this->bch_geometry;
+ struct mtd_info *mtd = &this->mtd;
+ struct nand_chip *chip = mtd->priv;
+ struct nand_oobfree *of = gpmi_hw_ecclayout.oobfree;
+ unsigned int block_mark_bit_offset;
+
+ if (!(chip->ecc_strength > 0 && chip->ecc_size > 0))
+ return false;
+
+ switch (chip->ecc_size) {
+ case SZ_512:
+ geo->gf_len = 13;
+ break;
+ case SZ_1K:
+ geo->gf_len = 14;
+ break;
+ default:
+ dev_err(this->dev,
+ "unsupported nand chip. ecc bits : %d, ecc size : %d\n",
+ chip->ecc_strength, chip->ecc_size);
+ return false;
+ }
+ geo->ecc_chunk_size = chip->ecc_size;
+ geo->ecc_strength = round_up(chip->ecc_strength, 2);
+ if (!gpmi_check_ecc(this))
+ return false;
+
+ /* Keep the C >= O */
+ if (geo->ecc_chunk_size < mtd->oobsize) {
+ dev_err(this->dev,
+ "unsupported nand chip. ecc size: %d, oob size : %d\n",
+ chip->ecc_size, mtd->oobsize);
+ return false;
+ }
+
+ /* The default value, see comment in the lagacy_set_geometry(). */
+ geo->metadata_size = 10;
+
+ geo->ecc_chunk_count = mtd->writesize / geo->ecc_chunk_size;
+
+ /*
+ * Now, the NAND chip with 2K page(data chunk is 512byte) shows below:
+ *
+ * | P |
+ * |<----------------------------------------------------->|
+ * | |
+ * | (Block Mark) |
+ * | P' | | | |
+ * |<-------------------------------------------->| D | | O' |
+ * | |<---->| |<--->|
+ * V V V V V
+ * +---+----------+-+----------+-+----------+-+----------+-+-----+
+ * | M | data |E| data |E| data |E| data |E| |
+ * +---+----------+-+----------+-+----------+-+----------+-+-----+
+ *
+ * P : the page size for BCH module.
+ * E : The ECC strength.
+ * G : the length of Galois Field.
+ * N : The chunk count of per page.
+ * M : the metasize of per page.
+ * C : the ecc chunk size, aka the "data" above.
+ * P': the nand chip's page size.
+ * O : the nand chip's oob size.
+ * O': the free oob.
+ *
+ * The formula for P is :
+ *
+ * E * G * N
+ * P = ------------ + P' + M
+ * 8
+ *
+ * The position of block mark moves forward in the ECC-based view
+ * of page, and the delta is:
+ *
+ * E * G * (N - 1)
+ * D = (---------------- + M)
+ * 8
+ *
+ * Please see the comment in lagacy_set_geometry().
+ * With the condition C >= O , we still can get same result.
+ * So the bit position of the physical block mark within the ECC-based
+ * view of the page is :
+ * (P' - D) * 8
+ */
+ geo->page_size = mtd->writesize + geo->metadata_size +
+ (geo->gf_len * geo->ecc_strength * geo->ecc_chunk_count) / 8;
+
+ /* The available oob size we have. */
+ if (geo->page_size < mtd->writesize + mtd->oobsize) {
+ of->offset = geo->page_size - mtd->writesize;
+ of->length = mtd->oobsize - of->offset;
+ mtd->oobavail = gpmi_hw_ecclayout.oobavail = of->length;
+ }
+
+ geo->payload_size = mtd->writesize;
+
+ geo->auxiliary_status_offset = ALIGN(geo->metadata_size, 4);
+ geo->auxiliary_size = ALIGN(geo->metadata_size, 4)
+ + ALIGN(geo->ecc_chunk_count, 4);
+
+ if (!this->swap_block_mark)
+ return true;
+
+ /* For bit swap. */
+ block_mark_bit_offset = mtd->writesize * 8 -
+ (geo->ecc_strength * geo->gf_len * (geo->ecc_chunk_count - 1)
+ + geo->metadata_size * 8);
+
+ geo->block_mark_byte_offset = block_mark_bit_offset / 8;
+ geo->block_mark_bit_offset = block_mark_bit_offset % 8;
+ return true;
+}
+
+static int lagacy_set_geometry(struct gpmi_nand_data *this)
{
struct bch_geometry *geo = &this->bch_geometry;
struct mtd_info *mtd = &this->mtd;
@@ -225,6 +346,11 @@ int common_nfc_set_geometry(struct gpmi_nand_data *this)
return 0;
}
+int common_nfc_set_geometry(struct gpmi_nand_data *this)
+{
+ return set_geometry_by_ecc_info(this) ? 0 : lagacy_set_geometry(this);
+}
+
struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
{
int chipnr = this->current_chip;
--
1.7.1
The current code uses the hardcode to detect the 16-bit bus width.
Use the onfi_feature() to replace it.
Signed-off-by: Huang Shijie <[email protected]>
---
drivers/mtd/nand/nand_base.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 584cb0f..8b2a792 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2974,9 +2974,9 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
chip->chipsize = le32_to_cpu(p->blocks_per_lun);
chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
- *busw = 0;
- if (le16_to_cpu(p->features) & 1)
- *busw = NAND_BUSWIDTH_16;
+
+ *busw = (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS) ?
+ NAND_BUSWIDTH_16 : 0;
if (p->ecc_bits != 0xff) {
chip->ecc_strength = p->ecc_bits;
--
1.7.1
Add the @ecc_info in the nand_flash_dev{}.
The lower 16 bits are used to store the ECC bits, while the upper 16 bits
are used to store the ECC data chunk size.
Signed-off-by: Huang Shijie <[email protected]>
---
include/linux/mtd/nand.h | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 063e517..f4c7777 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -624,6 +624,10 @@ struct nand_chip {
{ .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \
.options = (opts) }
+#define NAND_ECC_INFO(strength, size) (((strength) << 16) | (size))
+#define NAND_ECC_STRENGTH(x) (((x) >> 16) & 0xffff)
+#define NAND_ECC_SIZE(x) ((x) & 0xffff)
+
/**
* struct nand_flash_dev - NAND Flash Device ID Structure
* @name: a human-readable name of the NAND chip
@@ -641,6 +645,11 @@ struct nand_chip {
* @options: stores various chip bit options
* @id_len: The valid length of the @id.
* @oobsize: OOB size
+ * @ecc_info: The ECC information.
+ * lower 16 bits: store the ECC bits.
+ * upper 16 bits: store the ECC data chunk size.
+ * For example, the "4bit ECC for each 512Byte" can be set with
+ * NAND_ECC_INFO(4, 512) macro.
*/
struct nand_flash_dev {
char *name;
@@ -657,6 +666,7 @@ struct nand_flash_dev {
unsigned int options;
uint16_t id_len;
uint16_t oobsize;
+ uint32_t ecc_info;
};
/**
--
1.7.1
From: Huang Shijie <[email protected]>
add a helper to get the supported features for ONFI nand.
Also add the neccessary macros.
Signed-off-by: Huang Shijie <[email protected]>
---
include/linux/mtd/nand.h | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 3f47ad9..063e517 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -202,6 +202,10 @@ typedef enum {
/* Keep gcc happy */
struct nand_chip;
+/* ONFI features */
+#define ONFI_FEATURE_16_BIT_BUS (1 << 0)
+#define ONFI_FEATURE_EXT_PARAM_PAGE (1 << 7)
+
/* ONFI timing mode, used in both asynchronous and synchronous mode */
#define ONFI_TIMING_MODE_0 (1 << 0)
#define ONFI_TIMING_MODE_1 (1 << 1)
@@ -752,6 +756,12 @@ struct platform_nand_chip *get_platform_nandchip(struct mtd_info *mtd)
return chip->priv;
}
+/* return the supported features. */
+static inline int onfi_feature(struct nand_chip *chip)
+{
+ return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0;
+}
+
/* return the supported asynchronous timing mode. */
static inline int onfi_get_async_timing_mode(struct nand_chip *chip)
{
--
1.7.1
Add more commit for ecc_strength and ecc_size fields.
We can treat the comment as the initial semantics for the two fields.
Signed-off-by: Huang Shijie <[email protected]>
---
include/linux/mtd/nand.h | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 5458021..c1cc690 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -436,7 +436,9 @@ struct nand_buffers {
* @cellinfo: [INTERN] MLC/multichip data from chip ident
* @ecc_strength: [INTERN] ECC correctability from the datasheet.
* @ecc_size: [INTERN] ECC size required by the @ecc_strength,
- * also from the datasheet.
+ * also from the datasheet. If we do not know the
+ * @ecc_size and @ecc_strength of the nand chips, we can
+ * set zeros to them which are the default value.
* @numchips: [INTERN] number of physical chips
* @chipsize: [INTERN] the size of one chip for multichip arrays
* @pagemask: [INTERN] page number mask = number of (pages / chip) - 1
--
1.7.1
On Tue, Apr 23, 2013 at 1:54 AM, Huang Shijie <[email protected]> wrote:
> Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page
> to store the ECC info.
>
> The onfi spec tells us that if the nand chip's recommended ECC codeword
> size is not 512 bytes, then the @ecc_bits is 0xff. The host _SHOULD_ then
> read the Extended ECC information that is part of the extended parameter
> page to retrieve the ECC requirements for this device.
>
> This patch implement the reading of the Extended Parameter Page, and parses
> the sections for ECC type, and get the ECC info from the ECC section.
>
> Tested this patch with Micron MT29F64G08CBABAWP.
>
> Signed-off-by: Huang Shijie <[email protected]>
> ---
> drivers/mtd/nand/nand_base.c | 72 ++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 72 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index beff911..584cb0f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2846,6 +2846,73 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
> return crc;
> }
>
> +/* Parse the Extended Parameter Page. */
> +static int nand_flash_detect_ext_param_page(struct mtd_info *mtd,
> + struct nand_chip *chip, struct nand_onfi_params *p)
> +{
> + struct onfi_ext_param_page *ep;
> + struct onfi_ext_section *s;
> + struct onfi_ext_ecc_info *ecc;
> + uint8_t *cursor;
> + int len;
> + int ret;
> + int i;
> +
> + len = le16_to_cpu(p->ext_param_page_length) * 16;
> + ep = kmalloc(len, GFP_KERNEL);
> + if (!ep) {
> + pr_debug("Out of memory.\n");
I believe people are trying to kill such messages from the kernel. If
kmalloc() fails to allocate memory, the mm system should give plenty
of scary and helpful messages anyway. This print message just becomes
bloat, then.
> + ret = -ENOMEM;
> + goto ext_out;
> + }
> +
> + /* Send our own NAND_CMD_PARAM. */
> + chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> +
> + /*
> + * Use the Change Read Columm command to skip the ONFI Parameter Pages.
> + * The nand_command() does not support the Change Read Columm command,
> + * use the nand_command_lp() instead.
> + */
> + nand_command_lp(mtd, NAND_CMD_RNDOUT,
> + sizeof(*p) * p->num_of_param_pages , -1);
No, you cannot do this. Some drivers will provide their own cmdfunc,
so nand_command_lp() is unexpected for those drivers.
Your problem seems, instead, that you are executing this function too
early, before nand_flash_get_type() is able to assign
nand_command_lp() to be your cmdfunc. You might just want to call this
function after the following lines in nand_flash_get_type():
/* Do not replace user supplied command function! */
if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
chip->cmdfunc = nand_command_lp;
> + /* Read out the Extended Parameter Page. */
> + chip->read_buf(mtd, (uint8_t *)ep, len);
> + if ((onfi_crc16(ONFI_CRC_BASE, ((uint8_t *)ep) + 2, len - 2)
> + != le16_to_cpu(ep->crc)) || strncmp(ep->sig, "EPPS", 4)) {
> + pr_debug("fail in the CRC.\n");
> + ret = -EINVAL;
> + goto ext_out;
> + }
> +
> + /* find the ECC section. */
> + cursor = (uint8_t *)(ep + 1);
> + for (i = 0; i < ONFI_EXT_SECTION_MAX; i++) {
> + s = ep->sections + i;
> + if (s->type == ONFI_SECTION_TYPE_2)
> + break;
> + cursor += s->length * 16;
> + }
> + if (i == ONFI_EXT_SECTION_MAX) {
> + pr_debug("We can not find the ECC section.\n");
> + ret = -EINVAL;
> + goto ext_out;
> + }
> +
> + /* get the info we want. */
> + ecc = (struct onfi_ext_ecc_info *)cursor;
> + chip->ecc_strength = ecc->ecc_bits;
> + chip->ecc_size = 1 << ecc->codeword_size;
> +
> + pr_info("ONFI extended param page detected.\n");
> + return 0;
> +
> +ext_out:
> + kfree(ep);
> + return ret;
> +}
> +
> /*
> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> */
> @@ -2914,6 +2981,11 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> if (p->ecc_bits != 0xff) {
> chip->ecc_strength = p->ecc_bits;
> chip->ecc_size = 512;
> + } else if (chip->onfi_version >= 21 &&
> + (onfi_feature(chip) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
> + /* The Extended Parameter Page is supported since ONFI 2.1. */
> + if (nand_flash_detect_ext_param_page(mtd, chip, p))
> + pr_info("Failed to detect the extended param page.\n");
> }
>
> pr_info("ONFI flash detected\n");
Brian
I would recommend different wording, describing what they represent
rather than how we accomplish it. See below.
On Tue, Apr 23, 2013 at 1:54 AM, Huang Shijie <[email protected]> wrote:
> Add more commit for ecc_strength and ecc_size fields.
> We can treat the comment as the initial semantics for the two fields.
>
> Signed-off-by: Huang Shijie <[email protected]>
> ---
> include/linux/mtd/nand.h | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 5458021..c1cc690 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -436,7 +436,9 @@ struct nand_buffers {
> * @cellinfo: [INTERN] MLC/multichip data from chip ident
> * @ecc_strength: [INTERN] ECC correctability from the datasheet.
How about just: "minimum number of bits correctability, if known; if
unknown, set to 0
> * @ecc_size: [INTERN] ECC size required by the @ecc_strength,
> - * also from the datasheet.
> + * also from the datasheet. If we do not know the
> + * @ecc_size and @ecc_strength of the nand chips, we can
> + * set zeros to them which are the default value.
"recommend ECC step size, if known; if unknown, set to 0"
> * @numchips: [INTERN] number of physical chips
> * @chipsize: [INTERN] the size of one chip for multichip arrays
> * @pagemask: [INTERN] page number mask = number of (pages / chip) - 1
Brian
于 2013年04月25日 14:20, Brian Norris 写道:
> No, you cannot do this. Some drivers will provide their own cmdfunc,
> so nand_command_lp() is unexpected for those drivers.
ok. got it.
> Your problem seems, instead, that you are executing this function too
> early, before nand_flash_get_type() is able to assign
yes. This is just the case i meet.
> nand_command_lp() to be your cmdfunc. You might just want to call this
> function after the following lines in nand_flash_get_type():
>
> /* Do not replace user supplied command function! */
> if (mtd->writesize> 512&& chip->cmdfunc == nand_command)
> chip->cmdfunc = nand_command_lp;
>
I will send a fix patch about this patch.
thanks
Huang Shijie
于 2013年04月25日 14:32, Brian Norris 写道:
>> * @numchips: [INTERN] number of physical chips
>> > * @chipsize: [INTERN] the size of one chip for multichip arrays
>> > * @pagemask: [INTERN] page number mask = number of (pages / chip) - 1
> Brian
>
于 2013年04月25日 14:32, Brian Norris 写道:
> How about just: "minimum number of bits correctability, if known; if
> unknown, set to 0
>
ok. no problem.
thanks
Huang Shijie
Hi Huang,
On Tue, Apr 23, 2013 at 1:54 AM, Huang Shijie <[email protected]> wrote:
> v2 --> v3:
> [0] add a new patch to define the semantics of the two fields.
> [1] Use the Change Read Column command to remove the "last" argument.
> [2] simplify the onfi_feature().
> [3] Use kmalloc() to replace kcalloc().
> [4] others.
Please see especially my comment on your usage of nand_command_lp. But
other than that, I think that by the time we get to v4, this series
should be good. Anyway, I'll try to reserve any more nitpick-y
comments for minor things to my own follow-up patch(es). Thanks for
the work.
Brian
于 2013年04月25日 14:40, Brian Norris 写道:
> should be good. Anyway, I'll try to reserve any more nitpick-y
not at all :) .
Without your review, my patch set will wait for more long time...
> comments for minor things to my own follow-up patch(es). Thanks for
> the work.
I really appreciate for your reviews.
thanks
Huang Shijie
Last nitpicks for v3 :)
On Tue, Apr 23, 2013 at 1:54 AM, Huang Shijie <[email protected]> wrote:
> Add the @ecc_info in the nand_flash_dev{}.
> The lower 16 bits are used to store the ECC bits, while the upper 16 bits
> are used to store the ECC data chunk size.
A bit late on this one, but is there a good reason this wasn't just 2
separate 16-bit fields? We already have a few, and I don't see why
this couldn't be the same.
> Signed-off-by: Huang Shijie <[email protected]>
> ---
> include/linux/mtd/nand.h | 10 ++++++++++
> 1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 063e517..f4c7777 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -624,6 +624,10 @@ struct nand_chip {
> { .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \
> .options = (opts) }
>
> +#define NAND_ECC_INFO(strength, size) (((strength) << 16) | (size))
We could redefine this:
#define NAND_ECC_INFO(strength, size) .ecc_strength = (strength),
.ecc_size = (size)
> +#define NAND_ECC_STRENGTH(x) (((x) >> 16) & 0xffff)
> +#define NAND_ECC_SIZE(x) ((x) & 0xffff)
Then we could just drop these unpacking macros.
> /**
> * struct nand_flash_dev - NAND Flash Device ID Structure
> * @name: a human-readable name of the NAND chip
> @@ -641,6 +645,11 @@ struct nand_chip {
> * @options: stores various chip bit options
> * @id_len: The valid length of the @id.
> * @oobsize: OOB size
> + * @ecc_info: The ECC information.
> + * lower 16 bits: store the ECC bits.
> + * upper 16 bits: store the ECC data chunk size.
> + * For example, the "4bit ECC for each 512Byte" can be set with
> + * NAND_ECC_INFO(4, 512) macro.
> */
> struct nand_flash_dev {
> char *name;
> @@ -657,6 +666,7 @@ struct nand_flash_dev {
> unsigned int options;
> uint16_t id_len;
> uint16_t oobsize;
> + uint32_t ecc_info;
This could be split to:
uint16_t ecc_strength;
uint16_t ecc_size;
> };
>
> /**
Brian
于 2013年04月25日 14:57, Brian Norris 写道:
>
> A bit late on this one, but is there a good reason this wasn't just 2
> separate 16-bit fields? We already have a few, and I don't see why
> this couldn't be the same.
>
I just want to make the ecc_strength/ecc_size more coupled for the
nand_flash_dev{}.
If we spilit to two fields. It makes the nand_flash_ids[] less readable.
>> Signed-off-by: Huang Shijie<[email protected]>
>> ---
>> include/linux/mtd/nand.h | 10 ++++++++++
>> 1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index 063e517..f4c7777 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -624,6 +624,10 @@ struct nand_chip {
>> { .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \
>> .options = (opts) }
>>
>> +#define NAND_ECC_INFO(strength, size) (((strength)<< 16) | (size))
> We could redefine this:
>
> #define NAND_ECC_INFO(strength, size) .ecc_strength = (strength),
> .ecc_size = (size)
Do this type of macro could be accepted by the kernel?
I think your macro needs a pair of bracket, such as:
#define NAND_ECC_INFO(strength, size) (.ecc_strength = (strength),
.ecc_size = (size))
But if we add the brackets, we will meet a compiler error.
thanks
Huang Shijie
On Thu, Apr 25, 2013 at 1:56 AM, Huang Shijie <[email protected]> wrote:
> ?? 2013??04??25?? 14:57, Brian Norris д??:
>
>>
>> A bit late on this one, but is there a good reason this wasn't just 2
>> separate 16-bit fields? We already have a few, and I don't see why
>> this couldn't be the same.
>>
> I just want to make the ecc_strength/ecc_size more coupled for the
> nand_flash_dev{}.
> If we spilit to two fields. It makes the nand_flash_ids[] less readable.
Sure, that's a decent reason. But how about as an instance of an
anonymous struct instead? See my suggestion below.
>>> Signed-off-by: Huang Shijie<[email protected]>
>>> ---
>>> include/linux/mtd/nand.h | 10 ++++++++++
>>> 1 files changed, 10 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>>> index 063e517..f4c7777 100644
>>> --- a/include/linux/mtd/nand.h
>>> +++ b/include/linux/mtd/nand.h
>>> @@ -624,6 +624,10 @@ struct nand_chip {
>>> { .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \
>>> .options = (opts) }
>>>
>>> +#define NAND_ECC_INFO(strength, size) (((strength)<< 16) | (size))
>>
>> We could redefine this:
>>
>> #define NAND_ECC_INFO(strength, size) .ecc_strength = (strength),
>> .ecc_size = (size)
>
> Do this type of macro could be accepted by the kernel?
Something like this should be acceptable, I think, if I can straighten
it out so that it compiles right (!)
> I think your macro needs a pair of bracket, such as:
> #define NAND_ECC_INFO(strength, size) (.ecc_strength = (strength), .ecc_size
> = (size))
>
> But if we add the brackets, we will meet a compiler error.
Here's my modification (note the underscores, since we can't have the
field .strength in the same macro as the "parameter" strength):
#define ECC_INFO(_strength, _size) { .strength = (_strength),
.size = (_size) }
#define ECC_STRENGTH(type) ((type)->ecc.strength)
#define ECC_SIZE(type) ((type)->ecc.strength)
And replace the field in nand_flash_dev with:
struct {
uint16_t strength;
uint16_t size;
} ecc;
How does that look? It's actually quite similar to the construct Artem
used with mfr_id and dev_id, except that we give the struct a name.
And this time, it actually compiles!
Brian
?? 2013??04??26?? 14:15, Brian Norris д??:
> How does that look? It's actually quite similar to the construct Artem
> used with mfr_id and dev_id, except that we give the struct a name.
> And this time, it actually compiles!
>
It's ok to me. :)
I will use it in the next version.
thanks
Huang Shijie