On 4 April 2012 17:03, Ulf Hansson <[email protected]> wrote:
> On 03/30/2012 12:02 PM, Girish K S 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<[email protected]>
>> Signed-off-by: Girish K S<[email protected]>
>> ---
>> drivers/mmc/core/core.c | 36 ++++++++++++++----------------------
>> drivers/mmc/core/core.h | 1 +
>> drivers/mmc/core/mmc.c | 20 +++++++++++++-------
>> include/linux/mmc/host.h | 1 +
>> 4 files changed, 29 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 14f262e..dc85ee1 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1096,12 +1096,12 @@ void mmc_set_driver_type(struct mmc_host *host,
>> unsigned int drv_type)
>> mmc_host_clk_release(host);
>> }
>>
>> -static void mmc_poweroff_notify(struct mmc_host *host)
>> +int mmc_poweroff_notify(struct mmc_host *host)
>
>
> A more generic comment; why is not this function implemented as a bus_ops
> function, similar how sleep for mmc is done? That would be more preferred I
> think.
>
>
>> {
>> struct mmc_card *card;
>> unsigned int timeout;
>> unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>> - int err = 0;
>> + int err = -EINVAL;
>>
>> card = host->card;
>> mmc_claim_host(host);
>> @@ -1136,6 +1136,7 @@ static void mmc_poweroff_notify(struct mmc_host
>> *host)
>> card->poweroff_notify_state = MMC_NO_POWER_NOTIFICATION;
>> }
>> mmc_release_host(host);
>> + return err;
>> }
>>
>> /*
>> @@ -1194,30 +1195,12 @@ static void mmc_power_up(struct mmc_host *host)
>>
>> void mmc_power_off(struct mmc_host *host)
>> {
>> - int err = 0;
>> mmc_host_clk_hold(host);
>>
>> host->ios.clock = 0;
>> host->ios.vdd = 0;
>>
>> /*
>> - * For eMMC 4.5 device send AWAKE command before
>> - * POWER_OFF_NOTIFY command, because in sleep state
>> - * eMMC 4.5 devices respond to only RESET and AWAKE cmd
>> - */
>> - if (host->card&& mmc_card_is_sleep(host->card)&&
>> - host->bus_ops->resume) {
>> - err = host->bus_ops->resume(host);
>> -
>> - if (!err)
>> - mmc_poweroff_notify(host);
>> - else
>> - pr_warning("%s: error %d during resume "
>> - "(continue with poweroff sequence)\n",
>> - mmc_hostname(host), err);
>> - }
>> -
>> - /*
>> * Reset ocr mask to be the highest possible voltage supported for
>> * this mmc host. This value will be used at next power up.
>> */
>> @@ -2076,6 +2059,7 @@ void mmc_stop_host(struct mmc_host *host)
>> host->pm_flags = 0;
>>
>> mmc_bus_get(host);
>> + mmc_poweroff_notify(host);
>
>
> We must not do mmc_poweroff_notify, without knowing we have a card attached
> through the bus_ops.
>
>> if (host->bus_ops&& !host->bus_dead) {
>>
>> /* Calling bus_ops->remove() with a claimed host can
>> deadlock */
>> if (host->bus_ops->remove)
>> @@ -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);
>
>
> Same comment as above.
>
> Moreover, mmc_power_save_host is not working for eMMC cards supporting the
> SLEEP command (violating the eMMC spec for how VCC is cut). So, I am not
> sure we should introduce the poweroff_notify for the mmc_power_save_host
> right now. Better to fix SLEEP first, if needed at all.
>
> Additionally, this function is only used for SDIO if runtime PM is enabled
> at the moment.
>
>
>> mmc_bus_put(host);
>>
>> mmc_power_off(host);
>> @@ -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;
>
>
> Similar comments as for mmc_power_save_host
>
>> mmc_power_up(host);
>> ret = host->bus_ops->power_restore(host);
>>
>> @@ -2157,6 +2142,9 @@ int mmc_card_awake(struct mmc_host *host)
>> if (host->bus_ops&& !host->bus_dead&& host->bus_ops->awake)
>>
>> err = host->bus_ops->awake(host);
>>
>> + if (!err)
>> + mmc_card_clr_sleep(host->card);
>> +
>
>
> This code must be handled from withing the bus_ops function instead, or you
> need to move it inside the "if" above, since the awake bus_ops only exist
> for (e)MMC. Moreover, you do not want to set the sleep state unless the
> sleep cmd really has been executed.
>
>> mmc_bus_put(host);
>>
>> return err;
>> @@ -2175,6 +2163,9 @@ int mmc_card_sleep(struct mmc_host *host)
>> if (host->bus_ops&& !host->bus_dead&& host->bus_ops->sleep)
>>
>> err = host->bus_ops->sleep(host);
>>
>> + if (!err)
>> + mmc_card_set_sleep(host->card);
>> +
>
>
> I think this code should be handled from withing the bus_ops function
> instead, or you need to move it inside the "if" above, since the awake
> bus_ops only exist for (e)MMC.
>
>
>> mmc_bus_put(host);
>>
>> return err;
>> @@ -2393,6 +2384,7 @@ int mmc_pm_notify(struct notifier_block
>> *notify_block,
>> host->bus_ops->remove(host);
>>
>> mmc_claim_host(host);
>> + mmc_poweroff_notify(host);
>
>
> This wont work if the card has been removed from the bus_ops->remove above.
>
>
>> mmc_detach_bus(host);
>> mmc_power_off(host);
>> mmc_release_host(host);
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> index 3bdafbc..0bdc0fd 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -45,6 +45,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int
>> signal_voltage,
>> void mmc_set_timing(struct mmc_host *host, unsigned int timing);
>> void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
>> void mmc_power_off(struct mmc_host *host);
>> +int mmc_poweroff_notify(struct mmc_host *host);
>>
>> static inline void mmc_delay(unsigned int ms)
>> {
>> 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) {
>
> 1. missing whitespace after caps2
> 2. Can't we have an mmc_card_can_poweroff_notify to check instead, which
> also considering this new cap together with the card features.
>
>> + err = mmc_poweroff_notify(host);
>> if (!err)
>> - mmc_card_set_sleep(host->card);
>> - } else if (!mmc_host_is_spi(host))
>> + goto out;
>
> Suggest to remove the goto and just have another if-else, as we already have
> for sleep.
>>
>> + }
>> +
>> + if (mmc_card_can_sleep(host))
>> + err = mmc_card_sleep(host);
>> + else if (!mmc_host_is_spi(host))
>> mmc_deselect_cards(host);
>> +
>> +out:
>> host->card->state&= ~(MMC_STATE_HIGHSPEED |
>> MMC_STATE_HIGHSPEED_200);
>>
>> mmc_release_host(host);
>>
>> @@ -1364,11 +1370,11 @@ static int mmc_resume(struct mmc_host *host)
>> BUG_ON(!host->card);
>>
>> 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);
>> +
>> mmc_release_host(host);
>>
>> return err;
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 33a4d08..3749a42 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -237,6 +237,7 @@ struct mmc_host {
>> #define MMC_CAP2_BROKEN_VOLTAGE (1<< 7) /* Use the broken
>> voltage */
>> #define MMC_CAP2_DETECT_ON_ERR (1<< 8) /* On I/O err check
>> card removal */
>> #define MMC_CAP2_HC_ERASE_SZ (1<< 9) /* High-capacity erase size
>> */
>> +#define MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND (1<< 10)
>>
>> mmc_pm_flag_t pm_caps; /* supported pm features */
>> unsigned int power_notify_type;
>
>
I will check it once i am back from business trip.
> Kind regards
> Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
