Re: MMC runtime PM patches break libertas probe

[Ohad Ben-Cohen <ohad@wizery.com>]

On Mon, Nov 1, 2010 at 10:27 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Sun, Oct 31, 2010 at 9:06 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> ...
>> we need to support boards with controllers/cards
>> which we can't power off in runtime.
>>
>> On such boards, the right thing to do would be to disable runtime PM altogether.
>
> The patch below (/attached) should trivially fix the problem - can you
> please check it out ?
>
> It's very simple: it just requires hosts to explicitly indicate they
> support runtime powering off/on their cards (by means of
> MMC_CAP_RUNTIME_PM).
>
> There is no way around this I'm afraid: it is legitimate for a
> board/host/card configuration not to support powering off the card
> after boot, and we must allow it.
>
> In addition having an explicit MMC_CAP_RUNTIME_PM will also allow us
> smoother transition to runtime PM behavior. Developers will have to
> explicitly turn it on, and will not be surprised if things won't
> immediately work.
>
> Please note that this cap is not interchangeable, and can't be replace
> with CONFIG_PM_RUNTIME.
>
> Having said that, I still think we need to understand why CMD3 timed
> out on the XO-1.5.

Just to update the list, the problem with the XO-1.5 was because the
sd8686 has an external reset gpio line which is currently being
manipulated manually by an out-of-tree kernel patch:

http://dev.laptop.org/git/olpc-2.6/commit/?h=olpc-2.6.35&id=e9bee721fb0cc303286d1fe5df4930ce79b0b1e0

... which makes me wonder whether we really want to take that
MMC_CAP_RUNTIME_PM road. I'm not sure anymore.

We need a demonstrated hardware limitation before we take that
approach (or a setup which worked using vanilla kernels and now
doesn't), because frankly this MMC_CAP_RUNTIME_PM approach is
cumbersome and involving. I would not like to introduce it just to fix
out-of-tree software issues, and it seems that at least the XO-1.5
case can be cleanly solved by software (e.g. by letting the regulator
handle that sd8686 quirk).

I'm looping in Arnd, who reported similar problems with b43-sdio on
AP4EVB (arm) with tmio_mmc.

Arnd,

We're trying to exactly understand the reasons behind the reported
SDIO runtime pm failures (we had two, yours, and OLPC). So far it
seems that the OLPC had a software issue, and I'm wondering about
yours.

Can you please supply additional information about your board ? where
does your wifi card gets its power from ? is there an external gpio
involved ? Were you able to work with vanilla kernels (prior to
2.6.37) or do you carry some out-of-tree patches ?

Thanks,
Ohad.

>
> From 6b5691bdd8184cc0876d209c69d38844ea10f678 Mon Sep 17 00:00:00 2001
> From: Ohad Ben-Cohen <ohad@wizery.com>
> Date: Mon, 1 Nov 2010 09:41:44 +0200
> Subject: [PATCH] mmc: add MMC_CAP_RUNTIME_PM
>
> Some board/card/host configurations are not capable of powering off/on
> on the card during runtime.
>
> To support such configurations, and to allow smoother transition to
> runtime PM behavior, MMC_CAP_RUNTIME_PM is added, so hosts need to
> explicitly indicate that it's OK to power off their cards after boot.
>
> This will prevent sdio_bus_probe() from failing to power on the card
> when the driver is loaded on such setups.
>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
>  drivers/mmc/core/sdio.c     |   37 +++++++++++++++++++++++--------------
>  drivers/mmc/core/sdio_bus.c |   33 ++++++++++++++++++++++-----------
>  include/linux/mmc/host.h    |    1 +
>  3 files changed, 46 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 6a9ad40..373d56d 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -547,9 +547,11 @@ static void mmc_sdio_detect(struct mmc_host *host)
>        BUG_ON(!host->card);
>
>        /* Make sure card is powered before detecting it */
> -       err = pm_runtime_get_sync(&host->card->dev);
> -       if (err < 0)
> -               goto out;
> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
> +               err = pm_runtime_get_sync(&host->card->dev);
> +               if (err < 0)
> +                       goto out;
> +       }
>
>        mmc_claim_host(host);
>
> @@ -570,7 +572,8 @@ out:
>        }
>
>        /* Tell PM core that we're done */
> -       pm_runtime_put(&host->card->dev);
> +       if (host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_put(&host->card->dev);
>  }
>
>  /*
> @@ -720,16 +723,21 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>        card = host->card;
>
>        /*
> -        * Let runtime PM core know our card is active
> +        * Enable runtime PM only if supported by host+card+board
>         */
> -       err = pm_runtime_set_active(&card->dev);
> -       if (err)
> -               goto remove;
> +       if (host->caps & MMC_CAP_RUNTIME_PM) {
> +               /*
> +                * Let runtime PM core know our card is active
> +                */
> +               err = pm_runtime_set_active(&card->dev);
> +               if (err)
> +                       goto remove;
>
> -       /*
> -        * Enable runtime PM for this card
> -        */
> -       pm_runtime_enable(&card->dev);
> +               /*
> +                * Enable runtime PM for this card
> +                */
> +               pm_runtime_enable(&card->dev);
> +       }
>
>        /*
>         * The number of functions on the card is encoded inside
> @@ -747,9 +755,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>                        goto remove;
>
>                /*
> -                * Enable Runtime PM for this func
> +                * Enable Runtime PM for this func (if supported)
>                 */
> -               pm_runtime_enable(&card->sdio_func[i]->dev);
> +               if (host->caps & MMC_CAP_RUNTIME_PM)
> +                       pm_runtime_enable(&card->sdio_func[i]->dev);
>        }
>
>        mmc_release_host(host);
> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> index 2716c7a..f3ce21b 100644
> --- a/drivers/mmc/core/sdio_bus.c
> +++ b/drivers/mmc/core/sdio_bus.c
> @@ -17,6 +17,7 @@
>  #include <linux/pm_runtime.h>
>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/host.h>
>  #include <linux/mmc/sdio_func.h>
>
>  #include "sdio_cis.h"
> @@ -132,9 +133,11 @@ static int sdio_bus_probe(struct device *dev)
>         * it should call pm_runtime_put_noidle() in its probe routine and
>         * pm_runtime_get_noresume() in its remove routine.
>         */
> -       ret = pm_runtime_get_sync(dev);
> -       if (ret < 0)
> -               goto out;
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
> +               ret = pm_runtime_get_sync(dev);
> +               if (ret < 0)
> +                       goto out;
> +       }
>
>        /* Set the default block size so the driver is sure it's something
>         * sensible. */
> @@ -151,7 +154,8 @@ static int sdio_bus_probe(struct device *dev)
>        return 0;
>
>  disable_runtimepm:
> -       pm_runtime_put_noidle(dev);
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_put_noidle(dev);
>  out:
>        return ret;
>  }
> @@ -160,12 +164,14 @@ static int sdio_bus_remove(struct device *dev)
>  {
>        struct sdio_driver *drv = to_sdio_driver(dev->driver);
>        struct sdio_func *func = dev_to_sdio_func(dev);
> -       int ret;
> +       int ret = 0;
>
>        /* Make sure card is powered before invoking ->remove() */
> -       ret = pm_runtime_get_sync(dev);
> -       if (ret < 0)
> -               goto out;
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM) {
> +               ret = pm_runtime_get_sync(dev);
> +               if (ret < 0)
> +                       goto out;
> +       }
>
>        drv->remove(func);
>
> @@ -178,10 +184,12 @@ static int sdio_bus_remove(struct device *dev)
>        }
>
>        /* First, undo the increment made directly above */
> -       pm_runtime_put_noidle(dev);
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_put_noidle(dev);
>
>        /* Then undo the runtime PM settings in sdio_bus_probe() */
> -       pm_runtime_put_noidle(dev);
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_put_noidle(dev);
>
>  out:
>        return ret;
> @@ -191,6 +199,8 @@ out:
>
>  static int sdio_bus_pm_prepare(struct device *dev)
>  {
> +       struct sdio_func *func = dev_to_sdio_func(dev);
> +
>        /*
>         * Resume an SDIO device which was suspended at run time at this
>         * point, in order to allow standard SDIO suspend/resume paths
> @@ -212,7 +222,8 @@ static int sdio_bus_pm_prepare(struct device *dev)
>         * since there is little point in failing system suspend if a
>         * device can't be resumed.
>         */
> -       pm_runtime_resume(dev);
> +       if (func->card->host->caps & MMC_CAP_RUNTIME_PM)
> +               pm_runtime_resume(dev);
>
>        return 0;
>  }
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index c3ffad8..e5eee0e 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -167,6 +167,7 @@ struct mmc_host {
>                                                /* DDR mode at 1.8V */
>  #define MMC_CAP_1_2V_DDR       (1 << 12)       /* can support */
>                                                /* DDR mode at 1.2V */
> +#define MMC_CAP_RUNTIME_PM     (1 << 13)       /* Can power off/on in runtime */
>
>        mmc_pm_flag_t           pm_caps;        /* supported pm features */
>
> --
> 1.7.0.4
>