diff mbox

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

Message ID 1369015705-4720-1-git-send-email-b32955@freescale.com
State New, archived
Headers show

Commit Message

Huang Shijie May 20, 2013, 2:08 a.m. UTC 
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(-)

Comments

Brian Norris May 20, 2013, 6:05 a.m. UTC | #1 
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".
Huang Shijie May 22, 2013, 2:15 a.m. UTC | #2 
于 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
diff mbox

Patch

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");