diff mbox

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

Message ID 20150224024223.22719.91536.stgit@notabene.brown (mailing list archive)
State New, archived
Headers show

Commit Message

NeilBrown Feb. 24, 2015, 2:42 a.m. UTC
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(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ulf Hansson March 23, 2015, 9:10 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
NeilBrown March 25, 2015, 9:49 p.m. UTC | #2
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
diff mbox

Patch

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 */