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

[RFC 0/4] 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

* Card portion tested on AP4/Mackerel
* SDIO portion yet to be tested. I intend to schedule access to hardware
  to test this if the review of these patches is positive.

[PATCH 1/4] mmc: tmio, sdhi: Split tmio_mmc_irq() based on registers

[Simon Horman]

This splits tmio_mmc_irq() in two based on the registers
used to handle the interrupts.

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>
---
 drivers/mmc/host/tmio_mmc_pio.c |   71 +++++++++++++++-----------------------
 1 files changed, 28 insertions(+), 43 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 1f16357..e658cb4 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -543,43 +543,15 @@ out:
 	spin_unlock(&host->lock);
 }
 
-irqreturn_t tmio_mmc_irq(int irq, void *devid)
+static void __tmio_mmc_card_irq(struct tmio_mmc_host *host)
 {
-	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;
-
-	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;
 
-	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;
-
-		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);
-			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);
 
@@ -591,43 +563,56 @@ 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);
- */
-
 	/* 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;
 	}
+}
 
-	pr_warning("tmio_mmc: Spurious irq, disabling! "
-		"0x%08x 0x%08x 0x%08x\n", status, irq_mask, ireg);
-	pr_debug_status(status);
-	tmio_mmc_disable_mmc_irqs(host, status & ~irq_mask);
+static void __tmio_mmc_sdio_irq(struct tmio_mmc_host *host)
+{
+	struct mmc_host *mmc = host->mmc;
+	struct tmio_mmc_data *pdata = host->pdata;
+	unsigned int ireg, irq_mask, status;
+
+	if (!(pdata->flags & TMIO_MMC_SDIO_IRQ))
+		return;
+
+	status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
+	irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
+	ireg = status & TMIO_SDIO_MASK_ALL & ~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);
+}
+
+irqreturn_t tmio_mmc_irq(int irq, void *devid)
+{
+	struct tmio_mmc_host *host = devid;
+
+	pr_debug("MMC IRQ begin\n");
+	__tmio_mmc_card_irq(host);
+	__tmio_mmc_sdio_irq(host);
 
-out:
 	return IRQ_HANDLED;
 }
 EXPORT_SYMBOL(tmio_mmc_irq);
-- 
1.7.5.4

[PATCH 2/4] mmc: tmio, sdhi: Split card interrupts based on source

[Simon Horman]

SDHI hardware allows for two different card interrupt sources;
one for access and one for detect.

As preparation for wiring the sources up to separate interrupt handlers
this patch splits the card interrupt handler in two.

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/tmio_mmc_pio.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index e658cb4..3880750 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -543,17 +543,21 @@ out:
 	spin_unlock(&host->lock);
 }
 
-static void __tmio_mmc_card_irq(struct tmio_mmc_host *host)
+static void tmio_mmc_card_irq_status(struct tmio_mmc_host *host,
+				     int *ireg, int *irq_mask, int *status)
 {
-	struct mmc_host *mmc = host->mmc;
-	unsigned int ireg, irq_mask, status;
+	*status = sd_ctrl_read32(host, CTL_STATUS);
+	*irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK);
+	*ireg = *status & TMIO_MASK_IRQ & ~*irq_mask;
 
-	status = sd_ctrl_read32(host, CTL_STATUS);
-	irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK);
-	ireg = status & TMIO_MASK_IRQ & ~irq_mask;
+	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 irq_mask, int status)
+{
+	struct mmc_host *mmc = host->mmc;
 
 	/* Card insert / remove attempts */
 	if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
@@ -564,7 +568,11 @@ static void __tmio_mmc_card_irq(struct tmio_mmc_host *host)
 		    !work_pending(&mmc->detect.work))
 			mmc_detect_change(host->mmc, msecs_to_jiffies(100));
 	}
+}
 
+void __tmio_mmc_card_access_irq(struct tmio_mmc_host *host,
+				int ireg, int irq_mask, int status)
+{
 	/* Command completion */
 	if (ireg & (TMIO_STAT_CMDRESPEND | TMIO_STAT_CMDTIMEOUT)) {
 		tmio_mmc_ack_mmc_irqs(host,
@@ -586,6 +594,16 @@ static void __tmio_mmc_card_irq(struct tmio_mmc_host *host)
 	}
 }
 
+static void __tmio_mmc_card_irq(struct tmio_mmc_host *host)
+{
+	unsigned int ireg, irq_mask, status;
+
+	tmio_mmc_card_irq_status(host, &ireg, &irq_mask, &status);
+
+	__tmio_mmc_card_detect_irq(host, ireg, irq_mask, status);
+	__tmio_mmc_card_access_irq(host, ireg, irq_mask, status);
+}
+
 static void __tmio_mmc_sdio_irq(struct tmio_mmc_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
-- 
1.7.5.4

[PATCH 3/4] mmc: tmio, sdhi: Provide separate interrupt hadnlers

[Simon Horman]

Provide separate interrupt handlers which may be used by platforms where
SDHI has three interrupt 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/tmio_mmc.h     |    3 +++
 drivers/mmc/host/tmio_mmc_pio.c |   35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index eeaf643..647a24c 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -93,6 +93,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_card_access_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 3880750..322aa1b 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -570,6 +570,18 @@ static void __tmio_mmc_card_detect_irq(struct tmio_mmc_host *host,
 	}
 }
 
+irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid)
+{
+	unsigned int ireg, irq_mask, status;
+	struct tmio_mmc_host *host = devid;
+
+	pr_debug("MMC Card Detect IRQ begin\n");
+	tmio_mmc_card_irq_status(host, &ireg, &irq_mask, &status);
+	__tmio_mmc_card_detect_irq(host, ireg, irq_mask, status);
+
+	return IRQ_HANDLED;
+}
+
 void __tmio_mmc_card_access_irq(struct tmio_mmc_host *host,
 				int ireg, int irq_mask, int status)
 {
@@ -594,12 +606,23 @@ void __tmio_mmc_card_access_irq(struct tmio_mmc_host *host,
 	}
 }
 
-static void __tmio_mmc_card_irq(struct tmio_mmc_host *host)
+irqreturn_t tmio_mmc_card_access_irq(int irq, void *devid)
 {
 	unsigned int ireg, irq_mask, status;
+	struct tmio_mmc_host *host = devid;
 
+	pr_debug("MMC Card Access IRQ begin\n");
 	tmio_mmc_card_irq_status(host, &ireg, &irq_mask, &status);
+	__tmio_mmc_card_detect_irq(host, ireg, irq_mask, status);
 
+	return IRQ_HANDLED;
+}
+
+static void __tmio_mmc_card_irq(struct tmio_mmc_host *host)
+{
+	unsigned int ireg, irq_mask, status;
+
+	tmio_mmc_card_irq_status(host, &ireg, &irq_mask, &status);
 	__tmio_mmc_card_detect_irq(host, ireg, irq_mask, status);
 	__tmio_mmc_card_access_irq(host, ireg, irq_mask, status);
 }
@@ -623,6 +646,16 @@ static void __tmio_mmc_sdio_irq(struct tmio_mmc_host *host)
 		mmc_signal_sdio_irq(mmc);
 }
 
+irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
+{
+	struct tmio_mmc_host *host = devid;
+
+	pr_debug("MMC SDIO IRQ begin\n");
+	__tmio_mmc_sdio_irq(host);
+
+	return IRQ_HANDLED;
+}
+
 irqreturn_t tmio_mmc_irq(int irq, void *devid)
 {
 	struct tmio_mmc_host *host = devid;
-- 
1.7.5.4

[PATCH 4/4] 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 |   24 ++++++++++++++++++++----
 drivers/mmc/host/tmio_mmc_pio.c   |    2 +-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 774f643..08c1b3b 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -97,6 +97,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	struct tmio_mmc_host *host;
 	char clk_name[8];
 	int i, irq, ret;
+	bool multi_irq = false;
 
 	priv = kzalloc(sizeof(struct sh_mobile_sdhi), GFP_KERNEL);
 	if (priv == NULL) {
@@ -153,7 +154,9 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto eprobe;
 
-	for (i = 0; i < 3; i++) {
+	for (i = 2; i >= 0; i--) {
+		irqreturn_t (*f)(int irq, void *devid);
+
 		irq = platform_get_irq(pdev, i);
 		if (irq < 0) {
 			if (i) {
@@ -163,10 +166,23 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 				goto eirq;
 			}
 		}
-		ret = request_irq(irq, tmio_mmc_irq, 0,
-				  dev_name(&pdev->dev), host);
+
+		switch(i) {
+		case 2:
+			multi_irq = true;
+			f = tmio_mmc_sdio_irq;
+			break;
+		case 1:
+			f = tmio_mmc_card_access_irq;
+			break;
+		case 0:
+			f = multi_irq ? tmio_mmc_card_detect_irq : tmio_mmc_irq;
+			break;
+		}
+
+		ret = request_irq(irq, f, 0, dev_name(&pdev->dev), host);
 		if (ret) {
-			while (i--) {
+			while (i++ < 3) {
 				irq = platform_get_irq(pdev, i);
 				if (irq >= 0)
 					free_irq(irq, host);
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 322aa1b..7e01d37 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -613,7 +613,7 @@ irqreturn_t tmio_mmc_card_access_irq(int irq, void *devid)
 
 	pr_debug("MMC Card Access IRQ begin\n");
 	tmio_mmc_card_irq_status(host, &ireg, &irq_mask, &status);
-	__tmio_mmc_card_detect_irq(host, ireg, irq_mask, status);
+	__tmio_mmc_card_access_irq(host, ireg, irq_mask, status);
 
 	return IRQ_HANDLED;
 }
-- 
1.7.5.4

Re: [RFC 0/4] mmc: tmio, sdhi: provide multiple irq handlers

[Guennadi Liakhovetski]

Hi Simon

On Mon, 15 Aug 2011, Simon Horman wrote:

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

The idea is good, thanks for the patches. Only I'm not sure I find the way 
you split the patches extremely intuitive;-) How about:

[PATCH 1/x] cache IRQ masks
 * in this patch I'd propose to cache SD-card and SDIO IRQ masks in struct 
   tmio_mmc_host, instead of reading them every time from the hardware
[PATCH 2/x] split the ISR
 * in this patch you split the IRQ handler directly into the final form as 
   after the first your 3 patches, without intermediate steps, also adding 
   them to the header
[PATCH 3/x] SDHI: use specialized ISRs when available
 * you know what to do here:-) Also, I'd

#define SH_MOBILE_SDHI_IRQ_SDCARD	0
#define SH_MOBILE_SDHI_IRQ_CARD_DETECT	1
#define SH_MOBILE_SDHI_IRQ_SDIO		2

and use these defines both in platforms

	}, [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
...

and in sh_mobile_sdhi.c, instead of going "case 2:" Please, also consider 
unfolding the loop over platform IRQs in probing, it might look better 
flat.

"card_access" in function names I would replace with "io" or "data," 
"card_irq" with "sdcard_irq" because I believe, that "SD card" is a proper 
identification to pure storage card in SD format, as opposed to SDIO 
cards.

Also, maybe you can double-check, whether you really need all those 
functions with names, beginning with a double underscore, and whether 
better names wouldn't be possible for them.

> This series also wires up the broken-out irq handlers in the SDHI driver
> 
> * Card portion tested on AP4/Mackerel
> * SDIO portion yet to be tested. I intend to schedule access to hardware
>   to test this if the review of these patches is positive.

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

Re: [RFC 0/4] mmc: tmio, sdhi: provide multiple irq handlers

[Simon Horman]

On Mon, Aug 15, 2011 at 10:17:28AM +0200, Guennadi Liakhovetski wrote:
> Hi Simon
> 
> On Mon, 15 Aug 2011, Simon Horman wrote:
> 
> > 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.
> 
> The idea is good, thanks for the patches. Only I'm not sure I find the way 
> you split the patches extremely intuitive;-) How about:
> 
> [PATCH 1/x] cache IRQ masks
>  * in this patch I'd propose to cache SD-card and SDIO IRQ masks in struct 
>    tmio_mmc_host, instead of reading them every time from the hardware

Sure, that sounds reasonable - though it seems somewhat orthogonal to my series.

I'll check over the code, but it seems that you are implying that
the masks never change.

> [PATCH 2/x] split the ISR
>  * in this patch you split the IRQ handler directly into the final form as 
>    after the first your 3 patches, without intermediate steps, also adding 
>    them to the header

The current split was intended to make bite-size patches that
are easy to review. I'm happy to combine the patches as you
suggest if that is what you prefer.

> [PATCH 3/x] SDHI: use specialized ISRs when available
>  * you know what to do here:-) Also, I'd

> #define SH_MOBILE_SDHI_IRQ_SDCARD	0
> #define SH_MOBILE_SDHI_IRQ_CARD_DETECT	1
> #define SH_MOBILE_SDHI_IRQ_SDIO		2
> 
> and use these defines both in platforms
> 
> 	}, [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> ...

I think I see what you are getting at there.
I will try and make it so.

> and in sh_mobile_sdhi.c, instead of going "case 2:" Please, also consider 
> unfolding the loop over platform IRQs in probing, it might look better 
> flat.

I agree the current loop isn't entirely clean.
I'll unroll it and see how things look.

> "card_access" in function names I would replace with "io" or "data," 
> "card_irq" with "sdcard_irq" because I believe, that "SD card" is a proper 
> identification to pure storage card in SD format, as opposed to SDIO 
> cards.

Sure, if you would prefer that naming scheme.

> Also, maybe you can double-check, whether you really need all those 
> functions with names, beginning with a double underscore, and whether 
> better names wouldn't be possible for them.

The motivation behind that aspect of the implementation is
to allow re-use of code while avoiding extra register reads.

I believe the two sdio functions could be collapsed into a single function
while only losing (probably unused) debugging information. I will do that,
though I decided against that option previously for the sake of consistency.

For the card_irq() functions I think it is a bit difficult to collapse things
because the access and detect handlers actually need to read the same
register(s).