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.
v3 --> v4:
[0] remove the printk for "out of memory".
[1] remove the hardcode nand_command_lp(). Update the chip->cmd_func
before we call the nand_flash_detect_ext_param_page().
[2] split the ecc_info to two fields for full id nand.
[3] update the comments for ecc_strength/ecc_size (from Brian).
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.
*** BLURB HERE ***
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 ECC info for 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 | 85 ++++++++++++++++++++-
drivers/mtd/nand/nand_ids.c | 8 +-
include/linux/mtd/nand.h | 69 +++++++++++++++++-
4 files changed, 280 insertions(+), 10 deletions(-)
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 abec615..bc3442a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2969,9 +2969,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 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
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
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 bc3442a..5fe65ad 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3254,6 +3254,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);
+ chip->ecc_size = NAND_ECC_SIZE(type);
*busw = type->options & NAND_BUSWIDTH_16;
--
1.7.1
Add an instance of an anonymous struct to store the ECC infor for full id
nand chips.
@ecc.strength: ECC correctability from the datasheet.
@ecc.size: ECC size required by the @ecc.strength,
Also add the necessary macros to make the code simple and clean.
Signed-off-by: Huang Shijie <[email protected]>
---
include/linux/mtd/nand.h | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9b28739..5f1c0d1 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -625,6 +625,11 @@ struct nand_chip {
{ .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \
.options = (opts) }
+#define NAND_ECC_INFO(_strength, _size) \
+ { .strength = (_strength), .size = (_size) }
+#define NAND_ECC_STRENGTH(type) ((type)->ecc.strength)
+#define NAND_ECC_SIZE(type) ((type)->ecc.size)
+
/**
* struct nand_flash_dev - NAND Flash Device ID Structure
* @name: a human-readable name of the NAND chip
@@ -642,6 +647,12 @@ struct nand_chip {
* @options: stores various chip bit options
* @id_len: The valid length of the @id.
* @oobsize: OOB size
+ * @ecc.strength: The ECC correctability from the datasheet, same as the
+ * @ecc_strength in nand_chip{}.
+ * @ecc.size: The ECC size required by the @ecc.strength, same as the
+ * @ecc_size in nand_chip{}.
+ * For example, the "4bit ECC for each 512Byte" can be set with
+ * NAND_ECC_INFO(4, 512).
*/
struct nand_flash_dev {
char *name;
@@ -658,6 +669,10 @@ struct nand_flash_dev {
unsigned int options;
uint16_t id_len;
uint16_t oobsize;
+ struct {
+ uint16_t strength;
+ uint16_t size;
+ } ecc;
};
/**
--
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 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 a0c486b..9bad296 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
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 9bad296..9b28739 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)
@@ -753,6 +757,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
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 | 77 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 77 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index beff911..abec615 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2846,6 +2846,68 @@ 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) {
+ ret = -ENOMEM;
+ goto ext_out;
+ }
+
+ /* Send our own NAND_CMD_PARAM. */
+ chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
+
+ /* Use the Change Read Column command to skip the ONFI param pages. */
+ chip->cmdfunc(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 +2976,21 @@ 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 nand_flash_detect_ext_param_page() uses the
+ * Change Read Column command which maybe not supported
+ * by the chip->cmdfunc. So try to update the chip->cmdfunc
+ * now. We do not replace user supplied command function.
+ */
+ if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
+ chip->cmdfunc = nand_command_lp;
+
+ /* 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 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 | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 5458021..a0c486b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -435,8 +435,11 @@ struct nand_buffers {
* not bad when badblockbits == 7
* @cellinfo: [INTERN] MLC/multichip data from chip ident
* @ecc_strength: [INTERN] ECC correctability from the datasheet.
+ * The 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. It is the recommended 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
--
1.7.1
> - *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;
Is this really needed ? you have already checked the 'onfi_version' above in
nand_flash_detect_onfi() ..
if (!chip->onfi_version) {
pr_info("%s: unsupported ONFI version: %d\n", __func__, val);
return 0;
}
with regards, pekon
于 2013年04月30日 18:04, Gupta, Pekon 写道:
>> - *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;
> Is this really needed ? you have already checked the 'onfi_version' above in
> nand_flash_detect_onfi() ..
> if (!chip->onfi_version) {
> pr_info("%s: unsupported ONFI version: %d\n", __func__, val);
> return 0;
> }
>
>
I think checking the onfi_version has no relationship with this patch. :)
This patch is just replace the hardcode for 16-bit onfi nand check.
thanks
Huang Shijie
> >> - *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;
> > Is this really needed ? you have already checked the 'onfi_version'
> above in
> > nand_flash_detect_onfi() ..
> > if (!chip->onfi_version) {
> > pr_info("%s: unsupported ONFI version: %d\n",
> __func__, val);
> > return 0;
> > }
> >
> >
> I think checking the onfi_version has no relationship with this patch. :)
> This patch is just replace the hardcode for 16-bit onfi nand check.
>
[Pekon]: [Patch 3/9]: add a helper to get the supported features
I mean, do you really need this helper function ?
+static inline int onfi_feature(struct nand_chip *chip)
+{
+ return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0;
+ }
Following change should have been enough..
*busw = (le16_to_cpu(p->features) & ONFI_FEATURE_16_BIT_BUS) ?
NAND_BUSWIDTH_16 : 0;
with regards, pekon
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
于 2013年05月02日 13:42, Gupta, Pekon 写道:
>>>> - *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;
>>> Is this really needed ? you have already checked the 'onfi_version'
>> above in
>>> nand_flash_detect_onfi() ..
>>> if (!chip->onfi_version) {
>>> pr_info("%s: unsupported ONFI version: %d\n",
>> __func__, val);
>>> return 0;
>>> }
>>>
>>>
>> I think checking the onfi_version has no relationship with this patch. :)
>> This patch is just replace the hardcode for 16-bit onfi nand check.
>>
> [Pekon]: [Patch 3/9]: add a helper to get the supported features
> I mean, do you really need this helper function ?
> +static inline int onfi_feature(struct nand_chip *chip)
> +{
> + return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0;
> + }
>
> Following change should have been enough..
> *busw = (le16_to_cpu(p->features)& ONFI_FEATURE_16_BIT_BUS) ?
> NAND_BUSWIDTH_16 : 0;
yes. for this function, it's enough.
But we may use the onfi_feature() in other places, such as some drivers. That's reason why i added this helper.
thanks
Huang Shijie
> 于 2013年05月02日 13:42, Gupta, Pekon 写道:
> >>>> - *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;
> >>> Is this really needed ? you have already checked the 'onfi_version'
> >> above in
> >>> nand_flash_detect_onfi() ..
> >>> if (!chip->onfi_version) {
> >>> pr_info("%s: unsupported ONFI version: %d\n",
> >> __func__, val);
> >>> return 0;
> >>> }
> >>>
> >>>
> >> I think checking the onfi_version has no relationship with this patch. :)
> >> This patch is just replace the hardcode for 16-bit onfi nand check.
> >>
> > [Pekon]: [Patch 3/9]: add a helper to get the supported features
> > I mean, do you really need this helper function ?
> > +static inline int onfi_feature(struct nand_chip *chip)
> > +{
> > + return chip->onfi_version ? le16_to_cpu(chip-
> >onfi_params.features) : 0;
> > + }
> >
> > Following change should have been enough..
> > *busw = (le16_to_cpu(p->features)& ONFI_FEATURE_16_BIT_BUS) ?
> > NAND_BUSWIDTH_16 : 0;
> yes. for this function, it's enough.
>
> But we may use the onfi_feature() in other places, such as some drivers.
> That's reason why i added this helper.
>
[Pekon]: onfi_feature() is actually not useful unless someone re-scans the
param page again. And once the device is probed following should
be used to check if it’s a x16 device.
If (chip->options & NAND_BUSWIDTH_16)
My view-point was that the code should have less redundancy :-)
Anyways, Thanks for your other patch for reading extended_param_page.
I too wanted that ecc.codeword and ecc.strength should come from
device's parameters. I'll ack them once, I test using my driver.
with regards, pekon
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
于 2013年05月02日 14:17, Gupta, Pekon 写道:
> [Pekon]: onfi_feature() is actually not useful unless someone re-scans the
I do not think so. :)
I think the onfi_feature() is useful in the future.
I only add the two feauture for this helper:
[1] 16-bit and
[2] extended parameter page
But in actually, we may add more feature to this helper, such as
_synchronous_.
For example, some driver may support the synchronous mode for the ONFI nand.
We can use this onfi_feature() in the driver to check if the onfi nand
supports this
synchronous feature.
Add this helper makes the code more readable, though we introduce a
little redandancy.
thanks
Huang Shijie
> 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]>
Acked-by: Pekon Gupta <[email protected]>
Thanks..
with regards, pekon
> 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]>
Acked-by: Pekon Gupta <[email protected]>
Thanks..
with regards, pekon
On Wed, May 1, 2013 at 11:48 PM, Huang Shijie <[email protected]> wrote:
> ?? 2013??05??02?? 14:17, Gupta, Pekon д??:
>
>> [Pekon]: onfi_feature() is actually not useful unless someone re-scans the
>
> I do not think so. :)
>
> I think the onfi_feature() is useful in the future.
> I only add the two feauture for this helper:
> [1] 16-bit and
> [2] extended parameter page
>
> But in actually, we may add more feature to this helper, such as
> _synchronous_.
> For example, some driver may support the synchronous mode for the ONFI nand.
> We can use this onfi_feature() in the driver to check if the onfi nand
> supports this
> synchronous feature.
>
> Add this helper makes the code more readable, though we introduce a little
> redandancy.
I agree with the thoughts here. Readability and reusability are
improved a bit by including the ONFI version check.
(BTW, I haven't forgotten this series. I had some distractions here. I
will fully test and provide my Reviewed-by/Tested-by eventually...)
Brian
Hi Huang,
On Fri, Apr 26, 2013 at 2:08 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 | 77 ++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 77 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index beff911..abec615 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2846,6 +2846,68 @@ 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) {
> + ret = -ENOMEM;
> + goto ext_out;
> + }
> +
> + /* Send our own NAND_CMD_PARAM. */
> + chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> +
> + /* Use the Change Read Column command to skip the ONFI param pages. */
> + chip->cmdfunc(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 +2976,21 @@ 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 nand_flash_detect_ext_param_page() uses the
> + * Change Read Column command which maybe not supported
> + * by the chip->cmdfunc. So try to update the chip->cmdfunc
> + * now. We do not replace user supplied command function.
> + */
> + if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
> + chip->cmdfunc = nand_command_lp;
I don't think we want to assign chip->cmdfunc within the 'detect_onfi'
function. We already have several places from which the default
functions may be assigned. Rather, we should only code the above
statement *once* (where it already is, in nand_get_flash_type()) and
only run the extended parameter page function after that point.
If that's too cumbersome, then maybe it's fine as-is. Or I'll take a
crack at rewriting it myself.
> + /* 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
>
>
Brian
于 2013年05月03日 07:39, Brian Norris 写道:
> only run the extended parameter page function after that point.
sorry, I prefer to run the extended parameter page function here just
following the case
"p-> ecc_bits != 0xff". IMHO, the two cases should be put together as
the onfi spec tells, rather be split out
far away. The extended parameter page function is just the case
"p->ecc_bits == 0xff".
If we put the nand_flash_detect_ext_param_page() after the point we
reassign the chip->cmdfunc,
It's a little discrete in logic which makes the code not compact enough.
btw: Could you check you email client? I failed several times for the
HTML issue.
thanks
Huang Shijie
On Fri, 2013-04-26 at 17:08 +0800, Huang Shijie 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]>
Huang, let me drop the 3 patches I already merged. Please, re-send them
in v5. I think this is better because I see you start applying patches
on top of them, which is a bit confusing.
> * @cellinfo: [INTERN] MLC/multichip data from chip ident
> * @ecc_strength: [INTERN] ECC correctability from the datasheet.
> + * The minimum number of bits correctability, if known;
> + * if unknown, set to 0.
I find this confusing still. How about this comment.
ECC correctability from the datasheet. Minumum amount of bit errors per
@ecc_size guaranteed to be correctable). If unknown, set to zero.
> * @ecc_size: [INTERN] ECC size required by the @ecc_strength,
> - * also from the datasheet.
> + * also from the datasheet. It is the recommended ECC step
> + * size, if known; if unknown, set to 0.
Silly question, why you call this one "ecc_size", and not "ecc_step"?
--
Best Regards,
Artem Bityutskiy
于 2013年05月15日 15:27, Artem Bityutskiy 写道:
> On Fri, 2013-04-26 at 17:08 +0800, Huang Shijie 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]>
> Huang, let me drop the 3 patches I already merged. Please, re-send them
> in v5. I think this is better because I see you start applying patches
> on top of them, which is a bit confusing.
>
Ok, Please drop the 3 patches.
>> * @cellinfo: [INTERN] MLC/multichip data from chip ident
>> * @ecc_strength: [INTERN] ECC correctability from the datasheet.
>> + * The minimum number of bits correctability, if known;
>> + * if unknown, set to 0.
> I find this confusing still. How about this comment.
>
> ECC correctability from the datasheet. Minumum amount of bit errors per
> @ecc_size guaranteed to be correctable). If unknown, set to zero.
>
>
it's okay to me.
>> * @ecc_size: [INTERN] ECC size required by the @ecc_strength,
>> - * also from the datasheet.
>> + * also from the datasheet. It is the recommended ECC step
>> + * size, if known; if unknown, set to 0.
> Silly question, why you call this one "ecc_size", and not "ecc_step"?
>
In nand_ecc_ctrl{}, the ecc step is named to @size.
Personally, i perfer to ecc_step.
thanks
Huang Shijie
On Wed, 2013-05-15 at 15:38 +0800, Huang Shijie wrote:
> 于 2013年05月15日 15:27, Artem Bityutskiy 写道:
> > On Fri, 2013-04-26 at 17:08 +0800, Huang Shijie 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]>
> > Huang, let me drop the 3 patches I already merged. Please, re-send them
> > in v5. I think this is better because I see you start applying patches
> > on top of them, which is a bit confusing.
> >
> Ok, Please drop the 3 patches.
>
> >> * @cellinfo: [INTERN] MLC/multichip data from chip ident
> >> * @ecc_strength: [INTERN] ECC correctability from the datasheet.
> >> + * The minimum number of bits correctability, if known;
> >> + * if unknown, set to 0.
> > I find this confusing still. How about this comment.
> >
> > ECC correctability from the datasheet. Minumum amount of bit errors per
> > @ecc_size guaranteed to be correctable). If unknown, set to zero.
> >
> >
> it's okay to me.
> >> * @ecc_size: [INTERN] ECC size required by the @ecc_strength,
> >> - * also from the datasheet.
> >> + * also from the datasheet. It is the recommended ECC step
> >> + * size, if known; if unknown, set to 0.
> > Silly question, why you call this one "ecc_size", and not "ecc_step"?
> >
> In nand_ecc_ctrl{}, the ecc step is named to @size.
>
> Personally, i perfer to ecc_step.
You could harmonize the naming. Rename all the names to ecc_step, which
is a lot easier to understand.
You did not send v4 thus far, is this because you are busy or you did
not have any requests to address?
Thanks!
--
Best Regards,
Artem Bityutskiy
于 2013年05月15日 15:42, Artem Bityutskiy 写道:
> You did not send v4 thus far, is this because you are busy or you did
> not have any requests to address?
>
I am not busy.
I just thought the v4 is enough.
but now i will send out the v5.
thanks
Huang Shijie
On Wed, 2013-05-15 at 16:02 +0800, Huang Shijie wrote:
> 于 2013年05月15日 15:42, Artem Bityutskiy 写道:
> > You did not send v4 thus far, is this because you are busy or you did
> > not have any requests to address?
> >
> I am not busy.
> I just thought the v4 is enough.
>
> but now i will send out the v5.
To me it looked like Brian is not really satisfied with v4.
--
Best Regards,
Artem Bityutskiy