* [PATCH V3 1/4] mmc: core: Stop bkops for eMMC only from mmc suspend
2013-04-16 10:00 [PATCH V3 0/4] mmc: Use runtime pm for blkdevice Ulf Hansson
@ 2013-04-16 10:00 ` Ulf Hansson
2013-04-18 7:17 ` Jaehoon Chung
2013-04-16 10:00 ` [PATCH V3 2/4] mmc: core: Add bus_ops for runtime pm callbacks Ulf Hansson
` (2 subsequent siblings)
3 siblings, 1 reply; 21+ messages in thread
From: Ulf Hansson @ 2013-04-16 10:00 UTC (permalink / raw)
To: linux-mmc, Chris Ball
Cc: Ulf Hansson, Maya Erez, Subhash Jadavani, Arnd Bergmann,
Kevin Liu, Adrian Hunter, Daniel Drake, Ohad Ben-Cohen
From: Ulf Hansson <ulf.hansson@linaro.org>
Move mmc suspend specific operations to be executed from the .suspend
callback in the mmc bus_ops. This simplifies the mmc_suspend_host
function which is supposed to handle nothing but common suspend tasks.
Since eMMC can be considered non-removable there are no need to check
for ongoing bkops at PM_SUSPEND_PREPARE notification so remove it.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Maya Erez <merez@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Kevin Liu <kliu5@marvell.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Daniel Drake <dsd@laptop.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>
---
drivers/mmc/core/core.c | 22 +---------------------
drivers/mmc/core/mmc.c | 6 ++++++
2 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index ad7decc..f001097 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2631,14 +2631,8 @@ int mmc_suspend_host(struct mmc_host *host)
mmc_bus_get(host);
if (host->bus_ops && !host->bus_dead) {
- if (host->bus_ops->suspend) {
- if (mmc_card_doing_bkops(host->card)) {
- err = mmc_stop_bkops(host->card);
- if (err)
- goto out;
- }
+ if (host->bus_ops->suspend)
err = host->bus_ops->suspend(host);
- }
if (err == -ENOSYS || !host->bus_ops->resume) {
/*
@@ -2662,10 +2656,8 @@ int mmc_suspend_host(struct mmc_host *host)
if (!err && !mmc_card_keep_power(host))
mmc_power_off(host);
-out:
return err;
}
-
EXPORT_SYMBOL(mmc_suspend_host);
/**
@@ -2720,22 +2712,10 @@ int mmc_pm_notify(struct notifier_block *notify_block,
struct mmc_host *host = container_of(
notify_block, struct mmc_host, pm_notify);
unsigned long flags;
- int err = 0;
switch (mode) {
case PM_HIBERNATION_PREPARE:
case PM_SUSPEND_PREPARE:
- if (host->card && mmc_card_mmc(host->card) &&
- mmc_card_doing_bkops(host->card)) {
- err = mmc_stop_bkops(host->card);
- if (err) {
- pr_err("%s: didn't stop bkops\n",
- mmc_hostname(host));
- return err;
- }
- mmc_card_clr_doing_bkops(host->card);
- }
-
spin_lock_irqsave(&host->lock, flags);
host->rescan_disable = 1;
spin_unlock_irqrestore(&host->lock, flags);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index d584f7c..66a530e 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1411,6 +1411,12 @@ static int mmc_suspend(struct mmc_host *host)
mmc_claim_host(host);
+ if (mmc_card_doing_bkops(host->card)) {
+ err = mmc_stop_bkops(host->card);
+ if (err)
+ goto out;
+ }
+
err = mmc_cache_ctrl(host, 0);
if (err)
goto out;
--
1.7.10
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V3 1/4] mmc: core: Stop bkops for eMMC only from mmc suspend
2013-04-16 10:00 ` [PATCH V3 1/4] mmc: core: Stop bkops for eMMC only from mmc suspend Ulf Hansson
@ 2013-04-18 7:17 ` Jaehoon Chung
0 siblings, 0 replies; 21+ messages in thread
From: Jaehoon Chung @ 2013-04-18 7:17 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Chris Ball, Ulf Hansson, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Adrian Hunter, Daniel Drake,
Ohad Ben-Cohen
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
On 04/16/2013 07:00 PM, Ulf Hansson wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> Move mmc suspend specific operations to be executed from the .suspend
> callback in the mmc bus_ops. This simplifies the mmc_suspend_host
> function which is supposed to handle nothing but common suspend tasks.
>
> Since eMMC can be considered non-removable there are no need to check
> for ongoing bkops at PM_SUSPEND_PREPARE notification so remove it.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Maya Erez <merez@codeaurora.org>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kevin Liu <kliu5@marvell.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Daniel Drake <dsd@laptop.org>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> ---
> drivers/mmc/core/core.c | 22 +---------------------
> drivers/mmc/core/mmc.c | 6 ++++++
> 2 files changed, 7 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index ad7decc..f001097 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2631,14 +2631,8 @@ int mmc_suspend_host(struct mmc_host *host)
>
> mmc_bus_get(host);
> if (host->bus_ops && !host->bus_dead) {
> - if (host->bus_ops->suspend) {
> - if (mmc_card_doing_bkops(host->card)) {
> - err = mmc_stop_bkops(host->card);
> - if (err)
> - goto out;
> - }
> + if (host->bus_ops->suspend)
> err = host->bus_ops->suspend(host);
> - }
>
> if (err == -ENOSYS || !host->bus_ops->resume) {
> /*
> @@ -2662,10 +2656,8 @@ int mmc_suspend_host(struct mmc_host *host)
> if (!err && !mmc_card_keep_power(host))
> mmc_power_off(host);
>
> -out:
> return err;
> }
> -
> EXPORT_SYMBOL(mmc_suspend_host);
>
> /**
> @@ -2720,22 +2712,10 @@ int mmc_pm_notify(struct notifier_block *notify_block,
> struct mmc_host *host = container_of(
> notify_block, struct mmc_host, pm_notify);
> unsigned long flags;
> - int err = 0;
>
> switch (mode) {
> case PM_HIBERNATION_PREPARE:
> case PM_SUSPEND_PREPARE:
> - if (host->card && mmc_card_mmc(host->card) &&
> - mmc_card_doing_bkops(host->card)) {
> - err = mmc_stop_bkops(host->card);
> - if (err) {
> - pr_err("%s: didn't stop bkops\n",
> - mmc_hostname(host));
> - return err;
> - }
> - mmc_card_clr_doing_bkops(host->card);
> - }
> -
> spin_lock_irqsave(&host->lock, flags);
> host->rescan_disable = 1;
> spin_unlock_irqrestore(&host->lock, flags);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index d584f7c..66a530e 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1411,6 +1411,12 @@ static int mmc_suspend(struct mmc_host *host)
>
> mmc_claim_host(host);
>
> + if (mmc_card_doing_bkops(host->card)) {
> + err = mmc_stop_bkops(host->card);
> + if (err)
> + goto out;
> + }
> +
> err = mmc_cache_ctrl(host, 0);
> if (err)
> goto out;
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V3 2/4] mmc: core: Add bus_ops for runtime pm callbacks
2013-04-16 10:00 [PATCH V3 0/4] mmc: Use runtime pm for blkdevice Ulf Hansson
2013-04-16 10:00 ` [PATCH V3 1/4] mmc: core: Stop bkops for eMMC only from mmc suspend Ulf Hansson
@ 2013-04-16 10:00 ` Ulf Hansson
2013-04-26 13:11 ` Adrian Hunter
2013-04-16 10:00 ` [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice Ulf Hansson
2013-04-16 10:00 ` [PATCH V3 4/4] mmc: core: Support aggressive power management for (e)MMC/SD Ulf Hansson
3 siblings, 1 reply; 21+ messages in thread
From: Ulf Hansson @ 2013-04-16 10:00 UTC (permalink / raw)
To: linux-mmc, Chris Ball
Cc: Ulf Hansson, Maya Erez, Subhash Jadavani, Arnd Bergmann,
Kevin Liu, Adrian Hunter, Daniel Drake, Ohad Ben-Cohen
From: Ulf Hansson <ulf.hansson@linaro.org>
SDIO is the only protocol that uses runtime pm for the card device
right now. To provide the option for sd and mmc to use runtime pm as
well the bus_ops callback are extended with two new functions. One for
runtime_suspend and one for runtime_resume.
This patch will also implement the callbacks for SDIO to make sure
existing functionality is maintained. It also prepares to move
away from using the mmc_power_restore_host API, since it is not
needed when using runtime PM.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Maya Erez <merez@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Kevin Liu <kliu5@marvell.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Daniel Drake <dsd@laptop.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>
---
drivers/mmc/core/bus.c | 14 ++++++++++++--
drivers/mmc/core/core.c | 2 +-
drivers/mmc/core/core.h | 3 +++
drivers/mmc/core/sdio.c | 16 ++++++++++++++++
4 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index e219c97..d9e8c2b 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -151,15 +151,25 @@ static int mmc_bus_resume(struct device *dev)
static int mmc_runtime_suspend(struct device *dev)
{
struct mmc_card *card = mmc_dev_to_card(dev);
+ struct mmc_host *host = card->host;
+ int ret = 0;
+
+ if (host->bus_ops->runtime_suspend)
+ ret = host->bus_ops->runtime_suspend(host);
- return mmc_power_save_host(card->host);
+ return ret;
}
static int mmc_runtime_resume(struct device *dev)
{
struct mmc_card *card = mmc_dev_to_card(dev);
+ struct mmc_host *host = card->host;
+ int ret = 0;
+
+ if (host->bus_ops->runtime_resume)
+ ret = host->bus_ops->runtime_resume(host);
- return mmc_power_restore_host(card->host);
+ return ret;
}
static int mmc_runtime_idle(struct device *dev)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index f001097..b16b64a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1459,7 +1459,7 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
* If a host does all the power sequencing itself, ignore the
* initial MMC_POWER_UP stage.
*/
-static void mmc_power_up(struct mmc_host *host)
+void mmc_power_up(struct mmc_host *host)
{
int bit;
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index b9f18a2..6242ffb 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -22,6 +22,8 @@ struct mmc_bus_ops {
void (*detect)(struct mmc_host *);
int (*suspend)(struct mmc_host *);
int (*resume)(struct mmc_host *);
+ int (*runtime_suspend)(struct mmc_host *);
+ int (*runtime_resume)(struct mmc_host *);
int (*power_save)(struct mmc_host *);
int (*power_restore)(struct mmc_host *);
int (*alive)(struct mmc_host *);
@@ -44,6 +46,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
int __mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
void mmc_set_timing(struct mmc_host *host, unsigned int timing);
void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
+void mmc_power_up(struct mmc_host *host);
void mmc_power_off(struct mmc_host *host);
void mmc_power_cycle(struct mmc_host *host);
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index aa0719a..09428c7 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1049,11 +1049,27 @@ out:
return ret;
}
+static int mmc_sdio_runtime_suspend(struct mmc_host *host)
+{
+ /* No references to the card, cut the power to it. */
+ mmc_power_off(host);
+ return 0;
+}
+
+static int mmc_sdio_runtime_resume(struct mmc_host *host)
+{
+ /* Restore power and re-initialize. */
+ mmc_power_up(host);
+ return mmc_sdio_power_restore(host);
+}
+
static const struct mmc_bus_ops mmc_sdio_ops = {
.remove = mmc_sdio_remove,
.detect = mmc_sdio_detect,
.suspend = mmc_sdio_suspend,
.resume = mmc_sdio_resume,
+ .runtime_suspend = mmc_sdio_runtime_suspend,
+ .runtime_resume = mmc_sdio_runtime_resume,
.power_restore = mmc_sdio_power_restore,
.alive = mmc_sdio_alive,
};
--
1.7.10
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V3 2/4] mmc: core: Add bus_ops for runtime pm callbacks
2013-04-16 10:00 ` [PATCH V3 2/4] mmc: core: Add bus_ops for runtime pm callbacks Ulf Hansson
@ 2013-04-26 13:11 ` Adrian Hunter
2013-04-29 7:54 ` Adrian Hunter
0 siblings, 1 reply; 21+ messages in thread
From: Adrian Hunter @ 2013-04-26 13:11 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Chris Ball, Ulf Hansson, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Daniel Drake, Ohad Ben-Cohen
On 16/04/13 13:00, Ulf Hansson wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> SDIO is the only protocol that uses runtime pm for the card device
> right now. To provide the option for sd and mmc to use runtime pm as
> well the bus_ops callback are extended with two new functions. One for
> runtime_suspend and one for runtime_resume.
>
> This patch will also implement the callbacks for SDIO to make sure
> existing functionality is maintained. It also prepares to move
> away from using the mmc_power_restore_host API, since it is not
> needed when using runtime PM.
You have removed the bus_ops checks without explanation. Are you sure it is
OK to do so? I noticed that, in the SDIO case, runtime pm is not disabled
before detaching the bus making it hard to know if host->bus_ops can
disappear catastrophically during a runtime pm callback.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Maya Erez <merez@codeaurora.org>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kevin Liu <kliu5@marvell.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Daniel Drake <dsd@laptop.org>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> ---
> drivers/mmc/core/bus.c | 14 ++++++++++++--
> drivers/mmc/core/core.c | 2 +-
> drivers/mmc/core/core.h | 3 +++
> drivers/mmc/core/sdio.c | 16 ++++++++++++++++
> 4 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index e219c97..d9e8c2b 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -151,15 +151,25 @@ static int mmc_bus_resume(struct device *dev)
> static int mmc_runtime_suspend(struct device *dev)
> {
> struct mmc_card *card = mmc_dev_to_card(dev);
> + struct mmc_host *host = card->host;
> + int ret = 0;
> +
> + if (host->bus_ops->runtime_suspend)
> + ret = host->bus_ops->runtime_suspend(host);
>
> - return mmc_power_save_host(card->host);
> + return ret;
> }
>
> static int mmc_runtime_resume(struct device *dev)
> {
> struct mmc_card *card = mmc_dev_to_card(dev);
> + struct mmc_host *host = card->host;
> + int ret = 0;
> +
> + if (host->bus_ops->runtime_resume)
> + ret = host->bus_ops->runtime_resume(host);
>
> - return mmc_power_restore_host(card->host);
> + return ret;
> }
>
> static int mmc_runtime_idle(struct device *dev)
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index f001097..b16b64a 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1459,7 +1459,7 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
> * If a host does all the power sequencing itself, ignore the
> * initial MMC_POWER_UP stage.
> */
> -static void mmc_power_up(struct mmc_host *host)
> +void mmc_power_up(struct mmc_host *host)
> {
> int bit;
>
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index b9f18a2..6242ffb 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -22,6 +22,8 @@ struct mmc_bus_ops {
> void (*detect)(struct mmc_host *);
> int (*suspend)(struct mmc_host *);
> int (*resume)(struct mmc_host *);
> + int (*runtime_suspend)(struct mmc_host *);
> + int (*runtime_resume)(struct mmc_host *);
> int (*power_save)(struct mmc_host *);
> int (*power_restore)(struct mmc_host *);
> int (*alive)(struct mmc_host *);
> @@ -44,6 +46,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
> int __mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
> void mmc_set_timing(struct mmc_host *host, unsigned int timing);
> void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
> +void mmc_power_up(struct mmc_host *host);
> void mmc_power_off(struct mmc_host *host);
> void mmc_power_cycle(struct mmc_host *host);
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index aa0719a..09428c7 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -1049,11 +1049,27 @@ out:
> return ret;
> }
>
> +static int mmc_sdio_runtime_suspend(struct mmc_host *host)
> +{
> + /* No references to the card, cut the power to it. */
> + mmc_power_off(host);
> + return 0;
> +}
> +
> +static int mmc_sdio_runtime_resume(struct mmc_host *host)
> +{
> + /* Restore power and re-initialize. */
> + mmc_power_up(host);
> + return mmc_sdio_power_restore(host);
> +}
> +
> static const struct mmc_bus_ops mmc_sdio_ops = {
> .remove = mmc_sdio_remove,
> .detect = mmc_sdio_detect,
> .suspend = mmc_sdio_suspend,
> .resume = mmc_sdio_resume,
> + .runtime_suspend = mmc_sdio_runtime_suspend,
> + .runtime_resume = mmc_sdio_runtime_resume,
> .power_restore = mmc_sdio_power_restore,
> .alive = mmc_sdio_alive,
> };
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 2/4] mmc: core: Add bus_ops for runtime pm callbacks
2013-04-26 13:11 ` Adrian Hunter
@ 2013-04-29 7:54 ` Adrian Hunter
2013-04-29 13:42 ` Ulf Hansson
0 siblings, 1 reply; 21+ messages in thread
From: Adrian Hunter @ 2013-04-29 7:54 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Chris Ball, Ulf Hansson, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Daniel Drake, Ohad Ben-Cohen
On 26/04/13 16:11, Adrian Hunter wrote:
> On 16/04/13 13:00, Ulf Hansson wrote:
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> SDIO is the only protocol that uses runtime pm for the card device
>> right now. To provide the option for sd and mmc to use runtime pm as
>> well the bus_ops callback are extended with two new functions. One for
>> runtime_suspend and one for runtime_resume.
>>
>> This patch will also implement the callbacks for SDIO to make sure
>> existing functionality is maintained. It also prepares to move
>> away from using the mmc_power_restore_host API, since it is not
>> needed when using runtime PM.
>
> You have removed the bus_ops checks without explanation. Are you sure it is
> OK to do so? I noticed that, in the SDIO case, runtime pm is not disabled
> before detaching the bus making it hard to know if host->bus_ops can
> disappear catastrophically during a runtime pm callback.
Looking at this again - it seems OK. bus_ops are only detached after
host->bus_ops->remove is called which deletes the device (device_del)
disabling runtime pm.
>
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Maya Erez <merez@codeaurora.org>
>> Cc: Subhash Jadavani <subhashj@codeaurora.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Kevin Liu <kliu5@marvell.com>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Daniel Drake <dsd@laptop.org>
>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>> ---
>> drivers/mmc/core/bus.c | 14 ++++++++++++--
>> drivers/mmc/core/core.c | 2 +-
>> drivers/mmc/core/core.h | 3 +++
>> drivers/mmc/core/sdio.c | 16 ++++++++++++++++
>> 4 files changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index e219c97..d9e8c2b 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -151,15 +151,25 @@ static int mmc_bus_resume(struct device *dev)
>> static int mmc_runtime_suspend(struct device *dev)
>> {
>> struct mmc_card *card = mmc_dev_to_card(dev);
>> + struct mmc_host *host = card->host;
>> + int ret = 0;
>> +
>> + if (host->bus_ops->runtime_suspend)
>> + ret = host->bus_ops->runtime_suspend(host);
>>
>> - return mmc_power_save_host(card->host);
>> + return ret;
>> }
>>
>> static int mmc_runtime_resume(struct device *dev)
>> {
>> struct mmc_card *card = mmc_dev_to_card(dev);
>> + struct mmc_host *host = card->host;
>> + int ret = 0;
>> +
>> + if (host->bus_ops->runtime_resume)
>> + ret = host->bus_ops->runtime_resume(host);
>>
>> - return mmc_power_restore_host(card->host);
>> + return ret;
>> }
>>
>> static int mmc_runtime_idle(struct device *dev)
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index f001097..b16b64a 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1459,7 +1459,7 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
>> * If a host does all the power sequencing itself, ignore the
>> * initial MMC_POWER_UP stage.
>> */
>> -static void mmc_power_up(struct mmc_host *host)
>> +void mmc_power_up(struct mmc_host *host)
>> {
>> int bit;
>>
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> index b9f18a2..6242ffb 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -22,6 +22,8 @@ struct mmc_bus_ops {
>> void (*detect)(struct mmc_host *);
>> int (*suspend)(struct mmc_host *);
>> int (*resume)(struct mmc_host *);
>> + int (*runtime_suspend)(struct mmc_host *);
>> + int (*runtime_resume)(struct mmc_host *);
>> int (*power_save)(struct mmc_host *);
>> int (*power_restore)(struct mmc_host *);
>> int (*alive)(struct mmc_host *);
>> @@ -44,6 +46,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
>> int __mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
>> void mmc_set_timing(struct mmc_host *host, unsigned int timing);
>> void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
>> +void mmc_power_up(struct mmc_host *host);
>> void mmc_power_off(struct mmc_host *host);
>> void mmc_power_cycle(struct mmc_host *host);
>>
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index aa0719a..09428c7 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -1049,11 +1049,27 @@ out:
>> return ret;
>> }
>>
>> +static int mmc_sdio_runtime_suspend(struct mmc_host *host)
>> +{
>> + /* No references to the card, cut the power to it. */
>> + mmc_power_off(host);
>> + return 0;
>> +}
>> +
>> +static int mmc_sdio_runtime_resume(struct mmc_host *host)
>> +{
>> + /* Restore power and re-initialize. */
>> + mmc_power_up(host);
>> + return mmc_sdio_power_restore(host);
>> +}
>> +
>> static const struct mmc_bus_ops mmc_sdio_ops = {
>> .remove = mmc_sdio_remove,
>> .detect = mmc_sdio_detect,
>> .suspend = mmc_sdio_suspend,
>> .resume = mmc_sdio_resume,
>> + .runtime_suspend = mmc_sdio_runtime_suspend,
>> + .runtime_resume = mmc_sdio_runtime_resume,
>> .power_restore = mmc_sdio_power_restore,
>> .alive = mmc_sdio_alive,
>> };
>>
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 2/4] mmc: core: Add bus_ops for runtime pm callbacks
2013-04-29 7:54 ` Adrian Hunter
@ 2013-04-29 13:42 ` Ulf Hansson
0 siblings, 0 replies; 21+ messages in thread
From: Ulf Hansson @ 2013-04-29 13:42 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Chris Ball, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Daniel Drake, Ohad Ben-Cohen
On 29 April 2013 09:54, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 26/04/13 16:11, Adrian Hunter wrote:
>> On 16/04/13 13:00, Ulf Hansson wrote:
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> SDIO is the only protocol that uses runtime pm for the card device
>>> right now. To provide the option for sd and mmc to use runtime pm as
>>> well the bus_ops callback are extended with two new functions. One for
>>> runtime_suspend and one for runtime_resume.
>>>
>>> This patch will also implement the callbacks for SDIO to make sure
>>> existing functionality is maintained. It also prepares to move
>>> away from using the mmc_power_restore_host API, since it is not
>>> needed when using runtime PM.
>>
>> You have removed the bus_ops checks without explanation. Are you sure it is
>> OK to do so? I noticed that, in the SDIO case, runtime pm is not disabled
>> before detaching the bus making it hard to know if host->bus_ops can
>> disappear catastrophically during a runtime pm callback.
>
> Looking at this again - it seems OK. bus_ops are only detached after
> host->bus_ops->remove is called which deletes the device (device_del)
> disabling runtime pm.
You are correct. Thanks for reviewing Adrian.
Do you have any other thoughts on the rest of the patches in this patchset?
Kind regards
Ulf Hansson
>
>>
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Cc: Maya Erez <merez@codeaurora.org>
>>> Cc: Subhash Jadavani <subhashj@codeaurora.org>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Kevin Liu <kliu5@marvell.com>
>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>> Cc: Daniel Drake <dsd@laptop.org>
>>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>>> ---
>>> drivers/mmc/core/bus.c | 14 ++++++++++++--
>>> drivers/mmc/core/core.c | 2 +-
>>> drivers/mmc/core/core.h | 3 +++
>>> drivers/mmc/core/sdio.c | 16 ++++++++++++++++
>>> 4 files changed, 32 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>>> index e219c97..d9e8c2b 100644
>>> --- a/drivers/mmc/core/bus.c
>>> +++ b/drivers/mmc/core/bus.c
>>> @@ -151,15 +151,25 @@ static int mmc_bus_resume(struct device *dev)
>>> static int mmc_runtime_suspend(struct device *dev)
>>> {
>>> struct mmc_card *card = mmc_dev_to_card(dev);
>>> + struct mmc_host *host = card->host;
>>> + int ret = 0;
>>> +
>>> + if (host->bus_ops->runtime_suspend)
>>> + ret = host->bus_ops->runtime_suspend(host);
>>>
>>> - return mmc_power_save_host(card->host);
>>> + return ret;
>>> }
>>>
>>> static int mmc_runtime_resume(struct device *dev)
>>> {
>>> struct mmc_card *card = mmc_dev_to_card(dev);
>>> + struct mmc_host *host = card->host;
>>> + int ret = 0;
>>> +
>>> + if (host->bus_ops->runtime_resume)
>>> + ret = host->bus_ops->runtime_resume(host);
>>>
>>> - return mmc_power_restore_host(card->host);
>>> + return ret;
>>> }
>>>
>>> static int mmc_runtime_idle(struct device *dev)
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index f001097..b16b64a 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1459,7 +1459,7 @@ void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
>>> * If a host does all the power sequencing itself, ignore the
>>> * initial MMC_POWER_UP stage.
>>> */
>>> -static void mmc_power_up(struct mmc_host *host)
>>> +void mmc_power_up(struct mmc_host *host)
>>> {
>>> int bit;
>>>
>>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>>> index b9f18a2..6242ffb 100644
>>> --- a/drivers/mmc/core/core.h
>>> +++ b/drivers/mmc/core/core.h
>>> @@ -22,6 +22,8 @@ struct mmc_bus_ops {
>>> void (*detect)(struct mmc_host *);
>>> int (*suspend)(struct mmc_host *);
>>> int (*resume)(struct mmc_host *);
>>> + int (*runtime_suspend)(struct mmc_host *);
>>> + int (*runtime_resume)(struct mmc_host *);
>>> int (*power_save)(struct mmc_host *);
>>> int (*power_restore)(struct mmc_host *);
>>> int (*alive)(struct mmc_host *);
>>> @@ -44,6 +46,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
>>> int __mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage);
>>> void mmc_set_timing(struct mmc_host *host, unsigned int timing);
>>> void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type);
>>> +void mmc_power_up(struct mmc_host *host);
>>> void mmc_power_off(struct mmc_host *host);
>>> void mmc_power_cycle(struct mmc_host *host);
>>>
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>> index aa0719a..09428c7 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -1049,11 +1049,27 @@ out:
>>> return ret;
>>> }
>>>
>>> +static int mmc_sdio_runtime_suspend(struct mmc_host *host)
>>> +{
>>> + /* No references to the card, cut the power to it. */
>>> + mmc_power_off(host);
>>> + return 0;
>>> +}
>>> +
>>> +static int mmc_sdio_runtime_resume(struct mmc_host *host)
>>> +{
>>> + /* Restore power and re-initialize. */
>>> + mmc_power_up(host);
>>> + return mmc_sdio_power_restore(host);
>>> +}
>>> +
>>> static const struct mmc_bus_ops mmc_sdio_ops = {
>>> .remove = mmc_sdio_remove,
>>> .detect = mmc_sdio_detect,
>>> .suspend = mmc_sdio_suspend,
>>> .resume = mmc_sdio_resume,
>>> + .runtime_suspend = mmc_sdio_runtime_suspend,
>>> + .runtime_resume = mmc_sdio_runtime_resume,
>>> .power_restore = mmc_sdio_power_restore,
>>> .alive = mmc_sdio_alive,
>>> };
>>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice
2013-04-16 10:00 [PATCH V3 0/4] mmc: Use runtime pm for blkdevice Ulf Hansson
2013-04-16 10:00 ` [PATCH V3 1/4] mmc: core: Stop bkops for eMMC only from mmc suspend Ulf Hansson
2013-04-16 10:00 ` [PATCH V3 2/4] mmc: core: Add bus_ops for runtime pm callbacks Ulf Hansson
@ 2013-04-16 10:00 ` Ulf Hansson
2013-05-02 8:58 ` Adrian Hunter
[not found] ` <CAMj5BkiOmh8sz-=b0z1VF9owGPX0KpbZeNfPzETemCb=C2odGQ@mail.gmail.com>
2013-04-16 10:00 ` [PATCH V3 4/4] mmc: core: Support aggressive power management for (e)MMC/SD Ulf Hansson
3 siblings, 2 replies; 21+ messages in thread
From: Ulf Hansson @ 2013-04-16 10:00 UTC (permalink / raw)
To: linux-mmc, Chris Ball
Cc: Ulf Hansson, Maya Erez, Subhash Jadavani, Arnd Bergmann,
Kevin Liu, Adrian Hunter, Daniel Drake, Ohad Ben-Cohen
From: Ulf Hansson <ulf.hansson@linaro.org>
Once the mmc blkdevice is being probed, runtime pm will be enabled.
By using runtime autosuspend, the power save operations can be done
when request inactivity occurs for a certain time. Right now the
selected timeout value is set to 3 s. Obviously this value will likely
need to be configurable somehow since it needs to be trimmed depending
on the power save algorithm.
For SD-combo cards, we are still leaving the enablement of runtime PM
to the SDIO init sequence since it depends on the capabilities of the
SDIO func driver.
Moreover, when the blk device is being suspended, we make sure the device
will be runtime resumed. The reason for doing this is that we want the
host suspend sequence to be unaware of any runtime power save operations
done for the card in this phase. Thus it can just handle the suspend as
the card is fully powered from a runtime perspective.
Finally, this patch prepares to make it possible to move BKOPS handling
into the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
accomplished.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Maya Erez <merez@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Kevin Liu <kliu5@marvell.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Daniel Drake <dsd@laptop.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>
---
drivers/mmc/card/block.c | 32 ++++++++++++++++++++++++++------
drivers/mmc/core/core.c | 23 +++++++++++++++++++++++
drivers/mmc/core/debugfs.c | 8 ++++----
drivers/mmc/core/mmc.c | 4 ++--
drivers/mmc/core/sd.c | 4 ++--
include/linux/mmc/core.h | 3 +++
6 files changed, 60 insertions(+), 14 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index e12a03c..360a41a 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -34,6 +34,7 @@
#include <linux/delay.h>
#include <linux/capability.h>
#include <linux/compat.h>
+#include <linux/pm_runtime.h>
#include <linux/mmc/ioctl.h>
#include <linux/mmc/card.h>
@@ -222,7 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
md = mmc_blk_get(dev_to_disk(dev));
card = md->queue.card;
- mmc_claim_host(card->host);
+ mmc_get_card(card);
ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
card->ext_csd.boot_ro_lock |
@@ -233,7 +234,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
else
card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
- mmc_release_host(card->host);
+ mmc_put_card(card);
if (!ret) {
pr_info("%s: Locking boot partition ro until next power on\n",
@@ -492,7 +493,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
mrq.cmd = &cmd;
- mmc_claim_host(card->host);
+ mmc_get_card(card);
err = mmc_blk_part_switch(card, md);
if (err)
@@ -559,7 +560,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
}
cmd_rel_host:
- mmc_release_host(card->host);
+ mmc_put_card(card);
cmd_done:
mmc_blk_put(md);
@@ -1896,7 +1897,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
if (req && !mq->mqrq_prev->req)
/* claim host only for the first request */
- mmc_claim_host(card->host);
+ mmc_get_card(card);
ret = mmc_blk_part_switch(card, md);
if (ret) {
@@ -1940,7 +1941,7 @@ out:
* In case sepecial request, there is no reentry to
* the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
*/
- mmc_release_host(card->host);
+ mmc_put_card(card);
return ret;
}
@@ -2337,6 +2338,19 @@ static int mmc_blk_probe(struct mmc_card *card)
if (mmc_add_disk(part_md))
goto out;
}
+
+ pm_runtime_set_autosuspend_delay(&card->dev, 3000);
+ pm_runtime_use_autosuspend(&card->dev);
+
+ /*
+ * Don't enable runtime PM for SD-combo cards here. Leave that
+ * decision to be taken during the SDIO init sequence instead.
+ */
+ if (card->type != MMC_TYPE_SD_COMBO) {
+ pm_runtime_set_active(&card->dev);
+ pm_runtime_enable(&card->dev);
+ }
+
return 0;
out:
@@ -2350,9 +2364,13 @@ static void mmc_blk_remove(struct mmc_card *card)
struct mmc_blk_data *md = mmc_get_drvdata(card);
mmc_blk_remove_parts(card, md);
+ pm_runtime_get_sync(&card->dev);
mmc_claim_host(card->host);
mmc_blk_part_switch(card, md);
mmc_release_host(card->host);
+ if (card->type != MMC_TYPE_SD_COMBO)
+ pm_runtime_disable(&card->dev);
+ pm_runtime_put_noidle(&card->dev);
mmc_blk_remove_req(md);
mmc_set_drvdata(card, NULL);
}
@@ -2364,6 +2382,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
struct mmc_blk_data *md = mmc_get_drvdata(card);
if (md) {
+ pm_runtime_get_sync(&card->dev);
mmc_queue_suspend(&md->queue);
list_for_each_entry(part_md, &md->part, part) {
mmc_queue_suspend(&part_md->queue);
@@ -2387,6 +2406,7 @@ static int mmc_blk_resume(struct mmc_card *card)
list_for_each_entry(part_md, &md->part, part) {
mmc_queue_resume(&part_md->queue);
}
+ pm_runtime_put(&card->dev);
}
return 0;
}
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b16b64a..602db00 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -952,6 +952,29 @@ void mmc_release_host(struct mmc_host *host)
EXPORT_SYMBOL(mmc_release_host);
/*
+ * This is a helper function, which fetches a runtime pm reference for the
+ * card device and also claims the host.
+ */
+void mmc_get_card(struct mmc_card *card)
+{
+ pm_runtime_get_sync(&card->dev);
+ mmc_claim_host(card->host);
+}
+EXPORT_SYMBOL(mmc_get_card);
+
+/*
+ * This is a helper function, which releases the host and drops the runtime
+ * pm reference for the card device.
+ */
+void mmc_put_card(struct mmc_card *card)
+{
+ mmc_release_host(card->host);
+ pm_runtime_mark_last_busy(&card->dev);
+ pm_runtime_put_autosuspend(&card->dev);
+}
+EXPORT_SYMBOL(mmc_put_card);
+
+/*
* Internal function that does the actual ios call to the host driver,
* optionally printing some debug output.
*/
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 35c2f85..54829c0 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -258,13 +258,13 @@ static int mmc_dbg_card_status_get(void *data, u64 *val)
u32 status;
int ret;
- mmc_claim_host(card->host);
+ mmc_get_card(card);
ret = mmc_send_status(data, &status);
if (!ret)
*val = status;
- mmc_release_host(card->host);
+ mmc_put_card(card);
return ret;
}
@@ -291,9 +291,9 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
goto out_free;
}
- mmc_claim_host(card->host);
+ mmc_get_card(card);
err = mmc_send_ext_csd(card, ext_csd);
- mmc_release_host(card->host);
+ mmc_put_card(card);
if (err)
goto out_free;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 66a530e..bf19058 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1380,14 +1380,14 @@ static void mmc_detect(struct mmc_host *host)
BUG_ON(!host);
BUG_ON(!host->card);
- mmc_claim_host(host);
+ mmc_get_card(host->card);
/*
* Just check if our card has been removed.
*/
err = _mmc_detect_card_removed(host);
- mmc_release_host(host);
+ mmc_put_card(host->card);
if (err) {
mmc_remove(host);
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 9e645e1..30387d6 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1037,14 +1037,14 @@ static void mmc_sd_detect(struct mmc_host *host)
BUG_ON(!host);
BUG_ON(!host->card);
- mmc_claim_host(host);
+ mmc_get_card(host->card);
/*
* Just check if our card has been removed.
*/
err = _mmc_detect_card_removed(host);
- mmc_release_host(host);
+ mmc_put_card(host->card);
if (err) {
mmc_sd_remove(host);
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 39613b9..49fb132 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -188,6 +188,9 @@ extern int __mmc_claim_host(struct mmc_host *host, atomic_t *abort);
extern void mmc_release_host(struct mmc_host *host);
extern int mmc_try_claim_host(struct mmc_host *host);
+extern void mmc_get_card(struct mmc_card *card);
+extern void mmc_put_card(struct mmc_card *card);
+
extern int mmc_flush_cache(struct mmc_card *);
extern int mmc_detect_card_removed(struct mmc_host *host);
--
1.7.10
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice
2013-04-16 10:00 ` [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice Ulf Hansson
@ 2013-05-02 8:58 ` Adrian Hunter
2013-05-02 9:52 ` Ulf Hansson
[not found] ` <CAMj5BkiOmh8sz-=b0z1VF9owGPX0KpbZeNfPzETemCb=C2odGQ@mail.gmail.com>
1 sibling, 1 reply; 21+ messages in thread
From: Adrian Hunter @ 2013-05-02 8:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Chris Ball, Ulf Hansson, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Daniel Drake, Ohad Ben-Cohen
On 16/04/13 13:00, Ulf Hansson wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> Once the mmc blkdevice is being probed, runtime pm will be enabled.
> By using runtime autosuspend, the power save operations can be done
> when request inactivity occurs for a certain time. Right now the
> selected timeout value is set to 3 s. Obviously this value will likely
> need to be configurable somehow since it needs to be trimmed depending
> on the power save algorithm.
Already is configurable in sysfs "power/autosuspend_delay_ms"
Another issue is that re-initialization consumes power - possibly more than
is being saved by powering off. I wonder if the default value of 3 seconds
is realistic. Do you have any numbers to compare idle power consumption
with the power consumed by re-initialization?
>
> For SD-combo cards, we are still leaving the enablement of runtime PM
> to the SDIO init sequence since it depends on the capabilities of the
> SDIO func driver.
>
> Moreover, when the blk device is being suspended, we make sure the device
> will be runtime resumed. The reason for doing this is that we want the
> host suspend sequence to be unaware of any runtime power save operations
> done for the card in this phase. Thus it can just handle the suspend as
> the card is fully powered from a runtime perspective.
>
> Finally, this patch prepares to make it possible to move BKOPS handling
> into the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
> accomplished.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Maya Erez <merez@codeaurora.org>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kevin Liu <kliu5@marvell.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Daniel Drake <dsd@laptop.org>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> ---
> drivers/mmc/card/block.c | 32 ++++++++++++++++++++++++++------
> drivers/mmc/core/core.c | 23 +++++++++++++++++++++++
> drivers/mmc/core/debugfs.c | 8 ++++----
> drivers/mmc/core/mmc.c | 4 ++--
> drivers/mmc/core/sd.c | 4 ++--
> include/linux/mmc/core.h | 3 +++
> 6 files changed, 60 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index e12a03c..360a41a 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -34,6 +34,7 @@
> #include <linux/delay.h>
> #include <linux/capability.h>
> #include <linux/compat.h>
> +#include <linux/pm_runtime.h>
>
> #include <linux/mmc/ioctl.h>
> #include <linux/mmc/card.h>
> @@ -222,7 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
> md = mmc_blk_get(dev_to_disk(dev));
> card = md->queue.card;
>
> - mmc_claim_host(card->host);
> + mmc_get_card(card);
>
> ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
> card->ext_csd.boot_ro_lock |
> @@ -233,7 +234,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
> else
> card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>
> - mmc_release_host(card->host);
> + mmc_put_card(card);
>
> if (!ret) {
> pr_info("%s: Locking boot partition ro until next power on\n",
> @@ -492,7 +493,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>
> mrq.cmd = &cmd;
>
> - mmc_claim_host(card->host);
> + mmc_get_card(card);
>
> err = mmc_blk_part_switch(card, md);
> if (err)
> @@ -559,7 +560,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
> }
>
> cmd_rel_host:
> - mmc_release_host(card->host);
> + mmc_put_card(card);
>
> cmd_done:
> mmc_blk_put(md);
> @@ -1896,7 +1897,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>
> if (req && !mq->mqrq_prev->req)
> /* claim host only for the first request */
> - mmc_claim_host(card->host);
> + mmc_get_card(card);
>
> ret = mmc_blk_part_switch(card, md);
> if (ret) {
> @@ -1940,7 +1941,7 @@ out:
> * In case sepecial request, there is no reentry to
> * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
> */
> - mmc_release_host(card->host);
> + mmc_put_card(card);
> return ret;
> }
>
> @@ -2337,6 +2338,19 @@ static int mmc_blk_probe(struct mmc_card *card)
> if (mmc_add_disk(part_md))
> goto out;
> }
> +
> + pm_runtime_set_autosuspend_delay(&card->dev, 3000);
> + pm_runtime_use_autosuspend(&card->dev);
> +
> + /*
> + * Don't enable runtime PM for SD-combo cards here. Leave that
> + * decision to be taken during the SDIO init sequence instead.
> + */
> + if (card->type != MMC_TYPE_SD_COMBO) {
> + pm_runtime_set_active(&card->dev);
> + pm_runtime_enable(&card->dev);
> + }
> +
> return 0;
>
> out:
> @@ -2350,9 +2364,13 @@ static void mmc_blk_remove(struct mmc_card *card)
> struct mmc_blk_data *md = mmc_get_drvdata(card);
>
> mmc_blk_remove_parts(card, md);
> + pm_runtime_get_sync(&card->dev);
> mmc_claim_host(card->host);
> mmc_blk_part_switch(card, md);
> mmc_release_host(card->host);
> + if (card->type != MMC_TYPE_SD_COMBO)
> + pm_runtime_disable(&card->dev);
> + pm_runtime_put_noidle(&card->dev);
> mmc_blk_remove_req(md);
> mmc_set_drvdata(card, NULL);
> }
> @@ -2364,6 +2382,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
> struct mmc_blk_data *md = mmc_get_drvdata(card);
>
> if (md) {
> + pm_runtime_get_sync(&card->dev);
> mmc_queue_suspend(&md->queue);
> list_for_each_entry(part_md, &md->part, part) {
> mmc_queue_suspend(&part_md->queue);
> @@ -2387,6 +2406,7 @@ static int mmc_blk_resume(struct mmc_card *card)
> list_for_each_entry(part_md, &md->part, part) {
> mmc_queue_resume(&part_md->queue);
> }
> + pm_runtime_put(&card->dev);
> }
> return 0;
> }
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index b16b64a..602db00 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -952,6 +952,29 @@ void mmc_release_host(struct mmc_host *host)
> EXPORT_SYMBOL(mmc_release_host);
>
> /*
> + * This is a helper function, which fetches a runtime pm reference for the
> + * card device and also claims the host.
> + */
> +void mmc_get_card(struct mmc_card *card)
> +{
> + pm_runtime_get_sync(&card->dev);
> + mmc_claim_host(card->host);
> +}
> +EXPORT_SYMBOL(mmc_get_card);
> +
> +/*
> + * This is a helper function, which releases the host and drops the runtime
> + * pm reference for the card device.
> + */
> +void mmc_put_card(struct mmc_card *card)
> +{
> + mmc_release_host(card->host);
> + pm_runtime_mark_last_busy(&card->dev);
> + pm_runtime_put_autosuspend(&card->dev);
> +}
> +EXPORT_SYMBOL(mmc_put_card);
> +
> +/*
> * Internal function that does the actual ios call to the host driver,
> * optionally printing some debug output.
> */
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 35c2f85..54829c0 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -258,13 +258,13 @@ static int mmc_dbg_card_status_get(void *data, u64 *val)
> u32 status;
> int ret;
>
> - mmc_claim_host(card->host);
> + mmc_get_card(card);
>
> ret = mmc_send_status(data, &status);
> if (!ret)
> *val = status;
>
> - mmc_release_host(card->host);
> + mmc_put_card(card);
>
> return ret;
> }
> @@ -291,9 +291,9 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
> goto out_free;
> }
>
> - mmc_claim_host(card->host);
> + mmc_get_card(card);
> err = mmc_send_ext_csd(card, ext_csd);
> - mmc_release_host(card->host);
> + mmc_put_card(card);
> if (err)
> goto out_free;
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 66a530e..bf19058 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1380,14 +1380,14 @@ static void mmc_detect(struct mmc_host *host)
> BUG_ON(!host);
> BUG_ON(!host->card);
>
> - mmc_claim_host(host);
> + mmc_get_card(host->card);
>
> /*
> * Just check if our card has been removed.
> */
> err = _mmc_detect_card_removed(host);
>
> - mmc_release_host(host);
> + mmc_put_card(host->card);
>
> if (err) {
> mmc_remove(host);
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 9e645e1..30387d6 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1037,14 +1037,14 @@ static void mmc_sd_detect(struct mmc_host *host)
> BUG_ON(!host);
> BUG_ON(!host->card);
>
> - mmc_claim_host(host);
> + mmc_get_card(host->card);
>
> /*
> * Just check if our card has been removed.
> */
> err = _mmc_detect_card_removed(host);
>
> - mmc_release_host(host);
> + mmc_put_card(host->card);
>
> if (err) {
> mmc_sd_remove(host);
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 39613b9..49fb132 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -188,6 +188,9 @@ extern int __mmc_claim_host(struct mmc_host *host, atomic_t *abort);
> extern void mmc_release_host(struct mmc_host *host);
> extern int mmc_try_claim_host(struct mmc_host *host);
>
> +extern void mmc_get_card(struct mmc_card *card);
> +extern void mmc_put_card(struct mmc_card *card);
> +
> extern int mmc_flush_cache(struct mmc_card *);
>
> extern int mmc_detect_card_removed(struct mmc_host *host);
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice
2013-05-02 8:58 ` Adrian Hunter
@ 2013-05-02 9:52 ` Ulf Hansson
2013-05-02 9:57 ` Asutosh Das
0 siblings, 1 reply; 21+ messages in thread
From: Ulf Hansson @ 2013-05-02 9:52 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Chris Ball, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Daniel Drake, Ohad Ben-Cohen
On 2 May 2013 10:58, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 16/04/13 13:00, Ulf Hansson wrote:
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>> By using runtime autosuspend, the power save operations can be done
>> when request inactivity occurs for a certain time. Right now the
>> selected timeout value is set to 3 s. Obviously this value will likely
>> need to be configurable somehow since it needs to be trimmed depending
>> on the power save algorithm.
>
> Already is configurable in sysfs "power/autosuspend_delay_ms"
Yes you are right - it is already configurable from sysfs point of view.
What I had in mind was the default values, it could very well be
depending on what power save operation that is to be scheduled. For
example the aggressive power gating could likely use a longer timeout
than when about to schedule idle BKOPS. Anyway, I think it is fair to
leave this to a future patch to handle.
>
> Another issue is that re-initialization consumes power - possibly more than
> is being saved by powering off. I wonder if the default value of 3 seconds
> is realistic. Do you have any numbers to compare idle power consumption
> with the power consumed by re-initialization?
This is very card specific data.
- I have not done a measurement for the power consumed during a
re-init, but only the time it takes to do the re-init. Typically we
are talking about hundreds of ms.
- I have done measurements for a handful of pretty new SD-cards and
for some eMMCs (preventing them from putting them to "sleep"). uSD:
~100-400 uA, eMMC: ~10-150 uA - in both cases I expect the card to not
do any internal house keeping operations, since then we would likely
be talking about tens of mA instead.
The above numbers should also be possible to be fetched from vendors
data sheets.
Maybe some in the mmc code aurora team can elaborate more, since they
have been using this feature for a while now in end user products I
believe. Would be interesting to know what timeout they have chosen
here.
Kind regards
Ulf Hansson
>
>>
>> For SD-combo cards, we are still leaving the enablement of runtime PM
>> to the SDIO init sequence since it depends on the capabilities of the
>> SDIO func driver.
>>
>> Moreover, when the blk device is being suspended, we make sure the device
>> will be runtime resumed. The reason for doing this is that we want the
>> host suspend sequence to be unaware of any runtime power save operations
>> done for the card in this phase. Thus it can just handle the suspend as
>> the card is fully powered from a runtime perspective.
>>
>> Finally, this patch prepares to make it possible to move BKOPS handling
>> into the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>> accomplished.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Maya Erez <merez@codeaurora.org>
>> Cc: Subhash Jadavani <subhashj@codeaurora.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Kevin Liu <kliu5@marvell.com>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Daniel Drake <dsd@laptop.org>
>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>> ---
>> drivers/mmc/card/block.c | 32 ++++++++++++++++++++++++++------
>> drivers/mmc/core/core.c | 23 +++++++++++++++++++++++
>> drivers/mmc/core/debugfs.c | 8 ++++----
>> drivers/mmc/core/mmc.c | 4 ++--
>> drivers/mmc/core/sd.c | 4 ++--
>> include/linux/mmc/core.h | 3 +++
>> 6 files changed, 60 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index e12a03c..360a41a 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -34,6 +34,7 @@
>> #include <linux/delay.h>
>> #include <linux/capability.h>
>> #include <linux/compat.h>
>> +#include <linux/pm_runtime.h>
>>
>> #include <linux/mmc/ioctl.h>
>> #include <linux/mmc/card.h>
>> @@ -222,7 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
>> md = mmc_blk_get(dev_to_disk(dev));
>> card = md->queue.card;
>>
>> - mmc_claim_host(card->host);
>> + mmc_get_card(card);
>>
>> ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
>> card->ext_csd.boot_ro_lock |
>> @@ -233,7 +234,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
>> else
>> card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>
>> - mmc_release_host(card->host);
>> + mmc_put_card(card);
>>
>> if (!ret) {
>> pr_info("%s: Locking boot partition ro until next power on\n",
>> @@ -492,7 +493,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>
>> mrq.cmd = &cmd;
>>
>> - mmc_claim_host(card->host);
>> + mmc_get_card(card);
>>
>> err = mmc_blk_part_switch(card, md);
>> if (err)
>> @@ -559,7 +560,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>> }
>>
>> cmd_rel_host:
>> - mmc_release_host(card->host);
>> + mmc_put_card(card);
>>
>> cmd_done:
>> mmc_blk_put(md);
>> @@ -1896,7 +1897,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>>
>> if (req && !mq->mqrq_prev->req)
>> /* claim host only for the first request */
>> - mmc_claim_host(card->host);
>> + mmc_get_card(card);
>>
>> ret = mmc_blk_part_switch(card, md);
>> if (ret) {
>> @@ -1940,7 +1941,7 @@ out:
>> * In case sepecial request, there is no reentry to
>> * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
>> */
>> - mmc_release_host(card->host);
>> + mmc_put_card(card);
>> return ret;
>> }
>>
>> @@ -2337,6 +2338,19 @@ static int mmc_blk_probe(struct mmc_card *card)
>> if (mmc_add_disk(part_md))
>> goto out;
>> }
>> +
>> + pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>> + pm_runtime_use_autosuspend(&card->dev);
>> +
>> + /*
>> + * Don't enable runtime PM for SD-combo cards here. Leave that
>> + * decision to be taken during the SDIO init sequence instead.
>> + */
>> + if (card->type != MMC_TYPE_SD_COMBO) {
>> + pm_runtime_set_active(&card->dev);
>> + pm_runtime_enable(&card->dev);
>> + }
>> +
>> return 0;
>>
>> out:
>> @@ -2350,9 +2364,13 @@ static void mmc_blk_remove(struct mmc_card *card)
>> struct mmc_blk_data *md = mmc_get_drvdata(card);
>>
>> mmc_blk_remove_parts(card, md);
>> + pm_runtime_get_sync(&card->dev);
>> mmc_claim_host(card->host);
>> mmc_blk_part_switch(card, md);
>> mmc_release_host(card->host);
>> + if (card->type != MMC_TYPE_SD_COMBO)
>> + pm_runtime_disable(&card->dev);
>> + pm_runtime_put_noidle(&card->dev);
>> mmc_blk_remove_req(md);
>> mmc_set_drvdata(card, NULL);
>> }
>> @@ -2364,6 +2382,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
>> struct mmc_blk_data *md = mmc_get_drvdata(card);
>>
>> if (md) {
>> + pm_runtime_get_sync(&card->dev);
>> mmc_queue_suspend(&md->queue);
>> list_for_each_entry(part_md, &md->part, part) {
>> mmc_queue_suspend(&part_md->queue);
>> @@ -2387,6 +2406,7 @@ static int mmc_blk_resume(struct mmc_card *card)
>> list_for_each_entry(part_md, &md->part, part) {
>> mmc_queue_resume(&part_md->queue);
>> }
>> + pm_runtime_put(&card->dev);
>> }
>> return 0;
>> }
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index b16b64a..602db00 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -952,6 +952,29 @@ void mmc_release_host(struct mmc_host *host)
>> EXPORT_SYMBOL(mmc_release_host);
>>
>> /*
>> + * This is a helper function, which fetches a runtime pm reference for the
>> + * card device and also claims the host.
>> + */
>> +void mmc_get_card(struct mmc_card *card)
>> +{
>> + pm_runtime_get_sync(&card->dev);
>> + mmc_claim_host(card->host);
>> +}
>> +EXPORT_SYMBOL(mmc_get_card);
>> +
>> +/*
>> + * This is a helper function, which releases the host and drops the runtime
>> + * pm reference for the card device.
>> + */
>> +void mmc_put_card(struct mmc_card *card)
>> +{
>> + mmc_release_host(card->host);
>> + pm_runtime_mark_last_busy(&card->dev);
>> + pm_runtime_put_autosuspend(&card->dev);
>> +}
>> +EXPORT_SYMBOL(mmc_put_card);
>> +
>> +/*
>> * Internal function that does the actual ios call to the host driver,
>> * optionally printing some debug output.
>> */
>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>> index 35c2f85..54829c0 100644
>> --- a/drivers/mmc/core/debugfs.c
>> +++ b/drivers/mmc/core/debugfs.c
>> @@ -258,13 +258,13 @@ static int mmc_dbg_card_status_get(void *data, u64 *val)
>> u32 status;
>> int ret;
>>
>> - mmc_claim_host(card->host);
>> + mmc_get_card(card);
>>
>> ret = mmc_send_status(data, &status);
>> if (!ret)
>> *val = status;
>>
>> - mmc_release_host(card->host);
>> + mmc_put_card(card);
>>
>> return ret;
>> }
>> @@ -291,9 +291,9 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
>> goto out_free;
>> }
>>
>> - mmc_claim_host(card->host);
>> + mmc_get_card(card);
>> err = mmc_send_ext_csd(card, ext_csd);
>> - mmc_release_host(card->host);
>> + mmc_put_card(card);
>> if (err)
>> goto out_free;
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 66a530e..bf19058 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1380,14 +1380,14 @@ static void mmc_detect(struct mmc_host *host)
>> BUG_ON(!host);
>> BUG_ON(!host->card);
>>
>> - mmc_claim_host(host);
>> + mmc_get_card(host->card);
>>
>> /*
>> * Just check if our card has been removed.
>> */
>> err = _mmc_detect_card_removed(host);
>>
>> - mmc_release_host(host);
>> + mmc_put_card(host->card);
>>
>> if (err) {
>> mmc_remove(host);
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 9e645e1..30387d6 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -1037,14 +1037,14 @@ static void mmc_sd_detect(struct mmc_host *host)
>> BUG_ON(!host);
>> BUG_ON(!host->card);
>>
>> - mmc_claim_host(host);
>> + mmc_get_card(host->card);
>>
>> /*
>> * Just check if our card has been removed.
>> */
>> err = _mmc_detect_card_removed(host);
>>
>> - mmc_release_host(host);
>> + mmc_put_card(host->card);
>>
>> if (err) {
>> mmc_sd_remove(host);
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> index 39613b9..49fb132 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -188,6 +188,9 @@ extern int __mmc_claim_host(struct mmc_host *host, atomic_t *abort);
>> extern void mmc_release_host(struct mmc_host *host);
>> extern int mmc_try_claim_host(struct mmc_host *host);
>>
>> +extern void mmc_get_card(struct mmc_card *card);
>> +extern void mmc_put_card(struct mmc_card *card);
>> +
>> extern int mmc_flush_cache(struct mmc_card *);
>>
>> extern int mmc_detect_card_removed(struct mmc_host *host);
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice
2013-05-02 9:52 ` Ulf Hansson
@ 2013-05-02 9:57 ` Asutosh Das
2013-05-02 11:09 ` Ulf Hansson
0 siblings, 1 reply; 21+ messages in thread
From: Asutosh Das @ 2013-05-02 9:57 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Ulf Hansson, linux-mmc, Chris Ball, Maya Erez,
Subhash Jadavani, Arnd Bergmann, Kevin Liu, Daniel Drake,
Ohad Ben-Cohen
On 5/2/2013 3:22 PM, Ulf Hansson wrote:
> On 2 May 2013 10:58, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 16/04/13 13:00, Ulf Hansson wrote:
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>>> By using runtime autosuspend, the power save operations can be done
>>> when request inactivity occurs for a certain time. Right now the
>>> selected timeout value is set to 3 s. Obviously this value will likely
>>> need to be configurable somehow since it needs to be trimmed depending
>>> on the power save algorithm.
>> Already is configurable in sysfs "power/autosuspend_delay_ms"
> Yes you are right - it is already configurable from sysfs point of view.
>
> What I had in mind was the default values, it could very well be
> depending on what power save operation that is to be scheduled. For
> example the aggressive power gating could likely use a longer timeout
> than when about to schedule idle BKOPS. Anyway, I think it is fair to
> leave this to a future patch to handle.
>
>> Another issue is that re-initialization consumes power - possibly more than
>> is being saved by powering off. I wonder if the default value of 3 seconds
>> is realistic. Do you have any numbers to compare idle power consumption
>> with the power consumed by re-initialization?
> This is very card specific data.
>
> - I have not done a measurement for the power consumed during a
> re-init, but only the time it takes to do the re-init. Typically we
> are talking about hundreds of ms.
>
> - I have done measurements for a handful of pretty new SD-cards and
> for some eMMCs (preventing them from putting them to "sleep"). uSD:
> ~100-400 uA, eMMC: ~10-150 uA - in both cases I expect the card to not
> do any internal house keeping operations, since then we would likely
> be talking about tens of mA instead.
>
> The above numbers should also be possible to be fetched from vendors
> data sheets.
>
> Maybe some in the mmc code aurora team can elaborate more, since they
> have been using this feature for a while now in end user products I
> believe. Would be interesting to know what timeout they have chosen
> here.
Currently, we have a default idle-timeout of 10 seconds. However, we
haven't pulled in and tested Ulf's patch series yet.
>
> Kind regards
> Ulf Hansson
>
>>> For SD-combo cards, we are still leaving the enablement of runtime PM
>>> to the SDIO init sequence since it depends on the capabilities of the
>>> SDIO func driver.
>>>
>>> Moreover, when the blk device is being suspended, we make sure the device
>>> will be runtime resumed. The reason for doing this is that we want the
>>> host suspend sequence to be unaware of any runtime power save operations
>>> done for the card in this phase. Thus it can just handle the suspend as
>>> the card is fully powered from a runtime perspective.
>>>
>>> Finally, this patch prepares to make it possible to move BKOPS handling
>>> into the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>>> accomplished.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Cc: Maya Erez <merez@codeaurora.org>
>>> Cc: Subhash Jadavani <subhashj@codeaurora.org>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Kevin Liu <kliu5@marvell.com>
>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>> Cc: Daniel Drake <dsd@laptop.org>
>>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>>> ---
>>> drivers/mmc/card/block.c | 32 ++++++++++++++++++++++++++------
>>> drivers/mmc/core/core.c | 23 +++++++++++++++++++++++
>>> drivers/mmc/core/debugfs.c | 8 ++++----
>>> drivers/mmc/core/mmc.c | 4 ++--
>>> drivers/mmc/core/sd.c | 4 ++--
>>> include/linux/mmc/core.h | 3 +++
>>> 6 files changed, 60 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>> index e12a03c..360a41a 100644
>>> --- a/drivers/mmc/card/block.c
>>> +++ b/drivers/mmc/card/block.c
>>> @@ -34,6 +34,7 @@
>>> #include <linux/delay.h>
>>> #include <linux/capability.h>
>>> #include <linux/compat.h>
>>> +#include <linux/pm_runtime.h>
>>>
>>> #include <linux/mmc/ioctl.h>
>>> #include <linux/mmc/card.h>
>>> @@ -222,7 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
>>> md = mmc_blk_get(dev_to_disk(dev));
>>> card = md->queue.card;
>>>
>>> - mmc_claim_host(card->host);
>>> + mmc_get_card(card);
>>>
>>> ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
>>> card->ext_csd.boot_ro_lock |
>>> @@ -233,7 +234,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
>>> else
>>> card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>
>>> - mmc_release_host(card->host);
>>> + mmc_put_card(card);
>>>
>>> if (!ret) {
>>> pr_info("%s: Locking boot partition ro until next power on\n",
>>> @@ -492,7 +493,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>>
>>> mrq.cmd = &cmd;
>>>
>>> - mmc_claim_host(card->host);
>>> + mmc_get_card(card);
>>>
>>> err = mmc_blk_part_switch(card, md);
>>> if (err)
>>> @@ -559,7 +560,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>> }
>>>
>>> cmd_rel_host:
>>> - mmc_release_host(card->host);
>>> + mmc_put_card(card);
>>>
>>> cmd_done:
>>> mmc_blk_put(md);
>>> @@ -1896,7 +1897,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>>>
>>> if (req && !mq->mqrq_prev->req)
>>> /* claim host only for the first request */
>>> - mmc_claim_host(card->host);
>>> + mmc_get_card(card);
>>>
>>> ret = mmc_blk_part_switch(card, md);
>>> if (ret) {
>>> @@ -1940,7 +1941,7 @@ out:
>>> * In case sepecial request, there is no reentry to
>>> * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
>>> */
>>> - mmc_release_host(card->host);
>>> + mmc_put_card(card);
>>> return ret;
>>> }
>>>
>>> @@ -2337,6 +2338,19 @@ static int mmc_blk_probe(struct mmc_card *card)
>>> if (mmc_add_disk(part_md))
>>> goto out;
>>> }
>>> +
>>> + pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>> + pm_runtime_use_autosuspend(&card->dev);
>>> +
>>> + /*
>>> + * Don't enable runtime PM for SD-combo cards here. Leave that
>>> + * decision to be taken during the SDIO init sequence instead.
>>> + */
>>> + if (card->type != MMC_TYPE_SD_COMBO) {
>>> + pm_runtime_set_active(&card->dev);
>>> + pm_runtime_enable(&card->dev);
>>> + }
>>> +
>>> return 0;
>>>
>>> out:
>>> @@ -2350,9 +2364,13 @@ static void mmc_blk_remove(struct mmc_card *card)
>>> struct mmc_blk_data *md = mmc_get_drvdata(card);
>>>
>>> mmc_blk_remove_parts(card, md);
>>> + pm_runtime_get_sync(&card->dev);
>>> mmc_claim_host(card->host);
>>> mmc_blk_part_switch(card, md);
>>> mmc_release_host(card->host);
>>> + if (card->type != MMC_TYPE_SD_COMBO)
>>> + pm_runtime_disable(&card->dev);
>>> + pm_runtime_put_noidle(&card->dev);
>>> mmc_blk_remove_req(md);
>>> mmc_set_drvdata(card, NULL);
>>> }
>>> @@ -2364,6 +2382,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
>>> struct mmc_blk_data *md = mmc_get_drvdata(card);
>>>
>>> if (md) {
>>> + pm_runtime_get_sync(&card->dev);
>>> mmc_queue_suspend(&md->queue);
>>> list_for_each_entry(part_md, &md->part, part) {
>>> mmc_queue_suspend(&part_md->queue);
>>> @@ -2387,6 +2406,7 @@ static int mmc_blk_resume(struct mmc_card *card)
>>> list_for_each_entry(part_md, &md->part, part) {
>>> mmc_queue_resume(&part_md->queue);
>>> }
>>> + pm_runtime_put(&card->dev);
>>> }
>>> return 0;
>>> }
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index b16b64a..602db00 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -952,6 +952,29 @@ void mmc_release_host(struct mmc_host *host)
>>> EXPORT_SYMBOL(mmc_release_host);
>>>
>>> /*
>>> + * This is a helper function, which fetches a runtime pm reference for the
>>> + * card device and also claims the host.
>>> + */
>>> +void mmc_get_card(struct mmc_card *card)
>>> +{
>>> + pm_runtime_get_sync(&card->dev);
>>> + mmc_claim_host(card->host);
>>> +}
>>> +EXPORT_SYMBOL(mmc_get_card);
>>> +
>>> +/*
>>> + * This is a helper function, which releases the host and drops the runtime
>>> + * pm reference for the card device.
>>> + */
>>> +void mmc_put_card(struct mmc_card *card)
>>> +{
>>> + mmc_release_host(card->host);
>>> + pm_runtime_mark_last_busy(&card->dev);
>>> + pm_runtime_put_autosuspend(&card->dev);
>>> +}
>>> +EXPORT_SYMBOL(mmc_put_card);
>>> +
>>> +/*
>>> * Internal function that does the actual ios call to the host driver,
>>> * optionally printing some debug output.
>>> */
>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>> index 35c2f85..54829c0 100644
>>> --- a/drivers/mmc/core/debugfs.c
>>> +++ b/drivers/mmc/core/debugfs.c
>>> @@ -258,13 +258,13 @@ static int mmc_dbg_card_status_get(void *data, u64 *val)
>>> u32 status;
>>> int ret;
>>>
>>> - mmc_claim_host(card->host);
>>> + mmc_get_card(card);
>>>
>>> ret = mmc_send_status(data, &status);
>>> if (!ret)
>>> *val = status;
>>>
>>> - mmc_release_host(card->host);
>>> + mmc_put_card(card);
>>>
>>> return ret;
>>> }
>>> @@ -291,9 +291,9 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
>>> goto out_free;
>>> }
>>>
>>> - mmc_claim_host(card->host);
>>> + mmc_get_card(card);
>>> err = mmc_send_ext_csd(card, ext_csd);
>>> - mmc_release_host(card->host);
>>> + mmc_put_card(card);
>>> if (err)
>>> goto out_free;
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 66a530e..bf19058 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1380,14 +1380,14 @@ static void mmc_detect(struct mmc_host *host)
>>> BUG_ON(!host);
>>> BUG_ON(!host->card);
>>>
>>> - mmc_claim_host(host);
>>> + mmc_get_card(host->card);
>>>
>>> /*
>>> * Just check if our card has been removed.
>>> */
>>> err = _mmc_detect_card_removed(host);
>>>
>>> - mmc_release_host(host);
>>> + mmc_put_card(host->card);
>>>
>>> if (err) {
>>> mmc_remove(host);
>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>> index 9e645e1..30387d6 100644
>>> --- a/drivers/mmc/core/sd.c
>>> +++ b/drivers/mmc/core/sd.c
>>> @@ -1037,14 +1037,14 @@ static void mmc_sd_detect(struct mmc_host *host)
>>> BUG_ON(!host);
>>> BUG_ON(!host->card);
>>>
>>> - mmc_claim_host(host);
>>> + mmc_get_card(host->card);
>>>
>>> /*
>>> * Just check if our card has been removed.
>>> */
>>> err = _mmc_detect_card_removed(host);
>>>
>>> - mmc_release_host(host);
>>> + mmc_put_card(host->card);
>>>
>>> if (err) {
>>> mmc_sd_remove(host);
>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>>> index 39613b9..49fb132 100644
>>> --- a/include/linux/mmc/core.h
>>> +++ b/include/linux/mmc/core.h
>>> @@ -188,6 +188,9 @@ extern int __mmc_claim_host(struct mmc_host *host, atomic_t *abort);
>>> extern void mmc_release_host(struct mmc_host *host);
>>> extern int mmc_try_claim_host(struct mmc_host *host);
>>>
>>> +extern void mmc_get_card(struct mmc_card *card);
>>> +extern void mmc_put_card(struct mmc_card *card);
>>> +
>>> extern int mmc_flush_cache(struct mmc_card *);
>>>
>>> extern int mmc_detect_card_removed(struct mmc_host *host);
>>>
> --
> 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
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice
2013-05-02 9:57 ` Asutosh Das
@ 2013-05-02 11:09 ` Ulf Hansson
2013-05-02 12:22 ` Adrian Hunter
0 siblings, 1 reply; 21+ messages in thread
From: Ulf Hansson @ 2013-05-02 11:09 UTC (permalink / raw)
To: Asutosh Das
Cc: Adrian Hunter, Ulf Hansson, linux-mmc, Chris Ball, Maya Erez,
Subhash Jadavani, Arnd Bergmann, Kevin Liu, Daniel Drake,
Ohad Ben-Cohen
On 2 May 2013 11:57, Asutosh Das <asutoshd@codeaurora.org> wrote:
> On 5/2/2013 3:22 PM, Ulf Hansson wrote:
>>
>> On 2 May 2013 10:58, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 16/04/13 13:00, Ulf Hansson wrote:
>>>>
>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>>>> By using runtime autosuspend, the power save operations can be done
>>>> when request inactivity occurs for a certain time. Right now the
>>>> selected timeout value is set to 3 s. Obviously this value will likely
>>>> need to be configurable somehow since it needs to be trimmed depending
>>>> on the power save algorithm.
>>>
>>> Already is configurable in sysfs "power/autosuspend_delay_ms"
>>
>> Yes you are right - it is already configurable from sysfs point of view.
>>
>> What I had in mind was the default values, it could very well be
>> depending on what power save operation that is to be scheduled. For
>> example the aggressive power gating could likely use a longer timeout
>> than when about to schedule idle BKOPS. Anyway, I think it is fair to
>> leave this to a future patch to handle.
>>
>>> Another issue is that re-initialization consumes power - possibly more
>>> than
>>> is being saved by powering off. I wonder if the default value of 3
>>> seconds
>>> is realistic. Do you have any numbers to compare idle power consumption
>>> with the power consumed by re-initialization?
>>
>> This is very card specific data.
>>
>> - I have not done a measurement for the power consumed during a
>> re-init, but only the time it takes to do the re-init. Typically we
>> are talking about hundreds of ms.
>>
>> - I have done measurements for a handful of pretty new SD-cards and
>> for some eMMCs (preventing them from putting them to "sleep"). uSD:
>> ~100-400 uA, eMMC: ~10-150 uA - in both cases I expect the card to not
>> do any internal house keeping operations, since then we would likely
>> be talking about tens of mA instead.
>>
>> The above numbers should also be possible to be fetched from vendors
>> data sheets.
>>
>> Maybe some in the mmc code aurora team can elaborate more, since they
>> have been using this feature for a while now in end user products I
>> believe. Would be interesting to know what timeout they have chosen
>> here.
>
> Currently, we have a default idle-timeout of 10 seconds. However, we haven't
> pulled in and tested Ulf's patch series yet.
That sounds reasonable!
Although, I suggest we stick with the 3 s timeout in this patchset,
since it's to be considered as the generic default timeout. Then we
can send a new patch which possibly consider what power save
operations that are to be scheduled before changing timeout. Is that a
way forward for you?
>>
>>
>> Kind regards
>> Ulf Hansson
>>
>>>> For SD-combo cards, we are still leaving the enablement of runtime PM
>>>> to the SDIO init sequence since it depends on the capabilities of the
>>>> SDIO func driver.
>>>>
>>>> Moreover, when the blk device is being suspended, we make sure the
>>>> device
>>>> will be runtime resumed. The reason for doing this is that we want the
>>>> host suspend sequence to be unaware of any runtime power save operations
>>>> done for the card in this phase. Thus it can just handle the suspend as
>>>> the card is fully powered from a runtime perspective.
>>>>
>>>> Finally, this patch prepares to make it possible to move BKOPS handling
>>>> into the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>>>> accomplished.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Cc: Maya Erez <merez@codeaurora.org>
>>>> Cc: Subhash Jadavani <subhashj@codeaurora.org>
>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>> Cc: Kevin Liu <kliu5@marvell.com>
>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>> Cc: Daniel Drake <dsd@laptop.org>
>>>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>>>> ---
>>>> drivers/mmc/card/block.c | 32 ++++++++++++++++++++++++++------
>>>> drivers/mmc/core/core.c | 23 +++++++++++++++++++++++
>>>> drivers/mmc/core/debugfs.c | 8 ++++----
>>>> drivers/mmc/core/mmc.c | 4 ++--
>>>> drivers/mmc/core/sd.c | 4 ++--
>>>> include/linux/mmc/core.h | 3 +++
>>>> 6 files changed, 60 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>> index e12a03c..360a41a 100644
>>>> --- a/drivers/mmc/card/block.c
>>>> +++ b/drivers/mmc/card/block.c
>>>> @@ -34,6 +34,7 @@
>>>> #include <linux/delay.h>
>>>> #include <linux/capability.h>
>>>> #include <linux/compat.h>
>>>> +#include <linux/pm_runtime.h>
>>>>
>>>> #include <linux/mmc/ioctl.h>
>>>> #include <linux/mmc/card.h>
>>>> @@ -222,7 +223,7 @@ static ssize_t power_ro_lock_store(struct device
>>>> *dev,
>>>> md = mmc_blk_get(dev_to_disk(dev));
>>>> card = md->queue.card;
>>>>
>>>> - mmc_claim_host(card->host);
>>>> + mmc_get_card(card);
>>>>
>>>> ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
>>>> card->ext_csd.boot_ro_lock |
>>>> @@ -233,7 +234,7 @@ static ssize_t power_ro_lock_store(struct device
>>>> *dev,
>>>> else
>>>> card->ext_csd.boot_ro_lock |=
>>>> EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>>
>>>> - mmc_release_host(card->host);
>>>> + mmc_put_card(card);
>>>>
>>>> if (!ret) {
>>>> pr_info("%s: Locking boot partition ro until next power
>>>> on\n",
>>>> @@ -492,7 +493,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>> *bdev,
>>>>
>>>> mrq.cmd = &cmd;
>>>>
>>>> - mmc_claim_host(card->host);
>>>> + mmc_get_card(card);
>>>>
>>>> err = mmc_blk_part_switch(card, md);
>>>> if (err)
>>>> @@ -559,7 +560,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>> *bdev,
>>>> }
>>>>
>>>> cmd_rel_host:
>>>> - mmc_release_host(card->host);
>>>> + mmc_put_card(card);
>>>>
>>>> cmd_done:
>>>> mmc_blk_put(md);
>>>> @@ -1896,7 +1897,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>>>> struct request *req)
>>>>
>>>> if (req && !mq->mqrq_prev->req)
>>>> /* claim host only for the first request */
>>>> - mmc_claim_host(card->host);
>>>> + mmc_get_card(card);
>>>>
>>>> ret = mmc_blk_part_switch(card, md);
>>>> if (ret) {
>>>> @@ -1940,7 +1941,7 @@ out:
>>>> * In case sepecial request, there is no reentry to
>>>> * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
>>>> */
>>>> - mmc_release_host(card->host);
>>>> + mmc_put_card(card);
>>>> return ret;
>>>> }
>>>>
>>>> @@ -2337,6 +2338,19 @@ static int mmc_blk_probe(struct mmc_card *card)
>>>> if (mmc_add_disk(part_md))
>>>> goto out;
>>>> }
>>>> +
>>>> + pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>>> + pm_runtime_use_autosuspend(&card->dev);
>>>> +
>>>> + /*
>>>> + * Don't enable runtime PM for SD-combo cards here. Leave that
>>>> + * decision to be taken during the SDIO init sequence instead.
>>>> + */
>>>> + if (card->type != MMC_TYPE_SD_COMBO) {
>>>> + pm_runtime_set_active(&card->dev);
>>>> + pm_runtime_enable(&card->dev);
>>>> + }
>>>> +
>>>> return 0;
>>>>
>>>> out:
>>>> @@ -2350,9 +2364,13 @@ static void mmc_blk_remove(struct mmc_card *card)
>>>> struct mmc_blk_data *md = mmc_get_drvdata(card);
>>>>
>>>> mmc_blk_remove_parts(card, md);
>>>> + pm_runtime_get_sync(&card->dev);
>>>> mmc_claim_host(card->host);
>>>> mmc_blk_part_switch(card, md);
>>>> mmc_release_host(card->host);
>>>> + if (card->type != MMC_TYPE_SD_COMBO)
>>>> + pm_runtime_disable(&card->dev);
>>>> + pm_runtime_put_noidle(&card->dev);
>>>> mmc_blk_remove_req(md);
>>>> mmc_set_drvdata(card, NULL);
>>>> }
>>>> @@ -2364,6 +2382,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
>>>> struct mmc_blk_data *md = mmc_get_drvdata(card);
>>>>
>>>> if (md) {
>>>> + pm_runtime_get_sync(&card->dev);
>>>> mmc_queue_suspend(&md->queue);
>>>> list_for_each_entry(part_md, &md->part, part) {
>>>> mmc_queue_suspend(&part_md->queue);
>>>> @@ -2387,6 +2406,7 @@ static int mmc_blk_resume(struct mmc_card *card)
>>>> list_for_each_entry(part_md, &md->part, part) {
>>>> mmc_queue_resume(&part_md->queue);
>>>> }
>>>> + pm_runtime_put(&card->dev);
>>>> }
>>>> return 0;
>>>> }
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index b16b64a..602db00 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -952,6 +952,29 @@ void mmc_release_host(struct mmc_host *host)
>>>> EXPORT_SYMBOL(mmc_release_host);
>>>>
>>>> /*
>>>> + * This is a helper function, which fetches a runtime pm reference for
>>>> the
>>>> + * card device and also claims the host.
>>>> + */
>>>> +void mmc_get_card(struct mmc_card *card)
>>>> +{
>>>> + pm_runtime_get_sync(&card->dev);
>>>> + mmc_claim_host(card->host);
>>>> +}
>>>> +EXPORT_SYMBOL(mmc_get_card);
>>>> +
>>>> +/*
>>>> + * This is a helper function, which releases the host and drops the
>>>> runtime
>>>> + * pm reference for the card device.
>>>> + */
>>>> +void mmc_put_card(struct mmc_card *card)
>>>> +{
>>>> + mmc_release_host(card->host);
>>>> + pm_runtime_mark_last_busy(&card->dev);
>>>> + pm_runtime_put_autosuspend(&card->dev);
>>>> +}
>>>> +EXPORT_SYMBOL(mmc_put_card);
>>>> +
>>>> +/*
>>>> * Internal function that does the actual ios call to the host driver,
>>>> * optionally printing some debug output.
>>>> */
>>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>>> index 35c2f85..54829c0 100644
>>>> --- a/drivers/mmc/core/debugfs.c
>>>> +++ b/drivers/mmc/core/debugfs.c
>>>> @@ -258,13 +258,13 @@ static int mmc_dbg_card_status_get(void *data, u64
>>>> *val)
>>>> u32 status;
>>>> int ret;
>>>>
>>>> - mmc_claim_host(card->host);
>>>> + mmc_get_card(card);
>>>>
>>>> ret = mmc_send_status(data, &status);
>>>> if (!ret)
>>>> *val = status;
>>>>
>>>> - mmc_release_host(card->host);
>>>> + mmc_put_card(card);
>>>>
>>>> return ret;
>>>> }
>>>> @@ -291,9 +291,9 @@ static int mmc_ext_csd_open(struct inode *inode,
>>>> struct file *filp)
>>>> goto out_free;
>>>> }
>>>>
>>>> - mmc_claim_host(card->host);
>>>> + mmc_get_card(card);
>>>> err = mmc_send_ext_csd(card, ext_csd);
>>>> - mmc_release_host(card->host);
>>>> + mmc_put_card(card);
>>>> if (err)
>>>> goto out_free;
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index 66a530e..bf19058 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1380,14 +1380,14 @@ static void mmc_detect(struct mmc_host *host)
>>>> BUG_ON(!host);
>>>> BUG_ON(!host->card);
>>>>
>>>> - mmc_claim_host(host);
>>>> + mmc_get_card(host->card);
>>>>
>>>> /*
>>>> * Just check if our card has been removed.
>>>> */
>>>> err = _mmc_detect_card_removed(host);
>>>>
>>>> - mmc_release_host(host);
>>>> + mmc_put_card(host->card);
>>>>
>>>> if (err) {
>>>> mmc_remove(host);
>>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>>> index 9e645e1..30387d6 100644
>>>> --- a/drivers/mmc/core/sd.c
>>>> +++ b/drivers/mmc/core/sd.c
>>>> @@ -1037,14 +1037,14 @@ static void mmc_sd_detect(struct mmc_host *host)
>>>> BUG_ON(!host);
>>>> BUG_ON(!host->card);
>>>>
>>>> - mmc_claim_host(host);
>>>> + mmc_get_card(host->card);
>>>>
>>>> /*
>>>> * Just check if our card has been removed.
>>>> */
>>>> err = _mmc_detect_card_removed(host);
>>>>
>>>> - mmc_release_host(host);
>>>> + mmc_put_card(host->card);
>>>>
>>>> if (err) {
>>>> mmc_sd_remove(host);
>>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>>>> index 39613b9..49fb132 100644
>>>> --- a/include/linux/mmc/core.h
>>>> +++ b/include/linux/mmc/core.h
>>>> @@ -188,6 +188,9 @@ extern int __mmc_claim_host(struct mmc_host *host,
>>>> atomic_t *abort);
>>>> extern void mmc_release_host(struct mmc_host *host);
>>>> extern int mmc_try_claim_host(struct mmc_host *host);
>>>>
>>>> +extern void mmc_get_card(struct mmc_card *card);
>>>> +extern void mmc_put_card(struct mmc_card *card);
>>>> +
>>>> extern int mmc_flush_cache(struct mmc_card *);
>>>>
>>>> extern int mmc_detect_card_removed(struct mmc_host *host);
>>>>
>> --
>> 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
>
>
>
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice
2013-05-02 11:09 ` Ulf Hansson
@ 2013-05-02 12:22 ` Adrian Hunter
0 siblings, 0 replies; 21+ messages in thread
From: Adrian Hunter @ 2013-05-02 12:22 UTC (permalink / raw)
To: Ulf Hansson
Cc: Asutosh Das, Ulf Hansson, linux-mmc, Chris Ball, Maya Erez,
Subhash Jadavani, Arnd Bergmann, Kevin Liu, Daniel Drake,
Ohad Ben-Cohen
On 02/05/13 14:09, Ulf Hansson wrote:
> On 2 May 2013 11:57, Asutosh Das <asutoshd@codeaurora.org> wrote:
>> On 5/2/2013 3:22 PM, Ulf Hansson wrote:
>>>
>>> On 2 May 2013 10:58, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> On 16/04/13 13:00, Ulf Hansson wrote:
>>>>>
>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>
>>>>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>>>>> By using runtime autosuspend, the power save operations can be done
>>>>> when request inactivity occurs for a certain time. Right now the
>>>>> selected timeout value is set to 3 s. Obviously this value will likely
>>>>> need to be configurable somehow since it needs to be trimmed depending
>>>>> on the power save algorithm.
>>>>
>>>> Already is configurable in sysfs "power/autosuspend_delay_ms"
>>>
>>> Yes you are right - it is already configurable from sysfs point of view.
>>>
>>> What I had in mind was the default values, it could very well be
>>> depending on what power save operation that is to be scheduled. For
>>> example the aggressive power gating could likely use a longer timeout
>>> than when about to schedule idle BKOPS. Anyway, I think it is fair to
>>> leave this to a future patch to handle.
>>>
>>>> Another issue is that re-initialization consumes power - possibly more
>>>> than
>>>> is being saved by powering off. I wonder if the default value of 3
>>>> seconds
>>>> is realistic. Do you have any numbers to compare idle power consumption
>>>> with the power consumed by re-initialization?
>>>
>>> This is very card specific data.
>>>
>>> - I have not done a measurement for the power consumed during a
>>> re-init, but only the time it takes to do the re-init. Typically we
>>> are talking about hundreds of ms.
>>>
>>> - I have done measurements for a handful of pretty new SD-cards and
>>> for some eMMCs (preventing them from putting them to "sleep"). uSD:
>>> ~100-400 uA, eMMC: ~10-150 uA - in both cases I expect the card to not
>>> do any internal house keeping operations, since then we would likely
>>> be talking about tens of mA instead.
>>>
>>> The above numbers should also be possible to be fetched from vendors
>>> data sheets.
>>>
>>> Maybe some in the mmc code aurora team can elaborate more, since they
>>> have been using this feature for a while now in end user products I
>>> believe. Would be interesting to know what timeout they have chosen
>>> here.
>>
>> Currently, we have a default idle-timeout of 10 seconds. However, we haven't
>> pulled in and tested Ulf's patch series yet.
>
> That sounds reasonable!
>
> Although, I suggest we stick with the 3 s timeout in this patchset,
> since it's to be considered as the generic default timeout. Then we
> can send a new patch which possibly consider what power save
> operations that are to be scheduled before changing timeout. Is that a
> way forward for you?
OK
>
>>>
>>>
>>> Kind regards
>>> Ulf Hansson
>>>
>>>>> For SD-combo cards, we are still leaving the enablement of runtime PM
>>>>> to the SDIO init sequence since it depends on the capabilities of the
>>>>> SDIO func driver.
>>>>>
>>>>> Moreover, when the blk device is being suspended, we make sure the
>>>>> device
>>>>> will be runtime resumed. The reason for doing this is that we want the
>>>>> host suspend sequence to be unaware of any runtime power save operations
>>>>> done for the card in this phase. Thus it can just handle the suspend as
>>>>> the card is fully powered from a runtime perspective.
>>>>>
>>>>> Finally, this patch prepares to make it possible to move BKOPS handling
>>>>> into the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>>>>> accomplished.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Cc: Maya Erez <merez@codeaurora.org>
>>>>> Cc: Subhash Jadavani <subhashj@codeaurora.org>
>>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>>> Cc: Kevin Liu <kliu5@marvell.com>
>>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>>> Cc: Daniel Drake <dsd@laptop.org>
>>>>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>>>>> ---
>>>>> drivers/mmc/card/block.c | 32 ++++++++++++++++++++++++++------
>>>>> drivers/mmc/core/core.c | 23 +++++++++++++++++++++++
>>>>> drivers/mmc/core/debugfs.c | 8 ++++----
>>>>> drivers/mmc/core/mmc.c | 4 ++--
>>>>> drivers/mmc/core/sd.c | 4 ++--
>>>>> include/linux/mmc/core.h | 3 +++
>>>>> 6 files changed, 60 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>> index e12a03c..360a41a 100644
>>>>> --- a/drivers/mmc/card/block.c
>>>>> +++ b/drivers/mmc/card/block.c
>>>>> @@ -34,6 +34,7 @@
>>>>> #include <linux/delay.h>
>>>>> #include <linux/capability.h>
>>>>> #include <linux/compat.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>>
>>>>> #include <linux/mmc/ioctl.h>
>>>>> #include <linux/mmc/card.h>
>>>>> @@ -222,7 +223,7 @@ static ssize_t power_ro_lock_store(struct device
>>>>> *dev,
>>>>> md = mmc_blk_get(dev_to_disk(dev));
>>>>> card = md->queue.card;
>>>>>
>>>>> - mmc_claim_host(card->host);
>>>>> + mmc_get_card(card);
>>>>>
>>>>> ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
>>>>> card->ext_csd.boot_ro_lock |
>>>>> @@ -233,7 +234,7 @@ static ssize_t power_ro_lock_store(struct device
>>>>> *dev,
>>>>> else
>>>>> card->ext_csd.boot_ro_lock |=
>>>>> EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>>>
>>>>> - mmc_release_host(card->host);
>>>>> + mmc_put_card(card);
>>>>>
>>>>> if (!ret) {
>>>>> pr_info("%s: Locking boot partition ro until next power
>>>>> on\n",
>>>>> @@ -492,7 +493,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>>> *bdev,
>>>>>
>>>>> mrq.cmd = &cmd;
>>>>>
>>>>> - mmc_claim_host(card->host);
>>>>> + mmc_get_card(card);
>>>>>
>>>>> err = mmc_blk_part_switch(card, md);
>>>>> if (err)
>>>>> @@ -559,7 +560,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>>> *bdev,
>>>>> }
>>>>>
>>>>> cmd_rel_host:
>>>>> - mmc_release_host(card->host);
>>>>> + mmc_put_card(card);
>>>>>
>>>>> cmd_done:
>>>>> mmc_blk_put(md);
>>>>> @@ -1896,7 +1897,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>>>>> struct request *req)
>>>>>
>>>>> if (req && !mq->mqrq_prev->req)
>>>>> /* claim host only for the first request */
>>>>> - mmc_claim_host(card->host);
>>>>> + mmc_get_card(card);
>>>>>
>>>>> ret = mmc_blk_part_switch(card, md);
>>>>> if (ret) {
>>>>> @@ -1940,7 +1941,7 @@ out:
>>>>> * In case sepecial request, there is no reentry to
>>>>> * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
>>>>> */
>>>>> - mmc_release_host(card->host);
>>>>> + mmc_put_card(card);
>>>>> return ret;
>>>>> }
>>>>>
>>>>> @@ -2337,6 +2338,19 @@ static int mmc_blk_probe(struct mmc_card *card)
>>>>> if (mmc_add_disk(part_md))
>>>>> goto out;
>>>>> }
>>>>> +
>>>>> + pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>>>> + pm_runtime_use_autosuspend(&card->dev);
>>>>> +
>>>>> + /*
>>>>> + * Don't enable runtime PM for SD-combo cards here. Leave that
>>>>> + * decision to be taken during the SDIO init sequence instead.
>>>>> + */
>>>>> + if (card->type != MMC_TYPE_SD_COMBO) {
>>>>> + pm_runtime_set_active(&card->dev);
>>>>> + pm_runtime_enable(&card->dev);
>>>>> + }
>>>>> +
>>>>> return 0;
>>>>>
>>>>> out:
>>>>> @@ -2350,9 +2364,13 @@ static void mmc_blk_remove(struct mmc_card *card)
>>>>> struct mmc_blk_data *md = mmc_get_drvdata(card);
>>>>>
>>>>> mmc_blk_remove_parts(card, md);
>>>>> + pm_runtime_get_sync(&card->dev);
>>>>> mmc_claim_host(card->host);
>>>>> mmc_blk_part_switch(card, md);
>>>>> mmc_release_host(card->host);
>>>>> + if (card->type != MMC_TYPE_SD_COMBO)
>>>>> + pm_runtime_disable(&card->dev);
>>>>> + pm_runtime_put_noidle(&card->dev);
>>>>> mmc_blk_remove_req(md);
>>>>> mmc_set_drvdata(card, NULL);
>>>>> }
>>>>> @@ -2364,6 +2382,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
>>>>> struct mmc_blk_data *md = mmc_get_drvdata(card);
>>>>>
>>>>> if (md) {
>>>>> + pm_runtime_get_sync(&card->dev);
>>>>> mmc_queue_suspend(&md->queue);
>>>>> list_for_each_entry(part_md, &md->part, part) {
>>>>> mmc_queue_suspend(&part_md->queue);
>>>>> @@ -2387,6 +2406,7 @@ static int mmc_blk_resume(struct mmc_card *card)
>>>>> list_for_each_entry(part_md, &md->part, part) {
>>>>> mmc_queue_resume(&part_md->queue);
>>>>> }
>>>>> + pm_runtime_put(&card->dev);
>>>>> }
>>>>> return 0;
>>>>> }
>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>> index b16b64a..602db00 100644
>>>>> --- a/drivers/mmc/core/core.c
>>>>> +++ b/drivers/mmc/core/core.c
>>>>> @@ -952,6 +952,29 @@ void mmc_release_host(struct mmc_host *host)
>>>>> EXPORT_SYMBOL(mmc_release_host);
>>>>>
>>>>> /*
>>>>> + * This is a helper function, which fetches a runtime pm reference for
>>>>> the
>>>>> + * card device and also claims the host.
>>>>> + */
>>>>> +void mmc_get_card(struct mmc_card *card)
>>>>> +{
>>>>> + pm_runtime_get_sync(&card->dev);
>>>>> + mmc_claim_host(card->host);
>>>>> +}
>>>>> +EXPORT_SYMBOL(mmc_get_card);
>>>>> +
>>>>> +/*
>>>>> + * This is a helper function, which releases the host and drops the
>>>>> runtime
>>>>> + * pm reference for the card device.
>>>>> + */
>>>>> +void mmc_put_card(struct mmc_card *card)
>>>>> +{
>>>>> + mmc_release_host(card->host);
>>>>> + pm_runtime_mark_last_busy(&card->dev);
>>>>> + pm_runtime_put_autosuspend(&card->dev);
>>>>> +}
>>>>> +EXPORT_SYMBOL(mmc_put_card);
>>>>> +
>>>>> +/*
>>>>> * Internal function that does the actual ios call to the host driver,
>>>>> * optionally printing some debug output.
>>>>> */
>>>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>>>> index 35c2f85..54829c0 100644
>>>>> --- a/drivers/mmc/core/debugfs.c
>>>>> +++ b/drivers/mmc/core/debugfs.c
>>>>> @@ -258,13 +258,13 @@ static int mmc_dbg_card_status_get(void *data, u64
>>>>> *val)
>>>>> u32 status;
>>>>> int ret;
>>>>>
>>>>> - mmc_claim_host(card->host);
>>>>> + mmc_get_card(card);
>>>>>
>>>>> ret = mmc_send_status(data, &status);
>>>>> if (!ret)
>>>>> *val = status;
>>>>>
>>>>> - mmc_release_host(card->host);
>>>>> + mmc_put_card(card);
>>>>>
>>>>> return ret;
>>>>> }
>>>>> @@ -291,9 +291,9 @@ static int mmc_ext_csd_open(struct inode *inode,
>>>>> struct file *filp)
>>>>> goto out_free;
>>>>> }
>>>>>
>>>>> - mmc_claim_host(card->host);
>>>>> + mmc_get_card(card);
>>>>> err = mmc_send_ext_csd(card, ext_csd);
>>>>> - mmc_release_host(card->host);
>>>>> + mmc_put_card(card);
>>>>> if (err)
>>>>> goto out_free;
>>>>>
>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>>> index 66a530e..bf19058 100644
>>>>> --- a/drivers/mmc/core/mmc.c
>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>> @@ -1380,14 +1380,14 @@ static void mmc_detect(struct mmc_host *host)
>>>>> BUG_ON(!host);
>>>>> BUG_ON(!host->card);
>>>>>
>>>>> - mmc_claim_host(host);
>>>>> + mmc_get_card(host->card);
>>>>>
>>>>> /*
>>>>> * Just check if our card has been removed.
>>>>> */
>>>>> err = _mmc_detect_card_removed(host);
>>>>>
>>>>> - mmc_release_host(host);
>>>>> + mmc_put_card(host->card);
>>>>>
>>>>> if (err) {
>>>>> mmc_remove(host);
>>>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>>>> index 9e645e1..30387d6 100644
>>>>> --- a/drivers/mmc/core/sd.c
>>>>> +++ b/drivers/mmc/core/sd.c
>>>>> @@ -1037,14 +1037,14 @@ static void mmc_sd_detect(struct mmc_host *host)
>>>>> BUG_ON(!host);
>>>>> BUG_ON(!host->card);
>>>>>
>>>>> - mmc_claim_host(host);
>>>>> + mmc_get_card(host->card);
>>>>>
>>>>> /*
>>>>> * Just check if our card has been removed.
>>>>> */
>>>>> err = _mmc_detect_card_removed(host);
>>>>>
>>>>> - mmc_release_host(host);
>>>>> + mmc_put_card(host->card);
>>>>>
>>>>> if (err) {
>>>>> mmc_sd_remove(host);
>>>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>>>>> index 39613b9..49fb132 100644
>>>>> --- a/include/linux/mmc/core.h
>>>>> +++ b/include/linux/mmc/core.h
>>>>> @@ -188,6 +188,9 @@ extern int __mmc_claim_host(struct mmc_host *host,
>>>>> atomic_t *abort);
>>>>> extern void mmc_release_host(struct mmc_host *host);
>>>>> extern int mmc_try_claim_host(struct mmc_host *host);
>>>>>
>>>>> +extern void mmc_get_card(struct mmc_card *card);
>>>>> +extern void mmc_put_card(struct mmc_card *card);
>>>>> +
>>>>> extern int mmc_flush_cache(struct mmc_card *);
>>>>>
>>>>> extern int mmc_detect_card_removed(struct mmc_host *host);
>>>>>
>>> --
>>> 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
>>
>>
>>
>> --
>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <CAMj5BkiOmh8sz-=b0z1VF9owGPX0KpbZeNfPzETemCb=C2odGQ@mail.gmail.com>]
* Re: [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice
[not found] ` <CAMj5BkiOmh8sz-=b0z1VF9owGPX0KpbZeNfPzETemCb=C2odGQ@mail.gmail.com>
@ 2013-05-24 8:27 ` Ulf Hansson
[not found] ` <CAMj5Bki+1=DSzQWYyEC1L=Pa6LpSFQKF3YvoUkkuq62wHuMWow@mail.gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Ulf Hansson @ 2013-05-24 8:27 UTC (permalink / raw)
To: zhangfei gao
Cc: Ulf Hansson, linux-mmc@vger.kernel.org, Chris Ball, Maya Erez,
Subhash Jadavani, Arnd Bergmann, Kevin Liu, Adrian Hunter,
Daniel Drake, Ohad Ben-Cohen
On 24 May 2013 08:06, zhangfei gao <zhangfei.gao@gmail.com> wrote:
>
>
>
> On Tue, Apr 16, 2013 at 6:00 PM, Ulf Hansson <ulf.hansson@stericsson.com>
> wrote:
>>
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>> By using runtime autosuspend, the power save operations can be done
>> when request inactivity occurs for a certain time. Right now the
>> selected timeout value is set to 3 s. Obviously this value will likely
>> need to be configurable somehow since it needs to be trimmed depending
>> on the power save algorithm.
>>
>> For SD-combo cards, we are still leaving the enablement of runtime PM
>> to the SDIO init sequence since it depends on the capabilities of the
>> SDIO func driver.
>>
>> Moreover, when the blk device is being suspended, we make sure the device
>> will be runtime resumed. The reason for doing this is that we want the
>> host suspend sequence to be unaware of any runtime power save operations
>> done for the card in this phase. Thus it can just handle the suspend as
>> the card is fully powered from a runtime perspective.
>>
>
> If sd card is removed during system suspend, NULL pointer error would happen
> since card is removed.
Why, where? Have you tested this patch, then maybe you can share a log?
Myself has of course tested above scenario without observing any
issues, though my environment could be different from yours.
> mmc_detect_change would be called even CONFIG_MMC_UNSAFE_RESUME, so no issue
> without these patch.
You need to elaborate why you see a concern here, I can not follow.
>
> Besides, any chance to not call suspend if runtime suspended, such as flag?
> Otherwise, suspend -> runtime_resume -> resume -> suspend.
> Looks much repetition.
The above sequence can only be present while using
MMC_CAP_AGGRESSIVE_PM so your comment is not valid for this patch as
such.
For the MMC_CAP_AGGRESSIVE_PM patch; in this phase I decided to keep
this simple. You may want to optimize for the above sequence in future
patches.
>
> Thanks
Thanks for reviewing!
Kind regards
Ulf Hansson
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V3 4/4] mmc: core: Support aggressive power management for (e)MMC/SD
2013-04-16 10:00 [PATCH V3 0/4] mmc: Use runtime pm for blkdevice Ulf Hansson
` (2 preceding siblings ...)
2013-04-16 10:00 ` [PATCH V3 3/4] mmc: block: Enable runtime pm for mmc blkdevice Ulf Hansson
@ 2013-04-16 10:00 ` Ulf Hansson
2013-05-02 10:38 ` Adrian Hunter
3 siblings, 1 reply; 21+ messages in thread
From: Ulf Hansson @ 2013-04-16 10:00 UTC (permalink / raw)
To: linux-mmc, Chris Ball
Cc: Ulf Hansson, Ulf Hansson, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Adrian Hunter, Daniel Drake,
Ohad Ben-Cohen
Aggressive power management is suitable when saving power is
essential. At request inactivity timeout, aka pm runtime
autosuspend timeout, the card will be suspended.
Once a new request arrives, the card will be re-initalized and
thus the first request will suffer from a latency. This latency
is card-specific, experiments has shown in general that SD-cards
has quite poor initialization time, around 300ms-1100ms. eMMC is
not surprisingly far better but still a couple of hundreds of ms
has been observed.
Except for the request latency, it is important to know that
suspending the card will also prevent the card from executing
internal house-keeping operations in idle mode. This could mean
degradation in performance.
To use this feature make sure the request inactivity timeout is
chosen carefully. This has not been done as a part of this patch.
Enable this feature by using host cap MMC_CAP_AGGRESSIVE_PM and
by setting CONFIG_MMC_UNSAFE_RESUME.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Maya Erez <merez@codeaurora.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Kevin Liu <kliu5@marvell.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Daniel Drake <dsd@laptop.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>
---
drivers/mmc/core/mmc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/core/sd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
include/linux/mmc/host.h | 2 +-
3 files changed, 86 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index bf19058..8dfbc84 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1454,6 +1454,47 @@ static int mmc_resume(struct mmc_host *host)
return err;
}
+
+/*
+ * Callback for runtime_suspend.
+ */
+static int mmc_runtime_suspend(struct mmc_host *host)
+{
+ int err;
+
+ if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
+ return 0;
+
+ err = mmc_suspend(host);
+ if (err) {
+ pr_err("%s: error %d doing aggessive suspend\n",
+ mmc_hostname(host), err);
+ return err;
+ }
+
+ mmc_power_off(host);
+ return err;
+}
+
+/*
+ * Callback for runtime_resume.
+ */
+static int mmc_runtime_resume(struct mmc_host *host)
+{
+ int err;
+
+ if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
+ return 0;
+
+ mmc_power_up(host);
+ err = mmc_resume(host);
+ if (err)
+ pr_err("%s: error %d doing aggessive resume\n",
+ mmc_hostname(host), err);
+
+ return err;
+}
+
static int mmc_power_restore(struct mmc_host *host)
{
int ret;
@@ -1514,6 +1555,8 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
.detect = mmc_detect,
.suspend = mmc_suspend,
.resume = mmc_resume,
+ .runtime_suspend = mmc_runtime_suspend,
+ .runtime_resume = mmc_runtime_resume,
.power_restore = mmc_power_restore,
.alive = mmc_alive,
};
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 30387d6..e0458f9 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1095,6 +1095,46 @@ static int mmc_sd_resume(struct mmc_host *host)
return err;
}
+/*
+ * Callback for runtime_suspend.
+ */
+static int mmc_sd_runtime_suspend(struct mmc_host *host)
+{
+ int err;
+
+ if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
+ return 0;
+
+ err = mmc_sd_suspend(host);
+ if (err) {
+ pr_err("%s: error %d doing aggessive suspend\n",
+ mmc_hostname(host), err);
+ return err;
+ }
+
+ mmc_power_off(host);
+ return err;
+}
+
+/*
+ * Callback for runtime_resume.
+ */
+static int mmc_sd_runtime_resume(struct mmc_host *host)
+{
+ int err;
+
+ if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
+ return 0;
+
+ mmc_power_up(host);
+ err = mmc_sd_resume(host);
+ if (err)
+ pr_err("%s: error %d doing aggessive resume\n",
+ mmc_hostname(host), err);
+
+ return err;
+}
+
static int mmc_sd_power_restore(struct mmc_host *host)
{
int ret;
@@ -1119,6 +1159,8 @@ static const struct mmc_bus_ops mmc_sd_ops = {
static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
.remove = mmc_sd_remove,
.detect = mmc_sd_detect,
+ .runtime_suspend = mmc_sd_runtime_suspend,
+ .runtime_resume = mmc_sd_runtime_resume,
.suspend = mmc_sd_suspend,
.resume = mmc_sd_resume,
.power_restore = mmc_sd_power_restore,
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 17d7148..cec6684 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -239,7 +239,7 @@ struct mmc_host {
#define MMC_CAP_SPI (1 << 4) /* Talks only SPI protocols */
#define MMC_CAP_NEEDS_POLL (1 << 5) /* Needs polling for card-detection */
#define MMC_CAP_8_BIT_DATA (1 << 6) /* Can the host do 8 bit transfers */
-
+#define MMC_CAP_AGGRESSIVE_PM (1 << 7) /* Suspend (e)MMC/SD at idle */
#define MMC_CAP_NONREMOVABLE (1 << 8) /* Nonremovable e.g. eMMC */
#define MMC_CAP_WAIT_WHILE_BUSY (1 << 9) /* Waits while card is busy */
#define MMC_CAP_ERASE (1 << 10) /* Allow erase/trim commands */
--
1.7.10
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V3 4/4] mmc: core: Support aggressive power management for (e)MMC/SD
2013-04-16 10:00 ` [PATCH V3 4/4] mmc: core: Support aggressive power management for (e)MMC/SD Ulf Hansson
@ 2013-05-02 10:38 ` Adrian Hunter
2013-05-02 11:35 ` Ulf Hansson
0 siblings, 1 reply; 21+ messages in thread
From: Adrian Hunter @ 2013-05-02 10:38 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Chris Ball, Ulf Hansson, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Daniel Drake, Ohad Ben-Cohen
On 16/04/13 13:00, Ulf Hansson wrote:
> Aggressive power management is suitable when saving power is
> essential. At request inactivity timeout, aka pm runtime
> autosuspend timeout, the card will be suspended.
>
> Once a new request arrives, the card will be re-initalized and
> thus the first request will suffer from a latency. This latency
> is card-specific, experiments has shown in general that SD-cards
> has quite poor initialization time, around 300ms-1100ms. eMMC is
> not surprisingly far better but still a couple of hundreds of ms
> has been observed.
>
> Except for the request latency, it is important to know that
> suspending the card will also prevent the card from executing
> internal house-keeping operations in idle mode. This could mean
> degradation in performance.
>
> To use this feature make sure the request inactivity timeout is
> chosen carefully. This has not been done as a part of this patch.
>
> Enable this feature by using host cap MMC_CAP_AGGRESSIVE_PM and
> by setting CONFIG_MMC_UNSAFE_RESUME.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Maya Erez <merez@codeaurora.org>
> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Kevin Liu <kliu5@marvell.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Daniel Drake <dsd@laptop.org>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> ---
> drivers/mmc/core/mmc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> drivers/mmc/core/sd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/mmc/host.h | 2 +-
> 3 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index bf19058..8dfbc84 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1454,6 +1454,47 @@ static int mmc_resume(struct mmc_host *host)
> return err;
> }
>
> +
> +/*
> + * Callback for runtime_suspend.
> + */
> +static int mmc_runtime_suspend(struct mmc_host *host)
> +{
> + int err;
> +
> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
> + return 0;
> +
mmc_power_off() needs to be within mmc_claim_host() / mmc_release_host().
Claiming is nested, so you can out put mmc_claim_host() here:
mmc_claim_host(host);
> + err = mmc_suspend(host);
> + if (err) {
> + pr_err("%s: error %d doing aggessive suspend\n",
> + mmc_hostname(host), err);
> + return err;
goto out;
> + }
> +
> + mmc_power_off(host);
out:
mmc_release_host(host);
> + return err;
> +}
> +
> +/*
> + * Callback for runtime_resume.
> + */
> +static int mmc_runtime_resume(struct mmc_host *host)
> +{
> + int err;
> +
> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
> + return 0;
> +
> + mmc_power_up(host);
As above
> + err = mmc_resume(host);
> + if (err)
> + pr_err("%s: error %d doing aggessive resume\n",
> + mmc_hostname(host), err);
> +
> + return err;
The power is on - leaving the device in a RPM_SUSPENDED state does not seem
useful so better to return zero here.
> +}
> +
> static int mmc_power_restore(struct mmc_host *host)
> {
> int ret;
> @@ -1514,6 +1555,8 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
> .detect = mmc_detect,
> .suspend = mmc_suspend,
> .resume = mmc_resume,
> + .runtime_suspend = mmc_runtime_suspend,
> + .runtime_resume = mmc_runtime_resume,
> .power_restore = mmc_power_restore,
> .alive = mmc_alive,
> };
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 30387d6..e0458f9 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1095,6 +1095,46 @@ static int mmc_sd_resume(struct mmc_host *host)
> return err;
> }
>
> +/*
> + * Callback for runtime_suspend.
> + */
> +static int mmc_sd_runtime_suspend(struct mmc_host *host)
> +{
> + int err;
> +
> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
> + return 0;
> +
> + err = mmc_sd_suspend(host);
> + if (err) {
> + pr_err("%s: error %d doing aggessive suspend\n",
> + mmc_hostname(host), err);
> + return err;
> + }
> +
> + mmc_power_off(host);
As above
> + return err;
> +}
> +
> +/*
> + * Callback for runtime_resume.
> + */
> +static int mmc_sd_runtime_resume(struct mmc_host *host)
> +{
> + int err;
> +
> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
> + return 0;
> +
> + mmc_power_up(host);
As above
> + err = mmc_sd_resume(host);
> + if (err)
> + pr_err("%s: error %d doing aggessive resume\n",
> + mmc_hostname(host), err);
> +
> + return err;
As above
return 0
> +}
> +
> static int mmc_sd_power_restore(struct mmc_host *host)
> {
> int ret;
> @@ -1119,6 +1159,8 @@ static const struct mmc_bus_ops mmc_sd_ops = {
> static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
> .remove = mmc_sd_remove,
> .detect = mmc_sd_detect,
> + .runtime_suspend = mmc_sd_runtime_suspend,
> + .runtime_resume = mmc_sd_runtime_resume,
> .suspend = mmc_sd_suspend,
> .resume = mmc_sd_resume,
> .power_restore = mmc_sd_power_restore,
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 17d7148..cec6684 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -239,7 +239,7 @@ struct mmc_host {
> #define MMC_CAP_SPI (1 << 4) /* Talks only SPI protocols */
> #define MMC_CAP_NEEDS_POLL (1 << 5) /* Needs polling for card-detection */
> #define MMC_CAP_8_BIT_DATA (1 << 6) /* Can the host do 8 bit transfers */
> -
> +#define MMC_CAP_AGGRESSIVE_PM (1 << 7) /* Suspend (e)MMC/SD at idle */
Using a "cap" is not ideal here - it should really be under the control of
user space.
> #define MMC_CAP_NONREMOVABLE (1 << 8) /* Nonremovable e.g. eMMC */
> #define MMC_CAP_WAIT_WHILE_BUSY (1 << 9) /* Waits while card is busy */
> #define MMC_CAP_ERASE (1 << 10) /* Allow erase/trim commands */
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 4/4] mmc: core: Support aggressive power management for (e)MMC/SD
2013-05-02 10:38 ` Adrian Hunter
@ 2013-05-02 11:35 ` Ulf Hansson
2013-05-02 12:24 ` Adrian Hunter
0 siblings, 1 reply; 21+ messages in thread
From: Ulf Hansson @ 2013-05-02 11:35 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Chris Ball, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Daniel Drake, Ohad Ben-Cohen
On 2 May 2013 12:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 16/04/13 13:00, Ulf Hansson wrote:
>> Aggressive power management is suitable when saving power is
>> essential. At request inactivity timeout, aka pm runtime
>> autosuspend timeout, the card will be suspended.
>>
>> Once a new request arrives, the card will be re-initalized and
>> thus the first request will suffer from a latency. This latency
>> is card-specific, experiments has shown in general that SD-cards
>> has quite poor initialization time, around 300ms-1100ms. eMMC is
>> not surprisingly far better but still a couple of hundreds of ms
>> has been observed.
>>
>> Except for the request latency, it is important to know that
>> suspending the card will also prevent the card from executing
>> internal house-keeping operations in idle mode. This could mean
>> degradation in performance.
>>
>> To use this feature make sure the request inactivity timeout is
>> chosen carefully. This has not been done as a part of this patch.
>>
>> Enable this feature by using host cap MMC_CAP_AGGRESSIVE_PM and
>> by setting CONFIG_MMC_UNSAFE_RESUME.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Maya Erez <merez@codeaurora.org>
>> Cc: Subhash Jadavani <subhashj@codeaurora.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Kevin Liu <kliu5@marvell.com>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Daniel Drake <dsd@laptop.org>
>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>> ---
>> drivers/mmc/core/mmc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>> drivers/mmc/core/sd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mmc/host.h | 2 +-
>> 3 files changed, 86 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index bf19058..8dfbc84 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1454,6 +1454,47 @@ static int mmc_resume(struct mmc_host *host)
>> return err;
>> }
>>
>> +
>> +/*
>> + * Callback for runtime_suspend.
>> + */
>> +static int mmc_runtime_suspend(struct mmc_host *host)
>> +{
>> + int err;
>> +
>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>> + return 0;
>> +
>
> mmc_power_off() needs to be within mmc_claim_host() / mmc_release_host().
>
> Claiming is nested, so you can out put mmc_claim_host() here:
>
> mmc_claim_host(host);
OK
>
>
>> + err = mmc_suspend(host);
>> + if (err) {
>> + pr_err("%s: error %d doing aggessive suspend\n",
>> + mmc_hostname(host), err);
>> + return err;
>
> goto out;
>
>> + }
>> +
>> + mmc_power_off(host);
>
> out:
> mmc_release_host(host);
>
OK
>> + return err;
>> +}
>> +
>> +/*
>> + * Callback for runtime_resume.
>> + */
>> +static int mmc_runtime_resume(struct mmc_host *host)
>> +{
>> + int err;
>> +
>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>> + return 0;
>> +
>> + mmc_power_up(host);
>
> As above
>
OK
>> + err = mmc_resume(host);
>> + if (err)
>> + pr_err("%s: error %d doing aggessive resume\n",
>> + mmc_hostname(host), err);
>> +
>> + return err;
>
> The power is on - leaving the device in a RPM_SUSPENDED state does not seem
> useful so better to return zero here.
OK
>
>> +}
>> +
>> static int mmc_power_restore(struct mmc_host *host)
>> {
>> int ret;
>> @@ -1514,6 +1555,8 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>> .detect = mmc_detect,
>> .suspend = mmc_suspend,
>> .resume = mmc_resume,
>> + .runtime_suspend = mmc_runtime_suspend,
>> + .runtime_resume = mmc_runtime_resume,
>> .power_restore = mmc_power_restore,
>> .alive = mmc_alive,
>> };
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 30387d6..e0458f9 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -1095,6 +1095,46 @@ static int mmc_sd_resume(struct mmc_host *host)
>> return err;
>> }
>>
>> +/*
>> + * Callback for runtime_suspend.
>> + */
>> +static int mmc_sd_runtime_suspend(struct mmc_host *host)
>> +{
>> + int err;
>> +
>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>> + return 0;
>> +
>> + err = mmc_sd_suspend(host);
>> + if (err) {
>> + pr_err("%s: error %d doing aggessive suspend\n",
>> + mmc_hostname(host), err);
>> + return err;
>> + }
>> +
>> + mmc_power_off(host);
>
> As above
OK
>
>> + return err;
>> +}
>> +
>> +/*
>> + * Callback for runtime_resume.
>> + */
>> +static int mmc_sd_runtime_resume(struct mmc_host *host)
>> +{
>> + int err;
>> +
>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>> + return 0;
>> +
>> + mmc_power_up(host);
>
> As above
OK
>
>> + err = mmc_sd_resume(host);
>> + if (err)
>> + pr_err("%s: error %d doing aggessive resume\n",
>> + mmc_hostname(host), err);
>> +
>> + return err;
>
> As above
OK
>
> return 0
>
>> +}
>> +
>> static int mmc_sd_power_restore(struct mmc_host *host)
>> {
>> int ret;
>> @@ -1119,6 +1159,8 @@ static const struct mmc_bus_ops mmc_sd_ops = {
>> static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
>> .remove = mmc_sd_remove,
>> .detect = mmc_sd_detect,
>> + .runtime_suspend = mmc_sd_runtime_suspend,
>> + .runtime_resume = mmc_sd_runtime_resume,
>> .suspend = mmc_sd_suspend,
>> .resume = mmc_sd_resume,
>> .power_restore = mmc_sd_power_restore,
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 17d7148..cec6684 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -239,7 +239,7 @@ struct mmc_host {
>> #define MMC_CAP_SPI (1 << 4) /* Talks only SPI protocols */
>> #define MMC_CAP_NEEDS_POLL (1 << 5) /* Needs polling for card-detection */
>> #define MMC_CAP_8_BIT_DATA (1 << 6) /* Can the host do 8 bit transfers */
>> -
>> +#define MMC_CAP_AGGRESSIVE_PM (1 << 7) /* Suspend (e)MMC/SD at idle */
>
> Using a "cap" is not ideal here - it should really be under the control of
> user space.
Several other caps could be debated whether these should exist here as
well, but let's leave that to a separate discussion.
Until there are a solution for how to add new mmc features per host,
which should be runtime configurable, I believe this is the only way
we can do it.
Do note, that runtime pm can be disabled from user space, which
indirectly will prevent this feature from being used.
>
>> #define MMC_CAP_NONREMOVABLE (1 << 8) /* Nonremovable e.g. eMMC */
>> #define MMC_CAP_WAIT_WHILE_BUSY (1 << 9) /* Waits while card is busy */
>> #define MMC_CAP_ERASE (1 << 10) /* Allow erase/trim commands */
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 4/4] mmc: core: Support aggressive power management for (e)MMC/SD
2013-05-02 11:35 ` Ulf Hansson
@ 2013-05-02 12:24 ` Adrian Hunter
0 siblings, 0 replies; 21+ messages in thread
From: Adrian Hunter @ 2013-05-02 12:24 UTC (permalink / raw)
To: Ulf Hansson
Cc: Ulf Hansson, linux-mmc, Chris Ball, Maya Erez, Subhash Jadavani,
Arnd Bergmann, Kevin Liu, Daniel Drake, Ohad Ben-Cohen
On 02/05/13 14:35, Ulf Hansson wrote:
> On 2 May 2013 12:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 16/04/13 13:00, Ulf Hansson wrote:
>>> Aggressive power management is suitable when saving power is
>>> essential. At request inactivity timeout, aka pm runtime
>>> autosuspend timeout, the card will be suspended.
>>>
>>> Once a new request arrives, the card will be re-initalized and
>>> thus the first request will suffer from a latency. This latency
>>> is card-specific, experiments has shown in general that SD-cards
>>> has quite poor initialization time, around 300ms-1100ms. eMMC is
>>> not surprisingly far better but still a couple of hundreds of ms
>>> has been observed.
>>>
>>> Except for the request latency, it is important to know that
>>> suspending the card will also prevent the card from executing
>>> internal house-keeping operations in idle mode. This could mean
>>> degradation in performance.
>>>
>>> To use this feature make sure the request inactivity timeout is
>>> chosen carefully. This has not been done as a part of this patch.
>>>
>>> Enable this feature by using host cap MMC_CAP_AGGRESSIVE_PM and
>>> by setting CONFIG_MMC_UNSAFE_RESUME.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Cc: Maya Erez <merez@codeaurora.org>
>>> Cc: Subhash Jadavani <subhashj@codeaurora.org>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Kevin Liu <kliu5@marvell.com>
>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>> Cc: Daniel Drake <dsd@laptop.org>
>>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>>> ---
>>> drivers/mmc/core/mmc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>>> drivers/mmc/core/sd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/mmc/host.h | 2 +-
>>> 3 files changed, 86 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index bf19058..8dfbc84 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1454,6 +1454,47 @@ static int mmc_resume(struct mmc_host *host)
>>> return err;
>>> }
>>>
>>> +
>>> +/*
>>> + * Callback for runtime_suspend.
>>> + */
>>> +static int mmc_runtime_suspend(struct mmc_host *host)
>>> +{
>>> + int err;
>>> +
>>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>>> + return 0;
>>> +
>>
>> mmc_power_off() needs to be within mmc_claim_host() / mmc_release_host().
>>
>> Claiming is nested, so you can out put mmc_claim_host() here:
>>
>> mmc_claim_host(host);
>
> OK
>
>>
>>
>>> + err = mmc_suspend(host);
>>> + if (err) {
>>> + pr_err("%s: error %d doing aggessive suspend\n",
>>> + mmc_hostname(host), err);
>>> + return err;
>>
>> goto out;
>>
>>> + }
>>> +
>>> + mmc_power_off(host);
>>
>> out:
>> mmc_release_host(host);
>>
>
> OK
>
>>> + return err;
>>> +}
>>> +
>>> +/*
>>> + * Callback for runtime_resume.
>>> + */
>>> +static int mmc_runtime_resume(struct mmc_host *host)
>>> +{
>>> + int err;
>>> +
>>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>>> + return 0;
>>> +
>>> + mmc_power_up(host);
>>
>> As above
>>
>
> OK
>
>>> + err = mmc_resume(host);
>>> + if (err)
>>> + pr_err("%s: error %d doing aggessive resume\n",
>>> + mmc_hostname(host), err);
>>> +
>>> + return err;
>>
>> The power is on - leaving the device in a RPM_SUSPENDED state does not seem
>> useful so better to return zero here.
>
> OK
>
>>
>>> +}
>>> +
>>> static int mmc_power_restore(struct mmc_host *host)
>>> {
>>> int ret;
>>> @@ -1514,6 +1555,8 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>>> .detect = mmc_detect,
>>> .suspend = mmc_suspend,
>>> .resume = mmc_resume,
>>> + .runtime_suspend = mmc_runtime_suspend,
>>> + .runtime_resume = mmc_runtime_resume,
>>> .power_restore = mmc_power_restore,
>>> .alive = mmc_alive,
>>> };
>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>> index 30387d6..e0458f9 100644
>>> --- a/drivers/mmc/core/sd.c
>>> +++ b/drivers/mmc/core/sd.c
>>> @@ -1095,6 +1095,46 @@ static int mmc_sd_resume(struct mmc_host *host)
>>> return err;
>>> }
>>>
>>> +/*
>>> + * Callback for runtime_suspend.
>>> + */
>>> +static int mmc_sd_runtime_suspend(struct mmc_host *host)
>>> +{
>>> + int err;
>>> +
>>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>>> + return 0;
>>> +
>>> + err = mmc_sd_suspend(host);
>>> + if (err) {
>>> + pr_err("%s: error %d doing aggessive suspend\n",
>>> + mmc_hostname(host), err);
>>> + return err;
>>> + }
>>> +
>>> + mmc_power_off(host);
>>
>> As above
>
> OK
>
>>
>>> + return err;
>>> +}
>>> +
>>> +/*
>>> + * Callback for runtime_resume.
>>> + */
>>> +static int mmc_sd_runtime_resume(struct mmc_host *host)
>>> +{
>>> + int err;
>>> +
>>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
>>> + return 0;
>>> +
>>> + mmc_power_up(host);
>>
>> As above
>
> OK
>
>>
>>> + err = mmc_sd_resume(host);
>>> + if (err)
>>> + pr_err("%s: error %d doing aggessive resume\n",
>>> + mmc_hostname(host), err);
>>> +
>>> + return err;
>>
>> As above
>
> OK
>
>>
>> return 0
>>
>>> +}
>>> +
>>> static int mmc_sd_power_restore(struct mmc_host *host)
>>> {
>>> int ret;
>>> @@ -1119,6 +1159,8 @@ static const struct mmc_bus_ops mmc_sd_ops = {
>>> static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
>>> .remove = mmc_sd_remove,
>>> .detect = mmc_sd_detect,
>>> + .runtime_suspend = mmc_sd_runtime_suspend,
>>> + .runtime_resume = mmc_sd_runtime_resume,
>>> .suspend = mmc_sd_suspend,
>>> .resume = mmc_sd_resume,
>>> .power_restore = mmc_sd_power_restore,
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index 17d7148..cec6684 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -239,7 +239,7 @@ struct mmc_host {
>>> #define MMC_CAP_SPI (1 << 4) /* Talks only SPI protocols */
>>> #define MMC_CAP_NEEDS_POLL (1 << 5) /* Needs polling for card-detection */
>>> #define MMC_CAP_8_BIT_DATA (1 << 6) /* Can the host do 8 bit transfers */
>>> -
>>> +#define MMC_CAP_AGGRESSIVE_PM (1 << 7) /* Suspend (e)MMC/SD at idle */
>>
>> Using a "cap" is not ideal here - it should really be under the control of
>> user space.
>
> Several other caps could be debated whether these should exist here as
> well, but let's leave that to a separate discussion.
> Until there are a solution for how to add new mmc features per host,
> which should be runtime configurable, I believe this is the only way
> we can do it.
Yes, I guess it can be improved later.
>
> Do note, that runtime pm can be disabled from user space, which
> indirectly will prevent this feature from being used.
>
>>
>>> #define MMC_CAP_NONREMOVABLE (1 << 8) /* Nonremovable e.g. eMMC */
>>> #define MMC_CAP_WAIT_WHILE_BUSY (1 << 9) /* Waits while card is busy */
>>> #define MMC_CAP_ERASE (1 << 10) /* Allow erase/trim commands */
>>>
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread