Switch to 1-bit mode SDIO before disabling clocks.

Every call to sdio_enable_4bit_bus is followed (on success) but a call

to mmc_set_bus_width().

To simplify the code, include those calls directly in sdio_enable_4bit_bus().

Signed-off-by NeilBrown <[email protected]>

---
 drivers/mmc/core/sdio.c |   32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

drivers/mmc/core/sdio.c +12 -20

index ce6cc47206b0..5bc6c7dbbd60 100644
@@ -293,19 +293,22 @@ static int sdio_enable_4bit_bus(struct mmc_card *card)
        int err;
 
        if (card->type == MMC_TYPE_SDIO)
-               return sdio_enable_wide(card);
-
-       if ((card->host->caps & MMC_CAP_4_BIT_DATA) &&
-               (card->scr.bus_widths & SD_SCR_BUS_WIDTH_4)) {
+               err = sdio_enable_wide(card);
+       else if ((card->host->caps & MMC_CAP_4_BIT_DATA) &&
+                (card->scr.bus_widths & SD_SCR_BUS_WIDTH_4)) {
                err = mmc_app_set_bus_width(card, MMC_BUS_WIDTH_4);
                if (err)
                        return err;
+               err = sdio_enable_wide(card);
+               if (err <= 0)
+                       mmc_app_set_bus_width(card, MMC_BUS_WIDTH_1);
        } else
                return 0;
 
-       err = sdio_enable_wide(card);
-       if (err <= 0)
-               mmc_app_set_bus_width(card, MMC_BUS_WIDTH_1);
+       if (err > 0) {
+               mmc_set_bus_width(card->host, MMC_BUS_WIDTH_4);
+               err = 0;
+       }
 
        return err;
 }

@@ -547,13 +550,8 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
        /*
         * Switch to wider bus (if supported).
         */
-       if (card->host->caps & MMC_CAP_4_BIT_DATA) {
+       if (card->host->caps & MMC_CAP_4_BIT_DATA)
                err = sdio_enable_4bit_bus(card);
-               if (err > 0) {
-                       mmc_set_bus_width(card->host, MMC_BUS_WIDTH_4);
-                       err = 0;
-               }
-       }
 
        /* Set the driver strength for the card */
        sdio_select_driver_type(card);

@@ -803,9 +801,7 @@ try_again:
                 * Switch to wider bus (if supported).
                 */
                err = sdio_enable_4bit_bus(card);
-               if (err > 0)
-                       mmc_set_bus_width(card->host, MMC_BUS_WIDTH_4);
-               else if (err)
+               if (err)
                        goto remove;
        }
 finish:

@@ -983,10 +979,6 @@ static int mmc_sdio_resume(struct mmc_host *host)
        } else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
                /* We may have switched to 1-bit mode during suspend */
                err = sdio_enable_4bit_bus(host->card);
-               if (err > 0) {
-                       mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
-                       err = 0;
-               }
        }
 
        if (!err && host->sdio_irqs) {

Change the handling for the 'abort' flag so that if it is set, but we can claim the host, then do the claim, rather than aborting.

When the abort is async this just means that a race between aborting an allowing a claim is resolved slightly differently. Any code must already be able to handle 'abort' being set just as the host is claimed.

This allows extra functionality. If __mmc_claim_host() is called with an 'abort' pointer which is initialized to '1', it will effect a non-blocking 'claim'.

Signed-off-by NeilBrown <[email protected]>

---
 drivers/mmc/core/core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/mmc/core/core.c +0 -0

index 23f10f72e5f3..541c8903dc6b 100644

Quoted 23 lines · NeilBrown

Change the handling for the 'abort' flag so that if it is set, but we can claim the host, then do the claim, rather than aborting.

When the abort is async this just means that a race between aborting an allowing a claim is resolved slightly differently. Any code must already be able to handle 'abort' being set just as the host is claimed.

This allows extra functionality. If __mmc_claim_host() is called with an 'abort' pointer which is initialized to '1', it will effect a non-blocking 'claim'.

Signed-off-by NeilBrown <[email protected]>

---
 drivers/mmc/core/core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/mmc/core/core.c +0 -0

index 23f10f72e5f3..541c8903dc6b 100644

drivers/mmc/core/core.c int __mmc_claim_host(struct mmc_host *host, atomic_t *abort) · line 915

Quoted 5 lines

drivers/mmc/core/core.c +1 -1

@@ -912,10 +912,11 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
                spin_lock_irqsave(&host->lock, flags);
        }
        set_current_state(TASK_RUNNING);
-       if (!stop) {
+       if (!host->claimed || host->claimer == current) {

This seems risky in that regards that it will change the behaviour for the sdio_irq_thread(). Did you check that?

I guess we could change the sdio_irq_thread() to read it's sdio_irq_thread_abort value before trying to claim the host?

According to section 7.1.2 of

http://www.sandisk.com/media/File/OEM/Manuals/SD_SDIO_specsv1.pdf

In the case where the interrupt mechanism is used to wake the host while the card is in a low power state (i.e. no clocks), Both the card and the host shall be placed into the 1-bit SD mode prior to stopping the clock.

This is particularly important for the Marvell "libertas" wifi chip in the GTA04. While in 4-bit mode it will only signal an interrupt when the clock is running (which is why setting CLKEXTFREE is important).
In 1-bit mode, the interrupt is asynchronous (explained in OMAP3 TRM description of the CIRQ flag to MMCHS_STAT:

In 1-bit mode, interrupt source is asynchronous (can be a source of asynchronous wakeup).
In 4-bit mode, interrupt source is sampled during the interrupt cycle.

)

It is awkward to simply set 1-bit mode in ->runtime_suspend as that will call mmc_set_ios which calls ops->set_ios(), which will likely call pm_runtime_get_sync(), on the device that is currently suspending. This deadlocks.

So:
- create a work_struct to schedule setting of 1-bit mode
- introduce an 'sdio_narrowed' state flag which transitions:

     0 (normal) -> 1 (convert to 1-bit pending) ->
         2 (have switch to 1-bit mode) -> 0 (normal)

- create a function mmc_sdio_want_no_clocks() which can be called when the driver wants to turn off clocks (presumably after an idle timeout). This either succeeds (in 1-bit mode) or fails and schedules the work to switch to 1-bit mode.
- when the host is claimed, if sdio_narrowed is 2, restore the 4-bit bus
- When the host is released, if sdio_narrowed is 1, then some caller other than our worker claimed the host first, so clear sdio_narrowed.

This all allows a graceful and race-free switch to 1-bit mode before switching off the clocks, if SDIO interrupts are enabled.

A host should call mmc_sdio_want_no_clocks() when about to turn off clocks if sdio interrupts are enabled, and the ->disable() function should not use a timeout (pm_runtime_put_autosuspend) if ->sdio_narrowed is 2.

Quoted 42 lines · NeilBrown

According to section 7.1.2 of

http://www.sandisk.com/media/File/OEM/Manuals/SD_SDIO_specsv1.pdf

In the case where the interrupt mechanism is used to wake the host while the card is in a low power state (i.e. no clocks), Both the card and the host shall be placed into the 1-bit SD mode prior to stopping the clock.

This is particularly important for the Marvell "libertas" wifi chip in the GTA04. While in 4-bit mode it will only signal an interrupt when the clock is running (which is why setting CLKEXTFREE is important).
In 1-bit mode, the interrupt is asynchronous (explained in OMAP3 TRM description of the CIRQ flag to MMCHS_STAT:

In 1-bit mode, interrupt source is asynchronous (can be a source of asynchronous wakeup).
In 4-bit mode, interrupt source is sampled during the interrupt cycle.

)

It is awkward to simply set 1-bit mode in ->runtime_suspend as that will call mmc_set_ios which calls ops->set_ios(), which will likely call pm_runtime_get_sync(), on the device that is currently suspending. This deadlocks.

So:
- create a work_struct to schedule setting of 1-bit mode
- introduce an 'sdio_narrowed' state flag which transitions:

     0 (normal) -> 1 (convert to 1-bit pending) ->
         2 (have switch to 1-bit mode) -> 0 (normal)

- create a function mmc_sdio_want_no_clocks() which can be called when the driver wants to turn off clocks (presumably after an idle timeout). This either succeeds (in 1-bit mode) or fails and schedules the work to switch to 1-bit mode.
- when the host is claimed, if sdio_narrowed is 2, restore the 4-bit bus
- When the host is released, if sdio_narrowed is 1, then some caller other than our worker claimed the host first, so clear sdio_narrowed.

Quoted 7 lines

This all allows a graceful and race-free switch to 1-bit mode before switching off the clocks, if SDIO interrupts are enabled.

A host should call mmc_sdio_want_no_clocks() when about to turn off clocks if sdio interrupts are enabled, and the ->disable() function should not use a timeout (pm_runtime_put_autosuspend) if ->sdio_narrowed is 2.

Wow! A mystery finally solved for why libertas_sdio using devices like overo won't work for the wakeirqs.. The interface has to be in 1-bit mode for libertas to produce any SDIO interrupts..

Below is a patch enabling some more SDIO wakeirqs. Seems to work on overo now too :) So tor the whole series, please feel free to add:

Tested-by Tony Lindgren <[email protected]>

8< -------------------
From: Tony Lindgren <[email protected]>
Date: Thu, 26 Feb 2015 16:16:03 -0800
Subject: [PATCH] ARM: dts: Fix omap3 SDIO wakeirqs for devices using sdio_libertas

Turns out the the MMC interface needs to be in 1-bit mode for the libertas card to send any SDIO interrupts as pointed out by NeilBrown <[email protected]>. Now that the MMC framework is getting fixed for setting 1-bit mode for idle, we can enable SDIO wakeirqs for libertas using devices too.

Cc Andreas Fenkart <[email protected]>

Cc Ash Charles <[email protected]>

Cc Florian Vaussard <[email protected]>

Cc NeilBrown <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

arch/arm/boot/dts/omap3-gta04.dtsi line 373

Quoted 11 lines · On Tue, 3 Mar 2015 14:53:55 -0800 Tony Lindgren <[email protected]> wrote:

arch/arm/boot/dts/omap3-gta04.dtsi +1 -0

@@ -367,6 +367,7 @@
 };
 
 &mmc2 {
+	interrupts-extended = <&intc 86 &omap3_pmx_core 0x12e>;
 	vmmc-supply = <&vaux4>;
 	bus-width = <4>;
 	ti,non-removable;

I had

(quoted diff) +4 -0

+       interrupts-extended = <&intc 86 &gpio5 5 0>; /* GPIO_133 */
+       pinctrl-names = "default", "idle";
+       pinctrl-0 = <&mmc2_pins>;
+       pinctrl-1 = <&mmc2_cirq_pin>;

together with

(quoted diff) +6 -0

+       mmc2_cirq_pin: pinmux_cirq_pin {
+               pinctrl-single,pins = <
+                       OMAP3_CORE1_IOPAD(0x215e, PIN_INPUT_PULLUP | MUX_MODE4) 
+               >;
+       };
+

and a longer definition for mmc2_pins.

Is that one line reconfigure the pin on demand? How does that work?

According to section 7.1.2 of

http://www.sandisk.com/media/File/OEM/Manuals/SD_SDIO_specsv1.pdf

In the case where the interrupt mechanism is used to wake the host while the card is in a low power state (i.e. no clocks), Both the card and the host shall be placed into the 1-bit SD mode prior to stopping the clock.

This is particularly important for the Marvell "libertas" wifi chip in the GTA04. While in 4-bit mode it will only signal an interrupt when the clock is running (which is why setting CLKEXTFREE is important).
In 1-bit mode, the interrupt is asynchronous (explained in OMAP3 TRM description of the CIRQ flag to MMCHS_STAT:

In 1-bit mode, interrupt source is asynchronous (can be a source of asynchronous wakeup).
In 4-bit mode, interrupt source is sampled during the interrupt cycle.

)

It is awkward to simply set 1-bit mode in ->runtime_suspend as that will call mmc_set_ios which calls ops->set_ios(), which will likely call pm_runtime_get_sync(), on the device that is currently suspending. This deadlocks.

So:
- create a work_struct to schedule setting of 1-bit mode
- introduce an 'sdio_narrowed' state flag which transitions:

     0 (normal) -> 1 (convert to 1-bit pending) ->
         2 (have switch to 1-bit mode) -> 0 (normal)

- create a function mmc_sdio_want_no_clocks() which can be called when the driver wants to turn off clocks (presumably after an idle timeout). This either succeeds (in 1-bit mode) or fails and schedules the work to switch to 1-bit mode.
- when the host is claimed, if sdio_narrowed is 2, restore the 4-bit bus
- When the host is released, if sdio_narrowed is 1, then some caller other than our worker claimed the host first, so clear sdio_narrowed.

This all allows a graceful and race-free switch to 1-bit mode before switching off the clocks, if SDIO interrupts are enabled.

A host should call mmc_sdio_want_no_clocks() when about to turn off clocks if sdio interrupts are enabled, and the ->disable() function should not use a timeout (pm_runtime_put_autosuspend) if ->sdio_narrowed is 2.

Signed-off-by NeilBrown <[email protected]>

---
 drivers/mmc/core/core.c  |   18 ++++++++++++++----
 drivers/mmc/core/sdio.c  |   42 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/mmc/core.h |    2 ++
 include/linux/mmc/host.h |    2 ++
 4 files changed, 59 insertions(+), 5 deletions(-)

drivers/mmc/core/core.c +8 -2

index 541c8903dc6b..0258fdf1a03d 100644
@@ -921,8 +921,14 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
                wake_up(&host->wq);
        spin_unlock_irqrestore(&host->lock, flags);
        remove_wait_queue(&host->wq, &wait);
-       if (host->ops->enable && !stop && host->claim_cnt == 1)
-               host->ops->enable(host);
+       if (!stop && host->claim_cnt == 1) {
+               if (host->ops->enable)
+                       host->ops->enable(host);
+               if (atomic_read(&host->sdio_narrowed) == 2) {
+                       sdio_enable_4bit_bus(host->card);
+                       atomic_set(&host->sdio_narrowed, 0);
+               }
+       }
        return stop;
 }
 

@@ -941,8 +947,12 @@ void mmc_release_host(struct mmc_host *host)
 

drivers/mmc/core/core.c +0 -0

        spin_lock_irqsave(&host->lock, flags);
        if (--host->claim_cnt) {

drivers/mmc/core/sdio.c +41 -1

index 5bc6c7dbbd60..9761e4d5f49b 100644
@@ -288,7 +288,7 @@ static int sdio_disable_wide(struct mmc_card *card)
 }
 
 
-static int sdio_enable_4bit_bus(struct mmc_card *card)
+int sdio_enable_4bit_bus(struct mmc_card *card)
 {
        int err;
 

@@ -313,6 +313,45 @@ static int sdio_enable_4bit_bus(struct mmc_card *card)
        return err;
 }
 
+static void mmc_sdio_width_work(struct work_struct *work)
+{
+       struct mmc_host *host = container_of(work, struct mmc_host,
+                                            sdio_width_work);
+       atomic_t noblock;
+
+       atomic_set(&noblock, 1);
+       if (__mmc_claim_host(host, &noblock))
+               return;
+       if (atomic_read(&host->sdio_narrowed) != 1) {
+               /* Nothing to do */
+               mmc_release_host(host);
+               return;
+       }
+       if (sdio_disable_wide(host->card) == 0)
+               atomic_set(&host->sdio_narrowed, 2);
+       else
+               atomic_set(&host->sdio_narrowed, 0);
+       mmc_release_host(host);
+}
+
+int mmc_sdio_want_no_clocks(struct mmc_host *host)
+{
+       if (!(host->caps & MMC_CAP_SDIO_IRQ) ||
+           host->ios.bus_width == MMC_BUS_WIDTH_1)
+               /* Safe to turn off clocks */
+               return 1;
+
+
+       /* In 4-bit mode the card needs the clock
+        * to deliver interrupts, so it isn't safe
+        * to turn of clocks just yet
+        */
+       atomic_add_unless(&host->sdio_narrowed, 1, 1);
+
+       schedule_work(&host->sdio_width_work);
+       return 0;
+}
+EXPORT_SYMBOL_GPL(mmc_sdio_want_no_clocks);
 
 /*
  * Test if the card supports high-speed mode and, if so, switch to it.

@@ -1100,6 +1139,7 @@ int mmc_attach_sdio(struct mmc_host *host)
                goto err;
        }
 
+       INIT_WORK(&host->sdio_width_work, mmc_sdio_width_work);
        /*
         * Detect and init the card.
         */

include/linux/mmc/core.h +2 -0

index 160448f920ac..faf6d1be0971 100644
@@ -212,4 +212,6 @@ struct device_node;
 extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
 extern int mmc_of_parse_voltage(struct device_node *np, u32 *mask);
 
+extern int sdio_enable_4bit_bus(struct mmc_card *card);
+extern int mmc_sdio_want_no_clocks(struct mmc_host *host);
 #endif /* LINUX_MMC_CORE_H */

include/linux/mmc/host.h +0 -0

index 0c8cbe5d1550..7e6a54c49a15 100644

Quoted 4 lines · NeilBrown

drivers/mmc/core/core.c +0 -0

@@ -941,8 +947,12 @@ void mmc_release_host(struct mmc_host *host)
 
        WARN_ON(!host->claimed);
 

drivers/mmc/core/core.c void mmc_release_host(struct mmc_host *host) · line 955

Quoted 8 lines

drivers/mmc/core/core.c +6 -2

-       if (host->ops->disable && host->claim_cnt == 1)
-               host->ops->disable(host);
+       if (host->claim_cnt == 1) {
+               if (atomic_read(&host->sdio_narrowed) == 1)
+                       atomic_set(&host->sdio_narrowed, 0);
+               if (host->ops->disable)
+                       host->ops->disable(host);
+       }

As omap_hsmmc in currently the only user of the host_ops->enable|disable() callbacks, I wonder if this approach will work "race-free" for those hosts that don't implement these callbacks?