Re: [PATCH] OMAP35xx: Added SDIO IRQ support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dirk Behme wrote:
Madhusudhan wrote:

-----Original Message-----
From: linux-omap-owner@xxxxxxxxxxxxxxx [mailto:linux-omap-
owner@xxxxxxxxxxxxxxx] On Behalf Of Phaneendra Kumar Alapati
Sent: Wednesday, October 28, 2009 8:19 AM
To: linux-omap@xxxxxxxxxxxxxxx
Subject: [PATCH] OMAP35xx: Added SDIO IRQ support

This patch adds SDIO IRQ support for omap hsmmc driver.

Signed-off-by: Phaneendra Kumar Alapati <phani@xxxxxxxxxxx>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux