[PATCH v2 0/8] mtd: add datasheet's ECC information to nand_chip{}

[PATCH v2 0/8] 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 ~ patch 4:
	       Parse out the ecc info from the extended parameter page for
	       ONFI nand.
	       
   2.2) patch 5 ~ patch 7:
               Add the ECC info for full-id nand.

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


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 nand_ 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 (8):
  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           |   62 +++++++++++++++-
 drivers/mtd/nand/nand_ids.c            |    8 +-
 include/linux/mtd/nand.h               |   62 +++++++++++++++-
 4 files changed, 251 insertions(+), 9 deletions(-)

[PATCH v2 1/8] 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.

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 5458021..210bf36 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 v2 2/8] 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 210bf36..122322c 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -202,6 +202,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)
@@ -750,6 +755,14 @@ 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)
+{
+	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

Re: [PATCH v2 2/8] mtd: add a helper to get the supported features for ONFI nand

[Brian Norris]

On Mon, Apr 22, 2013 at 12:47 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 210bf36..122322c 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -202,6 +202,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)

Where did you get the UNKNOWN field from? Bit 15 looks like a reserved
field (in ONFI 3.1, at least) and certainly does *not* represent a
null value. Instead, I think it does not need a macro at all, and we
can just use 0 for non-ONFI chips.

>  /* ONFI timing mode, used in both asynchronous and synchronous mode */
>  #define ONFI_TIMING_MODE_0             (1 << 0)
>  #define ONFI_TIMING_MODE_1             (1 << 1)
> @@ -750,6 +755,14 @@ 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)
> +{
> +       if (!chip->onfi_version)
> +               return ONFI_FEATURE_UNKNOWN;

As noted above, just this macro is incorrect.

> +       return le16_to_cpu(chip->onfi_params.features);

Just make the whole function:

return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0;

It is up to the caller to ensure that they don't rely on the returned
value for non-ONFI chips.

> +}
> +
>  /* return the supported asynchronous timing mode. */
>  static inline int onfi_get_async_timing_mode(struct nand_chip *chip)
>  {

Brian

Re: [PATCH v2 2/8] mtd: add a helper to get the supported features for ONFI nand

[Huang Shijie]

于 2013年04月23日 05:04, Brian Norris 写道:
> return chip->onfi_version ? le16_to_cpu(chip->onfi_params.features) : 0;
it's ok to me. I will use 0 as the unknow feature in the next version.

thanks
Huang Shijie

[PATCH v2 3/8] 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 beff911..48ff097 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2846,6 +2846,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 onfi_ext_param_page *ep;
+	struct onfi_ext_section *s;
+	struct onfi_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 < 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)
+		goto ext_out;
+
+	/* get the info we want. */
+	ecc = (struct onfi_ext_ecc_info *)cursor;
+	chip->ecc_strength = ecc->ecc_bits;
+	chip->ecc_size = 1 << ecc->codeword_size;
+
+	pr_info("ONFI extended param page detected.\n");
+ext_out:
+	kfree(ep);
+}
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -2914,6 +2964,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_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

Re: [PATCH v2 3/8] mtd: get the ECC info from the Extended Parameter Page

[Brian Norris]

On Mon, Apr 22, 2013 at 12:47 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 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 beff911..48ff097 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2846,6 +2846,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)
> +{

I think we want a return code (int) for this function. It obviously
can fail, and the caller needs to know this.

The "last" parameter is not very obvious until you read the whole
function, where you see that this function assumes a lot about the
caller. Please address the comments below and/or fully document the
parameters and calling context for this function.

> +       struct onfi_ext_param_page *ep;
> +       struct onfi_ext_section *s;
> +       struct onfi_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.

Why not?

> +        */
> +       for (i = last + 1; i < p->num_of_param_pages; i++)
> +               chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p));

You never sent a command to the chip. How can you expect to read from it?

It seems that you are writing this function with the assumption of a
particular calling context (a context in which the last command was
CMD_PARAM). IMO, it would make a lot more sense that this function
actually send its own CMD_PARAM followed by either X bytes of skipped
read_buf() or a change read column command. Then it doesn't need the
"last" argument, and its usage makes much more sense.

> +
> +       /* 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 < 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)
> +               goto ext_out;
> +
> +       /* get the info we want. */
> +       ecc = (struct onfi_ext_ecc_info *)cursor;
> +       chip->ecc_strength = ecc->ecc_bits;
> +       chip->ecc_size = 1 << ecc->codeword_size;
> +
> +       pr_info("ONFI extended param page detected.\n");
> +ext_out:
> +       kfree(ep);
> +}
> +
>  /*
>   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
>   */
> @@ -2914,6 +2964,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_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
>
>

Re: [PATCH v2 3/8] mtd: get the ECC info from the Extended Parameter Page

[Huang Shijie]

于 2013年04月23日 05:22, Brian Norris 写道:
> On Mon, Apr 22, 2013 at 12:47 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 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 beff911..48ff097 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -2846,6 +2846,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)
>> +{
> I think we want a return code (int) for this function. It obviously
> can fail, and the caller needs to know this.
I not sure who will uses ecc_strength/ecc_size, except the gpmi.
So i ignore the return value.

If you think we should add it, i will add it.
> The "last" parameter is not very obvious until you read the whole
> function, where you see that this function assumes a lot about the
> caller. Please address the comments below and/or fully document the
> parameters and calling context for this function.
>
ok. I can add more comments.

>> +       struct onfi_ext_param_page *ep;
>> +       struct onfi_ext_section *s;
>> +       struct onfi_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.
> Why not?
>
You can give me a fix patch which bases on my patch set.
I can test it. :)

I tried to use the Change-read-columm command, but failed.

>> +        */
>> +       for (i = last + 1; i<  p->num_of_param_pages; i++)
>> +               chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p));
> You never sent a command to the chip. How can you expect to read from it?
we have sent a command in the nand_flash_detect_onfi().
> It seems that you are writing this function with the assumption of a
> particular calling context (a context in which the last command was
> CMD_PARAM). IMO, it would make a lot more sense that this function
> actually send its own CMD_PARAM followed by either X bytes of skipped
> read_buf() or a change read column command. Then it doesn't need the
> "last" argument, and its usage makes much more sense.
>
I added the "last" argument just because the Change-read-column command 
did not works.


thanks
Huang Shijie

Re: [PATCH v2 3/8] mtd: get the ECC info from the Extended Parameter Page

[Brian Norris]

On Mon, Apr 22, 2013 at 7:43 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年04月23日 05:22, Brian Norris 写道:
>
>> On Mon, Apr 22, 2013 at 12:47 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 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 beff911..48ff097 100644
>>> --- a/drivers/mtd/nand/nand_base.c
>>> +++ b/drivers/mtd/nand/nand_base.c
>>> @@ -2846,6 +2846,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)
>>> +{
>>
>> I think we want a return code (int) for this function. It obviously
>> can fail, and the caller needs to know this.
>
> I not sure who will uses ecc_strength/ecc_size, except the gpmi.
> So i ignore the return value.

Well, that's the dangerous part of this patch series: that it is
written entirely from the point of view of a specific use case (gpmi).
It can be improved a little to make it more generically useful.

> If you think we should add it, i will add it.

Well, the code doesn't clearly show what happens to
ecc_strength/ecc_size if (1) we are out of memory (but we are hosed in
this case anyway...) or (2) the extended parameter page is corrupted
and/or not present. In either case, the caller may want to zero out
the the parameters, set them to -1, or whatever else we decide for the
null case.

>> The "last" parameter is not very obvious until you read the whole
>> function, where you see that this function assumes a lot about the
>> caller. Please address the comments below and/or fully document the
>> parameters and calling context for this function.
>>
> ok. I can add more comments.
>
>
>>> +       struct onfi_ext_param_page *ep;
>>> +       struct onfi_ext_section *s;
>>> +       struct onfi_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);

Does this need to be zeroed out? We will overwrite it before using it
anyway. I'd just recommend kmalloc.

>>> +       if (!ep)
>>> +               goto ext_out;
>>> +
>>> +       /*
>>> +        * Skip the ONFI Parameter Pages.
>>> +        * The Change Read Columm command may does not works here.
>>
>> Why not?
>>
> You can give me a fix patch which bases on my patch set.
> I can test it. :)
>
> I tried to use the Change-read-columm command, but failed.

I believe the behavior here will really depend on the driver used. My
driver, for instance, intercepts these commands and sends them to the
controller in a different form. So my patches won't necessarily help
you if your driver is broken :)

>>> +        */
>>> +       for (i = last + 1; i<  p->num_of_param_pages; i++)
>>> +               chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p));
>>
>> You never sent a command to the chip. How can you expect to read from it?
>
> we have sent a command in the nand_flash_detect_onfi().

Right, I understand. But it's not very clean code if it makes this
assumption w/o documenting it.

>> It seems that you are writing this function with the assumption of a
>> particular calling context (a context in which the last command was
>> CMD_PARAM). IMO, it would make a lot more sense that this function
>> actually send its own CMD_PARAM followed by either X bytes of skipped
>> read_buf() or a change read column command. Then it doesn't need the
>> "last" argument, and its usage makes much more sense.
>>
> I added the "last" argument just because the Change-read-column command did
> not works.

Brian

Re: [PATCH v2 3/8] mtd: get the ECC info from the Extended Parameter Page

[Huang Shijie]

于 2013年04月23日 05:22, Brian Norris 写道:
> You never sent a command to the chip. How can you expect to read from it?
>
> It seems that you are writing this function with the assumption of a
> particular calling context (a context in which the last command was
> CMD_PARAM). IMO, it would make a lot more sense that this function
> actually send its own CMD_PARAM followed by either X bytes of skipped
> read_buf() or a change read column command. Then it doesn't need the
> "last" argument, and its usage makes much more sense.
>
I finally find why i can not use the CHANGE READ COLUMN command:

When we detect the ONFI nand, we actually use the nand_command() to 
issue the command which
does not works with CHANGE READ COLUMN command.

I use the nand_command_lp() to issue the CHANGE READ COLUMN command, it 
works fine.

I can remove the "last" argument now.

thanks Brian.

Huang Shijie

[PATCH v2 4/8] 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 48ff097..ba87e43 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2957,9 +2957,9 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
 	chip->chipsize = le32_to_cpu(p->blocks_per_lun);
 	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
-	*busw = 0;
-	if (le16_to_cpu(p->features) & 1)
-		*busw = NAND_BUSWIDTH_16;
+
+	*busw = (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS) ?
+			NAND_BUSWIDTH_16 : 0;
 
 	if (p->ecc_bits != 0xff) {
 		chip->ecc_strength = p->ecc_bits;
-- 
1.7.1

[PATCH v2] 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>
---
I missed to rename to the onfi_feature(). This patch fixes it.

---
 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 48ff097..ba87e43 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2957,9 +2957,9 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
 	chip->chipsize = le32_to_cpu(p->blocks_per_lun);
 	chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
-	*busw = 0;
-	if (le16_to_cpu(p->features) & 1)
-		*busw = NAND_BUSWIDTH_16;
+
+	*busw = (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS) ?
+			NAND_BUSWIDTH_16 : 0;
 
 	if (p->ecc_bits != 0xff) {
 		chip->ecc_strength = p->ecc_bits;
-- 
1.7.1

[PATCH v2 5/8] 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 122322c..8238e00 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -623,6 +623,10 @@ struct nand_chip {
 	{ .name = (nm), {{ .dev_id = (devid) }}, .chipsize = (chipsz), \
 	  .options = (opts) }
 
+#define NAND_ECC_INFO(strength, size)	(((strength) << 16) | (size))
+#define NAND_ECC_STRENGTH(x)		(((x) >> 16) & 0xffff)
+#define NAND_ECC_SIZE(x)		((x) & 0xffff)
+
 /**
  * struct nand_flash_dev - NAND Flash Device ID Structure
  * @name: a human-readable name of the NAND chip
@@ -640,6 +644,11 @@ struct nand_chip {
  * @options: stores various chip bit options
  * @id_len: The valid length of the @id.
  * @oobsize: OOB size
+ * @ecc_info: The ECC information.
+ *            lower 16 bits: store the ECC bits.
+ *            upper 16 bits: store the ECC data chunk size.
+ *            For example, the "4bit ECC for each 512Byte" can be set with
+ *            NAND_ECC_INFO(4, 512) macro.
  */
 struct nand_flash_dev {
 	char *name;
@@ -656,6 +665,7 @@ struct nand_flash_dev {
 	unsigned int options;
 	uint16_t id_len;
 	uint16_t oobsize;
+	uint32_t ecc_info;
 };
 
 /**
-- 
1.7.1

[PATCH v2 6/8] 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 ba87e43..1611b7a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3231,6 +3231,8 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->cellinfo = id_data[2];
 		chip->chipsize = (uint64_t)type->chipsize << 20;
 		chip->options |= type->options;
+		chip->ecc_strength = NAND_ECC_STRENGTH(type->ecc_info);
+		chip->ecc_size = NAND_ECC_SIZE(type->ecc_info);
 
 		*busw = type->options & NAND_BUSWIDTH_16;
 
-- 
1.7.1

[PATCH v2 7/8] 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 v2 8/8] 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