[PATCH 4/5] mtd: mxc_nand fixups

[PATCH 4/5] mtd: mxc_nand fixups

[John Ogness]

This patch reverts the driver to enabling/disabling the NFC interrupt
mask rather than enabling/disabling the system interrupt. This cleans
up the driver so that it doesn't rely on interrupts being disabled
within the interrupt handler.

The patch is against linux-next 20100618.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/mxc_nand.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: linux-next-20100618/drivers/mtd/nand/mxc_nand.c
===================================================================
--- linux-next-20100618.orig/drivers/mtd/nand/mxc_nand.c
+++ linux-next-20100618/drivers/mtd/nand/mxc_nand.c
@@ -173,8 +173,11 @@ static const char *part_probes[] = { "Re
 static irqreturn_t mxc_nfc_irq(int irq, void *dev_id)
 {
 	struct mxc_nand_host *host = dev_id;
+	uint16_t tmp;
 
-	disable_irq_nosync(irq);
+	tmp = readw(host->regs + NFC_CONFIG1);
+	tmp |= NFC_INT_MSK; /* disable NFC interrupts */
+	writew(tmp, host->regs + NFC_CONFIG1);
 
 	wake_up(&host->irq_waitq);
 
@@ -192,7 +195,9 @@ static void wait_op_done(struct mxc_nand
 	if (useirq) {
 		if ((readw(host->regs + NFC_CONFIG2) & NFC_INT) == 0) {
 
-			enable_irq(host->irq);
+			tmp = readw(host->regs + NFC_CONFIG1);
+			tmp  &= ~NFC_INT_MSK;   /* enable NFC interrupts */
+			writew(tmp, host->regs + NFC_CONFIG1);
 
 			wait_event(host->irq_waitq,
 				readw(host->regs + NFC_CONFIG2) & NFC_INT);
@@ -846,7 +851,7 @@ static int __init mxcnd_probe(struct pla
 
 	host->irq = platform_get_irq(pdev, 0);
 
-	err = request_irq(host->irq, mxc_nfc_irq, IRQF_DISABLED, DRIVER_NAME, host);
+	err = request_irq(host->irq, mxc_nfc_irq, 0, DRIVER_NAME, host);
 	if (err)
 		goto eirq;
 

Re: [PATCH 4/5] mtd: mxc_nand fixups

[Sascha Hauer]

[added Ivo to Cc]

On Fri, Jun 18, 2010 at 07:01:34PM +0200, John Ogness wrote:
> This patch reverts the driver to enabling/disabling the NFC interrupt
> mask rather than enabling/disabling the system interrupt. This cleans
> up the driver so that it doesn't rely on interrupts being disabled
> within the interrupt handler.

This behaviour was introduced in
a47bfd2eb66837653dc3b42541dfe4283dd41251 mxc_nand: support i.MX21

I guess this won't work on i.MX21.

Sascha

> 
> The patch is against linux-next 20100618.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> Index: linux-next-20100618/drivers/mtd/nand/mxc_nand.c
> ===================================================================
> --- linux-next-20100618.orig/drivers/mtd/nand/mxc_nand.c
> +++ linux-next-20100618/drivers/mtd/nand/mxc_nand.c
> @@ -173,8 +173,11 @@ static const char *part_probes[] = { "Re
>  static irqreturn_t mxc_nfc_irq(int irq, void *dev_id)
>  {
>  	struct mxc_nand_host *host = dev_id;
> +	uint16_t tmp;
>  
> -	disable_irq_nosync(irq);
> +	tmp = readw(host->regs + NFC_CONFIG1);
> +	tmp |= NFC_INT_MSK; /* disable NFC interrupts */
> +	writew(tmp, host->regs + NFC_CONFIG1);
>  
>  	wake_up(&host->irq_waitq);
>  
> @@ -192,7 +195,9 @@ static void wait_op_done(struct mxc_nand
>  	if (useirq) {
>  		if ((readw(host->regs + NFC_CONFIG2) & NFC_INT) == 0) {
>  
> -			enable_irq(host->irq);
> +			tmp = readw(host->regs + NFC_CONFIG1);
> +			tmp  &= ~NFC_INT_MSK;   /* enable NFC interrupts */
> +			writew(tmp, host->regs + NFC_CONFIG1);
>  
>  			wait_event(host->irq_waitq,
>  				readw(host->regs + NFC_CONFIG2) & NFC_INT);
> @@ -846,7 +851,7 @@ static int __init mxcnd_probe(struct pla
>  
>  	host->irq = platform_get_irq(pdev, 0);
>  
> -	err = request_irq(host->irq, mxc_nfc_irq, IRQF_DISABLED, DRIVER_NAME, host);
> +	err = request_irq(host->irq, mxc_nfc_irq, 0, DRIVER_NAME, host);
>  	if (err)
>  		goto eirq;
>  
> 

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

Re: [PATCH 4/5] mtd: mxc_nand fixups

[John Ogness]

On 2010-06-18, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> This patch reverts the driver to enabling/disabling the NFC interrupt
>> mask rather than enabling/disabling the system interrupt. This cleans
>> up the driver so that it doesn't rely on interrupts being disabled
>> within the interrupt handler.
>
> This behaviour was introduced in
> a47bfd2eb66837653dc3b42541dfe4283dd41251 mxc_nand: support i.MX21
>
> I guess this won't work on i.MX21.

I just downloaded the i.MX21 reference manual and its NFC interrupts can
also be masked with INT_MASK (bit 4) of NFC_Flash_Config1 (0xdf003e1a).

IMHO we should revert the driver to masking the NFC interrupts. The
current linux-next version with nosync irq disabling within the
interrupt handler is a bit silly. Especially since the interrupt handler
does nothing except signal a wait queue.

John Ogness

[PATCHv2 4/5] mtd: mxc_nand fixups

[John Ogness]

On 2010-06-20, Ivo Clarysse <ivo.clarysse@gmail.com> wrote:
> Yes, on i.MX21, NFC interrupts can be masked using
> NFC_CONFIG1:NFC_INT_MSK.  But I observed that as long as NFC_INT_MSK
> is set, NFC_CONFIG2:NFC_INT will always read out 0 on i.MX21, even if
> the operation is completed.
>
> So the driver will remain stuck at:
>
>                  wait_event(host->irq_waitq,
>                          readw(host->regs + NFC_CONFIG2) & NFC_INT);

OK. Here is alternative patch. Do you have access to an i.MX21 to test
this on?

This patch fixes the driver so that the irq is requested as disabled. This
prevents double irq enabling and also does not require the interrupt to
be disabled within the interrupt handler. (Actually, I beleive this was
the true intention of the original author.)

The patch is against linux-next 20100618.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/mxc_nand.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-next-20100618/drivers/mtd/nand/mxc_nand.c
===================================================================
--- linux-next-20100618.orig/drivers/mtd/nand/mxc_nand.c
+++ linux-next-20100618/drivers/mtd/nand/mxc_nand.c
@@ -30,6 +30,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/irq.h>
 
 #include <asm/mach/flash.h>
 #include <mach/mxc_nand.h>
@@ -846,7 +847,9 @@ static int __init mxcnd_probe(struct pla
 
 	host->irq = platform_get_irq(pdev, 0);
 
-	err = request_irq(host->irq, mxc_nfc_irq, IRQF_DISABLED, DRIVER_NAME, host);
+	/* request irq as disabled */
+	err = request_irq(host->irq, mxc_nfc_irq, IRQF_NOAUTOEN,
+			  DRIVER_NAME, host);
 	if (err)
 		goto eirq;
 

Re: [PATCHv2 4/5] mtd: mxc_nand fixups

[Ivo Clarysse]

On Sun, Jun 20, 2010 at 11:21 AM, John Ogness <john.ogness@linutronix.de> wrote:
>
> On 2010-06-20, Ivo Clarysse <ivo.clarysse@gmail.com> wrote:
> > Yes, on i.MX21, NFC interrupts can be masked using
> > NFC_CONFIG1:NFC_INT_MSK.  But I observed that as long as NFC_INT_MSK
> > is set, NFC_CONFIG2:NFC_INT will always read out 0 on i.MX21, even if
> > the operation is completed.
> >
> > So the driver will remain stuck at:
> >
> >                  wait_event(host->irq_waitq,
> >                          readw(host->regs + NFC_CONFIG2) & NFC_INT);
>
> OK. Here is alternative patch. Do you have access to an i.MX21 to test
> this on?
>
> This patch fixes the driver so that the irq is requested as disabled. This
> prevents double irq enabling and also does not require the interrupt to
> be disabled within the interrupt handler. (Actually, I beleive this was
> the true intention of the original author.)
[...]

Yes, that was indeed my true intention :)

I tested this change on an MX21ADS board.

Acked-by: Ivo Clarysse <ivo.clarysse@gmail.com>

[PATCHv3 4/5] mtd: mxc_nand fixups

[John Ogness]

The v2 version of this patch has 2 problems. I believe that this new
version addresses the problems and still respects the strange behavior
of the i.MX21.

Problem 1: The v2 patch passes IRQF_NOAUTOEN to request_irq(), but that
flag is not evaluated by request_irq(). That flag is only evaluated in
set_irq_flags(), which is now used by this patch.

Problem 2: We are still having problems with irq's possibly being
enabled twice when threaded interrupts are used. This happens because
the main thread that enables the irq can proceed even if the interrupt
hasn't fired yet (NFC_INT is set, but the irq thread hasn't processed it
yet). In this case it is possible for the main thread to enable the irq
again with a later call to wait_op_done().

This patch addresses the problem by allowing the function that enabled
the irq to also be responsible for disabling the irq. This obviously
avoids and double enabling.

In order to prevent interrupt flooding, the interrupt handler will mask
the interrupt. After the main thread has disabled the irq, it will
unmask the interrupt. This should allow the i.MX21 to work correctly
despite its masking behavior.

And finally, this patch correctly clears the interrupt bit each
time. Previously there was a case where the interrupt bit was not being
cleared.

The patch is against linux-next 20100618.

The patch is against linux-next 20100618.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/mxc_nand.c |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Index: linux-next-20100618/drivers/mtd/nand/mxc_nand.c
===================================================================
--- linux-next-20100618.orig/drivers/mtd/nand/mxc_nand.c
+++ linux-next-20100618/drivers/mtd/nand/mxc_nand.c
@@ -30,6 +30,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/irq.h>
 
 #include <asm/mach/flash.h>
 #include <mach/mxc_nand.h>
@@ -173,8 +174,12 @@ static const char *part_probes[] = { "Re
 static irqreturn_t mxc_nfc_irq(int irq, void *dev_id)
 {
 	struct mxc_nand_host *host = dev_id;
+	uint16_t tmp;
 
-	disable_irq_nosync(irq);
+	/* mask interrupts */
+	tmp = readw(host->regs + NFC_CONFIG1);
+	tmp |= NFC_INT_MSK;
+	writew(tmp, host->regs + NFC_CONFIG1);
 
 	wake_up(&host->irq_waitq);
 
@@ -197,10 +202,18 @@ static void wait_op_done(struct mxc_nand
 			wait_event(host->irq_waitq,
 				readw(host->regs + NFC_CONFIG2) & NFC_INT);
 
-			tmp = readw(host->regs + NFC_CONFIG2);
-			tmp  &= ~NFC_INT;
-			writew(tmp, host->regs + NFC_CONFIG2);
+			disable_irq(host->irq);
+
+			/* unmask interrupts */
+			tmp = readw(host->regs + NFC_CONFIG1);
+			tmp &= ~NFC_INT_MSK;
+			writew(tmp, host->regs + NFC_CONFIG1);
 		}
+
+		/* clear interrupt flag */
+		tmp = readw(host->regs + NFC_CONFIG2);
+		tmp &= ~NFC_INT;
+		writew(tmp, host->regs + NFC_CONFIG2);
 	} else {
 		while (max_retries-- > 0) {
 			if (readw(host->regs + NFC_CONFIG2) & NFC_INT) {
@@ -846,7 +859,9 @@ static int __init mxcnd_probe(struct pla
 
 	host->irq = platform_get_irq(pdev, 0);
 
-	err = request_irq(host->irq, mxc_nfc_irq, IRQF_DISABLED, DRIVER_NAME, host);
+	/* request irq as disabled */
+	set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);
+	err = request_irq(host->irq, mxc_nfc_irq, 0, DRIVER_NAME, host);
 	if (err)
 		goto eirq;
 

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[Ivo Clarysse]

On Tue, Jun 22, 2010 at 5:54 PM, John Ogness <john.ogness@linutronix.de> wrote:
> The v2 version of this patch has 2 problems. I believe that this new
> version addresses the problems and still respects the strange behavior
> of the i.MX21.

Unfortunately, this breaks i.MX21 support.

With this patch, the driver gets stuck at the first send_addr() invocation:


barebox 2010.06.0-00202-gc2db375-dirty (Jun 21 2010 - 11:06:13)

Board: Freescale i.MX21 ADS
cfi_probe: cfi_flash base: 0xc8000000 size: 0x02000000
NAND device: Manufacturer ID: 0xec, Chip ID: 0x36 (Samsung NAND 64MiB
1,8V 8-bit)
Scanning device for bad blocks
Bad eraseblock 2133 at 0x02154000
CS8900A Rev. F (PID: 0x0a00) found at 0xcc000000
got MAC address from EEPROM: 00:04:9F:00:25:5C
Malloc space: 0xc0c00000 -> 0xc1000000 (size  4 MB)
Stack space : 0xc0bf8000 -> 0xc0c00000 (size 32 kB)
Open /dev/env0 No such file or directory
no valid environment found on /dev/env0. Using default environment
running /env/bin/init...
barebox:/ nand_parts="256k(barebox)ro,128k(bareboxenv),2176k(kernel),-(root)"
barebox:/ addpart /dev/nand0 $nand_parts
barebox:/ nand -a /dev/nand0.kernel
barebox:/ bootargs="console=ttymxc0,115200"
barebox:/ bootz /dev/nand0.kernel.bb
loaded zImage from /dev/nand0.kernel.bb with size 1696652
Uncompressing Linux... done, booting the kernel.
Linux version 2.6.35-rc3 (ycl@maestro-ycl-quad) (gcc version 4.4.1
(Sourcery G++ Lite 2010q1-202) ) #6 PREEMPT Wed Jun 23 09:11:46 CEST
2010
CPU: ARM926EJ-S [41069264] revision 4 (ARMv5TEJ), cr=00053177
CPU: VIVT data cache, VIVT instruction cache
Machine: Freescale i.MX21ADS
Memory policy: ECC disabled, Data cache writeback
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 16256
Kernel command line: console=ttymxc0,115200
PID hash table entries: 256 (order: -2, 1024 bytes)
Dentry cache hash table entries: 8192 (order: 3, 32768 bytes)
Inode-cache hash table entries: 4096 (order: 2, 16384 bytes)
Memory: 64MB = 64MB total
Memory: 61776k/61776k available, 3760k reserved, 0K highmem
Virtual kernel memory layout:
    vector  : 0xffff0000 - 0xffff1000   (   4 kB)
    fixmap  : 0xfff00000 - 0xfffe0000   ( 896 kB)
    DMA     : 0xffc00000 - 0xffe00000   (   2 MB)
    vmalloc : 0xc4800000 - 0xf4000000   ( 760 MB)
    lowmem  : 0xc0000000 - 0xc4000000   (  64 MB)
    modules : 0xbf000000 - 0xc0000000   (  16 MB)
      .init : 0xc0008000 - 0xc0021000   ( 100 kB)
      .text : 0xc0021000 - 0xc02dc000   (2796 kB)
      .data : 0xc02ee000 - 0xc0309160   ( 109 kB)
Hierarchical RCU implementation.
	RCU-based detection of stalled CPUs is disabled.
	Verbose stalled-CPUs detection is disabled.
NR_IRQS:272
MXC GPIO hardware
MXC IRQ initialized
Console: colour dummy device 80x30
Calibrating delay loop... 131.89 BogoMIPS (lpj=659456)
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 512
CPU: Testing write buffer coherency: ok
NET: Registered protocol family 16
bio: create slab <bio-0> at 0
Switching to clocksource mxc_timer1
NET: Registered protocol family 2
IP route cache hash table entries: 1024 (order: 0, 4096 bytes)
TCP established hash table entries: 2048 (order: 2, 16384 bytes)
TCP bind hash table entries: 2048 (order: 1, 8192 bytes)
TCP: Hash tables configured (established 2048 bind 2048)
TCP reno registered
UDP hash table entries: 256 (order: 0, 4096 bytes)
UDP-Lite hash table entries: 256 (order: 0, 4096 bytes)
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
RPC: Registered tcp NFSv4.1 backchannel transport module.
JFFS2 version 2.2. (NAND) © 2001-2006 Red Hat, Inc.
msgmni has been set to 120
io scheduler noop registered (default)
imx-fb imx-fb.0: i.MX Framebuffer driver
Console: switching to colour frame buffer device 30x40
Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled
Serial: IMX driver
imx-uart.0: ttymxc0 at MMIO 0x1000a000 (irq = 20) is a IMX
console [ttymxc0] enabled
imx-uart.2: ttymxc2 at MMIO 0x1000c000 (irq = 18) is a IMX
imx-uart.3: ttymxc3 at MMIO 0x1000d000 (irq = 17) is a IMX
physmap platform flash device: 02000000 at c8000000
physmap-flash.0: Found 2 x16 devices at 0x0 in 32-bit bank
Amd/Fujitsu Extended Query Table at 0x0040
physmap-flash.0: CFI contains unrecognised boot bank location (1).
Assuming bottom.
number of CFI chips: 1
Searching for RedBoot partition table in physmap-flash.0 at offset 0x1fe0000
No RedBoot partition table detected in physmap-flash.0
mtd: Giving out device 0 to physmap-flash.0
mxc_nand_command (cmd = 0xff, col = 0xffffffff, page = 0xffffffff)
send_cmd(host, 0xff, 0)
mxc_nand_command (cmd = 0x90, col = 0x0, page = 0xffffffff)
send_cmd(host, 0x90, 1)
send_addr(host, 0x0 1)


Setting NFC_CONFIG1:NFC_INT_MSK in the interrupt handler causes
NFC_CONFIG2:NFC_INT to always read out 0 (on i.MX21), causing

           wait_event(host->irq_waitq,
                        readw(host->regs + NFC_CONFIG2) & NFC_INT);

to wait forever.

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[John Ogness]

On 2010-06-23, Ivo Clarysse <ivo.clarysse@gmail.com> wrote:
> Unfortunately, this breaks i.MX21 support.
>
> With this patch, the driver gets stuck at the first send_addr()
> invocation:
>
> [...]
>
> Setting NFC_CONFIG1:NFC_INT_MSK in the interrupt handler causes
> NFC_CONFIG2:NFC_INT to always read out 0 (on i.MX21), causing
>
>     wait_event(host->irq_waitq,
>                readw(host->regs + NFC_CONFIG2) & NFC_INT);
>
> to wait forever.

OK. Now I understand the problem. Here is a new patch that introduces a
flag that is set by the interrupt handler. This way we do not rely on
the i.MX21 being able to read NFC_INT when the interrupt is masked.

I have also added a lot of comments so that it is (hopefully) clear why
we are enabling and disabling the irq line rather than only masking and
unmasking the interrupt.

The patch is against linux-next 20100618.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/mxc_nand.c |   49 +++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 7 deletions(-)

Index: linux-next-20100618/drivers/mtd/nand/mxc_nand.c
===================================================================
--- linux-next-20100618.orig/drivers/mtd/nand/mxc_nand.c
+++ linux-next-20100618/drivers/mtd/nand/mxc_nand.c
@@ -30,6 +30,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/irq.h>
 
 #include <asm/mach/flash.h>
 #include <mach/mxc_nand.h>
@@ -117,6 +118,7 @@ struct mxc_nand_host {
 	int			clk_act;
 	int			irq;
 
+	int			nfc_int;
 	wait_queue_head_t	irq_waitq;
 
 	uint8_t			*data_buf;
@@ -173,8 +175,21 @@ static const char *part_probes[] = { "Re
 static irqreturn_t mxc_nfc_irq(int irq, void *dev_id)
 {
 	struct mxc_nand_host *host = dev_id;
+	uint16_t tmp;
+
+	/* If NFC_INT is not set, we have a spurious interrupt! */
+	if ((readw(host->regs + NFC_CONFIG2) & NFC_INT) == 0)
+		return IRQ_NONE;
+
+	/* We use the host->nfc_int flag because the i.MX21 cannot
+	 * read the CONFIG2:NFC_INT bit if interrupts are masked. */
+	host->nfc_int = 1;
 
-	disable_irq_nosync(irq);
+	/* Mask the interrupts so that we avoid an interrupt
+	 * flood until the irq line is disabled. */
+	tmp = readw(host->regs + NFC_CONFIG1);
+	tmp |= NFC_INT_MSK;
+	writew(tmp, host->regs + NFC_CONFIG1);
 
 	wake_up(&host->irq_waitq);
 
@@ -192,15 +207,33 @@ static void wait_op_done(struct mxc_nand
 	if (useirq) {
 		if ((readw(host->regs + NFC_CONFIG2) & NFC_INT) == 0) {
 
+			/* Clear the interrupt flag. It will be set
+			 * by the interrupt handler. */
+			host->nfc_int = 0;
+
+			/* We enable the irq line and wait for an interrupt.
+			 * We must use irq line enable/disable rather than
+			 * simply masking of the interrupt because the i.MX21
+			 * cannot read the CONFIG2:NFC_INT bit if the
+			 * interrupt is masked. This is also the reason we
+			 * use the host->nfc_int flag. */
 			enable_irq(host->irq);
 
-			wait_event(host->irq_waitq,
-				readw(host->regs + NFC_CONFIG2) & NFC_INT);
+			wait_event(host->irq_waitq, host->nfc_int);
+
+			disable_irq(host->irq);
 
-			tmp = readw(host->regs + NFC_CONFIG2);
-			tmp  &= ~NFC_INT;
-			writew(tmp, host->regs + NFC_CONFIG2);
+			/* The irq line has been disabled. We can now
+			 * unmask the interrupts. */
+			tmp = readw(host->regs + NFC_CONFIG1);
+			tmp &= ~NFC_INT_MSK;
+			writew(tmp, host->regs + NFC_CONFIG1);
 		}
+
+		/* clear interrupt flag */
+		tmp = readw(host->regs + NFC_CONFIG2);
+		tmp &= ~NFC_INT;
+		writew(tmp, host->regs + NFC_CONFIG2);
 	} else {
 		while (max_retries-- > 0) {
 			if (readw(host->regs + NFC_CONFIG2) & NFC_INT) {
@@ -846,7 +879,9 @@ static int __init mxcnd_probe(struct pla
 
 	host->irq = platform_get_irq(pdev, 0);
 
-	err = request_irq(host->irq, mxc_nfc_irq, IRQF_DISABLED, DRIVER_NAME, host);
+	/* request irq as disabled */
+	set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);
+	err = request_irq(host->irq, mxc_nfc_irq, 0, DRIVER_NAME, host);
 	if (err)
 		goto eirq;
 

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[Ivo Clarysse]

On Wed, Jun 23, 2010 at 10:48 AM, John Ogness <john.ogness@linutronix.de> wrote:
[...]
>
> OK. Now I understand the problem. Here is a new patch that introduces a
> flag that is set by the interrupt handler. This way we do not rely on
> the i.MX21 being able to read NFC_INT when the interrupt is masked.

Yes, this works on i.MX21 (tested on an MX21ADS board).

[...]
> @@ -117,6 +118,7 @@ struct mxc_nand_host {
>        int                     clk_act;
>        int                     irq;
>
> +       int                     nfc_int;

But is it OK to use a regular (non-volatile) variable to communicate
between interrupt context and the non-interrupt context ?

My original patch for i.MX21 used completions instead:

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-April/012694.html

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[John Ogness]

On 2010-06-23, Ivo Clarysse <ivo.clarysse@gmail.com> wrote:
> But is it OK to use a regular (non-volatile) variable to communicate
> between interrupt context and the non-interrupt context ?

In this case, yes.

> My original patch for i.MX21 used completions instead:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-April/012694.html

Ah. It seems you've been through all this before. I wish I had noticed
that thread before. I will need to check more carefully in the future.

Yes, your original patch achieves the exact same thing. Whether we use
wait_event() with a flag or wait_completion() really is the same
thing. So I guess Sascha can decide what we should do there.

What I like about your original patch is that only the i.MX21 has the
cost of constantly enabling/disabling the irq line. It adds 5
cpu_is_mx21() blocks to the code, but will lead to less work for the CPU
on non-i.MX21 boards.

John Ogness

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[Sascha Hauer]

On Wed, Jun 23, 2010 at 12:10:08PM +0200, John Ogness wrote:
> On 2010-06-23, Ivo Clarysse <ivo.clarysse@gmail.com> wrote:
> > But is it OK to use a regular (non-volatile) variable to communicate
> > between interrupt context and the non-interrupt context ?
> 
> In this case, yes.
> 
> > My original patch for i.MX21 used completions instead:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-April/012694.html
> 
> Ah. It seems you've been through all this before. I wish I had noticed
> that thread before. I will need to check more carefully in the future.
> 
> Yes, your original patch achieves the exact same thing. Whether we use
> wait_event() with a flag or wait_completion() really is the same
> thing. So I guess Sascha can decide what we should do there.
> 
> What I like about your original patch is that only the i.MX21 has the
> cost of constantly enabling/disabling the irq line. It adds 5
> cpu_is_mx21() blocks to the code, but will lead to less work for the CPU
> on non-i.MX21 boards.

Ok, if it's the only way out to have 5 cpu_is_* blocks, then lets go for
it.

BTW I observed that at least on i.MX27 the latencies introduced by
waiting for an interrupt cause a significant performance drop. The
driver gets much faster when we just poll all the time. I don't know how
this affects system performance otherwise, but it may be a possibility
to drop interrupt support at least for i.MX21. I have no idea how long
the longest possible time we'd have to poll is though.

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 |

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[John Ogness]

On 2010-06-24, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Ok, if it's the only way out to have 5 cpu_is_* blocks, then lets go
> for it.

Here is a new patch that puts the behavior behind a "nfc_avoid_masking"
macro. The macro is only used 3 times.

I also changed the code to use a completion, since that's exactly what
we are doing there anyway.

I also added some other macros to simplify reading of the code and
reduce possible copy/paste errors relating to masking and interrupt
handling.

> BTW I observed that at least on i.MX27 the latencies introduced by
> waiting for an interrupt cause a significant performance drop. The
> driver gets much faster when we just poll all the time. I don't know
> how this affects system performance otherwise, but it may be a
> possibility to drop interrupt support at least for i.MX21. I have no
> idea how long the longest possible time we'd have to poll is though.

I don't recommend moving to pure polling. Although it may be faster for
NAND-only performance tests, I expect it would have a dramatic affect on
the rest of the system during many NAND reads/writes.


This patch is based on linux-next 20100618.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/mxc_nand.c |   81 +++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 20 deletions(-)

Index: linux-next-20100618/drivers/mtd/nand/mxc_nand.c
===================================================================
--- linux-next-20100618.orig/drivers/mtd/nand/mxc_nand.c
+++ linux-next-20100618/drivers/mtd/nand/mxc_nand.c
@@ -30,6 +30,8 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/completion.h>
 
 #include <asm/mach/flash.h>
 #include <mach/mxc_nand.h>
@@ -40,6 +42,13 @@
 #define nfc_is_v21()		(cpu_is_mx25() || cpu_is_mx35())
 #define nfc_is_v1()		(cpu_is_mx31() || cpu_is_mx27() || cpu_is_mx21())
 
+/* It has been observeed that the i.MX21 cannot read the CONFIG2:INT bit
+ * if interrupts are masked (CONFIG1:INT_MSK is set). To handle this, the
+ * driver can enable/disable the irq line rather than simply masking the
+ * interrupts. The nfc_avoid_masking() macro identifies the systems that
+ * should use this workaround. */
+#define nfc_avoid_masking()	(cpu_is_mx21())
+
 /* Addresses for NFC registers */
 #define NFC_BUF_SIZE		0xE00
 #define NFC_BUF_ADDR		0xE04
@@ -100,6 +109,18 @@
 
 #define NFC_RSLTSPARE_AREA_MASK  0xff
 
+#define nfc_interrupt_set(_regs) \
+		(readw(_regs + NFC_CONFIG2) & NFC_INT)
+#define nfc_clear_interrupt(_regs) \
+		writew(readw(_regs + NFC_CONFIG2) & ~NFC_INT, \
+		       _regs + NFC_CONFIG2)
+#define nfc_mask_irq(_regs) \
+		writew(readw(_regs + NFC_CONFIG1) | NFC_INT_MSK, \
+		       _regs + NFC_CONFIG1)
+#define nfc_unmask_irq(_regs) \
+		writew(readw(_regs + NFC_CONFIG1) & ~NFC_INT_MSK, \
+		       _regs + NFC_CONFIG1)
+
 struct mxc_nand_host {
 	struct mtd_info		mtd;
 	struct nand_chip	nand;
@@ -117,7 +138,7 @@ struct mxc_nand_host {
 	int			clk_act;
 	int			irq;
 
-	wait_queue_head_t	irq_waitq;
+	struct completion	op_completion;
 
 	uint8_t			*data_buf;
 	unsigned int		buf_start;
@@ -174,9 +195,18 @@ static irqreturn_t mxc_nfc_irq(int irq, 
 {
 	struct mxc_nand_host *host = dev_id;
 
-	disable_irq_nosync(irq);
-
-	wake_up(&host->irq_waitq);
+	/* If NFC_INT is not set, we have a spurious interrupt! */
+	if (!nfc_interrupt_set(host->regs))
+		return IRQ_NONE;
+
+	/* Even with nfc_avoid_masking() we mask the interrupt
+	 * here to avoid an interrupt flood until the irq line
+	 * is disabled by the driver thread. */
+	nfc_mask_irq(host->regs);
+
+	/* Notify the driver thread that an interrupt has occurred.
+	 * The driver thread will clear the interrupt. */
+	complete(&host->op_completion);
 
 	return IRQ_HANDLED;
 }
@@ -186,27 +216,32 @@ static irqreturn_t mxc_nfc_irq(int irq, 
  */
 static void wait_op_done(struct mxc_nand_host *host, int useirq)
 {
-	uint16_t tmp;
 	int max_retries = 8000;
 
 	if (useirq) {
-		if ((readw(host->regs + NFC_CONFIG2) & NFC_INT) == 0) {
+		if (!nfc_interrupt_set(host->regs)) {
+
+			INIT_COMPLETION(host->op_completion);
 
-			enable_irq(host->irq);
+			if (nfc_avoid_masking())
+				enable_irq(host->irq);
+			else
+				nfc_unmask_irq(host->regs);
 
-			wait_event(host->irq_waitq,
-				readw(host->regs + NFC_CONFIG2) & NFC_INT);
+			/* wait for the interrupt */
+			wait_for_completion(&host->op_completion);
 
-			tmp = readw(host->regs + NFC_CONFIG2);
-			tmp  &= ~NFC_INT;
-			writew(tmp, host->regs + NFC_CONFIG2);
+			if (nfc_avoid_masking()) {
+				disable_irq(host->irq);
+				nfc_unmask_irq(host->regs);
+			}
 		}
+
+		nfc_clear_interrupt(host->regs);
 	} else {
 		while (max_retries-- > 0) {
-			if (readw(host->regs + NFC_CONFIG2) & NFC_INT) {
-				tmp = readw(host->regs + NFC_CONFIG2);
-				tmp  &= ~NFC_INT;
-				writew(tmp, host->regs + NFC_CONFIG2);
+			if (nfc_interrupt_set(host->regs)) {
+				nfc_clear_interrupt(host->regs);
 				break;
 			}
 			udelay(1);
@@ -582,9 +617,8 @@ static void preset(struct mtd_info *mtd)
 	struct mxc_nand_host *host = nand_chip->priv;
 	uint16_t tmp;
 
-	/* enable interrupt, disable spare enable, setup ECC */
+	/* disable spare-only, setup ECC */
 	tmp = readw(host->regs + NFC_CONFIG1);
-	tmp &= ~NFC_INT_MSK;
 	tmp &= ~NFC_SP_EN;
 	if (nand_chip->ecc.mode == NAND_ECC_HW) {
 		tmp |= NFC_ECC_EN;
@@ -842,11 +876,18 @@ static int __init mxcnd_probe(struct pla
 		this->options |= NAND_USE_FLASH_BBT;
 	}
 
-	init_waitqueue_head(&host->irq_waitq);
+	init_completion(&host->op_completion);
 
 	host->irq = platform_get_irq(pdev, 0);
 
-	err = request_irq(host->irq, mxc_nfc_irq, IRQF_DISABLED, DRIVER_NAME, host);
+	if (nfc_avoid_masking()) {
+		set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);
+		nfc_unmask_irq(host->regs);
+	} else {
+		nfc_mask_irq(host->regs);
+	}
+
+	err = request_irq(host->irq, mxc_nfc_irq, 0, DRIVER_NAME, host);
 	if (err)
 		goto eirq;
 

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[Ivo Clarysse]

On Thu, Jun 24, 2010 at 9:27 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
[...]
>
> BTW I observed that at least on i.MX27 the latencies introduced by
> waiting for an interrupt cause a significant performance drop. The
> driver gets much faster when we just poll all the time. I don't know how
> this affects system performance otherwise, but it may be a possibility
> to drop interrupt support at least for i.MX21. I have no idea how long
> the longest possible time we'd have to poll is though.

I'm noticing the same thing on i.MX21: when using interrupts,
"Scanning device for bad blocks" takes 982 (!) seconds.  When always
using polling, the same scan takes 'only' 500 seconds.


Ivo.

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[Ivo Clarysse]

On Thu, Jun 24, 2010 at 12:16 PM, John Ogness <john.ogness@linutronix.de> wrote:
> On 2010-06-24, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> Ok, if it's the only way out to have 5 cpu_is_* blocks, then lets go
>> for it.
>
> Here is a new patch that puts the behavior behind a "nfc_avoid_masking"
> macro. The macro is only used 3 times.
[...]

Tested on an MX21ADS board, and it still works.

> This patch is based on linux-next 20100618.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c |   81 +++++++++++++++++++++++++---------
>  1 file changed, 61 insertions(+), 20 deletions(-)
>
> Index: linux-next-20100618/drivers/mtd/nand/mxc_nand.c
> ===================================================================
> --- linux-next-20100618.orig/drivers/mtd/nand/mxc_nand.c
> +++ linux-next-20100618/drivers/mtd/nand/mxc_nand.c
[...]
> @@ -100,6 +109,18 @@
>
>  #define NFC_RSLTSPARE_AREA_MASK  0xff
>
> +#define nfc_interrupt_set(_regs) \
> +               (readw(_regs + NFC_CONFIG2) & NFC_INT)
> +#define nfc_clear_interrupt(_regs) \
> +               writew(readw(_regs + NFC_CONFIG2) & ~NFC_INT, \
> +                      _regs + NFC_CONFIG2)

Naming is not very consistent; I'd suggest nfc_set_interrupt /
nfc_clear_interrupt

> +#define nfc_mask_irq(_regs) \
> +               writew(readw(_regs + NFC_CONFIG1) | NFC_INT_MSK, \
> +                      _regs + NFC_CONFIG1)
> +#define nfc_unmask_irq(_regs) \
> +               writew(readw(_regs + NFC_CONFIG1) & ~NFC_INT_MSK, \
> +                      _regs + NFC_CONFIG1)
> +
[...]

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[John Ogness]

On 2010-06-25, Ivo Clarysse <ivo.clarysse@gmail.com> wrote:
> Tested on an MX21ADS board, and it still works.
>
> [...]
>
> Naming is not very consistent; I'd suggest nfc_set_interrupt /
> nfc_clear_interrupt

Actually, the set function is a query, not an action. I've renamed it
nfc_isset_interrupt() so that it is clearer. I also fixed a typo in one
of the comments.

This patch is based on linux-next 20100618.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/mxc_nand.c |   81 +++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 20 deletions(-)

Index: linux-next-20100618/drivers/mtd/nand/mxc_nand.c
===================================================================
--- linux-next-20100618.orig/drivers/mtd/nand/mxc_nand.c
+++ linux-next-20100618/drivers/mtd/nand/mxc_nand.c
@@ -30,6 +30,8 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/completion.h>
 
 #include <asm/mach/flash.h>
 #include <mach/mxc_nand.h>
@@ -40,6 +42,13 @@
 #define nfc_is_v21()		(cpu_is_mx25() || cpu_is_mx35())
 #define nfc_is_v1()		(cpu_is_mx31() || cpu_is_mx27() || cpu_is_mx21())
 
+/* It has been observed that the i.MX21 cannot read the CONFIG2:INT bit
+ * if interrupts are masked (CONFIG1:INT_MSK is set). To handle this, the
+ * driver can enable/disable the irq line rather than simply masking the
+ * interrupts. The nfc_avoid_masking() macro identifies the systems that
+ * should use this workaround. */
+#define nfc_avoid_masking()	(cpu_is_mx21())
+
 /* Addresses for NFC registers */
 #define NFC_BUF_SIZE		0xE00
 #define NFC_BUF_ADDR		0xE04
@@ -100,6 +109,18 @@
 
 #define NFC_RSLTSPARE_AREA_MASK  0xff
 
+#define nfc_isset_interrupt(_regs) \
+		(readw(_regs + NFC_CONFIG2) & NFC_INT)
+#define nfc_clear_interrupt(_regs) \
+		writew(readw(_regs + NFC_CONFIG2) & ~NFC_INT, \
+		       _regs + NFC_CONFIG2)
+#define nfc_mask_irq(_regs) \
+		writew(readw(_regs + NFC_CONFIG1) | NFC_INT_MSK, \
+		       _regs + NFC_CONFIG1)
+#define nfc_unmask_irq(_regs) \
+		writew(readw(_regs + NFC_CONFIG1) & ~NFC_INT_MSK, \
+		       _regs + NFC_CONFIG1)
+
 struct mxc_nand_host {
 	struct mtd_info		mtd;
 	struct nand_chip	nand;
@@ -117,7 +138,7 @@ struct mxc_nand_host {
 	int			clk_act;
 	int			irq;
 
-	wait_queue_head_t	irq_waitq;
+	struct completion	op_completion;
 
 	uint8_t			*data_buf;
 	unsigned int		buf_start;
@@ -174,9 +195,18 @@ static irqreturn_t mxc_nfc_irq(int irq, 
 {
 	struct mxc_nand_host *host = dev_id;
 
-	disable_irq_nosync(irq);
-
-	wake_up(&host->irq_waitq);
+	/* If NFC_INT is not set, we have a spurious interrupt! */
+	if (!nfc_isset_interrupt(host->regs))
+		return IRQ_NONE;
+
+	/* Even with nfc_avoid_masking() we mask the interrupt
+	 * here to avoid an interrupt flood until the irq line
+	 * is disabled by the driver thread. */
+	nfc_mask_irq(host->regs);
+
+	/* Notify the driver thread that an interrupt has occurred.
+	 * The driver thread will clear the interrupt. */
+	complete(&host->op_completion);
 
 	return IRQ_HANDLED;
 }
@@ -186,27 +216,32 @@ static irqreturn_t mxc_nfc_irq(int irq, 
  */
 static void wait_op_done(struct mxc_nand_host *host, int useirq)
 {
-	uint16_t tmp;
 	int max_retries = 8000;
 
 	if (useirq) {
-		if ((readw(host->regs + NFC_CONFIG2) & NFC_INT) == 0) {
+		if (!nfc_isset_interrupt(host->regs)) {
+
+			INIT_COMPLETION(host->op_completion);
 
-			enable_irq(host->irq);
+			if (nfc_avoid_masking())
+				enable_irq(host->irq);
+			else
+				nfc_unmask_irq(host->regs);
 
-			wait_event(host->irq_waitq,
-				readw(host->regs + NFC_CONFIG2) & NFC_INT);
+			/* wait for the interrupt */
+			wait_for_completion(&host->op_completion);
 
-			tmp = readw(host->regs + NFC_CONFIG2);
-			tmp  &= ~NFC_INT;
-			writew(tmp, host->regs + NFC_CONFIG2);
+			if (nfc_avoid_masking()) {
+				disable_irq(host->irq);
+				nfc_unmask_irq(host->regs);
+			}
 		}
+
+		nfc_clear_interrupt(host->regs);
 	} else {
 		while (max_retries-- > 0) {
-			if (readw(host->regs + NFC_CONFIG2) & NFC_INT) {
-				tmp = readw(host->regs + NFC_CONFIG2);
-				tmp  &= ~NFC_INT;
-				writew(tmp, host->regs + NFC_CONFIG2);
+			if (nfc_isset_interrupt(host->regs)) {
+				nfc_clear_interrupt(host->regs);
 				break;
 			}
 			udelay(1);
@@ -582,9 +617,8 @@ static void preset(struct mtd_info *mtd)
 	struct mxc_nand_host *host = nand_chip->priv;
 	uint16_t tmp;
 
-	/* enable interrupt, disable spare enable, setup ECC */
+	/* disable spare-only, setup ECC */
 	tmp = readw(host->regs + NFC_CONFIG1);
-	tmp &= ~NFC_INT_MSK;
 	tmp &= ~NFC_SP_EN;
 	if (nand_chip->ecc.mode == NAND_ECC_HW) {
 		tmp |= NFC_ECC_EN;
@@ -842,11 +876,18 @@ static int __init mxcnd_probe(struct pla
 		this->options |= NAND_USE_FLASH_BBT;
 	}
 
-	init_waitqueue_head(&host->irq_waitq);
+	init_completion(&host->op_completion);
 
 	host->irq = platform_get_irq(pdev, 0);
 
-	err = request_irq(host->irq, mxc_nfc_irq, IRQF_DISABLED, DRIVER_NAME, host);
+	if (nfc_avoid_masking()) {
+		set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);
+		nfc_unmask_irq(host->regs);
+	} else {
+		nfc_mask_irq(host->regs);
+	}
+
+	err = request_irq(host->irq, mxc_nfc_irq, 0, DRIVER_NAME, host);
 	if (err)
 		goto eirq;
 

Re: [PATCHv3 4/5] mtd: mxc_nand fixups

[Ivo Clarysse]

On Sat, Jun 26, 2010 at 11:17 AM, John Ogness <john.ogness@linutronix.de> wrote:
> On 2010-06-25, Ivo Clarysse <ivo.clarysse@gmail.com> wrote:
>> Tested on an MX21ADS board, and it still works.
>>
>> [...]
>>
>> Naming is not very consistent; I'd suggest nfc_set_interrupt /
>> nfc_clear_interrupt
>
> Actually, the set function is a query, not an action. I've renamed it
> nfc_isset_interrupt() so that it is clearer. I also fixed a typo in one
> of the comments.
>
> This patch is based on linux-next 20100618.
[...]

Verified as working successfully on an MX21ADS board.

Ivo.