diff mbox

[3/3] mmc: omap_hsmmc: switch to 1-bit before turning off clocks if interrupts expected.

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

Commit Message

NeilBrown Jan. 27, 2015, 11:35 p.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.

)

We cannot simply set 1-bit mode in omap_hsmmc_runtime_suspend
as that is called under a spinlock, and setting 1-bit mode requires
a sleeping call to the card.

So:
 - use a work_struct to schedule setting of 1-bit mode
 - intro a 'force_narrow' state flag which transitions:
     0 -> NARROW_PENDING -> NARROW_FORCED -> 0
 - have omap_hsmmc_runtime_suspend fail if interrupts are expected
   but bus is not in 1-bit mode.  When it fails it schedules
   the work to change the width. and sets NARROW_PENDING
 - when the host is claimed, if NARROW_FORCED is set, restore the
   4-bit bus
 - When the host is released (disable_fclk), if NARROW_FORCED,
   then suspend immediately, no autosuspend.  If NARROW_PENDING,
   clear that flag as the device has obviously just been used.

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

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 |   55 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 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 Jan. 28, 2015, 8:18 p.m. UTC | #1
On 28 January 2015 at 00:35, 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.
>
> )
>
> We cannot simply set 1-bit mode in omap_hsmmc_runtime_suspend
> as that is called under a spinlock, and setting 1-bit mode requires
> a sleeping call to the card.
>
> So:
>  - use a work_struct to schedule setting of 1-bit mode
>  - intro a 'force_narrow' state flag which transitions:
>      0 -> NARROW_PENDING -> NARROW_FORCED -> 0
>  - have omap_hsmmc_runtime_suspend fail if interrupts are expected
>    but bus is not in 1-bit mode.  When it fails it schedules
>    the work to change the width. and sets NARROW_PENDING
>  - when the host is claimed, if NARROW_FORCED is set, restore the
>    4-bit bus
>  - When the host is released (disable_fclk), if NARROW_FORCED,
>    then suspend immediately, no autosuspend.  If NARROW_PENDING,
>    clear that flag as the device has obviously just been used.

I can't give you more detailed comment about this patch(set) yet. Need
to think a bit more first.

Anyway, I have one concern, see comment below.

>
> This all allows a graceful and race-free switch to 1-bit mode
> before switching off the clocks, if interrupts are enabled.
>
> 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 |   55 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index f84cfb01716d..91ddebbec8a3 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -214,6 +214,10 @@ struct omap_hsmmc_host {
>         int                     reqs_blocked;
>         int                     use_reg;
>         int                     req_in_progress;
> +       int                     force_narrow;
> +#define NARROW_PENDING 1
> +#define NARROW_FORCED  2
> +       struct work_struct      width_work;
>         unsigned long           clk_rate;
>         unsigned int            flags;
>  #define AUTO_CMD23             (1 << 0)        /* Auto CMD23 support */
> @@ -1778,12 +1782,43 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
>         set_sd_bus_power(host);
>  }
>
> +static void omap_hsmmc_width_work(struct work_struct *work)
> +{
> +       struct omap_hsmmc_host *host = container_of(work,
> +                                                   struct omap_hsmmc_host,
> +                                                   width_work);
> +       atomic_t noblock;
> +
> +       atomic_set(&noblock, 1);
> +       if (__mmc_claim_host(host->mmc, &noblock)) {
> +               /* Device active again */
> +               host->force_narrow = 0;
> +               return;
> +       }
> +       if (host->force_narrow != NARROW_PENDING) {
> +               /* Someone claimed and released before we got here */
> +               mmc_release_host(host->mmc);
> +               return;
> +       }
> +       if (sdio_disable_wide(host->mmc->card) == 0)
> +               host->force_narrow = NARROW_FORCED;
> +       else
> +               host->force_narrow = 0;
> +       mmc_release_host(host->mmc);
> +}
> +
>  static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
>  {
>         struct omap_hsmmc_host *host = mmc_priv(mmc);
>
>         pm_runtime_get_sync(host->dev);
>
> +       if (host->force_narrow == NARROW_FORCED) {
> +               if (sdio_enable_4bit_bus(mmc->card) > 0)
> +                       mmc_set_bus_width(mmc, MMC_BUS_WIDTH_4);
> +               host->force_narrow = 0;
> +       }
> +
>         return 0;
>  }
>
> @@ -1791,8 +1826,13 @@ 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 (host->force_narrow == NARROW_FORCED) {
> +               pm_runtime_put_sync(host->dev);
> +       } else {
> +               host->force_narrow = 0;
> +               pm_runtime_mark_last_busy(host->dev);
> +               pm_runtime_put_autosuspend(host->dev);
> +       }
>
>         return 0;
>  }
> @@ -2024,6 +2064,7 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
>         host->power_mode = MMC_POWER_OFF;
>         host->next_data.cookie = 1;
>         host->pbias_enabled = 0;
> +       INIT_WORK(&host->width_work, omap_hsmmc_width_work);
>
>         ret = omap_hsmmc_gpio_init(mmc, host, pdata);
>         if (ret)
> @@ -2311,6 +2352,16 @@ 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 (host->mmc->ios.bus_width != MMC_BUS_WIDTH_1) {
> +                       /* In 4-bit mode the card need the clock
> +                        * to deliver interrupts, so it isn't safe
> +                        * to turn it off.
> +                        */
> +                       host->force_narrow = NARROW_PENDING;
> +                       schedule_work(&host->width_work);
> +                       ret = -EBUSY;
> +                       goto abort;

From a host driver perspective, don't you think this a bit too much to
implement to cover this case!?

I would rather see the host driver to invoke _one_ API provided by the
mmc core, which takes care of the needed things and also tells the
host driver whether it's safe to enter runtime PM suspend state or
not. Could that work?

> +               }
>                 /* disable sdio irq handling to prevent race */
>                 OMAP_HSMMC_WRITE(host->base, ISE, 0);
>                 OMAP_HSMMC_WRITE(host->base, IE, 0);
>
>

Kind regards
Uffe
--
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 Jan. 28, 2015, 9:08 p.m. UTC | #2
On Wed, 28 Jan 2015 21:18:40 +0100 Ulf Hansson <ulf.hansson@linaro.org> wrote:

> On 28 January 2015 at 00:35, 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.
> >
> > )
> >
> > We cannot simply set 1-bit mode in omap_hsmmc_runtime_suspend
> > as that is called under a spinlock, and setting 1-bit mode requires
> > a sleeping call to the card.
> >
> > So:
> >  - use a work_struct to schedule setting of 1-bit mode
> >  - intro a 'force_narrow' state flag which transitions:
> >      0 -> NARROW_PENDING -> NARROW_FORCED -> 0
> >  - have omap_hsmmc_runtime_suspend fail if interrupts are expected
> >    but bus is not in 1-bit mode.  When it fails it schedules
> >    the work to change the width. and sets NARROW_PENDING
> >  - when the host is claimed, if NARROW_FORCED is set, restore the
> >    4-bit bus
> >  - When the host is released (disable_fclk), if NARROW_FORCED,
> >    then suspend immediately, no autosuspend.  If NARROW_PENDING,
> >    clear that flag as the device has obviously just been used.
> 
> I can't give you more detailed comment about this patch(set) yet. Need
> to think a bit more first.
> 
> Anyway, I have one concern, see comment below.
> 
> >
> > This all allows a graceful and race-free switch to 1-bit mode
> > before switching off the clocks, if interrupts are enabled.
> >
> > 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 |   55 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> > index f84cfb01716d..91ddebbec8a3 100644
> > --- a/drivers/mmc/host/omap_hsmmc.c
> > +++ b/drivers/mmc/host/omap_hsmmc.c
> > @@ -214,6 +214,10 @@ struct omap_hsmmc_host {
> >         int                     reqs_blocked;
> >         int                     use_reg;
> >         int                     req_in_progress;
> > +       int                     force_narrow;
> > +#define NARROW_PENDING 1
> > +#define NARROW_FORCED  2
> > +       struct work_struct      width_work;
> >         unsigned long           clk_rate;
> >         unsigned int            flags;
> >  #define AUTO_CMD23             (1 << 0)        /* Auto CMD23 support */
> > @@ -1778,12 +1782,43 @@ static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
> >         set_sd_bus_power(host);
> >  }
> >
> > +static void omap_hsmmc_width_work(struct work_struct *work)
> > +{
> > +       struct omap_hsmmc_host *host = container_of(work,
> > +                                                   struct omap_hsmmc_host,
> > +                                                   width_work);
> > +       atomic_t noblock;
> > +
> > +       atomic_set(&noblock, 1);
> > +       if (__mmc_claim_host(host->mmc, &noblock)) {
> > +               /* Device active again */
> > +               host->force_narrow = 0;
> > +               return;
> > +       }
> > +       if (host->force_narrow != NARROW_PENDING) {
> > +               /* Someone claimed and released before we got here */
> > +               mmc_release_host(host->mmc);
> > +               return;
> > +       }
> > +       if (sdio_disable_wide(host->mmc->card) == 0)
> > +               host->force_narrow = NARROW_FORCED;
> > +       else
> > +               host->force_narrow = 0;
> > +       mmc_release_host(host->mmc);
> > +}
> > +
> >  static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
> >  {
> >         struct omap_hsmmc_host *host = mmc_priv(mmc);
> >
> >         pm_runtime_get_sync(host->dev);
> >
> > +       if (host->force_narrow == NARROW_FORCED) {
> > +               if (sdio_enable_4bit_bus(mmc->card) > 0)
> > +                       mmc_set_bus_width(mmc, MMC_BUS_WIDTH_4);
> > +               host->force_narrow = 0;
> > +       }
> > +
> >         return 0;
> >  }
> >
> > @@ -1791,8 +1826,13 @@ 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 (host->force_narrow == NARROW_FORCED) {
> > +               pm_runtime_put_sync(host->dev);
> > +       } else {
> > +               host->force_narrow = 0;
> > +               pm_runtime_mark_last_busy(host->dev);
> > +               pm_runtime_put_autosuspend(host->dev);
> > +       }
> >
> >         return 0;
> >  }
> > @@ -2024,6 +2064,7 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
> >         host->power_mode = MMC_POWER_OFF;
> >         host->next_data.cookie = 1;
> >         host->pbias_enabled = 0;
> > +       INIT_WORK(&host->width_work, omap_hsmmc_width_work);
> >
> >         ret = omap_hsmmc_gpio_init(mmc, host, pdata);
> >         if (ret)
> > @@ -2311,6 +2352,16 @@ 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 (host->mmc->ios.bus_width != MMC_BUS_WIDTH_1) {
> > +                       /* In 4-bit mode the card need the clock
> > +                        * to deliver interrupts, so it isn't safe
> > +                        * to turn it off.
> > +                        */
> > +                       host->force_narrow = NARROW_PENDING;
> > +                       schedule_work(&host->width_work);
> > +                       ret = -EBUSY;
> > +                       goto abort;
> 
> >From a host driver perspective, don't you think this a bit too much to
> implement to cover this case!?
> 
> I would rather see the host driver to invoke _one_ API provided by the
> mmc core, which takes care of the needed things and also tells the
> host driver whether it's safe to enter runtime PM suspend state or
> not. Could that work?

Since posting the patch I've been thinking along those lines too.  I hadn't
imagined making just a single call from the host driver, but now that I think
about it I don't see why not.  It should definitely work.

I'll re-spin them in the next day or two.

Thanks,
NeilBrown

> 
> > +               }
> >                 /* disable sdio irq handling to prevent race */
> >                 OMAP_HSMMC_WRITE(host->base, ISE, 0);
> >                 OMAP_HSMMC_WRITE(host->base, IE, 0);
> >
> >
> 
> Kind regards
> Uffe
diff mbox

Patch

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index f84cfb01716d..91ddebbec8a3 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -214,6 +214,10 @@  struct omap_hsmmc_host {
 	int			reqs_blocked;
 	int			use_reg;
 	int			req_in_progress;
+	int			force_narrow;
+#define NARROW_PENDING	1
+#define NARROW_FORCED	2
+	struct work_struct	width_work;
 	unsigned long		clk_rate;
 	unsigned int		flags;
 #define AUTO_CMD23		(1 << 0)        /* Auto CMD23 support */
@@ -1778,12 +1782,43 @@  static void omap_hsmmc_conf_bus_power(struct omap_hsmmc_host *host)
 	set_sd_bus_power(host);
 }
 
+static void omap_hsmmc_width_work(struct work_struct *work)
+{
+	struct omap_hsmmc_host *host = container_of(work,
+						    struct omap_hsmmc_host,
+						    width_work);
+	atomic_t noblock;
+
+	atomic_set(&noblock, 1);
+	if (__mmc_claim_host(host->mmc, &noblock)) {
+		/* Device active again */
+		host->force_narrow = 0;
+		return;
+	}
+	if (host->force_narrow != NARROW_PENDING) {
+		/* Someone claimed and released before we got here */
+		mmc_release_host(host->mmc);
+		return;
+	}
+	if (sdio_disable_wide(host->mmc->card) == 0)
+		host->force_narrow = NARROW_FORCED;
+	else
+		host->force_narrow = 0;
+	mmc_release_host(host->mmc);
+}
+
 static int omap_hsmmc_enable_fclk(struct mmc_host *mmc)
 {
 	struct omap_hsmmc_host *host = mmc_priv(mmc);
 
 	pm_runtime_get_sync(host->dev);
 
+	if (host->force_narrow == NARROW_FORCED) {
+		if (sdio_enable_4bit_bus(mmc->card) > 0)
+			mmc_set_bus_width(mmc, MMC_BUS_WIDTH_4);
+		host->force_narrow = 0;
+	}
+
 	return 0;
 }
 
@@ -1791,8 +1826,13 @@  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 (host->force_narrow == NARROW_FORCED) {
+		pm_runtime_put_sync(host->dev);
+	} else {
+		host->force_narrow = 0;
+		pm_runtime_mark_last_busy(host->dev);
+		pm_runtime_put_autosuspend(host->dev);
+	}
 
 	return 0;
 }
@@ -2024,6 +2064,7 @@  static int omap_hsmmc_probe(struct platform_device *pdev)
 	host->power_mode = MMC_POWER_OFF;
 	host->next_data.cookie = 1;
 	host->pbias_enabled = 0;
+	INIT_WORK(&host->width_work, omap_hsmmc_width_work);
 
 	ret = omap_hsmmc_gpio_init(mmc, host, pdata);
 	if (ret)
@@ -2311,6 +2352,16 @@  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 (host->mmc->ios.bus_width != MMC_BUS_WIDTH_1) {
+			/* In 4-bit mode the card need the clock
+			 * to deliver interrupts, so it isn't safe
+			 * to turn it off.
+			 */
+			host->force_narrow = NARROW_PENDING;
+			schedule_work(&host->width_work);
+			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);