On 30 March 2012 21:19, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> 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?
>
The mmc_poweroff_notify with MMC_HOST_PW_NOTIFY_LONG will take longer
time to complete than MMC_HOST_PW_NOTIFY_SHORT. So, the idea is that
if we use mmc_poweroff_notify on suspend path, then we should use
MMC_HOST_PW_NOTIFY_SHORT and if we use mmc_poweroff_notify from the
shutdown path, then we should use MMC_HOST_PW_NOTIFY_LONG.
We will add this in the description.
>> 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...)
>
eMMC does not support SPI mode. So, the POWER OFF NOTIFY, which is an
eMMC feature, can not be used on SPI mode. The above code (which you
are referring) puts the eMMC to low power "standby" state with
mmc_deselect_cards if sleep is not allowed. This logic has not been
modified by the POWER OFF NOTIFY patch.
>> +
>> +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
