Skip to contentSkip to conversation

mxc_nand: set spare size and pages per block

PATCH New John Ogness 3 participants 14 messages Aug 10, 2010 – Aug 29, 2010

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

John Ogness

author
#0 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

This patch sets the NFC registers for spare size and pages per block.

This patch is against the latest patches from Sascha Hauer.

Signed-off-by John Ogness <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
drivers/mtd/nand/mxc_nand.c +32 -0
Index: linux-2.6-454a740/drivers/mtd/nand/mxc_nand.c
===================================================================
@@ -68,6 +68,8 @@
 #define NFC_V1_V2_CONFIG1_RST		(1 << 6)
 #define NFC_V1_V2_CONFIG1_CE		(1 << 7)
 #define NFC_V1_V2_CONFIG1_ONE_CYCLE	(1 << 8)
+#define NFC_V1_V2_CONFIG1_PPB_SHIFT	9
+#define NFC_V1_V2_CONFIG1_PPB_MASK	0x3
 
 #define NFC_V1_V2_CONFIG2_INT		(1 << 15)
 

@@ -749,7 +751,37 @@ static void preset_v1_v2(struct mtd_info
 		host->eccsize = 1;
 	}
 
+	if (nfc_is_v21() && mtd->writesize) {
+		/* setup pages per block */
+		tmp &= ~(NFC_V1_V2_CONFIG1_PPB_MASK <<
+					NFC_V1_V2_CONFIG1_PPB_SHIFT);
+		switch (mtd->erasesize / mtd->writesize) {
+		case 32:
+			/* PPB set to 0 */
+			break;
+		case 64:
+			tmp |= 1 << NFC_V1_V2_CONFIG1_PPB_SHIFT;
+			break;
+		case 256:
+			tmp |= 3 << NFC_V1_V2_CONFIG1_PPB_SHIFT;
+			break;
+		default:
+			/* 128 (reset value) */
+			tmp |= 2 << NFC_V1_V2_CONFIG1_PPB_SHIFT;
+			break;
+		}
+	}
+
 	writew(tmp, NFC_V1_V2_CONFIG1);
+
+	if (nfc_is_v21()) {
+		/* configure spare size (in 16-bit units) */
+		tmp = readw(NFC_V1_V2_RSLTSPARE_AREA);
+		tmp &= ~0xff;
+		tmp |= host->spare_len >> 1;
+		writew(tmp, NFC_V1_V2_RSLTSPARE_AREA);
+	}
+
 	/* preset operation */
 
 	/* Unlock the internal RAM Buffer */
author
#1 lore raw

PATCH 2/3 mxc_nand: remove unused variables

This patch removes some unused variable declarations.

This patch is against the latest patches from Sascha Hauer.

Signed-off-by John Ogness <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c |    2 --
 1 file changed, 2 deletions(-)
drivers/mtd/nand/mxc_nand.c +0 -2
Index: linux-2.6-454a740/drivers/mtd/nand/mxc_nand.c
===================================================================
@@ -1221,8 +1221,6 @@ static int mxcnd_suspend(struct platform
 static int mxcnd_resume(struct platform_device *pdev)
 {
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct nand_chip *nand_chip = mtd->priv;
-	struct mxc_nand_host *host = nand_chip->priv;
 	int ret = 0;
 
 	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND resume\n");
author
#2 lore raw

PATCH 3/3 mxc_nand: mask instead of disabling (i.MX21 as exception)

This patch reverts the driver to using 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 required workaround for the i.MX21 has been encapsulated using the nfc_avoid_masking macro.

This patch is against the latest patches from Sascha Hauer.

Signed-off-by John Ogness <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c |  138 ++++++++++++++++++++++++++--------
 1 file changed, 109 insertions(+), 29 deletions(-)
drivers/mtd/nand/mxc_nand.c +109 -29
Index: linux-2.6-454a740/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>

@@ -42,6 +44,13 @@
 #define nfc_is_v3_2()		cpu_is_mx51()
 #define nfc_is_v3()		nfc_is_v3_2()
 
+/* It has been observed that the i.MX21 cannot read its 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_V1_V2_BUF_SIZE		(host->regs + 0x00)
 #define NFC_V1_V2_BUF_ADDR		(host->regs + 0x04)

@@ -151,7 +160,7 @@ struct mxc_nand_host {
 	int			irq;
 	int			eccsize;
 
-	wait_queue_head_t	irq_waitq;
+	struct completion	op_completion;
 
 	uint8_t			*data_buf;
 	unsigned int		buf_start;

@@ -163,7 +172,10 @@ struct mxc_nand_host {
 	void			(*send_page)(struct mtd_info *, unsigned int);
 	void			(*send_read_id)(struct mxc_nand_host *);
 	uint16_t		(*get_dev_status)(struct mxc_nand_host *);
-	int			(*check_int)(struct mxc_nand_host *);
+	int			(*isset_int)(struct mxc_nand_host *);
+	void			(*ack_int)(struct mxc_nand_host *);
+	void			(*mask_int)(struct mxc_nand_host *);
+	void			(*unmask_int)(struct mxc_nand_host *);
 };
 
 /* OOB placement block for use with hardware ecc generation */

@@ -216,38 +228,74 @@ 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 (!host->isset_int(host))
+		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. */
+	host->mask_int(host);
+
+	/* Notify the driver thread that an interrupt has occurred.
+	 * The driver thread will clear the interrupt. */
+	complete(&host->op_completion);
 
 	return IRQ_HANDLED;
 }
 
-static int check_int_v3(struct mxc_nand_host *host)
+static int isset_int_v3(struct mxc_nand_host *host)
 {
-	uint32_t tmp;
-
-	tmp = readl(NFC_V3_IPC);
-	if (!(tmp & NFC_V3_IPC_INT))
-		return 0;
+	uint32_t tmp = readl(NFC_V3_IPC);
+	return (tmp & NFC_V3_IPC_INT);
+}
 
+static void ack_int_v3(struct mxc_nand_host *host)
+{
+	uint32_t tmp = readl(NFC_V3_IPC);
 	tmp &= ~NFC_V3_IPC_INT;
 	writel(tmp, NFC_V3_IPC);
+}
 
-	return 1;
+static void mask_int_v3(struct mxc_nand_host *host)
+{
+	uint32_t tmp = readl(NFC_V3_CONFIG2);
+	tmp |= NFC_V3_CONFIG2_INT_MSK;
+	writel(tmp, NFC_V3_CONFIG2);
 }
 
-static int check_int_v1_v2(struct mxc_nand_host *host)
+static void unmask_int_v3(struct mxc_nand_host *host)
 {
-	uint32_t tmp;
+	uint32_t tmp = readl(NFC_V3_CONFIG2);
+	tmp &= ~NFC_V3_CONFIG2_INT_MSK;
+	writel(tmp, NFC_V3_CONFIG2);
+}
 
-	tmp = readw(NFC_V1_V2_CONFIG2);
-	if (!(tmp & NFC_V1_V2_CONFIG2_INT))
-		return 0;
+static int isset_int_v1_v2(struct mxc_nand_host *host)
+{
+	uint16_t tmp = readw(NFC_V1_V2_CONFIG2);
+	return (tmp & NFC_V1_V2_CONFIG2_INT);
+}
 
-	writew(tmp & ~NFC_V1_V2_CONFIG2_INT, NFC_V1_V2_CONFIG2);
+static void ack_int_v1_v2(struct mxc_nand_host *host)
+{
+	uint16_t tmp = readw(NFC_V1_V2_CONFIG2);
+	tmp &= ~NFC_V1_V2_CONFIG2_INT;
+	writew(tmp, NFC_V1_V2_CONFIG2);
+}
 
-	return 1;
+static void mask_int_v1_v2(struct mxc_nand_host *host)
+{
+	uint16_t tmp = readw(NFC_V1_V2_CONFIG1);
+	tmp |= NFC_V1_V2_CONFIG1_INT_MSK;
+	writew(tmp, NFC_V1_V2_CONFIG1);
+}
+
+static void unmask_int_v1_v2(struct mxc_nand_host *host)
+{
+	uint16_t tmp = readw(NFC_V1_V2_CONFIG1);
+	tmp &= ~NFC_V1_V2_CONFIG1_INT_MSK;
+	writew(tmp, NFC_V1_V2_CONFIG1);
 }
 
 /* This function polls the NANDFC to wait for the basic operation to

@@ -258,16 +306,33 @@ static void wait_op_done(struct mxc_nand
 	int max_retries = 8000;
 
 	if (useirq) {
-		if (!host->check_int(host)) {
-
-			enable_irq(host->irq);
+		if (!host->isset_int(host)) {
+			INIT_COMPLETION(host->op_completion);
 
-			wait_event(host->irq_waitq, host->check_int(host));
+			if (nfc_avoid_masking())
+				enable_irq(host->irq);
+			else
+				host->unmask_int(host);
+
+			/* wait for the interrupt */
+			wait_for_completion(&host->op_completion);
+
+			/* The irq handler masked the interrupt
+			 * and expects us to ack the interrupt. */
+
+			if (nfc_avoid_masking()) {
+				disable_irq(host->irq);
+				host->unmask_int(host);
+			}
 		}
+
+		host->ack_int(host);
 	} else {
 		while (max_retries-- > 0) {
-			if (host->check_int(host))
+			if (host->isset_int(host)) {
+				host->ack_int(host);
 				break;
+			}
 
 			udelay(1);
 		}

@@ -735,7 +800,6 @@ static void preset_v1_v2(struct mtd_info
 
 	/* enable interrupt, disable spare enable */
 	tmp = readw(NFC_V1_V2_CONFIG1);
-	tmp &= ~NFC_V1_V2_CONFIG1_INT_MSK;
 	tmp &= ~NFC_V1_V2_CONFIG1_SP_EN;
 	if (nand_chip->ecc.mode == NAND_ECC_HW) {
 		tmp |= NFC_V1_V2_CONFIG1_ECC_EN;

@@ -828,6 +892,9 @@ static void preset_v3(struct mtd_info *m
 		NFC_V3_CONFIG2_ST_CMD(0x70) |
 		NFC_V3_CONFIG2_NUM_ADDR_PHASE0;
 
+	if (!nfc_avoid_masking())
+		config2 |= NFC_V3_CONFIG2_INT_MSK;
+
 	if (chip->ecc.mode == NAND_ECC_HW)
 		config2 |= NFC_V3_CONFIG2_ECC_EN;
 

@@ -1050,7 +1117,10 @@ static int __init mxcnd_probe(struct pla
 		host->send_page = send_page_v1_v2;
 		host->send_read_id = send_read_id_v1_v2;
 		host->get_dev_status = get_dev_status_v1_v2;
-		host->check_int = check_int_v1_v2;
+		host->isset_int = isset_int_v1_v2;
+		host->ack_int = ack_int_v1_v2;
+		host->mask_int = mask_int_v1_v2;
+		host->unmask_int = unmask_int_v1_v2;
 	}
 
 	if (nfc_is_v21()) {

@@ -1087,7 +1157,10 @@ static int __init mxcnd_probe(struct pla
 		host->send_addr = send_addr_v3;
 		host->send_page = send_page_v3;
 		host->send_read_id = send_read_id_v3;
-		host->check_int = check_int_v3;
+		host->isset_int = isset_int_v3;
+		host->ack_int = ack_int_v3;
+		host->mask_int = mask_int_v3;
+		host->unmask_int = unmask_int_v3;
 		host->get_dev_status = get_dev_status_v3;
 		oob_smallpage = &nandv2_hw_eccoob_smallpage;
 		oob_largepage = &nandv2_hw_eccoob_largepage;

@@ -1120,11 +1193,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);
+		host->unmask_int(host);
+	} else {
+		host->mask_int(host);
+	}
+
+	err = request_irq(host->irq, mxc_nfc_irq, 0, DRIVER_NAME, host);
 	if (err)
 		goto eirq;
maintainer
#3 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

In reply to John Ogness

Hi John,

Sorry, last time I sent only up to 09/12, so the patches I explicitely mentioned to solve the things from your previous series were missing. I just sent them. My versions of the patches differ slightly. For this patch I decided to initialize every bit in NFC_V1_V2_CONFIG1 from scratch so that we do not depend on any reset or bootloader values. I think this is cleaner so I propose that we use my version of the patch.

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
#4 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

Quoted 3 lines · Sascha Hauer

Sorry, last time I sent only up to 09/12, so the patches I explicitely mentioned to solve the things from your previous series were missing. I just sent them. My versions of the patches differ slightly.

Your version allows a small window between request_irq() and irq_control() where on the i.MX21 there is a possibility of the interrupts being disabled twice. Namely, if an interrupt occurs before irq_control() has had a chance to disable it. IMHO it would be better to call:

    set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);

for the i.MX21 before requesting the irq. This closes the window.

For non-i.MX21 this window also exists, but since in that situation the irq hander simply unnecessarily sets a bit, it is not so dramatic. By masking the interrupt before requesting the irq, the windows is also closed for non-i.MX21.

Quoted 4 lines

For this patch I decided to initialize every bit in NFC_V1_V2_CONFIG1 from scratch so that we do not depend on any reset or bootloader values. I think this is cleaner so I propose that we use my version of the patch.

I agree that initializing all the bits is better. But you need to set the mask as well. Your latest patches clear the mask when initializing V1_V2_CONFIG1 and V3_CONFIG2. For non-i.MX21 the mask should always be set except when explicitly waiting for an interrupt in wait_op_done().

All the patches (01-12) were tested on an i.MX35 with 16-bit NAND and work as expected. My only recommendations would be to close the window at request_irq() and also include the following patch to set the mask during preset().

Signed-off-by John Ogness <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c |    3 +++
 1 file changed, 3 insertions(+)
drivers/mtd/nand/mxc_nand.c +3 -0
Index: linux-2.6-454a740/drivers/mtd/nand/mxc_nand.c
===================================================================
@@ -788,6 +788,8 @@ static void preset_v1_v2(struct mtd_info
 
 	if (nfc_is_v21())
 		config1 |= NFC_V2_CONFIG1_FP_INT;
+	else
+		config1 |= NFC_V1_V2_CONFIG1_INT_MSK;
 
 	if (nfc_is_v21() && mtd->writesize) {
 		uint16_t pages_per_block = mtd->erasesize / mtd->writesize;

@@ -846,6 +848,7 @@ static void preset_v3(struct mtd_info *m
 		NFC_V3_CONFIG2_2CMD_PHASES |
 		NFC_V3_CONFIG2_SPAS(mtd->oobsize >> 1) |
 		NFC_V3_CONFIG2_ST_CMD(0x70) |
+		NFC_V3_CONFIG2_INT_MSK |
 		NFC_V3_CONFIG2_NUM_ADDR_PHASE0;
 
 	if (chip->ecc.mode == NAND_ECC_HW)
author
#5 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

Quoted 9 lines · John Ogness
Quoted 4 lines

For this patch I decided to initialize every bit in NFC_V1_V2_CONFIG1 from scratch so that we do not depend on any reset or bootloader values. I think this is cleaner so I propose that we use my version of the patch.

I agree that initializing all the bits is better. But you need to set the mask as well. Your latest patches clear the mask when initializing V1_V2_CONFIG1 and V3_CONFIG2. For non-i.MX21 the mask should always be set except when explicitly waiting for an interrupt in wait_op_done().

Sorry, that last patch was crap. It wasn't setting the mask for v1/v2. I should have noticed it sooner because without the mask an extra interrupt is visible during boot. Here is the correct patch to fix the masking issue.

Signed-off-by John Ogness <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c |    4 ++++
 1 file changed, 4 insertions(+)
drivers/mtd/nand/mxc_nand.c +4 -0
Index: linux-2.6-454a740/drivers/mtd/nand/mxc_nand.c
===================================================================
@@ -789,6 +789,9 @@ static void preset_v1_v2(struct mtd_info
 	if (nfc_is_v21())
 		config1 |= NFC_V2_CONFIG1_FP_INT;
 
+	if (!cpu_is_mx21())
+		config1 |= NFC_V1_V2_CONFIG1_INT_MSK;
+
 	if (nfc_is_v21() && mtd->writesize) {
 		uint16_t pages_per_block = mtd->erasesize / mtd->writesize;
 

@@ -846,6 +849,7 @@ static void preset_v3(struct mtd_info *m
 		NFC_V3_CONFIG2_2CMD_PHASES |
 		NFC_V3_CONFIG2_SPAS(mtd->oobsize >> 1) |
 		NFC_V3_CONFIG2_ST_CMD(0x70) |
+		NFC_V3_CONFIG2_INT_MSK |
 		NFC_V3_CONFIG2_NUM_ADDR_PHASE0;
 
 	if (chip->ecc.mode == NAND_ECC_HW)
maintainer
#6 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

In reply to John Ogness
Quoted 11 lines · John Ogness
Quoted 3 lines · Sascha Hauer

Sorry, last time I sent only up to 09/12, so the patches I explicitely mentioned to solve the things from your previous series were missing. I just sent them. My versions of the patches differ slightly.

Your version allows a small window between request_irq() and irq_control() where on the i.MX21 there is a possibility of the interrupts being disabled twice. Namely, if an interrupt occurs before irq_control() has had a chance to disable it. IMHO it would be better to call:

Quoted 3 lines
    set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);

for the i.MX21 before requesting the irq. This closes the window.

IIRC it is not allowed to call set_irq_flags before request_irq. We are changing a resource we do not own yet.
I think the worst thing that could happen without this change is that we get an interrupt after request_irq.
Alternatively we could set the interrupt mask bit before requesting the irq.

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
#7 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

Quoted 6 lines · Sascha Hauer
Quoted 6 lines

Your version allows a small window between request_irq() and irq_control() where on the i.MX21 there is a possibility of the interrupts being disabled twice. Namely, if an interrupt occurs before irq_control() has had a chance to disable it. IMHO it would be better to call:

Quoted 6 lines
Quoted 3 lines
    set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);

for the i.MX21 before requesting the irq. This closes the window.

IIRC it is not allowed to call set_irq_flags before request_irq. We are changing a resource we do not own yet.

Normally, yes. But the only way the IRQF_NOAUTOEN flag can ever be useful is if set_irq_flags() is called before request_irq().

Quoted 2 lines

I think the worst thing that could happen without this change is that we get an interrupt after request_irq.

Yes. And this causes a problem on the i.MX21 because the interrupt handler will disable the interrupts. The irq_control() after request_irq() will _also_ disable the interrupts. This means the interrupts are disabled twice, which causes some issues on an RT kernel.

Quoted 2 lines

Alternatively we could set the interrupt mask bit before requesting the irq.

Yes. This would be best. But for the i.MX21 it is important that the interrupts are unmasked after irq_control() has disabled the interrupts.

John Ogness

maintainer
#8 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

Quoted 25 lines · John Ogness
Quoted 6 lines · Sascha Hauer
Quoted 6 lines

Your version allows a small window between request_irq() and irq_control() where on the i.MX21 there is a possibility of the interrupts being disabled twice. Namely, if an interrupt occurs before irq_control() has had a chance to disable it. IMHO it would be better to call:

Quoted 6 lines
Quoted 3 lines
    set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);

for the i.MX21 before requesting the irq. This closes the window.

IIRC it is not allowed to call set_irq_flags before request_irq. We are changing a resource we do not own yet.

Normally, yes. But the only way the IRQF_NOAUTOEN flag can ever be useful is if set_irq_flags() is called before request_irq().

Quoted 2 lines

I think the worst thing that could happen without this change is that we get an interrupt after request_irq.

Yes. And this causes a problem on the i.MX21 because the interrupt handler will disable the interrupts. The irq_control() after request_irq() will _also_ disable the interrupts. This means the interrupts are disabled twice, which causes some issues on an RT kernel.

Quoted 5 lines
Quoted 2 lines

Alternatively we could set the interrupt mask bit before requesting the irq.

Yes. This would be best. But for the i.MX21 it is important that the interrupts are unmasked after irq_control() has disabled the interrupts.

Ok, I'll update the patches to use this approach.

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 |
maintainer
#9 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

In reply to John Ogness
Quoted 25 lines · John Ogness
Quoted 6 lines · Sascha Hauer
Quoted 6 lines

Your version allows a small window between request_irq() and irq_control() where on the i.MX21 there is a possibility of the interrupts being disabled twice. Namely, if an interrupt occurs before irq_control() has had a chance to disable it. IMHO it would be better to call:

Quoted 6 lines
Quoted 3 lines
    set_irq_flags(host->irq, IRQF_VALID | IRQF_NOAUTOEN);

for the i.MX21 before requesting the irq. This closes the window.

IIRC it is not allowed to call set_irq_flags before request_irq. We are changing a resource we do not own yet.

Normally, yes. But the only way the IRQF_NOAUTOEN flag can ever be useful is if set_irq_flags() is called before request_irq().

Quoted 2 lines

I think the worst thing that could happen without this change is that we get an interrupt after request_irq.

Yes. And this causes a problem on the i.MX21 because the interrupt handler will disable the interrupts. The irq_control() after request_irq() will _also_ disable the interrupts. This means the interrupts are disabled twice, which causes some issues on an RT kernel.

Quoted 5 lines
Quoted 2 lines

Alternatively we could set the interrupt mask bit before requesting the irq.

Yes. This would be best. But for the i.MX21 it is important that the interrupts are unmasked after irq_control() has disabled the interrupts.

Ok, here is an updated version of this patch

Sascha

commit 3a411e6c690654f78bea93dd8cdb14c27ba418a5

Author: Sascha Hauer <[email protected]>
Date:   Mon Aug 9 14:21:00 2010 +0200

mxc_nand: do not depend on disabling the irq in the interrupt handler

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.
For i.MX21 we keep the current behaviour, that is calling enable_irq/disable_irq_nosync to enable/disable interrupts. This patch is based on earlier work by John Ogness.

Signed-off-by Sascha Hauer <[email protected]>
drivers/mtd/nand/mxc_nand.c +83 -11
index d58214b..c1a3d04 100644
@@ -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>

@@ -149,7 +151,7 @@ struct mxc_nand_host {
 	int			irq;
 	int			eccsize;
 
-	wait_queue_head_t	irq_waitq;
+	struct completion	op_completion;
 
 	uint8_t			*data_buf;
 	unsigned int		buf_start;

@@ -162,6 +164,7 @@ struct mxc_nand_host {
 	void			(*send_read_id)(struct mxc_nand_host *);
 	uint16_t		(*get_dev_status)(struct mxc_nand_host *);
 	int			(*check_int)(struct mxc_nand_host *);
+	void			(*irq_control)(struct mxc_nand_host *, int);
 };
 
 /* OOB placement block for use with hardware ecc generation */

@@ -214,9 +217,12 @@ static irqreturn_t mxc_nfc_irq(int irq, void *dev_id)
 {
 	struct mxc_nand_host *host = dev_id;
 
-	disable_irq_nosync(irq);
+	if (!host->check_int(host))
+		return IRQ_NONE;
 
-	wake_up(&host->irq_waitq);
+	host->irq_control(host, 0);
+
+	complete(&host->op_completion);
 
 	return IRQ_HANDLED;
 }

@@ -243,11 +249,54 @@ static int check_int_v1_v2(struct mxc_nand_host *host)
 	if (!(tmp & NFC_V1_V2_CONFIG2_INT))
 		return 0;
 
-	writew(tmp & ~NFC_V1_V2_CONFIG2_INT, NFC_V1_V2_CONFIG2);
+	if (!cpu_is_mx21())
+		writew(tmp & ~NFC_V1_V2_CONFIG2_INT, NFC_V1_V2_CONFIG2);
 
 	return 1;
 }
 
+/*
+ * 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.
+ */
+static void irq_control_mx21(struct mxc_nand_host *host, int activate)
+{
+	if (activate)
+		enable_irq(host->irq);
+	else
+		disable_irq_nosync(host->irq);
+}
+
+static void irq_control_v1_v2(struct mxc_nand_host *host, int activate)
+{
+	uint16_t tmp;
+
+	tmp = readw(NFC_V1_V2_CONFIG1);
+
+	if (activate)
+		tmp &= ~NFC_V1_V2_CONFIG1_INT_MSK;
+	else
+		tmp |= NFC_V1_V2_CONFIG1_INT_MSK;
+
+	writew(tmp, NFC_V1_V2_CONFIG1);
+}
+
+static void irq_control_v3(struct mxc_nand_host *host, int activate)
+{
+	uint32_t tmp;
+
+	tmp = readl(NFC_V3_CONFIG2);
+
+	if (activate)
+		tmp &= ~NFC_V3_CONFIG2_INT_MSK;
+	else
+		tmp |= NFC_V3_CONFIG2_INT_MSK;
+
+	writel(tmp, NFC_V3_CONFIG2);
+}
+
 /* This function polls the NANDFC to wait for the basic operation to
  * complete by checking the INT bit of config2 register.
  */

@@ -257,10 +306,9 @@ static void wait_op_done(struct mxc_nand_host *host, int useirq)
 
 	if (useirq) {
 		if (!host->check_int(host)) {
-
-			enable_irq(host->irq);
-
-			wait_event(host->irq_waitq, host->check_int(host));
+			INIT_COMPLETION(host->op_completion);
+			host->irq_control(host, 1);
+			wait_for_completion(&host->op_completion);
 		}
 	} else {
 		while (max_retries-- > 0) {

@@ -731,9 +779,8 @@ static void preset_v1_v2(struct mtd_info *mtd)
 	struct mxc_nand_host *host = nand_chip->priv;
 	uint16_t tmp;
 
-	/* enable interrupt, disable spare enable */
+	/* disable spare enable */
 	tmp = readw(NFC_V1_V2_CONFIG1);
-	tmp &= ~NFC_V1_V2_CONFIG1_INT_MSK;
 	tmp &= ~NFC_V1_V2_CONFIG1_SP_EN;
 	if (nand_chip->ecc.mode == NAND_ECC_HW) {
 		tmp |= NFC_V1_V2_CONFIG1_ECC_EN;

@@ -1020,6 +1067,10 @@ static int __init mxcnd_probe(struct platform_device *pdev)
 		host->send_read_id = send_read_id_v1_v2;
 		host->get_dev_status = get_dev_status_v1_v2;
 		host->check_int = check_int_v1_v2;
+		if (cpu_is_mx21())
+			host->irq_control = irq_control_mx21;
+		else
+			host->irq_control = irq_control_v1_v2;
 	}
 
 	if (nfc_is_v21()) {

@@ -1058,6 +1109,7 @@ static int __init mxcnd_probe(struct platform_device *pdev)
 		host->send_read_id = send_read_id_v3;
 		host->check_int = check_int_v3;
 		host->get_dev_status = get_dev_status_v3;
+		host->irq_control = irq_control_v3;
 		oob_smallpage = &nandv2_hw_eccoob_smallpage;
 		oob_largepage = &nandv2_hw_eccoob_largepage;
 	} else

@@ -1089,14 +1141,34 @@ static int __init mxcnd_probe(struct platform_device *pdev)
 		this->options |= NAND_USE_FLASH_BBT;
 	}
 
-	init_waitqueue_head(&host->irq_waitq);
+	init_completion(&host->op_completion);
 
 	host->irq = platform_get_irq(pdev, 0);
 
+	/*
+	 * mask the interrupt. For i.MX21 explicitely call
+	 * irq_control_v1_v2 to use the mask bit. We can't call
+	 * disable_irq_nosync() for an interrupt we do not own yet.
+	 */
+	if (cpu_is_mx21())
+		irq_control_v1_v2(host, 0);
+	else
+		host->irq_control(host, 0);
+
 	err = request_irq(host->irq, mxc_nfc_irq, IRQF_DISABLED, DRIVER_NAME, host);
 	if (err)
 		goto eirq;
 
+	host->irq_control(host, 0);
+
+	/*
+	 * Now that the interrupt is disabled make sure the interrupt
+	 * mask bit is cleared on i.MX21. Otherwise we can't read
+	 * the interrupt status bit on this machine.
+	 */
+	if (cpu_is_mx21())
+		irq_control_v1_v2(host, 1);
+
 	/* first scan to find the device and get the page size */
 	if (nand_scan_ident(mtd, 1, NULL)) {
 		err = -ENXIO;
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
#10 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

Quoted 10 lines · Sascha Hauer

Ok, here is an updated version of this patch

Sascha

commit 3a411e6c690654f78bea93dd8cdb14c27ba418a5

Author: Sascha Hauer <[email protected]>
Date:   Mon Aug 9 14:21:00 2010 +0200

mxc_nand: do not depend on disabling the irq in the interrupt handler

Quoted 7 lines

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.
For i.MX21 we keep the current behaviour, that is calling enable_irq/disable_irq_nosync to enable/disable interrupts. This patch is based on earlier work by John Ogness.

Acked-by John Ogness <[email protected]>
Quoted 1 line
Signed-off-by Sascha Hauer <[email protected]>
maintainer
#11 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

Quoted 19 lines · John Ogness
Quoted 10 lines · Sascha Hauer

Ok, here is an updated version of this patch

Sascha

commit 3a411e6c690654f78bea93dd8cdb14c27ba418a5

Author: Sascha Hauer <[email protected]>
Date:   Mon Aug 9 14:21:00 2010 +0200

mxc_nand: do not depend on disabling the irq in the interrupt handler

Quoted 7 lines

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.
For i.MX21 we keep the current behaviour, that is calling enable_irq/disable_irq_nosync to enable/disable interrupts. This patch is based on earlier work by John Ogness.

Quoted 1 line
Acked-by John Ogness <[email protected]>

Can I consider this as an Acked-by for the whole series? And since you tested it I would also add a tested-by. Here is a summary of the series again:

The following changes since commit da5cabf80e2433131bf0ed8993abc0f7ea618c73:

Linux 2.6.36-rc1 (2010-08-15 17:41:37 -0700)

are available in the git repository at:
git://git.pengutronix.de/git/imx/linux-2.6.git mxc-nand-pu

John Ogness (2):
mxc_nand: Do not do byte accesses to the NFC buffer.
mxc_nand: remove unused variables.

Sascha Hauer (10):
mxc_nand: remove 0xe00 offset from registers
mxc_nand: rework get_dev_status
mxc_nand: make some internally used functions overwriteable
mxc_nand: factor out a check_int function
mxc_nand: add V1_V2 namespace to registers
mxc_nand: support 8bit ecc
mxc_nand: fix correct_data function
mxc_nand: Add v3 (i.MX51) Support
mxc_nand: do not depend on disabling the irq in the interrupt handler
mxc_nand: configure pages per block for v2 controller

 drivers/mtd/nand/Kconfig    |    2 +-
 drivers/mtd/nand/mxc_nand.c |  671 +++++++++++++++++++++++++++++++++----------
 2 files changed, 521 insertions(+), 152 deletions(-)

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

PATCH 1/3 mxc_nand: set spare size and pages per block

Quoted 23 lines · Sascha Hauer

The following changes since commit da5cabf80e2433131bf0ed8993abc0f7ea618c73:

Linux 2.6.36-rc1 (2010-08-15 17:41:37 -0700)

are available in the git repository at:
git://git.pengutronix.de/git/imx/linux-2.6.git mxc-nand-pu

John Ogness (2):
mxc_nand: Do not do byte accesses to the NFC buffer.
mxc_nand: remove unused variables.

Sascha Hauer (10):
mxc_nand: remove 0xe00 offset from registers
mxc_nand: rework get_dev_status
mxc_nand: make some internally used functions overwriteable
mxc_nand: factor out a check_int function
mxc_nand: add V1_V2 namespace to registers
mxc_nand: support 8bit ecc
mxc_nand: fix correct_data function
mxc_nand: Add v3 (i.MX51) Support
mxc_nand: do not depend on disabling the irq in the interrupt handler
mxc_nand: configure pages per block for v2 controller

Quoted 3 lines
 drivers/mtd/nand/Kconfig    |    2 +-
 drivers/mtd/nand/mxc_nand.c |  671 +++++++++++++++++++++++++++++++++----------
 2 files changed, 521 insertions(+), 152 deletions(-)
Acked-by John Ogness <[email protected]>
Tested-by John Ogness <[email protected]>

I tested the driver with all the above changes on an i.MX35 with a 16-bit NAND chip. The driver performed as expected.

#13 lore raw

PATCH 1/3 mxc_nand: set spare size and pages per block

In reply to John Ogness
Quoted 8 lines · John Ogness

This patch sets the NFC registers for spare size and pages per block.

This patch is against the latest patches from Sascha Hauer.

Signed-off-by John Ogness <[email protected]>
---
 drivers/mtd/nand/mxc_nand.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

I'm not picking your patches and assume they'll be sent by Sascha.

Signature · 2 lines
Best Regards,
Artem Bityutskiy (???????? ?????)