[PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Huang Shijie]

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 0:
               Tell the reason why add the ecc info to nand_chip{}

   2.2) patch 1 ~ patch 6:
	       Parse out the ecc info for ONFI nand. If we can not get the ecc
	       info directly for the parameter page, we can try to get it from
	       the extended page.

   2.3) patch 7 ~ patch 9:
               Add the ECC info for full-id nand.

   2.4) patch 10:
               The gpmi uses the ecc info to set the BCH module. and with this
	       patch set, the gpmi can supports the MT29F64G08CBABA now.


Huang Shijie (11):
  mtd: add datasheet's ECC information to nand_chip{}
  mtd: increase max OOB size to 744
  mtd: get the ECC info from the parameter page for ONFI nand
  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_get_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           |   67 ++++++++++++++++-
 drivers/mtd/nand/nand_ids.c            |    8 +-
 include/linux/mtd/nand.h               |   69 +++++++++++++++++-
 4 files changed, 262 insertions(+), 10 deletions(-)

[PATCH 01/11] mtd: add datasheet's ECC information to nand_chip{}

[Huang Shijie]

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 new fields.
   The @ecc_strength stands for the ecc bits needed within the @ecc_size.
   Both of the new fields should be set which conform to the datasheet.

   For example:
	"4bit ECC for each 512Byte" could be:
		@ecc_strength = 4, @ecc_size = 512.
	"40bit ECC for each 1024Byte" could be:
		@ecc_strength = 40, @ecc_size = 1024.

3.) Why do not re-use the @strength and @size in the nand_ecc_ctrl{}?
   The @strength and @size in nand_ecc_ctrl{} is used by the nand controller
   driver, while the @ecc_strength and @ecc_size are get from the datasheet.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/linux/mtd/nand.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 4b87815..65b0e8b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -425,6 +425,9 @@ struct nand_buffers {
  *			bad block marker position; i.e., BBM == 11110111b is
  *			not bad when badblockbits == 7
  * @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.
  * @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
@@ -502,6 +505,8 @@ struct nand_chip {
 	unsigned int pagebuf_bitflips;
 	int subpagesize;
 	uint8_t cellinfo;
+	uint16_t ecc_strength;
+	uint16_t ecc_size;
 	int badblockpos;
 	int badblockbits;
 
-- 
1.7.1

[PATCH 02/11] mtd: increase max OOB size to 744

[Huang Shijie]

The oob size of Micron's MT29F64G08CBABAWP is 744 bytes.
So increase the NAND_MAX_OOBSIZE to 744.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/linux/mtd/nand.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 65b0e8b..9779e3e 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -56,7 +56,7 @@ extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
  * is supported now. If you add a chip with bigger oobsize/page
  * adjust this accordingly.
  */
-#define NAND_MAX_OOBSIZE	640
+#define NAND_MAX_OOBSIZE	744
 #define NAND_MAX_PAGESIZE	8192
 
 /*
-- 
1.7.1

[PATCH 03/11] mtd: get the ECC info from the parameter page for ONFI nand

[Huang Shijie]

>From the onfi spec, we can just get the ECC info from the @ecc_bits field of
the parameter page.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 49de9da..461fbc8 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2878,6 +2878,11 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	if (le16_to_cpu(p->features) & 1)
 		*busw = NAND_BUSWIDTH_16;
 
+	if (p->ecc_bits != 0xff) {
+		chip->ecc_strength = p->ecc_bits;
+		chip->ecc_size = 512;
+	}
+
 	pr_info("ONFI flash detected\n");
 	return 1;
 }
-- 
1.7.1

[PATCH 04/11] mtd: add data structures for Extended Parameter Page

[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 ext_ecc_info{} for Extended ECC information,
    [3] adds ext_section{} for extended sections,
    [4] and adds ext_param_page{} for the Extended Parameter Page.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 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 9779e3e..94ae957 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -217,7 +217,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];
@@ -274,6 +277,40 @@ struct nand_onfi_params {
 
 #define ONFI_CRC_BASE	0x4F4E
 
+/* Extended ECC information Block Definition (since onfi 2.1) */
+struct ext_ecc_info {
+	u8 ecc_bits;
+	u8 codeword_size;
+	__le16 bb_per_lun;
+	__le16 block_endurance;
+	u8 reserved[2];
+} __attribute__((packed));
+
+#define SECTION_TYPE_0	0	/* Unused section. */
+#define SECTION_TYPE_1	1	/* for additional sections. */
+#define SECTION_TYPE_2	2	/* for ECC information. */
+struct ext_section {
+	u8 type;
+	u8 length;
+} __attribute__((packed));
+
+#define EXT_SECTION_MAX 8
+
+/* Extended Parameter Page Definition (since onfi 2.1) */
+struct ext_param_page {
+	__le16 crc;
+	u8 sig[4];             /* 'E' 'P' 'P' 'S' */
+	u8 reserved0[10];
+	struct ext_section sections[EXT_SECTION_MAX];
+
+	/*
+	 * The actual size of the Extended Parameter Page is in
+	 * @ext_param_page_length of nand_onfi_params{}.
+	 * The followings are the unknown 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

[PATCH 05/11] mtd: add a helper to get the supported features for ONFI nand

[Huang Shijie]

From: Huang Shijie <shijie8@gmail.com>

add a helper to get the supported features for ONFI nand.
Also add the neccessary macros.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/linux/mtd/nand.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 94ae957..2c5a2e0 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -195,6 +195,11 @@ 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)
+#define ONFI_FEATURE_UNKNOWN		(1 << 15)
+
 /* ONFI timing mode, used in both asynchronous and synchronous mode */
 #define ONFI_TIMING_MODE_0		(1 << 0)
 #define ONFI_TIMING_MODE_1		(1 << 1)
@@ -743,6 +748,14 @@ struct platform_nand_chip *get_platform_nandchip(struct mtd_info *mtd)
 	return chip->priv;
 }
 
+/* return the supported features. */
+static inline int onfi_get_feature(struct nand_chip *chip)
+{
+	if (!chip->onfi_version)
+		return ONFI_FEATURE_UNKNOWN;
+	return le16_to_cpu(chip->onfi_params.features);
+}
+
 /* return the supported asynchronous timing mode. */
 static inline int onfi_get_async_timing_mode(struct nand_chip *chip)
 {
-- 
1.7.1

[PATCH 06/11] mtd: get the ECC info from the Extended Parameter Page

[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 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 <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |   54 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 461fbc8..f1cc1b9 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2813,6 +2813,56 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
 	return crc;
 }
 
+/* Parse the Extended Parameter Page. */
+static void nand_flash_detect_ext_param_page(struct mtd_info *mtd,
+		struct nand_chip *chip, struct nand_onfi_params *p, int last)
+{
+	struct ext_param_page *ep;
+	struct ext_section *s;
+	struct ext_ecc_info *ecc;
+	uint8_t *cursor;
+	int len;
+	int i;
+
+	len = le16_to_cpu(p->ext_param_page_length) * 16;
+	ep = kcalloc(1, max_t(int, len, sizeof(*p)), GFP_KERNEL);
+	if (!ep)
+		goto ext_out;
+
+	/*
+	 * Skip the ONFI Parameter Pages.
+	 * The Change Read Columm command may does not works here.
+	 */
+	for (i = last + 1; i < p->num_of_param_pages; i++)
+		chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p));
+
+	/* 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))
+		goto ext_out;
+
+	/* find the ECC section. */
+	cursor = (uint8_t *)(ep + 1);
+	for (i = 0; i < EXT_SECTION_MAX; i++) {
+		s = ep->sections + i;
+		if (s->type == SECTION_TYPE_2)
+			break;
+		cursor += s->length * 16;
+	}
+	if (i == EXT_SECTION_MAX)
+		goto ext_out;
+
+	/* get the info we want. */
+	ecc = (struct 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");
+ext_out:
+	kfree(ep);
+}
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -2881,6 +2931,10 @@ 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_get_feature(chip) & ONFI_FEATURE_EXT_PARAM_PAGE)) {
+		/* The Extended Parameter Page is supported since ONFI 2.1. */
+		nand_flash_detect_ext_param_page(mtd, chip, p, i);
 	}
 
 	pr_info("ONFI flash detected\n");
-- 
1.7.1

[PATCH 07/11] mtd: replace the hardcode with the onfi_get_feature()

[Huang Shijie]

The current code uses the hardcode to detect the 16-bit bus width.
Use the onfi_get_feature() to replace it.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 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 f1cc1b9..1b22ddc 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2924,9 +2924,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_get_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

[PATCH 08/11] mtd: add a new field for ecc info in the nand_flash_dev{}

[Huang Shijie]

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 <b32955@freescale.com>
---
 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 2c5a2e0..2837bc6 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -633,6 +633,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
+ *            ECC_INFO(4, 512) macro.
  */
 struct nand_flash_dev {
 	char *name;
@@ -649,6 +654,11 @@ struct nand_flash_dev {
 	unsigned int options;
 	uint16_t id_len;
 	uint16_t oobsize;
+
+#define ECC_INFO(strength, size)	(((strength) << 16) | (size))
+#define ECC_STRENGTH(x)			(((x) >> 16) & 0xffff)
+#define ECC_SIZE(x)			((x) & 0xffff)
+	uint32_t ecc_info;
 };
 
 /**
-- 
1.7.1

[PATCH 09/11] mtd: parse out the ECC info for the full-id nand chips

[Huang Shijie]

Parse out the ECC information for the full-id nand chips.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 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 1b22ddc..61a732a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3198,6 +3198,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 = ECC_STRENGTH(type->ecc_info);
+		chip->ecc_size = ECC_SIZE(type->ecc_info);
 
 		*busw = type->options & NAND_BUSWIDTH_16;
 
-- 
1.7.1

[PATCH 10/11] mtd: add the ecc info for some full-id nand chips

[Huang Shijie]

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 <b32955@freescale.com>
---
 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 1c242cc..6220ec3 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -30,16 +30,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, 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, 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, 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, ECC_INFO(40, SZ_1K)},
 
 	LEGACY_ID_NAND("NAND 4MiB 5V 8-bit",   0x6B, 512, 4, 0x2000, 0),
 	LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 512, 4, 0x2000, 0),
-- 
1.7.1

[PATCH 11/11] mtd: gpmi: set the BCH's geometry with the ecc info

[Huang Shijie]

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 <b32955@freescale.com>
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 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

Re: [PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Artem Bityutskiy]

On Mon, 2013-03-18 at 19:18 +0800, Huang Shijie wrote:
> 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.

Unfortunately no-one reviewed your patches. Let me CC Brian.

I've pushed patches 1-3 to l2-mtd.git so far.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 08/11] mtd: add a new field for ecc info in the nand_flash_dev{}

[Artem Bityutskiy]

On Mon, 2013-03-18 at 19:18 +0800, Huang Shijie wrote:
> +#define ECC_INFO(strength, size)	(((strength) << 16) | (size))
> +#define ECC_STRENGTH(x)			(((x) >> 16) & 0xffff)
> +#define ECC_SIZE(x)			((x) & 0xffff)
> +	uint32_t ecc_info;
>  };

Could you please avoid putting macro definition in the middle of the
structure. Put it before the structure.

Also, probably prefixing with NAND_ would make sense?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Huang Shijie]

于 2013年04月16日 21:55, Artem Bityutskiy 写道:
> Unfortunately no-one reviewed your patches. Let me CC Brian.
>
I CCed Brian already.
I also hope some one could reviews this patch set.

thanks
Huang Shijie

Re: [PATCH 08/11] mtd: add a new field for ecc info in the nand_flash_dev{}

[Huang Shijie]

于 2013年04月16日 22:56, Artem Bityutskiy 写道:
> On Mon, 2013-03-18 at 19:18 +0800, Huang Shijie wrote:
>> +#define ECC_INFO(strength, size)	(((strength)<<  16) | (size))
>> +#define ECC_STRENGTH(x)			(((x)>>  16)&  0xffff)
>> +#define ECC_SIZE(x)			((x)&  0xffff)
>> +	uint32_t ecc_info;
>>   };
> Could you please avoid putting macro definition in the middle of the
> structure. Put it before the structure.
>
> Also, probably prefixing with NAND_ would make sense?
got it.

thanks
Huang Shijie

Re: [PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Brian Norris]

On Tue, Apr 16, 2013 at 7:10 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年04月16日 21:55, Artem Bityutskiy 写道:
>
>> Unfortunately no-one reviewed your patches. Let me CC Brian.
>>
> I CCed Brian already.
> I also hope some one could reviews this patch set.

Things go by the wayside at times... sigh. Sorry for the delay. I do
plan to fully review this series.

Initial comments:

What are the semantics for these new ecc_strength and ecc_size fields
for chips which aren't encoded in nand_ids.c (or aren't
ONFI-compliant)? We can't necessarily say that '0' means "no data
provided" - it is perfectly valid to have a value of 0, e.g., for EZ
NAND (or something like that) which manages its own ECC on-die. I
think there are even a few words in the ONFI spec about it. Basically,
I'm trying to point out that there is a difference between those chips
which we can't/don't detect their ecc_{strength,size} and those which
need no ECC.

Also, I might consider different names for these fields so that you
don't just rely on the in-code comments to differentiate the nand_chip
and nand_ecc_ctrl values. Perhaps min_ecc_strength and ecc_size?
(can't think of a great distinguishing name for the second one)

Finally, it's probably fine to increase NAND_MAX_OOBSIZE (and
MTD_MAX_ECCPOS_ENTRIES_LARGE) yet again, but these really need to
die... I have a (yet untested) patch series for killing
NAND_MAX_OOBSIZE ready.

I'll try to fully review the series within the week.

Brian

Re: [PATCH 10/11] mtd: add the ecc info for some full-id nand chips

[Brian Norris]

On Mon, Mar 18, 2013 at 4:18 AM, Huang Shijie <b32955@freescale.com> wrote:
> 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 <b32955@freescale.com>

This patch needs rebased to the current l2-mtd.git.

>  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 1c242cc..6220ec3 100644
> --- a/drivers/mtd/nand/nand_ids.c
> +++ b/drivers/mtd/nand/nand_ids.c
> @@ -30,16 +30,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, 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, 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, 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, ECC_INFO(40, SZ_1K)},
>
>         LEGACY_ID_NAND("NAND 4MiB 5V 8-bit",   0x6B, 512, 4, 0x2000, 0),
>         LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 512, 4, 0x2000, 0),
> --
> 1.7.1
>
>

Re: [PATCH 04/11] mtd: add data structures for Extended Parameter Page

[Brian Norris]

On Mon, Mar 18, 2013 at 4:18 AM, Huang Shijie <b32955@freescale.com> 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 adds
>     [1] the neccessary fields for nand_onfi_params{},
>     [2] and adds the ext_ecc_info{} for Extended ECC information,
>     [3] adds ext_section{} for extended sections,
>     [4] and adds ext_param_page{} for the Extended Parameter Page.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  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 9779e3e..94ae957 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -217,7 +217,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];
> @@ -274,6 +277,40 @@ struct nand_onfi_params {
>
>  #define ONFI_CRC_BASE  0x4F4E
>
> +/* Extended ECC information Block Definition (since onfi 2.1) */
> +struct ext_ecc_info {

Should include NAND and/or ONFI in the name (e.g., onfi_ext_ecc_info)

> +       u8 ecc_bits;
> +       u8 codeword_size;
> +       __le16 bb_per_lun;
> +       __le16 block_endurance;
> +       u8 reserved[2];
> +} __attribute__((packed));
> +
> +#define SECTION_TYPE_0 0       /* Unused section. */
> +#define SECTION_TYPE_1 1       /* for additional sections. */
> +#define SECTION_TYPE_2 2       /* for ECC information. */

Should prefix these with ONFI_

> +struct ext_section {

Also needs an ONFI prefix, e.g., onfi_ext_section

> +       u8 type;
> +       u8 length;
> +} __attribute__((packed));
> +
> +#define EXT_SECTION_MAX 8

Also should be prefixed ONFI_

> +
> +/* Extended Parameter Page Definition (since onfi 2.1) */
> +struct ext_param_page {

onfi_ext_param_page

> +       __le16 crc;
> +       u8 sig[4];             /* 'E' 'P' 'P' 'S' */
> +       u8 reserved0[10];
> +       struct ext_section sections[EXT_SECTION_MAX];
> +
> +       /*
> +        * The actual size of the Extended Parameter Page is in
> +        * @ext_param_page_length of nand_onfi_params{}.
> +        * The followings are the unknown length sections.

s/followings/following/
s/unknown /variable-/

> +        * So we do not add any fields below. Please see the onfi spec.

s/onfi/ONFI/

> +        */
> +} __attribute__((packed));
> +
>  /**
>   * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
>   * @lock:               protection lock

Brian

Re: [PATCH 05/11] mtd: add a helper to get the supported features for ONFI nand

[Brian Norris]

On Mon, Mar 18, 2013 at 4:18 AM, Huang Shijie <b32955@freescale.com> wrote:
> From: Huang Shijie <shijie8@gmail.com>
>
> add a helper to get the supported features for ONFI nand.
> Also add the neccessary macros.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  include/linux/mtd/nand.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 94ae957..2c5a2e0 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -195,6 +195,11 @@ 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)
> +#define ONFI_FEATURE_UNKNOWN           (1 << 15)
> +
>  /* ONFI timing mode, used in both asynchronous and synchronous mode */
>  #define ONFI_TIMING_MODE_0             (1 << 0)
>  #define ONFI_TIMING_MODE_1             (1 << 1)
> @@ -743,6 +748,14 @@ struct platform_nand_chip *get_platform_nandchip(struct mtd_info *mtd)
>         return chip->priv;
>  }
>
> +/* return the supported features. */
> +static inline int onfi_get_feature(struct nand_chip *chip)

This function naming seems too close to the chip->onfi_get_features
function pointer. They serve totally different purposes (I think) and
should clearly be shown to be different in some way--preferably by
naming.

> +{
> +       if (!chip->onfi_version)
> +               return ONFI_FEATURE_UNKNOWN;
> +       return le16_to_cpu(chip->onfi_params.features);
> +}
> +
>  /* return the supported asynchronous timing mode. */
>  static inline int onfi_get_async_timing_mode(struct nand_chip *chip)
>  {

Brian

Re: [PATCH 04/11] mtd: add data structures for Extended Parameter Page

[Huang Shijie]

于 2013年04月22日 12:24, Brian Norris 写道:
> On Mon, Mar 18, 2013 at 4:18 AM, Huang Shijie<b32955@freescale.com>  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 adds
>>      [1] the neccessary fields for nand_onfi_params{},
>>      [2] and adds the ext_ecc_info{} for Extended ECC information,
>>      [3] adds ext_section{} for extended sections,
>>      [4] and adds ext_param_page{} for the Extended Parameter Page.
>>
>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>> ---
>>   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 9779e3e..94ae957 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -217,7 +217,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];
>> @@ -274,6 +277,40 @@ struct nand_onfi_params {
>>
>>   #define ONFI_CRC_BASE  0x4F4E
>>
>> +/* Extended ECC information Block Definition (since onfi 2.1) */
>> +struct ext_ecc_info {
> Should include NAND and/or ONFI in the name (e.g., onfi_ext_ecc_info)
>
>> +       u8 ecc_bits;
>> +       u8 codeword_size;
>> +       __le16 bb_per_lun;
>> +       __le16 block_endurance;
>> +       u8 reserved[2];
>> +} __attribute__((packed));
>> +
>> +#define SECTION_TYPE_0 0       /* Unused section. */
>> +#define SECTION_TYPE_1 1       /* for additional sections. */
>> +#define SECTION_TYPE_2 2       /* for ECC information. */
> Should prefix these with ONFI_
>
>> +struct ext_section {
> Also needs an ONFI prefix, e.g., onfi_ext_section
>
>> +       u8 type;
>> +       u8 length;
>> +} __attribute__((packed));
>> +
>> +#define EXT_SECTION_MAX 8
> Also should be prefixed ONFI_
>
>> +
>> +/* Extended Parameter Page Definition (since onfi 2.1) */
>> +struct ext_param_page {
> onfi_ext_param_page
>
>> +       __le16 crc;
>> +       u8 sig[4];             /* 'E' 'P' 'P' 'S' */
>> +       u8 reserved0[10];
>> +       struct ext_section sections[EXT_SECTION_MAX];
>> +
>> +       /*
>> +        * The actual size of the Extended Parameter Page is in
>> +        * @ext_param_page_length of nand_onfi_params{}.
>> +        * The followings are the unknown length sections.
> s/followings/following/
> s/unknown /variable-/
>
>> +        * So we do not add any fields below. Please see the onfi spec.
> s/onfi/ONFI/
>
>> +        */
>> +} __attribute__((packed));
>> +
>>   /**
>>    * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
>>    * @lock:               protection lock
>
thanks Brian.

I will fix it in next version.

Huang Shijie

Re: [PATCH 05/11] mtd: add a helper to get the supported features for ONFI nand

[Huang Shijie]

于 2013年04月22日 12:29, Brian Norris 写道:
>> +/* return the supported features. */
>> >  +static inline int onfi_get_feature(struct nand_chip *chip)
> This function naming seems too close to the chip->onfi_get_features
> function pointer. They serve totally different purposes (I think) and
> should clearly be shown to be different in some way--preferably by
> naming.
>
ok.

what's about onfi_feature()?

Remove the "get" from the function name.

thanks
Huang Shijie

Re: [PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Brian Norris]

Hi again Huang,

I had some initial comments a week ago (below) which haven't been
addressed. Can you please address them? I see that you are continuing
to send out v2 patches based on l2-mtd, where Artem already pulled in
your first 3 patches. But further work depends on the answer to my
questions for the first 3 patches.

On Tue, Apr 16, 2013 at 8:56 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Tue, Apr 16, 2013 at 7:10 PM, Huang Shijie <b32955@freescale.com> wrote:
>> 于 2013年04月16日 21:55, Artem Bityutskiy 写道:
>>
>>> Unfortunately no-one reviewed your patches. Let me CC Brian.
>>>
>> I CCed Brian already.
>> I also hope some one could reviews this patch set.
>
> Things go by the wayside at times... sigh. Sorry for the delay. I do
> plan to fully review this series.
>
> Initial comments:
>
> What are the semantics for these new ecc_strength and ecc_size fields
> for chips which aren't encoded in nand_ids.c (or aren't
> ONFI-compliant)? We can't necessarily say that '0' means "no data
> provided" - it is perfectly valid to have a value of 0, e.g., for EZ
> NAND (or something like that) which manages its own ECC on-die. I
> think there are even a few words in the ONFI spec about it. Basically,
> I'm trying to point out that there is a difference between those chips
> which we can't/don't detect their ecc_{strength,size} and those which
> need no ECC.

We need to straighten out these semantics now. Perhaps the fields in
struct nand_chip can be signed, where -1 is "unknown" and 0 is "no ECC
required."

> Also, I might consider different names for these fields so that you
> don't just rely on the in-code comments to differentiate the nand_chip
> and nand_ecc_ctrl values. Perhaps min_ecc_strength and ecc_size?
> (can't think of a great distinguishing name for the second one)

The naming can be changed after your series, if it seems worth it.

> Finally, it's probably fine to increase NAND_MAX_OOBSIZE (and
> MTD_MAX_ECCPOS_ENTRIES_LARGE) yet again, but these really need to
> die... I have a (yet untested) patch series for killing
> NAND_MAX_OOBSIZE ready.
>
> I'll try to fully review the series within the week.
>
> Brian

Re: [PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Huang Shijie]

于 2013年04月23日 05:35, Brian Norris 写道:
>> > Initial comments:
>> >
>> > What are the semantics for these new ecc_strength and ecc_size fields
>> > for chips which aren't encoded in nand_ids.c (or aren't
>> > ONFI-compliant)? We can't necessarily say that '0' means "no data
>> > provided" - it is perfectly valid to have a value of 0, e.g., for EZ
>> > NAND (or something like that) which manages its own ECC on-die. I
>> > think there are even a few words in the ONFI spec about it. Basically,
>> > I'm trying to point out that there is a difference between those chips
>> > which we can't/don't detect their ecc_{strength,size} and those which
>> > need no ECC.
> We need to straighten out these semantics now. Perhaps the fields in
> struct nand_chip can be signed, where -1 is "unknown" and 0 is "no ECC
> required."
>
IMHO, use -1 as "unknown" makes the things a little complexed.
You have to add a patch to fix the nand_ids or init these two fields in
nand_base.c.

For the ECC on-die case(sorry, i never meet this type of nand), I prefer
to add a
new flags to distinguish them.

For the "unknown" case, use the default 0.


>> > Also, I might consider different names for these fields so that you
>> > don't just rely on the in-code comments to differentiate the nand_chip
>> > and nand_ecc_ctrl values. Perhaps min_ecc_strength and ecc_size?
>> > (can't think of a great distinguishing name for the second one)
> The naming can be changed after your series, if it seems worth it.
If we use the min_ecc_strength, we'd better use the min_ecc_size too.


thanks
Huang Shijie

Re: [PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Brian Norris]

On Mon, Apr 22, 2013 at 7:34 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年04月23日 05:35, Brian Norris 写道:
>>> > Initial comments:
>>> >
>>> > What are the semantics for these new ecc_strength and ecc_size fields
>>> > for chips which aren't encoded in nand_ids.c (or aren't
>>> > ONFI-compliant)? We can't necessarily say that '0' means "no data
>>> > provided" - it is perfectly valid to have a value of 0, e.g., for EZ
>>> > NAND (or something like that) which manages its own ECC on-die. I
>>> > think there are even a few words in the ONFI spec about it. Basically,
>>> > I'm trying to point out that there is a difference between those chips
>>> > which we can't/don't detect their ecc_{strength,size} and those which
>>> > need no ECC.
>> We need to straighten out these semantics now. Perhaps the fields in
>> struct nand_chip can be signed, where -1 is "unknown" and 0 is "no ECC
>> required."
>>
> IMHO, use -1 as "unknown" makes the things a little complexed.
> You have to add a patch to fix the nand_ids or init these two fields in
> nand_base.c.

Neither of these options is very difficult, given that we added nice
macros for nand_ids.c. And with a proper return code for the ONFI
functions, we can easily distinguish "uninitialized" (failure) from
initialized-to-zero.

> For the ECC on-die case(sorry, i never meet this type of nand),

Yeah, I haven't seen them myself, but I've heard reports from others
trying to use them. And I believe a driver was submitted recently for
BENAND. I don't recall its fate, though. And such a driver wouldn't be
relying on this information; I'm guessing that it would assume the
flash doesn't need ECC.

> I prefer
> to add a
> new flags to distinguish them.

I'm not sure that would be a better way, if we can just as easily
support signed-int fields. But we should mention the semantics we're
defining, since drivers other than gpmi may (will) want to use this.

> For the "unknown" case, use the default 0.

I'm fine with your suggestion, at least for now. If the code is done
right, it shouldn't be too hard to support either the flag or "-1 as
uninitialized" in the future.

>>> > Also, I might consider different names for these fields so that you
>>> > don't just rely on the in-code comments to differentiate the nand_chip
>>> > and nand_ecc_ctrl values. Perhaps min_ecc_strength and ecc_size?
>>> > (can't think of a great distinguishing name for the second one)
>> The naming can be changed after your series, if it seems worth it.
> If we use the min_ecc_strength, we'd better use the min_ecc_size too.

But min_ecc_size is somewhat inaccurate. For example, for a flash
which requires 4-bit correction per 512-bytes, it is not acceptable to
use a larger ECC size (e.g., 4-bit correction per 1024-bytes). It is,
however, acceptable to *decrease* the ECC size (e.g., 4-bit correction
per 256-bytes, if such ECC exists), so it's more like a "max ECC
size." Really, though, it's neither a max nor a min (e.g., 8-bit per
1024-bytes also is acceptable); it just happens to pair with
min_ecc_strength.

So like I said, rather than hold up on this name, we could leave it
as-is. Or change it to min_ecc_strength/ecc_size. Or, if you still
think it's good enough, min_ecc_strength/min_ecc_size. Whatever you
choose. (With comments.)

Brian

Re: [PATCH 00/11] mtd: add datasheet's ECC information to nand_chip{}

[Huang Shijie]

于 2013年04月23日 15:26, Brian Norris 写道:
> I'm not sure that would be a better way, if we can just as easily
> support signed-int fields. But we should mention the semantics we're
> defining, since drivers other than gpmi may (will) want to use this.
I failed to send my email for the HTML issue. So i make my reply to short.

I will send an extra patch to add more comment about the two fields.
Use the comment as the init semantics.

And I prefer to keep the name as it is now. :)


thanks
Huang Shijie