Skip to contentSkip to conversation

mtd: teach mxc-nand about ONFI probing

PATCH New Uwe Kleine-König 4 participants 14 messages Feb 10, 2015 – Mar 11, 2015

Subsystems (3)
Arm i.MX and Layerscape Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

Uwe Kleine-König

author
#0 lore raw

cover mtd: teach mxc-nand about ONFI probing

Hello,

this series implements support for ONFI probing in mxc-nand. Before a PARAM command was just ignored and the following read_bytes read whatever then happened to be available from the controller's RAM. But actually this wasn't that bad because the driver already failed to read the ONFI marker at offset 32 in response to the READID command and so the PARAM command wasn't issued at all :-)

Patch 1 is a robustness fix. Patch 2 is necessary to not make the controller stuck when the READPAGE command is issued before the flash chip is identified. Patch 3 is needed to be able to read more than 6 bytes in the read_byte callback. Finally patch 4 fixes the driver to allow reading out the READID ONFI marker and patch 5 implements support for the PARAM command.

Patch 6 implements a WARN that triggers when the core requests an unknown command from the driver. In the case of missing PARAM support it wouldn't have triggered because READID at offset 0x20 failed, too, but it might be worthwhile anyhow; I'm not sure though, so I marked it RFC. Not sure the set of commands will grow in the near future ...

Best regards
Uwe

Uwe Kleine-K?nig (6):
mtd: mxc-nand: Add a timeout when waiting for interrupt
mtd: mxc-nand: Only enable hardware checksumming for fully detected
flashes
mtd: mxc-nand: Do the word to byte mangling in the read_byte callback
mtd: mxc-nand: Allow to use column addresses different from 0
mtd: mxc-nand: Implement support for PARAM command
[RFC] mtd: mxc-nand: Warn on unimplemented commands

 drivers/mtd/nand/mxc_nand.c | 146 ++++++++++++++++++++++++++++----------------
 1 file changed, 93 insertions(+), 53 deletions(-)
Signature · 1 line
2.1.4
author
#1 lore raw

PATCH 1/6 mtd: mxc-nand: Add a timeout when waiting for interrupt

While extending the mxc-nand driver it happend to me a few times that the device was stuck and this made the machine hang during boot. So implement a timeout and print a stack trace the first time this happens to make it debuggable. The return type of the waiting function is also changed to int to be able to handle the timeout in the caller.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c | 47 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)
drivers/mtd/nand/mxc_nand.c +36 -11
index a8f550fec35e..27ba07c07966 100644
@@ -386,26 +386,51 @@ static irqreturn_t mxc_nfc_irq(int irq, void *dev_id)
 /* This function polls the NANDFC to wait for the basic operation to
  * complete by checking the INT bit of config2 register.
  */
-static void wait_op_done(struct mxc_nand_host *host, int useirq)
+static int wait_op_done(struct mxc_nand_host *host, int useirq)
 {
-	int max_retries = 8000;
+	int ret = 0;
+
+	/*
+	 * If operation is already complete, don't bother to setup an irq or a
+	 * loop.
+	 */
+	if (host->devtype_data->check_int(host))
+		return 0;
 
 	if (useirq) {
-		if (!host->devtype_data->check_int(host)) {
-			reinit_completion(&host->op_completion);
-			irq_control(host, 1);
-			wait_for_completion(&host->op_completion);
+		unsigned long timeout;
+
+		reinit_completion(&host->op_completion);
+
+		irq_control(host, 1);
+
+		timeout = wait_for_completion_timeout(&host->op_completion, HZ);
+		if (!timeout && !host->devtype_data->check_int(host)) {
+			dev_dbg(host->dev, "timeout waiting for irq\n");
+			ret = -ETIMEDOUT;
 		}
 	} else {
-		while (max_retries-- > 0) {
-			if (host->devtype_data->check_int(host))
-				break;
+		int max_retries = 8000;
+		int done;
 
+		do {
 			udelay(1);
+
+			done = host->devtype_data->check_int(host);
+			if (done)
+				break;
+
+		} while (--max_retries);
+
+		if (!done) {
+			dev_dbg(host->dev, "timeout polling for completion\n");
+			ret = -ETIMEDOUT;
 		}
-		if (max_retries < 0)
-			pr_debug("%s: INT not set\n", __func__);
 	}
+
+	WARN_ONCE(ret < 0, "timeout! useirq=%d\n", useirq);
+
+	return ret;
 }
 
 static void send_cmd_v3(struct mxc_nand_host *host, uint16_t cmd, int useirq)
Signature · 1 line
2.1.4
author
#2 lore raw

PATCH 2/6 mtd: mxc-nand: Only enable hardware checksumming for fully detected flashes

In reply to Uwe Kleine-König

At least on i.MX25 (i.e. NFCv2) preset_v2 is called with mtd->writesize == 0 that is before the connect flash chip is detected. It then configures for 8 bit ECC mode which needs 26 bytes of OOB per 512 bytes main section. For flashes with a smaller OOB area issuing a read page command makes the controller stuck with this config.

Note that this currently doesn't hurt because the first read page command is issued only after detection is complete and preset is called once more.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/mtd/nand/mxc_nand.c +7 -7
index 27ba07c07966..d9637ae34719 100644
@@ -928,7 +928,7 @@ static void preset_v1(struct mtd_info *mtd)
 	struct mxc_nand_host *host = nand_chip->priv;
 	uint16_t config1 = 0;
 
-	if (nand_chip->ecc.mode == NAND_ECC_HW)
+	if (nand_chip->ecc.mode == NAND_ECC_HW && mtd->writesize)
 		config1 |= NFC_V1_V2_CONFIG1_ECC_EN;
 
 	if (!host->devtype_data->irqpending_quirk)

@@ -956,9 +956,6 @@ static void preset_v2(struct mtd_info *mtd)
 	struct mxc_nand_host *host = nand_chip->priv;
 	uint16_t config1 = 0;
 
-	if (nand_chip->ecc.mode == NAND_ECC_HW)
-		config1 |= NFC_V1_V2_CONFIG1_ECC_EN;
-
 	config1 |= NFC_V2_CONFIG1_FP_INT;
 
 	if (!host->devtype_data->irqpending_quirk)

@@ -967,6 +964,9 @@ static void preset_v2(struct mtd_info *mtd)
 	if (mtd->writesize) {
 		uint16_t pages_per_block = mtd->erasesize / mtd->writesize;
 
+		if (nand_chip->ecc.mode == NAND_ECC_HW)
+			config1 |= NFC_V1_V2_CONFIG1_ECC_EN;
+
 		host->eccsize = get_eccsize(mtd);
 		if (host->eccsize == 4)
 			config1 |= NFC_V2_CONFIG1_ECC_MODE_4;

@@ -1024,9 +1024,6 @@ static void preset_v3(struct mtd_info *mtd)
 		NFC_V3_CONFIG2_INT_MSK |
 		NFC_V3_CONFIG2_NUM_ADDR_PHASE0;
 
-	if (chip->ecc.mode == NAND_ECC_HW)
-		config2 |= NFC_V3_CONFIG2_ECC_EN;
-
 	addr_phases = fls(chip->pagemask) >> 3;
 
 	if (mtd->writesize == 2048) {

@@ -1041,6 +1038,9 @@ static void preset_v3(struct mtd_info *mtd)
 	}
 
 	if (mtd->writesize) {
+		if (chip->ecc.mode == NAND_ECC_HW)
+			config2 |= NFC_V3_CONFIG2_ECC_EN;
+
 		config2 |= NFC_V3_CONFIG2_PPB(
 				ffs(mtd->erasesize / mtd->writesize) - 6,
 				host->devtype_data->ppb_shift);
Signature · 1 line
2.1.4
author
#3 lore raw

PATCH 3/6 mtd: mxc-nand: Do the word to byte mangling in the read_byte callback

In reply to Uwe Kleine-König

When the hardware operates in 16 bit mode it always reads 16 bits even for operations that only have the lower 8 bits defined. So the upper bits must be discarded. Do this in the read_byte callback instead of when reading the NAND id to support reading byte wise more than 5 bytes and at other occations (like reading the ONFI parameter page).

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)
drivers/mtd/nand/mxc_nand.c +10 -24
index d9637ae34719..aa98a0dddcfb 100644
@@ -552,30 +552,17 @@ static void send_page_v1(struct mtd_info *mtd, unsigned int ops)
 
 static void send_read_id_v3(struct mxc_nand_host *host)
 {
-	struct nand_chip *this = &host->nand;
-
 	/* Read ID into main buffer */
 	writel(NFC_ID, NFC_V3_LAUNCH);
 
 	wait_op_done(host, true);
 
 	memcpy32_fromio(host->data_buf, host->main_area0, 16);
-
-	if (this->options & NAND_BUSWIDTH_16) {
-		/* compress the ID info */
-		host->data_buf[1] = host->data_buf[2];
-		host->data_buf[2] = host->data_buf[4];
-		host->data_buf[3] = host->data_buf[6];
-		host->data_buf[4] = host->data_buf[8];
-		host->data_buf[5] = host->data_buf[10];
-	}
 }
 
 /* Request the NANDFC to perform a read of the NAND device ID. */
 static void send_read_id_v1_v2(struct mxc_nand_host *host)
 {
-	struct nand_chip *this = &host->nand;
-
 	/* NANDFC buffer 0 is used for device ID output */
 	writew(host->active_cs << 4, NFC_V1_V2_BUF_ADDR);
 

@@ -585,15 +572,6 @@ static void send_read_id_v1_v2(struct mxc_nand_host *host)
 	wait_op_done(host, true);
 
 	memcpy32_fromio(host->data_buf, host->main_area0, 16);
-
-	if (this->options & NAND_BUSWIDTH_16) {
-		/* compress the ID info */
-		host->data_buf[1] = host->data_buf[2];
-		host->data_buf[2] = host->data_buf[4];
-		host->data_buf[3] = host->data_buf[6];
-		host->data_buf[4] = host->data_buf[8];
-		host->data_buf[5] = host->data_buf[10];
-	}
 }
 
 static uint16_t get_dev_status_v3(struct mxc_nand_host *host)

@@ -719,9 +697,17 @@ static u_char mxc_nand_read_byte(struct mtd_info *mtd)
 	if (host->status_request)
 		return host->devtype_data->get_dev_status(host) & 0xFF;
 
-	ret = *(uint8_t *)(host->data_buf + host->buf_start);
-	host->buf_start++;
+	if (nand_chip->options & NAND_BUSWIDTH_16) {
+		/* only take the lower byte of each word */
+		ret = *(uint16_t *)(host->data_buf + host->buf_start);
+
+		host->buf_start += 2;
+	} else {
+		ret = *(uint8_t *)(host->data_buf + host->buf_start);
+		host->buf_start++;
+	}
 
+	pr_debug("%s: ret=0x%hhx (start=%u)\n", __func__, ret, host->buf_start);
 	return ret;
 }
Signature · 1 line
2.1.4
author
#4 lore raw

PATCH 4/6 mtd: mxc-nand: Allow to use column addresses different from 0

In reply to Uwe Kleine-König

The mxc-nand controller works pagewise and so usually only sends commands to the flash chip with column == 0. A request with column != 0 from the upper layer is then fulfilled by indexing appropriately into the device's RAM buffer.

To be able to access the ONFI marker at offset 0x20 in reply to the READID command however it's invalid to read 32 bytes starting from column 0.

So let the function used to send the address cycles send the column address actually passed instead of 0 and fix all callers to pass 0 instead appropriately. Also add some warnings in case this patch changes the drivers semantics.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)
drivers/mtd/nand/mxc_nand.c +29 -11
index aa98a0dddcfb..0afe5904a337 100644
@@ -836,6 +836,12 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)
 	}
 }
 
+/*
+ * MXC NANDFC can only perform full page+spare or spare-only read/write.  When
+ * the upper layers perform a read/write buf operation, the saved column address
+ * is used to index into the full page. So usually this function is called with
+ * column == 0 (unless no column cycle is needed indicated by column == -1)
+ */
 static void mxc_do_addr_cycle(struct mtd_info *mtd, int column, int page_addr)
 {
 	struct nand_chip *nand_chip = mtd->priv;

@@ -843,16 +849,13 @@ static void mxc_do_addr_cycle(struct mtd_info *mtd, int column, int page_addr)
 
 	/* Write out column address, if necessary */
 	if (column != -1) {
-		/*
-		 * MXC NANDFC can only perform full page+spare or
-		 * spare-only read/write.  When the upper layers
-		 * perform a read/write buf operation, the saved column
-		  * address is used to index into the full page.
-		 */
-		host->devtype_data->send_addr(host, 0, page_addr == -1);
+		host->devtype_data->send_addr(host, column & 0xff,
+					      page_addr == -1);
 		if (mtd->writesize > 512)
 			/* another col addr cycle for 2k page */
-			host->devtype_data->send_addr(host, 0, false);
+			host->devtype_data->send_addr(host,
+						      (column >> 8) & 0xff,
+						      false);
 	}
 
 	/* Write out page address, if necessary */

@@ -1077,6 +1080,9 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		host->status_request = true;
 
 		host->devtype_data->send_cmd(host, command, true);
+		WARN_ONCE(column != -1 || page_addr != -1,
+			  "Unexpected column/row value (cmd=%u, col=%d, row=%d)\n",
+			  command, column, page_addr);
 		mxc_do_addr_cycle(mtd, column, page_addr);
 		break;
 

@@ -1090,7 +1096,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		command = NAND_CMD_READ0; /* only READ0 is valid */
 
 		host->devtype_data->send_cmd(host, command, false);
-		mxc_do_addr_cycle(mtd, column, page_addr);
+		WARN_ONCE(column < 0,
+			  "Unexpected column/row value (cmd=%u, col=%d, row=%d)\n",
+			  command, column, page_addr);
+		mxc_do_addr_cycle(mtd, 0, page_addr);
 
 		if (mtd->writesize > 512)
 			host->devtype_data->send_cmd(host,

@@ -1111,7 +1120,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		host->buf_start = column;
 
 		host->devtype_data->send_cmd(host, command, false);
-		mxc_do_addr_cycle(mtd, column, page_addr);
+		WARN_ONCE(column < -1,
+			  "Unexpected column/row value (cmd=%u, col=%d, row=%d)\n",
+			  command, column, page_addr);
+		mxc_do_addr_cycle(mtd, 0, page_addr);
 		break;
 
 	case NAND_CMD_PAGEPROG:

@@ -1119,6 +1131,9 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		copy_spare(mtd, false);
 		host->devtype_data->send_page(mtd, NFC_INPUT);
 		host->devtype_data->send_cmd(host, command, true);
+		WARN_ONCE(column != -1 || page_addr != -1,
+			  "Unexpected column/row value (cmd=%u, col=%d, row=%d)\n",
+			  command, column, page_addr);
 		mxc_do_addr_cycle(mtd, column, page_addr);
 		break;
 

@@ -1126,12 +1141,15 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		host->devtype_data->send_cmd(host, command, true);
 		mxc_do_addr_cycle(mtd, column, page_addr);
 		host->devtype_data->send_read_id(host);
-		host->buf_start = column;
+		host->buf_start = 0;
 		break;
 
 	case NAND_CMD_ERASE1:
 	case NAND_CMD_ERASE2:
 		host->devtype_data->send_cmd(host, command, false);
+		WARN_ONCE(column != -1,
+			  "Unexpected column value (cmd=%u, col=%d)\n",
+			  command, column);
 		mxc_do_addr_cycle(mtd, column, page_addr);
 
 		break;
Signature · 1 line
2.1.4
author
#5 lore raw

PATCH 5/6 mtd: mxc-nand: Implement support for PARAM command

In reply to Uwe Kleine-König

The mxc-nand driver never supported the PARAM command to read out the ONFI parameter page and so always relied on probing my manufacturer and device id (as provided by the READID command).

This patch implements reading out the first parameter page copy at least which should be good enough in practise.

This makes the boot log change from

	nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xb1
	nand: Micron NAND 128MiB 1,8V 16-bit

to

	nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xb1
	nand: Micron MT29F1G16ABBDAH4

on my machine.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/mtd/nand/mxc_nand.c +7 -0
index 0afe5904a337..0083b4ee4f33 100644
@@ -1153,6 +1153,13 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		mxc_do_addr_cycle(mtd, column, page_addr);
 
 		break;
+	case NAND_CMD_PARAM:
+		host->devtype_data->send_cmd(host, command, false);
+		mxc_do_addr_cycle(mtd, column, page_addr);
+		host->devtype_data->send_page(mtd, NFC_OUTPUT);
+		memcpy32_fromio(host->data_buf, host->main_area0, 512);
+		host->buf_start = 0;
+		break;
 	}
 }
Signature · 1 line
2.1.4
author
#6 lore raw

RFC PATCH 6/6 mtd: mxc-nand: Warn on unimplemented commands

In reply to Uwe Kleine-König

The PARAM command was long unimplemented and it probably wasn't noticed because chip probing using only the few bytes returned by the READID command are good enough in most cases to determine the chip in use.

Still to notice such a shortcoming earlier in the future would be nice in case it's something more vital.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mtd/nand/mxc_nand.c +4 -0
index 0083b4ee4f33..372e0e38f59b 100644
@@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		memcpy32_fromio(host->data_buf, host->main_area0, 512);
 		host->buf_start = 0;
 		break;
+	default:
+		WARN_ONCE(1, "Unimplemented command (cmd=%u)\n",
+			  command);
+		break;
 	}
 }
Signature · 1 line
2.1.4
#7 lore raw

RFC PATCH 6/6 mtd: mxc-nand: Warn on unimplemented commands

Hi,

Quoted 18 lines · Uwe Kleine-K?nig

The PARAM command was long unimplemented and it probably wasn't noticed because chip probing using only the few bytes returned by the READID command are good enough in most cases to determine the chip in use.

Still to notice such a shortcoming earlier in the future would be nice in case it's something more vital.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mtd/nand/mxc_nand.c +0 -0
index 0083b4ee4f33..372e0e38f59b 100644

drivers/mtd/nand/mxc_nand.c static void mxc_nand_command(struct mtd_info *mtd, unsigned command, · line 1167

Quoted 8 lines
drivers/mtd/nand/mxc_nand.c +4 -0
@@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		memcpy32_fromio(host->data_buf, host->main_area0, 512);
 		host->buf_start = 0;
 		break;
+	default:
+		WARN_ONCE(1, "Unimplemented command (cmd=%u)\n",
+			  command);
+		break;
 	}

useless break;

Lothar Wa?mann

Signature · 9 lines
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
author
#8 lore raw

RFC PATCH 6/6 mtd: mxc-nand: Warn on unimplemented commands

Hello Lothar,

Quoted 6 lines · Lothar Wa?mann
Quoted 6 lines
drivers/mtd/nand/mxc_nand.c +0 -0
index 0083b4ee4f33..372e0e38f59b 100644
@@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		memcpy32_fromio(host->data_buf, host->main_area0, 512);
Quoted 8 lines

drivers/mtd/nand/mxc_nand.c static void mxc_nand_command(struct mtd_info *mtd, unsigned command, · line 1167

Quoted 7 lines
drivers/mtd/nand/mxc_nand.c +4 -0
 		host->buf_start = 0;
 		break;
+	default:
+		WARN_ONCE(1, "Unimplemented command (cmd=%u)\n",
+			  command);
+		break;
 	}

useless break;

Do you mean the line break? That's right, I fixed it here for a later v2. But I guess you mean the (literal) break here. Right, it could be dropped without change in semantic, but I thought adding it matches the usually recommended style?!

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#9 lore raw

RFC PATCH 6/6 mtd: mxc-nand: Warn on unimplemented commands

Hi,

Quoted 17 lines · Uwe Kleine-K?nig

Hello Lothar,

Quoted 6 lines · Lothar Wa?mann
Quoted 6 lines
drivers/mtd/nand/mxc_nand.c +0 -0
index 0083b4ee4f33..372e0e38f59b 100644
@@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		memcpy32_fromio(host->data_buf, host->main_area0, 512);
Quoted 8 lines

drivers/mtd/nand/mxc_nand.c static void mxc_nand_command(struct mtd_info *mtd, unsigned command, · line 1167

Quoted 7 lines
drivers/mtd/nand/mxc_nand.c +4 -0
 		host->buf_start = 0;
 		break;
+	default:
+		WARN_ONCE(1, "Unimplemented command (cmd=%u)\n",
+			  command);
+		break;
 	}

useless break;

Quoted 5 lines

Do you mean the line break? That's right, I fixed it here for a later v2. But I guess you mean the (literal) break here. Right, it could be dropped without change in semantic, but I thought adding it matches the usually recommended style?!

Documentation/CodingStyle has this example:

|        default:
|                break;
|        }

but there is no useful statement in the 'default' case, so the 'break' is necessary here.
IMO this doesn't mandate to add a 'break' at the end of the default clause if there are actual statements in this path.

Lothar Wa?mann

Signature · 9 lines
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
author
#10 lore raw

RFC PATCH 6/6 mtd: mxc-nand: Warn on unimplemented commands

Hello,

Quoted 21 lines · Lothar Wa?mann
Quoted 15 lines · Uwe Kleine-K?nig
Quoted 6 lines · Lothar Wa?mann
Quoted 6 lines
drivers/mtd/nand/mxc_nand.c +0 -0
index 0083b4ee4f33..372e0e38f59b 100644
@@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		memcpy32_fromio(host->data_buf, host->main_area0, 512);
Quoted 8 lines

drivers/mtd/nand/mxc_nand.c static void mxc_nand_command(struct mtd_info *mtd, unsigned command, · line 1167

Quoted 7 lines
drivers/mtd/nand/mxc_nand.c +4 -0
 		host->buf_start = 0;
 		break;
+	default:
+		WARN_ONCE(1, "Unimplemented command (cmd=%u)\n",
+			  command);
+		break;
 	}

useless break;

Quoted 5 lines

Do you mean the line break? That's right, I fixed it here for a later v2. But I guess you mean the (literal) break here. Right, it could be dropped without change in semantic, but I thought adding it matches the usually recommended style?!

Quoted 8 lines

Documentation/CodingStyle has this example:

|        default:
|                break;
|        }

but there is no useful statement in the 'default' case, so the 'break' is necessary here.
IMO this doesn't mandate to add a 'break' at the end of the default clause if there are actual statements in this path.

OK, we agree that Documentation/CodingStyle isn't explict about this case here. For non-default case labels I already saw review requests to add an explicit break which I consider sensible. For default it's not that important but IMHO still good style. I want to keep it.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
maintainer
#11 lore raw

RFC PATCH 6/6 mtd: mxc-nand: Warn on unimplemented commands

In reply to Lothar Waßmann
Quoted 25 lines · Lothar Wa?mann

Hi,

Quoted 17 lines · Uwe Kleine-K?nig

Hello Lothar,

Quoted 6 lines · Lothar Wa?mann
Quoted 6 lines
drivers/mtd/nand/mxc_nand.c +0 -0
index 0083b4ee4f33..372e0e38f59b 100644
@@ -1160,6 +1160,10 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 		memcpy32_fromio(host->data_buf, host->main_area0, 512);
Quoted 8 lines

drivers/mtd/nand/mxc_nand.c static void mxc_nand_command(struct mtd_info *mtd, unsigned command, · line 1167

Quoted 7 lines
drivers/mtd/nand/mxc_nand.c +4 -0
 		host->buf_start = 0;
 		break;
+	default:
+		WARN_ONCE(1, "Unimplemented command (cmd=%u)\n",
+			  command);
+		break;
 	}

useless break;

Quoted 5 lines

Do you mean the line break? That's right, I fixed it here for a later v2. But I guess you mean the (literal) break here. Right, it could be dropped without change in semantic, but I thought adding it matches the usually recommended style?!

Quoted 8 lines

Documentation/CodingStyle has this example:

|        default:
|                break;
|        }

but there is no useful statement in the 'default' case, so the 'break' is necessary here.
IMO this doesn't mandate to add a 'break' at the end of the default clause if there are actual statements in this path.

The 'default:' is not necessarily at the end. Dropping the 'break' in the last case makes it easy to forget to add the break when additional cases are added below the last one.

IMO the 'break' should stay there.

Sascha

Signature · 4 lines
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
author
#12 lore raw

cover mtd: teach mxc-nand about ONFI probing

In reply to Uwe Kleine-König

Hello,

Quoted 23 lines · Uwe Kleine-K?nig

this series implements support for ONFI probing in mxc-nand. Before a PARAM command was just ignored and the following read_bytes read whatever then happened to be available from the controller's RAM. But actually this wasn't that bad because the driver already failed to read the ONFI marker at offset 32 in response to the READID command and so the PARAM command wasn't issued at all :-)

Patch 1 is a robustness fix. Patch 2 is necessary to not make the controller stuck when the READPAGE command is issued before the flash chip is identified. Patch 3 is needed to be able to read more than 6 bytes in the read_byte callback. Finally patch 4 fixes the driver to allow reading out the READID ONFI marker and patch 5 implements support for the PARAM command.

Patch 6 implements a WARN that triggers when the core requests an unknown command from the driver. In the case of missing PARAM support it wouldn't have triggered because READID at offset 0x20 failed, too, but it might be worthwhile anyhow; I'm not sure though, so I marked it RFC. Not sure the set of commands will grow in the near future ...

Best regards
Uwe

Quoted 8 lines

Uwe Kleine-K?nig (6):
mtd: mxc-nand: Add a timeout when waiting for interrupt
mtd: mxc-nand: Only enable hardware checksumming for fully detected
flashes
mtd: mxc-nand: Do the word to byte mangling in the read_byte callback
mtd: mxc-nand: Allow to use column addresses different from 0
mtd: mxc-nand: Implement support for PARAM command
[RFC] mtd: mxc-nand: Warn on unimplemented commands

I think this series if fine to go into 4.1-rc1. If so it would be great to get it into next in the near future. David? Brian? Any feedback?

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#13 lore raw

cover mtd: teach mxc-nand about ONFI probing

In reply to Uwe Kleine-König
Quoted 16 lines · Uwe Kleine-K?nig

Hello,

this series implements support for ONFI probing in mxc-nand. Before a PARAM command was just ignored and the following read_bytes read whatever then happened to be available from the controller's RAM. But actually this wasn't that bad because the driver already failed to read the ONFI marker at offset 32 in response to the READID command and so the PARAM command wasn't issued at all :-)

Patch 1 is a robustness fix. Patch 2 is necessary to not make the controller stuck when the READPAGE command is issued before the flash chip is identified. Patch 3 is needed to be able to read more than 6 bytes in the read_byte callback. Finally patch 4 fixes the driver to allow reading out the READID ONFI marker and patch 5 implements support for the PARAM command.

Quoted 8 lines

Patch 6 implements a WARN that triggers when the core requests an unknown command from the driver. In the case of missing PARAM support it wouldn't have triggered because READID at offset 0x20 failed, too, but it might be worthwhile anyhow; I'm not sure though, so I marked it RFC. Not sure the set of commands will grow in the near future ...

Best regards
Uwe

Looks OK to me. Pushed all to l2-mtd.git.

Brian