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
