Re: [RFC] MMC-4.5 Power OFF Notify rework

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Fri, Mar 30, 2012 at 12:02 PM, Girish K S
<girish.shivananjappa@xxxxxxxxxx> wrote:

> This is a rework of the existing POWER OFF NOTIFY patch. The current problem
> with the patch comes from the ambiguity on the usage of POWER OFF NOTIFY
> together with SLEEP and misunderstanding on the usage of MMC_POWER_OFF
> power_mode from mmc_set_ios in different host controller drivers.
>
> This new patch works around this problem by adding a new host CAP,
> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND, which when set sends a
> POWER OFF NOTIFY from mmc_suspend instead of SLEEP. It is expected that host
> controller drivers will set this CAP, if they switch off both Vcc and Vccq
> from MMC_POWER_OFF condition within mmc_set_ios. However, note that there
> is no harm in sending MMC_POWER_NOTIFY even if Vccq is not switched off.
>
> This patch also sends POWER OFF NOTIFY from power management routines (e.g.
> mmc_power_save_host, mmc_pm_notify/PM_SUSPEND_PREPARE, mmc_stop_host), which
> does reinitialization of the eMMC on the return path of the power management
> routines (e.g. mmc_power_restore_host, mmc_pm_notify/PM_POST_RESTORE,
> mmc_start_host).
>
> Signed-off-by: Saugata Das <saugata.das@xxxxxxxxxx>
> Signed-off-by: Girish K S <girish.shivananjappa@xxxxxxxxxx>

Overall this looks good!

I think it may be possible to split the patch.
First a patch that moves mmc_card_set_sleep() and mmc_card_clr_sleep()
into mmc_card_sleep() and mmc_card_awake(), which is a good
refactoring in its own right. Then a patch doing the rest of the changes.

(But that's no big deal to me if Chris is OK with this.)

> -static void mmc_poweroff_notify(struct mmc_host *host)
> +int mmc_poweroff_notify(struct mmc_host *host)

So now this function will return an errorcode if notification fails,
maybe add some kerneldoc...

> @@ -2112,7 +2096,8 @@ int mmc_power_save_host(struct mmc_host *host)
>
>        if (host->bus_ops->power_save)
>                ret = host->bus_ops->power_save(host);
> -
> +       host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> +       mmc_poweroff_notify(host);

So no risk in ignoring poweroff notification failure here I guess..
(Just thinking aloud.)

> @@ -2135,7 +2120,7 @@ int mmc_power_restore_host(struct mmc_host *host)
>                mmc_bus_put(host);
>                return -EINVAL;
>        }
> -
> +       host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;

This looks new, can you explain in the code as comments
or in the commit message when SHORT and LONG notifications are
used and why?

> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 02914d6..885ad61 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1338,12 +1338,18 @@ static int mmc_suspend(struct mmc_host *host)
>        BUG_ON(!host->card);
>
>        mmc_claim_host(host);
> -       if (mmc_card_can_sleep(host)) {
> -               err = mmc_card_sleep(host);
> +       if (host->caps2 & MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND) {
> +               err = mmc_poweroff_notify(host);
>                if (!err)
> -                       mmc_card_set_sleep(host->card);
> -       } else if (!mmc_host_is_spi(host))
> +                       goto out;
> +       }
> +
> +       if (mmc_card_can_sleep(host))
> +               err = mmc_card_sleep(host);
> +       else if (!mmc_host_is_spi(host))
>                mmc_deselect_cards(host);

Are you sure you should not deselect the card on an SPI host also if
you power off? (I'm just confused, better to ask...)

> +
> +out:

So if I understand correctly we power off if possible, else we
set the card to sleep. (Looks good.)

>        mmc_claim_host(host);
> -       if (mmc_card_is_sleep(host->card)) {
> +       if (mmc_card_is_sleep(host->card))
>                err = mmc_card_awake(host);
> -               mmc_card_clr_sleep(host->card);
> -       } else
> +       else
>                err = mmc_init_card(host, host->ocr, host->card);

So a powered-off card will be reinitialized here, nice!

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