login
Search
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;
--
[added Ivo to Cc] This behaviour was introduced in a47bfd2eb66837653dc3b42541dfe4283dd41251 mxc_nand: support i.MX21 I guess this won't work on i.MX21. -- 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 | --
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 --
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; --
[...] Yes, that was indeed my true intention :) I tested this change on an MX21ADS board. Acked-by: Ivo Clarysse <ivo.clarysse@gmail.com> --
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 ...
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 : ...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 ...On Wed, Jun 23, 2010 at 10:48 AM, John Ogness <john.ogness@linutronix.de> wrote: Yes, this works on i.MX21 (tested on an MX21ADS board). 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 --
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 --
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 | --
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 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 ...
[...] Naming is not very consistent; I'd suggest nfc_set_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 ...[...] Verified as working successfully on an MX21ADS board. Ivo. --
On Thu, Jun 24, 2010 at 9:27 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: 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. --
| Greg KH | Og dreams of kernels |
| Jens Axboe | [PATCH 31/33] Fusion: sg chaining support |
| Arnd Bergmann | Re: finding your own dead "CONFIG_" variables |
| Mark Brown | [PATCH 2/2] Subject: natsemi: Allow users to disable workaround for DspCfg reset |
| Tony Breeds |