[PATCH 0/6] mtd: teach mxc-nand about ONFI probing

[PATCH 0/6] mtd: teach mxc-nand about ONFI probing

[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.

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(-)

-- 
2.1.4

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

[Uwe Kleine-König]

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 <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 47 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index a8f550fec35e..27ba07c07966 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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)
-- 
2.1.4

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

[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 <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 27ba07c07966..d9637ae34719 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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);
-- 
2.1.4

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

[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 <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index d9637ae34719..aa98a0dddcfb 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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;
 }
 
-- 
2.1.4

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

[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 <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index aa98a0dddcfb..0afe5904a337 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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;
-- 
2.1.4

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

[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 <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 0afe5904a337..0083b4ee4f33 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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;
 	}
 }
 
-- 
2.1.4

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

[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 <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 0083b4ee4f33..372e0e38f59b 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -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;
 	}
 }
 
-- 
2.1.4

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

[Lothar Waßmann]

Hi,

Uwe Kleine-K?nig wrote:
> 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 <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 0083b4ee4f33..372e0e38f59b 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -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
-- 
___________________________________________________________

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
___________________________________________________________

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

[Uwe Kleine-König]

Hello Lothar,

On Wed, Feb 11, 2015 at 09:42:56AM +0100, Lothar Wa?mann wrote:
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index 0083b4ee4f33..372e0e38f59b 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -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;
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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Lothar Waßmann]

Hi,

Uwe Kleine-K?nig wrote:
> Hello Lothar,
> 
> On Wed, Feb 11, 2015 at 09:42:56AM +0100, Lothar Wa?mann wrote:
> > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > > index 0083b4ee4f33..372e0e38f59b 100644
> > > --- a/drivers/mtd/nand/mxc_nand.c
> > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > @@ -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;
> 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
-- 
___________________________________________________________

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
___________________________________________________________

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

[Uwe Kleine-König]

Hello,

On Wed, Feb 11, 2015 at 10:40:16AM +0100, Lothar Wa?mann wrote:
> Uwe Kleine-K?nig wrote:
> > On Wed, Feb 11, 2015 at 09:42:56AM +0100, Lothar Wa?mann wrote:
> > > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > > > index 0083b4ee4f33..372e0e38f59b 100644
> > > > --- a/drivers/mtd/nand/mxc_nand.c
> > > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > > @@ -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;
> > 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.
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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Sascha Hauer]

On Wed, Feb 11, 2015 at 10:40:16AM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Uwe Kleine-K?nig wrote:
> > Hello Lothar,
> > 
> > On Wed, Feb 11, 2015 at 09:42:56AM +0100, Lothar Wa?mann wrote:
> > > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > > > index 0083b4ee4f33..372e0e38f59b 100644
> > > > --- a/drivers/mtd/nand/mxc_nand.c
> > > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > > @@ -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;
> > 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.

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

-- 
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 |

[PATCH 0/6] mtd: teach mxc-nand about ONFI probing

[Uwe Kleine-König]

Hello,

On Tue, Feb 10, 2015 at 07:59:54PM +0100, Uwe Kleine-K?nig wrote:
> 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
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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 0/6] mtd: teach mxc-nand about ONFI probing

[Brian Norris]

On Tue, Feb 10, 2015 at 07:59:54PM +0100, Uwe Kleine-K?nig wrote:
> 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

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

Brian