[PATCH 0/4 v2] mmc: tmio, sdhi: provide multiple irq handlers

[PATCH 0/4 v2] mmc: tmio, sdhi: provide multiple irq handlers

[Simon Horman]

The SDHI driver already supports making use of up to three interrupt
sources.

This series breaks up the existing interrupt handler into three handlers,
one for card access, one for card detect interrupts, and one for SDIO
interrupts.  A cover-all handler, which makes use of these new broken-out
handlers is provided for for the case where there is only one interrupt
source.

This series also wires up the broken-out irq handlers in the SDHI driver

[PATCH 1/5] mmc: tmio: Cache interrupt masks

[Simon Horman]

This avoids the need to look up the masks each time an interrupt
is handled.

As suggested by Guennadi.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

* SDCARD portion tested on AP4/Mackerel
* SDIO portion untested
---
 drivers/mmc/host/tmio_mmc.h     |    4 ++++
 drivers/mmc/host/tmio_mmc_pio.c |   36 ++++++++++++++++++++----------------
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index eeaf643..1cf8db5 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -79,6 +79,10 @@ struct tmio_mmc_host {
 	struct delayed_work	delayed_reset_work;
 	struct work_struct	done;
 
+	/* Cache IRQ mask */
+	u32			sdcard_irq_mask;
+	u32			sdio_irq_mask;
+
 	spinlock_t		lock;		/* protect host private data */
 	unsigned long		last_req_ts;
 	struct mutex		ios_lock;	/* protect set_ios() context */
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 1f16357..c60b15f 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -48,14 +48,16 @@
 
 void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
 {
-	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) & ~(i & TMIO_MASK_IRQ);
-	sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
+	host->sdcard_irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK) &
+					~(i & TMIO_MASK_IRQ);
+	sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);
 }
 
 void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
 {
-	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) | (i & TMIO_MASK_IRQ);
-	sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
+	host->sdcard_irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK) |
+					(i & TMIO_MASK_IRQ);
+	sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);
 }
 
 static void tmio_mmc_ack_mmc_irqs(struct tmio_mmc_host *host, u32 i)
@@ -127,11 +129,13 @@ static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
 
 	if (enable) {
 		host->sdio_irq_enabled = 1;
+		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL &
+					~TMIO_SDIO_STAT_IOIRQ;
 		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001);
-		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK,
-			(TMIO_SDIO_MASK_ALL & ~TMIO_SDIO_STAT_IOIRQ));
+		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
 	} else {
-		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, TMIO_SDIO_MASK_ALL);
+		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
+		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
 		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0000);
 		host->sdio_irq_enabled = 0;
 	}
@@ -548,26 +552,26 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 	struct tmio_mmc_host *host = devid;
 	struct mmc_host *mmc = host->mmc;
 	struct tmio_mmc_data *pdata = host->pdata;
-	unsigned int ireg, irq_mask, status;
-	unsigned int sdio_ireg, sdio_irq_mask, sdio_status;
+	unsigned int ireg, status;
+	unsigned int sdio_ireg, sdio_status;
 
 	pr_debug("MMC IRQ begin\n");
 
 	status = sd_ctrl_read32(host, CTL_STATUS);
-	irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK);
-	ireg = status & TMIO_MASK_IRQ & ~irq_mask;
+	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
 
 	sdio_ireg = 0;
 	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
 		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
-		sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
-		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL & ~sdio_irq_mask;
+		host->sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
+		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
+				~host->sdio_irq_mask;
 
 		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
 
 		if (sdio_ireg && !host->sdio_irq_enabled) {
 			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
-				   sdio_status, sdio_irq_mask, sdio_ireg);
+				   sdio_status, host->sdio_irq_mask, sdio_ireg);
 			tmio_mmc_enable_sdio_irq(mmc, 0);
 			goto out;
 		}
@@ -623,9 +627,9 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 	}
 
 	pr_warning("tmio_mmc: Spurious irq, disabling! "
-		"0x%08x 0x%08x 0x%08x\n", status, irq_mask, ireg);
+		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
 	pr_debug_status(status);
-	tmio_mmc_disable_mmc_irqs(host, status & ~irq_mask);
+	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
 
 out:
 	return IRQ_HANDLED;
-- 
1.7.5.4

[PATCH 2/5] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

Provide separate interrupt handlers which may be used by platforms where
SDHI has three interrupt sources.

This involves two key changes to the logic:
1. Do not assume that only one interrupt has occurred.
   In particular because tmio_mmc_irq() handles interrupts
   from three sources. Also, because this allows
   the logic to be simplified.
2. Just ignore spurious interrupts.
   Its not clear to me that they can ever occur.

This patch also removes the commented-out handling of CRC and other errors.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

* SDCARD portion tested on AP4/Mackerel
* SDIO portion untested

v2
As suggested by Guennadi Liakhovetski
* Combine 3 patches into one
* Reduce the number of __tmio_..._irq() functions
* Rename "...card_access..." functions as "...sdcard..."
---
 drivers/mmc/host/tmio_mmc.h     |    3 +
 drivers/mmc/host/tmio_mmc_pio.c |  121 +++++++++++++++++++++++----------------
 2 files changed, 75 insertions(+), 49 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 1cf8db5..3020f98 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -97,6 +97,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host);
 void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
 void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
 irqreturn_t tmio_mmc_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid);
 
 static inline char *tmio_mmc_kmap_atomic(struct scatterlist *sg,
 					 unsigned long *flags)
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index c60b15f..e9640f2 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -547,45 +547,20 @@ out:
 	spin_unlock(&host->lock);
 }
 
-irqreturn_t tmio_mmc_irq(int irq, void *devid)
+static void tmio_mmc_card_irq_status(struct tmio_mmc_host *host,
+				       int *ireg, int *status)
 {
-	struct tmio_mmc_host *host = devid;
-	struct mmc_host *mmc = host->mmc;
-	struct tmio_mmc_data *pdata = host->pdata;
-	unsigned int ireg, status;
-	unsigned int sdio_ireg, sdio_status;
-
-	pr_debug("MMC IRQ begin\n");
-
-	status = sd_ctrl_read32(host, CTL_STATUS);
-	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
+	*status = sd_ctrl_read32(host, CTL_STATUS);
+	*ireg = *status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
 
-	sdio_ireg = 0;
-	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
-		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
-		host->sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
-		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
-				~host->sdio_irq_mask;
-
-		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
-
-		if (sdio_ireg && !host->sdio_irq_enabled) {
-			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
-				   sdio_status, host->sdio_irq_mask, sdio_ireg);
-			tmio_mmc_enable_sdio_irq(mmc, 0);
-			goto out;
-		}
-
-		if (mmc->caps & MMC_CAP_SDIO_IRQ &&
-			sdio_ireg & TMIO_SDIO_STAT_IOIRQ)
-			mmc_signal_sdio_irq(mmc);
-
-		if (sdio_ireg)
-			goto out;
-	}
+	pr_debug_status(*status);
+	pr_debug_status(*ireg);
+}
 
-	pr_debug_status(status);
-	pr_debug_status(ireg);
+static void __tmio_mmc_card_detect_irq(struct tmio_mmc_host *host,
+				       int ireg, int status)
+{
+	struct mmc_host *mmc = host->mmc;
 
 	/* Card insert / remove attempts */
 	if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
@@ -595,43 +570,91 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)) &&
 		    !work_pending(&mmc->detect.work))
 			mmc_detect_change(host->mmc, msecs_to_jiffies(100));
-		goto out;
 	}
+}
 
-	/* CRC and other errors */
-/*	if (ireg & TMIO_STAT_ERR_IRQ)
- *		handled |= tmio_error_irq(host, irq, stat);
- */
+irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid)
+{
+	unsigned int ireg, status;
+	struct tmio_mmc_host *host = devid;
+
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	__tmio_mmc_card_detect_irq(host, ireg, status);
 
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_card_detect_irq);
+
+void __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host, int ireg, int status)
+{
 	/* Command completion */
 	if (ireg & (TMIO_STAT_CMDRESPEND | TMIO_STAT_CMDTIMEOUT)) {
 		tmio_mmc_ack_mmc_irqs(host,
 			     TMIO_STAT_CMDRESPEND |
 			     TMIO_STAT_CMDTIMEOUT);
 		tmio_mmc_cmd_irq(host, status);
-		goto out;
 	}
 
 	/* Data transfer */
 	if (ireg & (TMIO_STAT_RXRDY | TMIO_STAT_TXRQ)) {
 		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_RXRDY | TMIO_STAT_TXRQ);
 		tmio_mmc_pio_irq(host);
-		goto out;
 	}
 
 	/* Data transfer completion */
 	if (ireg & TMIO_STAT_DATAEND) {
 		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND);
 		tmio_mmc_data_irq(host);
-		goto out;
 	}
+}
+
+irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid)
+{
+	unsigned int ireg, status;
+	struct tmio_mmc_host *host = devid;
 
-	pr_warning("tmio_mmc: Spurious irq, disabling! "
-		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
-	pr_debug_status(status);
-	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	__tmio_mmc_sdcard_irq(host, ireg, status);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_sdcard_irq);
+
+irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
+{
+	struct tmio_mmc_host *host = devid;
+	struct mmc_host *mmc = host->mmc;
+	struct tmio_mmc_data *pdata = host->pdata;
+	unsigned int ireg, status;
+
+	if (!(pdata->flags & TMIO_MMC_SDIO_IRQ))
+		return IRQ_HANDLED;
+
+	status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
+	ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdcard_irq_mask;
+
+	sd_ctrl_write16(host, CTL_SDIO_STATUS, status & ~TMIO_SDIO_MASK_ALL);
+
+	if (mmc->caps & MMC_CAP_SDIO_IRQ && ireg & TMIO_SDIO_STAT_IOIRQ)
+		mmc_signal_sdio_irq(mmc);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_sdio_irq);
+
+irqreturn_t tmio_mmc_irq(int irq, void *devid)
+{
+	struct tmio_mmc_host *host = devid;
+	unsigned int ireg, status;
+
+	pr_debug("MMC IRQ begin\n");
+
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	__tmio_mmc_card_detect_irq(host, ireg, status);
+	__tmio_mmc_sdcard_irq(host, ireg, status);
+
+	tmio_mmc_sdio_irq(irq, devid);
 
-out:
 	return IRQ_HANDLED;
 }
 EXPORT_SYMBOL(tmio_mmc_irq);
-- 
1.7.5.4

[PATCH 3/5] mmc: sdhi: Add defines for platform irq indexes

[Simon Horman]

This is intended to make it easier to correctly
order platform defines in platform data and
the logic that uses them.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 include/linux/mmc/sh_mobile_sdhi.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
index bd50b36..c66c58c 100644
--- a/include/linux/mmc/sh_mobile_sdhi.h
+++ b/include/linux/mmc/sh_mobile_sdhi.h
@@ -3,6 +3,10 @@
 
 #include <linux/types.h>
 
+#define SH_MOBILE_SDHI_IRQ_SDCARD	0
+#define SH_MOBILE_SDHI_IRQ_CARD_DETECT	1
+#define SH_MOBILE_SDHI_IRQ_SDIO		2
+
 struct platform_device;
 struct tmio_mmc_data;
 
-- 
1.7.5.4

[PATCH 4/5] mmc: sdhi: Make use of per-source irq handlers

[Simon Horman]

Make use of per-source irq handles if the
platform (data) has multiple irq sources.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 drivers/mmc/host/sh_mobile_sdhi.c |   58 +++++++++++++++++++++++-------------
 1 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 774f643..29dd0c8 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -96,7 +96,9 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	struct sh_mobile_sdhi_info *p = pdev->dev.platform_data;
 	struct tmio_mmc_host *host;
 	char clk_name[8];
-	int i, irq, ret;
+	int irq, ret;
+	irqreturn_t (*f)(int irq, void *devid);
+	bool multi_irq = false;
 
 	priv = kzalloc(sizeof(struct sh_mobile_sdhi), GFP_KERNEL);
 	if (priv == NULL) {
@@ -153,27 +155,33 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto eprobe;
 
-	for (i = 0; i < 3; i++) {
-		irq = platform_get_irq(pdev, i);
-		if (irq < 0) {
-			if (i) {
-				continue;
-			} else {
-				ret = irq;
-				goto eirq;
-			}
-		}
-		ret = request_irq(irq, tmio_mmc_irq, 0,
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
+	if (irq >= 0) {
+		multi_irq = true;
+		ret = request_irq(irq, tmio_mmc_sdio_irq, 0,
 				  dev_name(&pdev->dev), host);
-		if (ret) {
-			while (i--) {
-				irq = platform_get_irq(pdev, i);
-				if (irq >= 0)
-					free_irq(irq, host);
-			}
-			goto eirq;
-		}
+		if (ret)
+			goto eirq_sdio;
 	}
+
+	ret = irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_CARD_DETECT);
+	if (irq >= 0) {
+		multi_irq = true;
+		ret = request_irq(irq, tmio_mmc_sdcard_irq, 0,
+				  dev_name(&pdev->dev), host);
+		if (ret)
+			goto eirq_card_detect;
+	} else if (multi_irq)
+		goto eirq_card_detect;
+
+	ret = irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDCARD);
+	if (irq < 0)
+		goto eirq_sdcard;
+	f = multi_irq ? tmio_mmc_card_detect_irq : tmio_mmc_irq;
+	ret = request_irq(irq, f, 0, dev_name(&pdev->dev), host);
+	if (ret)
+		goto eirq_sdcard;
+
 	dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n",
 		 mmc_hostname(host->mmc), (unsigned long)
 		 (platform_get_resource(pdev,IORESOURCE_MEM, 0)->start),
@@ -181,7 +189,15 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 
 	return ret;
 
-eirq:
+eirq_sdcard:
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_CARD_DETECT);
+	if (irq >= 0)
+		free_irq(irq, host);
+eirq_card_detect:
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
+	if (irq >= 0)
+		free_irq(irq, host);
+eirq_sdio:
 	tmio_mmc_host_remove(host);
 eprobe:
 	clk_disable(priv->clk);
-- 
1.7.5.4

[PATCH 5/5] ARM: shmobile: ag5evm, ap4: Make use of irq index defines

[Simon Horman]

This is intended to make it easier to correctly order IRQs.

As suggested by Guennadi Liakhovetski.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Depends on "mmc: sdhi: Add defines for platform irq indexes"
---
 arch/arm/mach-shmobile/board-ag5evm.c   |   12 ++++++------
 arch/arm/mach-shmobile/board-mackerel.c |   18 +++++++++---------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
index ce5c251..c687f67 100644
--- a/arch/arm/mach-shmobile/board-ag5evm.c
+++ b/arch/arm/mach-shmobile/board-ag5evm.c
@@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
 		.end	= 0xee1000ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= gic_spi(83),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= gic_spi(84),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= gic_spi(85),
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -395,15 +395,15 @@ static struct resource sdhi1_resources[] = {
 		.end	= 0xee1200ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= gic_spi(87),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= gic_spi(88),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= gic_spi(89),
 		.flags	= IORESOURCE_IRQ,
 	},
diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index d41c01f..bc575eb 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -1022,15 +1022,15 @@ static struct resource sdhi0_resources[] = {
 		.end	= 0xe68500ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x0e00) /* SDHI0_SDHI0I0 */,
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x0e20) /* SDHI0_SDHI0I1 */,
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x0e40) /* SDHI0_SDHI0I2 */,
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -1065,15 +1065,15 @@ static struct resource sdhi1_resources[] = {
 		.end	= 0xe68600ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x0e80), /* SDHI1_SDHI1I0 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x0ea0), /* SDHI1_SDHI1I1 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x0ec0), /* SDHI1_SDHI1I2 */
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -1116,15 +1116,15 @@ static struct resource sdhi2_resources[] = {
 		.end	= 0xe68700ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x1200), /* SDHI2_SDHI2I0 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x1220), /* SDHI2_SDHI2I1 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x1240), /* SDHI2_SDHI2I2 */
 		.flags	= IORESOURCE_IRQ,
 	},
-- 
1.7.5.4

Re: [PATCH 1/5] mmc: tmio: Cache interrupt masks

[Guennadi Liakhovetski]

On Tue, 16 Aug 2011, Simon Horman wrote:

> This avoids the need to look up the masks each time an interrupt
> is handled.

Yes, almost... But I think, we can use the mask-caches even more 
extensively. In your patch you actually hardly gain anything, you continue 
reading the mask register instead of using the cache. Namely:

> 
> As suggested by Guennadi.
> 
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
> 
> * SDCARD portion tested on AP4/Mackerel
> * SDIO portion untested
> ---
>  drivers/mmc/host/tmio_mmc.h     |    4 ++++
>  drivers/mmc/host/tmio_mmc_pio.c |   36 ++++++++++++++++++++----------------
>  2 files changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index eeaf643..1cf8db5 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -79,6 +79,10 @@ struct tmio_mmc_host {
>  	struct delayed_work	delayed_reset_work;
>  	struct work_struct	done;
>  
> +	/* Cache IRQ mask */
> +	u32			sdcard_irq_mask;
> +	u32			sdio_irq_mask;
> +
>  	spinlock_t		lock;		/* protect host private data */
>  	unsigned long		last_req_ts;
>  	struct mutex		ios_lock;	/* protect set_ios() context */
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 1f16357..c60b15f 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -48,14 +48,16 @@
>  
>  void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
>  {
> -	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) & ~(i & TMIO_MASK_IRQ);
> -	sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
> +	host->sdcard_irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK) &
> +					~(i & TMIO_MASK_IRQ);
> +	sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);

This function is used often - from each command and from the ISR. Don't 
re-read the mask register, use the cached value.

>  }
>  
>  void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
>  {
> -	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) | (i & TMIO_MASK_IRQ);
> -	sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
> +	host->sdcard_irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK) |
> +					(i & TMIO_MASK_IRQ);
> +	sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);

ditto

>  }
>  
>  static void tmio_mmc_ack_mmc_irqs(struct tmio_mmc_host *host, u32 i)
> @@ -127,11 +129,13 @@ static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>  
>  	if (enable) {
>  		host->sdio_irq_enabled = 1;
> +		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL &
> +					~TMIO_SDIO_STAT_IOIRQ;
>  		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001);
> -		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK,
> -			(TMIO_SDIO_MASK_ALL & ~TMIO_SDIO_STAT_IOIRQ));
> +		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
>  	} else {
> -		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, TMIO_SDIO_MASK_ALL);
> +		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
> +		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
>  		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0000);
>  		host->sdio_irq_enabled = 0;
>  	}
> @@ -548,26 +552,26 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
>  	struct tmio_mmc_host *host = devid;
>  	struct mmc_host *mmc = host->mmc;
>  	struct tmio_mmc_data *pdata = host->pdata;
> -	unsigned int ireg, irq_mask, status;
> -	unsigned int sdio_ireg, sdio_irq_mask, sdio_status;
> +	unsigned int ireg, status;
> +	unsigned int sdio_ireg, sdio_status;
>  
>  	pr_debug("MMC IRQ begin\n");
>  
>  	status = sd_ctrl_read32(host, CTL_STATUS);
> -	irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK);
> -	ireg = status & TMIO_MASK_IRQ & ~irq_mask;
> +	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
>  
>  	sdio_ireg = 0;
>  	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
>  		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
> -		sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
> -		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL & ~sdio_irq_mask;
> +		host->sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);

Ditto - you're in ISR

> +		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
> +				~host->sdio_irq_mask;
>  
>  		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
>  
>  		if (sdio_ireg && !host->sdio_irq_enabled) {
>  			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
> -				   sdio_status, sdio_irq_mask, sdio_ireg);
> +				   sdio_status, host->sdio_irq_mask, sdio_ireg);
>  			tmio_mmc_enable_sdio_irq(mmc, 0);
>  			goto out;
>  		}
> @@ -623,9 +627,9 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
>  	}
>  
>  	pr_warning("tmio_mmc: Spurious irq, disabling! "
> -		"0x%08x 0x%08x 0x%08x\n", status, irq_mask, ireg);
> +		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
>  	pr_debug_status(status);
> -	tmio_mmc_disable_mmc_irqs(host, status & ~irq_mask);
> +	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
>  
>  out:
>  	return IRQ_HANDLED;
> -- 
> 1.7.5.4

Instead, what I thought would be a good idea is to initialise the 
.irq_mask and .sdio_irq_mask once in tmio_mmc_host_probe() before calling 
tmio_mmc_disable_mmc_irqs() and then never read CTL_IRQ_MASK and 
CTL_SDIO_IRQ_MASK again - ever!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/5] mmc: tmio: Provide separate interrupt handlers

[Guennadi Liakhovetski]

On Tue, 16 Aug 2011, Simon Horman wrote:

> Provide separate interrupt handlers which may be used by platforms where
> SDHI has three interrupt sources.

Yes, this looks much simpler and cleaner to me now, than 3 original 
patches. It might change a bit after you change your PATCH 1/5, 

> 
> This involves two key changes to the logic:
> 1. Do not assume that only one interrupt has occurred.
>    In particular because tmio_mmc_irq() handles interrupts
>    from three sources. Also, because this allows
>    the logic to be simplified.
> 2. Just ignore spurious interrupts.
>    Its not clear to me that they can ever occur.
> 
> This patch also removes the commented-out handling of CRC and other errors.
> 
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
> 
> * SDCARD portion tested on AP4/Mackerel
> * SDIO portion untested
> 
> v2
> As suggested by Guennadi Liakhovetski
> * Combine 3 patches into one
> * Reduce the number of __tmio_..._irq() functions
> * Rename "...card_access..." functions as "...sdcard..."
> ---
>  drivers/mmc/host/tmio_mmc.h     |    3 +
>  drivers/mmc/host/tmio_mmc_pio.c |  121 +++++++++++++++++++++++----------------
>  2 files changed, 75 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 1cf8db5..3020f98 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -97,6 +97,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host);
>  void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
>  void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
>  irqreturn_t tmio_mmc_irq(int irq, void *devid);
> +irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid);
> +irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid);
> +irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid);
>  
>  static inline char *tmio_mmc_kmap_atomic(struct scatterlist *sg,
>  					 unsigned long *flags)
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index c60b15f..e9640f2 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -547,45 +547,20 @@ out:
>  	spin_unlock(&host->lock);
>  }
>  
> -irqreturn_t tmio_mmc_irq(int irq, void *devid)
> +static void tmio_mmc_card_irq_status(struct tmio_mmc_host *host,
> +				       int *ireg, int *status)
>  {
> -	struct tmio_mmc_host *host = devid;
> -	struct mmc_host *mmc = host->mmc;
> -	struct tmio_mmc_data *pdata = host->pdata;
> -	unsigned int ireg, status;
> -	unsigned int sdio_ireg, sdio_status;
> -
> -	pr_debug("MMC IRQ begin\n");
> -
> -	status = sd_ctrl_read32(host, CTL_STATUS);
> -	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
> +	*status = sd_ctrl_read32(host, CTL_STATUS);
> +	*ireg = *status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
>  
> -	sdio_ireg = 0;
> -	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
> -		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
> -		host->sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
> -		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
> -				~host->sdio_irq_mask;
> -
> -		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
> -
> -		if (sdio_ireg && !host->sdio_irq_enabled) {
> -			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
> -				   sdio_status, host->sdio_irq_mask, sdio_ireg);
> -			tmio_mmc_enable_sdio_irq(mmc, 0);
> -			goto out;
> -		}
> -
> -		if (mmc->caps & MMC_CAP_SDIO_IRQ &&
> -			sdio_ireg & TMIO_SDIO_STAT_IOIRQ)
> -			mmc_signal_sdio_irq(mmc);
> -
> -		if (sdio_ireg)
> -			goto out;
> -	}
> +	pr_debug_status(*status);
> +	pr_debug_status(*ireg);
> +}
>  
> -	pr_debug_status(status);
> -	pr_debug_status(ireg);
> +static void __tmio_mmc_card_detect_irq(struct tmio_mmc_host *host,
> +				       int ireg, int status)
> +{
> +	struct mmc_host *mmc = host->mmc;
>  
>  	/* Card insert / remove attempts */
>  	if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
> @@ -595,43 +570,91 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
>  		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)) &&
>  		    !work_pending(&mmc->detect.work))
>  			mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> -		goto out;
>  	}
> +}
>  
> -	/* CRC and other errors */
> -/*	if (ireg & TMIO_STAT_ERR_IRQ)
> - *		handled |= tmio_error_irq(host, irq, stat);
> - */
> +irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid)
> +{
> +	unsigned int ireg, status;
> +	struct tmio_mmc_host *host = devid;
> +
> +	tmio_mmc_card_irq_status(host, &ireg, &status);
> +	__tmio_mmc_card_detect_irq(host, ireg, status);
>  
> +	return IRQ_HANDLED;
> +}
> +EXPORT_SYMBOL(tmio_mmc_card_detect_irq);
> +
> +void __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host, int ireg, int status)

"static" missing

> +{
>  	/* Command completion */
>  	if (ireg & (TMIO_STAT_CMDRESPEND | TMIO_STAT_CMDTIMEOUT)) {
>  		tmio_mmc_ack_mmc_irqs(host,
>  			     TMIO_STAT_CMDRESPEND |
>  			     TMIO_STAT_CMDTIMEOUT);
>  		tmio_mmc_cmd_irq(host, status);
> -		goto out;

Well, removing these goto's certainly changes behaviour - at least 
theoretically. Imagine entering the ISR with status = TMIO_STAT_CMDRESPEND 
| TMIO_STAT_DATAEND. Before your patch the driver would ack and process 
the TMIO_STAT_CMDRESPEND and return dropping the TMIO_STAT_DATAEND status. 
Anc we don't know whether a second interrupt would follow with 
TMIO_STAT_DATAEND set. After your patch the driver will ack and process 
both. Since this driver runs on a variaty of hardware, unless fixing a 
problem or really improving something, I would keep the original 
behaviour. So, I would rather keep those exit points - just put "return" 
there.

>  	}
>  
>  	/* Data transfer */
>  	if (ireg & (TMIO_STAT_RXRDY | TMIO_STAT_TXRQ)) {
>  		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_RXRDY | TMIO_STAT_TXRQ);
>  		tmio_mmc_pio_irq(host);
> -		goto out;
>  	}
>  
>  	/* Data transfer completion */
>  	if (ireg & TMIO_STAT_DATAEND) {
>  		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND);
>  		tmio_mmc_data_irq(host);
> -		goto out;
>  	}
> +}
> +
> +irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid)
> +{
> +	unsigned int ireg, status;
> +	struct tmio_mmc_host *host = devid;
>  
> -	pr_warning("tmio_mmc: Spurious irq, disabling! "
> -		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
> -	pr_debug_status(status);
> -	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
> +	tmio_mmc_card_irq_status(host, &ireg, &status);
> +	__tmio_mmc_sdcard_irq(host, ireg, status);
> +
> +	return IRQ_HANDLED;
> +}
> +EXPORT_SYMBOL(tmio_mmc_sdcard_irq);
> +
> +irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
> +{
> +	struct tmio_mmc_host *host = devid;
> +	struct mmc_host *mmc = host->mmc;
> +	struct tmio_mmc_data *pdata = host->pdata;
> +	unsigned int ireg, status;
> +
> +	if (!(pdata->flags & TMIO_MMC_SDIO_IRQ))
> +		return IRQ_HANDLED;
> +
> +	status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
> +	ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdcard_irq_mask;
> +
> +	sd_ctrl_write16(host, CTL_SDIO_STATUS, status & ~TMIO_SDIO_MASK_ALL);
> +
> +	if (mmc->caps & MMC_CAP_SDIO_IRQ && ireg & TMIO_SDIO_STAT_IOIRQ)
> +		mmc_signal_sdio_irq(mmc);
> +
> +	return IRQ_HANDLED;
> +}
> +EXPORT_SYMBOL(tmio_mmc_sdio_irq);
> +
> +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> +{
> +	struct tmio_mmc_host *host = devid;
> +	unsigned int ireg, status;
> +
> +	pr_debug("MMC IRQ begin\n");
> +
> +	tmio_mmc_card_irq_status(host, &ireg, &status);
> +	__tmio_mmc_card_detect_irq(host, ireg, status);

Same here - I would return, if a card hot-plug event occurred.

> +	__tmio_mmc_sdcard_irq(host, ireg, status);

Ditto

> +
> +	tmio_mmc_sdio_irq(irq, devid);

Any specific reason, why you now process SDIO IRQs last?

>  
> -out:
>  	return IRQ_HANDLED;
>  }
>  EXPORT_SYMBOL(tmio_mmc_irq);
> -- 
> 1.7.5.4

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 3/5] mmc: sdhi: Add defines for platform irq indexes

[Guennadi Liakhovetski]

On Tue, 16 Aug 2011, Simon Horman wrote:

> This is intended to make it easier to correctly
> order platform defines in platform data and
> the logic that uses them.
> 
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Paul Mundt <lethal@linux-sh.org>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> ---
>  include/linux/mmc/sh_mobile_sdhi.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
> index bd50b36..c66c58c 100644
> --- a/include/linux/mmc/sh_mobile_sdhi.h
> +++ b/include/linux/mmc/sh_mobile_sdhi.h
> @@ -3,6 +3,10 @@
>  
>  #include <linux/types.h>
>  
> +#define SH_MOBILE_SDHI_IRQ_SDCARD	0
> +#define SH_MOBILE_SDHI_IRQ_CARD_DETECT	1
> +#define SH_MOBILE_SDHI_IRQ_SDIO		2
> +

An idea: make this an enum, put SH_MOBILE_SDHI_IRQ_NUMBER as a last item 
and use it in sh_mobile_sdhi_remove() as

	for (i = 0; i < SH_MOBILE_SDHI_IRQ_NUMBER; i++) {

? I would also merge it with PATCH 4/5.

>  struct platform_device;
>  struct tmio_mmc_data;
>  
> -- 
> 1.7.5.4

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/5] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

On Tue, Aug 16, 2011 at 09:41:52AM +0200, Guennadi Liakhovetski wrote:
> On Tue, 16 Aug 2011, Simon Horman wrote:
> 
> > Provide separate interrupt handlers which may be used by platforms where
> > SDHI has three interrupt sources.
> 
> Yes, this looks much simpler and cleaner to me now, than 3 original 
> patches. It might change a bit after you change your PATCH 1/5, 
> 
> > 
> > This involves two key changes to the logic:
> > 1. Do not assume that only one interrupt has occurred.
> >    In particular because tmio_mmc_irq() handles interrupts
> >    from three sources. Also, because this allows
> >    the logic to be simplified.
> > 2. Just ignore spurious interrupts.
> >    Its not clear to me that they can ever occur.
> > 
> > This patch also removes the commented-out handling of CRC and other errors.
> > 
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> > 
> > ---
> > 
> > * SDCARD portion tested on AP4/Mackerel
> > * SDIO portion untested
> > 
> > v2
> > As suggested by Guennadi Liakhovetski
> > * Combine 3 patches into one
> > * Reduce the number of __tmio_..._irq() functions
> > * Rename "...card_access..." functions as "...sdcard..."
> > ---
> >  drivers/mmc/host/tmio_mmc.h     |    3 +
> >  drivers/mmc/host/tmio_mmc_pio.c |  121 +++++++++++++++++++++++----------------
> >  2 files changed, 75 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> > index 1cf8db5..3020f98 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -97,6 +97,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host);
> >  void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
> >  void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
> >  irqreturn_t tmio_mmc_irq(int irq, void *devid);
> > +irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid);
> > +irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid);
> > +irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid);
> >  
> >  static inline char *tmio_mmc_kmap_atomic(struct scatterlist *sg,
> >  					 unsigned long *flags)
> > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> > index c60b15f..e9640f2 100644
> > --- a/drivers/mmc/host/tmio_mmc_pio.c
> > +++ b/drivers/mmc/host/tmio_mmc_pio.c
> > @@ -547,45 +547,20 @@ out:
> >  	spin_unlock(&host->lock);
> >  }
> >  
> > -irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > +static void tmio_mmc_card_irq_status(struct tmio_mmc_host *host,
> > +				       int *ireg, int *status)
> >  {
> > -	struct tmio_mmc_host *host = devid;
> > -	struct mmc_host *mmc = host->mmc;
> > -	struct tmio_mmc_data *pdata = host->pdata;
> > -	unsigned int ireg, status;
> > -	unsigned int sdio_ireg, sdio_status;
> > -
> > -	pr_debug("MMC IRQ begin\n");
> > -
> > -	status = sd_ctrl_read32(host, CTL_STATUS);
> > -	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
> > +	*status = sd_ctrl_read32(host, CTL_STATUS);
> > +	*ireg = *status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
> >  
> > -	sdio_ireg = 0;
> > -	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
> > -		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
> > -		host->sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
> > -		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
> > -				~host->sdio_irq_mask;
> > -
> > -		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
> > -
> > -		if (sdio_ireg && !host->sdio_irq_enabled) {
> > -			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
> > -				   sdio_status, host->sdio_irq_mask, sdio_ireg);
> > -			tmio_mmc_enable_sdio_irq(mmc, 0);
> > -			goto out;
> > -		}
> > -
> > -		if (mmc->caps & MMC_CAP_SDIO_IRQ &&
> > -			sdio_ireg & TMIO_SDIO_STAT_IOIRQ)
> > -			mmc_signal_sdio_irq(mmc);
> > -
> > -		if (sdio_ireg)
> > -			goto out;
> > -	}
> > +	pr_debug_status(*status);
> > +	pr_debug_status(*ireg);
> > +}
> >  
> > -	pr_debug_status(status);
> > -	pr_debug_status(ireg);
> > +static void __tmio_mmc_card_detect_irq(struct tmio_mmc_host *host,
> > +				       int ireg, int status)
> > +{
> > +	struct mmc_host *mmc = host->mmc;
> >  
> >  	/* Card insert / remove attempts */
> >  	if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
> > @@ -595,43 +570,91 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> >  		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)) &&
> >  		    !work_pending(&mmc->detect.work))
> >  			mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > -		goto out;
> >  	}
> > +}
> >  
> > -	/* CRC and other errors */
> > -/*	if (ireg & TMIO_STAT_ERR_IRQ)
> > - *		handled |= tmio_error_irq(host, irq, stat);
> > - */
> > +irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid)
> > +{
> > +	unsigned int ireg, status;
> > +	struct tmio_mmc_host *host = devid;
> > +
> > +	tmio_mmc_card_irq_status(host, &ireg, &status);
> > +	__tmio_mmc_card_detect_irq(host, ireg, status);
> >  
> > +	return IRQ_HANDLED;
> > +}
> > +EXPORT_SYMBOL(tmio_mmc_card_detect_irq);
> > +
> > +void __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host, int ireg, int status)
> 
> "static" missing
> 
> > +{
> >  	/* Command completion */
> >  	if (ireg & (TMIO_STAT_CMDRESPEND | TMIO_STAT_CMDTIMEOUT)) {
> >  		tmio_mmc_ack_mmc_irqs(host,
> >  			     TMIO_STAT_CMDRESPEND |
> >  			     TMIO_STAT_CMDTIMEOUT);
> >  		tmio_mmc_cmd_irq(host, status);
> > -		goto out;
> 
> Well, removing these goto's certainly changes behaviour - at least 
> theoretically. Imagine entering the ISR with status = TMIO_STAT_CMDRESPEND 
> | TMIO_STAT_DATAEND. Before your patch the driver would ack and process 
> the TMIO_STAT_CMDRESPEND and return dropping the TMIO_STAT_DATAEND status. 
> Anc we don't know whether a second interrupt would follow with 
> TMIO_STAT_DATAEND set. After your patch the driver will ack and process 
> both. Since this driver runs on a variaty of hardware, unless fixing a 
> problem or really improving something, I would keep the original 
> behaviour. So, I would rather keep those exit points - just put "return" 
> there.

I specifically noted this change in the changelog, sorry if that wasn't
clear enough.

I will revert the behaviour as you suggest, although I suspect
that the new behaviour is correct.

> >  	}
> >  
> >  	/* Data transfer */
> >  	if (ireg & (TMIO_STAT_RXRDY | TMIO_STAT_TXRQ)) {
> >  		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_RXRDY | TMIO_STAT_TXRQ);
> >  		tmio_mmc_pio_irq(host);
> > -		goto out;
> >  	}
> >  
> >  	/* Data transfer completion */
> >  	if (ireg & TMIO_STAT_DATAEND) {
> >  		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND);
> >  		tmio_mmc_data_irq(host);
> > -		goto out;
> >  	}
> > +}
> > +
> > +irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid)
> > +{
> > +	unsigned int ireg, status;
> > +	struct tmio_mmc_host *host = devid;
> >  
> > -	pr_warning("tmio_mmc: Spurious irq, disabling! "
> > -		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
> > -	pr_debug_status(status);
> > -	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
> > +	tmio_mmc_card_irq_status(host, &ireg, &status);
> > +	__tmio_mmc_sdcard_irq(host, ireg, status);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +EXPORT_SYMBOL(tmio_mmc_sdcard_irq);
> > +
> > +irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
> > +{
> > +	struct tmio_mmc_host *host = devid;
> > +	struct mmc_host *mmc = host->mmc;
> > +	struct tmio_mmc_data *pdata = host->pdata;
> > +	unsigned int ireg, status;
> > +
> > +	if (!(pdata->flags & TMIO_MMC_SDIO_IRQ))
> > +		return IRQ_HANDLED;
> > +
> > +	status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
> > +	ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdcard_irq_mask;
> > +
> > +	sd_ctrl_write16(host, CTL_SDIO_STATUS, status & ~TMIO_SDIO_MASK_ALL);
> > +
> > +	if (mmc->caps & MMC_CAP_SDIO_IRQ && ireg & TMIO_SDIO_STAT_IOIRQ)
> > +		mmc_signal_sdio_irq(mmc);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +EXPORT_SYMBOL(tmio_mmc_sdio_irq);
> > +
> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > +{
> > +	struct tmio_mmc_host *host = devid;
> > +	unsigned int ireg, status;
> > +
> > +	pr_debug("MMC IRQ begin\n");
> > +
> > +	tmio_mmc_card_irq_status(host, &ireg, &status);
> > +	__tmio_mmc_card_detect_irq(host, ireg, status);
> 
> Same here - I would return, if a card hot-plug event occurred.

Will do.

> > +	__tmio_mmc_sdcard_irq(host, ireg, status);
> 
> Ditto
> 
> > +
> > +	tmio_mmc_sdio_irq(irq, devid);
> 
> Any specific reason, why you now process SDIO IRQs last?

I believe this is in keeping with the ordering implied by original code.

> >  
> > -out:
> >  	return IRQ_HANDLED;
> >  }
> >  EXPORT_SYMBOL(tmio_mmc_irq);
> > -- 
> > 1.7.5.4
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/5] mmc: sdhi: Add defines for platform irq indexes

[Simon Horman]

On Tue, Aug 16, 2011 at 09:51:00AM +0200, Guennadi Liakhovetski wrote:
> On Tue, 16 Aug 2011, Simon Horman wrote:
> 
> > This is intended to make it easier to correctly
> > order platform defines in platform data and
> > the logic that uses them.
> > 
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Cc: Paul Mundt <lethal@linux-sh.org>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> > ---
> >  include/linux/mmc/sh_mobile_sdhi.h |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
> > index bd50b36..c66c58c 100644
> > --- a/include/linux/mmc/sh_mobile_sdhi.h
> > +++ b/include/linux/mmc/sh_mobile_sdhi.h
> > @@ -3,6 +3,10 @@
> >  
> >  #include <linux/types.h>
> >  
> > +#define SH_MOBILE_SDHI_IRQ_SDCARD	0
> > +#define SH_MOBILE_SDHI_IRQ_CARD_DETECT	1
> > +#define SH_MOBILE_SDHI_IRQ_SDIO		2
> > +
> 
> An idea: make this an enum, put SH_MOBILE_SDHI_IRQ_NUMBER as a last item 
> and use it in sh_mobile_sdhi_remove() as
> 
> 	for (i = 0; i < SH_MOBILE_SDHI_IRQ_NUMBER; i++) {

Sure.

> ? I would also merge it with PATCH 4/5.

I prefer to keep it separate as 5/5 also depends on this change
and that is a change for a different maintainer.

> >  struct platform_device;
> >  struct tmio_mmc_data;
> >  
> > -- 
> > 1.7.5.4
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/5] mmc: tmio: Cache interrupt masks

[Simon Horman]

On Tue, Aug 16, 2011 at 09:19:12AM +0200, Guennadi Liakhovetski wrote:
> On Tue, 16 Aug 2011, Simon Horman wrote:
> 
> > This avoids the need to look up the masks each time an interrupt
> > is handled.
> 
> Yes, almost... But I think, we can use the mask-caches even more 
> extensively. In your patch you actually hardly gain anything, you continue 
> reading the mask register instead of using the cache. Namely:

Sure.

Re: [PATCH 4/5] mmc: sdhi: Make use of per-source irq handlers

[Guennadi Liakhovetski]

On Tue, 16 Aug 2011, Simon Horman wrote:

> Make use of per-source irq handles if the
> platform (data) has multiple irq sources.
> 
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> ---
>  drivers/mmc/host/sh_mobile_sdhi.c |   58 +++++++++++++++++++++++-------------
>  1 files changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 774f643..29dd0c8 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -96,7 +96,9 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
>  	struct sh_mobile_sdhi_info *p = pdev->dev.platform_data;
>  	struct tmio_mmc_host *host;
>  	char clk_name[8];
> -	int i, irq, ret;
> +	int irq, ret;
> +	irqreturn_t (*f)(int irq, void *devid);
> +	bool multi_irq = false;
>  
>  	priv = kzalloc(sizeof(struct sh_mobile_sdhi), GFP_KERNEL);
>  	if (priv == NULL) {
> @@ -153,27 +155,33 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto eprobe;
>  
> -	for (i = 0; i < 3; i++) {
> -		irq = platform_get_irq(pdev, i);
> -		if (irq < 0) {
> -			if (i) {
> -				continue;
> -			} else {
> -				ret = irq;
> -				goto eirq;
> -			}
> -		}
> -		ret = request_irq(irq, tmio_mmc_irq, 0,
> +	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
> +	if (irq >= 0) {
> +		multi_irq = true;
> +		ret = request_irq(irq, tmio_mmc_sdio_irq, 0,
>  				  dev_name(&pdev->dev), host);
> -		if (ret) {
> -			while (i--) {
> -				irq = platform_get_irq(pdev, i);
> -				if (irq >= 0)
> -					free_irq(irq, host);
> -			}
> -			goto eirq;
> -		}
> +		if (ret)
> +			goto eirq_sdio;
>  	}
> +
> +	ret = irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_CARD_DETECT);
> +	if (irq >= 0) {
> +		multi_irq = true;
> +		ret = request_irq(irq, tmio_mmc_sdcard_irq, 0,

Typo? You're checking card-detect and requesting sdcard_irq?

> +				  dev_name(&pdev->dev), host);
> +		if (ret)
> +			goto eirq_card_detect;
> +	} else if (multi_irq)
> +		goto eirq_card_detect;

Currently if any of SDIO or card-detect IRQs are missing in platform data 
the driver probing will just ignore them. Also for the case, when an SDIO 
IRQ is present and no card-detect IRQ is provided. Currently all platforms 
either only provide one interrupt, or all 3 of them, so, on them your 
version would work too, but isn't it possible, that a new platform will 
want to drop one of those "optional" IRQs? Actually, I am not sure what we 
want to do in such a partial case. Say, if the card-detect IRQ is missing. 
Do we then register the SDIO and SD-card IRQs with their respective 
handlers and drop card-detection completely (polling) or do we use the 
all-in-one ISR for the SD-card IRQ then? I think, the former would be more 
logical. I.e., if the platform provided > 1 IRQs, only request respective 
specialised ISRs and drop the rest. In any case erroring out as you do 
above seems wrong.

> +
> +	ret = irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDCARD);
> +	if (irq < 0)
> +		goto eirq_sdcard;
> +	f = multi_irq ? tmio_mmc_card_detect_irq : tmio_mmc_irq;

Also a typo? If your tests were successful (with card hot-plugging), then 
probably you swapped your macro values.

> +	ret = request_irq(irq, f, 0, dev_name(&pdev->dev), host);
> +	if (ret)
> +		goto eirq_sdcard;
> +
>  	dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n",
>  		 mmc_hostname(host->mmc), (unsigned long)
>  		 (platform_get_resource(pdev,IORESOURCE_MEM, 0)->start),
> @@ -181,7 +189,15 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
>  
>  	return ret;
>  
> -eirq:
> +eirq_sdcard:
> +	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_CARD_DETECT);
> +	if (irq >= 0)
> +		free_irq(irq, host);
> +eirq_card_detect:
> +	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
> +	if (irq >= 0)
> +		free_irq(irq, host);
> +eirq_sdio:
>  	tmio_mmc_host_remove(host);
>  eprobe:
>  	clk_disable(priv->clk);
> -- 
> 1.7.5.4

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 3/5] mmc: sdhi: Add defines for platform irq indexes

[Guennadi Liakhovetski]

On Tue, 16 Aug 2011, Simon Horman wrote:

> On Tue, Aug 16, 2011 at 09:51:00AM +0200, Guennadi Liakhovetski wrote:
> > On Tue, 16 Aug 2011, Simon Horman wrote:
> > 
> > > This is intended to make it easier to correctly
> > > order platform defines in platform data and
> > > the logic that uses them.
> > > 
> > > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > Cc: Magnus Damm <magnus.damm@gmail.com>
> > > Cc: Paul Mundt <lethal@linux-sh.org>
> > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > > ---
> > >  include/linux/mmc/sh_mobile_sdhi.h |    4 ++++
> > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
> > > index bd50b36..c66c58c 100644
> > > --- a/include/linux/mmc/sh_mobile_sdhi.h
> > > +++ b/include/linux/mmc/sh_mobile_sdhi.h
> > > @@ -3,6 +3,10 @@
> > >  
> > >  #include <linux/types.h>
> > >  
> > > +#define SH_MOBILE_SDHI_IRQ_SDCARD	0
> > > +#define SH_MOBILE_SDHI_IRQ_CARD_DETECT	1
> > > +#define SH_MOBILE_SDHI_IRQ_SDIO		2
> > > +
> > 
> > An idea: make this an enum, put SH_MOBILE_SDHI_IRQ_NUMBER as a last item 
> > and use it in sh_mobile_sdhi_remove() as
> > 
> > 	for (i = 0; i < SH_MOBILE_SDHI_IRQ_NUMBER; i++) {
> 
> Sure.
> 
> > ? I would also merge it with PATCH 4/5.
> 
> I prefer to keep it separate as 5/5 also depends on this change
> and that is a change for a different maintainer.

Yes, 5/5 anyway depends on 3/5 in your case and they are for different 
maintainers (or need an ack), and, in fact, 5/5 can also be extended to 
more mach-shmobile boards, and that is not urgent, it can be done any time 
later. Whereas 3/5 and 4/5 anyway go via one tree and I see no advantage 
in splitting them. But it's up to you in the end - not a big deal.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 3/5] mmc: sdhi: Add defines for platform irq indexes

[Simon Horman]

On Tue, Aug 16, 2011 at 10:34:22AM +0200, Guennadi Liakhovetski wrote:
> On Tue, 16 Aug 2011, Simon Horman wrote:
> 
> > On Tue, Aug 16, 2011 at 09:51:00AM +0200, Guennadi Liakhovetski wrote:
> > > On Tue, 16 Aug 2011, Simon Horman wrote:
> > > 
> > > > This is intended to make it easier to correctly
> > > > order platform defines in platform data and
> > > > the logic that uses them.
> > > > 
> > > > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > Cc: Magnus Damm <magnus.damm@gmail.com>
> > > > Cc: Paul Mundt <lethal@linux-sh.org>
> > > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > > > ---
> > > >  include/linux/mmc/sh_mobile_sdhi.h |    4 ++++
> > > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
> > > > index bd50b36..c66c58c 100644
> > > > --- a/include/linux/mmc/sh_mobile_sdhi.h
> > > > +++ b/include/linux/mmc/sh_mobile_sdhi.h
> > > > @@ -3,6 +3,10 @@
> > > >  
> > > >  #include <linux/types.h>
> > > >  
> > > > +#define SH_MOBILE_SDHI_IRQ_SDCARD	0
> > > > +#define SH_MOBILE_SDHI_IRQ_CARD_DETECT	1
> > > > +#define SH_MOBILE_SDHI_IRQ_SDIO		2
> > > > +
> > > 
> > > An idea: make this an enum, put SH_MOBILE_SDHI_IRQ_NUMBER as a last item 
> > > and use it in sh_mobile_sdhi_remove() as
> > > 
> > > 	for (i = 0; i < SH_MOBILE_SDHI_IRQ_NUMBER; i++) {
> > 
> > Sure.
> > 
> > > ? I would also merge it with PATCH 4/5.
> > 
> > I prefer to keep it separate as 5/5 also depends on this change
> > and that is a change for a different maintainer.
> 
> Yes, 5/5 anyway depends on 3/5 in your case and they are for different 
> maintainers (or need an ack), and, in fact, 5/5 can also be extended to 
> more mach-shmobile boards, and that is not urgent, it can be done any time 
> later. Whereas 3/5 and 4/5 anyway go via one tree and I see no advantage 
> in splitting them. But it's up to you in the end - not a big deal.

I reconsidered and have decided to merge 3/5 and 4/5 as you suggest.

Re: [PATCH 2/5] mmc: tmio: Provide separate interrupt handlers

[Guennadi Liakhovetski]

On Tue, 16 Aug 2011, Simon Horman wrote:

> On Tue, Aug 16, 2011 at 09:41:52AM +0200, Guennadi Liakhovetski wrote:
> > On Tue, 16 Aug 2011, Simon Horman wrote:

[snip]

> > > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > > +{
> > > +	struct tmio_mmc_host *host = devid;
> > > +	unsigned int ireg, status;
> > > +
> > > +	pr_debug("MMC IRQ begin\n");
> > > +
> > > +	tmio_mmc_card_irq_status(host, &ireg, &status);
> > > +	__tmio_mmc_card_detect_irq(host, ireg, status);
> > 
> > Same here - I would return, if a card hot-plug event occurred.
> 
> Will do.
> 
> > > +	__tmio_mmc_sdcard_irq(host, ireg, status);
> > 
> > Ditto
> > 
> > > +
> > > +	tmio_mmc_sdio_irq(irq, devid);
> > 
> > Any specific reason, why you now process SDIO IRQs last?
> 
> I believe this is in keeping with the ordering implied by original code.

I believe it's not. The original version did SDIO first, then hotplug, 
then normal IO. I'm not necessarily saying, that the original code was 
correct or better, I'm just saying, it was different. As for which one we 
should prefer... I think, I'd check for hotplug first: if the card is 
removed, no reason to try to communicate with it. And we have to first 
report a new card, before reporting any IRQs from it. But then - IO or 
SDIO as second? Well, since SDIO IRQs are asynchronous, it shouldn't be a 
big problem for them to wait a bit, whereas normal IO IRQs are card's 
response to host's actions, so, the originator might want to know the 
result before processing any asynchronous events. So, I actually like your 
ordering better... But I'll give it a short spin with SDIO, unless you do 
it yourself.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/5] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

On Tue, Aug 16, 2011 at 01:13:06PM +0200, Guennadi Liakhovetski wrote:
> On Tue, 16 Aug 2011, Simon Horman wrote:
> 
> > On Tue, Aug 16, 2011 at 09:41:52AM +0200, Guennadi Liakhovetski wrote:
> > > On Tue, 16 Aug 2011, Simon Horman wrote:
> 
> [snip]
> 
> > > > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > > > +{
> > > > +	struct tmio_mmc_host *host = devid;
> > > > +	unsigned int ireg, status;
> > > > +
> > > > +	pr_debug("MMC IRQ begin\n");
> > > > +
> > > > +	tmio_mmc_card_irq_status(host, &ireg, &status);
> > > > +	__tmio_mmc_card_detect_irq(host, ireg, status);
> > > 
> > > Same here - I would return, if a card hot-plug event occurred.
> > 
> > Will do.
> > 
> > > > +	__tmio_mmc_sdcard_irq(host, ireg, status);
> > > 
> > > Ditto
> > > 
> > > > +
> > > > +	tmio_mmc_sdio_irq(irq, devid);
> > > 
> > > Any specific reason, why you now process SDIO IRQs last?
> > 
> > I believe this is in keeping with the ordering implied by original code.
> 
> I believe it's not. The original version did SDIO first, then hotplug, 
> then normal IO.

My reading of the original code is that SDIO was the lowest priority
although its code appeared near the top of tmio_mmc_irq().

irqreturn_t tmio_mmc_irq(int irq, void *devid)
{
	...

	status = sd_ctrl_read32(host, CTL_STATUS);
	irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK);
	ireg = status & TMIO_MASK_IRQ & ~irq_mask;

	sdio_ireg = 0;
	if (!ireg pdata->flags & TMIO_MMC_SDIO_IRQ) {
		/* Handle SDIO Interrupt */
		...
		goto out;
	}

	/* Handle Card detect Interrupts */

	/* Handle other Interrupts */

	...
}

> I'm not necessarily saying, that the original code was 
> correct or better, I'm just saying, it was different. As for which one we 
> should prefer... I think, I'd check for hotplug first: if the card is 
> removed, no reason to try to communicate with it. And we have to first 
> report a new card, before reporting any IRQs from it. But then - IO or 
> SDIO as second? Well, since SDIO IRQs are asynchronous, it shouldn't be a 
> big problem for them to wait a bit, whereas normal IO IRQs are card's 
> response to host's actions, so, the originator might want to know the 
> result before processing any asynchronous events. So, I actually like your 
> ordering better... But I'll give it a short spin with SDIO, unless you do 
> it yourself.

I intend to test my code with SDIO, however I don't have access to hardware
at this exact moment. So if you could do so, that would be great.