* [PATCH] OMAP35xx: Added SDIO IRQ support
@ 2009-10-28 13:18 Phaneendra Kumar Alapati
2009-10-28 16:08 ` Madhusudhan
0 siblings, 1 reply; 8+ messages in thread
From: Phaneendra Kumar Alapati @ 2009-10-28 13:18 UTC (permalink / raw)
To: linux-omap
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)
+#define CTPL BIT(11)
+#define CLKEXTFREE BIT(16)
/*
* 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;
struct omap_mmc_platform_data *pdata;
};
@@ -240,8 +245,13 @@ mmc_omap_start_command(struct mmc_omap_host
*host, struct mmc_command *cmd,
* 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));
+ } 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);
+
+ 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);
+ }
+ }
+
if (host->mrq == NULL) {
OMAP_HSMMC_WRITE(host->base, STAT,
- OMAP_HSMMC_READ(host->base, STAT));
+ OMAP_HSMMC_READ(host->base, STAT));
/* 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);
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)
host->irq = irq;
host->id = pdev->id;
host->slot_id = 0;
+ host->sdio_int = 0;
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));
+
omap_hsmmc_init(host);
/* Select DMA lines */
--
1.6.0.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH] OMAP35xx: Added SDIO IRQ support
2009-10-28 13:18 [PATCH] OMAP35xx: Added SDIO IRQ support Phaneendra Kumar Alapati
@ 2009-10-28 16:08 ` Madhusudhan
2009-10-28 19:47 ` Dirk Behme
0 siblings, 1 reply; 8+ messages in thread
From: Madhusudhan @ 2009-10-28 16:08 UTC (permalink / raw)
To: 'Phaneendra Kumar Alapati', linux-omap
> -----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.
> +#define CTPL BIT(11)
> +#define CLKEXTFREE BIT(16)
Can we define them in the same way as other defines to maintain uniformity?
>
> /*
> * 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.
> struct omap_mmc_platform_data *pdata;
> };
>
> @@ -240,8 +245,13 @@ mmc_omap_start_command(struct mmc_omap_host
> *host, struct mmc_command *cmd,
> * 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".
> + } 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?
> +
> + 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);
> + }
> + }
> +
> 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.
> /* 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.
>
> 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)
> 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.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] OMAP35xx: Added SDIO IRQ support
2009-10-28 16:08 ` Madhusudhan
@ 2009-10-28 19:47 ` Dirk Behme
2009-10-28 20:52 ` Madhusudhan
2009-10-29 7:00 ` Dirk Behme
0 siblings, 2 replies; 8+ messages in thread
From: Dirk Behme @ 2009-10-28 19:47 UTC (permalink / raw)
To: Madhusudhan, 'Phaneendra Kumar Alapati'; +Cc: linux-omap
[-- 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 */
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] OMAP35xx: Added SDIO IRQ support
2009-10-28 19:47 ` Dirk Behme
@ 2009-10-28 20:52 ` Madhusudhan
2009-10-29 9:27 ` Phaneendra Kumar Alapati
2009-10-29 7:00 ` Dirk Behme
1 sibling, 1 reply; 8+ messages in thread
From: Madhusudhan @ 2009-10-28 20:52 UTC (permalink / raw)
To: 'Dirk Behme', 'Phaneendra Kumar Alapati'; +Cc: linux-omap
> -----Original Message-----
> From: Dirk Behme [mailto:dirk.behme@googlemail.com]
> Sent: Wednesday, October 28, 2009 2:48 PM
> To: Madhusudhan; 'Phaneendra Kumar Alapati'
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] OMAP35xx: Added SDIO IRQ support
>
> 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?
>
That is not needed because of the below implementation:
static inline void mmc_signal_sdio_irq(struct mmc_host *host)
{
host->ops->enable_sdio_irq(host, 0);
wake_up_process(host->sdio_irq_thread);
}
The host is asked to disable the irq inherently.
Regards,
Madhu
> - 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
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] OMAP35xx: Added SDIO IRQ support
2009-10-28 19:47 ` Dirk Behme
2009-10-28 20:52 ` Madhusudhan
@ 2009-10-29 7:00 ` Dirk Behme
2009-10-29 14:35 ` Phaneendra Kumar Alapati
1 sibling, 1 reply; 8+ messages in thread
From: Dirk Behme @ 2009-10-29 7:00 UTC (permalink / raw)
To: Madhusudhan, 'Phaneendra Kumar Alapati'; +Cc: linux-omap, Steve Sakoman
Dirk Behme wrote:
> 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.
I promised to come back with test results. As mentioned in this thread
already, I can't test on my own yet, instead Steve (many, many
thanks!) tests it on Overo air. Overo air uses Marvell's 88W8686
connected to MMC port 2 in 4 bit configuration.
First, my 'revised' version of Phani's patch doesn't work at all.
There seem to be some tricks in Phani's original patch we (I?) missed
in above review. I tried to incorporate all review comments, result is
a non working patch.
Second, Steve tested Phani's original patch, too (with manually fixed
wrapped lines). Let me copy his comments:
-- cut --
this is looking better!
libertas_sdio: Libertas SDIO driver
libertas_sdio: Copyright Pierre Ossman
libertas_sdio mmc1:0001:1: firmware: requesting sd8686_helper.bin
libertas_sdio mmc1:0001:1: firmware: requesting sd8686.bin
libertas: 00:19:88:20:fa:23, fw 9.70.3p24, cap 0x00000303
libertas: eth1: Marvell WLAN 802.11 adapter
udev: renamed network interface eth1 to wlan0
(with the other patches we always had timeouts and errors)
But performance is really bad (and flakey!)
When it does work, I get 5.88K/s as compared to 120K/s with the
unpatched code. It also dies fairly quickly.
No error messages, so I am really not sure what is going on!
-- cut --
Any ideas?
Thanks to Phani and Steve, best regards
Dirk
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] OMAP35xx: Added SDIO IRQ support
2009-10-28 20:52 ` Madhusudhan
@ 2009-10-29 9:27 ` Phaneendra Kumar Alapati
2009-10-29 21:08 ` Dirk Behme
0 siblings, 1 reply; 8+ messages in thread
From: Phaneendra Kumar Alapati @ 2009-10-29 9:27 UTC (permalink / raw)
To: Madhusudhan; +Cc: Dirk Behme, linux-omap
Dirk and Madhu, Thanks for the review/comments. I have explained below
the reasons behind certain changes i made esp the interrupt enabling
and irq routine changes.
On Thu, Oct 29, 2009 at 2:22 AM, Madhusudhan <madhu.cr@ti.com> wrote:
>
>
>> -----Original Message-----
>> From: Dirk Behme [mailto:dirk.behme@googlemail.com]
>> Sent: Wednesday, October 28, 2009 2:48 PM
>> To: Madhusudhan; 'Phaneendra Kumar Alapati'
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] OMAP35xx: Added SDIO IRQ support
>>
>> 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.
>>
I have kept separate macros in reference to the TRM as they are for
different registers. To keep the code simpler they can be merged
>> >> /*
>> >> * 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?
>>
Once sdio interrupts are enabled through enable_sdio_irq, commands
which are sent further disable the sdio interrupts by directly over
writing to the IE and ISE without preserving currently enabled
interrupts.
Best option is to read+modify+write the IE and ISE register as i did
in enable_sdio_irq. But the IE and ISE register seem to be modified in
other places too, so i made use of sdio_int flag to keep track of sdio
interrupt enabling/disabling.
>> >> + } 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?
>>
SDIO interrupts are generated asynchronously from the sdio card even
though when there are no mmc requests<mrq>. With out the changes in
the patch we will be returning from the interrupt routine in case of
mmc requests being NULL, there by SDIO interrupts will not be handled.
Hence i have moved status register read and sdio interrupt checking
code up in the irq routine to handle the sdio interrupts properly.
There is always scope for getting SDIO as well as controller
interrupts at the same instance. Hence i check for controller
interrupts after handling SDIO interrupts without returning from irq
handler.
Yeah.. as said by Dirk, the dev_dbg need not print the status.
> That is not needed because of the below implementation:
>
> static inline void mmc_signal_sdio_irq(struct mmc_host *host)
> {
> host->ops->enable_sdio_irq(host, 0);
> wake_up_process(host->sdio_irq_thread);
> }
>
> The host is asked to disable the irq inherently.
>
> Regards,
> Madhu
>
>> - 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 (?)
>>
As pointed out by Dirk, enable_sdio_irq wont be called if mmc->caps is
not set with MMC_CAP_SDIO_IRQ. So that should be kept separate.
I do not see any advantage (Any extra power saving ! ;)) in modifying
the CON register bits(CTPL and CLKEXTFREE) every time along with SDIO
interrupts. Also the extra read and write might add some overhead.
>> All in all, I wonder why IBG bit isn't used in this patch. Is this
>> tested with 1 or 4 bit (wire) SDIO?
>>
I have not yet verified the functionality with 1bit SDIO.
--- Regards,
Phani
>> 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
>> >
>
>
> --
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] OMAP35xx: Added SDIO IRQ support
2009-10-29 7:00 ` Dirk Behme
@ 2009-10-29 14:35 ` Phaneendra Kumar Alapati
0 siblings, 0 replies; 8+ messages in thread
From: Phaneendra Kumar Alapati @ 2009-10-29 14:35 UTC (permalink / raw)
To: Dirk Behme; +Cc: Madhusudhan, linux-omap, Steve Sakoman
On Thu, Oct 29, 2009 at 12:30 PM, Dirk Behme <dirk.behme@googlemail.com> wrote:
> Dirk Behme wrote:
>
> I promised to come back with test results. As mentioned in this thread
> already, I can't test on my own yet, instead Steve (many, many thanks!)
> tests it on Overo air. Overo air uses Marvell's 88W8686 connected to MMC
> port 2 in 4 bit configuration.
>
> First, my 'revised' version of Phani's patch doesn't work at all. There seem
> to be some tricks in Phani's original patch we (I?) missed in above review.
> I tried to incorporate all review comments, result is a non working patch.
>
As i mentioned earlier, the code changes you made will result in loss
of sdio interrupts which will effect the host driver functionality.
> Second, Steve tested Phani's original patch, too (with manually fixed
> wrapped lines). Let me copy his comments:
>
> -- cut --
> this is looking better!
>
> libertas_sdio: Libertas SDIO driver
> libertas_sdio: Copyright Pierre Ossman
> libertas_sdio mmc1:0001:1: firmware: requesting sd8686_helper.bin
> libertas_sdio mmc1:0001:1: firmware: requesting sd8686.bin
> libertas: 00:19:88:20:fa:23, fw 9.70.3p24, cap 0x00000303
> libertas: eth1: Marvell WLAN 802.11 adapter
> udev: renamed network interface eth1 to wlan0
>
> (with the other patches we always had timeouts and errors)
>
> But performance is really bad (and flakey!)
>
> When it does work, I get 5.88K/s as compared to 120K/s with the unpatched
> code. It also dies fairly quickly.
>
> No error messages, so I am really not sure what is going on!
> -- cut --
Iam not sure about what might be causing such a drastically low
performance with steve's testing. The initial SDIO interrupts occur
once the firmware gets loaded properly. From the log it looks the
firmware is loading successfully and interface is up. In which case it
means the interrupt mechanism is proper as it should be. So the
reduction in performance cant be that worse when handling SDIO
interrupts with irq support. Even with the SDIO interrupt mechanism in
polling mode i observed throughput in the range of 1 to 2 Mbps.
Given below are details of my test environment:
Target boards: OMAP3530EVM and AM3517EVM
Linux kernel version: 2.6.31-rc7.
The host operating frequency is 25MHz and in sdio 4bit mode of operation.
Libertas firmware<9.70.3.p24-26409.P45-GPL> from marvell
<http://www.marvell.com/drivers/driverDisplay.do?driverId=203>
Mode: Infrastructure mode
Channel 11
Phy Mode: G
Observed throughput [using ttcp utility]: 13Mbps (Downlink), 10.5 Mbps(Uplink)
The throughput measurements are taken with Marvell 88W8686 card on one
end connected to an access point and on the other end the access point
connected to a test system through ethernet. The WiFi performance
would enhance under proper test conditions.
Regards,
Phani
> Any ideas?
>
> Thanks to Phani and Steve, best regards
>
> Dirk
> --
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] OMAP35xx: Added SDIO IRQ support
2009-10-29 9:27 ` Phaneendra Kumar Alapati
@ 2009-10-29 21:08 ` Dirk Behme
0 siblings, 0 replies; 8+ messages in thread
From: Dirk Behme @ 2009-10-29 21:08 UTC (permalink / raw)
To: Phaneendra Kumar Alapati; +Cc: Madhusudhan, linux-omap
Comments below...
Phaneendra Kumar Alapati wrote:
> Dirk and Madhu, Thanks for the review/comments. I have explained below
> the reasons behind certain changes i made esp the interrupt enabling
> and irq routine changes.
>
> On Thu, Oct 29, 2009 at 2:22 AM, Madhusudhan <madhu.cr@ti.com> wrote:
>>
>>> -----Original Message-----
>>> From: Dirk Behme [mailto:dirk.behme@googlemail.com]
>>> Sent: Wednesday, October 28, 2009 2:48 PM
>>> To: Madhusudhan; 'Phaneendra Kumar Alapati'
>>> Cc: linux-omap@vger.kernel.org
>>> Subject: Re: [PATCH] OMAP35xx: Added SDIO IRQ support
>>>
>>> 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.
>>>
>
> I have kept separate macros in reference to the TRM as they are for
> different registers. To keep the code simpler they can be merged
>
>>>>> /*
>>>>> * 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?
>>>
>
> Once sdio interrupts are enabled through enable_sdio_irq, commands
> which are sent further disable the sdio interrupts by directly over
> writing to the IE and ISE without preserving currently enabled
> interrupts.
>
> Best option is to read+modify+write the IE and ISE register as i did
> in enable_sdio_irq. But the IE and ISE register seem to be modified in
> other places too, so i made use of sdio_int flag to keep track of sdio
> interrupt enabling/disabling.
>
>>>>> + } 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?
>>>
>
> SDIO interrupts are generated asynchronously from the sdio card even
> though when there are no mmc requests<mrq>. With out the changes in
> the patch we will be returning from the interrupt routine in case of
> mmc requests being NULL, there by SDIO interrupts will not be handled.
>
> Hence i have moved status register read and sdio interrupt checking
> code up in the irq routine to handle the sdio interrupts properly.
>
> There is always scope for getting SDIO as well as controller
> interrupts at the same instance. Hence i check for controller
> interrupts after handling SDIO interrupts without returning from irq
> handler.
>
> Yeah.. as said by Dirk, the dev_dbg need not print the status.
>
>
>> That is not needed because of the below implementation:
>>
>> static inline void mmc_signal_sdio_irq(struct mmc_host *host)
>> {
>> host->ops->enable_sdio_irq(host, 0);
>> wake_up_process(host->sdio_irq_thread);
>> }
>>
>> The host is asked to disable the irq inherently.
>>
>> Regards,
>> Madhu
>>
>>> - 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 (?)
>>>
>
> As pointed out by Dirk, enable_sdio_irq wont be called if mmc->caps is
> not set with MMC_CAP_SDIO_IRQ. So that should be kept separate.
>
> I do not see any advantage (Any extra power saving ! ;)) in modifying
> the CON register bits(CTPL and CLKEXTFREE) every time along with SDIO
> interrupts. Also the extra read and write might add some overhead.
>
>>> All in all, I wonder why IBG bit isn't used in this patch. Is this
>>> tested with 1 or 4 bit (wire) SDIO?
>>>
>
> I have not yet verified the functionality with 1bit SDIO.
Ok, as Overo uses 4bit, too, we don't need 1bit for testing now.
Do you like to update your patch with
* using
#define CIRQ (1 << 8)
#define CIRQ_ENABLE (1 << 8)
#define CTPL (1 << 11)
#define CLKEXTFREE (1 << 16)
* using
dev_dbg(mmc_dev(host->mmc), "SDIO interrupt\n");
* Remove the additional tab space from
+ OMAP_HSMMC_READ(host->base, STAT));
* trying to send it with git send-email to avoid line wrapping?
?
And do you have any comments on IBG bit? Would be nice if you could
test setting this bit, too (as mentioned, I have not test environment,
yet).
Many thanks
Dirk
>>> 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
>>>>
>>
>> --
>> 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
>>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-10-29 21:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-28 13:18 [PATCH] OMAP35xx: Added SDIO IRQ support Phaneendra Kumar Alapati
2009-10-28 16:08 ` Madhusudhan
2009-10-28 19:47 ` Dirk Behme
2009-10-28 20:52 ` Madhusudhan
2009-10-29 9:27 ` Phaneendra Kumar Alapati
2009-10-29 21:08 ` Dirk Behme
2009-10-29 7:00 ` Dirk Behme
2009-10-29 14:35 ` Phaneendra Kumar Alapati
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox