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

[PATCH v6 00/10] 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 1:
   	      The keynote patch.

   2.2) patch 2 ~ patch 6:
               These patches are for ONFI nand.
               Parse out the ecc info from the parameter page if we can, else
	       parse out the ecc info from the extended parameter page.
	       
   2.2) patch 7 ~ patch 9:
               Add the ECC info for full-id nand, and parse it out.

   2.3) 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.

v5 --> v6:
	[0] rename the ecc_strength/ecc_step to ecc_strength_ds/ecc_step_ds.
	[1] fix a case when "the codeword is 0".
	[2] fix typo.
	[3] add the 'O' to the diagram in the gpmi patch.

v4 --> v5:
	[0] rebase the new l2-mtd(dropped the 3 merged patches).
	[1] rename ecc_size to ecc_step.
	[2] change the comment for the new fields.

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.

Huang Shijie (10):
  mtd: add datasheet's ECC information to nand_chip{}
  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_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 |  132 +++++++++++++++++++++++++++++++-
 drivers/mtd/nand/nand_base.c           |   93 ++++++++++++++++++++++-
 drivers/mtd/nand/nand_ids.c            |    8 +-
 include/linux/mtd/nand.h               |   72 +++++++++++++++++-
 4 files changed, 296 insertions(+), 9 deletions(-)

[PATCH v6 01/10] 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_ds stands for the ecc bits needed within the @ecc_step_ds.
   The two fields should be set from the nand chip's datasheets.

   For example:
	"4bit ECC for each 512Byte" could be:
		@ecc_strength_ds = 4, @ecc_step_ds = 512.
	"40bit ECC for each 1024Byte" could be:
		@ecc_strength_ds = 40, @ecc_step_ds = 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_ds and @ecc_step_ds are get from the datasheet.

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

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 38535eb..ee696f1 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -434,6 +434,12 @@ 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_ds:	[INTERN] ECC correctability from the datasheet.
+ *			Minimum amount of bit errors per @ecc_step_ds guaranteed
+ *			to be correctable. If unknown, set to zero.
+ * @ecc_step_ds:	[INTERN] ECC step required by the @ecc_strength_ds,
+ *                      also from the datasheet. It is the recommended ECC step
+ *			size, if known; if unknown, set to zero.
  * @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
@@ -510,6 +516,8 @@ struct nand_chip {
 	unsigned int pagebuf_bitflips;
 	int subpagesize;
 	uint8_t cellinfo;
+	uint16_t ecc_strength_ds;
+	uint16_t ecc_step_ds;
 	int badblockpos;
 	int badblockbits;
 
-- 
1.7.1

[PATCH v6 02/10] 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 edc7663..b63b731 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2900,6 +2900,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_ds = p->ecc_bits;
+		chip->ecc_step_ds = 512;
+	}
+
 	pr_info("ONFI flash detected\n");
 	return 1;
 }
-- 
1.7.1

[PATCH v6 03/10] 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 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.

Acked-by: Pekon Gupta <pekon@ti.com>
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 ee696f1..f1ce48e 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

[PATCH v6 04/10] 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 |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index f1ce48e..9574e0a 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

[PATCH v6 05/10] 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.

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

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b63b731..0b1162a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2835,6 +2835,71 @@ 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;
+
+	if (ecc->codeword_size) {
+		chip->ecc_strength_ds = ecc->ecc_bits;
+		chip->ecc_step_ds = 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.
  */
@@ -2903,6 +2968,21 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	if (p->ecc_bits != 0xff) {
 		chip->ecc_strength_ds = p->ecc_bits;
 		chip->ecc_step_ds = 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

Re: [PATCH v6 05/10] mtd: get the ECC info from the Extended Parameter Page

[Vikram Narayanan]

Hello Huang,

On 5/17/2013 8:47 AM, Huang Shijie wrote:
> Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page
> to store the ECC info.
>
<snip>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index b63b731..0b1162a 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2835,6 +2835,71 @@ 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)
> +{
<snip>
> +
> +	/* 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)) {

 From section 5.7.2.2.
///Byte 2-5: Extended parameter page signature
This field contains the extended parameter page signature. When two or 
more bytes of the signature are valid, then it denotes that a valid copy 
of the extended parameter page is present///

But here you are doing a strict check. What if the first two bytes are 
valid? Or did I misunderstood something?
If not, I'd prefer to take the strncmp to a separate 'if' so, that you 
can do the comparison in the way specified in the ONFI spec.

> +		pr_debug("fail in the CRC.\n");

Also, this is the error for Signature failure as well. Please move the 
signature comparison to a new if to give better error messages.

> +		ret = -EINVAL;
> +		goto ext_out;
> +	}

Regards,
Vikram

[PATCH v6 05/10 fix] 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.

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

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b63b731..3edf825 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2835,6 +2835,79 @@ 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 ret = -EINVAL;
+	int len;
+	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))) {
+		pr_debug("fail in the CRC.\n");
+		goto ext_out;
+	}
+
+	/*
+	 * From section 5.7.2.2, we know that the Extened Param Page is valid
+	 * when two or more bytes of the signatrue are valid.
+	 * So we only check the first two bytes here.
+	 */
+	if (strncmp(ep->sig, "EP", 2)) {
+		pr_debug("The signatrue is invalid.\n");
+		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");
+		goto ext_out;
+	}
+
+	/* get the info we want. */
+	ecc = (struct onfi_ext_ecc_info *)cursor;
+
+	if (ecc->codeword_size) {
+		chip->ecc_strength_ds = ecc->ecc_bits;
+		chip->ecc_step_ds = 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.
  */
@@ -2903,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_ds = p->ecc_bits;
 		chip->ecc_step_ds = 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

Re: [PATCH v6 05/10 fix] mtd: get the ECC info from the Extended Parameter Page

[Brian Norris]

Hi Huang, Vikram,

On 05/19/2013 07:08 PM, Huang Shijie wrote:
...
> +	/*
> +	 * From section 5.7.2.2, we know that the Extened Param Page is valid
> +	 * when two or more bytes of the signatrue are valid.

s/signatrue/signature

> +	 * So we only check the first two bytes here.
> +	 */
> +	if (strncmp(ep->sig, "EP", 2)) {
> +		pr_debug("The signatrue is invalid.\n");

Ditto.

> +		goto ext_out;
> +	}

What's the reasoning about this whole "only check 2 bytes" thing? I 
understand that this is technically what spec *says* (although you're 
actually not checking the other 5 combinations that are valid: 'ExPx' 
'ExxS' 'xPPx' 'xPxS' 'xxPS'). But *why* does the spec say this? To 
tolerate errors or to tolerate changes in the spec (e.g., new types of 
parameter pages that say 'QPPS' [1], for instance)? The former doesn't 
really sound plausible, since if we're going to have 2 whole bytes of 
errors in the signature, then we really shouldn't trust the whole 
(extended) parameter page. And the latter doesn't really make sense to 
me; any future backwards-compatible modifications should just use the 
same signature string.

Anyway, my point is that there has to be some logic to strictly 
following the letter of the specification. Shortening the check to just 
the 2-byte "EP" string does not actually cover *exactly* what the spec 
might allow (e.g., it doesn't allow "QPPS" [1]). But it also doesn't 
make any sense why we want to check anything besides "EPPS". So my 
natural inclination is to be strict in what we accept (i.e., exactly the 
"EPPS" string) until we find a reason otherwise.

Or, if you're gonna pull the strict compliance card, check all 6 
combinations, not just 1 of them.

Brian

[1] This is a totally made-up example. I do not understand at all why 
ONFI would allow anything besides exactly "EPPS".

Re: [PATCH v6 05/10 fix] mtd: get the ECC info from the Extended Parameter Page

[Huang Shijie]

于 2013年05月22日 00:35, Vikram Narayanan 写道:
> What if we do like this? Let's do a strict check for now. ("EPPS")
> If some chip manufacturer had _strictly_ followed the spec, i.e., any
> two bytes are valid.
> Then make the code comply with the spec.
> Any comments?
Agreed.

I will send a new patch for this.

thanks for the comment.

Huang Shijie

[PATCH v6 05/10 fix2] 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.

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

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b63b731..fa6ea8e 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2835,6 +2835,78 @@ 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 ret = -EINVAL;
+	int len;
+	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))) {
+		pr_debug("fail in the CRC.\n");
+		goto ext_out;
+	}
+
+	/*
+	 * Check the signature.
+	 * Do not strictly follow the ONFI spec, maybe changed in future.
+	 */
+	if (strncmp(ep->sig, "EPPS", 4)) {
+		pr_debug("The signature is invalid.\n");
+		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");
+		goto ext_out;
+	}
+
+	/* get the info we want. */
+	ecc = (struct onfi_ext_ecc_info *)cursor;
+
+	if (ecc->codeword_size) {
+		chip->ecc_strength_ds = ecc->ecc_bits;
+		chip->ecc_step_ds = 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.
  */
@@ -2903,6 +2975,21 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	if (p->ecc_bits != 0xff) {
 		chip->ecc_strength_ds = p->ecc_bits;
 		chip->ecc_step_ds = 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

[PATCH v6 06/10] mtd: replace the hardcode with the onfi_feature()

[Huang Shijie]

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 <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 0b1162a..355976d 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2961,9 +2961,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_ds = p->ecc_bits;
-- 
1.7.1

[PATCH v6 07/10] mtd: add ECC info for nand_flash_dev{}

[Huang Shijie]

Add an instance of an anonymous struct to store the ECC infor for full id
nand chips.
	@ecc.strength_ds: ECC correctability from the datasheet.
	@ecc.step_ds: ECC size required by the @ecc.strength_ds,

These two fields are all from the datasheet.

Also add the necessary macros to make the code simple and clean.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 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 9574e0a..217cdfe 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, _step)	\
+			{ .strength_ds = (_strength), .step_ds = (_step) }
+#define NAND_ECC_STRENGTH(type)		((type)->ecc.strength_ds)
+#define NAND_ECC_STEP(type)		((type)->ecc.step_ds)
+
 /**
  * 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_ds: The ECC correctability from the datasheet, same as the
+ *                   @ecc_strength_ds in nand_chip{}.
+ * @ecc.step_ds: The ECC step required by the @ecc.strength_ds, same as the
+ *               @ecc_step_ds in nand_chip{}, also from the datasheet.
+ *               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_ds;
+		uint16_t step_ds;
+	} ecc;
 };
 
 /**
-- 
1.7.1

[PATCH v6 08/10] 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 355976d..f7b514b 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3246,6 +3246,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_ds = NAND_ECC_STRENGTH(type);
+		chip->ecc_step_ds = NAND_ECC_STEP(type);
 
 		*busw = type->options & NAND_BUSWIDTH_16;
 
-- 
1.7.1

[PATCH v6 09/10] 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 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

[PATCH v6 10/10] 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 legacy_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>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |  132 +++++++++++++++++++++++++++++++-
 1 files changed, 131 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 25ecfa1..92c760b 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -112,7 +112,132 @@ 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_ds > 0 && chip->ecc_step_ds > 0))
+		return false;
+
+	switch (chip->ecc_step_ds) {
+	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_ds, chip->ecc_step_ds);
+		return false;
+	}
+	geo->ecc_chunk_size = chip->ecc_step_ds;
+	geo->ecc_strength = round_up(chip->ecc_strength_ds, 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_step_ds, mtd->oobsize);
+		return false;
+	}
+
+	/* The default value, see comment in the legacy_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|     |
+	 *    +---+----------+-+----------+-+----------+-+----------+-+-----+
+	 *                                                   ^              ^
+	 *                                                   |      O       |
+	 *                                                   |<------------>|
+	 *                                                   |              |
+	 *
+	 *	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 legacy_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 legacy_set_geometry(struct gpmi_nand_data *this)
 {
 	struct bch_geometry *geo = &this->bch_geometry;
 	struct mtd_info *mtd = &this->mtd;
@@ -224,6 +349,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 : legacy_set_geometry(this);
+}
+
 struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
 {
 	int chipnr = this->current_chip;
-- 
1.7.1

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

[Brian Norris]

Hi Huang and others,

On Thu, May 16, 2013 at 8:17 PM, Huang Shijie <b32955@freescale.com> 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.
>
>    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:
>               The keynote patch.
>
>    2.2) patch 2 ~ patch 6:
>                These patches are for ONFI nand.
>                Parse out the ecc info from the parameter page if we can, else
>                parse out the ecc info from the extended parameter page.
>
>    2.2) patch 7 ~ patch 9:
>                Add the ECC info for full-id nand, and parse it out.
>
>    2.3) 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.

What's the status on this patch set? Surely by v6 we have some
reasonable stable state on things like naming. Does anyone have any
other objections? Unfortunately, I've been awfully distracted, and on
top of that, I'm running into some bugs with my NAND controller
sending the ONFI parameter read/change column commands. But any time
my controller actually outputs a correct parameter page + extended
parameter page, this series has worked for me.

I've put my 2 cents in on most of the issues I had, and I tested the
whole series on my driver at around v5. The only issues I have with it
are somewhat cosmetic and not worth bikeshedding. So for all the
non-GPMI specific stuff I'll give my:

Reviewed-by: Brian Norris <computersforpeace@gmail.com>
Tested-by: Brian Norris <computersforpeace@gmail.com>

Thanks for the work Huang.

Brian

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

[Huang Shijie]

Hi Artem & Brian:
> Hi Huang and others,
>
> On Thu, May 16, 2013 at 8:17 PM, Huang Shijie<b32955@freescale.com>  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.
>>
>>     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:
>>                The keynote patch.
>>
>>     2.2) patch 2 ~ patch 6:
>>                 These patches are for ONFI nand.
>>                 Parse out the ecc info from the parameter page if we can, else
>>                 parse out the ecc info from the extended parameter page.
>>
>>     2.2) patch 7 ~ patch 9:
>>                 Add the ECC info for full-id nand, and parse it out.
>>
>>     2.3) 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.
> What's the status on this patch set? Surely by v6 we have some
> reasonable stable state on things like naming. Does anyone have any
> other objections? Unfortunately, I've been awfully distracted, and on
> top of that, I'm running into some bugs with my NAND controller
> sending the ONFI parameter read/change column commands. But any time
> my controller actually outputs a correct parameter page + extended
> parameter page, this series has worked for me.
>
> I've put my 2 cents in on most of the issues I had, and I tested the
> whole series on my driver at around v5. The only issues I have with it
> are somewhat cosmetic and not worth bikeshedding. So for all the
> non-GPMI specific stuff I'll give my:
>
> Reviewed-by: Brian Norris<computersforpeace@gmail.com>
> Tested-by: Brian Norris<computersforpeace@gmail.com>
>
> Thanks for the work Huang.
>
> Brian
>
Could you please merge this patch set?


thanks

Huang Shijie

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

[Brian Norris]

On Thu, Aug 08, 2013 at 04:33:35PM +0800, Huang Shijie wrote:
> Hi Artem & Brian:
> >Hi Huang and others,
> >
> >On Thu, May 16, 2013 at 8:17 PM, Huang Shijie<b32955@freescale.com>  wrote:
> >>1.) Why add the ECC information to the nand_chip{} ?
...
> >Reviewed-by: Brian Norris<computersforpeace@gmail.com>
> >Tested-by: Brian Norris<computersforpeace@gmail.com>
> >
> >Thanks for the work Huang.
> >
> Could you please merge this patch set?

Thanks for the reminder. I haven't sorted through all the piles of
backed up stuff yet!

I made a few minor changes (for checkpatch.pl, regarding
__attribute__((packed)), and to make the style a little more
easily-read) and tested the generic stuff on my platform. I've pushed
the series to l2-mtd.git. Thanks for the patience!

Brian

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

[Artem Bityutskiy]

On Thu, 2013-08-08 at 16:06 -0700, Brian Norris wrote:
> On Thu, Aug 08, 2013 at 04:33:35PM +0800, Huang Shijie wrote:
> > Hi Artem & Brian:
> > >Hi Huang and others,
> > >
> > >On Thu, May 16, 2013 at 8:17 PM, Huang Shijie<b32955@freescale.com>  wrote:
> > >>1.) Why add the ECC information to the nand_chip{} ?
> ...
> > >Reviewed-by: Brian Norris<computersforpeace@gmail.com>
> > >Tested-by: Brian Norris<computersforpeace@gmail.com>
> > >
> > >Thanks for the work Huang.
> > >
> > Could you please merge this patch set?
> 
> Thanks for the reminder. I haven't sorted through all the piles of
> backed up stuff yet!
> 
> I made a few minor changes (for checkpatch.pl, regarding
> __attribute__((packed)), and to make the style a little more
> easily-read) and tested the generic stuff on my platform. I've pushed
> the series to l2-mtd.git. Thanks for the patience!

I guess you can try aiaiai, I run it for all the patches I take to
l2-mtd.git. It uses also coccinelle/smatch/sparse and other tools to
verify the patches, not only checkpatch. And this git tree contains
scripts and various defconfigs for various MTD drivers - the scripts run
aiaiai.

For example, to check Huang's patches, I run something like:

aiaiai-concat-mboxes ~/tmp/huang*.mbox | ./verify ../l2-mtd/ gpmi-nand

where "huang*.mbox" is Huang's patch set.

There are aliases for verious drivers. There is a 'gen' alias which I
use for general changes like changes in nand_base.c.

Read about the tool here:
http://git.infradead.org/users/dedekind/aiaiai.git/blob/HEAD:/doc/README.announcement
http://git.infradead.org/users/dedekind/aiaiai.git/blob/HEAD:/doc/README

The mtd maintenance helpers:
http://git.infradead.org/users/dedekind/maintaining.git

There are few minor issues in aiaiai, which I can fix if you hit them
and complain. Also, I did not invest much time in making the user
interface and experience very nice. But this can also be improved.

-- 
Best Regards,
Artem Bityutskiy

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

[Brian Norris]

On 08/08/2013 08:58 PM, Artem Bityutskiy wrote:
> On Thu, 2013-08-08 at 16:06 -0700, Brian Norris wrote:
>> On Thu, Aug 08, 2013 at 04:33:35PM +0800, Huang Shijie wrote:
>>> Hi Artem & Brian:
>>>> Hi Huang and others,
>>>>
>>>> On Thu, May 16, 2013 at 8:17 PM, Huang Shijie<b32955@freescale.com>  wrote:
>>>>> 1.) Why add the ECC information to the nand_chip{} ?
>> ...
>>>> Reviewed-by: Brian Norris<computersforpeace@gmail.com>
>>>> Tested-by: Brian Norris<computersforpeace@gmail.com>
>>>>
>>>> Thanks for the work Huang.
>>>>
>>> Could you please merge this patch set?
>>
>> Thanks for the reminder. I haven't sorted through all the piles of
>> backed up stuff yet!
>>
>> I made a few minor changes (for checkpatch.pl, regarding
>> __attribute__((packed)), and to make the style a little more
>> easily-read) and tested the generic stuff on my platform. I've pushed
>> the series to l2-mtd.git. Thanks for the patience!
>
> I guess you can try aiaiai, I run it for all the patches I take to
> l2-mtd.git. It uses also coccinelle/smatch/sparse and other tools to
> verify the patches, not only checkpatch. And this git tree contains
> scripts and various defconfigs for various MTD drivers - the scripts run
> aiaiai.

Thanks, I actually already used it on his series :)

> For example, to check Huang's patches, I run something like:
>
> aiaiai-concat-mboxes ~/tmp/huang*.mbox | ./verify ../l2-mtd/ gpmi-nand
>
> where "huang*.mbox" is Huang's patch set.

What's the use of 'aiai-concat-mboxes' over a simple 'cat'? Just 
inserting the extra blank line, I guess? But if I have a concatenated 
mailbox, this is better:

./verify ../l2-mtd/ gpmi-name < ~/tmp/huang.mbox

> There are few minor issues in aiaiai, which I can fix if you hit them
> and complain. Also, I did not invest much time in making the user
> interface and experience very nice. But this can also be improved.

One issue (possibly related to the above question?) was that aiaia 
complained (via checkpatch.pl) about no sign-offs when I processed using 
the input redirection method on a multi-message mbox. I can take a 
closer look next time, but it's not a big deal, since I can verify this 
pretty easily.

Thanks again for the help and for the nice tools.

Brian

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

[Artem Bityutskiy]

On Thu, 2013-08-08 at 23:00 -0700, Brian Norris wrote:
> On 08/08/2013 08:58 PM, Artem Bityutskiy wrote:
> > On Thu, 2013-08-08 at 16:06 -0700, Brian Norris wrote:
> >> On Thu, Aug 08, 2013 at 04:33:35PM +0800, Huang Shijie wrote:
> >>> Hi Artem & Brian:
> >>>> Hi Huang and others,
> >>>>
> >>>> On Thu, May 16, 2013 at 8:17 PM, Huang Shijie<b32955@freescale.com>  wrote:
> >>>>> 1.) Why add the ECC information to the nand_chip{} ?
> >> ...
> >>>> Reviewed-by: Brian Norris<computersforpeace@gmail.com>
> >>>> Tested-by: Brian Norris<computersforpeace@gmail.com>
> >>>>
> >>>> Thanks for the work Huang.
> >>>>
> >>> Could you please merge this patch set?
> >>
> >> Thanks for the reminder. I haven't sorted through all the piles of
> >> backed up stuff yet!
> >>
> >> I made a few minor changes (for checkpatch.pl, regarding
> >> __attribute__((packed)), and to make the style a little more
> >> easily-read) and tested the generic stuff on my platform. I've pushed
> >> the series to l2-mtd.git. Thanks for the patience!
> >
> > I guess you can try aiaiai, I run it for all the patches I take to
> > l2-mtd.git. It uses also coccinelle/smatch/sparse and other tools to
> > verify the patches, not only checkpatch. And this git tree contains
> > scripts and various defconfigs for various MTD drivers - the scripts run
> > aiaiai.
> 
> Thanks, I actually already used it on his series :)
> 
> > For example, to check Huang's patches, I run something like:
> >
> > aiaiai-concat-mboxes ~/tmp/huang*.mbox | ./verify ../l2-mtd/ gpmi-nand
> >
> > where "huang*.mbox" is Huang's patch set.
> 
> What's the use of 'aiai-concat-mboxes' over a simple 'cat'?

Good question, I do not remember. There was some problem with plain cat.
May be related to the fact that with plain cat you end up with an extra
newline, but right now I do not see how this can be a problem.

Weird, why I wrote this script?

We used Aiaiai for automatic testing of all patches in a mailing list,
and at that times I decided that I need that script, but don't remember
now.

Sorry :-)

>  Just 
> inserting the extra blank line, I guess? But if I have a concatenated 
> mailbox, this is better:
> 
> ./verify ../l2-mtd/ gpmi-name < ~/tmp/huang.mbox
> 
> > There are few minor issues in aiaiai, which I can fix if you hit them
> > and complain. Also, I did not invest much time in making the user
> > interface and experience very nice. But this can also be improved.
> 
> One issue (possibly related to the above question?) was that aiaia 
> complained (via checkpatch.pl) about no sign-offs when I processed using 
> the input redirection method on a multi-message mbox. 

Yes, aiaiai runs checkpatch.pl for individual patches, and then to a
concatenated patch, and the latter does not have a signed-off-by. Fixing
is about adding --no-signoff to the right place.

Other issue is that in ARM targets there are some AS strings which are
not properly filtered out. I the simplest fix is about filtering those
out completely in aiaiai-diff-log. More complex fix would need to match
those lines. The problem is that the warnings are in tmp files, and the
file-names are different, thus they are shown in the diff.

> I can take a 
> closer look next time, but it's not a big deal, since I can verify this 
> pretty easily.
> 
> Thanks again for the help and for the nice tools.

NP, thank you for helping too.

I am a bit worried that at some point we may screw each other's work by
pushing things out at the same time. We should be very careful with
--force.

-- 
Best Regards,
Artem Bityutskiy

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

[Brian Norris]

On Fri, Aug 9, 2013 at 12:23 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Thu, 2013-08-08 at 23:00 -0700, Brian Norris wrote:
>> On 08/08/2013 08:58 PM, Artem Bityutskiy wrote:
>> > There are few minor issues in aiaiai, which I can fix if you hit them
>> > and complain. Also, I did not invest much time in making the user
>> > interface and experience very nice. But this can also be improved.
>>
>> One issue (possibly related to the above question?) was that aiaia
>> complained (via checkpatch.pl) about no sign-offs when I processed using
>> the input redirection method on a multi-message mbox.
>
> Yes, aiaiai runs checkpatch.pl for individual patches, and then to a
> concatenated patch, and the latter does not have a signed-off-by. Fixing
> is about adding --no-signoff to the right place.

I can look at that if I run into it again and have the time to fix it.

> I am a bit worried that at some point we may screw each other's work by
> pushing things out at the same time. We should be very careful with
> --force.

I think by limiting our use of rebase and exercising care with
--force, we should be fine. And if we do a 'fetch' just before, then
the probability of a collision should be pretty low. Famous last
words.

Brian

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

[Huang Shijie]

于 2013年05月16日 23:17, 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 1:
>    	      The keynote patch.
>
>    2.2) patch 2 ~ patch 6:
>                These patches are for ONFI nand.
>                Parse out the ecc info from the parameter page if we can, else
> 	       parse out the ecc info from the extended parameter page.
> 	       
>    2.2) patch 7 ~ patch 9:
>                Add the ECC info for full-id nand, and parse it out.
>
>    2.3) 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.
>
> v5 --> v6:
> 	[0] rename the ecc_strength/ecc_step to ecc_strength_ds/ecc_step_ds.
> 	[1] fix a case when "the codeword is 0".
> 	[2] fix typo.
> 	[3] add the 'O' to the diagram in the gpmi patch.
>
> v4 --> v5:
> 	[0] rebase the new l2-mtd(dropped the 3 merged patches).
> 	[1] rename ecc_size to ecc_step.
> 	[2] change the comment for the new fields.
>
> 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.
>
> Huang Shijie (10):
>   mtd: add datasheet's ECC information to nand_chip{}
>   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_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 |  132 +++++++++++++++++++++++++++++++-
>  drivers/mtd/nand/nand_base.c           |   93 ++++++++++++++++++++++-
>  drivers/mtd/nand/nand_ids.c            |    8 +-
>  include/linux/mtd/nand.h               |   72 +++++++++++++++++-
>  4 files changed, 296 insertions(+), 9 deletions(-)
>
Hi Artem:
Could you merge this patch set?


thanks
Huang Shijie