Skip to contentSkip to conversation

NAND: add support for reading ONFI parameters from NAND device

PATCH In review Florian Fainelli 3 participants 4 messages Aug 30, 2010 – Oct 21, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v3 3/3 NAND: add support for reading ONFI parameters from NAND device

This patch adds support for reading NAND device ONFI parameters and use the ONFI informations to define its geometry. In case the device supports ONFI, the onfi_version field in struct nand_chip contains the version (BCD) and the onfi_params structure can be used by drivers to set up timings and such. We currently only support ONFI 1.0 parameters.

---
Changes since v1:
- factored out crc16 to lib/crc16.c
- reversed the ONFI version checking order: from highest to lowest
- added define for the CRC16 initialization value
- use strim to cleanup whitespace in string

Changes since v2:
- put back crc16 as onfi_crc16 which is ONFI specific

drivers/mtd/nand/nand_base.c +134 -30
index d551ddd..9b3271f 100644
@@ -2787,14 +2787,46 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
 }
 
 /*
+ * sanitize ONFI strings so we can safely print them
+ */
+static void sanitize_string(uint8_t *s, size_t len)
+{
+	ssize_t i;
+
+	/* null terminate */
+	s[len - 1] = 0;
+
+	/* remove non printable chars */
+	for (i = 0; i < len - 1; i++) {
+		if (s[i] < ' ' || s[i] > 127)
+			s[i] = '?';
+	}
+
+	/* remove trailing spaces */
+	strim(s);
+}
+
+static u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
+{
+	int i;
+	while (len--) {
+		crc ^= *p++ << 8;
+		for (i = 0; i < 8; i++)
+			crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
+	}
+
+	return crc;
+}
+
+/*
  * Get the flash and manufacturer id and lookup if the type is supported
  */
 static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 						  struct nand_chip *chip,
-						  int busw, int *maf_id,
+						  int busw, int *maf_id, int *dev_id,
 						  struct nand_flash_dev *type)
 {
-	int i, dev_id, maf_idx;
+	int i, maf_idx;
 	u8 id_data[8];
 
 	/* Select the device */
@@ -2811,7 +2843,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 
 	/* Read manufacturer and device IDs */
 	*maf_id = chip->read_byte(mtd);
-	dev_id = chip->read_byte(mtd);
+	*dev_id = chip->read_byte(mtd);
 
 	/* Try again to make sure, as some systems the bus-hold or other
 	 * interface concerns can cause random data which looks like a
@@ -2821,15 +2853,13 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 
 	chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
 
-	/* Read entire ID string */
-
-	for (i = 0; i < 8; i++)
+	for (i = 0; i < 2; i++)
 		id_data[i] = chip->read_byte(mtd);
 
-	if (id_data[0] != *maf_id || id_data[1] != dev_id) {
+	if (id_data[0] != *maf_id || id_data[1] != *dev_id) {
 		printk(KERN_INFO "%s: second ID read did not match "
 		       "%02x,%02x against %02x,%02x\n", __func__,
-		       *maf_id, dev_id, id_data[0], id_data[1]);
+		       *maf_id, *dev_id, id_data[0], id_data[1]);
 		return ERR_PTR(-ENODEV);
 	}
 
@@ -2837,9 +2867,81 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		type = nand_flash_ids;
 
 	for (; type->name != NULL; type++)
-		if (dev_id == type->id)
+		if (*dev_id == type->id)
                         break;
 
+	chip->onfi_version = 0;
+	if (!type->name || !type->pagesize) {
+		/* try ONFI for unknow chip or LP */
+		chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
+		if (chip->read_byte(mtd) == 'O' &&
+			chip->read_byte(mtd) == 'N' &&
+			chip->read_byte(mtd) == 'F' &&
+			chip->read_byte(mtd) == 'I') {
+
+			struct nand_onfi_params *p = &chip->onfi_params;
+			int i;
+
+			printk(KERN_INFO "ONFI flash detected\n");
+			chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
+			for (i = 0; i < 3; i++) {
+				chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+				if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
+								le16_to_cpu(p->crc))
+				{
+					 printk(KERN_INFO "ONFI param page %d valid\n", i);
+					 break;
+				}
+			}
+
+			if (i < 3) {
+				/* check version */
+				int val = le16_to_cpu(p->revision);
+				if (val == 1 || val > (1 << 4))
+					printk(KERN_INFO "%s: unsupported ONFI version: %d\n",
+						__func__, val);
+				else {
+					if (val & (1 << 4))
+						chip->onfi_version = 22;
+					else if (val & (1 << 3))
+						chip->onfi_version = 21;
+					else if (val & (1 << 2))
+						chip->onfi_version = 20;
+					else
+						chip->onfi_version = 10;
+				}
+			}
+
+			if (chip->onfi_version) {
+				sanitize_string(p->manufacturer, sizeof(p->manufacturer));
+				sanitize_string(p->model, sizeof(p->model));
+				if (!mtd->name)
+					mtd->name = p->model;
+				mtd->writesize = le32_to_cpu(p->byte_per_page);
+				mtd->erasesize = le32_to_cpu(p->pages_per_block)*mtd->writesize;
+				mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
+				chip->chipsize = le32_to_cpu(p->blocks_per_lun) * mtd->erasesize;
+				busw = 0;
+				if (le16_to_cpu(p->features) & 1)
+					busw = NAND_BUSWIDTH_16;
+
+				chip->options &= ~NAND_CHIPOPTIONS_MSK;
+				chip->options |= (NAND_NO_READRDY |
+						NAND_NO_AUTOINCR) & NAND_CHIPOPTIONS_MSK;
+
+				goto ident_done;
+
+			}
+		}
+	}
+
+	chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
+
+	/* Read entire ID string */
+
+	for (i = 0; i < 8; i++)
+		id_data[i] = chip->read_byte(mtd);
+
 	if (!type->name)
 		return ERR_PTR(-ENODEV);
 
@@ -2901,6 +3003,21 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		mtd->oobsize = mtd->writesize / 32;
 		busw = type->options & NAND_BUSWIDTH_16;
 	}
+	/* Get chip options, preserve non chip based options */
+	chip->options &= ~NAND_CHIPOPTIONS_MSK;
+	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
+
+	/* Check if chip is a not a samsung device. Do not clear the
+	 * options for chips which are not having an extended id.
+	 */
+	if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
+		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
+ident_done:
+
+	/*
+	 * Set chip as a default. Board drivers can override it, if necessary
+	 */
+	chip->options |= NAND_NO_AUTOINCR;
 
 	/* Try to identify manufacturer */
 	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
@@ -2915,7 +3032,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
 		printk(KERN_INFO "NAND device: Manufacturer ID:"
 		       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
-		       dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
+		       *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
 		printk(KERN_WARNING "NAND bus width %d instead %d bit\n",
 		       (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
 		       busw ? 16 : 8);
@@ -2940,21 +3057,6 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	else
 		chip->badblockpos = NAND_SMALL_BADBLOCK_POS;
 
-	/* Get chip options, preserve non chip based options */
-	chip->options &= ~NAND_CHIPOPTIONS_MSK;
-	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
-
-	/*
-	 * Set chip as a default. Board drivers can override it, if necessary
-	 */
-	chip->options |= NAND_NO_AUTOINCR;
-
-	/* Check if chip is a not a samsung device. Do not clear the
-	 * options for chips which are not having an extended id.
-	 */
-	if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
-		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
-
 	/*
 	 * Bad block marker is stored in the last page of each block
 	 * on Samsung and Hynix MLC devices; stored in first two pages
@@ -2994,9 +3096,11 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
 		chip->cmdfunc = nand_command_lp;
 
+	/* TODO onfi flash name */
 	printk(KERN_INFO "NAND device: Manufacturer ID:"
-	       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, dev_id,
-	       nand_manuf_ids[maf_idx].name, type->name);
+		" 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id,
+		nand_manuf_ids[maf_idx].name,
+		chip->onfi_version ? type->name:chip->onfi_params.model);
 
 	return type;
 }
@@ -3015,7 +3119,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		    struct nand_flash_dev *table)
 {
-	int i, busw, nand_maf_id;
+	int i, busw, nand_maf_id, nand_dev_id;
 	struct nand_chip *chip = mtd->priv;
 	struct nand_flash_dev *type;
 
@@ -3025,7 +3129,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	nand_set_defaults(chip, busw);
 
 	/* Read the flash type */
-	type = nand_get_flash_type(mtd, chip, busw, &nand_maf_id, table);
+	type = nand_get_flash_type(mtd, chip, busw, &nand_maf_id, &nand_dev_id, table);
 
 	if (IS_ERR(type)) {
 		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
@@ -3043,7 +3147,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
 		/* Read manufacturer and device IDs */
 		if (nand_maf_id != chip->read_byte(mtd) ||
-		    type->id != chip->read_byte(mtd))
+		    nand_dev_id != chip->read_byte(mtd))
 			break;
 	}
 	if (i > 1)
include/linux/mtd/nand.h +68 -0

86 lines not on this page · raw message

#1 lore raw

Hi,

I've added these to my l2-mtd-2.6 / dunno tree, but I have a comment: is it possible/feasible to make the below block to be a separate function. It is quite large, and difficult to read when it is embedded into the function. Also, the indentation becomes more difficult because of too much nesting.

Also, some stylistic nit-picks below.

Quoted 10 lines · Florian Fainelli
drivers/mtd/nand/nand_base.c +10 -0
+		/* try ONFI for unknow chip or LP */
+		chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
+		if (chip->read_byte(mtd) == 'O' &&
+			chip->read_byte(mtd) == 'N' &&
+			chip->read_byte(mtd) == 'F' &&
+			chip->read_byte(mtd) == 'I') {
+
+			struct nand_onfi_params *p = &chip->onfi_params;
+			int i;
+

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, · line 2894

Quoted 10 lines
drivers/mtd/nand/nand_base.c +10 -0
+			printk(KERN_INFO "ONFI flash detected\n");
+			chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
+			for (i = 0; i < 3; i++) {
+				chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+				if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
+								le16_to_cpu(p->crc))
+				{
+					 printk(KERN_INFO "ONFI param page %d valid\n", i);
+					 break;
+				}

I think { should me on the same line as if. Moving to a separate func will help. Also, check the patch with scripts/checkpatch.pl.

Quoted 19 lines
drivers/mtd/nand/nand_base.c +19 -0
+			}
+
+			if (i < 3) {
+				/* check version */
+				int val = le16_to_cpu(p->revision);
+				if (val == 1 || val > (1 << 4))
+					printk(KERN_INFO "%s: unsupported ONFI version: %d\n",
+						__func__, val);
+				else {
+					if (val & (1 << 4))
+						chip->onfi_version = 22;
+					else if (val & (1 << 3))
+						chip->onfi_version = 21;
+					else if (val & (1 << 2))
+						chip->onfi_version = 20;
+					else
+						chip->onfi_version = 10;
+				}
+			}

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, · line 2923

Quoted 10 lines
drivers/mtd/nand/nand_base.c +10 -0
+
+			if (chip->onfi_version) {
+				sanitize_string(p->manufacturer, sizeof(p->manufacturer));
+				sanitize_string(p->model, sizeof(p->model));
+				if (!mtd->name)
+					mtd->name = p->model;
+				mtd->writesize = le32_to_cpu(p->byte_per_page);
+				mtd->erasesize = le32_to_cpu(p->pages_per_block)*mtd->writesize;
+				mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
+				chip->chipsize = le32_to_cpu(p->blocks_per_lun) * mtd->erasesize;

You have whitespaces around '*' here, but 2 lines before you did not have.

Quoted 12 lines
drivers/mtd/nand/nand_base.c +12 -0
+				busw = 0;
+				if (le16_to_cpu(p->features) & 1)
+					busw = NAND_BUSWIDTH_16;
+
+				chip->options &= ~NAND_CHIPOPTIONS_MSK;
+				chip->options |= (NAND_NO_READRDY |
+						NAND_NO_AUTOINCR) & NAND_CHIPOPTIONS_MSK;
+
+				goto ident_done;
+
+			}
+		}
Signature · 2 lines
Best Regards,
Artem Bityutskiy (Битюцкий Артём)
#2 lore raw

Hello Artem,

Quoted 7 lines · On Tuesday 31 August 2010 13:46:59 Artem Bityutskiy wrote:

Hi,

I've added these to my l2-mtd-2.6 / dunno tree, but I have a comment: is it possible/feasible to make the below block to be a separate function. It is quite large, and difficult to read when it is embedded into the function. Also, the indentation becomes more difficult because of too much nesting.

I will submit an incremental patch which addresses the issue you mentionned. Thanks for your review!

Quoted 14 lines

Also, some stylistic nit-picks below.

Quoted 10 lines · Florian Fainelli
drivers/mtd/nand/nand_base.c +10 -0
+		/* try ONFI for unknow chip or LP */
+		chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
+		if (chip->read_byte(mtd) == 'O' &&
+			chip->read_byte(mtd) == 'N' &&
+			chip->read_byte(mtd) == 'F' &&
+			chip->read_byte(mtd) == 'I') {
+
+			struct nand_onfi_params *p = &chip->onfi_params;
+			int i;
+
Quoted 8 lines

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,

Quoted 8 lines
drivers/mtd/nand/nand_base.c +8 -0
+			printk(KERN_INFO "ONFI flash detected\n");
+			chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
+			for (i = 0; i < 3; i++) {
+				chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+				if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
+								le16_to_cpu(p->crc))
+				{
+					 printk(KERN_INFO "ONFI param page %d 

valid\n", i);

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+					 break;
+				}

I think { should me on the same line as if. Moving to a separate func will help. Also, check the patch with scripts/checkpatch.pl.

To be frank, nand_base.c already had quite some stylistic issues so I did not even give it a try.

Quoted 8 lines

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,

Quoted 8 lines
drivers/mtd/nand/nand_base.c +7 -0
+			}
+
+			if (i < 3) {
+				/* check version */
+				int val = le16_to_cpu(p->revision);
+				if (val == 1 || val > (1 << 4))
+					printk(KERN_INFO "%s: unsupported ONFI 

version: %d\n",

Quoted 22 lines
Quoted 5 lines
drivers/mtd/nand/nand_base.c +5 -0
+						__func__, val);
+				else {
+					if (val & (1 << 4))
+						chip->onfi_version = 22;
+					else if (val & (1 << 3))

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,

Quoted 10 lines
drivers/mtd/nand/nand_base.c +10 -0
+						chip->onfi_version = 21;
+					else if (val & (1 << 2))
+						chip->onfi_version = 20;
+					else
+						chip->onfi_version = 10;
+				}
+			}
+
+			if (chip->onfi_version) {
+				sanitize_string(p->manufacturer, sizeof(p-

manufacturer));

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,

Quoted 5 lines
drivers/mtd/nand/nand_base.c +5 -0
+				sanitize_string(p->model, sizeof(p->model));
+				if (!mtd->name)
+					mtd->name = p->model;
+				mtd->writesize = le32_to_cpu(p->byte_per_page);
+				mtd->erasesize = le32_to_cpu(p-

pages_per_block)*mtd->writesize;

Quoted 3 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+				mtd->oobsize = le16_to_cpu(p-

spare_bytes_per_page);

Review

Quoted 1 line
(quoted diff) +1 -0
+				chip->chipsize = le32_to_cpu(p->blocks_per_lun) * 

mtd->erasesize;

Quoted 11 lines

You have whitespaces around '*' here, but 2 lines before you did not have.

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,

Quoted 7 lines
drivers/mtd/nand/nand_base.c +7 -0
+				busw = 0;
+				if (le16_to_cpu(p->features) & 1)
+					busw = NAND_BUSWIDTH_16;
+
+				chip->options &= ~NAND_CHIPOPTIONS_MSK;
+				chip->options |= (NAND_NO_READRDY |
+						NAND_NO_AUTOINCR) & 

NAND_CHIPOPTIONS_MSK;

Quoted 5 lines
Quoted 5 lines
drivers/mtd/nand/nand_base.c +5 -0
+
+				goto ident_done;
+
+			}
+		}
#3 lore raw
In reply to Florian Fainelli

Hi,

Quoted 11 lines · Florian Fainelli

This patch adds support for reading NAND device ONFI parameters and use the ONFI informations to define its geometry. In case the device supports ONFI, the onfi_version field in struct nand_chip contains the version (BCD) and the onfi_params structure can be used by drivers to set up timings and such. We currently only support ONFI 1.0 parameters.

---

                chip->cmdfunc = nand_command_lp;

drivers/mtd/nand/nand_base.c static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, · line 3103

Quoted 9 lines
drivers/mtd/nand/nand_base.c +4 -2
+       /* TODO onfi flash name */
        printk(KERN_INFO "NAND device: Manufacturer ID:"
-              " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, dev_id,
-              nand_manuf_ids[maf_idx].name, type->name);
+               " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id,
+               nand_manuf_ids[maf_idx].name,
+               chip->onfi_version ? type->name:chip->onfi_params.model);

I don't know if it has been noticed but it should be chip->onfi_version ? chip->onfi_params.model : type->name

Matthieu