Re: [PATCH v1 1/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode

Re: [PATCH v1 1/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode

[Ohad Ben-Cohen]

Hello Wilson,

On Fri, Mar 25, 2011 at 5:58 AM, Wilson Loi <wlsloi@gmail.com> wrote:
> The current power_restore handler only support cut off mode.
> It is better to support keep power mode too.

This doesn't make much sense really;

mmc_power_save_host() and mmc_power_restore_host() are invoked by
runtime PM when it is time to power off/on the card.

If your driver doesn't want the power to go, it should just maintain a
positive runtime PM usage_count (e.g. in case it's an SDIO driver,
don't call pm_runtime_put{_sync} after being probed).

>  static const struct mmc_bus_ops mmc_sdio_ops = {
>      .remove = mmc_sdio_remove,
>      .detect = mmc_sdio_detect,
>      .suspend = mmc_sdio_suspend,
>      .resume = mmc_sdio_resume,
> -    .power_restore = mmc_sdio_power_restore,
> +    .power_save = mmc_sdio_suspend,
> +    .power_restore = mmc_sdio_resume,

This can break SDIO runtime PM.

While -ENOSYS is a valid return value for mmc_sdio_suspend (it tells
mmc_suspend_host to remove the entire MMC card on system suspend), it
will be treated as an error by runtime PM.

In addition, this can really yield unexpected results, since drivers
do not expect their suspend/resume handlers to be called during
runtime PM transitions.

Re: [PATCH v1 2/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode

[Ohad Ben-Cohen]

On Fri, Mar 25, 2011 at 6:00 AM, Wilson Loi <wlsloi@gmail.com> wrote:
> Another patch file
> ----------------------------
>
> diff -ruN a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c

Please follow Documentation/SubmittingPatches when submitting patches,
or at least just add -p to diff, otherwise it's a bit difficult to
follow. thanks.

>      mmc_bus_put(host);
>
> -    mmc_power_off(host);
> +    if (!ret && !(host->pm_flags & MMC_PM_KEEP_POWER))
> +        mmc_power_off(host);

...
>
> -    mmc_power_up(host);
> +    if (!(host->pm_flags & MMC_PM_KEEP_POWER))
> +        mmc_power_up(host);
>      ret = host->bus_ops->power_restore(host);


I have the same question here: if you driver doesn't want its card to
be powered down, why does it invoke (or let runtime PM invoke on its
behalf) mmc_power_save_host() in the first place ?

Re: [PATCH v1 2/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode

[Nicolas Pitre]

On Sat, 26 Mar 2011, Wilson Loi wrote:

> Sorry, this is the first time I sent a patch.
> 
> There are two case for the WLAN SDIO card to do power saving.
> 1. Totally cut off the power of WLAN network interface.
> 2. offload the network state to WLAN SDIO firmware.

Case #2 is already supported with current code.  Patches for the 
libertas driver are in the OLPC repository (no idea if they were 
submitted upstream yet).


Nicolas

Re: [PATCH v1 2/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode

[Chris Ball]

Hi,

On Fri, Mar 25 2011, Nicolas Pitre wrote:
> On Sat, 26 Mar 2011, Wilson Loi wrote:
>
>> Sorry, this is the first time I sent a patch.
>> 
>> There are two case for the WLAN SDIO card to do power saving.
>> 1. Totally cut off the power of WLAN network interface.
>> 2. offload the network state to WLAN SDIO firmware.
>
> Case #2 is already supported with current code.  Patches for the 
> libertas driver are in the OLPC repository (no idea if they were 
> submitted upstream yet).

Yes, they're upstream now.  libertas/if_sdio.c:if_sdio_suspend() sets
MMC_PM_KEEP_POWER if a wakeup source has been set with "ethtool wol".

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v1 2/2] sdio: add power_restore support with MMC_PM_KEEP_POWER mode

[Ohad Ben-Cohen]

On Fri, Mar 25, 2011 at 7:08 PM, Wilson Loi <wlsloi@gmail.com> wrote:
> Therefore, there is a case that we power save for the host CPU and let the
> firmware handle the wlan state.
> WLAN card will wake up the host by SDIO IRQ or GPIO if there is a new
> incoming packet later.

Then you're talking about keeping the card powered during system
suspend, which is already supported (as mentioned by Nicolas and
Chris), but your patches really changed SDIO's runtime PM path (which
is not needed since anyway the driver fully controls it).