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
