[PATCH 0/4] Switch to 1-bit mode SDIO before disabling clocks.

[PATCH 0/4] Switch to 1-bit mode SDIO before disabling clocks.

[NeilBrown]

This is a reworking of this code that I promised at the end of
January.

SDIO interrupt in 4-bit mode require the clock to be running.  So we
need to switch to 1-bit mode before turning off the clock.

This series provides infrastructure in mmc-core, and adds the
functionality to omap_hsmmc.

Thanks,
NeilBrown


---

NeilBrown (4):
      mmc: core: fold mmc_set_bus_width calls into sdio_enable_4bit_bus.
      mmc: core: allow non-blocking form of mmc_claim_host
      mmc: sdio: support switching to 1-bit before turning off clocks
      mmc: omap_hsmmc: switch to 1-bit before stopping clocks.


 drivers/mmc/core/core.c       |   21 +++++++++---
 drivers/mmc/core/sdio.c       |   74 +++++++++++++++++++++++++++++------------
 drivers/mmc/host/omap_hsmmc.c |   13 ++++++-
 include/linux/mmc/core.h      |    2 +
 include/linux/mmc/host.h      |    2 +
 5 files changed, 83 insertions(+), 29 deletions(-)

--
Signature

[PATCH 3/4] mmc: sdio: support switching to 1-bit before turning off clocks

[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.

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 <neil@brown.name>
---
 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(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 541c8903dc6b..0258fdf1a03d 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -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)
 
 	WARN_ON(!host->claimed);
 
-	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);
+	}
 
 	spin_lock_irqsave(&host->lock, flags);
 	if (--host->claim_cnt) {
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 5bc6c7dbbd60..9761e4d5f49b 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -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.
 	 */
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 160448f920ac..faf6d1be0971 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -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 */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c8cbe5d1550..7e6a54c49a15 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -350,6 +350,8 @@ struct mmc_host {
 	struct task_struct	*sdio_irq_thread;
 	bool			sdio_irq_pending;
 	atomic_t		sdio_irq_thread_abort;
+	struct work_struct	sdio_width_work;
+	atomic_t		sdio_narrowed; /* 1==pending, 2==complete*/
 
 	mmc_pm_flag_t		pm_flags;	/* requested pm features */
 

Re: [PATCH 3/4] mmc: sdio: support switching to 1-bit before turning off clocks

[Tony Lindgren]

* NeilBrown <neilb@suse.de> [150223 18:47]:
> 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.

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 <tony@atomide.com>

8< -------------------
From: Tony Lindgren <tony@atomide.com>
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 <neilb@suse.de>. 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 <afenkart@gmail.com>
Cc: Ash Charles <ash@gumstix.com>
Cc: Florian Vaussard <florian.vaussard@epfl.ch>
Cc: NeilBrown <neil@brown.name>
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -367,6 +367,7 @@
 };
 
 &mmc2 {
+	interrupts-extended = <&intc 86 &omap3_pmx_core 0x12e>;
 	vmmc-supply = <&vaux4>;
 	bus-width = <4>;
 	ti,non-removable;
--- a/arch/arm/boot/dts/omap3-overo-base.dtsi
+++ b/arch/arm/boot/dts/omap3-overo-base.dtsi
@@ -185,6 +185,7 @@
 &mmc2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc2_pins>;
+	interrupts-extended = <&intc 86 &omap3_pmx_core 0x12e>;
 	vmmc-supply = <&w3cbw003c_npoweron>;
 	vqmmc-supply = <&w3cbw003c_bt_nreset>;
 	vmmc_aux-supply = <&w3cbw003c_wifi_nreset>;

Re: [PATCH 3/4] mmc: sdio: support switching to 1-bit before turning off clocks

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4452 bytes --]

On Tue, 3 Mar 2015 14:53:55 -0800 Tony Lindgren <tony@atomide.com> wrote:

> * NeilBrown <neilb@suse.de> [150223 18:47]:
> > 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.
> 
> 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 <tony@atomide.com>

Thanks a lot!


> 
> 8< -------------------
> From: Tony Lindgren <tony@atomide.com>
> 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 <neilb@suse.de>. 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 <afenkart@gmail.com>
> Cc: Ash Charles <ash@gumstix.com>
> Cc: Florian Vaussard <florian.vaussard@epfl.ch>
> Cc: NeilBrown <neil@brown.name>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -367,6 +367,7 @@
>  };
>  
>  &mmc2 {
> +	interrupts-extended = <&intc 86 &omap3_pmx_core 0x12e>;
>  	vmmc-supply = <&vaux4>;
>  	bus-width = <4>;
>  	ti,non-removable;

I had 

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

together with

+       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?


Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 3/4] mmc: sdio: support switching to 1-bit before turning off clocks

[Tony Lindgren]

* NeilBrown <neilb@suse.de> [150303 21:29]:
> On Tue, 3 Mar 2015 14:53:55 -0800 Tony Lindgren <tony@atomide.com> wrote:
> > 
> > --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> > +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> > @@ -367,6 +367,7 @@
> >  };
> >  
> >  &mmc2 {
> > +	interrupts-extended = <&intc 86 &omap3_pmx_core 0x12e>;
> >  	vmmc-supply = <&vaux4>;
> >  	bus-width = <4>;
> >  	ti,non-removable;
> 
> I had 
> 
> +       interrupts-extended = <&intc 86 &gpio5 5 0>; /* GPIO_133 */
> +       pinctrl-names = "default", "idle";
> +       pinctrl-0 = <&mmc2_pins>;
> +       pinctrl-1 = <&mmc2_cirq_pin>;
> 
> together with
> 
> +       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?

Well the remuxing of the mmc2 dat1 pin is not needed on omap3/4/5.
These SoCs have the iochain wake-up events.

Remuxing is needed on SoCs with no iochain wake-up events, such as
am355x, am437x and dra7 variants.

Not sure if we actually have the gpio5 bank wake-up the system
properly currently from off-idle. It could be that only gpio1 bank
is currently capable of wake-up from off-idle without needing
to use the iochain wake-up also for GPIO lines.

Regards,

Tony

Re: [PATCH 3/4] mmc: sdio: support switching to 1-bit before turning off clocks

[Ulf Hansson]

On 24 February 2015 at 03:42, NeilBrown <neilb@suse.de> wrote:
> 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 <neil@brown.name>

Hi Neil,

Thanks for sending this patchset, and sorry for the delay in response.

Overall I like the approach taken in this patchset, though I have some
questions see below.

> ---
>  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(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 541c8903dc6b..0258fdf1a03d 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -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)
>
>         WARN_ON(!host->claimed);
>
> -       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?

Also, I was kind of hoping that we should be able to remove these
host_ops callbacks, when omap_hsmmc has moved away from using them. Do
you think that can be done?

Within this context, I am also reviewing Adrian Hunter's patchset
around "periodic re-tuning" [1] and to me, you guys are sharing a
similar issue.

The host driver's runtime PM suspend callback needs to be able to
notify the mmc core to take some specific actions, and in a
"race-free" manner.

I wonder whether we could have one common solution to that? Do you
think this approach will work for Adrian's "periodic re-tuning" as
well?

>
>         spin_lock_irqsave(&host->lock, flags);
>         if (--host->claim_cnt) {
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 5bc6c7dbbd60..9761e4d5f49b 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -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.
>          */
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 160448f920ac..faf6d1be0971 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -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 */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0c8cbe5d1550..7e6a54c49a15 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -350,6 +350,8 @@ struct mmc_host {
>         struct task_struct      *sdio_irq_thread;
>         bool                    sdio_irq_pending;
>         atomic_t                sdio_irq_thread_abort;
> +       struct work_struct      sdio_width_work;
> +       atomic_t                sdio_narrowed; /* 1==pending, 2==complete*/
>
>         mmc_pm_flag_t           pm_flags;       /* requested pm features */
>
>
>

Kind regards
Uffe

[1]
[PATCH V3 00/15] mmc: host: Add facility to support re-tuning
http://www.spinics.net/lists/linux-mmc/msg31020.html

Re: [PATCH 3/4] mmc: sdio: support switching to 1-bit before turning off clocks

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

On Mon, 23 Mar 2015 10:10:18 +0100 Ulf Hansson <ulf.hansson@linaro.org> wrote:

> On 24 February 2015 at 03:42, NeilBrown <neilb@suse.de> wrote:

> > @@ -941,8 +947,12 @@ void mmc_release_host(struct mmc_host *host)
> >
> >         WARN_ON(!host->claimed);
> >
> > -       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?

Hi Ulf,
 you might have guessed, but to be explicit: I withdraw this patchset.
I much prefer the other approach that I suggested using runtime PM on
the mmc_host device and switching to 1-bit more in the pm_runtime_suspend
function.

As you say, omap_hsmmc is the only user of enable/disable and they were an
important part of my strategy in this patchset.
Which raises the question:  what is omap_hsmmc using enable/disable?

My guess is that it is largely historical - added because omap_hsmmc wanted
runtime pm before sufficient generic infrastructure was available.

I've tested a patch to remove it, and it seem fine.  I'll post it.

Thanks,
NeilBrown


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH 1/4] mmc: core: fold mmc_set_bus_width calls into sdio_enable_4bit_bus.

[NeilBrown]

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 <neil@brown.name>
---
 drivers/mmc/core/sdio.c |   32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ce6cc47206b0..5bc6c7dbbd60 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -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) {

Re: [PATCH 1/4] mmc: core: fold mmc_set_bus_width calls into sdio_enable_4bit_bus.

[Ulf Hansson]

On 24 February 2015 at 03:42, NeilBrown <neilb@suse.de> wrote:
> Every call to sdio_enable_4bit_bus is followed (on success) but a call

/s /but / by

> to mmc_set_bus_width().
>
> To simplify the code, include those calls directly in
> sdio_enable_4bit_bus().
>
> Signed-off-by: NeilBrown <neil@brown.name>

Nice cleanup! Applied for next with the above minor change.

Kind regards
Uffe

> ---
>  drivers/mmc/core/sdio.c |   32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index ce6cc47206b0..5bc6c7dbbd60 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -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) {
>
>

[PATCH 2/4] mmc: core: allow non-blocking form of mmc_claim_host

[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 <neil@brown.name>
---
 drivers/mmc/core/core.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 23f10f72e5f3..541c8903dc6b 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -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) {
 		host->claimed = 1;
 		host->claimer = current;
 		host->claim_cnt += 1;
+		stop = 0;
 	} else
 		wake_up(&host->wq);
 	spin_unlock_irqrestore(&host->lock, flags);

Re: [PATCH 2/4] mmc: core: allow non-blocking form of mmc_claim_host

[Ulf Hansson]

On 24 February 2015 at 03:42, NeilBrown <neilb@suse.de> wrote:
> 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 <neil@brown.name>
> ---
>  drivers/mmc/core/core.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 23f10f72e5f3..541c8903dc6b 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -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?

>                 host->claimed = 1;
>                 host->claimer = current;
>                 host->claim_cnt += 1;
> +               stop = 0;
>         } else
>                 wake_up(&host->wq);
>         spin_unlock_irqrestore(&host->lock, flags);
>
>

Kind regards
Uffe

Re: [PATCH 2/4] mmc: core: allow non-blocking form of mmc_claim_host

[Ulf Hansson]

On 23 March 2015 at 10:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 24 February 2015 at 03:42, NeilBrown <neilb@suse.de> wrote:
>> 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 <neil@brown.name>
>> ---
>>  drivers/mmc/core/core.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 23f10f72e5f3..541c8903dc6b 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -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?

Ah, that won't work. Since we may be spinning to claim the host and
the abort value may change dynamically during this time.

Another option would be to re-invent mmc_try_claim_host(). Which we
removed in commit:
b83e867026ca (mmc: core: remove dead function mmc_try_claim_host)

Kind regards
Uffe

[PATCH 4/4] mmc: omap_hsmmc: switch to 1-bit before stopping clocks.

[NeilBrown]

Make use of the new mmc_sdio_want_no_clocks() call to avoid stopping
clocks while SD Card interrupts are enabled and we aren't in
1-bit mode.

Also stop clocks immediately in omap_hsmmc_disable_fclk() if
1-bit mode has been entered for this purpose.

With this, I can use my libertas wifi with a 4-bit bus, with
interrupts and runtime power-management enabled, and get around
14Mb/sec throughput (which is the best I've seen).

Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/mmc/host/omap_hsmmc.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index f84cfb01716d..14fce3b92633 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1791,9 +1791,12 @@ static int omap_hsmmc_disable_fclk(struct mmc_host *mmc)
 {
 	struct omap_hsmmc_host *host = mmc_priv(mmc);
 
-	pm_runtime_mark_last_busy(host->dev);
-	pm_runtime_put_autosuspend(host->dev);
-
+	if (atomic_read(&mmc->sdio_narrowed) == 2)
+		pm_runtime_put_sync(host->dev);
+	else {
+		pm_runtime_mark_last_busy(host->dev);
+		pm_runtime_put_autosuspend(host->dev);
+	}
 	return 0;
 }
 
@@ -2311,6 +2314,10 @@ static int omap_hsmmc_runtime_suspend(struct device *dev)
 	spin_lock_irqsave(&host->irq_lock, flags);
 	if ((host->mmc->caps & MMC_CAP_SDIO_IRQ) &&
 	    (host->flags & HSMMC_SDIO_IRQ_ENABLED)) {
+		if (mmc_sdio_want_no_clocks(host->mmc) == 0) {
+			ret = -EBUSY;
+			goto abort;
+		}
 		/* disable sdio irq handling to prevent race */
 		OMAP_HSMMC_WRITE(host->base, ISE, 0);
 		OMAP_HSMMC_WRITE(host->base, IE, 0);