[v5] MMC-4.5 Power OFF Notify Rework

Message ID	1337687104-19376-1-git-send-email-girish.shivananjappa@linaro.org
State	New
Headers	show Return-Path: <patch+caf_=linaro-patchwork=canonical.com@linaro.org> Received-SPF: neutral (google.com: 209.85.210.50 is neither permitted nor denied by best guess record for domain of girish.shivananjappa@linaro.org) client-ip=209.85.210.50; From: Girish K S <girish.shivananjappa@linaro.org> To: linux-mmc@vger.kernel.org Cc: cjb@laptop.org, patches@linaro.org, ulf.hansson@stericsson.com, saugata.das@linaro.org, subhashj@codeaurora.org, Girish K S <girish.shivananjappa@linaro.org> Subject: [PATCH v5] MMC-4.5 Power OFF Notify Rework Date: Tue, 22 May 2012 17:15:04 +0530 Message-Id: <1337687104-19376-1-git-send-email-girish.shivananjappa@linaro.org>

Girish K S May 22, 2012, 11:45 a.m. UTC

From: Saugata Das <saugata.das@linaro.org>

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).

This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent from
the suspend sequence. If it is sent from shutdown sequence then it is set to
POWER_OFF_LONG.

Earlier implementation of PowerOff Notify as a core function is replaced as
a device's bus operation.

Signed-off-by: Saugata Das <saugata.das@linaro.org>
Signed-off-by: Girish K S <girish.shivananjappa@linaro.org>

changes in v5:
	modified the the handling of return value in mmc_poweroff_notify.
changes in v4:
	As suggested in review,
	- Moved mmc_can_poweroff_notify to core.c
	- Moved mmc_claim_host, mmc_release_host outside mmc_poweroff_notify
	- Added check for wrong initialization for poweroff_notify_type
	- mmc_poweroff_notify is modified to take as 2nd parameter
changes in v3:
	This version addresses the review comments given by Subhash and Ulf
changes in v2:
	This version addresses the changes suggested by Ulf
---
 drivers/mmc/core/core.c   |  108 ++++++++++++++++++--------------------------
 drivers/mmc/core/core.h   |    1 +
 drivers/mmc/core/mmc.c    |   56 ++++++++++++++++++++---
 drivers/mmc/host/dw_mmc.c |    5 --
 drivers/mmc/host/sdhci.c  |    9 ----
 include/linux/mmc/card.h  |    5 +-
 include/linux/mmc/core.h  |    1 +
 include/linux/mmc/host.h  |    5 +--
 include/linux/mmc/mmc.h   |    7 +++
 9 files changed, 104 insertions(+), 93 deletions(-)

Subhash Jadavani May 23, 2012, 8 a.m. UTC | #1

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

On 5/22/2012 5:15 PM, Girish K S wrote:
> From: Saugata Das<saugata.das@linaro.org>
>
> 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).
>
> This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent from
> the suspend sequence. If it is sent from shutdown sequence then it is set to
> POWER_OFF_LONG.
>
> Earlier implementation of PowerOff Notify as a core function is replaced as
> a device's bus operation.
>
> Signed-off-by: Saugata Das<saugata.das@linaro.org>
> Signed-off-by: Girish K S<girish.shivananjappa@linaro.org>
>
> changes in v5:
> 	modified the the handling of return value in mmc_poweroff_notify.
> changes in v4:
> 	As suggested in review,
> 	- Moved mmc_can_poweroff_notify to core.c
> 	- Moved mmc_claim_host, mmc_release_host outside mmc_poweroff_notify
> 	- Added check for wrong initialization for poweroff_notify_type
> 	- mmc_poweroff_notify is modified to take as 2nd parameter
> changes in v3:
> 	This version addresses the review comments given by Subhash and Ulf
> changes in v2:
> 	This version addresses the changes suggested by Ulf
> ---
>   drivers/mmc/core/core.c   |  108 ++++++++++++++++++--------------------------
>   drivers/mmc/core/core.h   |    1 +
>   drivers/mmc/core/mmc.c    |   56 ++++++++++++++++++++---
>   drivers/mmc/host/dw_mmc.c |    5 --
>   drivers/mmc/host/sdhci.c  |    9 ----
>   include/linux/mmc/card.h  |    5 +-
>   include/linux/mmc/core.h  |    1 +
>   include/linux/mmc/host.h  |    5 +--
>   include/linux/mmc/mmc.h   |    7 +++
>   9 files changed, 104 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 0b6141d..fe616b9 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1101,48 +1101,6 @@ 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)
> -{
> -	struct mmc_card *card;
> -	unsigned int timeout;
> -	unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
> -	int err = 0;
> -
> -	card = host->card;
> -	mmc_claim_host(host);
> -
> -	/*
> -	 * Send power notify command only if card
> -	 * is mmc and notify state is powered ON
> -	 */
> -	if (card&&  mmc_card_mmc(card)&&
> -	    (card->poweroff_notify_state == MMC_POWERED_ON)) {
> -
> -		if (host->power_notify_type == MMC_HOST_PW_NOTIFY_SHORT) {
> -			notify_type = EXT_CSD_POWER_OFF_SHORT;
> -			timeout = card->ext_csd.generic_cmd6_time;
> -			card->poweroff_notify_state = MMC_POWEROFF_SHORT;
> -		} else {
> -			notify_type = EXT_CSD_POWER_OFF_LONG;
> -			timeout = card->ext_csd.power_off_longtime;
> -			card->poweroff_notify_state = MMC_POWEROFF_LONG;
> -		}
> -
> -		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> -				 EXT_CSD_POWER_OFF_NOTIFICATION,
> -				 notify_type, timeout);
> -
> -		if (err&&  err != -EBADMSG)
> -			pr_err("Device failed to respond within %d poweroff "
> -			       "time. Forcefully powering down the device\n",
> -			       timeout);
> -
> -		/* Set the card state to no notification after the poweroff */
> -		card->poweroff_notify_state = MMC_NO_POWER_NOTIFICATION;
> -	}
> -	mmc_release_host(host);
> -}
> -
>   /*
>    * Apply power to the MMC stack.  This is a two-stage process.
>    * First, we enable power to the card without the clock running.
> @@ -1202,8 +1160,6 @@ static void mmc_power_up(struct mmc_host *host)
>
>   void mmc_power_off(struct mmc_host *host)
>   {
> -	int err = 0;
> -
>   	if (host->ios.power_mode == MMC_POWER_OFF)
>   		return;
>
> @@ -1212,22 +1168,6 @@ void mmc_power_off(struct mmc_host *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
> @@ -1726,6 +1666,15 @@ int mmc_can_secure_erase_trim(struct mmc_card *card)
>   }
>   EXPORT_SYMBOL(mmc_can_secure_erase_trim);
>
> +int mmc_can_poweroff_notify(const struct mmc_card *card)
> +{
> +	return card&&
> +		mmc_card_mmc(card)&&
> +		card->host->bus_ops->poweroff_notify&&
> +		(card->poweroff_notify_state == MMC_POWERED_ON);
> +}
> +EXPORT_SYMBOL(mmc_can_poweroff_notify);
> +
>   int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>   			    unsigned int nr)
>   {
> @@ -2096,6 +2045,15 @@ void mmc_stop_host(struct mmc_host *host)
>
>   	mmc_bus_get(host);
>   	if (host->bus_ops&&  !host->bus_dead) {
> +		mmc_claim_host(host);
> +		if (mmc_can_poweroff_notify(host->card)) {
> +			int err = host->bus_ops->poweroff_notify(host,
> +						MMC_PW_OFF_NOTIFY_LONG);
> +			if (err)
> +				pr_info("%s: error [%d] in poweroff notify\n",
> +					mmc_hostname(host), err);
> +		}
> +		mmc_release_host(host);
>   		/* Calling bus_ops->remove() with a claimed host can deadlock */
>   		if (host->bus_ops->remove)
>   			host->bus_ops->remove(host);
> @@ -2131,6 +2089,15 @@ int mmc_power_save_host(struct mmc_host *host)
>
>   	if (host->bus_ops->power_save)
>   		ret = host->bus_ops->power_save(host);
> +	mmc_claim_host(host);
> +	if (mmc_can_poweroff_notify(host->card)) {
> +		int err = host->bus_ops->poweroff_notify(host,
> +					MMC_PW_OFF_NOTIFY_SHORT);
> +		if (err)
> +			pr_info("%s: error [%d] in poweroff notify\n",
> +				mmc_hostname(host), err);
> +	}
> +	mmc_release_host(host);
>
>   	mmc_bus_put(host);
>
> @@ -2173,8 +2140,11 @@ int mmc_card_awake(struct mmc_host *host)
>
>   	mmc_bus_get(host);
>
> -	if (host->bus_ops&&  !host->bus_dead&&  host->bus_ops->awake)
> +	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);
> +	}
>
>   	mmc_bus_put(host);
>
> @@ -2191,8 +2161,11 @@ int mmc_card_sleep(struct mmc_host *host)
>
>   	mmc_bus_get(host);
>
> -	if (host->bus_ops&&  !host->bus_dead&&  host->bus_ops->sleep)
> +	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);
> +	}
>
>   	mmc_bus_put(host);
>
> @@ -2385,12 +2358,20 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>
>   		spin_lock_irqsave(&host->lock, flags);
>   		host->rescan_disable = 1;
> -		host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>   		spin_unlock_irqrestore(&host->lock, flags);
>   		cancel_delayed_work_sync(&host->detect);
>
>   		if (!host->bus_ops || host->bus_ops->suspend)
>   			break;
> +		mmc_claim_host(host);
> +		if (mmc_can_poweroff_notify(host->card)) {
> +			int err = host->bus_ops->poweroff_notify(host,
> +						MMC_PW_OFF_NOTIFY_SHORT);
> +			if (err)
> +				pr_info("%s: error [%d] in poweroff notify\n",
> +					mmc_hostname(host), err);
> +		}
> +		mmc_release_host(host);
>
>   		/* Calling bus_ops->remove() with a claimed host can deadlock */
>   		if (host->bus_ops->remove)
> @@ -2409,7 +2390,6 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>
>   		spin_lock_irqsave(&host->lock, flags);
>   		host->rescan_disable = 0;
> -		host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;
>   		spin_unlock_irqrestore(&host->lock, flags);
>   		mmc_detect_change(host, 0);
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 3bdafbc..15b918d 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -25,6 +25,7 @@ struct mmc_bus_ops {
>   	int (*power_save)(struct mmc_host *);
>   	int (*power_restore)(struct mmc_host *);
>   	int (*alive)(struct mmc_host *);
> +	int (*poweroff_notify)(struct mmc_host *, int notify);
>   };
>
>   void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 2f0e11c..042dbdc 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1259,6 +1259,40 @@ err:
>   	return err;
>   }
>
> +static int mmc_poweroff_notify(struct mmc_host *host, int notify)
> +{
> +	struct mmc_card *card;
> +	unsigned int timeout;
> +	unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
> +	int err;
> +
> +	card = host->card;
> +
> +	if (notify == MMC_PW_OFF_NOTIFY_SHORT) {
> +		notify_type = EXT_CSD_POWER_OFF_SHORT;
> +		timeout = card->ext_csd.generic_cmd6_time;
> +	} else if (notify == MMC_PW_OFF_NOTIFY_LONG) {
> +		notify_type = EXT_CSD_POWER_OFF_LONG;
> +		timeout = card->ext_csd.power_off_longtime;
> +	} else {
> +		pr_info("%s: mmc_poweroff_notify called "
> +		        "with notify type %d\n", mmc_hostname(host), notify);
> +		return -EINVAL;
> +	}
> +
> +	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +			 EXT_CSD_POWER_OFF_NOTIFICATION,
> +			 notify_type, timeout);
> +
> +	if (err)
> +		pr_err("%s: Device failed to respond within %d "
> +		       "poweroff timeout.\n", mmc_hostname(host), timeout);
> +	else
> +		card->poweroff_notify_state =
> +					MMC_NO_POWER_NOTIFICATION;
> +
> +	return err;
> +}
>   /*
>    * Host is being removed. Free up the current card.
>    */
> @@ -1319,13 +1353,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 (!err)
> -			mmc_card_set_sleep(host->card);
> -	} else if (!mmc_host_is_spi(host))
> -		mmc_deselect_cards(host);
> -	host->card->state&= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
> +	if (mmc_can_poweroff_notify(host->card)&&
> +		(host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
> +		err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
> +	} else {
> +		if (mmc_card_can_sleep(host))
> +			err = mmc_card_sleep(host);
> +		else if (!mmc_host_is_spi(host))
> +			mmc_deselect_cards(host);
> +	}
> +	if (!err)
> +		host->card->state&=
> +			~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
>   	mmc_release_host(host);
>
>   	return err;
> @@ -1347,7 +1386,6 @@ static int mmc_resume(struct mmc_host *host)
>   	mmc_claim_host(host);
>   	if (mmc_card_is_sleep(host->card)) {
>   		err = mmc_card_awake(host);
> -		mmc_card_clr_sleep(host->card);
>   	} else
>   		err = mmc_init_card(host, host->ocr, host->card);
>   	mmc_release_host(host);
> @@ -1407,6 +1445,7 @@ static const struct mmc_bus_ops mmc_ops = {
>   	.resume = NULL,
>   	.power_restore = mmc_power_restore,
>   	.alive = mmc_alive,
> +	.poweroff_notify = mmc_poweroff_notify,
>   };
>
>   static const struct mmc_bus_ops mmc_ops_unsafe = {
> @@ -1418,6 +1457,7 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>   	.resume = mmc_resume,
>   	.power_restore = mmc_power_restore,
>   	.alive = mmc_alive,
> +	.poweroff_notify = mmc_poweroff_notify,
>   };
>
>   static void mmc_attach_bus_ops(struct mmc_host *host)
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 1532357..463130f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1788,11 +1788,6 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>   	if (host->pdata->quirks&  DW_MCI_QUIRK_HIGHSPEED)
>   		mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>
> -	if (mmc->caps2&  MMC_CAP2_POWEROFF_NOTIFY)
> -		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> -	else
> -		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
> -
>   	if (host->pdata->blk_settings) {
>   		mmc->max_segs = host->pdata->blk_settings->max_segs;
>   		mmc->max_blk_size = host->pdata->blk_settings->max_blk_size;
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e626732..c0a5a91 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2812,15 +2812,6 @@ int sdhci_add_host(struct sdhci_host *host)
>   	if (caps[1]&  SDHCI_DRIVER_TYPE_D)
>   		mmc->caps |= MMC_CAP_DRIVER_TYPE_D;
>
> -	/*
> -	 * If Power Off Notify capability is enabled by the host,
> -	 * set notify to short power off notify timeout value.
> -	 */
> -	if (mmc->caps2&  MMC_CAP2_POWEROFF_NOTIFY)
> -		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> -	else
> -		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
> -
>   	/* Initial value for re-tuning timer count */
>   	host->tuning_count = (caps[1]&  SDHCI_RETUNING_TIMER_COUNT_MASK)>>
>   			SDHCI_RETUNING_TIMER_COUNT_SHIFT;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index d76513b..040eec4 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -239,11 +239,10 @@ struct mmc_card {
>   #define MMC_QUIRK_BROKEN_BYTE_MODE_512 (1<<8)	/* Avoid sending 512 bytes in */
>   #define MMC_QUIRK_LONG_READ_TIME (1<<9)		/* Data read time>  CSD says */
>   						/* byte mode */
> -	unsigned int    poweroff_notify_state;	/* eMMC4.5 notify feature */
> +	unsigned int		poweroff_notify_state; /* MMC-4.5 poweroff
> +							notify feature */
>   #define MMC_NO_POWER_NOTIFICATION	0
>   #define MMC_POWERED_ON			1
> -#define MMC_POWEROFF_SHORT		2
> -#define MMC_POWEROFF_LONG		3
>
>   	unsigned int		erase_size;	/* erase size in sectors */
>    	unsigned int		erase_shift;	/* if erase unit is power 2 */
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 1b431c7..54894d6 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -161,6 +161,7 @@ extern int mmc_can_trim(struct mmc_card *card);
>   extern int mmc_can_discard(struct mmc_card *card);
>   extern int mmc_can_sanitize(struct mmc_card *card);
>   extern int mmc_can_secure_erase_trim(struct mmc_card *card);
> +extern int mmc_can_poweroff_notify(const struct mmc_card *card);
>   extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>   				   unsigned int nr);
>   extern unsigned int mmc_calc_max_discard(struct mmc_card *card);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0707d22..0e9adac 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -238,12 +238,9 @@ 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;
> -#define MMC_HOST_PW_NOTIFY_NONE		0
> -#define MMC_HOST_PW_NOTIFY_SHORT	1
> -#define MMC_HOST_PW_NOTIFY_LONG		2
>
>   #ifdef CONFIG_MMC_CLKGATE
>   	int			clk_requests;	/* internal reference counter */
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index d425cab..b11876b 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -386,4 +386,11 @@ struct _mmc_csd {
>   #define MMC_SWITCH_MODE_CLEAR_BITS	0x02	/* Clear bits which are 1 in value */
>   #define MMC_SWITCH_MODE_WRITE_BYTE	0x03	/* Set target to value */
>
> +/*
> + * MMC Poweroff Notify types
> + */
> +#define MMC_PW_OFF_NOTIFY_NONE		0
> +#define MMC_PW_OFF_NOTIFY_SHORT		1
> +#define MMC_PW_OFF_NOTIFY_LONG		2
> +
>   #endif /* LINUX_MMC_MMC_H */

Ulf Hansson May 23, 2012, 11:16 a.m. UTC | #2

Hi Girish and Saugata,

This patch looks great to me!

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Ulf Hansson

On 05/22/2012 01:45 PM, Girish K S wrote:
> From: Saugata Das<saugata.das@linaro.org>
>
> 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).
>
> This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent from
> the suspend sequence. If it is sent from shutdown sequence then it is set to
> POWER_OFF_LONG.
>
> Earlier implementation of PowerOff Notify as a core function is replaced as
> a device's bus operation.
>
> Signed-off-by: Saugata Das<saugata.das@linaro.org>
> Signed-off-by: Girish K S<girish.shivananjappa@linaro.org>
>
> changes in v5:
>          modified the the handling of return value in mmc_poweroff_notify.
> changes in v4:
>          As suggested in review,
>          - Moved mmc_can_poweroff_notify to core.c
>          - Moved mmc_claim_host, mmc_release_host outside mmc_poweroff_notify
>          - Added check for wrong initialization for poweroff_notify_type
>          - mmc_poweroff_notify is modified to take as 2nd parameter
> changes in v3:
>          This version addresses the review comments given by Subhash and Ulf
> changes in v2:
>          This version addresses the changes suggested by Ulf
> ---
>   drivers/mmc/core/core.c   |  108 ++++++++++++++++++--------------------------
>   drivers/mmc/core/core.h   |    1 +
>   drivers/mmc/core/mmc.c    |   56 ++++++++++++++++++++---
>   drivers/mmc/host/dw_mmc.c |    5 --
>   drivers/mmc/host/sdhci.c  |    9 ----
>   include/linux/mmc/card.h  |    5 +-
>   include/linux/mmc/core.h  |    1 +
>   include/linux/mmc/host.h  |    5 +--
>   include/linux/mmc/mmc.h   |    7 +++
>   9 files changed, 104 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 0b6141d..fe616b9 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1101,48 +1101,6 @@ 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)
> -{
> -       struct mmc_card *card;
> -       unsigned int timeout;
> -       unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
> -       int err = 0;
> -
> -       card = host->card;
> -       mmc_claim_host(host);
> -
> -       /*
> -        * Send power notify command only if card
> -        * is mmc and notify state is powered ON
> -        */
> -       if (card&&  mmc_card_mmc(card)&&
> -           (card->poweroff_notify_state == MMC_POWERED_ON)) {
> -
> -               if (host->power_notify_type == MMC_HOST_PW_NOTIFY_SHORT) {
> -                       notify_type = EXT_CSD_POWER_OFF_SHORT;
> -                       timeout = card->ext_csd.generic_cmd6_time;
> -                       card->poweroff_notify_state = MMC_POWEROFF_SHORT;
> -               } else {
> -                       notify_type = EXT_CSD_POWER_OFF_LONG;
> -                       timeout = card->ext_csd.power_off_longtime;
> -                       card->poweroff_notify_state = MMC_POWEROFF_LONG;
> -               }
> -
> -               err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> -                                EXT_CSD_POWER_OFF_NOTIFICATION,
> -                                notify_type, timeout);
> -
> -               if (err&&  err != -EBADMSG)
> -                       pr_err("Device failed to respond within %d poweroff "
> -                              "time. Forcefully powering down the device\n",
> -                              timeout);
> -
> -               /* Set the card state to no notification after the poweroff */
> -               card->poweroff_notify_state = MMC_NO_POWER_NOTIFICATION;
> -       }
> -       mmc_release_host(host);
> -}
> -
>   /*
>    * Apply power to the MMC stack.  This is a two-stage process.
>    * First, we enable power to the card without the clock running.
> @@ -1202,8 +1160,6 @@ static void mmc_power_up(struct mmc_host *host)
>
>   void mmc_power_off(struct mmc_host *host)
>   {
> -       int err = 0;
> -
>          if (host->ios.power_mode == MMC_POWER_OFF)
>                  return;
>
> @@ -1212,22 +1168,6 @@ void mmc_power_off(struct mmc_host *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
> @@ -1726,6 +1666,15 @@ int mmc_can_secure_erase_trim(struct mmc_card *card)
>   }
>   EXPORT_SYMBOL(mmc_can_secure_erase_trim);
>
> +int mmc_can_poweroff_notify(const struct mmc_card *card)
> +{
> +       return card&&
> +               mmc_card_mmc(card)&&
> +               card->host->bus_ops->poweroff_notify&&
> +               (card->poweroff_notify_state == MMC_POWERED_ON);
> +}
> +EXPORT_SYMBOL(mmc_can_poweroff_notify);
> +
>   int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>                              unsigned int nr)
>   {
> @@ -2096,6 +2045,15 @@ void mmc_stop_host(struct mmc_host *host)
>
>          mmc_bus_get(host);
>          if (host->bus_ops&&  !host->bus_dead) {
> +               mmc_claim_host(host);
> +               if (mmc_can_poweroff_notify(host->card)) {
> +                       int err = host->bus_ops->poweroff_notify(host,
> +                                               MMC_PW_OFF_NOTIFY_LONG);
> +                       if (err)
> +                               pr_info("%s: error [%d] in poweroff notify\n",
> +                                       mmc_hostname(host), err);
> +               }
> +               mmc_release_host(host);
>                  /* Calling bus_ops->remove() with a claimed host can deadlock */
>                  if (host->bus_ops->remove)
>                          host->bus_ops->remove(host);
> @@ -2131,6 +2089,15 @@ int mmc_power_save_host(struct mmc_host *host)
>
>          if (host->bus_ops->power_save)
>                  ret = host->bus_ops->power_save(host);
> +       mmc_claim_host(host);
> +       if (mmc_can_poweroff_notify(host->card)) {
> +               int err = host->bus_ops->poweroff_notify(host,
> +                                       MMC_PW_OFF_NOTIFY_SHORT);
> +               if (err)
> +                       pr_info("%s: error [%d] in poweroff notify\n",
> +                               mmc_hostname(host), err);
> +       }
> +       mmc_release_host(host);
>
>          mmc_bus_put(host);
>
> @@ -2173,8 +2140,11 @@ int mmc_card_awake(struct mmc_host *host)
>
>          mmc_bus_get(host);
>
> -       if (host->bus_ops&&  !host->bus_dead&&  host->bus_ops->awake)
> +       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);
> +       }
>
>          mmc_bus_put(host);
>
> @@ -2191,8 +2161,11 @@ int mmc_card_sleep(struct mmc_host *host)
>
>          mmc_bus_get(host);
>
> -       if (host->bus_ops&&  !host->bus_dead&&  host->bus_ops->sleep)
> +       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);
> +       }
>
>          mmc_bus_put(host);
>
> @@ -2385,12 +2358,20 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>
>                  spin_lock_irqsave(&host->lock, flags);
>                  host->rescan_disable = 1;
> -               host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>                  spin_unlock_irqrestore(&host->lock, flags);
>                  cancel_delayed_work_sync(&host->detect);
>
>                  if (!host->bus_ops || host->bus_ops->suspend)
>                          break;
> +               mmc_claim_host(host);
> +               if (mmc_can_poweroff_notify(host->card)) {
> +                       int err = host->bus_ops->poweroff_notify(host,
> +                                               MMC_PW_OFF_NOTIFY_SHORT);
> +                       if (err)
> +                               pr_info("%s: error [%d] in poweroff notify\n",
> +                                       mmc_hostname(host), err);
> +               }
> +               mmc_release_host(host);
>
>                  /* Calling bus_ops->remove() with a claimed host can deadlock */
>                  if (host->bus_ops->remove)
> @@ -2409,7 +2390,6 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>
>                  spin_lock_irqsave(&host->lock, flags);
>                  host->rescan_disable = 0;
> -               host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;
>                  spin_unlock_irqrestore(&host->lock, flags);
>                  mmc_detect_change(host, 0);
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 3bdafbc..15b918d 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -25,6 +25,7 @@ struct mmc_bus_ops {
>          int (*power_save)(struct mmc_host *);
>          int (*power_restore)(struct mmc_host *);
>          int (*alive)(struct mmc_host *);
> +       int (*poweroff_notify)(struct mmc_host *, int notify);
>   };
>
>   void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 2f0e11c..042dbdc 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1259,6 +1259,40 @@ err:
>          return err;
>   }
>
> +static int mmc_poweroff_notify(struct mmc_host *host, int notify)
> +{
> +       struct mmc_card *card;
> +       unsigned int timeout;
> +       unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
> +       int err;
> +
> +       card = host->card;
> +
> +       if (notify == MMC_PW_OFF_NOTIFY_SHORT) {
> +               notify_type = EXT_CSD_POWER_OFF_SHORT;
> +               timeout = card->ext_csd.generic_cmd6_time;
> +       } else if (notify == MMC_PW_OFF_NOTIFY_LONG) {
> +               notify_type = EXT_CSD_POWER_OFF_LONG;
> +               timeout = card->ext_csd.power_off_longtime;
> +       } else {
> +               pr_info("%s: mmc_poweroff_notify called "
> +                       "with notify type %d\n", mmc_hostname(host), notify);
> +               return -EINVAL;
> +       }
> +
> +       err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +                        EXT_CSD_POWER_OFF_NOTIFICATION,
> +                        notify_type, timeout);
> +
> +       if (err)
> +               pr_err("%s: Device failed to respond within %d "
> +                      "poweroff timeout.\n", mmc_hostname(host), timeout);
> +       else
> +               card->poweroff_notify_state =
> +                                       MMC_NO_POWER_NOTIFICATION;
> +
> +       return err;
> +}
>   /*
>    * Host is being removed. Free up the current card.
>    */
> @@ -1319,13 +1353,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 (!err)
> -                       mmc_card_set_sleep(host->card);
> -       } else if (!mmc_host_is_spi(host))
> -               mmc_deselect_cards(host);
> -       host->card->state&= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
> +       if (mmc_can_poweroff_notify(host->card)&&
> +               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
> +               err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
> +       } else {
> +               if (mmc_card_can_sleep(host))
> +                       err = mmc_card_sleep(host);
> +               else if (!mmc_host_is_spi(host))
> +                       mmc_deselect_cards(host);
> +       }
> +       if (!err)
> +               host->card->state&=
> +                       ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
>          mmc_release_host(host);
>
>          return err;
> @@ -1347,7 +1386,6 @@ static int mmc_resume(struct mmc_host *host)
>          mmc_claim_host(host);
>          if (mmc_card_is_sleep(host->card)) {
>                  err = mmc_card_awake(host);
> -               mmc_card_clr_sleep(host->card);
>          } else
>                  err = mmc_init_card(host, host->ocr, host->card);
>          mmc_release_host(host);
> @@ -1407,6 +1445,7 @@ static const struct mmc_bus_ops mmc_ops = {
>          .resume = NULL,
>          .power_restore = mmc_power_restore,
>          .alive = mmc_alive,
> +       .poweroff_notify = mmc_poweroff_notify,
>   };
>
>   static const struct mmc_bus_ops mmc_ops_unsafe = {
> @@ -1418,6 +1457,7 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>          .resume = mmc_resume,
>          .power_restore = mmc_power_restore,
>          .alive = mmc_alive,
> +       .poweroff_notify = mmc_poweroff_notify,
>   };
>
>   static void mmc_attach_bus_ops(struct mmc_host *host)
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 1532357..463130f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1788,11 +1788,6 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>          if (host->pdata->quirks&  DW_MCI_QUIRK_HIGHSPEED)
>                  mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>
> -       if (mmc->caps2&  MMC_CAP2_POWEROFF_NOTIFY)
> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> -       else
> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
> -
>          if (host->pdata->blk_settings) {
>                  mmc->max_segs = host->pdata->blk_settings->max_segs;
>                  mmc->max_blk_size = host->pdata->blk_settings->max_blk_size;
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e626732..c0a5a91 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2812,15 +2812,6 @@ int sdhci_add_host(struct sdhci_host *host)
>          if (caps[1]&  SDHCI_DRIVER_TYPE_D)
>                  mmc->caps |= MMC_CAP_DRIVER_TYPE_D;
>
> -       /*
> -        * If Power Off Notify capability is enabled by the host,
> -        * set notify to short power off notify timeout value.
> -        */
> -       if (mmc->caps2&  MMC_CAP2_POWEROFF_NOTIFY)
> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> -       else
> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
> -
>          /* Initial value for re-tuning timer count */
>          host->tuning_count = (caps[1]&  SDHCI_RETUNING_TIMER_COUNT_MASK)>>
>                                SDHCI_RETUNING_TIMER_COUNT_SHIFT;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index d76513b..040eec4 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -239,11 +239,10 @@ struct mmc_card {
>   #define MMC_QUIRK_BROKEN_BYTE_MODE_512 (1<<8)  /* Avoid sending 512 bytes in */
>   #define MMC_QUIRK_LONG_READ_TIME (1<<9)                /* Data read time>  CSD says */
>                                                  /* byte mode */
> -       unsigned int    poweroff_notify_state;  /* eMMC4.5 notify feature */
> +       unsigned int            poweroff_notify_state; /* MMC-4.5 poweroff
> +                                                       notify feature */
>   #define MMC_NO_POWER_NOTIFICATION      0
>   #define MMC_POWERED_ON                 1
> -#define MMC_POWEROFF_SHORT             2
> -#define MMC_POWEROFF_LONG              3
>
>          unsigned int            erase_size;     /* erase size in sectors */
>          unsigned int            erase_shift;    /* if erase unit is power 2 */
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 1b431c7..54894d6 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -161,6 +161,7 @@ extern int mmc_can_trim(struct mmc_card *card);
>   extern int mmc_can_discard(struct mmc_card *card);
>   extern int mmc_can_sanitize(struct mmc_card *card);
>   extern int mmc_can_secure_erase_trim(struct mmc_card *card);
> +extern int mmc_can_poweroff_notify(const struct mmc_card *card);
>   extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>                                     unsigned int nr);
>   extern unsigned int mmc_calc_max_discard(struct mmc_card *card);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0707d22..0e9adac 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -238,12 +238,9 @@ 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;
> -#define MMC_HOST_PW_NOTIFY_NONE                0
> -#define MMC_HOST_PW_NOTIFY_SHORT       1
> -#define MMC_HOST_PW_NOTIFY_LONG                2
>
>   #ifdef CONFIG_MMC_CLKGATE
>          int                     clk_requests;   /* internal reference counter */
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index d425cab..b11876b 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -386,4 +386,11 @@ struct _mmc_csd {
>   #define MMC_SWITCH_MODE_CLEAR_BITS     0x02    /* Clear bits which are 1 in value */
>   #define MMC_SWITCH_MODE_WRITE_BYTE     0x03    /* Set target to value */
>
> +/*
> + * MMC Poweroff Notify types
> + */
> +#define MMC_PW_OFF_NOTIFY_NONE         0
> +#define MMC_PW_OFF_NOTIFY_SHORT                1
> +#define MMC_PW_OFF_NOTIFY_LONG         2
> +
>   #endif /* LINUX_MMC_MMC_H */
> --
> 1.7.4.1
>

Subhash Jadavani May 28, 2012, 11:03 a.m. UTC | #3

Hi Girish, Saugata,

There is an issue with this patch during resume. Please find comments inline
below:

> -----Original Message-----
> From: Girish K S [mailto:girish.shivananjappa@linaro.org]
> Sent: Tuesday, May 22, 2012 5:15 PM
> To: linux-mmc@vger.kernel.org
> Cc: cjb@laptop.org; patches@linaro.org; ulf.hansson@stericsson.com;
> saugata.das@linaro.org; subhashj@codeaurora.org; Girish K S
> Subject: [PATCH v5] MMC-4.5 Power OFF Notify Rework
> 
> From: Saugata Das <saugata.das@linaro.org>
> 
> 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).
> 
> This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent
> from the suspend sequence. If it is sent from shutdown sequence then it is
set
> to POWER_OFF_LONG.
> 
> Earlier implementation of PowerOff Notify as a core function is replaced
as a
> device's bus operation.
> 
> Signed-off-by: Saugata Das <saugata.das@linaro.org>
> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org>
> 
> changes in v5:
> 	modified the the handling of return value in mmc_poweroff_notify.
> changes in v4:
> 	As suggested in review,
> 	- Moved mmc_can_poweroff_notify to core.c
> 	- Moved mmc_claim_host, mmc_release_host outside
> mmc_poweroff_notify
> 	- Added check for wrong initialization for poweroff_notify_type
> 	- mmc_poweroff_notify is modified to take as 2nd parameter changes
> in v3:
> 	This version addresses the review comments given by Subhash and Ulf
> changes in v2:
> 	This version addresses the changes suggested by Ulf
> ---
>  drivers/mmc/core/core.c   |  108
++++++++++++++++++--------------------------
>  drivers/mmc/core/core.h   |    1 +
>  drivers/mmc/core/mmc.c    |   56 ++++++++++++++++++++---
>  drivers/mmc/host/dw_mmc.c |    5 --
>  drivers/mmc/host/sdhci.c  |    9 ----
>  include/linux/mmc/card.h  |    5 +-
>  include/linux/mmc/core.h  |    1 +
>  include/linux/mmc/host.h  |    5 +--
>  include/linux/mmc/mmc.h   |    7 +++
>  9 files changed, 104 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> 0b6141d..fe616b9 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1101,48 +1101,6 @@ 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) -{
> -	struct mmc_card *card;
> -	unsigned int timeout;
> -	unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
> -	int err = 0;
> -
> -	card = host->card;
> -	mmc_claim_host(host);
> -
> -	/*
> -	 * Send power notify command only if card
> -	 * is mmc and notify state is powered ON
> -	 */
> -	if (card && mmc_card_mmc(card) &&
> -	    (card->poweroff_notify_state == MMC_POWERED_ON)) {
> -
> -		if (host->power_notify_type ==
> MMC_HOST_PW_NOTIFY_SHORT) {
> -			notify_type = EXT_CSD_POWER_OFF_SHORT;
> -			timeout = card->ext_csd.generic_cmd6_time;
> -			card->poweroff_notify_state =
> MMC_POWEROFF_SHORT;
> -		} else {
> -			notify_type = EXT_CSD_POWER_OFF_LONG;
> -			timeout = card->ext_csd.power_off_longtime;
> -			card->poweroff_notify_state =
> MMC_POWEROFF_LONG;
> -		}
> -
> -		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> -				 EXT_CSD_POWER_OFF_NOTIFICATION,
> -				 notify_type, timeout);
> -
> -		if (err && err != -EBADMSG)
> -			pr_err("Device failed to respond within %d poweroff
"
> -			       "time. Forcefully powering down the
device\n",
> -			       timeout);
> -
> -		/* Set the card state to no notification after the poweroff
*/
> -		card->poweroff_notify_state =
> MMC_NO_POWER_NOTIFICATION;
> -	}
> -	mmc_release_host(host);
> -}
> -
>  /*
>   * Apply power to the MMC stack.  This is a two-stage process.
>   * First, we enable power to the card without the clock running.
> @@ -1202,8 +1160,6 @@ static void mmc_power_up(struct mmc_host *host)
> 
>  void mmc_power_off(struct mmc_host *host)  {
> -	int err = 0;
> -
>  	if (host->ios.power_mode == MMC_POWER_OFF)
>  		return;
> 
> @@ -1212,22 +1168,6 @@ void mmc_power_off(struct mmc_host *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
@@
> -1726,6 +1666,15 @@ int mmc_can_secure_erase_trim(struct mmc_card
> *card)  }  EXPORT_SYMBOL(mmc_can_secure_erase_trim);
> 
> +int mmc_can_poweroff_notify(const struct mmc_card *card) {
> +	return card &&
> +		mmc_card_mmc(card) &&
> +		card->host->bus_ops->poweroff_notify &&
> +		(card->poweroff_notify_state == MMC_POWERED_ON); }
> +EXPORT_SYMBOL(mmc_can_poweroff_notify);
> +
>  int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>  			    unsigned int nr)
>  {
> @@ -2096,6 +2045,15 @@ void mmc_stop_host(struct mmc_host *host)
> 
>  	mmc_bus_get(host);
>  	if (host->bus_ops && !host->bus_dead) {
> +		mmc_claim_host(host);
> +		if (mmc_can_poweroff_notify(host->card)) {
> +			int err = host->bus_ops->poweroff_notify(host,
> +
> 	MMC_PW_OFF_NOTIFY_LONG);
> +			if (err)
> +				pr_info("%s: error [%d] in poweroff
notify\n",
> +					mmc_hostname(host), err);
> +		}
> +		mmc_release_host(host);
>  		/* Calling bus_ops->remove() with a claimed host can
deadlock
> */
>  		if (host->bus_ops->remove)
>  			host->bus_ops->remove(host);
> @@ -2131,6 +2089,15 @@ int mmc_power_save_host(struct mmc_host *host)
> 
>  	if (host->bus_ops->power_save)
>  		ret = host->bus_ops->power_save(host);
> +	mmc_claim_host(host);
> +	if (mmc_can_poweroff_notify(host->card)) {
> +		int err = host->bus_ops->poweroff_notify(host,
> +					MMC_PW_OFF_NOTIFY_SHORT);
> +		if (err)
> +			pr_info("%s: error [%d] in poweroff notify\n",
> +				mmc_hostname(host), err);
> +	}
> +	mmc_release_host(host);
> 
>  	mmc_bus_put(host);
> 
> @@ -2173,8 +2140,11 @@ int mmc_card_awake(struct mmc_host *host)
> 
>  	mmc_bus_get(host);
> 
> -	if (host->bus_ops && !host->bus_dead && host->bus_ops->awake)
> +	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);
> +	}
> 
>  	mmc_bus_put(host);
> 
> @@ -2191,8 +2161,11 @@ int mmc_card_sleep(struct mmc_host *host)
> 
>  	mmc_bus_get(host);
> 
> -	if (host->bus_ops && !host->bus_dead && host->bus_ops->sleep)
> +	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);
> +	}
> 
>  	mmc_bus_put(host);
> 
> @@ -2385,12 +2358,20 @@ int mmc_pm_notify(struct notifier_block
> *notify_block,
> 
>  		spin_lock_irqsave(&host->lock, flags);
>  		host->rescan_disable = 1;
> -		host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>  		spin_unlock_irqrestore(&host->lock, flags);
>  		cancel_delayed_work_sync(&host->detect);
> 
>  		if (!host->bus_ops || host->bus_ops->suspend)
>  			break;
> +		mmc_claim_host(host);
> +		if (mmc_can_poweroff_notify(host->card)) {
> +			int err = host->bus_ops->poweroff_notify(host,
> +
> 	MMC_PW_OFF_NOTIFY_SHORT);
> +			if (err)
> +				pr_info("%s: error [%d] in poweroff
notify\n",
> +					mmc_hostname(host), err);
> +		}
> +		mmc_release_host(host);
> 
>  		/* Calling bus_ops->remove() with a claimed host can
deadlock
> */
>  		if (host->bus_ops->remove)
> @@ -2409,7 +2390,6 @@ int mmc_pm_notify(struct notifier_block
> *notify_block,
> 
>  		spin_lock_irqsave(&host->lock, flags);
>  		host->rescan_disable = 0;
> -		host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;
>  		spin_unlock_irqrestore(&host->lock, flags);
>  		mmc_detect_change(host, 0);
> 
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index
> 3bdafbc..15b918d 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -25,6 +25,7 @@ struct mmc_bus_ops {
>  	int (*power_save)(struct mmc_host *);
>  	int (*power_restore)(struct mmc_host *);
>  	int (*alive)(struct mmc_host *);
> +	int (*poweroff_notify)(struct mmc_host *, int notify);
>  };
> 
>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops
> *ops); diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
> 2f0e11c..042dbdc 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1259,6 +1259,40 @@ err:
>  	return err;
>  }
> 
> +static int mmc_poweroff_notify(struct mmc_host *host, int notify) {
> +	struct mmc_card *card;
> +	unsigned int timeout;
> +	unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
> +	int err;
> +
> +	card = host->card;
> +
> +	if (notify == MMC_PW_OFF_NOTIFY_SHORT) {
> +		notify_type = EXT_CSD_POWER_OFF_SHORT;
> +		timeout = card->ext_csd.generic_cmd6_time;
> +	} else if (notify == MMC_PW_OFF_NOTIFY_LONG) {
> +		notify_type = EXT_CSD_POWER_OFF_LONG;
> +		timeout = card->ext_csd.power_off_longtime;
> +	} else {
> +		pr_info("%s: mmc_poweroff_notify called "
> +		        "with notify type %d\n", mmc_hostname(host),
notify);
> +		return -EINVAL;
> +	}
> +
> +	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +			 EXT_CSD_POWER_OFF_NOTIFICATION,
> +			 notify_type, timeout);
> +
> +	if (err)
> +		pr_err("%s: Device failed to respond within %d "
> +		       "poweroff timeout.\n", mmc_hostname(host), timeout);
> +	else
> +		card->poweroff_notify_state =
> +					MMC_NO_POWER_NOTIFICATION;
> +
> +	return err;
> +}
>  /*
>   * Host is being removed. Free up the current card.
>   */
> @@ -1319,13 +1353,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 (!err)
> -			mmc_card_set_sleep(host->card);
> -	} else if (!mmc_host_is_spi(host))
> -		mmc_deselect_cards(host);
> -	host->card->state &= ~(MMC_STATE_HIGHSPEED |
> MMC_STATE_HIGHSPEED_200);
> +	if (mmc_can_poweroff_notify(host->card) &&
> +		(host->caps2 &
> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
> +		err = mmc_poweroff_notify(host,
> MMC_PW_OFF_NOTIFY_SHORT);
> +	} else {
> +		if (mmc_card_can_sleep(host))
> +			err = mmc_card_sleep(host);
> +		else if (!mmc_host_is_spi(host))
> +			mmc_deselect_cards(host);
> +	}
> +	if (!err)
> +		host->card->state &=
> +			~(MMC_STATE_HIGHSPEED |
> MMC_STATE_HIGHSPEED_200);
>  	mmc_release_host(host);
> 
>  	return err;
> @@ -1347,7 +1386,6 @@ static int mmc_resume(struct mmc_host *host)
>  	mmc_claim_host(host);
>  	if (mmc_card_is_sleep(host->card)) {
>  		err = mmc_card_awake(host);
> -		mmc_card_clr_sleep(host->card);

Without this patch, this was the suspend sequence/resume sequence for MMC
cards:
mmc_suspend_host()
	-> host->bus_ops->suspend() 
		-> mmc_suspend() 
			-> mmc_card_can_sleep() 
			-> mmc_card_sleep() 
			-> mmc_card_set_sleep()
	-> mmc_power_off()
		-> mmc_card_is_sleep(host->card)
		-> host->bus_ops->resume()
			-> mmc_resume()
				-> mmc_card_is_sleep()
				-> mmc_card_awake()
				-> mmc_card_clr_sleep()


mmc_resume_host()
	-> mmc_power_up()
		-> This sets the buswidth=1, timing=legacy and clock=f_init
	-> host->bus_ops->resume()
		-> mmc_resume()
			-> mmc_card_is_sleep() -> this would return false
because in mmc_suspend_host(), host->bus_ops->resume() call had cleared the
sleep flag.
		-> mmc_init_card() -> As card sleep state is cleared,
init_card() will be done which sets up the card again.


But with this patch, you have removed the "host->bus_ops->resume()" call
from mmc_power_off() which would mean card sleep state would be set to
MMC_STATE_SLEEP during mmc_resume_host(). 
Following is the sequence with this patch:

mmc_suspend_host()
	-> host->bus_ops->suspend() 
		-> mmc_suspend() 
			-> mmc_card_can_sleep() 
			-> mmc_card_sleep() 
			-> mmc_card_set_sleep()
	-> mmc_power_off()

mmc_resume_host()
	-> mmc_power_up()
		-> This sets the bus_width=1, timing=legacy and clock=f_init
	-> host->bus_ops->resume()
		-> mmc_resume()
			-> mmc_card_is_sleep() -> this would return true
because card sleep flag is set.
			-> mmc_card_awake() 

Block read/write operation:
	-> mmc_blk_issue_rw_rq
		-> send CMD25
			-> But at this point, bus_width=1, timing=legacy and
clock=f_init. Which is causing card not to respond back to data blocks sent
from card and we are seeing the data timeout errors.

So basically to fix this, in mmc_resume() either you have to call
mmc_init_card() instead of mmc_card_awake() or if we still want to just call
the mmc_card_awake() from resume, we need to save the state of host->ios
before the sleep command issue so during resume, before calling mmc_awake(),
we need to restore the saved host->ios state back and call mmc_set_ios() to
restore the original parameters.

Also, I couple of other concerns here: In mmc_suspend_host(), after we put
the card into suspend (host->bus_ops_suspend()), we call mmc_power_off()
which asks the host controller to switch off the eMMC power. Now assume that
a particular host controller driver turns off both VCC and VCCQ rails when
ios.power_mode = MMC_POWER_OFF. 
Now in mmc_resume_host(), we call mmc_power_up() which would switch ON the
power back which would mean eMMC card have seen power down and power up
sequence and that means card is no longer in sleep state. And now after
mmc_power_up(), when we call the mmc_card_awake() from mmc_resume(), card
may not even respond to CMD5 awake. So in this scenario, we should actually
call mmc_init_card() to bring the card back to proper state.

Regards,
Subhash

>  	} else
>  		err = mmc_init_card(host, host->ocr, host->card);
>  	mmc_release_host(host);
> @@ -1407,6 +1445,7 @@ static const struct mmc_bus_ops mmc_ops = {
>  	.resume = NULL,
>  	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
> +	.poweroff_notify = mmc_poweroff_notify,
>  };
> 
>  static const struct mmc_bus_ops mmc_ops_unsafe = { @@ -1418,6 +1457,7
> @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>  	.resume = mmc_resume,
>  	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
> +	.poweroff_notify = mmc_poweroff_notify,
>  };
> 
>  static void mmc_attach_bus_ops(struct mmc_host *host) diff --git
> a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index
> 1532357..463130f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1788,11 +1788,6 @@ static int __init dw_mci_init_slot(struct dw_mci
> *host, unsigned int id)
>  	if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
>  		mmc->caps |= MMC_CAP_SD_HIGHSPEED |
> MMC_CAP_MMC_HIGHSPEED;
> 
> -	if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
> -		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> -	else
> -		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
> -
>  	if (host->pdata->blk_settings) {
>  		mmc->max_segs = host->pdata->blk_settings->max_segs;
>  		mmc->max_blk_size = host->pdata->blk_settings-
> >max_blk_size;
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
> e626732..c0a5a91 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2812,15 +2812,6 @@ int sdhci_add_host(struct sdhci_host *host)
>  	if (caps[1] & SDHCI_DRIVER_TYPE_D)
>  		mmc->caps |= MMC_CAP_DRIVER_TYPE_D;
> 
> -	/*
> -	 * If Power Off Notify capability is enabled by the host,
> -	 * set notify to short power off notify timeout value.
> -	 */
> -	if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
> -		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> -	else
> -		mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
> -
>  	/* Initial value for re-tuning timer count */
>  	host->tuning_count = (caps[1] &
> SDHCI_RETUNING_TIMER_COUNT_MASK) >>
>  			      SDHCI_RETUNING_TIMER_COUNT_SHIFT; diff --git
> a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
> d76513b..040eec4 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -239,11 +239,10 @@ struct mmc_card {
>  #define MMC_QUIRK_BROKEN_BYTE_MODE_512 (1<<8)	/* Avoid
> sending 512 bytes in */
>  #define MMC_QUIRK_LONG_READ_TIME (1<<9)		/* Data read
> time > CSD says */
>  						/* byte mode */
> -	unsigned int    poweroff_notify_state;	/* eMMC4.5 notify feature */
> +	unsigned int		poweroff_notify_state; /* MMC-4.5 poweroff
> +							notify feature */
>  #define MMC_NO_POWER_NOTIFICATION	0
>  #define MMC_POWERED_ON			1
> -#define MMC_POWEROFF_SHORT		2
> -#define MMC_POWEROFF_LONG		3
> 
>  	unsigned int		erase_size;	/* erase size in sectors */
>   	unsigned int		erase_shift;	/* if erase unit is power 2
*/
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index
> 1b431c7..54894d6 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -161,6 +161,7 @@ extern int mmc_can_trim(struct mmc_card *card);
> extern int mmc_can_discard(struct mmc_card *card);  extern int
> mmc_can_sanitize(struct mmc_card *card);  extern int
> mmc_can_secure_erase_trim(struct mmc_card *card);
> +extern int mmc_can_poweroff_notify(const struct mmc_card *card);
>  extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int
> from,
>  				   unsigned int nr);
>  extern unsigned int mmc_calc_max_discard(struct mmc_card *card); diff
--git
> a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
> 0707d22..0e9adac 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -238,12 +238,9 @@ 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;
> -#define MMC_HOST_PW_NOTIFY_NONE		0
> -#define MMC_HOST_PW_NOTIFY_SHORT	1
> -#define MMC_HOST_PW_NOTIFY_LONG		2
> 
>  #ifdef CONFIG_MMC_CLKGATE
>  	int			clk_requests;	/* internal reference
counter
> */
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index
> d425cab..b11876b 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -386,4 +386,11 @@ struct _mmc_csd {
>  #define MMC_SWITCH_MODE_CLEAR_BITS	0x02	/* Clear bits which are
> 1 in value */
>  #define MMC_SWITCH_MODE_WRITE_BYTE	0x03	/* Set target to value
> */
> 
> +/*
> + * MMC Poweroff Notify types
> + */
> +#define MMC_PW_OFF_NOTIFY_NONE		0
> +#define MMC_PW_OFF_NOTIFY_SHORT		1
> +#define MMC_PW_OFF_NOTIFY_LONG		2
> +
>  #endif /* LINUX_MMC_MMC_H */
> --
> 1.7.4.1

Saugata Das May 28, 2012, 3:07 p.m. UTC | #4

On 28 May 2012 16:33, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> Hi Girish, Saugata,
>
> There is an issue with this patch during resume. Please find comments inline
> below:
>
>> -----Original Message-----
>> From: Girish K S [mailto:girish.shivananjappa@linaro.org]
>> Sent: Tuesday, May 22, 2012 5:15 PM
>> To: linux-mmc@vger.kernel.org
>> Cc: cjb@laptop.org; patches@linaro.org; ulf.hansson@stericsson.com;
>> saugata.das@linaro.org; subhashj@codeaurora.org; Girish K S
>> Subject: [PATCH v5] MMC-4.5 Power OFF Notify Rework
>>
>> From: Saugata Das <saugata.das@linaro.org>
>>
>> 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).
>>
>> This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent
>> from the suspend sequence. If it is sent from shutdown sequence then it is
> set
>> to POWER_OFF_LONG.
>>
>> Earlier implementation of PowerOff Notify as a core function is replaced
> as a
>> device's bus operation.
>>
>> Signed-off-by: Saugata Das <saugata.das@linaro.org>
>> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org>
>>
>> changes in v5:
>>       modified the the handling of return value in mmc_poweroff_notify.
>> changes in v4:
>>       As suggested in review,
>>       - Moved mmc_can_poweroff_notify to core.c
>>       - Moved mmc_claim_host, mmc_release_host outside
>> mmc_poweroff_notify
>>       - Added check for wrong initialization for poweroff_notify_type
>>       - mmc_poweroff_notify is modified to take as 2nd parameter changes
>> in v3:
>>       This version addresses the review comments given by Subhash and Ulf
>> changes in v2:
>>       This version addresses the changes suggested by Ulf
>> ---
>>  drivers/mmc/core/core.c   |  108
> ++++++++++++++++++--------------------------
>>  drivers/mmc/core/core.h   |    1 +
>>  drivers/mmc/core/mmc.c    |   56 ++++++++++++++++++++---
>>  drivers/mmc/host/dw_mmc.c |    5 --
>>  drivers/mmc/host/sdhci.c  |    9 ----
>>  include/linux/mmc/card.h  |    5 +-
>>  include/linux/mmc/core.h  |    1 +
>>  include/linux/mmc/host.h  |    5 +--
>>  include/linux/mmc/mmc.h   |    7 +++
>>  9 files changed, 104 insertions(+), 93 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
>> 0b6141d..fe616b9 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1101,48 +1101,6 @@ 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) -{
>> -     struct mmc_card *card;
>> -     unsigned int timeout;
>> -     unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>> -     int err = 0;
>> -
>> -     card = host->card;
>> -     mmc_claim_host(host);
>> -
>> -     /*
>> -      * Send power notify command only if card
>> -      * is mmc and notify state is powered ON
>> -      */
>> -     if (card && mmc_card_mmc(card) &&
>> -         (card->poweroff_notify_state == MMC_POWERED_ON)) {
>> -
>> -             if (host->power_notify_type ==
>> MMC_HOST_PW_NOTIFY_SHORT) {
>> -                     notify_type = EXT_CSD_POWER_OFF_SHORT;
>> -                     timeout = card->ext_csd.generic_cmd6_time;
>> -                     card->poweroff_notify_state =
>> MMC_POWEROFF_SHORT;
>> -             } else {
>> -                     notify_type = EXT_CSD_POWER_OFF_LONG;
>> -                     timeout = card->ext_csd.power_off_longtime;
>> -                     card->poweroff_notify_state =
>> MMC_POWEROFF_LONG;
>> -             }
>> -
>> -             err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> -                              EXT_CSD_POWER_OFF_NOTIFICATION,
>> -                              notify_type, timeout);
>> -
>> -             if (err && err != -EBADMSG)
>> -                     pr_err("Device failed to respond within %d poweroff
> "
>> -                            "time. Forcefully powering down the
> device\n",
>> -                            timeout);
>> -
>> -             /* Set the card state to no notification after the poweroff
> */
>> -             card->poweroff_notify_state =
>> MMC_NO_POWER_NOTIFICATION;
>> -     }
>> -     mmc_release_host(host);
>> -}
>> -
>>  /*
>>   * Apply power to the MMC stack.  This is a two-stage process.
>>   * First, we enable power to the card without the clock running.
>> @@ -1202,8 +1160,6 @@ static void mmc_power_up(struct mmc_host *host)
>>
>>  void mmc_power_off(struct mmc_host *host)  {
>> -     int err = 0;
>> -
>>       if (host->ios.power_mode == MMC_POWER_OFF)
>>               return;
>>
>> @@ -1212,22 +1168,6 @@ void mmc_power_off(struct mmc_host *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
> @@
>> -1726,6 +1666,15 @@ int mmc_can_secure_erase_trim(struct mmc_card
>> *card)  }  EXPORT_SYMBOL(mmc_can_secure_erase_trim);
>>
>> +int mmc_can_poweroff_notify(const struct mmc_card *card) {
>> +     return card &&
>> +             mmc_card_mmc(card) &&
>> +             card->host->bus_ops->poweroff_notify &&
>> +             (card->poweroff_notify_state == MMC_POWERED_ON); }
>> +EXPORT_SYMBOL(mmc_can_poweroff_notify);
>> +
>>  int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>>                           unsigned int nr)
>>  {
>> @@ -2096,6 +2045,15 @@ void mmc_stop_host(struct mmc_host *host)
>>
>>       mmc_bus_get(host);
>>       if (host->bus_ops && !host->bus_dead) {
>> +             mmc_claim_host(host);
>> +             if (mmc_can_poweroff_notify(host->card)) {
>> +                     int err = host->bus_ops->poweroff_notify(host,
>> +
>>       MMC_PW_OFF_NOTIFY_LONG);
>> +                     if (err)
>> +                             pr_info("%s: error [%d] in poweroff
> notify\n",
>> +                                     mmc_hostname(host), err);
>> +             }
>> +             mmc_release_host(host);
>>               /* Calling bus_ops->remove() with a claimed host can
> deadlock
>> */
>>               if (host->bus_ops->remove)
>>                       host->bus_ops->remove(host);
>> @@ -2131,6 +2089,15 @@ int mmc_power_save_host(struct mmc_host *host)
>>
>>       if (host->bus_ops->power_save)
>>               ret = host->bus_ops->power_save(host);
>> +     mmc_claim_host(host);
>> +     if (mmc_can_poweroff_notify(host->card)) {
>> +             int err = host->bus_ops->poweroff_notify(host,
>> +                                     MMC_PW_OFF_NOTIFY_SHORT);
>> +             if (err)
>> +                     pr_info("%s: error [%d] in poweroff notify\n",
>> +                             mmc_hostname(host), err);
>> +     }
>> +     mmc_release_host(host);
>>
>>       mmc_bus_put(host);
>>
>> @@ -2173,8 +2140,11 @@ int mmc_card_awake(struct mmc_host *host)
>>
>>       mmc_bus_get(host);
>>
>> -     if (host->bus_ops && !host->bus_dead && host->bus_ops->awake)
>> +     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);
>> +     }
>>
>>       mmc_bus_put(host);
>>
>> @@ -2191,8 +2161,11 @@ int mmc_card_sleep(struct mmc_host *host)
>>
>>       mmc_bus_get(host);
>>
>> -     if (host->bus_ops && !host->bus_dead && host->bus_ops->sleep)
>> +     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);
>> +     }
>>
>>       mmc_bus_put(host);
>>
>> @@ -2385,12 +2358,20 @@ int mmc_pm_notify(struct notifier_block
>> *notify_block,
>>
>>               spin_lock_irqsave(&host->lock, flags);
>>               host->rescan_disable = 1;
>> -             host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>>               spin_unlock_irqrestore(&host->lock, flags);
>>               cancel_delayed_work_sync(&host->detect);
>>
>>               if (!host->bus_ops || host->bus_ops->suspend)
>>                       break;
>> +             mmc_claim_host(host);
>> +             if (mmc_can_poweroff_notify(host->card)) {
>> +                     int err = host->bus_ops->poweroff_notify(host,
>> +
>>       MMC_PW_OFF_NOTIFY_SHORT);
>> +                     if (err)
>> +                             pr_info("%s: error [%d] in poweroff
> notify\n",
>> +                                     mmc_hostname(host), err);
>> +             }
>> +             mmc_release_host(host);
>>
>>               /* Calling bus_ops->remove() with a claimed host can
> deadlock
>> */
>>               if (host->bus_ops->remove)
>> @@ -2409,7 +2390,6 @@ int mmc_pm_notify(struct notifier_block
>> *notify_block,
>>
>>               spin_lock_irqsave(&host->lock, flags);
>>               host->rescan_disable = 0;
>> -             host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;
>>               spin_unlock_irqrestore(&host->lock, flags);
>>               mmc_detect_change(host, 0);
>>
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index
>> 3bdafbc..15b918d 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -25,6 +25,7 @@ struct mmc_bus_ops {
>>       int (*power_save)(struct mmc_host *);
>>       int (*power_restore)(struct mmc_host *);
>>       int (*alive)(struct mmc_host *);
>> +     int (*poweroff_notify)(struct mmc_host *, int notify);
>>  };
>>
>>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops
>> *ops); diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
>> 2f0e11c..042dbdc 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1259,6 +1259,40 @@ err:
>>       return err;
>>  }
>>
>> +static int mmc_poweroff_notify(struct mmc_host *host, int notify) {
>> +     struct mmc_card *card;
>> +     unsigned int timeout;
>> +     unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>> +     int err;
>> +
>> +     card = host->card;
>> +
>> +     if (notify == MMC_PW_OFF_NOTIFY_SHORT) {
>> +             notify_type = EXT_CSD_POWER_OFF_SHORT;
>> +             timeout = card->ext_csd.generic_cmd6_time;
>> +     } else if (notify == MMC_PW_OFF_NOTIFY_LONG) {
>> +             notify_type = EXT_CSD_POWER_OFF_LONG;
>> +             timeout = card->ext_csd.power_off_longtime;
>> +     } else {
>> +             pr_info("%s: mmc_poweroff_notify called "
>> +                     "with notify type %d\n", mmc_hostname(host),
> notify);
>> +             return -EINVAL;
>> +     }
>> +
>> +     err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> +                      EXT_CSD_POWER_OFF_NOTIFICATION,
>> +                      notify_type, timeout);
>> +
>> +     if (err)
>> +             pr_err("%s: Device failed to respond within %d "
>> +                    "poweroff timeout.\n", mmc_hostname(host), timeout);
>> +     else
>> +             card->poweroff_notify_state =
>> +                                     MMC_NO_POWER_NOTIFICATION;
>> +
>> +     return err;
>> +}
>>  /*
>>   * Host is being removed. Free up the current card.
>>   */
>> @@ -1319,13 +1353,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 (!err)
>> -                     mmc_card_set_sleep(host->card);
>> -     } else if (!mmc_host_is_spi(host))
>> -             mmc_deselect_cards(host);
>> -     host->card->state &= ~(MMC_STATE_HIGHSPEED |
>> MMC_STATE_HIGHSPEED_200);
>> +     if (mmc_can_poweroff_notify(host->card) &&
>> +             (host->caps2 &
>> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>> +             err = mmc_poweroff_notify(host,
>> MMC_PW_OFF_NOTIFY_SHORT);
>> +     } else {
>> +             if (mmc_card_can_sleep(host))
>> +                     err = mmc_card_sleep(host);
>> +             else if (!mmc_host_is_spi(host))
>> +                     mmc_deselect_cards(host);
>> +     }
>> +     if (!err)
>> +             host->card->state &=
>> +                     ~(MMC_STATE_HIGHSPEED |
>> MMC_STATE_HIGHSPEED_200);
>>       mmc_release_host(host);
>>
>>       return err;
>> @@ -1347,7 +1386,6 @@ static int mmc_resume(struct mmc_host *host)
>>       mmc_claim_host(host);
>>       if (mmc_card_is_sleep(host->card)) {
>>               err = mmc_card_awake(host);
>> -             mmc_card_clr_sleep(host->card);
>
> Without this patch, this was the suspend sequence/resume sequence for MMC
> cards:
> mmc_suspend_host()
>        -> host->bus_ops->suspend()
>                -> mmc_suspend()
>                        -> mmc_card_can_sleep()
>                        -> mmc_card_sleep()
>                        -> mmc_card_set_sleep()
>        -> mmc_power_off()
>                -> mmc_card_is_sleep(host->card)
>                -> host->bus_ops->resume()
>                        -> mmc_resume()
>                                -> mmc_card_is_sleep()
>                                -> mmc_card_awake()
>                                -> mmc_card_clr_sleep()
>
>
> mmc_resume_host()
>        -> mmc_power_up()
>                -> This sets the buswidth=1, timing=legacy and clock=f_init
>        -> host->bus_ops->resume()
>                -> mmc_resume()
>                        -> mmc_card_is_sleep() -> this would return false
> because in mmc_suspend_host(), host->bus_ops->resume() call had cleared the
> sleep flag.
>                -> mmc_init_card() -> As card sleep state is cleared,
> init_card() will be done which sets up the card again.
>
>
> But with this patch, you have removed the "host->bus_ops->resume()" call
> from mmc_power_off() which would mean card sleep state would be set to
> MMC_STATE_SLEEP during mmc_resume_host().
> Following is the sequence with this patch:
>
> mmc_suspend_host()
>        -> host->bus_ops->suspend()
>                -> mmc_suspend()
>                        -> mmc_card_can_sleep()
>                        -> mmc_card_sleep()
>                        -> mmc_card_set_sleep()
>        -> mmc_power_off()
>
> mmc_resume_host()
>        -> mmc_power_up()
>                -> This sets the bus_width=1, timing=legacy and clock=f_init
>        -> host->bus_ops->resume()
>                -> mmc_resume()
>                        -> mmc_card_is_sleep() -> this would return true
> because card sleep flag is set.
>                        -> mmc_card_awake()
>
> Block read/write operation:
>        -> mmc_blk_issue_rw_rq
>                -> send CMD25
>                        -> But at this point, bus_width=1, timing=legacy and
> clock=f_init. Which is causing card not to respond back to data blocks sent
> from card and we are seeing the data timeout errors.
>
> So basically to fix this, in mmc_resume() either you have to call
> mmc_init_card() instead of mmc_card_awake() or if we still want to just call
> the mmc_card_awake() from resume, we need to save the state of host->ios
> before the sleep command issue so during resume, before calling mmc_awake(),
> we need to restore the saved host->ios state back and call mmc_set_ios() to
> restore the original parameters.

Since the idea was not to go through a costly mmc_init_card during
resume, we will do the second approach, i.e. we will save "ios" into a
new mmc_host member "saved_ios" in mmc_power_off and restore this
"saved_ios" in mmc_power_up, if mmc_card_is_sleep is TRUE.

>
> Also, I couple of other concerns here: In mmc_suspend_host(), after we put
> the card into suspend (host->bus_ops_suspend()), we call mmc_power_off()
> which asks the host controller to switch off the eMMC power. Now assume that
> a particular host controller driver turns off both VCC and VCCQ rails when
> ios.power_mode = MMC_POWER_OFF.
> Now in mmc_resume_host(), we call mmc_power_up() which would switch ON the
> power back which would mean eMMC card have seen power down and power up
> sequence and that means card is no longer in sleep state. And now after
> mmc_power_up(), when we call the mmc_card_awake() from mmc_resume(), card
> may not even respond to CMD5 awake. So in this scenario, we should actually
> call mmc_init_card() to bring the card back to proper state.

For this case, where host driver turns off both VCC and VCCQ, we added
the cap MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND. This should handle the
case  you mentioned above, i.e. call mmc_init_card from mmc_resume

>
> Regards,
> Subhash
>
>>       } else
>>               err = mmc_init_card(host, host->ocr, host->card);
>>       mmc_release_host(host);
>> @@ -1407,6 +1445,7 @@ static const struct mmc_bus_ops mmc_ops = {
>>       .resume = NULL,
>>       .power_restore = mmc_power_restore,
>>       .alive = mmc_alive,
>> +     .poweroff_notify = mmc_poweroff_notify,
>>  };
>>
>>  static const struct mmc_bus_ops mmc_ops_unsafe = { @@ -1418,6 +1457,7
>> @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>>       .resume = mmc_resume,
>>       .power_restore = mmc_power_restore,
>>       .alive = mmc_alive,
>> +     .poweroff_notify = mmc_poweroff_notify,
>>  };
>>
>>  static void mmc_attach_bus_ops(struct mmc_host *host) diff --git
>> a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index
>> 1532357..463130f 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1788,11 +1788,6 @@ static int __init dw_mci_init_slot(struct dw_mci
>> *host, unsigned int id)
>>       if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
>>               mmc->caps |= MMC_CAP_SD_HIGHSPEED |
>> MMC_CAP_MMC_HIGHSPEED;
>>
>> -     if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
>> -             mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>> -     else
>> -             mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
>> -
>>       if (host->pdata->blk_settings) {
>>               mmc->max_segs = host->pdata->blk_settings->max_segs;
>>               mmc->max_blk_size = host->pdata->blk_settings-
>> >max_blk_size;
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
>> e626732..c0a5a91 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2812,15 +2812,6 @@ int sdhci_add_host(struct sdhci_host *host)
>>       if (caps[1] & SDHCI_DRIVER_TYPE_D)
>>               mmc->caps |= MMC_CAP_DRIVER_TYPE_D;
>>
>> -     /*
>> -      * If Power Off Notify capability is enabled by the host,
>> -      * set notify to short power off notify timeout value.
>> -      */
>> -     if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
>> -             mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>> -     else
>> -             mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
>> -
>>       /* Initial value for re-tuning timer count */
>>       host->tuning_count = (caps[1] &
>> SDHCI_RETUNING_TIMER_COUNT_MASK) >>
>>                             SDHCI_RETUNING_TIMER_COUNT_SHIFT; diff --git
>> a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
>> d76513b..040eec4 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -239,11 +239,10 @@ struct mmc_card {
>>  #define MMC_QUIRK_BROKEN_BYTE_MODE_512 (1<<8)        /* Avoid
>> sending 512 bytes in */
>>  #define MMC_QUIRK_LONG_READ_TIME (1<<9)              /* Data read
>> time > CSD says */
>>                                               /* byte mode */
>> -     unsigned int    poweroff_notify_state;  /* eMMC4.5 notify feature */
>> +     unsigned int            poweroff_notify_state; /* MMC-4.5 poweroff
>> +                                                     notify feature */
>>  #define MMC_NO_POWER_NOTIFICATION    0
>>  #define MMC_POWERED_ON                       1
>> -#define MMC_POWEROFF_SHORT           2
>> -#define MMC_POWEROFF_LONG            3
>>
>>       unsigned int            erase_size;     /* erase size in sectors */
>>       unsigned int            erase_shift;    /* if erase unit is power 2
> */
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index
>> 1b431c7..54894d6 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -161,6 +161,7 @@ extern int mmc_can_trim(struct mmc_card *card);
>> extern int mmc_can_discard(struct mmc_card *card);  extern int
>> mmc_can_sanitize(struct mmc_card *card);  extern int
>> mmc_can_secure_erase_trim(struct mmc_card *card);
>> +extern int mmc_can_poweroff_notify(const struct mmc_card *card);
>>  extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int
>> from,
>>                                  unsigned int nr);
>>  extern unsigned int mmc_calc_max_discard(struct mmc_card *card); diff
> --git
>> a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
>> 0707d22..0e9adac 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -238,12 +238,9 @@ 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;
>> -#define MMC_HOST_PW_NOTIFY_NONE              0
>> -#define MMC_HOST_PW_NOTIFY_SHORT     1
>> -#define MMC_HOST_PW_NOTIFY_LONG              2
>>
>>  #ifdef CONFIG_MMC_CLKGATE
>>       int                     clk_requests;   /* internal reference
> counter
>> */
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index
>> d425cab..b11876b 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -386,4 +386,11 @@ struct _mmc_csd {
>>  #define MMC_SWITCH_MODE_CLEAR_BITS   0x02    /* Clear bits which are
>> 1 in value */
>>  #define MMC_SWITCH_MODE_WRITE_BYTE   0x03    /* Set target to value
>> */
>>
>> +/*
>> + * MMC Poweroff Notify types
>> + */
>> +#define MMC_PW_OFF_NOTIFY_NONE               0
>> +#define MMC_PW_OFF_NOTIFY_SHORT              1
>> +#define MMC_PW_OFF_NOTIFY_LONG               2
>> +
>>  #endif /* LINUX_MMC_MMC_H */
>> --
>> 1.7.4.1
>
>

Girish K S May 29, 2012, 3:36 a.m. UTC | #5

On 28 May 2012 16:33, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> Hi Girish, Saugata,
>
> There is an issue with this patch during resume. Please find comments inline
> below:
This is a patch for poweroff_notification. If CAP2 is enabled with
CAP2_POWEROFF_NOTIFY, you never enter the sleep path.
>
>> -----Original Message-----
>> From: Girish K S [mailto:girish.shivananjappa@linaro.org]
>> Sent: Tuesday, May 22, 2012 5:15 PM
>> To: linux-mmc@vger.kernel.org
>> Cc: cjb@laptop.org; patches@linaro.org; ulf.hansson@stericsson.com;
>> saugata.das@linaro.org; subhashj@codeaurora.org; Girish K S
>> Subject: [PATCH v5] MMC-4.5 Power OFF Notify Rework
>>
>> From: Saugata Das <saugata.das@linaro.org>
>>
>> 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).
>>
>> This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent
>> from the suspend sequence. If it is sent from shutdown sequence then it is
> set
>> to POWER_OFF_LONG.
>>
>> Earlier implementation of PowerOff Notify as a core function is replaced
> as a
>> device's bus operation.
>>
>> Signed-off-by: Saugata Das <saugata.das@linaro.org>
>> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org>
>>
>> changes in v5:
>>       modified the the handling of return value in mmc_poweroff_notify.
>> changes in v4:
>>       As suggested in review,
>>       - Moved mmc_can_poweroff_notify to core.c
>>       - Moved mmc_claim_host, mmc_release_host outside
>> mmc_poweroff_notify
>>       - Added check for wrong initialization for poweroff_notify_type
>>       - mmc_poweroff_notify is modified to take as 2nd parameter changes
>> in v3:
>>       This version addresses the review comments given by Subhash and Ulf
>> changes in v2:
>>       This version addresses the changes suggested by Ulf
>> ---
>>  drivers/mmc/core/core.c   |  108
> ++++++++++++++++++--------------------------
>>  drivers/mmc/core/core.h   |    1 +
>>  drivers/mmc/core/mmc.c    |   56 ++++++++++++++++++++---
>>  drivers/mmc/host/dw_mmc.c |    5 --
>>  drivers/mmc/host/sdhci.c  |    9 ----
>>  include/linux/mmc/card.h  |    5 +-
>>  include/linux/mmc/core.h  |    1 +
>>  include/linux/mmc/host.h  |    5 +--
>>  include/linux/mmc/mmc.h   |    7 +++
>>  9 files changed, 104 insertions(+), 93 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
>> 0b6141d..fe616b9 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1101,48 +1101,6 @@ 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) -{
>> -     struct mmc_card *card;
>> -     unsigned int timeout;
>> -     unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>> -     int err = 0;
>> -
>> -     card = host->card;
>> -     mmc_claim_host(host);
>> -
>> -     /*
>> -      * Send power notify command only if card
>> -      * is mmc and notify state is powered ON
>> -      */
>> -     if (card && mmc_card_mmc(card) &&
>> -         (card->poweroff_notify_state == MMC_POWERED_ON)) {
>> -
>> -             if (host->power_notify_type ==
>> MMC_HOST_PW_NOTIFY_SHORT) {
>> -                     notify_type = EXT_CSD_POWER_OFF_SHORT;
>> -                     timeout = card->ext_csd.generic_cmd6_time;
>> -                     card->poweroff_notify_state =
>> MMC_POWEROFF_SHORT;
>> -             } else {
>> -                     notify_type = EXT_CSD_POWER_OFF_LONG;
>> -                     timeout = card->ext_csd.power_off_longtime;
>> -                     card->poweroff_notify_state =
>> MMC_POWEROFF_LONG;
>> -             }
>> -
>> -             err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> -                              EXT_CSD_POWER_OFF_NOTIFICATION,
>> -                              notify_type, timeout);
>> -
>> -             if (err && err != -EBADMSG)
>> -                     pr_err("Device failed to respond within %d poweroff
> "
>> -                            "time. Forcefully powering down the
> device\n",
>> -                            timeout);
>> -
>> -             /* Set the card state to no notification after the poweroff
> */
>> -             card->poweroff_notify_state =
>> MMC_NO_POWER_NOTIFICATION;
>> -     }
>> -     mmc_release_host(host);
>> -}
>> -
>>  /*
>>   * Apply power to the MMC stack.  This is a two-stage process.
>>   * First, we enable power to the card without the clock running.
>> @@ -1202,8 +1160,6 @@ static void mmc_power_up(struct mmc_host *host)
>>
>>  void mmc_power_off(struct mmc_host *host)  {
>> -     int err = 0;
>> -
>>       if (host->ios.power_mode == MMC_POWER_OFF)
>>               return;
>>
>> @@ -1212,22 +1168,6 @@ void mmc_power_off(struct mmc_host *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
> @@
>> -1726,6 +1666,15 @@ int mmc_can_secure_erase_trim(struct mmc_card
>> *card)  }  EXPORT_SYMBOL(mmc_can_secure_erase_trim);
>>
>> +int mmc_can_poweroff_notify(const struct mmc_card *card) {
>> +     return card &&
>> +             mmc_card_mmc(card) &&
>> +             card->host->bus_ops->poweroff_notify &&
>> +             (card->poweroff_notify_state == MMC_POWERED_ON); }
>> +EXPORT_SYMBOL(mmc_can_poweroff_notify);
>> +
>>  int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>>                           unsigned int nr)
>>  {
>> @@ -2096,6 +2045,15 @@ void mmc_stop_host(struct mmc_host *host)
>>
>>       mmc_bus_get(host);
>>       if (host->bus_ops && !host->bus_dead) {
>> +             mmc_claim_host(host);
>> +             if (mmc_can_poweroff_notify(host->card)) {
>> +                     int err = host->bus_ops->poweroff_notify(host,
>> +
>>       MMC_PW_OFF_NOTIFY_LONG);
>> +                     if (err)
>> +                             pr_info("%s: error [%d] in poweroff
> notify\n",
>> +                                     mmc_hostname(host), err);
>> +             }
>> +             mmc_release_host(host);
>>               /* Calling bus_ops->remove() with a claimed host can
> deadlock
>> */
>>               if (host->bus_ops->remove)
>>                       host->bus_ops->remove(host);
>> @@ -2131,6 +2089,15 @@ int mmc_power_save_host(struct mmc_host *host)
>>
>>       if (host->bus_ops->power_save)
>>               ret = host->bus_ops->power_save(host);
>> +     mmc_claim_host(host);
>> +     if (mmc_can_poweroff_notify(host->card)) {
>> +             int err = host->bus_ops->poweroff_notify(host,
>> +                                     MMC_PW_OFF_NOTIFY_SHORT);
>> +             if (err)
>> +                     pr_info("%s: error [%d] in poweroff notify\n",
>> +                             mmc_hostname(host), err);
>> +     }
>> +     mmc_release_host(host);
>>
>>       mmc_bus_put(host);
>>
>> @@ -2173,8 +2140,11 @@ int mmc_card_awake(struct mmc_host *host)
>>
>>       mmc_bus_get(host);
>>
>> -     if (host->bus_ops && !host->bus_dead && host->bus_ops->awake)
>> +     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);
>> +     }
>>
>>       mmc_bus_put(host);
>>
>> @@ -2191,8 +2161,11 @@ int mmc_card_sleep(struct mmc_host *host)
>>
>>       mmc_bus_get(host);
>>
>> -     if (host->bus_ops && !host->bus_dead && host->bus_ops->sleep)
>> +     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);
>> +     }
>>
>>       mmc_bus_put(host);
>>
>> @@ -2385,12 +2358,20 @@ int mmc_pm_notify(struct notifier_block
>> *notify_block,
>>
>>               spin_lock_irqsave(&host->lock, flags);
>>               host->rescan_disable = 1;
>> -             host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>>               spin_unlock_irqrestore(&host->lock, flags);
>>               cancel_delayed_work_sync(&host->detect);
>>
>>               if (!host->bus_ops || host->bus_ops->suspend)
>>                       break;
>> +             mmc_claim_host(host);
>> +             if (mmc_can_poweroff_notify(host->card)) {
>> +                     int err = host->bus_ops->poweroff_notify(host,
>> +
>>       MMC_PW_OFF_NOTIFY_SHORT);
>> +                     if (err)
>> +                             pr_info("%s: error [%d] in poweroff
> notify\n",
>> +                                     mmc_hostname(host), err);
>> +             }
>> +             mmc_release_host(host);
>>
>>               /* Calling bus_ops->remove() with a claimed host can
> deadlock
>> */
>>               if (host->bus_ops->remove)
>> @@ -2409,7 +2390,6 @@ int mmc_pm_notify(struct notifier_block
>> *notify_block,
>>
>>               spin_lock_irqsave(&host->lock, flags);
>>               host->rescan_disable = 0;
>> -             host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;
>>               spin_unlock_irqrestore(&host->lock, flags);
>>               mmc_detect_change(host, 0);
>>
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index
>> 3bdafbc..15b918d 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -25,6 +25,7 @@ struct mmc_bus_ops {
>>       int (*power_save)(struct mmc_host *);
>>       int (*power_restore)(struct mmc_host *);
>>       int (*alive)(struct mmc_host *);
>> +     int (*poweroff_notify)(struct mmc_host *, int notify);
>>  };
>>
>>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops
>> *ops); diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
>> 2f0e11c..042dbdc 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1259,6 +1259,40 @@ err:
>>       return err;
>>  }
>>
>> +static int mmc_poweroff_notify(struct mmc_host *host, int notify) {
>> +     struct mmc_card *card;
>> +     unsigned int timeout;
>> +     unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>> +     int err;
>> +
>> +     card = host->card;
>> +
>> +     if (notify == MMC_PW_OFF_NOTIFY_SHORT) {
>> +             notify_type = EXT_CSD_POWER_OFF_SHORT;
>> +             timeout = card->ext_csd.generic_cmd6_time;
>> +     } else if (notify == MMC_PW_OFF_NOTIFY_LONG) {
>> +             notify_type = EXT_CSD_POWER_OFF_LONG;
>> +             timeout = card->ext_csd.power_off_longtime;
>> +     } else {
>> +             pr_info("%s: mmc_poweroff_notify called "
>> +                     "with notify type %d\n", mmc_hostname(host),
> notify);
>> +             return -EINVAL;
>> +     }
>> +
>> +     err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> +                      EXT_CSD_POWER_OFF_NOTIFICATION,
>> +                      notify_type, timeout);
>> +
>> +     if (err)
>> +             pr_err("%s: Device failed to respond within %d "
>> +                    "poweroff timeout.\n", mmc_hostname(host), timeout);
>> +     else
>> +             card->poweroff_notify_state =
>> +                                     MMC_NO_POWER_NOTIFICATION;
>> +
>> +     return err;
>> +}
>>  /*
>>   * Host is being removed. Free up the current card.
>>   */
>> @@ -1319,13 +1353,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 (!err)
>> -                     mmc_card_set_sleep(host->card);
>> -     } else if (!mmc_host_is_spi(host))
>> -             mmc_deselect_cards(host);
>> -     host->card->state &= ~(MMC_STATE_HIGHSPEED |
>> MMC_STATE_HIGHSPEED_200);
>> +     if (mmc_can_poweroff_notify(host->card) &&
>> +             (host->caps2 &
>> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>> +             err = mmc_poweroff_notify(host,
>> MMC_PW_OFF_NOTIFY_SHORT);
>> +     } else {
>> +             if (mmc_card_can_sleep(host))
>> +                     err = mmc_card_sleep(host);
>> +             else if (!mmc_host_is_spi(host))
>> +                     mmc_deselect_cards(host);
>> +     }
>> +     if (!err)
>> +             host->card->state &=
>> +                     ~(MMC_STATE_HIGHSPEED |
>> MMC_STATE_HIGHSPEED_200);
>>       mmc_release_host(host);
>>
>>       return err;
>> @@ -1347,7 +1386,6 @@ static int mmc_resume(struct mmc_host *host)
>>       mmc_claim_host(host);
>>       if (mmc_card_is_sleep(host->card)) {
>>               err = mmc_card_awake(host);
>> -             mmc_card_clr_sleep(host->card);
>
> Without this patch, this was the suspend sequence/resume sequence for MMC
> cards:
Actually mmc_resume should not be invoked from the poweroff function
(if you have seen earlier discussions with Ulf). so the poweroff
function is reverted to the previous state.
Also we decided to implement the sleep/awake if the card supports. If
card doesnt   support then mmc_init_card will be called.
> mmc_suspend_host()
>        -> host->bus_ops->suspend()
>                -> mmc_suspend()
>                        -> mmc_card_can_sleep()
>                        -> mmc_card_sleep()
>                        -> mmc_card_set_sleep()
>        -> mmc_power_off()
>                -> mmc_card_is_sleep(host->card)
>                -> host->bus_ops->resume()
>                        -> mmc_resume()
>                                -> mmc_card_is_sleep()
>                                -> mmc_card_awake()
>                                -> mmc_card_clr_sleep()
>
>
> mmc_resume_host()
>        -> mmc_power_up()
>                -> This sets the buswidth=1, timing=legacy and clock=f_init
>        -> host->bus_ops->resume()
>                -> mmc_resume()
>                        -> mmc_card_is_sleep() -> this would return false
> because in mmc_suspend_host(), host->bus_ops->resume() call had cleared the
> sleep flag.
>                -> mmc_init_card() -> As card sleep state is cleared,
> init_card() will be done which sets up the card again.
>
>
> But with this patch, you have removed the "host->bus_ops->resume()" call
> from mmc_power_off() which would mean card sleep state would be set to
> MMC_STATE_SLEEP during mmc_resume_host().
> Following is the sequence with this patch:
>
> mmc_suspend_host()
>        -> host->bus_ops->suspend()
>                -> mmc_suspend()
>                        -> mmc_card_can_sleep()
>                        -> mmc_card_sleep()
>                        -> mmc_card_set_sleep()
>        -> mmc_power_off()
>
> mmc_resume_host()
>        -> mmc_power_up()
>                -> This sets the bus_width=1, timing=legacy and clock=f_init
>        -> host->bus_ops->resume()
>                -> mmc_resume()
>                        -> mmc_card_is_sleep() -> this would return true
> because card sleep flag is set.
>                        -> mmc_card_awake()
>
> Block read/write operation:
>        -> mmc_blk_issue_rw_rq
>                -> send CMD25
>                        -> But at this point, bus_width=1, timing=legacy and
> clock=f_init. Which is causing card not to respond back to data blocks sent
> from card and we are seeing the data timeout errors.
>
> So basically to fix this, in mmc_resume() either you have to call
> mmc_init_card() instead of mmc_card_awake() or if we still want to just call
> the mmc_card_awake() from resume, we need to save the state of host->ios
> before the sleep command issue so during resume, before calling mmc_awake(),
> we need to restore the saved host->ios state back and call mmc_set_ios() to
> restore the original parameters.
Agreed this is missing. will save the ios context, as it is much
faster than re init.
>
> Also, I couple of other concerns here: In mmc_suspend_host(), after we put
> the card into suspend (host->bus_ops_suspend()), we call mmc_power_off()
> which asks the host controller to switch off the eMMC power. Now assume that
> a particular host controller driver turns off both VCC and VCCQ rails when
> ios.power_mode = MMC_POWER_OFF.
> Now in mmc_resume_host(), we call mmc_power_up() which would switch ON the
> power back which would mean eMMC card have seen power down and power up
> sequence and that means card is no longer in sleep state. And now after
> mmc_power_up(), when we call the mmc_card_awake() from mmc_resume(), card
> may not even respond to CMD5 awake. So in this scenario, we should actually
> call mmc_init_card() to bring the card back to proper state.
>
> Regards,
> Subhash
>
>>       } else
>>               err = mmc_init_card(host, host->ocr, host->card);
>>       mmc_release_host(host);
>> @@ -1407,6 +1445,7 @@ static const struct mmc_bus_ops mmc_ops = {
>>       .resume = NULL,
>>       .power_restore = mmc_power_restore,
>>       .alive = mmc_alive,
>> +     .poweroff_notify = mmc_poweroff_notify,
>>  };
>>
>>  static const struct mmc_bus_ops mmc_ops_unsafe = { @@ -1418,6 +1457,7
>> @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>>       .resume = mmc_resume,
>>       .power_restore = mmc_power_restore,
>>       .alive = mmc_alive,
>> +     .poweroff_notify = mmc_poweroff_notify,
>>  };
>>
>>  static void mmc_attach_bus_ops(struct mmc_host *host) diff --git
>> a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index
>> 1532357..463130f 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1788,11 +1788,6 @@ static int __init dw_mci_init_slot(struct dw_mci
>> *host, unsigned int id)
>>       if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
>>               mmc->caps |= MMC_CAP_SD_HIGHSPEED |
>> MMC_CAP_MMC_HIGHSPEED;
>>
>> -     if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
>> -             mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>> -     else
>> -             mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
>> -
>>       if (host->pdata->blk_settings) {
>>               mmc->max_segs = host->pdata->blk_settings->max_segs;
>>               mmc->max_blk_size = host->pdata->blk_settings-
>> >max_blk_size;
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
>> e626732..c0a5a91 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2812,15 +2812,6 @@ int sdhci_add_host(struct sdhci_host *host)
>>       if (caps[1] & SDHCI_DRIVER_TYPE_D)
>>               mmc->caps |= MMC_CAP_DRIVER_TYPE_D;
>>
>> -     /*
>> -      * If Power Off Notify capability is enabled by the host,
>> -      * set notify to short power off notify timeout value.
>> -      */
>> -     if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
>> -             mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>> -     else
>> -             mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
>> -
>>       /* Initial value for re-tuning timer count */
>>       host->tuning_count = (caps[1] &
>> SDHCI_RETUNING_TIMER_COUNT_MASK) >>
>>                             SDHCI_RETUNING_TIMER_COUNT_SHIFT; diff --git
>> a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
>> d76513b..040eec4 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -239,11 +239,10 @@ struct mmc_card {
>>  #define MMC_QUIRK_BROKEN_BYTE_MODE_512 (1<<8)        /* Avoid
>> sending 512 bytes in */
>>  #define MMC_QUIRK_LONG_READ_TIME (1<<9)              /* Data read
>> time > CSD says */
>>                                               /* byte mode */
>> -     unsigned int    poweroff_notify_state;  /* eMMC4.5 notify feature */
>> +     unsigned int            poweroff_notify_state; /* MMC-4.5 poweroff
>> +                                                     notify feature */
>>  #define MMC_NO_POWER_NOTIFICATION    0
>>  #define MMC_POWERED_ON                       1
>> -#define MMC_POWEROFF_SHORT           2
>> -#define MMC_POWEROFF_LONG            3
>>
>>       unsigned int            erase_size;     /* erase size in sectors */
>>       unsigned int            erase_shift;    /* if erase unit is power 2
> */
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index
>> 1b431c7..54894d6 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -161,6 +161,7 @@ extern int mmc_can_trim(struct mmc_card *card);
>> extern int mmc_can_discard(struct mmc_card *card);  extern int
>> mmc_can_sanitize(struct mmc_card *card);  extern int
>> mmc_can_secure_erase_trim(struct mmc_card *card);
>> +extern int mmc_can_poweroff_notify(const struct mmc_card *card);
>>  extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int
>> from,
>>                                  unsigned int nr);
>>  extern unsigned int mmc_calc_max_discard(struct mmc_card *card); diff
> --git
>> a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
>> 0707d22..0e9adac 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -238,12 +238,9 @@ 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;
>> -#define MMC_HOST_PW_NOTIFY_NONE              0
>> -#define MMC_HOST_PW_NOTIFY_SHORT     1
>> -#define MMC_HOST_PW_NOTIFY_LONG              2
>>
>>  #ifdef CONFIG_MMC_CLKGATE
>>       int                     clk_requests;   /* internal reference
> counter
>> */
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index
>> d425cab..b11876b 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -386,4 +386,11 @@ struct _mmc_csd {
>>  #define MMC_SWITCH_MODE_CLEAR_BITS   0x02    /* Clear bits which are
>> 1 in value */
>>  #define MMC_SWITCH_MODE_WRITE_BYTE   0x03    /* Set target to value
>> */
>>
>> +/*
>> + * MMC Poweroff Notify types
>> + */
>> +#define MMC_PW_OFF_NOTIFY_NONE               0
>> +#define MMC_PW_OFF_NOTIFY_SHORT              1
>> +#define MMC_PW_OFF_NOTIFY_LONG               2
>> +
>>  #endif /* LINUX_MMC_MMC_H */
>> --
>> 1.7.4.1
>
>

Per Forlin June 14, 2012, 1:13 p.m. UTC | #6

Hi Girish and Suagata,

I have run some regression tests with this patch on our board (ux500
family) running suspend and resume of the eMMC 4.41 device.

The two patches I have looked at are:
1. "mmc: core: Fix PowerOff Notify suspend/resume" (merged)
2. " MMC-4.5 Power OFF Notify Rework"

With only patch #1 the eMMC doesn't power up after in resume() after
being suspended. The eMMC doesn't respond at all after suspend. It's
not powered up.
Running tests with #1 and #2, the card is powered up but it doesn't
wake up after CMD5. Commands that arrive are after resume/CMD5
timeouts. I even tried by increasing the awake timeout to 5 seconds
but i didn't help.

The eMMC on my board successfully suspends and resumes with patch #1
and #2 if waking up the card using CMD0 (mmc_card_init()) instead of
CMD5.

Have anyone else seen the same issue?
Have this patch been verified on a board together with eMMC 4.41 that
supports card power off.

BR
Per

On Tue, May 22, 2012 at 1:45 PM, Girish K S
<girish.shivananjappa@linaro.org> wrote:
> From: Saugata Das <saugata.das@linaro.org>
>
> 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).
>
> This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent from
> the suspend sequence. If it is sent from shutdown sequence then it is set to
> POWER_OFF_LONG.
>
> Earlier implementation of PowerOff Notify as a core function is replaced as
> a device's bus operation.
>
> Signed-off-by: Saugata Das <saugata.das@linaro.org>
> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org>
>
> changes in v5:
>        modified the the handling of return value in mmc_poweroff_notify.
> changes in v4:
>        As suggested in review,
>        - Moved mmc_can_poweroff_notify to core.c
>        - Moved mmc_claim_host, mmc_release_host outside mmc_poweroff_notify
>        - Added check for wrong initialization for poweroff_notify_type
>        - mmc_poweroff_notify is modified to take as 2nd parameter
> changes in v3:
>        This version addresses the review comments given by Subhash and Ulf
> changes in v2:
>        This version addresses the changes suggested by Ulf
> ---
>  drivers/mmc/core/core.c   |  108 ++++++++++++++++++--------------------------
>  drivers/mmc/core/core.h   |    1 +
>  drivers/mmc/core/mmc.c    |   56 ++++++++++++++++++++---
>  drivers/mmc/host/dw_mmc.c |    5 --
>  drivers/mmc/host/sdhci.c  |    9 ----
>  include/linux/mmc/card.h  |    5 +-
>  include/linux/mmc/core.h  |    1 +
>  include/linux/mmc/host.h  |    5 +--
>  include/linux/mmc/mmc.h   |    7 +++
>  9 files changed, 104 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 0b6141d..fe616b9 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1101,48 +1101,6 @@ 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)
> -{
> -       struct mmc_card *card;
> -       unsigned int timeout;
> -       unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
> -       int err = 0;
> -
> -       card = host->card;
> -       mmc_claim_host(host);
> -
> -       /*
> -        * Send power notify command only if card
> -        * is mmc and notify state is powered ON
> -        */
> -       if (card && mmc_card_mmc(card) &&
> -           (card->poweroff_notify_state == MMC_POWERED_ON)) {
> -
> -               if (host->power_notify_type == MMC_HOST_PW_NOTIFY_SHORT) {
> -                       notify_type = EXT_CSD_POWER_OFF_SHORT;
> -                       timeout = card->ext_csd.generic_cmd6_time;
> -                       card->poweroff_notify_state = MMC_POWEROFF_SHORT;
> -               } else {
> -                       notify_type = EXT_CSD_POWER_OFF_LONG;
> -                       timeout = card->ext_csd.power_off_longtime;
> -                       card->poweroff_notify_state = MMC_POWEROFF_LONG;
> -               }
> -
> -               err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> -                                EXT_CSD_POWER_OFF_NOTIFICATION,
> -                                notify_type, timeout);
> -
> -               if (err && err != -EBADMSG)
> -                       pr_err("Device failed to respond within %d poweroff "
> -                              "time. Forcefully powering down the device\n",
> -                              timeout);
> -
> -               /* Set the card state to no notification after the poweroff */
> -               card->poweroff_notify_state = MMC_NO_POWER_NOTIFICATION;
> -       }
> -       mmc_release_host(host);
> -}
> -
>  /*
>  * Apply power to the MMC stack.  This is a two-stage process.
>  * First, we enable power to the card without the clock running.
> @@ -1202,8 +1160,6 @@ static void mmc_power_up(struct mmc_host *host)
>
>  void mmc_power_off(struct mmc_host *host)
>  {
> -       int err = 0;
> -
>        if (host->ios.power_mode == MMC_POWER_OFF)
>                return;
>
> @@ -1212,22 +1168,6 @@ void mmc_power_off(struct mmc_host *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
> @@ -1726,6 +1666,15 @@ int mmc_can_secure_erase_trim(struct mmc_card *card)
>  }
>  EXPORT_SYMBOL(mmc_can_secure_erase_trim);
>
> +int mmc_can_poweroff_notify(const struct mmc_card *card)
> +{
> +       return card &&
> +               mmc_card_mmc(card) &&
> +               card->host->bus_ops->poweroff_notify &&
> +               (card->poweroff_notify_state == MMC_POWERED_ON);
> +}
> +EXPORT_SYMBOL(mmc_can_poweroff_notify);
> +
>  int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>                            unsigned int nr)
>  {
> @@ -2096,6 +2045,15 @@ void mmc_stop_host(struct mmc_host *host)
>
>        mmc_bus_get(host);
>        if (host->bus_ops && !host->bus_dead) {
> +               mmc_claim_host(host);
> +               if (mmc_can_poweroff_notify(host->card)) {
> +                       int err = host->bus_ops->poweroff_notify(host,
> +                                               MMC_PW_OFF_NOTIFY_LONG);
> +                       if (err)
> +                               pr_info("%s: error [%d] in poweroff notify\n",
> +                                       mmc_hostname(host), err);
> +               }
> +               mmc_release_host(host);
>                /* Calling bus_ops->remove() with a claimed host can deadlock */
>                if (host->bus_ops->remove)
>                        host->bus_ops->remove(host);
> @@ -2131,6 +2089,15 @@ int mmc_power_save_host(struct mmc_host *host)
>
>        if (host->bus_ops->power_save)
>                ret = host->bus_ops->power_save(host);
> +       mmc_claim_host(host);
> +       if (mmc_can_poweroff_notify(host->card)) {
> +               int err = host->bus_ops->poweroff_notify(host,
> +                                       MMC_PW_OFF_NOTIFY_SHORT);
> +               if (err)
> +                       pr_info("%s: error [%d] in poweroff notify\n",
> +                               mmc_hostname(host), err);
> +       }
> +       mmc_release_host(host);
>
>        mmc_bus_put(host);
>
> @@ -2173,8 +2140,11 @@ int mmc_card_awake(struct mmc_host *host)
>
>        mmc_bus_get(host);
>
> -       if (host->bus_ops && !host->bus_dead && host->bus_ops->awake)
> +       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);
> +       }
>
>        mmc_bus_put(host);
>
> @@ -2191,8 +2161,11 @@ int mmc_card_sleep(struct mmc_host *host)
>
>        mmc_bus_get(host);
>
> -       if (host->bus_ops && !host->bus_dead && host->bus_ops->sleep)
> +       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);
> +       }
>
>        mmc_bus_put(host);
>
> @@ -2385,12 +2358,20 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>
>                spin_lock_irqsave(&host->lock, flags);
>                host->rescan_disable = 1;
> -               host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>                spin_unlock_irqrestore(&host->lock, flags);
>                cancel_delayed_work_sync(&host->detect);
>
>                if (!host->bus_ops || host->bus_ops->suspend)
>                        break;
> +               mmc_claim_host(host);
> +               if (mmc_can_poweroff_notify(host->card)) {
> +                       int err = host->bus_ops->poweroff_notify(host,
> +                                               MMC_PW_OFF_NOTIFY_SHORT);
> +                       if (err)
> +                               pr_info("%s: error [%d] in poweroff notify\n",
> +                                       mmc_hostname(host), err);
> +               }
> +               mmc_release_host(host);
>
>                /* Calling bus_ops->remove() with a claimed host can deadlock */
>                if (host->bus_ops->remove)
> @@ -2409,7 +2390,6 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>
>                spin_lock_irqsave(&host->lock, flags);
>                host->rescan_disable = 0;
> -               host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;
>                spin_unlock_irqrestore(&host->lock, flags);
>                mmc_detect_change(host, 0);
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 3bdafbc..15b918d 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -25,6 +25,7 @@ struct mmc_bus_ops {
>        int (*power_save)(struct mmc_host *);
>        int (*power_restore)(struct mmc_host *);
>        int (*alive)(struct mmc_host *);
> +       int (*poweroff_notify)(struct mmc_host *, int notify);
>  };
>
>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 2f0e11c..042dbdc 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1259,6 +1259,40 @@ err:
>        return err;
>  }
>
> +static int mmc_poweroff_notify(struct mmc_host *host, int notify)
> +{
> +       struct mmc_card *card;
> +       unsigned int timeout;
> +       unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
> +       int err;
> +
> +       card = host->card;
> +
> +       if (notify == MMC_PW_OFF_NOTIFY_SHORT) {
> +               notify_type = EXT_CSD_POWER_OFF_SHORT;
> +               timeout = card->ext_csd.generic_cmd6_time;
> +       } else if (notify == MMC_PW_OFF_NOTIFY_LONG) {
> +               notify_type = EXT_CSD_POWER_OFF_LONG;
> +               timeout = card->ext_csd.power_off_longtime;
> +       } else {
> +               pr_info("%s: mmc_poweroff_notify called "
> +                       "with notify type %d\n", mmc_hostname(host), notify);
> +               return -EINVAL;
> +       }
> +
> +       err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> +                        EXT_CSD_POWER_OFF_NOTIFICATION,
> +                        notify_type, timeout);
> +
> +       if (err)
> +               pr_err("%s: Device failed to respond within %d "
> +                      "poweroff timeout.\n", mmc_hostname(host), timeout);
> +       else
> +               card->poweroff_notify_state =
> +                                       MMC_NO_POWER_NOTIFICATION;
> +
> +       return err;
> +}
>  /*
>  * Host is being removed. Free up the current card.
>  */
> @@ -1319,13 +1353,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 (!err)
> -                       mmc_card_set_sleep(host->card);
> -       } else if (!mmc_host_is_spi(host))
> -               mmc_deselect_cards(host);
> -       host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
> +       if (mmc_can_poweroff_notify(host->card) &&
> +               (host->caps2 & MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
> +               err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
> +       } else {
> +               if (mmc_card_can_sleep(host))
> +                       err = mmc_card_sleep(host);
> +               else if (!mmc_host_is_spi(host))
> +                       mmc_deselect_cards(host);
> +       }
> +       if (!err)
> +               host->card->state &=
> +                       ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
>        mmc_release_host(host);
>
>        return err;
> @@ -1347,7 +1386,6 @@ static int mmc_resume(struct mmc_host *host)
>        mmc_claim_host(host);
>        if (mmc_card_is_sleep(host->card)) {
>                err = mmc_card_awake(host);
> -               mmc_card_clr_sleep(host->card);
>        } else
>                err = mmc_init_card(host, host->ocr, host->card);
>        mmc_release_host(host);
> @@ -1407,6 +1445,7 @@ static const struct mmc_bus_ops mmc_ops = {
>        .resume = NULL,
>        .power_restore = mmc_power_restore,
>        .alive = mmc_alive,
> +       .poweroff_notify = mmc_poweroff_notify,
>  };
>
>  static const struct mmc_bus_ops mmc_ops_unsafe = {
> @@ -1418,6 +1457,7 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>        .resume = mmc_resume,
>        .power_restore = mmc_power_restore,
>        .alive = mmc_alive,
> +       .poweroff_notify = mmc_poweroff_notify,
>  };
>
>  static void mmc_attach_bus_ops(struct mmc_host *host)
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 1532357..463130f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1788,11 +1788,6 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>        if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
>                mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>
> -       if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> -       else
> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
> -
>        if (host->pdata->blk_settings) {
>                mmc->max_segs = host->pdata->blk_settings->max_segs;
>                mmc->max_blk_size = host->pdata->blk_settings->max_blk_size;
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e626732..c0a5a91 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2812,15 +2812,6 @@ int sdhci_add_host(struct sdhci_host *host)
>        if (caps[1] & SDHCI_DRIVER_TYPE_D)
>                mmc->caps |= MMC_CAP_DRIVER_TYPE_D;
>
> -       /*
> -        * If Power Off Notify capability is enabled by the host,
> -        * set notify to short power off notify timeout value.
> -        */
> -       if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
> -       else
> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
> -
>        /* Initial value for re-tuning timer count */
>        host->tuning_count = (caps[1] & SDHCI_RETUNING_TIMER_COUNT_MASK) >>
>                              SDHCI_RETUNING_TIMER_COUNT_SHIFT;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index d76513b..040eec4 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -239,11 +239,10 @@ struct mmc_card {
>  #define MMC_QUIRK_BROKEN_BYTE_MODE_512 (1<<8)  /* Avoid sending 512 bytes in */
>  #define MMC_QUIRK_LONG_READ_TIME (1<<9)                /* Data read time > CSD says */
>                                                /* byte mode */
> -       unsigned int    poweroff_notify_state;  /* eMMC4.5 notify feature */
> +       unsigned int            poweroff_notify_state; /* MMC-4.5 poweroff
> +                                                       notify feature */
>  #define MMC_NO_POWER_NOTIFICATION      0
>  #define MMC_POWERED_ON                 1
> -#define MMC_POWEROFF_SHORT             2
> -#define MMC_POWEROFF_LONG              3
>
>        unsigned int            erase_size;     /* erase size in sectors */
>        unsigned int            erase_shift;    /* if erase unit is power 2 */
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 1b431c7..54894d6 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -161,6 +161,7 @@ extern int mmc_can_trim(struct mmc_card *card);
>  extern int mmc_can_discard(struct mmc_card *card);
>  extern int mmc_can_sanitize(struct mmc_card *card);
>  extern int mmc_can_secure_erase_trim(struct mmc_card *card);
> +extern int mmc_can_poweroff_notify(const struct mmc_card *card);
>  extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>                                   unsigned int nr);
>  extern unsigned int mmc_calc_max_discard(struct mmc_card *card);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0707d22..0e9adac 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -238,12 +238,9 @@ 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;
> -#define MMC_HOST_PW_NOTIFY_NONE                0
> -#define MMC_HOST_PW_NOTIFY_SHORT       1
> -#define MMC_HOST_PW_NOTIFY_LONG                2
>
>  #ifdef CONFIG_MMC_CLKGATE
>        int                     clk_requests;   /* internal reference counter */
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index d425cab..b11876b 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -386,4 +386,11 @@ struct _mmc_csd {
>  #define MMC_SWITCH_MODE_CLEAR_BITS     0x02    /* Clear bits which are 1 in value */
>  #define MMC_SWITCH_MODE_WRITE_BYTE     0x03    /* Set target to value */
>
> +/*
> + * MMC Poweroff Notify types
> + */
> +#define MMC_PW_OFF_NOTIFY_NONE         0
> +#define MMC_PW_OFF_NOTIFY_SHORT                1
> +#define MMC_PW_OFF_NOTIFY_LONG         2
> +
>  #endif /* LINUX_MMC_MMC_H */
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Girish K S June 14, 2012, 1:21 p.m. UTC | #7

On 14 June 2012 18:43, Per Forlin <per.lkml@gmail.com> wrote:
> Hi Girish and Suagata,
>
> I have run some regression tests with this patch on our board (ux500
> family) running suspend and resume of the eMMC 4.41 device.
>
> The two patches I have looked at are:
> 1. "mmc: core: Fix PowerOff Notify suspend/resume" (merged)
> 2. " MMC-4.5 Power OFF Notify Rework"
>
> With only patch #1 the eMMC doesn't power up after in resume() after
> being suspended. The eMMC doesn't respond at all after suspend. It's
> not powered up.
> Running tests with #1 and #2, the card is powered up but it doesn't
> wake up after CMD5. Commands that arrive are after resume/CMD5
> timeouts. I even tried by increasing the awake timeout to 5 seconds
> but i didn't help.
>
> The eMMC on my board successfully suspends and resumes with patch #1
> and #2 if waking up the card using CMD0 (mmc_card_init()) instead of
> CMD5.
>
> Have anyone else seen the same issue?
> Have this patch been verified on a board together with eMMC 4.41 that
> supports card power off.
This rework patch is still under progress. we are modifying it. In our
earlier discussions subhash has posted the
same issue and a solution for this.  we should save ios context before
sleep and restore ios before awake. soon rework patch will be
posted with the above recomenedded solution.

>
> BR
> Per
>
> On Tue, May 22, 2012 at 1:45 PM, Girish K S
> <girish.shivananjappa@linaro.org> wrote:
>> From: Saugata Das <saugata.das@linaro.org>
>>
>> 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).
>>
>> This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent from
>> the suspend sequence. If it is sent from shutdown sequence then it is set to
>> POWER_OFF_LONG.
>>
>> Earlier implementation of PowerOff Notify as a core function is replaced as
>> a device's bus operation.
>>
>> Signed-off-by: Saugata Das <saugata.das@linaro.org>
>> Signed-off-by: Girish K S <girish.shivananjappa@linaro.org>
>>
>> changes in v5:
>>        modified the the handling of return value in mmc_poweroff_notify.
>> changes in v4:
>>        As suggested in review,
>>        - Moved mmc_can_poweroff_notify to core.c
>>        - Moved mmc_claim_host, mmc_release_host outside mmc_poweroff_notify
>>        - Added check for wrong initialization for poweroff_notify_type
>>        - mmc_poweroff_notify is modified to take as 2nd parameter
>> changes in v3:
>>        This version addresses the review comments given by Subhash and Ulf
>> changes in v2:
>>        This version addresses the changes suggested by Ulf
>> ---
>>  drivers/mmc/core/core.c   |  108 ++++++++++++++++++--------------------------
>>  drivers/mmc/core/core.h   |    1 +
>>  drivers/mmc/core/mmc.c    |   56 ++++++++++++++++++++---
>>  drivers/mmc/host/dw_mmc.c |    5 --
>>  drivers/mmc/host/sdhci.c  |    9 ----
>>  include/linux/mmc/card.h  |    5 +-
>>  include/linux/mmc/core.h  |    1 +
>>  include/linux/mmc/host.h  |    5 +--
>>  include/linux/mmc/mmc.h   |    7 +++
>>  9 files changed, 104 insertions(+), 93 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 0b6141d..fe616b9 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1101,48 +1101,6 @@ 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)
>> -{
>> -       struct mmc_card *card;
>> -       unsigned int timeout;
>> -       unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>> -       int err = 0;
>> -
>> -       card = host->card;
>> -       mmc_claim_host(host);
>> -
>> -       /*
>> -        * Send power notify command only if card
>> -        * is mmc and notify state is powered ON
>> -        */
>> -       if (card && mmc_card_mmc(card) &&
>> -           (card->poweroff_notify_state == MMC_POWERED_ON)) {
>> -
>> -               if (host->power_notify_type == MMC_HOST_PW_NOTIFY_SHORT) {
>> -                       notify_type = EXT_CSD_POWER_OFF_SHORT;
>> -                       timeout = card->ext_csd.generic_cmd6_time;
>> -                       card->poweroff_notify_state = MMC_POWEROFF_SHORT;
>> -               } else {
>> -                       notify_type = EXT_CSD_POWER_OFF_LONG;
>> -                       timeout = card->ext_csd.power_off_longtime;
>> -                       card->poweroff_notify_state = MMC_POWEROFF_LONG;
>> -               }
>> -
>> -               err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> -                                EXT_CSD_POWER_OFF_NOTIFICATION,
>> -                                notify_type, timeout);
>> -
>> -               if (err && err != -EBADMSG)
>> -                       pr_err("Device failed to respond within %d poweroff "
>> -                              "time. Forcefully powering down the device\n",
>> -                              timeout);
>> -
>> -               /* Set the card state to no notification after the poweroff */
>> -               card->poweroff_notify_state = MMC_NO_POWER_NOTIFICATION;
>> -       }
>> -       mmc_release_host(host);
>> -}
>> -
>>  /*
>>  * Apply power to the MMC stack.  This is a two-stage process.
>>  * First, we enable power to the card without the clock running.
>> @@ -1202,8 +1160,6 @@ static void mmc_power_up(struct mmc_host *host)
>>
>>  void mmc_power_off(struct mmc_host *host)
>>  {
>> -       int err = 0;
>> -
>>        if (host->ios.power_mode == MMC_POWER_OFF)
>>                return;
>>
>> @@ -1212,22 +1168,6 @@ void mmc_power_off(struct mmc_host *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
>> @@ -1726,6 +1666,15 @@ int mmc_can_secure_erase_trim(struct mmc_card *card)
>>  }
>>  EXPORT_SYMBOL(mmc_can_secure_erase_trim);
>>
>> +int mmc_can_poweroff_notify(const struct mmc_card *card)
>> +{
>> +       return card &&
>> +               mmc_card_mmc(card) &&
>> +               card->host->bus_ops->poweroff_notify &&
>> +               (card->poweroff_notify_state == MMC_POWERED_ON);
>> +}
>> +EXPORT_SYMBOL(mmc_can_poweroff_notify);
>> +
>>  int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>>                            unsigned int nr)
>>  {
>> @@ -2096,6 +2045,15 @@ void mmc_stop_host(struct mmc_host *host)
>>
>>        mmc_bus_get(host);
>>        if (host->bus_ops && !host->bus_dead) {
>> +               mmc_claim_host(host);
>> +               if (mmc_can_poweroff_notify(host->card)) {
>> +                       int err = host->bus_ops->poweroff_notify(host,
>> +                                               MMC_PW_OFF_NOTIFY_LONG);
>> +                       if (err)
>> +                               pr_info("%s: error [%d] in poweroff notify\n",
>> +                                       mmc_hostname(host), err);
>> +               }
>> +               mmc_release_host(host);
>>                /* Calling bus_ops->remove() with a claimed host can deadlock */
>>                if (host->bus_ops->remove)
>>                        host->bus_ops->remove(host);
>> @@ -2131,6 +2089,15 @@ int mmc_power_save_host(struct mmc_host *host)
>>
>>        if (host->bus_ops->power_save)
>>                ret = host->bus_ops->power_save(host);
>> +       mmc_claim_host(host);
>> +       if (mmc_can_poweroff_notify(host->card)) {
>> +               int err = host->bus_ops->poweroff_notify(host,
>> +                                       MMC_PW_OFF_NOTIFY_SHORT);
>> +               if (err)
>> +                       pr_info("%s: error [%d] in poweroff notify\n",
>> +                               mmc_hostname(host), err);
>> +       }
>> +       mmc_release_host(host);
>>
>>        mmc_bus_put(host);
>>
>> @@ -2173,8 +2140,11 @@ int mmc_card_awake(struct mmc_host *host)
>>
>>        mmc_bus_get(host);
>>
>> -       if (host->bus_ops && !host->bus_dead && host->bus_ops->awake)
>> +       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);
>> +       }
>>
>>        mmc_bus_put(host);
>>
>> @@ -2191,8 +2161,11 @@ int mmc_card_sleep(struct mmc_host *host)
>>
>>        mmc_bus_get(host);
>>
>> -       if (host->bus_ops && !host->bus_dead && host->bus_ops->sleep)
>> +       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);
>> +       }
>>
>>        mmc_bus_put(host);
>>
>> @@ -2385,12 +2358,20 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>>
>>                spin_lock_irqsave(&host->lock, flags);
>>                host->rescan_disable = 1;
>> -               host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>>                spin_unlock_irqrestore(&host->lock, flags);
>>                cancel_delayed_work_sync(&host->detect);
>>
>>                if (!host->bus_ops || host->bus_ops->suspend)
>>                        break;
>> +               mmc_claim_host(host);
>> +               if (mmc_can_poweroff_notify(host->card)) {
>> +                       int err = host->bus_ops->poweroff_notify(host,
>> +                                               MMC_PW_OFF_NOTIFY_SHORT);
>> +                       if (err)
>> +                               pr_info("%s: error [%d] in poweroff notify\n",
>> +                                       mmc_hostname(host), err);
>> +               }
>> +               mmc_release_host(host);
>>
>>                /* Calling bus_ops->remove() with a claimed host can deadlock */
>>                if (host->bus_ops->remove)
>> @@ -2409,7 +2390,6 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>>
>>                spin_lock_irqsave(&host->lock, flags);
>>                host->rescan_disable = 0;
>> -               host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;
>>                spin_unlock_irqrestore(&host->lock, flags);
>>                mmc_detect_change(host, 0);
>>
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> index 3bdafbc..15b918d 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -25,6 +25,7 @@ struct mmc_bus_ops {
>>        int (*power_save)(struct mmc_host *);
>>        int (*power_restore)(struct mmc_host *);
>>        int (*alive)(struct mmc_host *);
>> +       int (*poweroff_notify)(struct mmc_host *, int notify);
>>  };
>>
>>  void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 2f0e11c..042dbdc 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1259,6 +1259,40 @@ err:
>>        return err;
>>  }
>>
>> +static int mmc_poweroff_notify(struct mmc_host *host, int notify)
>> +{
>> +       struct mmc_card *card;
>> +       unsigned int timeout;
>> +       unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>> +       int err;
>> +
>> +       card = host->card;
>> +
>> +       if (notify == MMC_PW_OFF_NOTIFY_SHORT) {
>> +               notify_type = EXT_CSD_POWER_OFF_SHORT;
>> +               timeout = card->ext_csd.generic_cmd6_time;
>> +       } else if (notify == MMC_PW_OFF_NOTIFY_LONG) {
>> +               notify_type = EXT_CSD_POWER_OFF_LONG;
>> +               timeout = card->ext_csd.power_off_longtime;
>> +       } else {
>> +               pr_info("%s: mmc_poweroff_notify called "
>> +                       "with notify type %d\n", mmc_hostname(host), notify);
>> +               return -EINVAL;
>> +       }
>> +
>> +       err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> +                        EXT_CSD_POWER_OFF_NOTIFICATION,
>> +                        notify_type, timeout);
>> +
>> +       if (err)
>> +               pr_err("%s: Device failed to respond within %d "
>> +                      "poweroff timeout.\n", mmc_hostname(host), timeout);
>> +       else
>> +               card->poweroff_notify_state =
>> +                                       MMC_NO_POWER_NOTIFICATION;
>> +
>> +       return err;
>> +}
>>  /*
>>  * Host is being removed. Free up the current card.
>>  */
>> @@ -1319,13 +1353,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 (!err)
>> -                       mmc_card_set_sleep(host->card);
>> -       } else if (!mmc_host_is_spi(host))
>> -               mmc_deselect_cards(host);
>> -       host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
>> +       if (mmc_can_poweroff_notify(host->card) &&
>> +               (host->caps2 & MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>> +               err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>> +       } else {
>> +               if (mmc_card_can_sleep(host))
>> +                       err = mmc_card_sleep(host);
>> +               else if (!mmc_host_is_spi(host))
>> +                       mmc_deselect_cards(host);
>> +       }
>> +       if (!err)
>> +               host->card->state &=
>> +                       ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
>>        mmc_release_host(host);
>>
>>        return err;
>> @@ -1347,7 +1386,6 @@ static int mmc_resume(struct mmc_host *host)
>>        mmc_claim_host(host);
>>        if (mmc_card_is_sleep(host->card)) {
>>                err = mmc_card_awake(host);
>> -               mmc_card_clr_sleep(host->card);
>>        } else
>>                err = mmc_init_card(host, host->ocr, host->card);
>>        mmc_release_host(host);
>> @@ -1407,6 +1445,7 @@ static const struct mmc_bus_ops mmc_ops = {
>>        .resume = NULL,
>>        .power_restore = mmc_power_restore,
>>        .alive = mmc_alive,
>> +       .poweroff_notify = mmc_poweroff_notify,
>>  };
>>
>>  static const struct mmc_bus_ops mmc_ops_unsafe = {
>> @@ -1418,6 +1457,7 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>>        .resume = mmc_resume,
>>        .power_restore = mmc_power_restore,
>>        .alive = mmc_alive,
>> +       .poweroff_notify = mmc_poweroff_notify,
>>  };
>>
>>  static void mmc_attach_bus_ops(struct mmc_host *host)
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 1532357..463130f 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1788,11 +1788,6 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>>        if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
>>                mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>>
>> -       if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
>> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>> -       else
>> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
>> -
>>        if (host->pdata->blk_settings) {
>>                mmc->max_segs = host->pdata->blk_settings->max_segs;
>>                mmc->max_blk_size = host->pdata->blk_settings->max_blk_size;
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index e626732..c0a5a91 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2812,15 +2812,6 @@ int sdhci_add_host(struct sdhci_host *host)
>>        if (caps[1] & SDHCI_DRIVER_TYPE_D)
>>                mmc->caps |= MMC_CAP_DRIVER_TYPE_D;
>>
>> -       /*
>> -        * If Power Off Notify capability is enabled by the host,
>> -        * set notify to short power off notify timeout value.
>> -        */
>> -       if (mmc->caps2 & MMC_CAP2_POWEROFF_NOTIFY)
>> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>> -       else
>> -               mmc->power_notify_type = MMC_HOST_PW_NOTIFY_NONE;
>> -
>>        /* Initial value for re-tuning timer count */
>>        host->tuning_count = (caps[1] & SDHCI_RETUNING_TIMER_COUNT_MASK) >>
>>                              SDHCI_RETUNING_TIMER_COUNT_SHIFT;
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index d76513b..040eec4 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -239,11 +239,10 @@ struct mmc_card {
>>  #define MMC_QUIRK_BROKEN_BYTE_MODE_512 (1<<8)  /* Avoid sending 512 bytes in */
>>  #define MMC_QUIRK_LONG_READ_TIME (1<<9)                /* Data read time > CSD says */
>>                                                /* byte mode */
>> -       unsigned int    poweroff_notify_state;  /* eMMC4.5 notify feature */
>> +       unsigned int            poweroff_notify_state; /* MMC-4.5 poweroff
>> +                                                       notify feature */
>>  #define MMC_NO_POWER_NOTIFICATION      0
>>  #define MMC_POWERED_ON                 1
>> -#define MMC_POWEROFF_SHORT             2
>> -#define MMC_POWEROFF_LONG              3
>>
>>        unsigned int            erase_size;     /* erase size in sectors */
>>        unsigned int            erase_shift;    /* if erase unit is power 2 */
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> index 1b431c7..54894d6 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -161,6 +161,7 @@ extern int mmc_can_trim(struct mmc_card *card);
>>  extern int mmc_can_discard(struct mmc_card *card);
>>  extern int mmc_can_sanitize(struct mmc_card *card);
>>  extern int mmc_can_secure_erase_trim(struct mmc_card *card);
>> +extern int mmc_can_poweroff_notify(const struct mmc_card *card);
>>  extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
>>                                   unsigned int nr);
>>  extern unsigned int mmc_calc_max_discard(struct mmc_card *card);
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 0707d22..0e9adac 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -238,12 +238,9 @@ 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;
>> -#define MMC_HOST_PW_NOTIFY_NONE                0
>> -#define MMC_HOST_PW_NOTIFY_SHORT       1
>> -#define MMC_HOST_PW_NOTIFY_LONG                2
>>
>>  #ifdef CONFIG_MMC_CLKGATE
>>        int                     clk_requests;   /* internal reference counter */
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index d425cab..b11876b 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -386,4 +386,11 @@ struct _mmc_csd {
>>  #define MMC_SWITCH_MODE_CLEAR_BITS     0x02    /* Clear bits which are 1 in value */
>>  #define MMC_SWITCH_MODE_WRITE_BYTE     0x03    /* Set target to value */
>>
>> +/*
>> + * MMC Poweroff Notify types
>> + */
>> +#define MMC_PW_OFF_NOTIFY_NONE         0
>> +#define MMC_PW_OFF_NOTIFY_SHORT                1
>> +#define MMC_PW_OFF_NOTIFY_LONG         2
>> +
>>  #endif /* LINUX_MMC_MMC_H */
>> --
>> 1.7.4.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson June 14, 2012, 2:50 p.m. UTC | #8

Hi Girish,

On 14 June 2012 15:21, Girish K S <girish.shivananjappa@linaro.org> wrote:
> On 14 June 2012 18:43, Per Forlin <per.lkml@gmail.com> wrote:
>> Hi Girish and Suagata,
>>
>> I have run some regression tests with this patch on our board (ux500
>> family) running suspend and resume of the eMMC 4.41 device.
>>
>> The two patches I have looked at are:
>> 1. "mmc: core: Fix PowerOff Notify suspend/resume" (merged)
>> 2. " MMC-4.5 Power OFF Notify Rework"
>>
>> With only patch #1 the eMMC doesn't power up after in resume() after
>> being suspended. The eMMC doesn't respond at all after suspend. It's
>> not powered up.
>> Running tests with #1 and #2, the card is powered up but it doesn't
>> wake up after CMD5. Commands that arrive are after resume/CMD5
>> timeouts. I even tried by increasing the awake timeout to 5 seconds
>> but i didn't help.
>>
>> The eMMC on my board successfully suspends and resumes with patch #1
>> and #2 if waking up the card using CMD0 (mmc_card_init()) instead of
>> CMD5.
>>
>> Have anyone else seen the same issue?
>> Have this patch been verified on a board together with eMMC 4.41 that
>> supports card power off.
> This rework patch is still under progress. we are modifying it. In our
> earlier discussions subhash has posted the
> same issue and a solution for this.  we should save ios context before
> sleep and restore ios before awake. soon rework patch will be
> posted with the above recomenedded solution.
>

I think the best solution is to always do mmc_card_init when doing
resume, it will be nice a simple.
Otherwise it will be somewhat tricky to keep track of what state we
are in, and if the ios should be restored or not.

Finally, I would be glad to help out in posting an updated version of
this patch, if that OK with you?

Kind regards
Ulf Hansson

Saugata Das June 14, 2012, 3:15 p.m. UTC | #9

On 14 June 2012 20:20, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Hi Girish,
>
> On 14 June 2012 15:21, Girish K S <girish.shivananjappa@linaro.org> wrote:
>> On 14 June 2012 18:43, Per Forlin <per.lkml@gmail.com> wrote:
>>> Hi Girish and Suagata,
>>>
>>> I have run some regression tests with this patch on our board (ux500
>>> family) running suspend and resume of the eMMC 4.41 device.
>>>
>>> The two patches I have looked at are:
>>> 1. "mmc: core: Fix PowerOff Notify suspend/resume" (merged)
>>> 2. " MMC-4.5 Power OFF Notify Rework"
>>>
>>> With only patch #1 the eMMC doesn't power up after in resume() after
>>> being suspended. The eMMC doesn't respond at all after suspend. It's
>>> not powered up.
>>> Running tests with #1 and #2, the card is powered up but it doesn't
>>> wake up after CMD5. Commands that arrive are after resume/CMD5
>>> timeouts. I even tried by increasing the awake timeout to 5 seconds
>>> but i didn't help.
>>>
>>> The eMMC on my board successfully suspends and resumes with patch #1
>>> and #2 if waking up the card using CMD0 (mmc_card_init()) instead of
>>> CMD5.
>>>
>>> Have anyone else seen the same issue?
>>> Have this patch been verified on a board together with eMMC 4.41 that
>>> supports card power off.
>> This rework patch is still under progress. we are modifying it. In our
>> earlier discussions subhash has posted the
>> same issue and a solution for this.  we should save ios context before
>> sleep and restore ios before awake. soon rework patch will be
>> posted with the above recomenedded solution.
>>
>
> I think the best solution is to always do mmc_card_init when doing
> resume, it will be nice a simple.

Note that, with power OFF notify (MMC-4.5), there will be some pending
operation with the MMC controller. If we do mmc_card_init after
suspend, then there could be some data loss.

I  have passed to Per the latest patch (Subhash reported that it is
working). I shall forward to you as well. Lets solve the issue. If you
can work around, without mmc_card_init after suspend, then you are
most welcome to update the patch :-)


> Otherwise it will be somewhat tricky to keep track of what state we
> are in, and if the ios should be restored or not.
>
> Finally, I would be glad to help out in posting an updated version of
> this patch, if that OK with you?
>
> Kind regards
> Ulf Hansson

Per Forlin June 14, 2012, 7:06 p.m. UTC | #10

Hi Saugata,

I can have a go and test it. But first I would like to bring up 3
concerns that I have with this patch.

1. This patch should be sent to cc-stable in order to repair the bug
introduced in 3.4

2. Is the bus_ops for poweroff_notify really necessary since only mmc
use it? There are already bus_ops for suspend/resume,
power_save/power_restore and remove. It feels like it would be
possible to address poweroff_notify internally from mmc.c from theses
bus_ops. I would be nice to add this feature without having to touch
core.c.

For instance. Call mmc_suspend() from mmc_remove()
+++ b/drivers/mmc/core/mmc.c
@@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
+       __mmc_suspend(host, true);
        mmc_remove_card(host->card);

@@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
-static int mmc_suspend(struct mmc_host *host)
+static int __mmc_suspend(struct mmc_host *host, bool remove)

@@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
        mmc_claim_host(host);
        if (mmc_can_poweroff_notify(host->card) &&
-               (host->caps2 & MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
+               (host->caps2 & MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
+                remove)) {
                err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
        } else {
                if (mmc_card_can_sleep(host))

@@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
+static int mmc_suspend(struct mmc_host *host)
+{
+       return __mmc_suspend(host, false);
+}
+

Calling mmc_suspend from mmc_remove() has another advantage since it
may issue SLEEP (CMD5) before the card is removed and power off. This
is recommended by eMMC Vendors in order to shutdown the eMMC safely to
prevent data corruption. When the platform shuts down the power to the
eMMC will be turned off no matter what.

3. About the sleep and awake issue. This is not really related to
poweroff_notify() as I see it. I would recommend to use CMD 0 to
re-init the card safely after sleep in this patch. Then you could send
out a sleep/awake patch that address this separately.  This would also
make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
new feature and should not be sent to the cc-stable list. What do you
think?

BR
/Per

On Thu, Jun 14, 2012 at 5:15 PM, Saugata Das <saugata.das@linaro.org> wrote:
> On 14 June 2012 20:20, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Hi Girish,
>>
>> On 14 June 2012 15:21, Girish K S <girish.shivananjappa@linaro.org> wrote:
>>> On 14 June 2012 18:43, Per Forlin <per.lkml@gmail.com> wrote:
>>>> Hi Girish and Suagata,
>>>>
>>>> I have run some regression tests with this patch on our board (ux500
>>>> family) running suspend and resume of the eMMC 4.41 device.
>>>>
>>>> The two patches I have looked at are:
>>>> 1. "mmc: core: Fix PowerOff Notify suspend/resume" (merged)
>>>> 2. " MMC-4.5 Power OFF Notify Rework"
>>>>
>>>> With only patch #1 the eMMC doesn't power up after in resume() after
>>>> being suspended. The eMMC doesn't respond at all after suspend. It's
>>>> not powered up.
>>>> Running tests with #1 and #2, the card is powered up but it doesn't
>>>> wake up after CMD5. Commands that arrive are after resume/CMD5
>>>> timeouts. I even tried by increasing the awake timeout to 5 seconds
>>>> but i didn't help.
>>>>
>>>> The eMMC on my board successfully suspends and resumes with patch #1
>>>> and #2 if waking up the card using CMD0 (mmc_card_init()) instead of
>>>> CMD5.
>>>>
>>>> Have anyone else seen the same issue?
>>>> Have this patch been verified on a board together with eMMC 4.41 that
>>>> supports card power off.
>>> This rework patch is still under progress. we are modifying it. In our
>>> earlier discussions subhash has posted the
>>> same issue and a solution for this.  we should save ios context before
>>> sleep and restore ios before awake. soon rework patch will be
>>> posted with the above recomenedded solution.
>>>
>>
>> I think the best solution is to always do mmc_card_init when doing
>> resume, it will be nice a simple.
>
> Note that, with power OFF notify (MMC-4.5), there will be some pending
> operation with the MMC controller. If we do mmc_card_init after
> suspend, then there could be some data loss.
>
> I  have passed to Per the latest patch (Subhash reported that it is
> working). I shall forward to you as well. Lets solve the issue. If you
> can work around, without mmc_card_init after suspend, then you are
> most welcome to update the patch :-)
>
>
>> Otherwise it will be somewhat tricky to keep track of what state we
>> are in, and if the ios should be restored or not.
>>
>> Finally, I would be glad to help out in posting an updated version of
>> this patch, if that OK with you?
>>
>> Kind regards
>> Ulf Hansson

Saugata Das June 15, 2012, 3:49 a.m. UTC | #11

On 15 June 2012 00:36, Per Forlin <per.lkml@gmail.com> wrote:
> Hi Saugata,
>
> I can have a go and test it. But first I would like to bring up 3
> concerns that I have with this patch.
>
> 1. This patch should be sent to cc-stable in order to repair the bug
> introduced in 3.4

I shall sent it out today.

> 2. Is the bus_ops for poweroff_notify really necessary since only mmc
> use it?

This was recommended in the review from Ulf. If it is not adding to a
bug, I propose to keep it this way. Otherwise, we will be going in
circles :-)

> There are already bus_ops for suspend/resume,
> power_save/power_restore and remove. It feels like it would be
> possible to address poweroff_notify internally from mmc.c from theses
> bus_ops. I would be nice to add this feature without having to touch
> core.c.
>
> For instance. Call mmc_suspend() from mmc_remove()
> +++ b/drivers/mmc/core/mmc.c
> @@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
> +       __mmc_suspend(host, true);
>        mmc_remove_card(host->card);
>
> @@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
> -static int mmc_suspend(struct mmc_host *host)
> +static int __mmc_suspend(struct mmc_host *host, bool remove)
>
> @@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
>        mmc_claim_host(host);
>        if (mmc_can_poweroff_notify(host->card) &&
> -               (host->caps2 & MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
> +               (host->caps2 & MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
> +                remove)) {
>                err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>        } else {
>                if (mmc_card_can_sleep(host))
>
> @@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
> +static int mmc_suspend(struct mmc_host *host)
> +{
> +       return __mmc_suspend(host, false);
> +}
> +
>
> Calling mmc_suspend from mmc_remove() has another advantage since it
> may issue SLEEP (CMD5) before the card is removed and power off. This
> is recommended by eMMC Vendors in order to shutdown the eMMC safely to
> prevent data corruption. When the platform shuts down the power to the
> eMMC will be turned off no matter what.

May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
has to be power OFF notify when the power is removed. Lets do it for
another patch, since the intention of this patch is to fix the issues
around power OFF notify.

>
> 3. About the sleep and awake issue. This is not really related to
> poweroff_notify() as I see it. I would recommend to use CMD 0 to
> re-init the card safely after sleep in this patch. Then you could send
> out a sleep/awake patch that address this separately.  This would also
> make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
> new feature and should not be sent to the cc-stable list. What do you
> think?

The problem in sending CMD0 without power OFF notify is possibility of
some data loss in MMC-4.5 devices.


>
> BR
> /Per
>
> On Thu, Jun 14, 2012 at 5:15 PM, Saugata Das <saugata.das@linaro.org> wrote:
>> On 14 June 2012 20:20, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Hi Girish,
>>>
>>> On 14 June 2012 15:21, Girish K S <girish.shivananjappa@linaro.org> wrote:
>>>> On 14 June 2012 18:43, Per Forlin <per.lkml@gmail.com> wrote:
>>>>> Hi Girish and Suagata,
>>>>>
>>>>> I have run some regression tests with this patch on our board (ux500
>>>>> family) running suspend and resume of the eMMC 4.41 device.
>>>>>
>>>>> The two patches I have looked at are:
>>>>> 1. "mmc: core: Fix PowerOff Notify suspend/resume" (merged)
>>>>> 2. " MMC-4.5 Power OFF Notify Rework"
>>>>>
>>>>> With only patch #1 the eMMC doesn't power up after in resume() after
>>>>> being suspended. The eMMC doesn't respond at all after suspend. It's
>>>>> not powered up.
>>>>> Running tests with #1 and #2, the card is powered up but it doesn't
>>>>> wake up after CMD5. Commands that arrive are after resume/CMD5
>>>>> timeouts. I even tried by increasing the awake timeout to 5 seconds
>>>>> but i didn't help.
>>>>>
>>>>> The eMMC on my board successfully suspends and resumes with patch #1
>>>>> and #2 if waking up the card using CMD0 (mmc_card_init()) instead of
>>>>> CMD5.
>>>>>
>>>>> Have anyone else seen the same issue?
>>>>> Have this patch been verified on a board together with eMMC 4.41 that
>>>>> supports card power off.
>>>> This rework patch is still under progress. we are modifying it. In our
>>>> earlier discussions subhash has posted the
>>>> same issue and a solution for this.  we should save ios context before
>>>> sleep and restore ios before awake. soon rework patch will be
>>>> posted with the above recomenedded solution.
>>>>
>>>
>>> I think the best solution is to always do mmc_card_init when doing
>>> resume, it will be nice a simple.
>>
>> Note that, with power OFF notify (MMC-4.5), there will be some pending
>> operation with the MMC controller. If we do mmc_card_init after
>> suspend, then there could be some data loss.
>>
>> I  have passed to Per the latest patch (Subhash reported that it is
>> working). I shall forward to you as well. Lets solve the issue. If you
>> can work around, without mmc_card_init after suspend, then you are
>> most welcome to update the patch :-)
>>
>>
>>> Otherwise it will be somewhat tricky to keep track of what state we
>>> are in, and if the ios should be restored or not.
>>>
>>> Finally, I would be glad to help out in posting an updated version of
>>> this patch, if that OK with you?
>>>
>>> Kind regards
>>> Ulf Hansson

Ulf Hansson June 15, 2012, 7:22 a.m. UTC | #12

On 06/15/2012 05:49 AM, Saugata Das wrote:
> On 15 June 2012 00:36, Per Forlin<per.lkml@gmail.com>  wrote:
>> Hi Saugata,
>>
>> I can have a go and test it. But first I would like to bring up 3
>> concerns that I have with this patch.
>>
>> 1. This patch should be sent to cc-stable in order to repair the bug
>> introduced in 3.4
>
> I shall sent it out today.
>
>> 2. Is the bus_ops for poweroff_notify really necessary since only mmc
>> use it?
>
> This was recommended in the review from Ulf. If it is not adding to a
> bug, I propose to keep it this way. Otherwise, we will be going in
> circles :-)

Moreover it seems close related to sleep, which is implemented with 
bus_ops. So I would say, keep as is. We might change later, then both 
sleep and poweroff_notify together.

>
>> There are already bus_ops for suspend/resume,
>> power_save/power_restore and remove. It feels like it would be
>> possible to address poweroff_notify internally from mmc.c from theses
>> bus_ops. I would be nice to add this feature without having to touch
>> core.c.
>>
>> For instance. Call mmc_suspend() from mmc_remove()
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
>> +       __mmc_suspend(host, true);
>>         mmc_remove_card(host->card);
>>
>> @@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
>> -static int mmc_suspend(struct mmc_host *host)
>> +static int __mmc_suspend(struct mmc_host *host, bool remove)
>>
>> @@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
>>         mmc_claim_host(host);
>>         if (mmc_can_poweroff_notify(host->card)&&
>> -               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>> +               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
>> +                remove)) {
>>                 err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>>         } else {
>>                 if (mmc_card_can_sleep(host))
>>
>> @@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
>> +static int mmc_suspend(struct mmc_host *host)
>> +{
>> +       return __mmc_suspend(host, false);
>> +}
>> +
>>
>> Calling mmc_suspend from mmc_remove() has another advantage since it
>> may issue SLEEP (CMD5) before the card is removed and power off. This
>> is recommended by eMMC Vendors in order to shutdown the eMMC safely to
>> prevent data corruption. When the platform shuts down the power to the
>> eMMC will be turned off no matter what.
>
> May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
> has to be power OFF notify when the power is removed. Lets do it for
> another patch, since the intention of this patch is to fix the issues
> around power OFF notify.

I agree with you Saugata, the exact same sequence as in suspend can not 
be used. The reason is simply that we do not want to consider 
MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, 
as we want in suspend.

Leave this to be fixed in a separate patch instead.

>
>>
>> 3. About the sleep and awake issue. This is not really related to
>> poweroff_notify() as I see it. I would recommend to use CMD 0 to
>> re-init the card safely after sleep in this patch. Then you could send
>> out a sleep/awake patch that address this separately.  This would also
>> make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
>> new feature and should not be sent to the cc-stable list. What do you
>> think?
>
> The problem in sending CMD0 without power OFF notify is possibility of
> some data loss in MMC-4.5 devices.

I don't see the problem here. You will be sending power OFF notify when 
you can. The only difference is when you "wake" the device from sleep 
mode. Instead of using CMD5, which may work is some cases and in some 
cases not (without restoring ios). So using CMD0 as common way of waking 
up from suspend should be fine. Unless I missed something of course. :-)

Kind regards
Ulf Hansson

Per Forlin June 15, 2012, 7:49 a.m. UTC | #13

Hi Ulf,

On Fri, Jun 15, 2012 at 9:22 AM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> On 06/15/2012 05:49 AM, Saugata Das wrote:
>>
>> On 15 June 2012 00:36, Per Forlin<per.lkml@gmail.com>  wrote:
>>>
>>> Hi Saugata,
>>>
>>> I can have a go and test it. But first I would like to bring up 3
>>> concerns that I have with this patch.
>>>
>>> 1. This patch should be sent to cc-stable in order to repair the bug
>>> introduced in 3.4
>>
>>
>> I shall sent it out today.
>>
>>> 2. Is the bus_ops for poweroff_notify really necessary since only mmc
>>> use it?
>>
>>
>> This was recommended in the review from Ulf. If it is not adding to a
>> bug, I propose to keep it this way. Otherwise, we will be going in
>> circles :-)
>
>
> Moreover it seems close related to sleep, which is implemented with bus_ops.
> So I would say, keep as is. We might change later, then both sleep and
> poweroff_notify together.
>
>>
>>> There are already bus_ops for suspend/resume,
>>> power_save/power_restore and remove. It feels like it would be
>>> possible to address poweroff_notify internally from mmc.c from theses
>>> bus_ops. I would be nice to add this feature without having to touch
>>> core.c.
>>>
>>> For instance. Call mmc_suspend() from mmc_remove()
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
>>> +       __mmc_suspend(host, true);
>>>        mmc_remove_card(host->card);
>>>
>>> @@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
>>> -static int mmc_suspend(struct mmc_host *host)
>>> +static int __mmc_suspend(struct mmc_host *host, bool remove)
The remove parameter takes care of the remove case.
>>>
>>> @@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
>>>        mmc_claim_host(host);
>>>        if (mmc_can_poweroff_notify(host->card)&&
>>> -               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>>> +               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
>>>
>>> +                remove)) {
>>>                err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>>>        } else {
>>>                if (mmc_card_can_sleep(host))
>>>
>>> @@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
>>> +static int mmc_suspend(struct mmc_host *host)
>>> +{
>>> +       return __mmc_suspend(host, false);
>>> +}
>>> +
>>>
>>> Calling mmc_suspend from mmc_remove() has another advantage since it
>>> may issue SLEEP (CMD5) before the card is removed and power off. This
>>> is recommended by eMMC Vendors in order to shutdown the eMMC safely to
>>> prevent data corruption. When the platform shuts down the power to the
>>> eMMC will be turned off no matter what.
>>
>>
>> May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
>> has to be power OFF notify when the power is removed. Lets do it for
>> another patch, since the intention of this patch is to fix the issues
>> around power OFF notify.
>
>
> I agree with you Saugata, the exact same sequence as in suspend can not be
> used. The reason is simply that we do not want to consider
> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, as we
> want in suspend.
>
> Leave this to be fixed in a separate patch instead.
I have addressed this in my prototype patch above. The remove param in
__suspend tells if we are removing or just suspending. This approach
actually removes lines of code in core.c mmc_stop_host().

>
>
>>
>>>
>>> 3. About the sleep and awake issue. This is not really related to
>>> poweroff_notify() as I see it. I would recommend to use CMD 0 to
>>> re-init the card safely after sleep in this patch. Then you could send
>>> out a sleep/awake patch that address this separately.  This would also
>>> make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
>>> new feature and should not be sent to the cc-stable list. What do you
>>> think?
>>
>>
>> The problem in sending CMD0 without power OFF notify is possibility of
>> some data loss in MMC-4.5 devices.
>
>
> I don't see the problem here. You will be sending power OFF notify when you
> can. The only difference is when you "wake" the device from sleep mode.
> Instead of using CMD5, which may work is some cases and in some cases not
> (without restoring ios). So using CMD0 as common way of waking up from
> suspend should be fine. Unless I missed something of course. :-)
>
I'm in favour of simplifying this patch. CMD0 instead of CMD5 reduces
the number of lines in this patch. It also make this patch work :)
Using CMD 5 to wake up could be done in a separate patch.

Bottom line. If the patch is clean and works I'm happy.
I can help out and test the patch.

Regards,
Per

Saugata Das June 15, 2012, 8:34 a.m. UTC | #14

On 15 June 2012 12:52, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> On 06/15/2012 05:49 AM, Saugata Das wrote:
>>
>> On 15 June 2012 00:36, Per Forlin<per.lkml@gmail.com>  wrote:
>>>
>>> Hi Saugata,
>>>
>>> I can have a go and test it. But first I would like to bring up 3
>>> concerns that I have with this patch.
>>>
>>> 1. This patch should be sent to cc-stable in order to repair the bug
>>> introduced in 3.4
>>
>>
>> I shall sent it out today.
>>
>>> 2. Is the bus_ops for poweroff_notify really necessary since only mmc
>>> use it?
>>
>>
>> This was recommended in the review from Ulf. If it is not adding to a
>> bug, I propose to keep it this way. Otherwise, we will be going in
>> circles :-)
>
>
> Moreover it seems close related to sleep, which is implemented with bus_ops.
> So I would say, keep as is. We might change later, then both sleep and
> poweroff_notify together.
>
>>
>>> There are already bus_ops for suspend/resume,
>>> power_save/power_restore and remove. It feels like it would be
>>> possible to address poweroff_notify internally from mmc.c from theses
>>> bus_ops. I would be nice to add this feature without having to touch
>>> core.c.
>>>
>>> For instance. Call mmc_suspend() from mmc_remove()
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
>>> +       __mmc_suspend(host, true);
>>>        mmc_remove_card(host->card);
>>>
>>> @@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
>>> -static int mmc_suspend(struct mmc_host *host)
>>> +static int __mmc_suspend(struct mmc_host *host, bool remove)
>>>
>>> @@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
>>>        mmc_claim_host(host);
>>>        if (mmc_can_poweroff_notify(host->card)&&
>>> -               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>>> +               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
>>>
>>> +                remove)) {
>>>                err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>>>        } else {
>>>                if (mmc_card_can_sleep(host))
>>>
>>> @@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
>>> +static int mmc_suspend(struct mmc_host *host)
>>> +{
>>> +       return __mmc_suspend(host, false);
>>> +}
>>> +
>>>
>>> Calling mmc_suspend from mmc_remove() has another advantage since it
>>> may issue SLEEP (CMD5) before the card is removed and power off. This
>>> is recommended by eMMC Vendors in order to shutdown the eMMC safely to
>>> prevent data corruption. When the platform shuts down the power to the
>>> eMMC will be turned off no matter what.
>>
>>
>> May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
>> has to be power OFF notify when the power is removed. Lets do it for
>> another patch, since the intention of this patch is to fix the issues
>> around power OFF notify.
>
>
> I agree with you Saugata, the exact same sequence as in suspend can not be
> used. The reason is simply that we do not want to consider
> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, as we
> want in suspend.
>
> Leave this to be fixed in a separate patch instead.
>
>
>>
>>>
>>> 3. About the sleep and awake issue. This is not really related to
>>> poweroff_notify() as I see it. I would recommend to use CMD 0 to
>>> re-init the card safely after sleep in this patch. Then you could send
>>> out a sleep/awake patch that address this separately.  This would also
>>> make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
>>> new feature and should not be sent to the cc-stable list. What do you
>>> think?
>>
>>
>> The problem in sending CMD0 without power OFF notify is possibility of
>> some data loss in MMC-4.5 devices.
>
>
> I don't see the problem here. You will be sending power OFF notify when you
> can. The only difference is when you "wake" the device from sleep mode.
> Instead of using CMD5, which may work is some cases and in some cases not
> (without restoring ios). So using CMD0 as common way of waking up from
> suspend should be fine. Unless I missed something of course. :-)
>

CMD0 is a reset. I expect with power OFF notify enable, the eMMC
device will postpone some control information update to its internal
non-volatile memory (e.g. some data structures which are kept in the
controller buffers and not stored in NAND). If we do a CMD0, then the
eMMC device will be reset and we may lose some data. In addition to
that, doing complete card initialization will increase the wakeup time
(for 4.4 devices).

Till now, we have done complete card initialization during resume and
not done a real resume. I am not sure, if this patch is exposing some
host drivers issue now. So, please check the drivers as well.


> Kind regards
> Ulf Hansson

Per Forlin June 15, 2012, 9:52 a.m. UTC | #15

On Fri, Jun 15, 2012 at 10:34 AM, Saugata Das <saugata.das@linaro.org> wrote:
> On 15 June 2012 12:52, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>> On 06/15/2012 05:49 AM, Saugata Das wrote:
>>>
>>> On 15 June 2012 00:36, Per Forlin<per.lkml@gmail.com>  wrote:
>>>>
>>>> Hi Saugata,
>>>>
>>>> I can have a go and test it. But first I would like to bring up 3
>>>> concerns that I have with this patch.
>>>>
>>>> 1. This patch should be sent to cc-stable in order to repair the bug
>>>> introduced in 3.4
>>>
>>>
>>> I shall sent it out today.
>>>
>>>> 2. Is the bus_ops for poweroff_notify really necessary since only mmc
>>>> use it?
>>>
>>>
>>> This was recommended in the review from Ulf. If it is not adding to a
>>> bug, I propose to keep it this way. Otherwise, we will be going in
>>> circles :-)
>>
>>
>> Moreover it seems close related to sleep, which is implemented with bus_ops.
>> So I would say, keep as is. We might change later, then both sleep and
>> poweroff_notify together.
>>
>>>
>>>> There are already bus_ops for suspend/resume,
>>>> power_save/power_restore and remove. It feels like it would be
>>>> possible to address poweroff_notify internally from mmc.c from theses
>>>> bus_ops. I would be nice to add this feature without having to touch
>>>> core.c.
>>>>
>>>> For instance. Call mmc_suspend() from mmc_remove()
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
>>>> +       __mmc_suspend(host, true);
>>>>        mmc_remove_card(host->card);
>>>>
>>>> @@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
>>>> -static int mmc_suspend(struct mmc_host *host)
>>>> +static int __mmc_suspend(struct mmc_host *host, bool remove)
>>>>
>>>> @@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
>>>>        mmc_claim_host(host);
>>>>        if (mmc_can_poweroff_notify(host->card)&&
>>>> -               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>>>> +               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
>>>>
>>>> +                remove)) {
>>>>                err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>>>>        } else {
>>>>                if (mmc_card_can_sleep(host))
>>>>
>>>> @@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
>>>> +static int mmc_suspend(struct mmc_host *host)
>>>> +{
>>>> +       return __mmc_suspend(host, false);
>>>> +}
>>>> +
>>>>
>>>> Calling mmc_suspend from mmc_remove() has another advantage since it
>>>> may issue SLEEP (CMD5) before the card is removed and power off. This
>>>> is recommended by eMMC Vendors in order to shutdown the eMMC safely to
>>>> prevent data corruption. When the platform shuts down the power to the
>>>> eMMC will be turned off no matter what.
>>>
>>>
>>> May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
>>> has to be power OFF notify when the power is removed. Lets do it for
>>> another patch, since the intention of this patch is to fix the issues
>>> around power OFF notify.
>>
>>
>> I agree with you Saugata, the exact same sequence as in suspend can not be
>> used. The reason is simply that we do not want to consider
>> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, as we
>> want in suspend.
>>
>> Leave this to be fixed in a separate patch instead.
>>
>>
>>>
>>>>
>>>> 3. About the sleep and awake issue. This is not really related to
>>>> poweroff_notify() as I see it. I would recommend to use CMD 0 to
>>>> re-init the card safely after sleep in this patch. Then you could send
>>>> out a sleep/awake patch that address this separately.  This would also
>>>> make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
>>>> new feature and should not be sent to the cc-stable list. What do you
>>>> think?
>>>
>>>
>>> The problem in sending CMD0 without power OFF notify is possibility of
>>> some data loss in MMC-4.5 devices.
>>
>>
>> I don't see the problem here. You will be sending power OFF notify when you
>> can. The only difference is when you "wake" the device from sleep mode.
>> Instead of using CMD5, which may work is some cases and in some cases not
>> (without restoring ios). So using CMD0 as common way of waking up from
>> suspend should be fine. Unless I missed something of course. :-)
>>
>
> CMD0 is a reset. I expect with power OFF notify enable, the eMMC
> device will postpone some control information update to its internal
> non-volatile memory (e.g. some data structures which are kept in the
> controller buffers and not stored in NAND). If we do a CMD0, then the
> eMMC device will be reset and we may lose some data. In addition to
> that, doing complete card initialization will increase the wakeup time
> (for 4.4 devices).
>
> Till now, we have done complete card initialization during resume
Yes, me and Ulf think we should still do a complete initialization, at
least for now and in this patch.

A separate patch may deal with resume awake CMD5 and IOS save/restore.

We may also discuss a clean up patch later on to reduce the number of
bus_ops. Sleep, awake, and poweroff_notify are MMC specific.
power_save/power_restore maps to suspend/resume. But let's not discuss
this now :)

BR
Per

Saugata Das June 15, 2012, 10:52 a.m. UTC | #16

On 15 June 2012 15:22, Per Forlin <per.lkml@gmail.com> wrote:
> On Fri, Jun 15, 2012 at 10:34 AM, Saugata Das <saugata.das@linaro.org> wrote:
>> On 15 June 2012 12:52, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>>> On 06/15/2012 05:49 AM, Saugata Das wrote:
>>>>
>>>> On 15 June 2012 00:36, Per Forlin<per.lkml@gmail.com>  wrote:
>>>>>
>>>>> Hi Saugata,
>>>>>
>>>>> I can have a go and test it. But first I would like to bring up 3
>>>>> concerns that I have with this patch.
>>>>>
>>>>> 1. This patch should be sent to cc-stable in order to repair the bug
>>>>> introduced in 3.4
>>>>
>>>>
>>>> I shall sent it out today.
>>>>
>>>>> 2. Is the bus_ops for poweroff_notify really necessary since only mmc
>>>>> use it?
>>>>
>>>>
>>>> This was recommended in the review from Ulf. If it is not adding to a
>>>> bug, I propose to keep it this way. Otherwise, we will be going in
>>>> circles :-)
>>>
>>>
>>> Moreover it seems close related to sleep, which is implemented with bus_ops.
>>> So I would say, keep as is. We might change later, then both sleep and
>>> poweroff_notify together.
>>>
>>>>
>>>>> There are already bus_ops for suspend/resume,
>>>>> power_save/power_restore and remove. It feels like it would be
>>>>> possible to address poweroff_notify internally from mmc.c from theses
>>>>> bus_ops. I would be nice to add this feature without having to touch
>>>>> core.c.
>>>>>
>>>>> For instance. Call mmc_suspend() from mmc_remove()
>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>> @@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
>>>>> +       __mmc_suspend(host, true);
>>>>>        mmc_remove_card(host->card);
>>>>>
>>>>> @@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
>>>>> -static int mmc_suspend(struct mmc_host *host)
>>>>> +static int __mmc_suspend(struct mmc_host *host, bool remove)
>>>>>
>>>>> @@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
>>>>>        mmc_claim_host(host);
>>>>>        if (mmc_can_poweroff_notify(host->card)&&
>>>>> -               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>>>>> +               (host->caps2&  MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
>>>>>
>>>>> +                remove)) {
>>>>>                err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
>>>>>        } else {
>>>>>                if (mmc_card_can_sleep(host))
>>>>>
>>>>> @@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
>>>>> +static int mmc_suspend(struct mmc_host *host)
>>>>> +{
>>>>> +       return __mmc_suspend(host, false);
>>>>> +}
>>>>> +
>>>>>
>>>>> Calling mmc_suspend from mmc_remove() has another advantage since it
>>>>> may issue SLEEP (CMD5) before the card is removed and power off. This
>>>>> is recommended by eMMC Vendors in order to shutdown the eMMC safely to
>>>>> prevent data corruption. When the platform shuts down the power to the
>>>>> eMMC will be turned off no matter what.
>>>>
>>>>
>>>> May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
>>>> has to be power OFF notify when the power is removed. Lets do it for
>>>> another patch, since the intention of this patch is to fix the issues
>>>> around power OFF notify.
>>>
>>>
>>> I agree with you Saugata, the exact same sequence as in suspend can not be
>>> used. The reason is simply that we do not want to consider
>>> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, as we
>>> want in suspend.
>>>
>>> Leave this to be fixed in a separate patch instead.
>>>
>>>
>>>>
>>>>>
>>>>> 3. About the sleep and awake issue. This is not really related to
>>>>> poweroff_notify() as I see it. I would recommend to use CMD 0 to
>>>>> re-init the card safely after sleep in this patch. Then you could send
>>>>> out a sleep/awake patch that address this separately.  This would also
>>>>> make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
>>>>> new feature and should not be sent to the cc-stable list. What do you
>>>>> think?
>>>>
>>>>
>>>> The problem in sending CMD0 without power OFF notify is possibility of
>>>> some data loss in MMC-4.5 devices.
>>>
>>>
>>> I don't see the problem here. You will be sending power OFF notify when you
>>> can. The only difference is when you "wake" the device from sleep mode.
>>> Instead of using CMD5, which may work is some cases and in some cases not
>>> (without restoring ios). So using CMD0 as common way of waking up from
>>> suspend should be fine. Unless I missed something of course. :-)
>>>
>>
>> CMD0 is a reset. I expect with power OFF notify enable, the eMMC
>> device will postpone some control information update to its internal
>> non-volatile memory (e.g. some data structures which are kept in the
>> controller buffers and not stored in NAND). If we do a CMD0, then the
>> eMMC device will be reset and we may lose some data. In addition to
>> that, doing complete card initialization will increase the wakeup time
>> (for 4.4 devices).
>>
>> Till now, we have done complete card initialization during resume
> Yes, me and Ulf think we should still do a complete initialization, at
> least for now and in this patch.
>

In my opinion, that's incorrect on MMC-4.5 device and unoptimized for
MMC-4.41 device.

Let me propose a new cap, MMC_CAP2_NO_INIT_ON_RESUME and do something
like following in mmc_resume,

 	mmc_claim_host(host);
-	if (mmc_card_is_sleep(host->card)) {
+	if (mmc_card_is_sleep(host->card) &&
+		(host->caps2 & MMC_CAP2_NO_INIT_ON_RESUME)) {
 		mmc_restore_ios(host, &host->saved_ios);
 		err = mmc_card_awake(host);
	} else
		err = mmc_init_card(host, host->ocr, host->card);

I hope it's OK for Ulf, Per, Subhash, Girish, Asutosh.


> A separate patch may deal with resume awake CMD5 and IOS save/restore.
>
> We may also discuss a clean up patch later on to reduce the number of
> bus_ops. Sleep, awake, and poweroff_notify are MMC specific.
> power_save/power_restore maps to suspend/resume. But let's not discuss
> this now :)
>
> BR
> Per

Ulf Hansson June 15, 2012, 11:26 a.m. UTC | #17

Hi Saugata,

snip.

>>>>>
>>>>> The problem in sending CMD0 without power OFF notify is possibility of
>>>>> some data loss in MMC-4.5 devices.
>>>>
>>>>
>>>> I don't see the problem here. You will be sending power OFF notify when you
>>>> can. The only difference is when you "wake" the device from sleep mode.
>>>> Instead of using CMD5, which may work is some cases and in some cases not
>>>> (without restoring ios). So using CMD0 as common way of waking up from
>>>> suspend should be fine. Unless I missed something of course. :-)
>>>>
>>>
>>> CMD0 is a reset. I expect with power OFF notify enable, the eMMC
>>> device will postpone some control information update to its internal
>>> non-volatile memory (e.g. some data structures which are kept in the
>>> controller buffers and not stored in NAND). If we do a CMD0, then the
>>> eMMC device will be reset and we may lose some data. In addition to
>>> that, doing complete card initialization will increase the wakeup time
>>> (for 4.4 devices).

When doing poweroff_notify at suspend you have _always_ cut both vcc and 
vccq, according to MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND, which means 
you must always use CMD0 to wake up. There is no present internal cache 
in the eMMC here.

In sleep mode, you can use CMD5, but until the poweroff notify patches 
(the patch that broke suspend/resume, not this one), we have used CMD0 
to wake up. Let's go back to that solution. Then we can address you 
concern about "data loss" for sleep mode in separate patch.

>>>
>>> Till now, we have done complete card initialization during resume
>> Yes, me and Ulf think we should still do a complete initialization, at
>> least for now and in this patch.
>>
>
> In my opinion, that's incorrect on MMC-4.5 device and unoptimized for
> MMC-4.41 device.

Unoptimized for 4.41 with sleep, might be correct. But, again, let's 
look into that in a second step.

As stated for 4.5 devices with poweroff_notify, there are no issues.

>
> Let me propose a new cap, MMC_CAP2_NO_INIT_ON_RESUME and do something
> like following in mmc_resume,
>
>   	mmc_claim_host(host);
> -	if (mmc_card_is_sleep(host->card)) {
> +	if (mmc_card_is_sleep(host->card)&&
> +		(host->caps2&  MMC_CAP2_NO_INIT_ON_RESUME)) {
>   		mmc_restore_ios(host,&host->saved_ios);
>   		err = mmc_card_awake(host);
> 	} else
> 		err = mmc_init_card(host, host->ocr, host->card);
>
> I hope it's OK for Ulf, Per, Subhash, Girish, Asutosh.
>
>
>> A separate patch may deal with resume awake CMD5 and IOS save/restore.
>>
>> We may also discuss a clean up patch later on to reduce the number of
>> bus_ops. Sleep, awake, and poweroff_notify are MMC specific.
>> power_save/power_restore maps to suspend/resume. But let's not discuss
>> this now :)
>>
>> BR
>> Per


Kind regards
Ulf Hansson

Saugata Das June 15, 2012, 11:34 a.m. UTC | #18

On 15 June 2012 16:56, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> Hi Saugata,
>
> snip.
>
>
>>>>>>
>>>>>> The problem in sending CMD0 without power OFF notify is possibility of
>>>>>> some data loss in MMC-4.5 devices.
>>>>>
>>>>>
>>>>>
>>>>> I don't see the problem here. You will be sending power OFF notify when
>>>>> you
>>>>> can. The only difference is when you "wake" the device from sleep mode.
>>>>> Instead of using CMD5, which may work is some cases and in some cases
>>>>> not
>>>>> (without restoring ios). So using CMD0 as common way of waking up from
>>>>> suspend should be fine. Unless I missed something of course. :-)
>>>>>
>>>>
>>>> CMD0 is a reset. I expect with power OFF notify enable, the eMMC
>>>> device will postpone some control information update to its internal
>>>> non-volatile memory (e.g. some data structures which are kept in the
>>>> controller buffers and not stored in NAND). If we do a CMD0, then the
>>>> eMMC device will be reset and we may lose some data. In addition to
>>>> that, doing complete card initialization will increase the wakeup time
>>>> (for 4.4 devices).
>
>
> When doing poweroff_notify at suspend you have _always_ cut both vcc and
> vccq, according to MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND, which means you
> must always use CMD0 to wake up. There is no present internal cache in the
> eMMC here.
>
> In sleep mode, you can use CMD5, but until the poweroff notify patches (the
> patch that broke suspend/resume, not this one), we have used CMD0 to wake
> up. Let's go back to that solution. Then we can address you concern about
> "data loss" for sleep mode in separate patch.
>

Yes, you will get back to the same code flow with the introduction of
the MMC_CAP2_NO_INIT_ON_RESUME. If some host drivers are capable of
executing the CMD5 resume then they enable this cap and go on the
optimized path. The rest go on CMD0 path.

>
>>>>
>>>> Till now, we have done complete card initialization during resume
>>>
>>> Yes, me and Ulf think we should still do a complete initialization, at
>>> least for now and in this patch.
>>>
>>
>> In my opinion, that's incorrect on MMC-4.5 device and unoptimized for
>> MMC-4.41 device.
>
>
> Unoptimized for 4.41 with sleep, might be correct. But, again, let's look
> into that in a second step.
>
> As stated for 4.5 devices with poweroff_notify, there are no issues.
>
>>
>> Let me propose a new cap, MMC_CAP2_NO_INIT_ON_RESUME and do something
>> like following in mmc_resume,
>>
>>        mmc_claim_host(host);
>> -       if (mmc_card_is_sleep(host->card)) {
>> +       if (mmc_card_is_sleep(host->card)&&
>> +               (host->caps2&  MMC_CAP2_NO_INIT_ON_RESUME)) {
>>                mmc_restore_ios(host,&host->saved_ios);
>>
>>                err = mmc_card_awake(host);
>>        } else
>>                err = mmc_init_card(host, host->ocr, host->card);
>>
>> I hope it's OK for Ulf, Per, Subhash, Girish, Asutosh.
>>
>>
>>> A separate patch may deal with resume awake CMD5 and IOS save/restore.
>>>
>>> We may also discuss a clean up patch later on to reduce the number of
>>> bus_ops. Sleep, awake, and poweroff_notify are MMC specific.
>>> power_save/power_restore maps to suspend/resume. But let's not discuss
>>> this now :)
>>>
>>> BR
>>> Per
>
>
>
> Kind regards
> Ulf Hansson

Ulf Hansson June 15, 2012, 12:07 p.m. UTC | #19

On 06/15/2012 01:34 PM, Saugata Das wrote:
> On 15 June 2012 16:56, Ulf Hansson<ulf.hansson@stericsson.com>  wrote:
>> Hi Saugata,
>>
>> snip.
>>
>>
>>>>>>>
>>>>>>> The problem in sending CMD0 without power OFF notify is possibility of
>>>>>>> some data loss in MMC-4.5 devices.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I don't see the problem here. You will be sending power OFF notify when
>>>>>> you
>>>>>> can. The only difference is when you "wake" the device from sleep mode.
>>>>>> Instead of using CMD5, which may work is some cases and in some cases
>>>>>> not
>>>>>> (without restoring ios). So using CMD0 as common way of waking up from
>>>>>> suspend should be fine. Unless I missed something of course. :-)
>>>>>>
>>>>>
>>>>> CMD0 is a reset. I expect with power OFF notify enable, the eMMC
>>>>> device will postpone some control information update to its internal
>>>>> non-volatile memory (e.g. some data structures which are kept in the
>>>>> controller buffers and not stored in NAND). If we do a CMD0, then the
>>>>> eMMC device will be reset and we may lose some data. In addition to
>>>>> that, doing complete card initialization will increase the wakeup time
>>>>> (for 4.4 devices).
>>
>>
>> When doing poweroff_notify at suspend you have _always_ cut both vcc and
>> vccq, according to MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND, which means you
>> must always use CMD0 to wake up. There is no present internal cache in the
>> eMMC here.
>>
>> In sleep mode, you can use CMD5, but until the poweroff notify patches (the
>> patch that broke suspend/resume, not this one), we have used CMD0 to wake
>> up. Let's go back to that solution. Then we can address you concern about
>> "data loss" for sleep mode in separate patch.
>>
>
> Yes, you will get back to the same code flow with the introduction of
> the MMC_CAP2_NO_INIT_ON_RESUME. If some host drivers are capable of
> executing the CMD5 resume then they enable this cap and go on the
> optimized path. The rest go on CMD0 path.

Please do that as a separate patch so we can get this merged asap. 
Moreover, I think we should try to prevent from adding another cap for this.

There are another option for a host driver tell whether CMD0 shall be 
used or not, by using the regulator supplies. See a patch by Guennadi 
Liakhovetski in: http://article.gmane.org/gmane.linux.kernel.mmc/14635, 
still being discussed though.

The MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND, can likely soon be removed 
as well, when above patch is accepted and host driver is starting to use 
the new API.

Kind regards
Ulf Hansson

[v5] MMC-4.5 Power OFF Notify Rework

Commit Message

Comments

Patch