Re: MMC_CAP_SDIO_IRQ for omap 3430

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

John Rigby wrote:
> First the disclaimers:
> The board is new board that has had hardware mmc/sdio problems before.
>  It is based closely on beagle but uses a different package which has
> some package specific issues.  We think we have found them all but who
> knows.
> 
> My kernel is a few months old 2.6.29.

Then you can't apply my patch 1:1. Steve already found that there are 
a lot of differences between recent git head and 2.6.31. A lot of 
omap_hsmmc changes were done since 2.6.31.

> Ok, given the above issues, it looks to me like I am getting
> interrupted every time  CIRQ is enabled.

Infinite interrupts. This is what I assumed :(

> Nor sure if the libertas
> driver is not disabling as it should or what.  

I'm not sure if libertas has to do something with this.

>So I end up in this
> death spiral:
> 
> irq handler is called and status indicates CIRQ is pending
> 
> mmc_signal_sdio_irq is called which calls omap_hsmmc_enable_sdio_irq
> with via mmc_host_ops, en is zero so CIRQ gets disabled
> 
> mmc_signal_sdio_irq then wakes up sdio_irq_thread which does its thing
> then before going to sleep calls omap_hsmmc_enable_irq with en set to
> 1 so CIRC gets turned back on

Sounds ok. And thanks for giving the call stack. As mentioned, I did 
only compile check ;)

> with CIRC enabled the irq handler gets called and we are back where we started.

The issue then is not acknowledging the interrupt correctly. The TRM 
(spruf98.c) tells us:

-- cut --
Card interrupt Enable
A clear of this bit also clears the corresponding status bit.
During 1-bit mode, if the interrupt routine does not remove the
source of a card interrupt in the SDIO card, the status bit is
reasserted when this bit is set to 1.
-- cut --

The clear card interrupt enable should be done by ~(CIRQ_ENABLE) in

+if (status & CIRQ) {
+	dev_dbg(mmc_dev(host->mmc), "SDIO interrupt");
+	OMAP_HSMMC_WRITE(host->base, IE, OMAP_HSMMC_READ(host->base, IE)
+			 & ~(CIRQ_ENABLE));
+	mmc_signal_sdio_irq(host->mmc);
+	spin_unlock(&host->irq_lock);
+	return IRQ_HANDLED;
+}

Anybody sees anything wrong here? Maybe we missed something?

I assume that we are not in 1-bit mode. We should be in 4 bit mode. It 
could be checked by some test code that we are in 4 bit mode and IBG 
is set.

Additionally, what I wonder regarding this, is the davinci driver [1] 
additionally checking for DAT1 level in enable_sdio_irq

1114         if (enable) {
1115                 if (!((readl(host->base + DAVINCI_SDIOST0))
1116                             & SDIOST0_DAT1_HI)) {
1117                         writel(SDIOIST_IOINT,
1118                                         host->base + 
DAVINCI_SDIOIST);
1119                         mmc_signal_sdio_irq(host->mmc);
1120                 } else {
... enable int
1124                 }
1125         } else {
... disable int
1129         }

And it does similar stuff in xfer_done

  873         if (host->mmc->caps & MMC_CAP_SDIO_IRQ) {
  874                 if (host->sdio_int && (!((readl(host->base + 
DAVINCI_SDIOST0))
  875                                         & SDIOST0_DAT1_HI))) {
  876                         writel(SDIOIST_IOINT, host->base + 
DAVINCI_SDIOIST);
  877                         mmc_signal_sdio_irq(host->mmc);
  878                 }
  879         }

Most probably there is a reason for doing so?

Best regards

Dirk

[1] 
http://arago-project.org/git/projects/?p=linux-davinci.git;a=blob;f=drivers/mmc/host/davinci_mmc.c;h=1bf0587250614c6d8abfe02028b96e0e47148ac8;hb=HEAD

> Looks like either the libertas driver is not disabling the irq or the
> CIRQ irq is somehow getting stuck or I have a hw problem.
> 
> One thing I did try was to have the sdio_irq_thread basically ignore
> the MMC_CAP_SDIO_IRQ flag except for calling
> host->ops->enable_sdio_irq(host, 1) and I changed mmc_signal_sdio_irq
> to not reschedule sdio_irq_thread.  In shows that the mere act of
> enabling CIRQ does not break anything.  In this mode I get one CIRQ
> irq for each time around the sdio_irq_thread loop.
> 
> I guess I should try checking the CIRQ bit in the status register
> after calling the libertas handler which should clear the irq?
> 
> 
> John
> 
> 
> On Fri, Oct 16, 2009 at 10:06 AM, Dirk Behme <dirk.behme@googlemail.com> wrote:
>> John Rigby wrote:
>>> Dirk,
>>>
>>> Thanks for the update.  After looking more closely at your patch I
>>> found that the only thing my attempt was missing was the IBG setting.
>>> I added that to mine with the result that the system hangs on loading
>>> the libertas modules.
>> On which board are you working? libertas does mean you are working on wifi
>> connected via SDIO?
>>
>>> The last thing i see is the firmware request
>>> message.
>> Ok, you see a hang. Below we have a timeout.
>>
>> I would vote for some interrupt issues. For timeouts I would vote for
>> missing interrupts, while system hang might be infinite interrupts
>> (something like this was reported in [1]).
>>
>> Could you add some debug code in omap_hsmmc_enable_sdio_irq() to trace if
>> enable/disable is called in the correct order? And in omap_hsmmc_irq() in if
>> (status & CIRQ) { to check if and how often it is called?
>>
>> Best regards
>>
>> Dirk
>>
>> [1] http://groups.google.com/group/beagleboard/msg/5cdfe2a319531937
>>
>>> On Fri, Oct 16, 2009 at 9:02 AM, Dirk Behme <dirk.behme@googlemail.com>
>>> wrote:
>>>> John,
>>>>
>>>> John Rigby wrote:
>>>>> Dirk,
>>>>>
>>>>> Many thanks, after sending the request yesterday I made my own attempt
>>>>> which failed miserably.  My patch looks like a subset of yours so that
>>>>> is to be expected.  I'll try yours out today and report back my
>>>>> experience.
>>>> I got already some (private) feedback (thanks!):
>>>>
>>>> - Accessing host->mmc->card in omap_hsmmc_set_ios() results in a null
>>>> pointer :( Seems that mmc->card isn't set yet while calling
>>>> omap_hsmmc_set_ios().
>>>>
>>>> As workaround, for testing only, I hard coded setting IBG with something
>>>> like
>>>>
>>>>       case MMC_BUS_WIDTH_4:
>>>>               OMAP_HSMMC_WRITE(host->base, CON, con & ~DW8);
>>>> -               OMAP_HSMMC_WRITE(host->base, HCTL,
>>>> -                       OMAP_HSMMC_READ(host->base, HCTL) | FOUR_BIT);
>>>> +                       OMAP_HSMMC_WRITE(host->base, HCTL,
>>>> +                                        OMAP_HSMMC_READ(host->base,
>>>> HCTL) |
>>>> IBG | FOUR_BIT);
>>>>              break;
>>>>
>>>> If you test this, be careful that this doesn't hurt any other 4 bit cards
>>>> except the SDIO you want to test.
>>>>
>>>> Later, we have to find a way how to detect that we are in SDIO mode. We
>>>> want
>>>> to set IBG only if in SDIO and 4 bit mode.
>>>>
>>>> -  After this, I got the report that null pointer crash is gone. But SDIO
>>>> doesn't seem to work any more:
>>>>
>>>> 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: command 0x0003 timed out
>>>> libertas: requeueing command 0x0003 due to timeout (#1)
>>>> libertas: command 0x0003 timed out
>>>> libertas: requeueing command 0x0003 due to timeout (#2)
>>>> libertas: command 0x0003 timed out
>>>> libertas: requeueing command 0x0003 due to timeout (#3)
>>>> libertas: command 0x0003 timed out
>>>> libertas: Excessive timeouts submitting command 0x0003
>>>> libertas: PREP_CMD: command 0x0003 failed: -110
>>>> libertas_sdio: probe of mmc1:0001:1 failed with error -110
>>>>
>>>> I now start to think about it ;)
>>>>
>>>> Again, any help would be appreciated!
>>>>
>>>> Best regards
>>>>
>>>> Dirk
>>>>
>>>>> On Fri, Oct 16, 2009 at 1:16 AM, Dirk Behme <dirk.behme@googlemail.com>
>>>>> wrote:
>>>>>> John Rigby wrote:
>>>>>>> I have seen several discussions about lack of sdio irq support in the
>>>>>>> hsmmc driver but no patches.  Has anyone on this list implemented this
>>>>>>> and/or can anyone point me to patches?
>>>>>> What a coincidence ;)
>>>>>>
>>>>>> I'm currently working on this. See attachment what I currently have. It
>>>>>> is
>>>>>> compile tested only against recent omap linux head. I don't have a
>>>>>> board
>>>>>> using SDIO at the moment, so no real testing possible :(
>>>>>>
>>>>>> Some background, maybe it helps people to step in:
>>>>>>
>>>>>> Gumstix OMAP3 based Overo air board connects Marvell 88W8686 wifi by
>>>>>> MMC
>>>>>> port 2 in 4 bit configuration [1]. The wifi performance is quite bad
>>>>>> (~100kB/s). There is some rumor that this might be SDIO irq related
>>>>>> [2].
>>>>>> There was an attempt to fix this [3] already, but this doesn't work
>>>>>> [4].
>>>>>> Having this, I started to look into it.
>>>>>>
>>>>>> I used [3], the TI Davinci driver [5] (supporting SDIO irq), the SDIO
>>>>>> Simplified Specification [6] and the OMAP35x TRM [7] as starting
>>>>>> points.
>>>>>>
>>>>>> Unfortunately, the Davinci MMC registers and irqs are different
>>>>>> (Davinci
>>>>>> has
>>>>>> a dedicated SDIO irq). But combining [3] and [5] helps to get an idea
>>>>>> what
>>>>>> has to be done.
>>>>>>
>>>>>> I think the main issues of [3] were that it doesn't enable IBG for 4
>>>>>> bit
>>>>>> mode ([6] chapter 8.1.2) and that mmc_omap_irq() doesn't reset the irq
>>>>>> bits.
>>>>>>
>>>>>> Topics I still open:
>>>>>>
>>>>>> - Is it always necessary to deal with IE _and_ ISE register? I'm not
>>>>>> totally
>>>>>> clear what the difference between these two registers are ;) And in
>>>>>> which
>>>>>> order they have to be set.
>>>>>>
>>>>>> - Davinci driver [5] in line 1115 checks for data line to call
>>>>>> mmc_signal_sdio_irq() for irq enable.
>>>>>>
>>>>>> - Davinci driver deals with SDIO in xfer_done() (line 873)
>>>>>>
>>>>>> - Davinci driver sets block size to 64 if SDIO in line 701
>>>>>>
>>>>>> It would be quite nice if anybody likes to comment on attachment and
>>>>>> help
>>>>>> testing.
>>>>>>
>>>>>> Many thanks and best regards
>>>>>>
>>>>>> Dirk
>>>>>>
>>>>>> [1] http://gumstix.net/wiki/index.php?title=Overo_Wifi
>>>>>>
>>>>>> [2] http://groups.google.com/group/beagleboard/msg/14e822778c5eeb56
>>>>>>
>>>>>> [3] http://groups.google.com/group/beagleboard/msg/d0eb69f4c20673be
>>>>>>
>>>>>> [4] http://groups.google.com/group/beagleboard/msg/5cdfe2a319531937
>>>>>>
>>>>>> [5]
>>>>>>
>>>>>>
>>>>>> http://arago-project.org/git/projects/?p=linux-davinci.git;a=blob;f=drivers/mmc/host/davinci_mmc.c;h=1bf0587250614c6d8abfe02028b96e0e47148ac8;hb=HEAD
>>>>>>
>>>>>> [6] http://www.sdcard.org/developers/tech/sdio/sd_bluetooth_spec/
>>>>>>
>>>>>> [7] http://focus.ti.com/lit/ug/spruf98c/spruf98c.pdf
>>>>>>
>>>>>>
>>>>>> Subject: [PATCH][RFC] OMAP HSMMC: Add SDIO interrupt support
>>>>>> Form: Dirk Behme <dirk.behme@googlemail.com>
>>>>>>
>>>>>> At the moment, OMAP HSMMC driver supports only SDIO polling, resulting
>>>>>> in
>>>>>> poor
>>>>>> performance. Add support for SDIO interrupt handling.
>>>>>>
>>>>>> Signed-off-by: Dirk Behme <dirk.behme@googlemail.com>
>>>>>> ---
>>>>>>
>>>>>> Patch against recent omap-linux head "Linux omap got rebuilt from
>>>>>> scratch"
>>>>>> 274c94b29ee7c53609a756acca974e4742c59559
>>>>>>
>>>>>> Compile tested only. Please comment and help testing.
>>>>>>
>>>>>>  drivers/mmc/host/omap_hsmmc.c |   48
>>>>>> +++++++++++++++++++++++++++++++++++++-----
>>>>>>  1 file changed, 43 insertions(+), 5 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
>>>>>> @@ -27,6 +27,7 @@
>>>>>>  #include <linux/timer.h>
>>>>>>  #include <linux/clk.h>
>>>>>>  #include <linux/mmc/host.h>
>>>>>> +#include <linux/mmc/card.h>
>>>>>>  #include <linux/mmc/core.h>
>>>>>>  #include <linux/io.h>
>>>>>>  #include <linux/semaphore.h>
>>>>>> @@ -65,6 +66,7 @@
>>>>>>  #define SDVSDET                        0x00000400
>>>>>>  #define AUTOIDLE               0x1
>>>>>>  #define SDBP                   (1 << 8)
>>>>>> +#define IBG                    (1 << 19)
>>>>>>  #define DTO                    0xe
>>>>>>  #define ICE                    0x1
>>>>>>  #define ICS                    0x2
>>>>>> @@ -76,6 +78,7 @@
>>>>>>  #define INT_EN_MASK            0x307F0033
>>>>>>  #define BWR_ENABLE             (1 << 4)
>>>>>>  #define BRR_ENABLE             (1 << 5)
>>>>>> +#define CIRQ_ENABLE            (1 << 8)
>>>>>>  #define INIT_STREAM            (1 << 1)
>>>>>>  #define DP_SELECT              (1 << 21)
>>>>>>  #define DDIR                   (1 << 4)
>>>>>> @@ -87,6 +90,7 @@
>>>>>>  #define CC                     0x1
>>>>>>  #define TC                     0x02
>>>>>>  #define OD                     0x1
>>>>>> +#define CIRQ                   (1 <<  8)
>>>>>>  #define ERR                    (1 << 15)
>>>>>>  #define CMD_TIMEOUT            (1 << 16)
>>>>>>  #define DATA_TIMEOUT           (1 << 20)
>>>>>> @@ -653,6 +657,15 @@ static irqreturn_t omap_hsmmc_irq(int ir
>>>>>>      status = OMAP_HSMMC_READ(host->base, STAT);
>>>>>>      dev_dbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
>>>>>>
>>>>>> +       if (status & CIRQ) {
>>>>>> +               dev_dbg(mmc_dev(host->mmc), "SDIO interrupt");
>>>>>> +               OMAP_HSMMC_WRITE(host->base, IE,
>>>>>> OMAP_HSMMC_READ(host->base,
>>>>>> IE)
>>>>>> +                                & ~(CIRQ_ENABLE));
>>>>>> +               mmc_signal_sdio_irq(host->mmc);
>>>>>> +               spin_unlock(&host->irq_lock);
>>>>>> +               return IRQ_HANDLED;
>>>>>> +       }
>>>>>> +
>>>>>>      if (status & ERR) {
>>>>>>  #ifdef CONFIG_MMC_DEBUG
>>>>>>              omap_hsmmc_report_irq(host, status);
>>>>>> @@ -1165,8 +1178,15 @@ static void omap_hsmmc_set_ios(struct mm
>>>>>>              break;
>>>>>>      case MMC_BUS_WIDTH_4:
>>>>>>              OMAP_HSMMC_WRITE(host->base, CON, con & ~DW8);
>>>>>> -               OMAP_HSMMC_WRITE(host->base, HCTL,
>>>>>> -                       OMAP_HSMMC_READ(host->base, HCTL) | FOUR_BIT);
>>>>>> +               if (mmc_card_sdio(host->mmc->card)) {
>>>>>> +                       OMAP_HSMMC_WRITE(host->base, HCTL,
>>>>>> +                                        OMAP_HSMMC_READ(host->base,
>>>>>> HCTL)
>>>>>> +                                        | IBG | FOUR_BIT);
>>>>>> +               } else {
>>>>>> +                       OMAP_HSMMC_WRITE(host->base, HCTL,
>>>>>> +                                        OMAP_HSMMC_READ(host->base,
>>>>>> HCTL)
>>>>>> +                                        | FOUR_BIT);
>>>>>> +               }
>>>>>>              break;
>>>>>>      case MMC_BUS_WIDTH_1:
>>>>>>              OMAP_HSMMC_WRITE(host->base, CON, con & ~DW8);
>>>>>> @@ -1512,6 +1532,24 @@ static int omap_hsmmc_disable_fclk(struc
>>>>>>      return 0;
>>>>>>  }
>>>>>>
>>>>>> +static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int
>>>>>> enable)
>>>>>> +{
>>>>>> +       struct omap_hsmmc_host *host = mmc_priv(mmc);
>>>>>> +       u32 ie, ise;
>>>>>> +
>>>>>> +       ise = OMAP_HSMMC_READ(host->base, ISE);
>>>>>> +       ie  = OMAP_HSMMC_READ(host->base, IE);
>>>>>> +
>>>>>> +       if (enable) {
>>>>>> +               OMAP_HSMMC_WRITE(host->base, ISE, ise | CIRQ_ENABLE);
>>>>>> +               OMAP_HSMMC_WRITE(host->base, IE,  ie  | CIRQ_ENABLE);
>>>>>> +       } else {
>>>>>> +               OMAP_HSMMC_WRITE(host->base, ISE, ise & ~CIRQ_ENABLE);
>>>>>> +               OMAP_HSMMC_WRITE(host->base, IE,  ie  & ~CIRQ_ENABLE);
>>>>>> +       }
>>>>>> +
>>>>>> +}
>>>>>> +
>>>>>>  static const struct mmc_host_ops omap_hsmmc_ops = {
>>>>>>      .enable = omap_hsmmc_enable_fclk,
>>>>>>      .disable = omap_hsmmc_disable_fclk,
>>>>>> @@ -1519,7 +1557,7 @@ static const struct mmc_host_ops omap_hs
>>>>>>      .set_ios = omap_hsmmc_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 const struct mmc_host_ops omap_hsmmc_ps_ops = {
>>>>>> @@ -1529,7 +1567,7 @@ static const struct mmc_host_ops omap_hs
>>>>>>      .set_ios = omap_hsmmc_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,
>>>>>>  };
>>>>>>
>>>>>>  #ifdef CONFIG_DEBUG_FS
>>>>>> @@ -1734,7 +1772,7 @@ static int __init omap_hsmmc_probe(struc
>>>>>>      mmc->max_seg_size = mmc->max_req_size;
>>>>>>
>>>>>>      mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
>>>>>> -                    MMC_CAP_WAIT_WHILE_BUSY;
>>>>>> +                    MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_SDIO_IRQ;
>>>>>>
>>>>>>      if (mmc_slot(host).wires >= 8)
>>>>>>              mmc->caps |= MMC_CAP_8_BIT_DATA;
>>>>>>
>>>>>>
>>
>