Re: [PATCH] OMAP35xx: Added SDIO IRQ support

[Dirk Behme <dirk.behme@googlemail.com>]

[-- Attachment #1: Type: text/plain, Size: 7336 bytes --]

Madhusudhan wrote:
> 
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Phaneendra Kumar Alapati
>> Sent: Wednesday, October 28, 2009 8:19 AM
>> To: linux-omap@vger.kernel.org
>> Subject: [PATCH] OMAP35xx: Added SDIO IRQ support
>>
>> This patch adds SDIO IRQ support for omap hsmmc driver.
>>
>> Signed-off-by: Phaneendra Kumar Alapati <phani@embwise.com>
>> ---
>>  drivers/mmc/host/omap_hsmmc.c |    62 ++--
>>  1 files changed, 55 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 1cf9cfb..a540626 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -92,6 +92,10 @@
>>  #define DUAL_VOLT_OCR_BIT	7
>>  #define SRC			(1 << 25)
>>  #define SRD			(1 << 26)
>> +#define OMAP_HSMMC_CARD_INT	BIT(8)
>> +#define SDIO_INT_EN		BIT(8)
> Why multiple defines of the same? One of them should be enough.

What I think meant here is

#define CIRQ			(1 << 8)
#define CIRQ_ENABLE		(1 << 8)

One is for the status register, the other is for the interrupt enable 
registers. To be compatible with the TRM, I would use both in this way.

>> +#define CTPL			BIT(11)
>> +#define CLKEXTFREE		BIT(16)
> Can we define them in the same way as other defines to maintain uniformity?

Yes, ack.

>>  /*
>>   * FIXME: Most likely all the data using these _DEVID defines should come
>> @@ -149,6 +153,7 @@ struct mmc_omap_host {
>>  	int			slot_id;
>>  	int			dbclk_enabled;
>>  	int			response_busy;
>> +	int			sdio_int;
> 
> What purpose does this serve? IMHO, this is not needed.

Hmm. This is set to != 0 in omap_hsmmc_enable_sdio_irq() when IRQs are 
enabled. Then in mmc_omap_start_command() these interrupts are enabled 
again if sdio_int is != 0. Yes, looks unneeded, indeed. But maybe 
there is some trick ;)

>>  	struct	omap_mmc_platform_data	*pdata;
>>  };
>>
>> @@ -240,8 +245,13 @@ mmc_omap_start_command(struct mmc_omap_host
>> *host, struct mmc_command *cmd,

Patch is line wrapped by mailer.

>>  	 * Clear status bits and enable interrupts
>>  	 */
>>  	OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
>> -	OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK);
>> -	OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK);
>> +	if (host->sdio_int) {
>> +		OMAP_HSMMC_WRITE(host->base, ISE, (INT_EN_MASK |
>> SDIO_INT_EN));
>> +		OMAP_HSMMC_WRITE(host->base, IE, (INT_EN_MASK |
> SDIO_INT_EN));
> Why? It is being taken care in "enable_sdio_irq".

Yes, why?

>> +	} else {
>> +		OMAP_HSMMC_WRITE(host->base, ISE, INT_EN_MASK);
>> +		OMAP_HSMMC_WRITE(host->base, IE, INT_EN_MASK);
>> +	}
>>
>>  	host->response_busy = 0;
>>  	if (cmd->flags & MMC_RSP_PRESENT) {
>> @@ -430,17 +440,27 @@ static irqreturn_t mmc_omap_irq(int irq, void
>> *dev_id)
>>  	struct mmc_data *data;
>>  	int end_cmd = 0, end_trans = 0, status;
>>
>> +	data = host->data;
>> +	status = OMAP_HSMMC_READ(host->base, STAT);
>> +	dev_dbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
> Why are these moved up?

Yes, why? Why not move the block below down instead?

>> +
>> +	if (host->mmc->caps & MMC_CAP_SDIO_IRQ) {
>> +		if (status & OMAP_HSMMC_CARD_INT) {
>> +			dev_dbg(mmc_dev(host->mmc),
>> +					" SDIO CARD interrupt status %x\n",
>> +					status);
>> +			mmc_signal_sdio_irq(host->mmc);
>> +		}
>> +	}

- It makes no sense to print status in dev_dbg here again, as it is 
already printed some lines above. Something like

dev_dbg(mmc_dev(host->mmc), "SDIO interrupt\n");

would be sufficient here.

- Why isn't IRQ acknowledged here? I.e. why not doing something like

OMAP_HSMMC_WRITE(host->base, IE, OMAP_HSMMC_READ(host->base, IE) & 
~(CIRQ_ENABLE));

here?

- No return IRQ_HANDLED; here? Mmm, maybe this makes sense.

>>  	if (host->mrq == NULL) {
>>  		OMAP_HSMMC_WRITE(host->base, STAT,
>> -			OMAP_HSMMC_READ(host->base, STAT));
>> +				OMAP_HSMMC_READ(host->base, STAT));
> This just adds a tab space. Not needed.

Ack.

>>  		/* Flush posted write */
>>  		OMAP_HSMMC_READ(host->base, STAT);
>>  		return IRQ_HANDLED;
>>  	}
>>
>> -	data = host->data;
>> -	status = OMAP_HSMMC_READ(host->base, STAT);
>> -	dev_dbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
> Check my comment above.

Yes, from theory it looks better to move the new code below this, instead.

>>  	if (status & ERR) {
>>  #ifdef CONFIG_MMC_DEBUG
>> @@ -932,6 +952,29 @@ static int omap_hsmmc_get_ro(struct mmc_host *mmc)
>>  	return pdata->slots[0].get_ro(host->dev, 0);
>>  }
>>
>> +static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>> +{
>> +	struct mmc_omap_host *host = mmc_priv(mmc);
>> +
>> +	host->sdio_int = enable;
>> +	if (enable) {
>> +		OMAP_HSMMC_WRITE(host->base, ISE,
>> +				(OMAP_HSMMC_READ(host->base, ISE) |
>> +				 OMAP_HSMMC_CARD_INT));
>> +		OMAP_HSMMC_WRITE(host->base, IE,
>> +				(OMAP_HSMMC_READ(host->base, IE) |
>> +				 OMAP_HSMMC_CARD_INT));
>> +	} else {
>> +		OMAP_HSMMC_WRITE(host->base, IE,
>> +				(OMAP_HSMMC_READ(host->base, IE) &
>> +				 (~OMAP_HSMMC_CARD_INT)));
>> +		OMAP_HSMMC_WRITE(host->base, ISE,
>> +				(OMAP_HSMMC_READ(host->base, ISE) &
>> +				 (~OMAP_HSMMC_CARD_INT)));
>> +	}
>> +
>> +}
>> +
>>  static void omap_hsmmc_init(struct mmc_omap_host *host)
>>  {
>>  	u32 hctl, capa, value;
>> @@ -964,7 +1007,7 @@ static struct mmc_host_ops mmc_omap_ops = {
>>  	.set_ios = omap_mmc_set_ios,
>>  	.get_cd = omap_hsmmc_get_cd,
>>  	.get_ro = omap_hsmmc_get_ro,
>> -	/* NYET -- enable_sdio_irq */
>> +	.enable_sdio_irq = omap_hsmmc_enable_sdio_irq,
>>  };
>>
>>  static int __init omap_mmc_probe(struct platform_device *pdev)
>> @@ -1011,6 +1054,7 @@ static int __init omap_mmc_probe(struct
>> platform_device *pdev)

Greetings from the mailer again.

>>  	host->irq	= irq;
>>  	host->id	= pdev->id;
>>  	host->slot_id	= 0;
>> +	host->sdio_int 	= 0;
> Not needed.
> 
>>  	host->mapbase	= res->start;
>>  	host->base	= ioremap(host->mapbase, SZ_4K);
>>
>> @@ -1080,6 +1124,10 @@ static int __init omap_mmc_probe(struct
>> platform_device *pdev)
>>  	else if (pdata->slots[host->slot_id].wires >= 4)
>>  		mmc->caps |= MMC_CAP_4_BIT_DATA;
>>
>> +	mmc->caps |= MMC_CAP_SDIO_IRQ;
>> +	OMAP_HSMMC_WRITE(host->base, CON,
>> +			OMAP_HSMMC_READ(host->base, CON) | (CTPL |
> CLKEXTFREE));
> How about moving this to "enable_sdio_irq" so that these are done for SDIO
> alone? Also can be disabled in the same fn.

Ack. But I think that mmc->caps |= MMC_CAP_SDIO_IRQ has to stay here. 
Else "enable_sdio_irq" will never be called (?)

All in all, I wonder why IBG bit isn't used in this patch. Is this 
tested with 1 or 4 bit (wire) SDIO?

Just for reference the slightly modified version of this patch for 
testing in attachment (based on pure theory ;) ). I will come back 
with testing results.

Best regards

Dirk

> Regards,
> Madhusudhan
>> +
>>  	omap_hsmmc_init(host);
>>
>>  	/* Select DMA lines */
>> --
>> 1.6.0.4
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: omap_hsmmc_sdio_irq_2_6_31_phaneendra_patch.txt --]
[-- Type: text/plain, Size: 3412 bytes --]

Subject: [PATCH] OMAP35xx: Added SDIO IRQ support
From: Phaneendra Kumar Alapati <phani@embwise.com>
Date: Wed, 28 Oct 2009 18:48:38 +0530
To: linux-omap@vger.kernel.org

This patch adds SDIO IRQ support for omap hsmmc driver.

Signed-off-by: Phaneendra Kumar Alapati <phani@embwise.com>
---

Revised version of

http://marc.info/?l=linux-omap&m=125673594321210&w=2

based on comments and theory. Please test.

Signed-off-by: Dirk Behme <dirk.behme@googlemail.com>

 drivers/mmc/host/omap_hsmmc.c |   48 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 4 deletions(-)

Index: linux-beagle/drivers/mmc/host/omap_hsmmc.c
===================================================================
--- linux-beagle.orig/drivers/mmc/host/omap_hsmmc.c
+++ linux-beagle/drivers/mmc/host/omap_hsmmc.c
@@ -92,6 +92,10 @@
 #define DUAL_VOLT_OCR_BIT	7
 #define SRC			(1 << 25)
 #define SRD			(1 << 26)
+#define CIRQ			(1 << 8)
+#define CIRQ_ENABLE		(1 << 8)
+#define CTPL			(1 << 11)
+#define CLKEXTFREE		(1 << 16)
 
 /*
  * FIXME: Most likely all the data using these _DEVID defines should come
@@ -430,6 +434,15 @@ static irqreturn_t mmc_omap_irq(int irq,
 	struct mmc_data *data;
 	int end_cmd = 0, end_trans = 0, status;
 
+	data = host->data;
+	status = OMAP_HSMMC_READ(host->base, STAT);
+	dev_dbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
+
+	if ((status & CIRQ) && (host->mmc->caps & MMC_CAP_SDIO_IRQ)) {
+		dev_dbg(mmc_dev(host->mmc), "SDIO interrupt\n");
+		mmc_signal_sdio_irq(host->mmc);
+	}
+
 	if (host->mrq == NULL) {
 		OMAP_HSMMC_WRITE(host->base, STAT,
 			OMAP_HSMMC_READ(host->base, STAT));
@@ -438,9 +451,6 @@ static irqreturn_t mmc_omap_irq(int irq,
 		return IRQ_HANDLED;
 	}
 
-	data = host->data;
-	status = OMAP_HSMMC_READ(host->base, STAT);
-	dev_dbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
 
 	if (status & ERR) {
 #ifdef CONFIG_MMC_DEBUG
@@ -932,6 +942,34 @@ static int omap_hsmmc_get_ro(struct mmc_
 	return pdata->slots[0].get_ro(host->dev, 0);
 }
 
+static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+	struct mmc_omap_host *host = mmc_priv(mmc);
+
+	if (enable) {
+		OMAP_HSMMC_WRITE(host->base, CON,
+				 OMAP_HSMMC_READ(host->base, CON) |
+				 CTPL | CLKEXTFREE);
+		OMAP_HSMMC_WRITE(host->base, ISE,
+				(OMAP_HSMMC_READ(host->base, ISE) |
+				 CIRQ_ENABLE));
+		OMAP_HSMMC_WRITE(host->base, IE,
+				(OMAP_HSMMC_READ(host->base, IE) |
+				 CIRQ_ENABLE));
+	} else {
+		OMAP_HSMMC_WRITE(host->base, IE,
+				(OMAP_HSMMC_READ(host->base, IE) &
+				 (~CIRQ_ENABLE)));
+		OMAP_HSMMC_WRITE(host->base, ISE,
+				(OMAP_HSMMC_READ(host->base, ISE) &
+				 (~CIRQ_ENABLE)));
+		OMAP_HSMMC_WRITE(host->base, CON,
+				 OMAP_HSMMC_READ(host->base, CON) &
+				 ~(CTPL | CLKEXTFREE));
+	}
+
+}
+
 static void omap_hsmmc_init(struct mmc_omap_host *host)
 {
 	u32 hctl, capa, value;
@@ -964,7 +1002,7 @@ static struct mmc_host_ops mmc_omap_ops
 	.set_ios = omap_mmc_set_ios,
 	.get_cd = omap_hsmmc_get_cd,
 	.get_ro = omap_hsmmc_get_ro,
-	/* NYET -- enable_sdio_irq */
+	.enable_sdio_irq = omap_hsmmc_enable_sdio_irq,
 };
 
 static int __init omap_mmc_probe(struct platform_device *pdev)
@@ -1080,6 +1118,8 @@ static int __init omap_mmc_probe(struct
 	else if (pdata->slots[host->slot_id].wires >= 4)
 		mmc->caps |= MMC_CAP_4_BIT_DATA;
 
+	mmc->caps |= MMC_CAP_SDIO_IRQ;
+
 	omap_hsmmc_init(host);
 
 	/* Select DMA lines */