[PATCH 0/3] mmc: Use runtime pm for blkdevice

[PATCH 0/3] mmc: Use runtime pm for blkdevice

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

SDIO has been using runtime pm for a while to handle runtime power save
operations. This patchset is enabling the option to make the sd/mmc
blockdevices to use runtime pm as well.

The runtime pm implementation for the block device will make use of
autosuspend to defer power save operation to after request inactivty for
a certain time.

To actually perform some power save operations the corresponding bus ops
for mmc and sd shall be implemented. Typically it could make sense to do
BKOPS for eMMC in here.

Ulf Hansson (3):
  mmc: core: Remove power_restore bus_ops for mmc and sd
  mmc: core: Add bus_ops for runtime pm callbacks
  mmc: block: Enable runtime pm for mmc blkdevice

 drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
 drivers/mmc/core/bus.c   |   14 ++++++++++++--
 drivers/mmc/core/core.h  |    2 ++
 drivers/mmc/core/mmc.c   |   14 --------------
 drivers/mmc/core/sd.c    |   14 --------------
 drivers/mmc/core/sdio.c  |   20 ++++++++++++++++++++
 6 files changed, 60 insertions(+), 32 deletions(-)

-- 
1.7.10

[PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
func drivers are also moving towards use of runtime pm to accomplish the
the same operation and since this API is not used for mmc and sd it makes
sense to remove the corresponding bus_ops.

Moreover, at least for eMMC the mmc bus_ops is broken, since no consideration
is taken for mmc sleep and mmc power off notify.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c |   14 --------------
 drivers/mmc/core/sd.c  |   14 --------------
 2 files changed, 28 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c8f3d6e..edc6bc4 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1445,18 +1445,6 @@ static int mmc_resume(struct mmc_host *host)
 	return err;
 }
 
-static int mmc_power_restore(struct mmc_host *host)
-{
-	int ret;
-
-	host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
-	mmc_claim_host(host);
-	ret = mmc_init_card(host, host->ocr, host->card);
-	mmc_release_host(host);
-
-	return ret;
-}
-
 static int mmc_sleep(struct mmc_host *host)
 {
 	struct mmc_card *card = host->card;
@@ -1494,7 +1482,6 @@ static const struct mmc_bus_ops mmc_ops = {
 	.detect = mmc_detect,
 	.suspend = NULL,
 	.resume = NULL,
-	.power_restore = mmc_power_restore,
 	.alive = mmc_alive,
 };
 
@@ -1505,7 +1492,6 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
 	.detect = mmc_detect,
 	.suspend = mmc_suspend,
 	.resume = mmc_resume,
-	.power_restore = mmc_power_restore,
 	.alive = mmc_alive,
 };
 
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 9e645e1..b71d906 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1095,24 +1095,11 @@ static int mmc_sd_resume(struct mmc_host *host)
 	return err;
 }
 
-static int mmc_sd_power_restore(struct mmc_host *host)
-{
-	int ret;
-
-	host->card->state &= ~MMC_STATE_HIGHSPEED;
-	mmc_claim_host(host);
-	ret = mmc_sd_init_card(host, host->ocr, host->card);
-	mmc_release_host(host);
-
-	return ret;
-}
-
 static const struct mmc_bus_ops mmc_sd_ops = {
 	.remove = mmc_sd_remove,
 	.detect = mmc_sd_detect,
 	.suspend = NULL,
 	.resume = NULL,
-	.power_restore = mmc_sd_power_restore,
 	.alive = mmc_sd_alive,
 };
 
@@ -1121,7 +1108,6 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
 	.detect = mmc_sd_detect,
 	.suspend = mmc_sd_suspend,
 	.resume = mmc_sd_resume,
-	.power_restore = mmc_sd_power_restore,
 	.alive = mmc_sd_alive,
 };
 
-- 
1.7.10

Re: [PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[merez]

Acked-by: Maya Erez <merez@codeaurora.org>

> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> The mmc_power_restore|save_host API is only used by SDIO func drivers.
> SDIO
> func drivers are also moving towards use of runtime pm to accomplish the
> the same operation and since this API is not used for mmc and sd it makes
> sense to remove the corresponding bus_ops.
>
> Moreover, at least for eMMC the mmc bus_ops is broken, since no
> consideration
> is taken for mmc sleep and mmc power off notify.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c |   14 --------------
>  drivers/mmc/core/sd.c  |   14 --------------
>  2 files changed, 28 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c8f3d6e..edc6bc4 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1445,18 +1445,6 @@ static int mmc_resume(struct mmc_host *host)
>  	return err;
>  }
>
> -static int mmc_power_restore(struct mmc_host *host)
> -{
> -	int ret;
> -
> -	host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
> -	mmc_claim_host(host);
> -	ret = mmc_init_card(host, host->ocr, host->card);
> -	mmc_release_host(host);
> -
> -	return ret;
> -}
> -
>  static int mmc_sleep(struct mmc_host *host)
>  {
>  	struct mmc_card *card = host->card;
> @@ -1494,7 +1482,6 @@ static const struct mmc_bus_ops mmc_ops = {
>  	.detect = mmc_detect,
>  	.suspend = NULL,
>  	.resume = NULL,
> -	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
>  };
>
> @@ -1505,7 +1492,6 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>  	.detect = mmc_detect,
>  	.suspend = mmc_suspend,
>  	.resume = mmc_resume,
> -	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
>  };
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 9e645e1..b71d906 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1095,24 +1095,11 @@ static int mmc_sd_resume(struct mmc_host *host)
>  	return err;
>  }
>
> -static int mmc_sd_power_restore(struct mmc_host *host)
> -{
> -	int ret;
> -
> -	host->card->state &= ~MMC_STATE_HIGHSPEED;
> -	mmc_claim_host(host);
> -	ret = mmc_sd_init_card(host, host->ocr, host->card);
> -	mmc_release_host(host);
> -
> -	return ret;
> -}
> -
>  static const struct mmc_bus_ops mmc_sd_ops = {
>  	.remove = mmc_sd_remove,
>  	.detect = mmc_sd_detect,
>  	.suspend = NULL,
>  	.resume = NULL,
> -	.power_restore = mmc_sd_power_restore,
>  	.alive = mmc_sd_alive,
>  };
>
> @@ -1121,7 +1108,6 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe =
> {
>  	.detect = mmc_sd_detect,
>  	.suspend = mmc_sd_suspend,
>  	.resume = mmc_sd_resume,
> -	.power_restore = mmc_sd_power_restore,
>  	.alive = mmc_sd_alive,
>  };
>
> --
> 1.7.10
>
> --
> 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
>


-- 
Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[Adrian Hunter]

On 01/03/13 14:47, Ulf Hansson wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
> 
> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO

NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()

> func drivers are also moving towards use of runtime pm to accomplish the
> the same operation and since this API is not used for mmc and sd it makes
> sense to remove the corresponding bus_ops.
> 
> Moreover, at least for eMMC the mmc bus_ops is broken, since no consideration
> is taken for mmc sleep and mmc power off notify.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c |   14 --------------
>  drivers/mmc/core/sd.c  |   14 --------------
>  2 files changed, 28 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index c8f3d6e..edc6bc4 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1445,18 +1445,6 @@ static int mmc_resume(struct mmc_host *host)
>  	return err;
>  }
>  
> -static int mmc_power_restore(struct mmc_host *host)
> -{
> -	int ret;
> -
> -	host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
> -	mmc_claim_host(host);
> -	ret = mmc_init_card(host, host->ocr, host->card);
> -	mmc_release_host(host);
> -
> -	return ret;
> -}
> -
>  static int mmc_sleep(struct mmc_host *host)
>  {
>  	struct mmc_card *card = host->card;
> @@ -1494,7 +1482,6 @@ static const struct mmc_bus_ops mmc_ops = {
>  	.detect = mmc_detect,
>  	.suspend = NULL,
>  	.resume = NULL,
> -	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
>  };
>  
> @@ -1505,7 +1492,6 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>  	.detect = mmc_detect,
>  	.suspend = mmc_suspend,
>  	.resume = mmc_resume,
> -	.power_restore = mmc_power_restore,
>  	.alive = mmc_alive,
>  };
>  
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 9e645e1..b71d906 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1095,24 +1095,11 @@ static int mmc_sd_resume(struct mmc_host *host)
>  	return err;
>  }
>  
> -static int mmc_sd_power_restore(struct mmc_host *host)
> -{
> -	int ret;
> -
> -	host->card->state &= ~MMC_STATE_HIGHSPEED;
> -	mmc_claim_host(host);
> -	ret = mmc_sd_init_card(host, host->ocr, host->card);
> -	mmc_release_host(host);
> -
> -	return ret;
> -}
> -
>  static const struct mmc_bus_ops mmc_sd_ops = {
>  	.remove = mmc_sd_remove,
>  	.detect = mmc_sd_detect,
>  	.suspend = NULL,
>  	.resume = NULL,
> -	.power_restore = mmc_sd_power_restore,
>  	.alive = mmc_sd_alive,
>  };
>  
> @@ -1121,7 +1108,6 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
>  	.detect = mmc_sd_detect,
>  	.suspend = mmc_sd_suspend,
>  	.resume = mmc_sd_resume,
> -	.power_restore = mmc_sd_power_restore,
>  	.alive = mmc_sd_alive,
>  };
>  
> 

Re: [PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[Ulf Hansson]

On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 01/03/13 14:47, Ulf Hansson wrote:
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>
> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()

True for eMMC, but for SD card the bus_ops can go away. Thanks for
spotting this Adrian!

Although, I see some serious problems with the mmc_do_hw_reset
function - it could cause eMMC data corruption.

Issuing hw reset and doing re-initialization by using the mmc
bus_ops->power_restore will mean no consideration is taken to "cache
ctrl", "bkops" and "power off notify". I think it must.

So the more proper way instead of calling power_restore, should be to
use bus_ops->suspend and bus_ops->resume callbacks from the
mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
successfully, we should be able to skip the actual hw reset and just
do bus_ops->resume.

Do you have any thoughts on this?

Kind regards
Ulf Hansson

>
>> func drivers are also moving towards use of runtime pm to accomplish the
>> the same operation and since this API is not used for mmc and sd it makes
>> sense to remove the corresponding bus_ops.
>>
>> Moreover, at least for eMMC the mmc bus_ops is broken, since no consideration
>> is taken for mmc sleep and mmc power off notify.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/mmc.c |   14 --------------
>>  drivers/mmc/core/sd.c  |   14 --------------
>>  2 files changed, 28 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index c8f3d6e..edc6bc4 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1445,18 +1445,6 @@ static int mmc_resume(struct mmc_host *host)
>>       return err;
>>  }
>>
>> -static int mmc_power_restore(struct mmc_host *host)
>> -{
>> -     int ret;
>> -
>> -     host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
>> -     mmc_claim_host(host);
>> -     ret = mmc_init_card(host, host->ocr, host->card);
>> -     mmc_release_host(host);
>> -
>> -     return ret;
>> -}
>> -
>>  static int mmc_sleep(struct mmc_host *host)
>>  {
>>       struct mmc_card *card = host->card;
>> @@ -1494,7 +1482,6 @@ static const struct mmc_bus_ops mmc_ops = {
>>       .detect = mmc_detect,
>>       .suspend = NULL,
>>       .resume = NULL,
>> -     .power_restore = mmc_power_restore,
>>       .alive = mmc_alive,
>>  };
>>
>> @@ -1505,7 +1492,6 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>>       .detect = mmc_detect,
>>       .suspend = mmc_suspend,
>>       .resume = mmc_resume,
>> -     .power_restore = mmc_power_restore,
>>       .alive = mmc_alive,
>>  };
>>
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 9e645e1..b71d906 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -1095,24 +1095,11 @@ static int mmc_sd_resume(struct mmc_host *host)
>>       return err;
>>  }
>>
>> -static int mmc_sd_power_restore(struct mmc_host *host)
>> -{
>> -     int ret;
>> -
>> -     host->card->state &= ~MMC_STATE_HIGHSPEED;
>> -     mmc_claim_host(host);
>> -     ret = mmc_sd_init_card(host, host->ocr, host->card);
>> -     mmc_release_host(host);
>> -
>> -     return ret;
>> -}
>> -
>>  static const struct mmc_bus_ops mmc_sd_ops = {
>>       .remove = mmc_sd_remove,
>>       .detect = mmc_sd_detect,
>>       .suspend = NULL,
>>       .resume = NULL,
>> -     .power_restore = mmc_sd_power_restore,
>>       .alive = mmc_sd_alive,
>>  };
>>
>> @@ -1121,7 +1108,6 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
>>       .detect = mmc_sd_detect,
>>       .suspend = mmc_sd_suspend,
>>       .resume = mmc_sd_resume,
>> -     .power_restore = mmc_sd_power_restore,
>>       .alive = mmc_sd_alive,
>>  };
>>
>>
>

Re: [PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[Adrian Hunter]

On 04/04/13 12:55, Ulf Hansson wrote:
> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 01/03/13 14:47, Ulf Hansson wrote:
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>
>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
> 
> True for eMMC, but for SD card the bus_ops can go away. Thanks for
> spotting this Adrian!
> 
> Although, I see some serious problems with the mmc_do_hw_reset
> function - it could cause eMMC data corruption.
> 
> Issuing hw reset and doing re-initialization by using the mmc
> bus_ops->power_restore will mean no consideration is taken to "cache
> ctrl", "bkops" and "power off notify". I think it must.
> 
> So the more proper way instead of calling power_restore, should be to
> use bus_ops->suspend and bus_ops->resume callbacks from the
> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
> successfully, we should be able to skip the actual hw reset and just
> do bus_ops->resume.
> 
> Do you have any thoughts on this?

Certainly the bootloader should leave the eMMC is a safe state including:
flushing the cache or turning it off (why did it turn on?), stopping
background operations (why did it start them?), disabling power-off
notification CMD0? (again why it it enable it?)

Note that according to spec. CMD0 anyway clears the cache so you have lost
your data anyway.

Re: [PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[Ulf Hansson]

On 4 April 2013 13:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/04/13 12:55, Ulf Hansson wrote:
>> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 01/03/13 14:47, Ulf Hansson wrote:
>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>>
>>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
>>
>> True for eMMC, but for SD card the bus_ops can go away. Thanks for
>> spotting this Adrian!
>>
>> Although, I see some serious problems with the mmc_do_hw_reset
>> function - it could cause eMMC data corruption.
>>
>> Issuing hw reset and doing re-initialization by using the mmc
>> bus_ops->power_restore will mean no consideration is taken to "cache
>> ctrl", "bkops" and "power off notify". I think it must.
>>
>> So the more proper way instead of calling power_restore, should be to
>> use bus_ops->suspend and bus_ops->resume callbacks from the
>> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
>> successfully, we should be able to skip the actual hw reset and just
>> do bus_ops->resume.
>>
>> Do you have any thoughts on this?
>
> Certainly the bootloader should leave the eMMC is a safe state including:
> flushing the cache or turning it off (why did it turn on?), stopping
> background operations (why did it start them?), disabling power-off
> notification CMD0? (again why it it enable it?)

Not sure what you mean here. What has a booloader to do with this?

>
> Note that according to spec. CMD0 anyway clears the cache so you have lost
> your data anyway.

What I am saying that we can try send "cache ctrl" and "power off
notify" before we send CMD0 / do hw reset. The no data shall be lost.

>
>

Re: [PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[Adrian Hunter]

On 04/04/13 14:52, Ulf Hansson wrote:
> On 4 April 2013 13:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 04/04/13 12:55, Ulf Hansson wrote:
>>> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 01/03/13 14:47, Ulf Hansson wrote:
>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>
>>>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>>>
>>>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
>>>
>>> True for eMMC, but for SD card the bus_ops can go away. Thanks for
>>> spotting this Adrian!
>>>
>>> Although, I see some serious problems with the mmc_do_hw_reset
>>> function - it could cause eMMC data corruption.
>>>
>>> Issuing hw reset and doing re-initialization by using the mmc
>>> bus_ops->power_restore will mean no consideration is taken to "cache
>>> ctrl", "bkops" and "power off notify". I think it must.
>>>
>>> So the more proper way instead of calling power_restore, should be to
>>> use bus_ops->suspend and bus_ops->resume callbacks from the
>>> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
>>> successfully, we should be able to skip the actual hw reset and just
>>> do bus_ops->resume.
>>>
>>> Do you have any thoughts on this?
>>
>> Certainly the bootloader should leave the eMMC is a safe state including:
>> flushing the cache or turning it off (why did it turn on?), stopping
>> background operations (why did it start them?), disabling power-off
>> notification CMD0? (again why it it enable it?)
> 
> Not sure what you mean here. What has a booloader to do with this?

When do you think hw reset is used?

> 
>>
>> Note that according to spec. CMD0 anyway clears the cache so you have lost
>> your data anyway.
> 
> What I am saying that we can try send "cache ctrl" and "power off
> notify" before we send CMD0 / do hw reset. The no data shall be lost.

With an uninitialized bus?  Or an unresponsive card?

Re: [PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[Ulf Hansson]

On 4 April 2013 14:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/04/13 14:52, Ulf Hansson wrote:
>> On 4 April 2013 13:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 04/04/13 12:55, Ulf Hansson wrote:
>>>> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 01/03/13 14:47, Ulf Hansson wrote:
>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>
>>>>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>>>>
>>>>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
>>>>
>>>> True for eMMC, but for SD card the bus_ops can go away. Thanks for
>>>> spotting this Adrian!
>>>>
>>>> Although, I see some serious problems with the mmc_do_hw_reset
>>>> function - it could cause eMMC data corruption.
>>>>
>>>> Issuing hw reset and doing re-initialization by using the mmc
>>>> bus_ops->power_restore will mean no consideration is taken to "cache
>>>> ctrl", "bkops" and "power off notify". I think it must.
>>>>
>>>> So the more proper way instead of calling power_restore, should be to
>>>> use bus_ops->suspend and bus_ops->resume callbacks from the
>>>> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
>>>> successfully, we should be able to skip the actual hw reset and just
>>>> do bus_ops->resume.
>>>>
>>>> Do you have any thoughts on this?
>>>
>>> Certainly the bootloader should leave the eMMC is a safe state including:
>>> flushing the cache or turning it off (why did it turn on?), stopping
>>> background operations (why did it start them?), disabling power-off
>>> notification CMD0? (again why it it enable it?)
>>
>> Not sure what you mean here. What has a booloader to do with this?
>
> When do you think hw reset is used?

Two types is being used.

1. At the mmc_rescan sequence. At this point mmc_do_hw_reset is _not_
used. Instead mmc_hw_reset_for_init, which onlcy makes use of
host->ops->hw_reset, no "power_restore" of course. So this has no
issues whatsoever with this patch.

2. At blk request errors, which I thought we were discussing from the
beginning. In this case mmc_do_hw_reset is used. And it here my
proposals doing bus_ops->suspend|resume make sense.


>
>>
>>>
>>> Note that according to spec. CMD0 anyway clears the cache so you have lost
>>> your data anyway.
>>
>> What I am saying that we can try send "cache ctrl" and "power off
>> notify" before we send CMD0 / do hw reset. The no data shall be lost.
>
> With an uninitialized bus?  Or an unresponsive card?

See above.

The bus is never uninitialized.

The card has responded with an error, but that does not have to mean
that it is completely "unresponsive".

>

Re: [PATCH 1/3] mmc: core: Remove power_restore bus_ops for mmc and sd

[Adrian Hunter]

On 04/04/13 17:58, Ulf Hansson wrote:
> On 4 April 2013 14:00, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 04/04/13 14:52, Ulf Hansson wrote:
>>> On 4 April 2013 13:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 04/04/13 12:55, Ulf Hansson wrote:
>>>>> On 4 April 2013 10:46, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> On 01/03/13 14:47, Ulf Hansson wrote:
>>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>>
>>>>>>> The mmc_power_restore|save_host API is only used by SDIO func drivers. SDIO
>>>>>>
>>>>>> NAK - it is also used by eMMC hardware reset i.e. mmc_do_hw_reset()
>>>>>
>>>>> True for eMMC, but for SD card the bus_ops can go away. Thanks for
>>>>> spotting this Adrian!
>>>>>
>>>>> Although, I see some serious problems with the mmc_do_hw_reset
>>>>> function - it could cause eMMC data corruption.
>>>>>
>>>>> Issuing hw reset and doing re-initialization by using the mmc
>>>>> bus_ops->power_restore will mean no consideration is taken to "cache
>>>>> ctrl", "bkops" and "power off notify". I think it must.
>>>>>
>>>>> So the more proper way instead of calling power_restore, should be to
>>>>> use bus_ops->suspend and bus_ops->resume callbacks from the
>>>>> mmc_do_hw_reset function. Additionally if bus_ops->suspend is done
>>>>> successfully, we should be able to skip the actual hw reset and just
>>>>> do bus_ops->resume.
>>>>>
>>>>> Do you have any thoughts on this?
>>>>
>>>> Certainly the bootloader should leave the eMMC is a safe state including:
>>>> flushing the cache or turning it off (why did it turn on?), stopping
>>>> background operations (why did it start them?), disabling power-off
>>>> notification CMD0? (again why it it enable it?)
>>>
>>> Not sure what you mean here. What has a booloader to do with this?
>>
>> When do you think hw reset is used?
> 
> Two types is being used.
> 
> 1. At the mmc_rescan sequence. At this point mmc_do_hw_reset is _not_
> used. Instead mmc_hw_reset_for_init, which onlcy makes use of
> host->ops->hw_reset, no "power_restore" of course. So this has no
> issues whatsoever with this patch.
> 
> 2. At blk request errors, which I thought we were discussing from the
> beginning. In this case mmc_do_hw_reset is used. And it here my
> proposals doing bus_ops->suspend|resume make sense.
> 
> 
>>
>>>
>>>>
>>>> Note that according to spec. CMD0 anyway clears the cache so you have lost
>>>> your data anyway.
>>>
>>> What I am saying that we can try send "cache ctrl" and "power off
>>> notify" before we send CMD0 / do hw reset. The no data shall be lost.
>>
>> With an uninitialized bus?  Or an unresponsive card?
> 
> See above.
> 
> The bus is never uninitialized.
> 
> The card has responded with an error, but that does not have to mean
> that it is completely "unresponsive".

True.  Arguably caching should be disabled at the first sign of trouble
and never re-enabled.

However reset could attempt to flush the cache.  Then, even if the reset is
successful, an error must still be returned if the flush failed.

[PATCH 2/3] mmc: core: Add bus_ops for runtime pm callbacks

[Ulf Hansson]

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 functionallity is maintained.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/bus.c  |   14 ++++++++++++--
 drivers/mmc/core/core.h |    2 ++
 drivers/mmc/core/sdio.c |   20 ++++++++++++++++++++
 3 files changed, 34 insertions(+), 2 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.h b/drivers/mmc/core/core.h
index b9f18a2..9445519 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 *);
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index aa0719a..0e6e8c1 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -988,6 +988,24 @@ static int mmc_sdio_resume(struct mmc_host *host)
 	return err;
 }
 
+static int mmc_sdio_runtime_suspend(struct mmc_host *host)
+{
+	/*
+	 * Once sdio clients has entirely switched to runtime pm we wrap
+	 * the call to power_save here.
+	 */
+	return mmc_power_save_host(host);
+}
+
+static int mmc_sdio_runtime_resume(struct mmc_host *host)
+{
+	/*
+	 * Once sdio clients has entirely switched to runtime pm we wrap
+	 * the call to power_restore here.
+	 */
+	return mmc_power_restore_host(host);
+}
+
 static int mmc_sdio_power_restore(struct mmc_host *host)
 {
 	int ret;
@@ -1054,6 +1072,8 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 	.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

Re: [PATCH 2/3] mmc: core: Add bus_ops for runtime pm callbacks

[merez]

Acked-by: Maya Erez <merez@codeaurora.org>

> 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 functionallity is maintained.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/bus.c  |   14 ++++++++++++--
>  drivers/mmc/core/core.h |    2 ++
>  drivers/mmc/core/sdio.c |   20 ++++++++++++++++++++
>  3 files changed, 34 insertions(+), 2 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.h b/drivers/mmc/core/core.h
> index b9f18a2..9445519 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 *);
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index aa0719a..0e6e8c1 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -988,6 +988,24 @@ static int mmc_sdio_resume(struct mmc_host *host)
>  	return err;
>  }
>
> +static int mmc_sdio_runtime_suspend(struct mmc_host *host)
> +{
> +	/*
> +	 * Once sdio clients has entirely switched to runtime pm we wrap
> +	 * the call to power_save here.
> +	 */
> +	return mmc_power_save_host(host);
> +}
> +
> +static int mmc_sdio_runtime_resume(struct mmc_host *host)
> +{
> +	/*
> +	 * Once sdio clients has entirely switched to runtime pm we wrap
> +	 * the call to power_restore here.
> +	 */
> +	return mmc_power_restore_host(host);
> +}
> +
>  static int mmc_sdio_power_restore(struct mmc_host *host)
>  {
>  	int ret;
> @@ -1054,6 +1072,8 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
>  	.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
>
> --
> 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
>


-- 
Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

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.

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 what the
host suspend sequence to be unaware of any runtime power save operations,
so it can just handle the suspend as the device is fully powerered from a
runtime perspective.

This patch is preparing 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>
---
 drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
 	md = mmc_blk_get(dev_to_disk(dev));
 	card = md->queue.card;
 
+	pm_runtime_get_sync(&card->dev);
 	mmc_claim_host(card->host);
 
 	ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
@@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device *dev,
 		card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
 
 	mmc_release_host(card->host);
+	pm_runtime_mark_last_busy(&card->dev);
+	pm_runtime_put_autosuspend(&card->dev);
 
 	if (!ret) {
 		pr_info("%s: Locking boot partition ro until next power on\n",
@@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 	mrq.cmd = &cmd;
 
+	pm_runtime_get_sync(&card->dev);
 	mmc_claim_host(card->host);
 
 	err = mmc_blk_part_switch(card, md);
@@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 cmd_rel_host:
 	mmc_release_host(card->host);
+	pm_runtime_mark_last_busy(&card->dev);
+	pm_runtime_put_autosuspend(&card->dev);
 
 cmd_done:
 	mmc_blk_put(md);
@@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 	struct mmc_host *host = card->host;
 	unsigned long flags;
 
-	if (req && !mq->mqrq_prev->req)
+	if (req && !mq->mqrq_prev->req) {
+		pm_runtime_get_sync(&card->dev);
 		/* claim host only for the first request */
 		mmc_claim_host(card->host);
+	}
 
 	ret = mmc_blk_part_switch(card, md);
 	if (ret) {
@@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 	}
 
 out:
-	if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
+	if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
 		/* release host only when there are no more requests */
 		mmc_release_host(card->host);
+		pm_runtime_mark_last_busy(&card->dev);
+		pm_runtime_put_autosuspend(&card->dev);
+	}
+
 	return ret;
 }
 
@@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct mmc_card *card)
 		if (mmc_add_disk(part_md))
 			goto out;
 	}
+
+	pm_runtime_set_active(&card->dev);
+	pm_runtime_set_autosuspend_delay(&card->dev, 3000);
+	pm_runtime_use_autosuspend(&card->dev);
+	pm_runtime_enable(&card->dev);
+
 	return 0;
 
  out:
@@ -2344,9 +2363,12 @@ 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);
+	pm_runtime_disable(&card->dev);
+	pm_runtime_put_noidle(&card->dev);
 	mmc_blk_remove_req(md);
 	mmc_set_drvdata(card, NULL);
 }
@@ -2358,6 +2380,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);
@@ -2381,6 +2404,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;
 }
-- 
1.7.10

Re: [PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[Asutosh Das]

On 3/1/2013 6:17 PM, 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.
>
> 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 what the
> host suspend sequence to be unaware of any runtime power save operations,
> so it can just handle the suspend as the device is fully powerered from a
> runtime perspective.
>
> This patch is preparing 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>
> ---
>   drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>   1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
>   	md = mmc_blk_get(dev_to_disk(dev));
>   	card = md->queue.card;
>   
> +	pm_runtime_get_sync(&card->dev);
>   	mmc_claim_host(card->host);
>   
>   	ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device *dev,
>   		card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>   
>   	mmc_release_host(card->host);
> +	pm_runtime_mark_last_busy(&card->dev);
> +	pm_runtime_put_autosuspend(&card->dev);
>   
>   	if (!ret) {
>   		pr_info("%s: Locking boot partition ro until next power on\n",
> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>   
>   	mrq.cmd = &cmd;
>   
> +	pm_runtime_get_sync(&card->dev);
>   	mmc_claim_host(card->host);
>   
>   	err = mmc_blk_part_switch(card, md);
> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>   
>   cmd_rel_host:
>   	mmc_release_host(card->host);
> +	pm_runtime_mark_last_busy(&card->dev);
> +	pm_runtime_put_autosuspend(&card->dev);
>   
>   cmd_done:
>   	mmc_blk_put(md);
> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>   	struct mmc_host *host = card->host;
>   	unsigned long flags;
>   
> -	if (req && !mq->mqrq_prev->req)
> +	if (req && !mq->mqrq_prev->req) {
> +		pm_runtime_get_sync(&card->dev);
>   		/* claim host only for the first request */
>   		mmc_claim_host(card->host);
> +	}
>   
>   	ret = mmc_blk_part_switch(card, md);
>   	if (ret) {
> @@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
>   	}
>   
>   out:
> -	if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
> +	if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
>   		/* release host only when there are no more requests */
>   		mmc_release_host(card->host);
> +		pm_runtime_mark_last_busy(&card->dev);
> +		pm_runtime_put_autosuspend(&card->dev);
> +	}
> +
>   	return ret;
>   }
>   
> @@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct mmc_card *card)
>   		if (mmc_add_disk(part_md))
>   			goto out;
>   	}
> +
> +	pm_runtime_set_active(&card->dev);
> +	pm_runtime_set_autosuspend_delay(&card->dev, 3000);
> +	pm_runtime_use_autosuspend(&card->dev);
> +	pm_runtime_enable(&card->dev);
> +
>   	return 0;
>   
>    out:
> @@ -2344,9 +2363,12 @@ 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);
> +	pm_runtime_disable(&card->dev);
> +	pm_runtime_put_noidle(&card->dev);
>   	mmc_blk_remove_req(md);
>   	mmc_set_drvdata(card, NULL);
>   }
> @@ -2358,6 +2380,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);
> @@ -2381,6 +2404,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;
>   }
Hi Ulf
Currently, I am implementing a patch that facilitates each device to 
manage is runtime pm on its own.
I am using the parent-child relationship that is already established in 
the mmc stack for this implementation. In this case,
mmc_card is a child of mmc_host which is in turn is the child of 
platform-device.
The following contexts exist which would have to invoke get_sync and 
put_sync on the mmc_card device:
1. mmcqd
2. bkops
3. mmc_rescan

The get_sync on card device would resume all the 3 devices starting from 
the platform-device and the put-sync would suspend all the 3 devices 
starting from the card-device.
pm_auto_suspend/pm_schedule_suspend may be used to trigger the suspend 
from the above contexts.
I believe this would simplify the runtime pm functionality.

I can put up the patch for review in a couple of days.

Please let me know your opinion about this approach.

--
Thanks
Asutosh

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

On 4 March 2013 21:48, Asutosh Das <asutoshd@codeaurora.org> wrote:
> On 3/1/2013 6:17 PM, 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.
>>
>> 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 what the
>> host suspend sequence to be unaware of any runtime power save operations,
>> so it can just handle the suspend as the device is fully powerered from a
>> runtime perspective.
>>
>> This patch is preparing 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>
>> ---
>>   drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
>>         md = mmc_blk_get(dev_to_disk(dev));
>>         card = md->queue.card;
>>   +     pm_runtime_get_sync(&card->dev);
>>         mmc_claim_host(card->host);
>>         ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
>> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device *dev,
>>                 card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>         mmc_release_host(card->host);
>> +       pm_runtime_mark_last_busy(&card->dev);
>> +       pm_runtime_put_autosuspend(&card->dev);
>>         if (!ret) {
>>                 pr_info("%s: Locking boot partition ro until next power
>> on\n",
>> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>> *bdev,
>>         mrq.cmd = &cmd;
>>   +     pm_runtime_get_sync(&card->dev);
>>         mmc_claim_host(card->host);
>>         err = mmc_blk_part_switch(card, md);
>> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device
>> *bdev,
>>     cmd_rel_host:
>>         mmc_release_host(card->host);
>> +       pm_runtime_mark_last_busy(&card->dev);
>> +       pm_runtime_put_autosuspend(&card->dev);
>>     cmd_done:
>>         mmc_blk_put(md);
>> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>> struct request *req)
>>         struct mmc_host *host = card->host;
>>         unsigned long flags;
>>   -     if (req && !mq->mqrq_prev->req)
>> +       if (req && !mq->mqrq_prev->req) {
>> +               pm_runtime_get_sync(&card->dev);
>>                 /* claim host only for the first request */
>>                 mmc_claim_host(card->host);
>> +       }
>>         ret = mmc_blk_part_switch(card, md);
>>         if (ret) {
>> @@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>> struct request *req)
>>         }
>>     out:
>> -       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
>> +       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
>>                 /* release host only when there are no more requests */
>>                 mmc_release_host(card->host);
>> +               pm_runtime_mark_last_busy(&card->dev);
>> +               pm_runtime_put_autosuspend(&card->dev);
>> +       }
>> +
>>         return ret;
>>   }
>>   @@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct mmc_card *card)
>>                 if (mmc_add_disk(part_md))
>>                         goto out;
>>         }
>> +
>> +       pm_runtime_set_active(&card->dev);
>> +       pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>> +       pm_runtime_use_autosuspend(&card->dev);
>> +       pm_runtime_enable(&card->dev);
>> +
>>         return 0;
>>      out:
>> @@ -2344,9 +2363,12 @@ 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);
>> +       pm_runtime_disable(&card->dev);
>> +       pm_runtime_put_noidle(&card->dev);
>>         mmc_blk_remove_req(md);
>>         mmc_set_drvdata(card, NULL);
>>   }
>> @@ -2358,6 +2380,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);
>> @@ -2381,6 +2404,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;
>>   }
>

Hi Asutosh,


> Hi Ulf
> Currently, I am implementing a patch that facilitates each device to manage
> is runtime pm on its own.
> I am using the parent-child relationship that is already established in the
> mmc stack for this implementation. In this case,
> mmc_card is a child of mmc_host which is in turn is the child of
> platform-device.
> The following contexts exist which would have to invoke get_sync and
> put_sync on the mmc_card device:
> 1. mmcqd
> 2. bkops
> 3. mmc_rescan
>
> The get_sync on card device would resume all the 3 devices starting from the
> platform-device and the put-sync would suspend all the 3 devices starting
> from the card-device.
> pm_auto_suspend/pm_schedule_suspend may be used to trigger the suspend from
> the above contexts.
> I believe this would simplify the runtime pm functionality.
>
> I can put up the patch for review in a couple of days.
>
> Please let me know your opinion about this approach.

No, I don't think this is way of doing it. I will try to elaborate a
bit with the approach I took in my patchset and why.

First of all, the host platform device must be kept separate (no
parent/child and not the same bus) from the card device. There is many
reason to why, but within this context (runtime pm point of view), the
host must be able to handle it's runtime pm resources completely
separate from the blk device (card device) runtime resources. More
precisely and from a BKOPS point of view; while doing BKOPS the host
platform device must still be able to enter runtime power save states.

I realize that in your case, you are doing mmc_suspend|resume_host in
your host drivers runtime callbacks, which is kind of a very special
case, though worth to consider of course. There are two solutions to
enable the option for this functionality. In both cases a host caps
flag will be needed to enable this.

1.
In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
device"), and vice verse in the runtime resume callback. This will
prevent the host driver from entering runtime power save sate while
for example doing BKOPS, thus preventing your host driver from doing
mmc_suspend_host while BKOPS is running.

2.
Move mmc_suspend|resume_host from your host runtime callbacks, into
the bus_ops runtime callbacks. Typically, when no BKOPS is needed
mmc_suspend_host can be done.

What are you thoughts around this?

Kind regards
Ulf Hansson


>
> --
> Thanks
> Asutosh
>
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>

Re: [PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[Asutosh Das]

On 3/5/2013 7:09 AM, Ulf Hansson wrote:
> On 4 March 2013 21:48, Asutosh Das <asutoshd@codeaurora.org> wrote:
>> On 3/1/2013 6:17 PM, 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.
>>>
>>> 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 what the
>>> host suspend sequence to be unaware of any runtime power save operations,
>>> so it can just handle the suspend as the device is fully powerered from a
>>> runtime perspective.
>>>
>>> This patch is preparing 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>
>>> ---
>>>    drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>    1 file changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>> index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
>>>          md = mmc_blk_get(dev_to_disk(dev));
>>>          card = md->queue.card;
>>>    +     pm_runtime_get_sync(&card->dev);
>>>          mmc_claim_host(card->host);
>>>          ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
>>> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device *dev,
>>>                  card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>          mmc_release_host(card->host);
>>> +       pm_runtime_mark_last_busy(&card->dev);
>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>          if (!ret) {
>>>                  pr_info("%s: Locking boot partition ro until next power
>>> on\n",
>>> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>> *bdev,
>>>          mrq.cmd = &cmd;
>>>    +     pm_runtime_get_sync(&card->dev);
>>>          mmc_claim_host(card->host);
>>>          err = mmc_blk_part_switch(card, md);
>>> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>> *bdev,
>>>      cmd_rel_host:
>>>          mmc_release_host(card->host);
>>> +       pm_runtime_mark_last_busy(&card->dev);
>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>      cmd_done:
>>>          mmc_blk_put(md);
>>> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>>> struct request *req)
>>>          struct mmc_host *host = card->host;
>>>          unsigned long flags;
>>>    -     if (req && !mq->mqrq_prev->req)
>>> +       if (req && !mq->mqrq_prev->req) {
>>> +               pm_runtime_get_sync(&card->dev);
>>>                  /* claim host only for the first request */
>>>                  mmc_claim_host(card->host);
>>> +       }
>>>          ret = mmc_blk_part_switch(card, md);
>>>          if (ret) {
>>> @@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>>> struct request *req)
>>>          }
>>>      out:
>>> -       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
>>> +       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
>>>                  /* release host only when there are no more requests */
>>>                  mmc_release_host(card->host);
>>> +               pm_runtime_mark_last_busy(&card->dev);
>>> +               pm_runtime_put_autosuspend(&card->dev);
>>> +       }
>>> +
>>>          return ret;
>>>    }
>>>    @@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct mmc_card *card)
>>>                  if (mmc_add_disk(part_md))
>>>                          goto out;
>>>          }
>>> +
>>> +       pm_runtime_set_active(&card->dev);
>>> +       pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>> +       pm_runtime_use_autosuspend(&card->dev);
>>> +       pm_runtime_enable(&card->dev);
>>> +
>>>          return 0;
>>>       out:
>>> @@ -2344,9 +2363,12 @@ 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);
>>> +       pm_runtime_disable(&card->dev);
>>> +       pm_runtime_put_noidle(&card->dev);
>>>          mmc_blk_remove_req(md);
>>>          mmc_set_drvdata(card, NULL);
>>>    }
>>> @@ -2358,6 +2380,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);
>>> @@ -2381,6 +2404,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;
>>>    }
> Hi Asutosh,
>
>
>> Hi Ulf
>> Currently, I am implementing a patch that facilitates each device to manage
>> is runtime pm on its own.
>> I am using the parent-child relationship that is already established in the
>> mmc stack for this implementation. In this case,
>> mmc_card is a child of mmc_host which is in turn is the child of
>> platform-device.
>> The following contexts exist which would have to invoke get_sync and
>> put_sync on the mmc_card device:
>> 1. mmcqd
>> 2. bkops
>> 3. mmc_rescan
>>
>> The get_sync on card device would resume all the 3 devices starting from the
>> platform-device and the put-sync would suspend all the 3 devices starting
>> from the card-device.
>> pm_auto_suspend/pm_schedule_suspend may be used to trigger the suspend from
>> the above contexts.
>> I believe this would simplify the runtime pm functionality.
>>
>> I can put up the patch for review in a couple of days.
>>
>> Please let me know your opinion about this approach.
> No, I don't think this is way of doing it. I will try to elaborate a
> bit with the approach I took in my patchset and why.
>
> First of all, the host platform device must be kept separate (no
> parent/child and not the same bus) from the card device. There is many
> reason to why, but within this context (runtime pm point of view), the
> host must be able to handle it's runtime pm resources completely
> separate from the blk device (card device) runtime resources. More
> precisely and from a BKOPS point of view; while doing BKOPS the host
> platform device must still be able to enter runtime power save states.
>
> I realize that in your case, you are doing mmc_suspend|resume_host in
> your host drivers runtime callbacks, which is kind of a very special
> case, though worth to consider of course. There are two solutions to
> enable the option for this functionality. In both cases a host caps
> flag will be needed to enable this.
>
> 1.
> In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
> device"), and vice verse in the runtime resume callback. This will
> prevent the host driver from entering runtime power save sate while
> for example doing BKOPS, thus preventing your host driver from doing
> mmc_suspend_host while BKOPS is running.
>
> 2.
> Move mmc_suspend|resume_host from your host runtime callbacks, into
> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
> mmc_suspend_host can be done.
>
> What are you thoughts around this?
>
> Kind regards
> Ulf Hansson
>
>
>> --
>> Thanks
>> Asutosh
>>
>> --
>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>>
Hi Ulf,
Typically, when the pltfm device enters power-save during bkops, it can 
only shut-off the clocks to the card (?), the power would still be on.
With clock-gating in place, this would be done anyhow.

I wanted to separate the functionality as detailed below:

Say we do aggressive power management: (invoke mmc_[suspend/resume]_host 
in runtime pm as well), idle-timeout of 10 s (configurable though)

during suspend of mmc_card device:
- do all card related power management, like power-off notifications etc.

during suspend of mmc_host device:
- do all the host related power management, like power-off host, 
shut-off clocks etc.

during suspend of pltfm device:
- do all the pltfm specific power management, like disable irqs, 
configure wake-ups etc.

During system-suspend, it can be checked if the device is 
runtime-suspended, if yes, return.

On resume, the above path would be retraced in the reverse order.

I think each bus can be responsible for suspending its devices during 
system suspend.

 >> First of all, the host platform device must be kept separate (no 
parent/child and not the same bus) from the card device.
Can you please elaborate on this point a bit.

I guess you would be joining the IRC chat tomorrow, if yes, we can 
discuss there itself.

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

On 6 March 2013 02:04, Asutosh Das <asutoshd@codeaurora.org> wrote:
> On 3/5/2013 7:09 AM, Ulf Hansson wrote:
>>
>> On 4 March 2013 21:48, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>>
>>> On 3/1/2013 6:17 PM, 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.
>>>>
>>>> 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 what the
>>>> host suspend sequence to be unaware of any runtime power save
>>>> operations,
>>>> so it can just handle the suspend as the device is fully powerered from
>>>> a
>>>> runtime perspective.
>>>>
>>>> This patch is preparing 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>
>>>> ---
>>>>    drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>    1 file changed, 26 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>> index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device
>>>> *dev,
>>>>          md = mmc_blk_get(dev_to_disk(dev));
>>>>          card = md->queue.card;
>>>>    +     pm_runtime_get_sync(&card->dev);
>>>>          mmc_claim_host(card->host);
>>>>          ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
>>>> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device
>>>> *dev,
>>>>                  card->ext_csd.boot_ro_lock |=
>>>> EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>>          mmc_release_host(card->host);
>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>          if (!ret) {
>>>>                  pr_info("%s: Locking boot partition ro until next power
>>>> on\n",
>>>> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>> *bdev,
>>>>          mrq.cmd = &cmd;
>>>>    +     pm_runtime_get_sync(&card->dev);
>>>>          mmc_claim_host(card->host);
>>>>          err = mmc_blk_part_switch(card, md);
>>>> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>> *bdev,
>>>>      cmd_rel_host:
>>>>          mmc_release_host(card->host);
>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>      cmd_done:
>>>>          mmc_blk_put(md);
>>>> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>>>> struct request *req)
>>>>          struct mmc_host *host = card->host;
>>>>          unsigned long flags;
>>>>    -     if (req && !mq->mqrq_prev->req)
>>>> +       if (req && !mq->mqrq_prev->req) {
>>>> +               pm_runtime_get_sync(&card->dev);
>>>>                  /* claim host only for the first request */
>>>>                  mmc_claim_host(card->host);
>>>> +       }
>>>>          ret = mmc_blk_part_switch(card, md);
>>>>          if (ret) {
>>>> @@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>>>> struct request *req)
>>>>          }
>>>>      out:
>>>> -       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
>>>> +       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
>>>>                  /* release host only when there are no more requests */
>>>>                  mmc_release_host(card->host);
>>>> +               pm_runtime_mark_last_busy(&card->dev);
>>>> +               pm_runtime_put_autosuspend(&card->dev);
>>>> +       }
>>>> +
>>>>          return ret;
>>>>    }
>>>>    @@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct mmc_card
>>>> *card)
>>>>                  if (mmc_add_disk(part_md))
>>>>                          goto out;
>>>>          }
>>>> +
>>>> +       pm_runtime_set_active(&card->dev);
>>>> +       pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>>> +       pm_runtime_use_autosuspend(&card->dev);
>>>> +       pm_runtime_enable(&card->dev);
>>>> +
>>>>          return 0;
>>>>       out:
>>>> @@ -2344,9 +2363,12 @@ 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);
>>>> +       pm_runtime_disable(&card->dev);
>>>> +       pm_runtime_put_noidle(&card->dev);
>>>>          mmc_blk_remove_req(md);
>>>>          mmc_set_drvdata(card, NULL);
>>>>    }
>>>> @@ -2358,6 +2380,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);
>>>> @@ -2381,6 +2404,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;
>>>>    }
>>
>> Hi Asutosh,
>>
>>
>>> Hi Ulf
>>> Currently, I am implementing a patch that facilitates each device to
>>> manage
>>> is runtime pm on its own.
>>> I am using the parent-child relationship that is already established in
>>> the
>>> mmc stack for this implementation. In this case,
>>> mmc_card is a child of mmc_host which is in turn is the child of
>>> platform-device.
>>> The following contexts exist which would have to invoke get_sync and
>>> put_sync on the mmc_card device:
>>> 1. mmcqd
>>> 2. bkops
>>> 3. mmc_rescan
>>>
>>> The get_sync on card device would resume all the 3 devices starting from
>>> the
>>> platform-device and the put-sync would suspend all the 3 devices starting
>>> from the card-device.
>>> pm_auto_suspend/pm_schedule_suspend may be used to trigger the suspend
>>> from
>>> the above contexts.
>>> I believe this would simplify the runtime pm functionality.
>>>
>>> I can put up the patch for review in a couple of days.
>>>
>>> Please let me know your opinion about this approach.
>>
>> No, I don't think this is way of doing it. I will try to elaborate a
>> bit with the approach I took in my patchset and why.
>>
>> First of all, the host platform device must be kept separate (no
>> parent/child and not the same bus) from the card device. There is many
>> reason to why, but within this context (runtime pm point of view), the
>> host must be able to handle it's runtime pm resources completely
>> separate from the blk device (card device) runtime resources. More
>> precisely and from a BKOPS point of view; while doing BKOPS the host
>> platform device must still be able to enter runtime power save states.
>>
>> I realize that in your case, you are doing mmc_suspend|resume_host in
>> your host drivers runtime callbacks, which is kind of a very special
>> case, though worth to consider of course. There are two solutions to
>> enable the option for this functionality. In both cases a host caps
>> flag will be needed to enable this.
>>
>> 1.
>> In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
>> device"), and vice verse in the runtime resume callback. This will
>> prevent the host driver from entering runtime power save sate while
>> for example doing BKOPS, thus preventing your host driver from doing
>> mmc_suspend_host while BKOPS is running.
>>
>> 2.
>> Move mmc_suspend|resume_host from your host runtime callbacks, into
>> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
>> mmc_suspend_host can be done.
>>
>> What are you thoughts around this?
>>
>> Kind regards
>> Ulf Hansson
>>
>>
>>> --
>>> Thanks
>>> Asutosh
>>>
>>> --
>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum.
>>>
> Hi Ulf,
> Typically, when the pltfm device enters power-save during bkops, it can only
> shut-off the clocks to the card (?), the power would still be on.
> With clock-gating in place, this would be done anyhow.

Clock gating is only one of the things you might want to do.

For example;

Your host plf device can reside in a power domain.
You might want to save power by doing pinctrl.
Disable/enable irqs.
Etc.

So, to conclude it's is realy important runtime pm for the block
device (card device) is kept separate from the host plf device.
They do handle completly different stuff.

>
> I wanted to separate the functionality as detailed below:
>
> Say we do aggressive power management: (invoke mmc_[suspend/resume]_host in
> runtime pm as well), idle-timeout of 10 s (configurable though)
>
> during suspend of mmc_card device:
> - do all card related power management, like power-off notifications etc.
>
> during suspend of mmc_host device:
> - do all the host related power management, like power-off host, shut-off
> clocks etc.
>
> during suspend of pltfm device:
> - do all the pltfm specific power management, like disable irqs, configure
> wake-ups etc.
>
> During system-suspend, it can be checked if the device is runtime-suspended,
> if yes, return.
>
> On resume, the above path would be retraced in the reverse order.
>
> I think each bus can be responsible for suspending its devices during system
> suspend.

According to my comment above, I don't think this sequence will be possible.

>
>
>>> First of all, the host platform device must be kept separate (no
>>> parent/child and not the same bus) from the card device.
> Can you please elaborate on this point a bit.

See above comment for what actions a host plf device can do at runtime
power management.

>
> I guess you would be joining the IRC chat tomorrow, if yes, we can discuss
> there itself.

It wont be possible for me to joing IRC today, I am at Hongkong with
Linaro Connect this week. Although it seems like a good discussion on
IRC, I suppose it would help to clarify on this topic.

>
>
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>

Kind regards
Ulf Hansson

Re: [PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[Asutosh Das]

On 3/6/2013 12:27 PM, Ulf Hansson wrote:
> On 6 March 2013 02:04, Asutosh Das <asutoshd@codeaurora.org> wrote:
>> On 3/5/2013 7:09 AM, Ulf Hansson wrote:
>>> On 4 March 2013 21:48, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>>> On 3/1/2013 6:17 PM, 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.
>>>>>
>>>>> 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 what the
>>>>> host suspend sequence to be unaware of any runtime power save
>>>>> operations,
>>>>> so it can just handle the suspend as the device is fully powerered from
>>>>> a
>>>>> runtime perspective.
>>>>>
>>>>> This patch is preparing 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>
>>>>> ---
>>>>>     drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>>     1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>> index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device
>>>>> *dev,
>>>>>           md = mmc_blk_get(dev_to_disk(dev));
>>>>>           card = md->queue.card;
>>>>>     +     pm_runtime_get_sync(&card->dev);
>>>>>           mmc_claim_host(card->host);
>>>>>           ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
>>>>> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device
>>>>> *dev,
>>>>>                   card->ext_csd.boot_ro_lock |=
>>>>> EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>>>           mmc_release_host(card->host);
>>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>>           if (!ret) {
>>>>>                   pr_info("%s: Locking boot partition ro until next power
>>>>> on\n",
>>>>> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>>> *bdev,
>>>>>           mrq.cmd = &cmd;
>>>>>     +     pm_runtime_get_sync(&card->dev);
>>>>>           mmc_claim_host(card->host);
>>>>>           err = mmc_blk_part_switch(card, md);
>>>>> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>>> *bdev,
>>>>>       cmd_rel_host:
>>>>>           mmc_release_host(card->host);
>>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>>       cmd_done:
>>>>>           mmc_blk_put(md);
>>>>> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>>>>> struct request *req)
>>>>>           struct mmc_host *host = card->host;
>>>>>           unsigned long flags;
>>>>>     -     if (req && !mq->mqrq_prev->req)
>>>>> +       if (req && !mq->mqrq_prev->req) {
>>>>> +               pm_runtime_get_sync(&card->dev);
>>>>>                   /* claim host only for the first request */
>>>>>                   mmc_claim_host(card->host);
>>>>> +       }
>>>>>           ret = mmc_blk_part_switch(card, md);
>>>>>           if (ret) {
>>>>> @@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>>>>> struct request *req)
>>>>>           }
>>>>>       out:
>>>>> -       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
>>>>> +       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
>>>>>                   /* release host only when there are no more requests */
>>>>>                   mmc_release_host(card->host);
>>>>> +               pm_runtime_mark_last_busy(&card->dev);
>>>>> +               pm_runtime_put_autosuspend(&card->dev);
>>>>> +       }
>>>>> +
>>>>>           return ret;
>>>>>     }
>>>>>     @@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct mmc_card
>>>>> *card)
>>>>>                   if (mmc_add_disk(part_md))
>>>>>                           goto out;
>>>>>           }
>>>>> +
>>>>> +       pm_runtime_set_active(&card->dev);
>>>>> +       pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>>>> +       pm_runtime_use_autosuspend(&card->dev);
>>>>> +       pm_runtime_enable(&card->dev);
>>>>> +
>>>>>           return 0;
>>>>>        out:
>>>>> @@ -2344,9 +2363,12 @@ 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);
>>>>> +       pm_runtime_disable(&card->dev);
>>>>> +       pm_runtime_put_noidle(&card->dev);
>>>>>           mmc_blk_remove_req(md);
>>>>>           mmc_set_drvdata(card, NULL);
>>>>>     }
>>>>> @@ -2358,6 +2380,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);
>>>>> @@ -2381,6 +2404,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;
>>>>>     }
>>> Hi Asutosh,
>>>
>>>
>>>> Hi Ulf
>>>> Currently, I am implementing a patch that facilitates each device to
>>>> manage
>>>> is runtime pm on its own.
>>>> I am using the parent-child relationship that is already established in
>>>> the
>>>> mmc stack for this implementation. In this case,
>>>> mmc_card is a child of mmc_host which is in turn is the child of
>>>> platform-device.
>>>> The following contexts exist which would have to invoke get_sync and
>>>> put_sync on the mmc_card device:
>>>> 1. mmcqd
>>>> 2. bkops
>>>> 3. mmc_rescan
>>>>
>>>> The get_sync on card device would resume all the 3 devices starting from
>>>> the
>>>> platform-device and the put-sync would suspend all the 3 devices starting
>>>> from the card-device.
>>>> pm_auto_suspend/pm_schedule_suspend may be used to trigger the suspend
>>>> from
>>>> the above contexts.
>>>> I believe this would simplify the runtime pm functionality.
>>>>
>>>> I can put up the patch for review in a couple of days.
>>>>
>>>> Please let me know your opinion about this approach.
>>> No, I don't think this is way of doing it. I will try to elaborate a
>>> bit with the approach I took in my patchset and why.
>>>
>>> First of all, the host platform device must be kept separate (no
>>> parent/child and not the same bus) from the card device. There is many
>>> reason to why, but within this context (runtime pm point of view), the
>>> host must be able to handle it's runtime pm resources completely
>>> separate from the blk device (card device) runtime resources. More
>>> precisely and from a BKOPS point of view; while doing BKOPS the host
>>> platform device must still be able to enter runtime power save states.
>>>
>>> I realize that in your case, you are doing mmc_suspend|resume_host in
>>> your host drivers runtime callbacks, which is kind of a very special
>>> case, though worth to consider of course. There are two solutions to
>>> enable the option for this functionality. In both cases a host caps
>>> flag will be needed to enable this.
>>>
>>> 1.
>>> In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
>>> device"), and vice verse in the runtime resume callback. This will
>>> prevent the host driver from entering runtime power save sate while
>>> for example doing BKOPS, thus preventing your host driver from doing
>>> mmc_suspend_host while BKOPS is running.
>>>
>>> 2.
>>> Move mmc_suspend|resume_host from your host runtime callbacks, into
>>> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
>>> mmc_suspend_host can be done.
>>>
>>> What are you thoughts around this?
>>>
>>> Kind regards
>>> Ulf Hansson
>>>
>>>
>>>> --
>>>> Thanks
>>>> Asutosh
>>>>
>>>> --
>>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>> Forum.
>>>>
>> Hi Ulf,
>> Typically, when the pltfm device enters power-save during bkops, it can only
>> shut-off the clocks to the card (?), the power would still be on.
>> With clock-gating in place, this would be done anyhow.
> Clock gating is only one of the things you might want to do.
>
> For example;
>
> Your host plf device can reside in a power domain.
> You might want to save power by doing pinctrl.
> Disable/enable irqs.
> Etc.
>
> So, to conclude it's is realy important runtime pm for the block
> device (card device) is kept separate from the host plf device.
> They do handle completly different stuff.
>
>> I wanted to separate the functionality as detailed below:
>>
>> Say we do aggressive power management: (invoke mmc_[suspend/resume]_host in
>> runtime pm as well), idle-timeout of 10 s (configurable though)
>>
>> during suspend of mmc_card device:
>> - do all card related power management, like power-off notifications etc.
>>
>> during suspend of mmc_host device:
>> - do all the host related power management, like power-off host, shut-off
>> clocks etc.
>>
>> during suspend of pltfm device:
>> - do all the pltfm specific power management, like disable irqs, configure
>> wake-ups etc.
>>
>> During system-suspend, it can be checked if the device is runtime-suspended,
>> if yes, return.
>>
>> On resume, the above path would be retraced in the reverse order.
>>
>> I think each bus can be responsible for suspending its devices during system
>> suspend.
> According to my comment above, I don't think this sequence will be possible.
>
>>
>>>> First of all, the host platform device must be kept separate (no
>>>> parent/child and not the same bus) from the card device.
>> Can you please elaborate on this point a bit.
> See above comment for what actions a host plf device can do at runtime
> power management.
>
>> I guess you would be joining the IRC chat tomorrow, if yes, we can discuss
>> there itself.
> It wont be possible for me to joing IRC today, I am at Hongkong with
> Linaro Connect this week. Although it seems like a good discussion on
> IRC, I suppose it would help to clarify on this topic.
>
>>
>> --
>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>>
> Kind regards
> Ulf Hansson
Hi Ulf
Sorry for the delayed response.

The card and host are anyhow coupled except in the idle-time bkops case, 
which is kind of a special case. I think this can be handled in the 
approach I am suggesting by not invoking mmc_suspend_host if bkops is 
running. In this way, the pltfm device can still be put in low-power 
mode (like you suggested), when card is doing bkops.
Moreover, the parent-child relationship is already established (in 
mmc_alloc_host and mmc_alloc_card) and runtime-pm framework only uses it 
if we enable runtime-pm of the particular device.
Even now, we first put the card to sleep and then the host.

What are your thoughts on this ?

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

On 1 April 2013 10:28, Asutosh Das <asutoshd@codeaurora.org> wrote:
> On 3/6/2013 12:27 PM, Ulf Hansson wrote:
>>
>> On 6 March 2013 02:04, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>>
>>> On 3/5/2013 7:09 AM, Ulf Hansson wrote:
>>>>
>>>> On 4 March 2013 21:48, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>>>>
>>>>> On 3/1/2013 6:17 PM, 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.
>>>>>>
>>>>>> 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 what the
>>>>>> host suspend sequence to be unaware of any runtime power save
>>>>>> operations,
>>>>>> so it can just handle the suspend as the device is fully powerered
>>>>>> from
>>>>>> a
>>>>>> runtime perspective.
>>>>>>
>>>>>> This patch is preparing 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>
>>>>>> ---
>>>>>>     drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>>>     1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>>> index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device
>>>>>> *dev,
>>>>>>           md = mmc_blk_get(dev_to_disk(dev));
>>>>>>           card = md->queue.card;
>>>>>>     +     pm_runtime_get_sync(&card->dev);
>>>>>>           mmc_claim_host(card->host);
>>>>>>           ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>>>> EXT_CSD_BOOT_WP,
>>>>>> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device
>>>>>> *dev,
>>>>>>                   card->ext_csd.boot_ro_lock |=
>>>>>> EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>>>>           mmc_release_host(card->host);
>>>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>>>           if (!ret) {
>>>>>>                   pr_info("%s: Locking boot partition ro until next
>>>>>> power
>>>>>> on\n",
>>>>>> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>>>> *bdev,
>>>>>>           mrq.cmd = &cmd;
>>>>>>     +     pm_runtime_get_sync(&card->dev);
>>>>>>           mmc_claim_host(card->host);
>>>>>>           err = mmc_blk_part_switch(card, md);
>>>>>> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>>>> *bdev,
>>>>>>       cmd_rel_host:
>>>>>>           mmc_release_host(card->host);
>>>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>>>       cmd_done:
>>>>>>           mmc_blk_put(md);
>>>>>> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue
>>>>>> *mq,
>>>>>> struct request *req)
>>>>>>           struct mmc_host *host = card->host;
>>>>>>           unsigned long flags;
>>>>>>     -     if (req && !mq->mqrq_prev->req)
>>>>>> +       if (req && !mq->mqrq_prev->req) {
>>>>>> +               pm_runtime_get_sync(&card->dev);
>>>>>>                   /* claim host only for the first request */
>>>>>>                   mmc_claim_host(card->host);
>>>>>> +       }
>>>>>>           ret = mmc_blk_part_switch(card, md);
>>>>>>           if (ret) {
>>>>>> @@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct mmc_queue
>>>>>> *mq,
>>>>>> struct request *req)
>>>>>>           }
>>>>>>       out:
>>>>>> -       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
>>>>>> +       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
>>>>>>                   /* release host only when there are no more requests
>>>>>> */
>>>>>>                   mmc_release_host(card->host);
>>>>>> +               pm_runtime_mark_last_busy(&card->dev);
>>>>>> +               pm_runtime_put_autosuspend(&card->dev);
>>>>>> +       }
>>>>>> +
>>>>>>           return ret;
>>>>>>     }
>>>>>>     @@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct mmc_card
>>>>>> *card)
>>>>>>                   if (mmc_add_disk(part_md))
>>>>>>                           goto out;
>>>>>>           }
>>>>>> +
>>>>>> +       pm_runtime_set_active(&card->dev);
>>>>>> +       pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>>>>> +       pm_runtime_use_autosuspend(&card->dev);
>>>>>> +       pm_runtime_enable(&card->dev);
>>>>>> +
>>>>>>           return 0;
>>>>>>        out:
>>>>>> @@ -2344,9 +2363,12 @@ 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);
>>>>>> +       pm_runtime_disable(&card->dev);
>>>>>> +       pm_runtime_put_noidle(&card->dev);
>>>>>>           mmc_blk_remove_req(md);
>>>>>>           mmc_set_drvdata(card, NULL);
>>>>>>     }
>>>>>> @@ -2358,6 +2380,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);
>>>>>> @@ -2381,6 +2404,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;
>>>>>>     }
>>>>
>>>> Hi Asutosh,
>>>>
>>>>
>>>>> Hi Ulf
>>>>> Currently, I am implementing a patch that facilitates each device to
>>>>> manage
>>>>> is runtime pm on its own.
>>>>> I am using the parent-child relationship that is already established in
>>>>> the
>>>>> mmc stack for this implementation. In this case,
>>>>> mmc_card is a child of mmc_host which is in turn is the child of
>>>>> platform-device.
>>>>> The following contexts exist which would have to invoke get_sync and
>>>>> put_sync on the mmc_card device:
>>>>> 1. mmcqd
>>>>> 2. bkops
>>>>> 3. mmc_rescan
>>>>>
>>>>> The get_sync on card device would resume all the 3 devices starting
>>>>> from
>>>>> the
>>>>> platform-device and the put-sync would suspend all the 3 devices
>>>>> starting
>>>>> from the card-device.
>>>>> pm_auto_suspend/pm_schedule_suspend may be used to trigger the suspend
>>>>> from
>>>>> the above contexts.
>>>>> I believe this would simplify the runtime pm functionality.
>>>>>
>>>>> I can put up the patch for review in a couple of days.
>>>>>
>>>>> Please let me know your opinion about this approach.
>>>>
>>>> No, I don't think this is way of doing it. I will try to elaborate a
>>>> bit with the approach I took in my patchset and why.
>>>>
>>>> First of all, the host platform device must be kept separate (no
>>>> parent/child and not the same bus) from the card device. There is many
>>>> reason to why, but within this context (runtime pm point of view), the
>>>> host must be able to handle it's runtime pm resources completely
>>>> separate from the blk device (card device) runtime resources. More
>>>> precisely and from a BKOPS point of view; while doing BKOPS the host
>>>> platform device must still be able to enter runtime power save states.
>>>>
>>>> I realize that in your case, you are doing mmc_suspend|resume_host in
>>>> your host drivers runtime callbacks, which is kind of a very special
>>>> case, though worth to consider of course. There are two solutions to
>>>> enable the option for this functionality. In both cases a host caps
>>>> flag will be needed to enable this.
>>>>
>>>> 1.
>>>> In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
>>>> device"), and vice verse in the runtime resume callback. This will
>>>> prevent the host driver from entering runtime power save sate while
>>>> for example doing BKOPS, thus preventing your host driver from doing
>>>> mmc_suspend_host while BKOPS is running.
>>>>
>>>> 2.
>>>> Move mmc_suspend|resume_host from your host runtime callbacks, into
>>>> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
>>>> mmc_suspend_host can be done.
>>>>
>>>> What are you thoughts around this?
>>>>
>>>> Kind regards
>>>> Ulf Hansson
>>>>
>>>>
>>>>> --
>>>>> Thanks
>>>>> Asutosh
>>>>>
>>>>> --
>>>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>>> Forum.
>>>>>
>>> Hi Ulf,
>>> Typically, when the pltfm device enters power-save during bkops, it can
>>> only
>>> shut-off the clocks to the card (?), the power would still be on.
>>> With clock-gating in place, this would be done anyhow.
>>
>> Clock gating is only one of the things you might want to do.
>>
>> For example;
>>
>> Your host plf device can reside in a power domain.
>> You might want to save power by doing pinctrl.
>> Disable/enable irqs.
>> Etc.
>>
>> So, to conclude it's is realy important runtime pm for the block
>> device (card device) is kept separate from the host plf device.
>> They do handle completly different stuff.
>>
>>> I wanted to separate the functionality as detailed below:
>>>
>>> Say we do aggressive power management: (invoke mmc_[suspend/resume]_host
>>> in
>>> runtime pm as well), idle-timeout of 10 s (configurable though)
>>>
>>> during suspend of mmc_card device:
>>> - do all card related power management, like power-off notifications etc.
>>>
>>> during suspend of mmc_host device:
>>> - do all the host related power management, like power-off host, shut-off
>>> clocks etc.
>>>
>>> during suspend of pltfm device:
>>> - do all the pltfm specific power management, like disable irqs,
>>> configure
>>> wake-ups etc.
>>>
>>> During system-suspend, it can be checked if the device is
>>> runtime-suspended,
>>> if yes, return.
>>>
>>> On resume, the above path would be retraced in the reverse order.
>>>
>>> I think each bus can be responsible for suspending its devices during
>>> system
>>> suspend.
>>
>> According to my comment above, I don't think this sequence will be
>> possible.
>>
>>>
>>>>> First of all, the host platform device must be kept separate (no
>>>>> parent/child and not the same bus) from the card device.
>>>
>>> Can you please elaborate on this point a bit.
>>
>> See above comment for what actions a host plf device can do at runtime
>> power management.
>>
>>> I guess you would be joining the IRC chat tomorrow, if yes, we can
>>> discuss
>>> there itself.
>>
>> It wont be possible for me to joing IRC today, I am at Hongkong with
>> Linaro Connect this week. Although it seems like a good discussion on
>> IRC, I suppose it would help to clarify on this topic.
>>
>>>
>>> --
>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum.
>>>
>> Kind regards
>> Ulf Hansson
>
> Hi Ulf
> Sorry for the delayed response.
>
> The card and host are anyhow coupled except in the idle-time bkops case,
> which is kind of a special case. I think this can be handled in the approach
> I am suggesting by not invoking mmc_suspend_host if bkops is running.In
> this way, the pltfm device can still be put in low-power mode (like you
> suggested), when card is doing bkops.

A running BKOPS must not prevent the system from suspend (host driver
calls mmc_suspend_host). Thus if we are running a BKOPS when system
suspend occurs, we need to interrupt the BKOPS (send HPI). So I think
this will not work properly, unless I missed something here.

Moreover as I also stated earlier, the pm_runtime_get_sync of the card
device when the card device enters suspend state, will accomplish just
that if BKOPS is adapted properly.

> Moreover, the parent-child relationship is already established (in
> mmc_alloc_host and mmc_alloc_card) and runtime-pm framework only uses it if
> we enable runtime-pm of the particular device.
> Even now, we first put the card to sleep and then the host.

I am not sure I understand what you are proposing. In what way should
should I change my patch?

>
> What are your thoughts on this ?
>
>
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>

Some final thoughts, this patchset has been around for some time now.
We have mainly been discussing hypothetical issues which relates to
"aggresive card suspend", for which code right now not even exists in
the mainline kernel. Nevertheless very good discussions, but I think
we shoud be able to handle all these additional features as new
patches build on top of this patchset, don't you think?

It is always easier to disuss around real patches, but discussing
hypothetical issues, if you see what mean.

Kind regards
Ulf Hansson

Re: [PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[Subhash Jadavani]

On 4/2/2013 4:08 PM, Ulf Hansson wrote:
> On 1 April 2013 10:28, Asutosh Das <asutoshd@codeaurora.org> wrote:
>> On 3/6/2013 12:27 PM, Ulf Hansson wrote:
>>> On 6 March 2013 02:04, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>>> On 3/5/2013 7:09 AM, Ulf Hansson wrote:
>>>>> On 4 March 2013 21:48, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>>>>> On 3/1/2013 6:17 PM, 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.
>>>>>>>
>>>>>>> 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 what the
>>>>>>> host suspend sequence to be unaware of any runtime power save
>>>>>>> operations,
>>>>>>> so it can just handle the suspend as the device is fully powerered
>>>>>>> from
>>>>>>> a
>>>>>>> runtime perspective.
>>>>>>>
>>>>>>> This patch is preparing 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>
>>>>>>> ---
>>>>>>>      drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>>>>      1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>>>> index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device
>>>>>>> *dev,
>>>>>>>            md = mmc_blk_get(dev_to_disk(dev));
>>>>>>>            card = md->queue.card;
>>>>>>>      +     pm_runtime_get_sync(&card->dev);
>>>>>>>            mmc_claim_host(card->host);
>>>>>>>            ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>>>>> EXT_CSD_BOOT_WP,
>>>>>>> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device
>>>>>>> *dev,
>>>>>>>                    card->ext_csd.boot_ro_lock |=
>>>>>>> EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>>>>>            mmc_release_host(card->host);
>>>>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>>>>            if (!ret) {
>>>>>>>                    pr_info("%s: Locking boot partition ro until next
>>>>>>> power
>>>>>>> on\n",
>>>>>>> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>>>>> *bdev,
>>>>>>>            mrq.cmd = &cmd;
>>>>>>>      +     pm_runtime_get_sync(&card->dev);
>>>>>>>            mmc_claim_host(card->host);
>>>>>>>            err = mmc_blk_part_switch(card, md);
>>>>>>> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device
>>>>>>> *bdev,
>>>>>>>        cmd_rel_host:
>>>>>>>            mmc_release_host(card->host);
>>>>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>>>>        cmd_done:
>>>>>>>            mmc_blk_put(md);
>>>>>>> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue
>>>>>>> *mq,
>>>>>>> struct request *req)
>>>>>>>            struct mmc_host *host = card->host;
>>>>>>>            unsigned long flags;
>>>>>>>      -     if (req && !mq->mqrq_prev->req)
>>>>>>> +       if (req && !mq->mqrq_prev->req) {
>>>>>>> +               pm_runtime_get_sync(&card->dev);
>>>>>>>                    /* claim host only for the first request */
>>>>>>>                    mmc_claim_host(card->host);
>>>>>>> +       }
>>>>>>>            ret = mmc_blk_part_switch(card, md);
>>>>>>>            if (ret) {
>>>>>>> @@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct mmc_queue
>>>>>>> *mq,
>>>>>>> struct request *req)
>>>>>>>            }
>>>>>>>        out:
>>>>>>> -       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
>>>>>>> +       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
>>>>>>>                    /* release host only when there are no more requests
>>>>>>> */
>>>>>>>                    mmc_release_host(card->host);
>>>>>>> +               pm_runtime_mark_last_busy(&card->dev);
>>>>>>> +               pm_runtime_put_autosuspend(&card->dev);
>>>>>>> +       }
>>>>>>> +
>>>>>>>            return ret;
>>>>>>>      }
>>>>>>>      @@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct mmc_card
>>>>>>> *card)
>>>>>>>                    if (mmc_add_disk(part_md))
>>>>>>>                            goto out;
>>>>>>>            }
>>>>>>> +
>>>>>>> +       pm_runtime_set_active(&card->dev);
>>>>>>> +       pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>>>>>> +       pm_runtime_use_autosuspend(&card->dev);
>>>>>>> +       pm_runtime_enable(&card->dev);
>>>>>>> +
>>>>>>>            return 0;
>>>>>>>         out:
>>>>>>> @@ -2344,9 +2363,12 @@ 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);
>>>>>>> +       pm_runtime_disable(&card->dev);
>>>>>>> +       pm_runtime_put_noidle(&card->dev);
>>>>>>>            mmc_blk_remove_req(md);
>>>>>>>            mmc_set_drvdata(card, NULL);
>>>>>>>      }
>>>>>>> @@ -2358,6 +2380,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);
>>>>>>> @@ -2381,6 +2404,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;
>>>>>>>      }
>>>>> Hi Asutosh,
>>>>>
>>>>>
>>>>>> Hi Ulf
>>>>>> Currently, I am implementing a patch that facilitates each device to
>>>>>> manage
>>>>>> is runtime pm on its own.
>>>>>> I am using the parent-child relationship that is already established in
>>>>>> the
>>>>>> mmc stack for this implementation. In this case,
>>>>>> mmc_card is a child of mmc_host which is in turn is the child of
>>>>>> platform-device.
>>>>>> The following contexts exist which would have to invoke get_sync and
>>>>>> put_sync on the mmc_card device:
>>>>>> 1. mmcqd
>>>>>> 2. bkops
>>>>>> 3. mmc_rescan
>>>>>>
>>>>>> The get_sync on card device would resume all the 3 devices starting
>>>>>> from
>>>>>> the
>>>>>> platform-device and the put-sync would suspend all the 3 devices
>>>>>> starting
>>>>>> from the card-device.
>>>>>> pm_auto_suspend/pm_schedule_suspend may be used to trigger the suspend
>>>>>> from
>>>>>> the above contexts.
>>>>>> I believe this would simplify the runtime pm functionality.
>>>>>>
>>>>>> I can put up the patch for review in a couple of days.
>>>>>>
>>>>>> Please let me know your opinion about this approach.
>>>>> No, I don't think this is way of doing it. I will try to elaborate a
>>>>> bit with the approach I took in my patchset and why.
>>>>>
>>>>> First of all, the host platform device must be kept separate (no
>>>>> parent/child and not the same bus) from the card device. There is many
>>>>> reason to why, but within this context (runtime pm point of view), the
>>>>> host must be able to handle it's runtime pm resources completely
>>>>> separate from the blk device (card device) runtime resources. More
>>>>> precisely and from a BKOPS point of view; while doing BKOPS the host
>>>>> platform device must still be able to enter runtime power save states.
>>>>>
>>>>> I realize that in your case, you are doing mmc_suspend|resume_host in
>>>>> your host drivers runtime callbacks, which is kind of a very special
>>>>> case, though worth to consider of course. There are two solutions to
>>>>> enable the option for this functionality. In both cases a host caps
>>>>> flag will be needed to enable this.
>>>>>
>>>>> 1.
>>>>> In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
>>>>> device"), and vice verse in the runtime resume callback. This will
>>>>> prevent the host driver from entering runtime power save sate while
>>>>> for example doing BKOPS, thus preventing your host driver from doing
>>>>> mmc_suspend_host while BKOPS is running.
>>>>>
>>>>> 2.
>>>>> Move mmc_suspend|resume_host from your host runtime callbacks, into
>>>>> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
>>>>> mmc_suspend_host can be done.
>>>>>
>>>>> What are you thoughts around this?
>>>>>
>>>>> Kind regards
>>>>> Ulf Hansson
>>>>>
>>>>>
>>>>>> --
>>>>>> Thanks
>>>>>> Asutosh
>>>>>>
>>>>>> --
>>>>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>>>> Forum.
>>>>>>
>>>> Hi Ulf,
>>>> Typically, when the pltfm device enters power-save during bkops, it can
>>>> only
>>>> shut-off the clocks to the card (?), the power would still be on.
>>>> With clock-gating in place, this would be done anyhow.
>>> Clock gating is only one of the things you might want to do.
>>>
>>> For example;
>>>
>>> Your host plf device can reside in a power domain.
>>> You might want to save power by doing pinctrl.
>>> Disable/enable irqs.
>>> Etc.
>>>
>>> So, to conclude it's is realy important runtime pm for the block
>>> device (card device) is kept separate from the host plf device.
>>> They do handle completly different stuff.
>>>
>>>> I wanted to separate the functionality as detailed below:
>>>>
>>>> Say we do aggressive power management: (invoke mmc_[suspend/resume]_host
>>>> in
>>>> runtime pm as well), idle-timeout of 10 s (configurable though)
>>>>
>>>> during suspend of mmc_card device:
>>>> - do all card related power management, like power-off notifications etc.
>>>>
>>>> during suspend of mmc_host device:
>>>> - do all the host related power management, like power-off host, shut-off
>>>> clocks etc.
>>>>
>>>> during suspend of pltfm device:
>>>> - do all the pltfm specific power management, like disable irqs,
>>>> configure
>>>> wake-ups etc.
>>>>
>>>> During system-suspend, it can be checked if the device is
>>>> runtime-suspended,
>>>> if yes, return.
>>>>
>>>> On resume, the above path would be retraced in the reverse order.
>>>>
>>>> I think each bus can be responsible for suspending its devices during
>>>> system
>>>> suspend.
>>> According to my comment above, I don't think this sequence will be
>>> possible.
>>>
>>>>>> First of all, the host platform device must be kept separate (no
>>>>>> parent/child and not the same bus) from the card device.
>>>> Can you please elaborate on this point a bit.
>>> See above comment for what actions a host plf device can do at runtime
>>> power management.
>>>
>>>> I guess you would be joining the IRC chat tomorrow, if yes, we can
>>>> discuss
>>>> there itself.
>>> It wont be possible for me to joing IRC today, I am at Hongkong with
>>> Linaro Connect this week. Although it seems like a good discussion on
>>> IRC, I suppose it would help to clarify on this topic.
>>>
>>>> --
>>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>> Forum.
>>>>
>>> Kind regards
>>> Ulf Hansson
>> Hi Ulf
>> Sorry for the delayed response.
>>
>> The card and host are anyhow coupled except in the idle-time bkops case,
>> which is kind of a special case. I think this can be handled in the approach
>> I am suggesting by not invoking mmc_suspend_host if bkops is running.In
>> this way, the pltfm device can still be put in low-power mode (like you
>> suggested), when card is doing bkops.
> A running BKOPS must not prevent the system from suspend (host driver
> calls mmc_suspend_host). Thus if we are running a BKOPS when system
> suspend occurs, we need to interrupt the BKOPS (send HPI). So I think
> this will not work properly, unless I missed something here.
>
> Moreover as I also stated earlier, the pm_runtime_get_sync of the card
> device when the card device enters suspend state, will accomplish just
> that if BKOPS is adapted properly.
>
>> Moreover, the parent-child relationship is already established (in
>> mmc_alloc_host and mmc_alloc_card) and runtime-pm framework only uses it if
>> we enable runtime-pm of the particular device.
>> Even now, we first put the card to sleep and then the host.
> I am not sure I understand what you are proposing. In what way should
> should I change my patch?
>
>> What are your thoughts on this ?
>>
>>
>> --
>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>>
> Some final thoughts, this patchset has been around for some time now.
> We have mainly been discussing hypothetical issues which relates to
> "aggresive card suspend", for which code right now not even exists in
> the mainline kernel. Nevertheless very good discussions, but I think
> we shoud be able to handle all these additional features as new
> patches build on top of this patchset, don't you think?
Thanks Ulf, we got your point.

I guess you may agree that even if none of the in-tree drivers 
implementaing "agreesive card suspend" during runtime suspend, its worth 
considering it and lets atleast give an option (defining new cap for 
agressive RPM) to choose to different flavours of host controller 
drivers. I guess other host controllers will also benefit from this. If 
you are interested to look at the power savings achieved with this 
agreesive RPM, we may post the same.

But main point is to keep this aggressive power management requirement 
in mind, when reviewing this patch or any additional enhacements coming 
in on top of this patch.

Regards,
Subhash

>
> It is always easier to disuss around real patches, but discussing
> hypothetical issues, if you see what mean.
>
> Kind regards
> Ulf Hansson
> --
> 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

Re: [PATCH 3/3] mmc: block: Enable runtime pm for mmc blkdevice

[merez]

Acked-by: Maya Erez <merez@codeaurora.org>

> On 4/2/2013 4:08 PM, Ulf Hansson wrote:
>> On 1 April 2013 10:28, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>> On 3/6/2013 12:27 PM, Ulf Hansson wrote:
>>>> On 6 March 2013 02:04, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>>>> On 3/5/2013 7:09 AM, Ulf Hansson wrote:
>>>>>> On 4 March 2013 21:48, Asutosh Das <asutoshd@codeaurora.org> wrote:
>>>>>>> On 3/1/2013 6:17 PM, 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.
>>>>>>>>
>>>>>>>> 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 what
>>>>>>>> the
>>>>>>>> host suspend sequence to be unaware of any runtime power save
>>>>>>>> operations,
>>>>>>>> so it can just handle the suspend as the device is fully powerered
>>>>>>>> from
>>>>>>>> a
>>>>>>>> runtime perspective.
>>>>>>>>
>>>>>>>> This patch is preparing 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>
>>>>>>>> ---
>>>>>>>>      drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>>>>>      1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>>>>> index 5bab73b..89d1c39 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,6 +223,7 @@ static ssize_t power_ro_lock_store(struct
>>>>>>>> device
>>>>>>>> *dev,
>>>>>>>>            md = mmc_blk_get(dev_to_disk(dev));
>>>>>>>>            card = md->queue.card;
>>>>>>>>      +     pm_runtime_get_sync(&card->dev);
>>>>>>>>            mmc_claim_host(card->host);
>>>>>>>>            ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>>>>>> EXT_CSD_BOOT_WP,
>>>>>>>> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct
>>>>>>>> device
>>>>>>>> *dev,
>>>>>>>>                    card->ext_csd.boot_ro_lock |=
>>>>>>>> EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>>>>>>>>            mmc_release_host(card->host);
>>>>>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>>>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>>>>>            if (!ret) {
>>>>>>>>                    pr_info("%s: Locking boot partition ro until
>>>>>>>> next
>>>>>>>> power
>>>>>>>> on\n",
>>>>>>>> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct
>>>>>>>> block_device
>>>>>>>> *bdev,
>>>>>>>>            mrq.cmd = &cmd;
>>>>>>>>      +     pm_runtime_get_sync(&card->dev);
>>>>>>>>            mmc_claim_host(card->host);
>>>>>>>>            err = mmc_blk_part_switch(card, md);
>>>>>>>> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct
>>>>>>>> block_device
>>>>>>>> *bdev,
>>>>>>>>        cmd_rel_host:
>>>>>>>>            mmc_release_host(card->host);
>>>>>>>> +       pm_runtime_mark_last_busy(&card->dev);
>>>>>>>> +       pm_runtime_put_autosuspend(&card->dev);
>>>>>>>>        cmd_done:
>>>>>>>>            mmc_blk_put(md);
>>>>>>>> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct
>>>>>>>> mmc_queue
>>>>>>>> *mq,
>>>>>>>> struct request *req)
>>>>>>>>            struct mmc_host *host = card->host;
>>>>>>>>            unsigned long flags;
>>>>>>>>      -     if (req && !mq->mqrq_prev->req)
>>>>>>>> +       if (req && !mq->mqrq_prev->req) {
>>>>>>>> +               pm_runtime_get_sync(&card->dev);
>>>>>>>>                    /* claim host only for the first request */
>>>>>>>>                    mmc_claim_host(card->host);
>>>>>>>> +       }
>>>>>>>>            ret = mmc_blk_part_switch(card, md);
>>>>>>>>            if (ret) {
>>>>>>>> @@ -1932,9 +1941,13 @@ static int mmc_blk_issue_rq(struct
>>>>>>>> mmc_queue
>>>>>>>> *mq,
>>>>>>>> struct request *req)
>>>>>>>>            }
>>>>>>>>        out:
>>>>>>>> -       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST))
>>>>>>>> +       if (!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) {
>>>>>>>>                    /* release host only when there are no more
>>>>>>>> requests
>>>>>>>> */
>>>>>>>>                    mmc_release_host(card->host);
>>>>>>>> +               pm_runtime_mark_last_busy(&card->dev);
>>>>>>>> +               pm_runtime_put_autosuspend(&card->dev);
>>>>>>>> +       }
>>>>>>>> +
>>>>>>>>            return ret;
>>>>>>>>      }
>>>>>>>>      @@ -2331,6 +2344,12 @@ static int mmc_blk_probe(struct
>>>>>>>> mmc_card
>>>>>>>> *card)
>>>>>>>>                    if (mmc_add_disk(part_md))
>>>>>>>>                            goto out;
>>>>>>>>            }
>>>>>>>> +
>>>>>>>> +       pm_runtime_set_active(&card->dev);
>>>>>>>> +       pm_runtime_set_autosuspend_delay(&card->dev, 3000);
>>>>>>>> +       pm_runtime_use_autosuspend(&card->dev);
>>>>>>>> +       pm_runtime_enable(&card->dev);
>>>>>>>> +
>>>>>>>>            return 0;
>>>>>>>>         out:
>>>>>>>> @@ -2344,9 +2363,12 @@ 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);
>>>>>>>> +       pm_runtime_disable(&card->dev);
>>>>>>>> +       pm_runtime_put_noidle(&card->dev);
>>>>>>>>            mmc_blk_remove_req(md);
>>>>>>>>            mmc_set_drvdata(card, NULL);
>>>>>>>>      }
>>>>>>>> @@ -2358,6 +2380,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);
>>>>>>>> @@ -2381,6 +2404,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;
>>>>>>>>      }
>>>>>> Hi Asutosh,
>>>>>>
>>>>>>
>>>>>>> Hi Ulf
>>>>>>> Currently, I am implementing a patch that facilitates each device
>>>>>>> to
>>>>>>> manage
>>>>>>> is runtime pm on its own.
>>>>>>> I am using the parent-child relationship that is already
>>>>>>> established in
>>>>>>> the
>>>>>>> mmc stack for this implementation. In this case,
>>>>>>> mmc_card is a child of mmc_host which is in turn is the child of
>>>>>>> platform-device.
>>>>>>> The following contexts exist which would have to invoke get_sync
>>>>>>> and
>>>>>>> put_sync on the mmc_card device:
>>>>>>> 1. mmcqd
>>>>>>> 2. bkops
>>>>>>> 3. mmc_rescan
>>>>>>>
>>>>>>> The get_sync on card device would resume all the 3 devices starting
>>>>>>> from
>>>>>>> the
>>>>>>> platform-device and the put-sync would suspend all the 3 devices
>>>>>>> starting
>>>>>>> from the card-device.
>>>>>>> pm_auto_suspend/pm_schedule_suspend may be used to trigger the
>>>>>>> suspend
>>>>>>> from
>>>>>>> the above contexts.
>>>>>>> I believe this would simplify the runtime pm functionality.
>>>>>>>
>>>>>>> I can put up the patch for review in a couple of days.
>>>>>>>
>>>>>>> Please let me know your opinion about this approach.
>>>>>> No, I don't think this is way of doing it. I will try to elaborate a
>>>>>> bit with the approach I took in my patchset and why.
>>>>>>
>>>>>> First of all, the host platform device must be kept separate (no
>>>>>> parent/child and not the same bus) from the card device. There is
>>>>>> many
>>>>>> reason to why, but within this context (runtime pm point of view),
>>>>>> the
>>>>>> host must be able to handle it's runtime pm resources completely
>>>>>> separate from the blk device (card device) runtime resources. More
>>>>>> precisely and from a BKOPS point of view; while doing BKOPS the host
>>>>>> platform device must still be able to enter runtime power save
>>>>>> states.
>>>>>>
>>>>>> I realize that in your case, you are doing mmc_suspend|resume_host
>>>>>> in
>>>>>> your host drivers runtime callbacks, which is kind of a very special
>>>>>> case, though worth to consider of course. There are two solutions to
>>>>>> enable the option for this functionality. In both cases a host caps
>>>>>> flag will be needed to enable this.
>>>>>>
>>>>>> 1.
>>>>>> In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
>>>>>> device"), and vice verse in the runtime resume callback. This will
>>>>>> prevent the host driver from entering runtime power save sate while
>>>>>> for example doing BKOPS, thus preventing your host driver from doing
>>>>>> mmc_suspend_host while BKOPS is running.
>>>>>>
>>>>>> 2.
>>>>>> Move mmc_suspend|resume_host from your host runtime callbacks, into
>>>>>> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
>>>>>> mmc_suspend_host can be done.
>>>>>>
>>>>>> What are you thoughts around this?
>>>>>>
>>>>>> Kind regards
>>>>>> Ulf Hansson
>>>>>>
>>>>>>
>>>>>>> --
>>>>>>> Thanks
>>>>>>> Asutosh
>>>>>>>
>>>>>>> --
>>>>>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>>>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>>>>> Forum.
>>>>>>>
>>>>> Hi Ulf,
>>>>> Typically, when the pltfm device enters power-save during bkops, it
>>>>> can
>>>>> only
>>>>> shut-off the clocks to the card (?), the power would still be on.
>>>>> With clock-gating in place, this would be done anyhow.
>>>> Clock gating is only one of the things you might want to do.
>>>>
>>>> For example;
>>>>
>>>> Your host plf device can reside in a power domain.
>>>> You might want to save power by doing pinctrl.
>>>> Disable/enable irqs.
>>>> Etc.
>>>>
>>>> So, to conclude it's is realy important runtime pm for the block
>>>> device (card device) is kept separate from the host plf device.
>>>> They do handle completly different stuff.
>>>>
>>>>> I wanted to separate the functionality as detailed below:
>>>>>
>>>>> Say we do aggressive power management: (invoke
>>>>> mmc_[suspend/resume]_host
>>>>> in
>>>>> runtime pm as well), idle-timeout of 10 s (configurable though)
>>>>>
>>>>> during suspend of mmc_card device:
>>>>> - do all card related power management, like power-off notifications
>>>>> etc.
>>>>>
>>>>> during suspend of mmc_host device:
>>>>> - do all the host related power management, like power-off host,
>>>>> shut-off
>>>>> clocks etc.
>>>>>
>>>>> during suspend of pltfm device:
>>>>> - do all the pltfm specific power management, like disable irqs,
>>>>> configure
>>>>> wake-ups etc.
>>>>>
>>>>> During system-suspend, it can be checked if the device is
>>>>> runtime-suspended,
>>>>> if yes, return.
>>>>>
>>>>> On resume, the above path would be retraced in the reverse order.
>>>>>
>>>>> I think each bus can be responsible for suspending its devices during
>>>>> system
>>>>> suspend.
>>>> According to my comment above, I don't think this sequence will be
>>>> possible.
>>>>
>>>>>>> First of all, the host platform device must be kept separate (no
>>>>>>> parent/child and not the same bus) from the card device.
>>>>> Can you please elaborate on this point a bit.
>>>> See above comment for what actions a host plf device can do at runtime
>>>> power management.
>>>>
>>>>> I guess you would be joining the IRC chat tomorrow, if yes, we can
>>>>> discuss
>>>>> there itself.
>>>> It wont be possible for me to joing IRC today, I am at Hongkong with
>>>> Linaro Connect this week. Although it seems like a good discussion on
>>>> IRC, I suppose it would help to clarify on this topic.
>>>>
>>>>> --
>>>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>>> Forum.
>>>>>
>>>> Kind regards
>>>> Ulf Hansson
>>> Hi Ulf
>>> Sorry for the delayed response.
>>>
>>> The card and host are anyhow coupled except in the idle-time bkops
>>> case,
>>> which is kind of a special case. I think this can be handled in the
>>> approach
>>> I am suggesting by not invoking mmc_suspend_host if bkops is running.In
>>> this way, the pltfm device can still be put in low-power mode (like you
>>> suggested), when card is doing bkops.
>> A running BKOPS must not prevent the system from suspend (host driver
>> calls mmc_suspend_host). Thus if we are running a BKOPS when system
>> suspend occurs, we need to interrupt the BKOPS (send HPI). So I think
>> this will not work properly, unless I missed something here.
>>
>> Moreover as I also stated earlier, the pm_runtime_get_sync of the card
>> device when the card device enters suspend state, will accomplish just
>> that if BKOPS is adapted properly.
>>
>>> Moreover, the parent-child relationship is already established (in
>>> mmc_alloc_host and mmc_alloc_card) and runtime-pm framework only uses
>>> it if
>>> we enable runtime-pm of the particular device.
>>> Even now, we first put the card to sleep and then the host.
>> I am not sure I understand what you are proposing. In what way should
>> should I change my patch?
>>
>>> What are your thoughts on this ?
>>>
>>>
>>> --
>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum.
>>>
>> Some final thoughts, this patchset has been around for some time now.
>> We have mainly been discussing hypothetical issues which relates to
>> "aggresive card suspend", for which code right now not even exists in
>> the mainline kernel. Nevertheless very good discussions, but I think
>> we shoud be able to handle all these additional features as new
>> patches build on top of this patchset, don't you think?
> Thanks Ulf, we got your point.
>
> I guess you may agree that even if none of the in-tree drivers
> implementaing "agreesive card suspend" during runtime suspend, its worth
> considering it and lets atleast give an option (defining new cap for
> agressive RPM) to choose to different flavours of host controller
> drivers. I guess other host controllers will also benefit from this. If
> you are interested to look at the power savings achieved with this
> agreesive RPM, we may post the same.
>
> But main point is to keep this aggressive power management requirement
> in mind, when reviewing this patch or any additional enhacements coming
> in on top of this patch.
>
> Regards,
> Subhash
>
>>
>> It is always easier to disuss around real patches, but discussing
>> hypothetical issues, if you see what mean.
>>
>> Kind regards
>> Ulf Hansson
>> --
>> 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
>
> --
> 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
>


-- 
Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Maya Erez]

Thanks Ulf.
I will go over the new patch and will develop the periodic BKOPS on top of
it.

Thanks,
Maya

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org
[mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Ulf Hansson
Sent: Friday, March 01, 2013 2:47 PM
To: linux-mmc@vger.kernel.org; Chris Ball
Cc: Johan Rudholm; Ulf Hansson
Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice

From: Ulf Hansson <ulf.hansson@linaro.org>

SDIO has been using runtime pm for a while to handle runtime power save
operations. This patchset is enabling the option to make the sd/mmc
blockdevices to use runtime pm as well.

The runtime pm implementation for the block device will make use of
autosuspend to defer power save operation to after request inactivty for a
certain time.

To actually perform some power save operations the corresponding bus ops for
mmc and sd shall be implemented. Typically it could make sense to do BKOPS
for eMMC in here.

Ulf Hansson (3):
  mmc: core: Remove power_restore bus_ops for mmc and sd
  mmc: core: Add bus_ops for runtime pm callbacks
  mmc: block: Enable runtime pm for mmc blkdevice

 drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
 drivers/mmc/core/bus.c   |   14 ++++++++++++--
 drivers/mmc/core/core.h  |    2 ++
 drivers/mmc/core/mmc.c   |   14 --------------
 drivers/mmc/core/sd.c    |   14 --------------
 drivers/mmc/core/sdio.c  |   20 ++++++++++++++++++++
 6 files changed, 60 insertions(+), 32 deletions(-)

--
1.7.10

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

Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Chris Ball]

Hi Maya, Kevin, Seungwon, Jaehoon,

On Fri, Mar 01 2013, Ulf Hansson wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> SDIO has been using runtime pm for a while to handle runtime power save
> operations. This patchset is enabling the option to make the sd/mmc
> blockdevices to use runtime pm as well.
>
> The runtime pm implementation for the block device will make use of
> autosuspend to defer power save operation to after request inactivty for
> a certain time.
>
> To actually perform some power save operations the corresponding bus ops
> for mmc and sd shall be implemented. Typically it could make sense to do
> BKOPS for eMMC in here.
>
> Ulf Hansson (3):
>   mmc: core: Remove power_restore bus_ops for mmc and sd
>   mmc: core: Add bus_ops for runtime pm callbacks
>   mmc: block: Enable runtime pm for mmc blkdevice
>
>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>  drivers/mmc/core/bus.c   |   14 ++++++++++++--
>  drivers/mmc/core/core.h  |    2 ++
>  drivers/mmc/core/mmc.c   |   14 --------------
>  drivers/mmc/core/sd.c    |   14 --------------
>  drivers/mmc/core/sdio.c  |   20 ++++++++++++++++++++
>  6 files changed, 60 insertions(+), 32 deletions(-)

This looks good to me, but I'd like to make sure the design works for
you before we decide to merge it.  Any concerns?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Arnd Bergmann]

On Friday 01 March 2013, Ulf Hansson wrote:
> 
> From: Ulf Hansson <ulf.hansson@linaro.org>
> 
> SDIO has been using runtime pm for a while to handle runtime power save
> operations. This patchset is enabling the option to make the sd/mmc
> blockdevices to use runtime pm as well.
> 
> The runtime pm implementation for the block device will make use of
> autosuspend to defer power save operation to after request inactivty for
> a certain time.
> 
> To actually perform some power save operations the corresponding bus ops
> for mmc and sd shall be implemented. Typically it could make sense to do
> BKOPS for eMMC in here.

As per IRC discussion:

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: FW: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Kevin Liu]

> From: Ulf Hansson <ulf.hansson@stericsson.com<mailto:ulf.hansson@stericsson.com>>
> Date: Fri, Mar 1, 2013 at 8:47 PM
> Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice
> To: linux-mmc@vger.kernel.org<mailto:linux-mmc@vger.kernel.org>, Chris Ball <cjb@laptop.org<mailto:cjb@laptop.org>>
> Cc: Johan Rudholm <johan.rudholm@stericsson.com<mailto:johan.rudholm@stericsson.com>>, Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>
>
> From: Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>
> SDIO has been using runtime pm for a while to handle runtime power save
> operations. This patchset is enabling the option to make the sd/mmc
> blockdevices to use runtime pm as well.
>
> The runtime pm implementation for the block device will make use of
> autosuspend to defer power save operation to after request inactivty for
> a certain time.
>
> To actually perform some power save operations the corresponding bus ops
> for mmc and sd shall be implemented. Typically it could make sense to do
> BKOPS for eMMC in here.
>
> Ulf Hansson (3):
>   mmc: core: Remove power_restore bus_ops for mmc and sd
>   mmc: core: Add bus_ops for runtime pm callbacks
>   mmc: block: Enable runtime pm for mmc blkdevice
>
Ulf,

sdhci.c has added pm_runtime which also protect between request and
task finish. And some sdhci.c based host drivers has provided
pm_runtime_suspend/resume functions like sdhci-pxav3.c. From the
powersave viewpoint, I think adding pm_runtime in driver level is
better than doing that on bus level since the control granularity is
even smaller. And adding pm_runtime in both block.c and sdhci.c will
call pm_runtime twice. How do you think?

Thanks
Kevin

Re: FW: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Ulf Hansson]

On 7 March 2013 01:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>> From: Ulf Hansson <ulf.hansson@stericsson.com<mailto:ulf.hansson@stericsson.com>>
>> Date: Fri, Mar 1, 2013 at 8:47 PM
>> Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>> To: linux-mmc@vger.kernel.org<mailto:linux-mmc@vger.kernel.org>, Chris Ball <cjb@laptop.org<mailto:cjb@laptop.org>>
>> Cc: Johan Rudholm <johan.rudholm@stericsson.com<mailto:johan.rudholm@stericsson.com>>, Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>
>>
>> From: Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>
>> SDIO has been using runtime pm for a while to handle runtime power save
>> operations. This patchset is enabling the option to make the sd/mmc
>> blockdevices to use runtime pm as well.
>>
>> The runtime pm implementation for the block device will make use of
>> autosuspend to defer power save operation to after request inactivty for
>> a certain time.
>>
>> To actually perform some power save operations the corresponding bus ops
>> for mmc and sd shall be implemented. Typically it could make sense to do
>> BKOPS for eMMC in here.
>>
>> Ulf Hansson (3):
>>   mmc: core: Remove power_restore bus_ops for mmc and sd
>>   mmc: core: Add bus_ops for runtime pm callbacks
>>   mmc: block: Enable runtime pm for mmc blkdevice
>>
> Ulf,
>
> sdhci.c has added pm_runtime which also protect between request and
> task finish. And some sdhci.c based host drivers has provided
> pm_runtime_suspend/resume functions like sdhci-pxav3.c. From the
> powersave viewpoint, I think adding pm_runtime in driver level is
> better than doing that on bus level since the control granularity is
> even smaller. And adding pm_runtime in both block.c and sdhci.c will
> call pm_runtime twice. How do you think?
>
> Thanks
> Kevin

Hi Kevin,

Thanks for your response!

It seems like we need some more clarification around this area.
Runtime pm for a host device driver shall ultimately be responsible
for taking care of runtime power management of the host device - only.
It should not handle runtime power management of a block device, which
in principle means BKOPS shall be handled in the blkdevice. At least
this is my view.

So, why is this? I will try to elaborate on the runtime pm support in
host drivers here.
The host device driver controls a MMC/SD/SDIO IP. This IP could very
well reside (for some SoC) in what you call a power domain. In
principle, once the IP needs to be used, a host driver has done a
pm_runtime_get of it's device. This will mean a reference to the power
domain has been fetched. Once the IP is not needed any more,
pm_runtime_put is done and the reference to the power domain is
released. Once no reference to the power domain exist the power domain
can enter lower sleep states, which is preferred to happen as soon as
possible and as long as possible - of course.

Hope this gives a better understanding. :-)

Kind regards
Ulf Hansson

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Kevin Liu]

2013/3/7 Ulf Hansson <ulf.hansson@linaro.org>:
> On 7 March 2013 01:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>> From: Ulf Hansson <ulf.hansson@stericsson.com<mailto:ulf.hansson@stericsson.com>>
>>> Date: Fri, Mar 1, 2013 at 8:47 PM
>>> Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>> To: linux-mmc@vger.kernel.org<mailto:linux-mmc@vger.kernel.org>, Chris Ball <cjb@laptop.org<mailto:cjb@laptop.org>>
>>> Cc: Johan Rudholm <johan.rudholm@stericsson.com<mailto:johan.rudholm@stericsson.com>>, Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>
>>>
>>> From: Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>
>>> SDIO has been using runtime pm for a while to handle runtime power save
>>> operations. This patchset is enabling the option to make the sd/mmc
>>> blockdevices to use runtime pm as well.
>>>
>>> The runtime pm implementation for the block device will make use of
>>> autosuspend to defer power save operation to after request inactivty for
>>> a certain time.
>>>
>>> To actually perform some power save operations the corresponding bus ops
>>> for mmc and sd shall be implemented. Typically it could make sense to do
>>> BKOPS for eMMC in here.
>>>
>>> Ulf Hansson (3):
>>>   mmc: core: Remove power_restore bus_ops for mmc and sd
>>>   mmc: core: Add bus_ops for runtime pm callbacks
>>>   mmc: block: Enable runtime pm for mmc blkdevice
>>>
>> Ulf,
>>
>> sdhci.c has added pm_runtime which also protect between request and
>> task finish. And some sdhci.c based host drivers has provided
>> pm_runtime_suspend/resume functions like sdhci-pxav3.c. From the
>> powersave viewpoint, I think adding pm_runtime in driver level is
>> better than doing that on bus level since the control granularity is
>> even smaller. And adding pm_runtime in both block.c and sdhci.c will
>> call pm_runtime twice. How do you think?
>>
>> Thanks
>> Kevin
>
> Hi Kevin,
>
> Thanks for your response!
>
> It seems like we need some more clarification around this area.
> Runtime pm for a host device driver shall ultimately be responsible
> for taking care of runtime power management of the host device - only.
> It should not handle runtime power management of a block device, which
> in principle means BKOPS shall be handled in the blkdevice. At least
> this is my view.
>
> So, why is this? I will try to elaborate on the runtime pm support in
> host drivers here.
> The host device driver controls a MMC/SD/SDIO IP. This IP could very
> well reside (for some SoC) in what you call a power domain. In
> principle, once the IP needs to be used, a host driver has done a
> pm_runtime_get of it's device. This will mean a reference to the power
> domain has been fetched. Once the IP is not needed any more,
> pm_runtime_put is done and the reference to the power domain is
> released. Once no reference to the power domain exist the power domain
> can enter lower sleep states, which is preferred to happen as soon as
> possible and as long as possible - of course.
>
> Hope this gives a better understanding. :-)
>
Ulf,

Thanks for the explanations!
Then do you mean to start bkops when blkdev pm_runtime auto suspended
while stop bkops when blkdev pm_runtime resumed?
My only concern is that we have implemented pm_runtime for host device
and its pm_runtime functions will turn on/off bus clock when host dev
runtime resume/suspend. Let's see below sequence when an issue request
come:
1. blkdev pm_runtime resumed in mmc_blk_issue_rq.
2. blkdev issue request
3. host dev pm_runtime resumed in host->ops->request.
4. host finished the transfer and host dev pm_runtime suspended.
5. 3s later, blkdev pm_runtime suspended.
The bus clock will be turn off in step 4 by host dev
pm_runtime_suspend function. Then how can bkops run in step 5?

Kevin

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Kevin Liu]

2013/3/7 Kevin Liu <keyuan.liu@gmail.com>:
> 2013/3/7 Ulf Hansson <ulf.hansson@linaro.org>:
>> On 7 March 2013 01:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>>> From: Ulf Hansson <ulf.hansson@stericsson.com<mailto:ulf.hansson@stericsson.com>>
>>>> Date: Fri, Mar 1, 2013 at 8:47 PM
>>>> Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>>> To: linux-mmc@vger.kernel.org<mailto:linux-mmc@vger.kernel.org>, Chris Ball <cjb@laptop.org<mailto:cjb@laptop.org>>
>>>> Cc: Johan Rudholm <johan.rudholm@stericsson.com<mailto:johan.rudholm@stericsson.com>>, Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>
>>>>
>>>> From: Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>
>>>> SDIO has been using runtime pm for a while to handle runtime power save
>>>> operations. This patchset is enabling the option to make the sd/mmc
>>>> blockdevices to use runtime pm as well.
>>>>
>>>> The runtime pm implementation for the block device will make use of
>>>> autosuspend to defer power save operation to after request inactivty for
>>>> a certain time.
>>>>
>>>> To actually perform some power save operations the corresponding bus ops
>>>> for mmc and sd shall be implemented. Typically it could make sense to do
>>>> BKOPS for eMMC in here.
>>>>
>>>> Ulf Hansson (3):
>>>>   mmc: core: Remove power_restore bus_ops for mmc and sd
>>>>   mmc: core: Add bus_ops for runtime pm callbacks
>>>>   mmc: block: Enable runtime pm for mmc blkdevice
>>>>
>>> Ulf,
>>>
>>> sdhci.c has added pm_runtime which also protect between request and
>>> task finish. And some sdhci.c based host drivers has provided
>>> pm_runtime_suspend/resume functions like sdhci-pxav3.c. From the
>>> powersave viewpoint, I think adding pm_runtime in driver level is
>>> better than doing that on bus level since the control granularity is
>>> even smaller. And adding pm_runtime in both block.c and sdhci.c will
>>> call pm_runtime twice. How do you think?
>>>
>>> Thanks
>>> Kevin
>>
>> Hi Kevin,
>>
>> Thanks for your response!
>>
>> It seems like we need some more clarification around this area.
>> Runtime pm for a host device driver shall ultimately be responsible
>> for taking care of runtime power management of the host device - only.
>> It should not handle runtime power management of a block device, which
>> in principle means BKOPS shall be handled in the blkdevice. At least
>> this is my view.
>>
>> So, why is this? I will try to elaborate on the runtime pm support in
>> host drivers here.
>> The host device driver controls a MMC/SD/SDIO IP. This IP could very
>> well reside (for some SoC) in what you call a power domain. In
>> principle, once the IP needs to be used, a host driver has done a
>> pm_runtime_get of it's device. This will mean a reference to the power
>> domain has been fetched. Once the IP is not needed any more,
>> pm_runtime_put is done and the reference to the power domain is
>> released. Once no reference to the power domain exist the power domain
>> can enter lower sleep states, which is preferred to happen as soon as
>> possible and as long as possible - of course.
>>
>> Hope this gives a better understanding. :-)
>>
> Ulf,
>
> Thanks for the explanations!
> Then do you mean to start bkops when blkdev pm_runtime auto suspended
> while stop bkops when blkdev pm_runtime resumed?
> My only concern is that we have implemented pm_runtime for host device
> and its pm_runtime functions will turn on/off bus clock when host dev
> runtime resume/suspend. Let's see below sequence when an issue request
> come:
> 1. blkdev pm_runtime resumed in mmc_blk_issue_rq.
> 2. blkdev issue request
> 3. host dev pm_runtime resumed in host->ops->request.
> 4. host finished the transfer and host dev pm_runtime suspended.
> 5. 3s later, blkdev pm_runtime suspended.
> The bus clock will be turn off in step 4 by host dev
> pm_runtime_suspend function. Then how can bkops run in step 5?
>
My question is host dev will stop bus clock by pm_runtime_suspend once
the request transfer is finished. But bkops on emmc chip should still
need the bus clock after bkops started. How to handle this?

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Ulf Hansson]

On 7 March 2013 22:14, Kevin Liu <keyuan.liu@gmail.com> wrote:
> 2013/3/7 Kevin Liu <keyuan.liu@gmail.com>:
>> 2013/3/7 Ulf Hansson <ulf.hansson@linaro.org>:
>>> On 7 March 2013 01:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>>>> From: Ulf Hansson <ulf.hansson@stericsson.com<mailto:ulf.hansson@stericsson.com>>
>>>>> Date: Fri, Mar 1, 2013 at 8:47 PM
>>>>> Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>>>> To: linux-mmc@vger.kernel.org<mailto:linux-mmc@vger.kernel.org>, Chris Ball <cjb@laptop.org<mailto:cjb@laptop.org>>
>>>>> Cc: Johan Rudholm <johan.rudholm@stericsson.com<mailto:johan.rudholm@stericsson.com>>, Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>>
>>>>>
>>>>> From: Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>>
>>>>> SDIO has been using runtime pm for a while to handle runtime power save
>>>>> operations. This patchset is enabling the option to make the sd/mmc
>>>>> blockdevices to use runtime pm as well.
>>>>>
>>>>> The runtime pm implementation for the block device will make use of
>>>>> autosuspend to defer power save operation to after request inactivty for
>>>>> a certain time.
>>>>>
>>>>> To actually perform some power save operations the corresponding bus ops
>>>>> for mmc and sd shall be implemented. Typically it could make sense to do
>>>>> BKOPS for eMMC in here.
>>>>>
>>>>> Ulf Hansson (3):
>>>>>   mmc: core: Remove power_restore bus_ops for mmc and sd
>>>>>   mmc: core: Add bus_ops for runtime pm callbacks
>>>>>   mmc: block: Enable runtime pm for mmc blkdevice
>>>>>
>>>> Ulf,
>>>>
>>>> sdhci.c has added pm_runtime which also protect between request and
>>>> task finish. And some sdhci.c based host drivers has provided
>>>> pm_runtime_suspend/resume functions like sdhci-pxav3.c. From the
>>>> powersave viewpoint, I think adding pm_runtime in driver level is
>>>> better than doing that on bus level since the control granularity is
>>>> even smaller. And adding pm_runtime in both block.c and sdhci.c will
>>>> call pm_runtime twice. How do you think?
>>>>
>>>> Thanks
>>>> Kevin
>>>
>>> Hi Kevin,
>>>
>>> Thanks for your response!
>>>
>>> It seems like we need some more clarification around this area.
>>> Runtime pm for a host device driver shall ultimately be responsible
>>> for taking care of runtime power management of the host device - only.
>>> It should not handle runtime power management of a block device, which
>>> in principle means BKOPS shall be handled in the blkdevice. At least
>>> this is my view.
>>>
>>> So, why is this? I will try to elaborate on the runtime pm support in
>>> host drivers here.
>>> The host device driver controls a MMC/SD/SDIO IP. This IP could very
>>> well reside (for some SoC) in what you call a power domain. In
>>> principle, once the IP needs to be used, a host driver has done a
>>> pm_runtime_get of it's device. This will mean a reference to the power
>>> domain has been fetched. Once the IP is not needed any more,
>>> pm_runtime_put is done and the reference to the power domain is
>>> released. Once no reference to the power domain exist the power domain
>>> can enter lower sleep states, which is preferred to happen as soon as
>>> possible and as long as possible - of course.
>>>
>>> Hope this gives a better understanding. :-)
>>>
>> Ulf,
>>
>> Thanks for the explanations!
>> Then do you mean to start bkops when blkdev pm_runtime auto suspended
>> while stop bkops when blkdev pm_runtime resumed?
>> My only concern is that we have implemented pm_runtime for host device
>> and its pm_runtime functions will turn on/off bus clock when host dev
>> runtime resume/suspend. Let's see below sequence when an issue request
>> come:
>> 1. blkdev pm_runtime resumed in mmc_blk_issue_rq.
>> 2. blkdev issue request
>> 3. host dev pm_runtime resumed in host->ops->request.
>> 4. host finished the transfer and host dev pm_runtime suspended.
>> 5. 3s later, blkdev pm_runtime suspended.
>> The bus clock will be turn off in step 4 by host dev
>> pm_runtime_suspend function. Then how can bkops run in step 5?
>>
> My question is host dev will stop bus clock by pm_runtime_suspend once
> the request transfer is finished. But bkops on emmc chip should still
> need the bus clock after bkops started. How to handle this?

According the eMMC spec I can't see any requirement that the bus clock
needs to be on while a BKOPS is running. Moreover it is clearly stated
it is allowed to gate the bus clock for a device which indicates busy.
So, I can't see that this is needed.

Kind regards
Ulf Hansson

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Kevin Liu]

2013/3/8 Ulf Hansson <ulf.hansson@linaro.org>:
> On 7 March 2013 22:14, Kevin Liu <keyuan.liu@gmail.com> wrote:
>> 2013/3/7 Kevin Liu <keyuan.liu@gmail.com>:
>>> 2013/3/7 Ulf Hansson <ulf.hansson@linaro.org>:
>>>> On 7 March 2013 01:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>>>>> From: Ulf Hansson <ulf.hansson@stericsson.com<mailto:ulf.hansson@stericsson.com>>
>>>>>> Date: Fri, Mar 1, 2013 at 8:47 PM
>>>>>> Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>>>>> To: linux-mmc@vger.kernel.org<mailto:linux-mmc@vger.kernel.org>, Chris Ball <cjb@laptop.org<mailto:cjb@laptop.org>>
>>>>>> Cc: Johan Rudholm <johan.rudholm@stericsson.com<mailto:johan.rudholm@stericsson.com>>, Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>>>
>>>>>>
>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>>>
>>>>>> SDIO has been using runtime pm for a while to handle runtime power save
>>>>>> operations. This patchset is enabling the option to make the sd/mmc
>>>>>> blockdevices to use runtime pm as well.
>>>>>>
>>>>>> The runtime pm implementation for the block device will make use of
>>>>>> autosuspend to defer power save operation to after request inactivty for
>>>>>> a certain time.
>>>>>>
>>>>>> To actually perform some power save operations the corresponding bus ops
>>>>>> for mmc and sd shall be implemented. Typically it could make sense to do
>>>>>> BKOPS for eMMC in here.
>>>>>>
>>>>>> Ulf Hansson (3):
>>>>>>   mmc: core: Remove power_restore bus_ops for mmc and sd
>>>>>>   mmc: core: Add bus_ops for runtime pm callbacks
>>>>>>   mmc: block: Enable runtime pm for mmc blkdevice
>>>>>>
>>>>> Ulf,
>>>>>
>>>>> sdhci.c has added pm_runtime which also protect between request and
>>>>> task finish. And some sdhci.c based host drivers has provided
>>>>> pm_runtime_suspend/resume functions like sdhci-pxav3.c. From the
>>>>> powersave viewpoint, I think adding pm_runtime in driver level is
>>>>> better than doing that on bus level since the control granularity is
>>>>> even smaller. And adding pm_runtime in both block.c and sdhci.c will
>>>>> call pm_runtime twice. How do you think?
>>>>>
>>>>> Thanks
>>>>> Kevin
>>>>
>>>> Hi Kevin,
>>>>
>>>> Thanks for your response!
>>>>
>>>> It seems like we need some more clarification around this area.
>>>> Runtime pm for a host device driver shall ultimately be responsible
>>>> for taking care of runtime power management of the host device - only.
>>>> It should not handle runtime power management of a block device, which
>>>> in principle means BKOPS shall be handled in the blkdevice. At least
>>>> this is my view.
>>>>
>>>> So, why is this? I will try to elaborate on the runtime pm support in
>>>> host drivers here.
>>>> The host device driver controls a MMC/SD/SDIO IP. This IP could very
>>>> well reside (for some SoC) in what you call a power domain. In
>>>> principle, once the IP needs to be used, a host driver has done a
>>>> pm_runtime_get of it's device. This will mean a reference to the power
>>>> domain has been fetched. Once the IP is not needed any more,
>>>> pm_runtime_put is done and the reference to the power domain is
>>>> released. Once no reference to the power domain exist the power domain
>>>> can enter lower sleep states, which is preferred to happen as soon as
>>>> possible and as long as possible - of course.
>>>>
>>>> Hope this gives a better understanding. :-)
>>>>
>>> Ulf,
>>>
>>> Thanks for the explanations!
>>> Then do you mean to start bkops when blkdev pm_runtime auto suspended
>>> while stop bkops when blkdev pm_runtime resumed?
>>> My only concern is that we have implemented pm_runtime for host device
>>> and its pm_runtime functions will turn on/off bus clock when host dev
>>> runtime resume/suspend. Let's see below sequence when an issue request
>>> come:
>>> 1. blkdev pm_runtime resumed in mmc_blk_issue_rq.
>>> 2. blkdev issue request
>>> 3. host dev pm_runtime resumed in host->ops->request.
>>> 4. host finished the transfer and host dev pm_runtime suspended.
>>> 5. 3s later, blkdev pm_runtime suspended.
>>> The bus clock will be turn off in step 4 by host dev
>>> pm_runtime_suspend function. Then how can bkops run in step 5?
>>>
>> My question is host dev will stop bus clock by pm_runtime_suspend once
>> the request transfer is finished. But bkops on emmc chip should still
>> need the bus clock after bkops started. How to handle this?
>
> According the eMMC spec I can't see any requirement that the bus clock
> needs to be on while a BKOPS is running. Moreover it is clearly stated
> it is allowed to gate the bus clock for a device which indicates busy.
> So, I can't see that this is needed.
>

Got it. Thanks!

> Kind regards
> Ulf Hansson

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Sujit Reddy Thumma]

On 3/8/2013 8:44 AM, Ulf Hansson wrote:
> On 7 March 2013 22:14, Kevin Liu <keyuan.liu@gmail.com> wrote:
>> 2013/3/7 Kevin Liu <keyuan.liu@gmail.com>:
>>> 2013/3/7 Ulf Hansson <ulf.hansson@linaro.org>:
>>>> On 7 March 2013 01:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>>>>> From: Ulf Hansson <ulf.hansson@stericsson.com<mailto:ulf.hansson@stericsson.com>>
>>>>>> Date: Fri, Mar 1, 2013 at 8:47 PM
>>>>>> Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>>>>> To: linux-mmc@vger.kernel.org<mailto:linux-mmc@vger.kernel.org>, Chris Ball <cjb@laptop.org<mailto:cjb@laptop.org>>
>>>>>> Cc: Johan Rudholm <johan.rudholm@stericsson.com<mailto:johan.rudholm@stericsson.com>>, Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>>>
>>>>>>
>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>>>
>>>>>> SDIO has been using runtime pm for a while to handle runtime power save
>>>>>> operations. This patchset is enabling the option to make the sd/mmc
>>>>>> blockdevices to use runtime pm as well.
>>>>>>
>>>>>> The runtime pm implementation for the block device will make use of
>>>>>> autosuspend to defer power save operation to after request inactivty for
>>>>>> a certain time.
>>>>>>
>>>>>> To actually perform some power save operations the corresponding bus ops
>>>>>> for mmc and sd shall be implemented. Typically it could make sense to do
>>>>>> BKOPS for eMMC in here.
>>>>>>
>>>>>> Ulf Hansson (3):
>>>>>>    mmc: core: Remove power_restore bus_ops for mmc and sd
>>>>>>    mmc: core: Add bus_ops for runtime pm callbacks
>>>>>>    mmc: block: Enable runtime pm for mmc blkdevice
>>>>>>
>>>>> Ulf,
>>>>>
>>>>> sdhci.c has added pm_runtime which also protect between request and
>>>>> task finish. And some sdhci.c based host drivers has provided
>>>>> pm_runtime_suspend/resume functions like sdhci-pxav3.c. From the
>>>>> powersave viewpoint, I think adding pm_runtime in driver level is
>>>>> better than doing that on bus level since the control granularity is
>>>>> even smaller. And adding pm_runtime in both block.c and sdhci.c will
>>>>> call pm_runtime twice. How do you think?
>>>>>
>>>>> Thanks
>>>>> Kevin
>>>>
>>>> Hi Kevin,
>>>>
>>>> Thanks for your response!
>>>>
>>>> It seems like we need some more clarification around this area.
>>>> Runtime pm for a host device driver shall ultimately be responsible
>>>> for taking care of runtime power management of the host device - only.
>>>> It should not handle runtime power management of a block device, which
>>>> in principle means BKOPS shall be handled in the blkdevice. At least
>>>> this is my view.
>>>>
>>>> So, why is this? I will try to elaborate on the runtime pm support in
>>>> host drivers here.
>>>> The host device driver controls a MMC/SD/SDIO IP. This IP could very
>>>> well reside (for some SoC) in what you call a power domain. In
>>>> principle, once the IP needs to be used, a host driver has done a
>>>> pm_runtime_get of it's device. This will mean a reference to the power
>>>> domain has been fetched. Once the IP is not needed any more,
>>>> pm_runtime_put is done and the reference to the power domain is
>>>> released. Once no reference to the power domain exist the power domain
>>>> can enter lower sleep states, which is preferred to happen as soon as
>>>> possible and as long as possible - of course.
>>>>
>>>> Hope this gives a better understanding. :-)
>>>>
>>> Ulf,
>>>
>>> Thanks for the explanations!
>>> Then do you mean to start bkops when blkdev pm_runtime auto suspended
>>> while stop bkops when blkdev pm_runtime resumed?
>>> My only concern is that we have implemented pm_runtime for host device
>>> and its pm_runtime functions will turn on/off bus clock when host dev
>>> runtime resume/suspend. Let's see below sequence when an issue request
>>> come:
>>> 1. blkdev pm_runtime resumed in mmc_blk_issue_rq.
>>> 2. blkdev issue request
>>> 3. host dev pm_runtime resumed in host->ops->request.
>>> 4. host finished the transfer and host dev pm_runtime suspended.
>>> 5. 3s later, blkdev pm_runtime suspended.
>>> The bus clock will be turn off in step 4 by host dev
>>> pm_runtime_suspend function. Then how can bkops run in step 5?
>>>
>> My question is host dev will stop bus clock by pm_runtime_suspend once
>> the request transfer is finished. But bkops on emmc chip should still
>> need the bus clock after bkops started. How to handle this?
>
> According the eMMC spec I can't see any requirement that the bus clock
> needs to be on while a BKOPS is running. Moreover it is clearly stated
> it is allowed to gate the bus clock for a device which indicates busy.
> So, I can't see that this is needed.
>

What if host is aggressive and wants to keep eMMC in sleep-mode and turn 
off VCC regulator during runtime power management? I believe that eMMC 
card needs VCC supply as well in addition to VCCQ to carry out BKOPS. Do 
you think that the block device also needs to take a reference for VCC 
regulator while BKOPS is started in runtime suspend of block device?


-- 
Regards,
Sujit

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Ulf Hansson]

On 15 March 2013 05:18, Sujit Reddy Thumma <sthumma@codeaurora.org> wrote:
> On 3/8/2013 8:44 AM, Ulf Hansson wrote:
>>
>> On 7 March 2013 22:14, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>>
>>> 2013/3/7 Kevin Liu <keyuan.liu@gmail.com>:
>>>>
>>>> 2013/3/7 Ulf Hansson <ulf.hansson@linaro.org>:
>>>>>
>>>>> On 7 March 2013 01:12, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>>>>>>
>>>>>>> From: Ulf Hansson
>>>>>>> <ulf.hansson@stericsson.com<mailto:ulf.hansson@stericsson.com>>
>>>>>>> Date: Fri, Mar 1, 2013 at 8:47 PM
>>>>>>> Subject: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>>>>>> To: linux-mmc@vger.kernel.org<mailto:linux-mmc@vger.kernel.org>,
>>>>>>> Chris Ball <cjb@laptop.org<mailto:cjb@laptop.org>>
>>>>>>> Cc: Johan Rudholm
>>>>>>> <johan.rudholm@stericsson.com<mailto:johan.rudholm@stericsson.com>>, Ulf
>>>>>>> Hansson <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>>>>
>>>>>>>
>>>>>>> From: Ulf Hansson
>>>>>>> <ulf.hansson@linaro.org<mailto:ulf.hansson@linaro.org>>
>>>>>>>
>>>>>>> SDIO has been using runtime pm for a while to handle runtime power
>>>>>>> save
>>>>>>> operations. This patchset is enabling the option to make the sd/mmc
>>>>>>> blockdevices to use runtime pm as well.
>>>>>>>
>>>>>>> The runtime pm implementation for the block device will make use of
>>>>>>> autosuspend to defer power save operation to after request inactivty
>>>>>>> for
>>>>>>> a certain time.
>>>>>>>
>>>>>>> To actually perform some power save operations the corresponding bus
>>>>>>> ops
>>>>>>> for mmc and sd shall be implemented. Typically it could make sense to
>>>>>>> do
>>>>>>> BKOPS for eMMC in here.
>>>>>>>
>>>>>>> Ulf Hansson (3):
>>>>>>>    mmc: core: Remove power_restore bus_ops for mmc and sd
>>>>>>>    mmc: core: Add bus_ops for runtime pm callbacks
>>>>>>>    mmc: block: Enable runtime pm for mmc blkdevice
>>>>>>>
>>>>>> Ulf,
>>>>>>
>>>>>> sdhci.c has added pm_runtime which also protect between request and
>>>>>> task finish. And some sdhci.c based host drivers has provided
>>>>>> pm_runtime_suspend/resume functions like sdhci-pxav3.c. From the
>>>>>> powersave viewpoint, I think adding pm_runtime in driver level is
>>>>>> better than doing that on bus level since the control granularity is
>>>>>> even smaller. And adding pm_runtime in both block.c and sdhci.c will
>>>>>> call pm_runtime twice. How do you think?
>>>>>>
>>>>>> Thanks
>>>>>> Kevin
>>>>>
>>>>>
>>>>> Hi Kevin,
>>>>>
>>>>> Thanks for your response!
>>>>>
>>>>> It seems like we need some more clarification around this area.
>>>>> Runtime pm for a host device driver shall ultimately be responsible
>>>>> for taking care of runtime power management of the host device - only.
>>>>> It should not handle runtime power management of a block device, which
>>>>> in principle means BKOPS shall be handled in the blkdevice. At least
>>>>> this is my view.
>>>>>
>>>>> So, why is this? I will try to elaborate on the runtime pm support in
>>>>> host drivers here.
>>>>> The host device driver controls a MMC/SD/SDIO IP. This IP could very
>>>>> well reside (for some SoC) in what you call a power domain. In
>>>>> principle, once the IP needs to be used, a host driver has done a
>>>>> pm_runtime_get of it's device. This will mean a reference to the power
>>>>> domain has been fetched. Once the IP is not needed any more,
>>>>> pm_runtime_put is done and the reference to the power domain is
>>>>> released. Once no reference to the power domain exist the power domain
>>>>> can enter lower sleep states, which is preferred to happen as soon as
>>>>> possible and as long as possible - of course.
>>>>>
>>>>> Hope this gives a better understanding. :-)
>>>>>
>>>> Ulf,
>>>>
>>>> Thanks for the explanations!
>>>> Then do you mean to start bkops when blkdev pm_runtime auto suspended
>>>> while stop bkops when blkdev pm_runtime resumed?
>>>> My only concern is that we have implemented pm_runtime for host device
>>>> and its pm_runtime functions will turn on/off bus clock when host dev
>>>> runtime resume/suspend. Let's see below sequence when an issue request
>>>> come:
>>>> 1. blkdev pm_runtime resumed in mmc_blk_issue_rq.
>>>> 2. blkdev issue request
>>>> 3. host dev pm_runtime resumed in host->ops->request.
>>>> 4. host finished the transfer and host dev pm_runtime suspended.
>>>> 5. 3s later, blkdev pm_runtime suspended.
>>>> The bus clock will be turn off in step 4 by host dev
>>>> pm_runtime_suspend function. Then how can bkops run in step 5?
>>>>
>>> My question is host dev will stop bus clock by pm_runtime_suspend once
>>> the request transfer is finished. But bkops on emmc chip should still
>>> need the bus clock after bkops started. How to handle this?
>>
>>
>> According the eMMC spec I can't see any requirement that the bus clock
>> needs to be on while a BKOPS is running. Moreover it is clearly stated
>> it is allowed to gate the bus clock for a device which indicates busy.
>> So, I can't see that this is needed.
>>
>
> What if host is aggressive and wants to keep eMMC in sleep-mode and turn off
> VCC regulator during runtime power management? I believe that eMMC card
> needs VCC supply as well in addition to VCCQ to carry out BKOPS. Do you
> think that the block device also needs to take a reference for VCC regulator
> while BKOPS is started in runtime suspend of block device?

What you are thinking of would be exactly the same scenario as doing
"mmc_suspend_host" from a host runtime suspend callback, which have
been discussed here earlier. Right now, no up-streamed host driver is
doing this and I would guess it would never happen either. Anyway,
still worth to consider somehow.

Please have a look at below thread to find the answers to your questions:
http://thread.gmane.org/gmane.linux.kernel.mmc/19444/focus=19443


>
>
> --
> Regards,
> Sujit

Kind regards
Ulf Hansson

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Sujit Reddy Thumma]

>>> According the eMMC spec I can't see any requirement that the bus clock
>>> needs to be on while a BKOPS is running. Moreover it is clearly stated
>>> it is allowed to gate the bus clock for a device which indicates busy.
>>> So, I can't see that this is needed.
>>>
>>
>> What if host is aggressive and wants to keep eMMC in sleep-mode and turn off
>> VCC regulator during runtime power management? I believe that eMMC card
>> needs VCC supply as well in addition to VCCQ to carry out BKOPS. Do you
>> think that the block device also needs to take a reference for VCC regulator
>> while BKOPS is started in runtime suspend of block device?
>
> What you are thinking of would be exactly the same scenario as doing
> "mmc_suspend_host" from a host runtime suspend callback, which have
> been discussed here earlier. Right now, no up-streamed host driver is
> doing this and I would guess it would never happen either. Anyway,
> still worth to consider somehow.

If any driver wants to implement this then the runtime PM code would be 
refactored again. So I guess we might want to think about this now itself?

What about SD cards? For SD cards the runtime PM is not doing any 
advantage but instead waste cpu cycles with a timer interrupt and 
running noop runtime PM callbacks? I guess allowing to power off cards 
in such cases would have decent power savings.

>
> Please have a look at below thread to find the answers to your questions:
> http://thread.gmane.org/gmane.linux.kernel.mmc/19444/focus=19443
>

Thanks a lot. I have missed this discussion :(
I have some comments on the possible solutions:

"In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
device"), and vice verse in the runtime resume callback. This will
prevent the host driver from entering runtime power save sate while
for example doing BKOPS, thus preventing your host driver from doing
mmc_suspend_host while BKOPS is running"

[Sujit] In addition, probably we can allow host to turn off the clocks 
while carrying out BKOPS. But, how can we know whether card is done with 
BKOPS and is idle so that we can call mmc_suspend_host()?

"Move mmc_suspend|resume_host from your host runtime callbacks, into
the bus_ops runtime callbacks. Typically, when no BKOPS is needed
mmc_suspend_host can be done."

[Sujit] Doesn't it look like we are establishing parent child 
relationship here? If the card has nothing to do, suspend the host?


-- 
Regards,
Sujit

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Ulf Hansson]

On 20 March 2013 16:44, Sujit Reddy Thumma <sthumma@codeaurora.org> wrote:
>>>> According the eMMC spec I can't see any requirement that the bus clock
>>>> needs to be on while a BKOPS is running. Moreover it is clearly stated
>>>> it is allowed to gate the bus clock for a device which indicates busy.
>>>> So, I can't see that this is needed.
>>>>
>>>
>>> What if host is aggressive and wants to keep eMMC in sleep-mode and turn
>>> off
>>> VCC regulator during runtime power management? I believe that eMMC card
>>> needs VCC supply as well in addition to VCCQ to carry out BKOPS. Do you
>>> think that the block device also needs to take a reference for VCC
>>> regulator
>>> while BKOPS is started in runtime suspend of block device?
>>
>>
>> What you are thinking of would be exactly the same scenario as doing
>> "mmc_suspend_host" from a host runtime suspend callback, which have
>> been discussed here earlier. Right now, no up-streamed host driver is
>> doing this and I would guess it would never happen either. Anyway,
>> still worth to consider somehow.
>
>
> If any driver wants to implement this then the runtime PM code would be
> refactored again. So I guess we might want to think about this now itself?

Refactored, no.

It is just a new feature that needs to be added, should be a rather
simple patch. Since this kind of code has not been upstreamed for your
host driver I did not consider it in this initial step. Do you want me
to create an additional patch on top of this patchset? I can help out
if you like.

>
> What about SD cards? For SD cards the runtime PM is not doing any advantage
> but instead waste cpu cycles with a timer interrupt and running noop runtime
> PM callbacks? I guess allowing to power off cards in such cases would have
> decent power savings.

We will waste some cpu cycles, true.

Do you think that will have bad impact on performance? In that case
why do we even bother doing runtime PM in host drivers and in many
other places in the kernel? Of course we could optmize the code and
only enable runtime PM if there are a corresponding runtime pm
callbacks implemented in the bus_ops, but is it needed?

>
>
>>
>> Please have a look at below thread to find the answers to your questions:
>> http://thread.gmane.org/gmane.linux.kernel.mmc/19444/focus=19443
>>
>
> Thanks a lot. I have missed this discussion :(
> I have some comments on the possible solutions:
>
> "In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
> device"), and vice verse in the runtime resume callback. This will
> prevent the host driver from entering runtime power save sate while
> for example doing BKOPS, thus preventing your host driver from doing
> mmc_suspend_host while BKOPS is running"
>
> [Sujit] In addition, probably we can allow host to turn off the clocks while
> carrying out BKOPS. But, how can we know whether card is done with BKOPS and
> is idle so that we can call mmc_suspend_host()?

We are then going into details about how to implement IDLE BKOPS,
which is a bit out of scope for this patch, but let me try to comment
anyway.

The initial patch for BKOPS could skip your consideration, and just
check for BKOPS complete once runtime suspend callback gets called.
This will then be rather simple to implement and work for all cases
but yours. I realize that a new blk request will be needed to move out
from BKOPS state then.

The next step could be to schedule a timer/work when issuing BKOPS,
that polls for completion. I belive it should be rather simple to
extend the runtime pm callbacks with this support, right?

>
> "Move mmc_suspend|resume_host from your host runtime callbacks, into
> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
> mmc_suspend_host can be done."
>
> [Sujit] Doesn't it look like we are establishing parent child relationship
> here? If the card has nothing to do, suspend the host?

Well, the naming of these functions are not correct. It is not the
host that actually gets suspended, it is the card.

Right now these functions happens to be called when a host enters
suspend though, which indeed is also kind of strange. It would make
more sense to handle card suspend from the mmc/sdio bus instead; but
let's leave that for a separate discussion. :-)

I also assume that if your host driver runtime pm callbacks calls
mmc_suspend|resume_host, your host driver system suspend|resume
callbacks must not - otherwise you will have races! Instead upper
layers like a power domain, must force your device into runtime
suspend when entering system suspend and vice verse when doing system
resume. These issues exists with or without my patches.


>
>
> --
> Regards,
> Sujit

Thanks a lot for comment, Sujit!

Kind regards
Ulf Hansson

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Ulf Hansson]

On 20 March 2013 22:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 20 March 2013 16:44, Sujit Reddy Thumma <sthumma@codeaurora.org> wrote:
>>>>> According the eMMC spec I can't see any requirement that the bus clock
>>>>> needs to be on while a BKOPS is running. Moreover it is clearly stated
>>>>> it is allowed to gate the bus clock for a device which indicates busy.
>>>>> So, I can't see that this is needed.
>>>>>
>>>>
>>>> What if host is aggressive and wants to keep eMMC in sleep-mode and turn
>>>> off
>>>> VCC regulator during runtime power management? I believe that eMMC card
>>>> needs VCC supply as well in addition to VCCQ to carry out BKOPS. Do you
>>>> think that the block device also needs to take a reference for VCC
>>>> regulator
>>>> while BKOPS is started in runtime suspend of block device?
>>>
>>>
>>> What you are thinking of would be exactly the same scenario as doing
>>> "mmc_suspend_host" from a host runtime suspend callback, which have
>>> been discussed here earlier. Right now, no up-streamed host driver is
>>> doing this and I would guess it would never happen either. Anyway,
>>> still worth to consider somehow.
>>
>>
>> If any driver wants to implement this then the runtime PM code would be
>> refactored again. So I guess we might want to think about this now itself?
>
> Refactored, no.
>
> It is just a new feature that needs to be added, should be a rather
> simple patch. Since this kind of code has not been upstreamed for your
> host driver I did not consider it in this initial step. Do you want me
> to create an additional patch on top of this patchset? I can help out
> if you like.
>
>>
>> What about SD cards? For SD cards the runtime PM is not doing any advantage
>> but instead waste cpu cycles with a timer interrupt and running noop runtime
>> PM callbacks? I guess allowing to power off cards in such cases would have
>> decent power savings.
>
> We will waste some cpu cycles, true.
>
> Do you think that will have bad impact on performance? In that case
> why do we even bother doing runtime PM in host drivers and in many
> other places in the kernel? Of course we could optmize the code and
> only enable runtime PM if there are a corresponding runtime pm
> callbacks implemented in the bus_ops, but is it needed?
>
>>
>>
>>>
>>> Please have a look at below thread to find the answers to your questions:
>>> http://thread.gmane.org/gmane.linux.kernel.mmc/19444/focus=19443
>>>
>>
>> Thanks a lot. I have missed this discussion :(
>> I have some comments on the possible solutions:
>>
>> "In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
>> device"), and vice verse in the runtime resume callback. This will
>> prevent the host driver from entering runtime power save sate while
>> for example doing BKOPS, thus preventing your host driver from doing
>> mmc_suspend_host while BKOPS is running"
>>
>> [Sujit] In addition, probably we can allow host to turn off the clocks while
>> carrying out BKOPS. But, how can we know whether card is done with BKOPS and
>> is idle so that we can call mmc_suspend_host()?
>
> We are then going into details about how to implement IDLE BKOPS,
> which is a bit out of scope for this patch, but let me try to comment
> anyway.
>
> The initial patch for BKOPS could skip your consideration, and just
> check for BKOPS complete once runtime suspend callback gets called.

Sorry, I mean when runtime resume get called...

> This will then be rather simple to implement and work for all cases
> but yours. I realize that a new blk request will be needed to move out
> from BKOPS state then.
>
> The next step could be to schedule a timer/work when issuing BKOPS,
> that polls for completion. I belive it should be rather simple to
> extend the runtime pm callbacks with this support, right?
>
>>
>> "Move mmc_suspend|resume_host from your host runtime callbacks, into
>> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
>> mmc_suspend_host can be done."
>>
>> [Sujit] Doesn't it look like we are establishing parent child relationship
>> here? If the card has nothing to do, suspend the host?
>
> Well, the naming of these functions are not correct. It is not the
> host that actually gets suspended, it is the card.
>
> Right now these functions happens to be called when a host enters
> suspend though, which indeed is also kind of strange. It would make
> more sense to handle card suspend from the mmc/sdio bus instead; but
> let's leave that for a separate discussion. :-)
>
> I also assume that if your host driver runtime pm callbacks calls
> mmc_suspend|resume_host, your host driver system suspend|resume
> callbacks must not - otherwise you will have races! Instead upper
> layers like a power domain, must force your device into runtime
> suspend when entering system suspend and vice verse when doing system
> resume. These issues exists with or without my patches.
>
>
>>
>>
>> --
>> Regards,
>> Sujit
>
> Thanks a lot for comment, Sujit!
>
> Kind regards
> Ulf Hansson

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Sujit Reddy Thumma]

Hi Ulf,

Sorry for the delayed response.
The patches looks good to me except one concern mentioned below.

On 3/21/2013 3:28 AM, Ulf Hansson wrote:

>>
>> If any driver wants to implement this then the runtime PM code would be
>> refactored again. So I guess we might want to think about this now itself?
>
> Refactored, no.
>
> It is just a new feature that needs to be added, should be a rather
> simple patch. Since this kind of code has not been upstreamed for your
> host driver I did not consider it in this initial step. Do you want me
> to create an additional patch on top of this patchset? I can help out
> if you like.
>
>>
>> What about SD cards? For SD cards the runtime PM is not doing any advantage
>> but instead waste cpu cycles with a timer interrupt and running noop runtime
>> PM callbacks? I guess allowing to power off cards in such cases would have
>> decent power savings.
>
> We will waste some cpu cycles, true.
>
> Do you think that will have bad impact on performance? In that case
> why do we even bother doing runtime PM in host drivers and in many
> other places in the kernel? Of course we could optmize the code and
> only enable runtime PM if there are a corresponding runtime pm
> callbacks implemented in the bus_ops, but is it needed?

Well.. my point here is that runtime PM framework unnecessarily wakeup 
processor (if idle) every "x" secs without doing any useful work. If 
that is agreeable then I am okay with not having the optimization.

>
>>
>>
>>>
>>> Please have a look at below thread to find the answers to your questions:
>>> http://thread.gmane.org/gmane.linux.kernel.mmc/19444/focus=19443
>>>
>>
>> Thanks a lot. I have missed this discussion :(
>> I have some comments on the possible solutions:
>>
>> "In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
>> device"), and vice verse in the runtime resume callback. This will
>> prevent the host driver from entering runtime power save sate while
>> for example doing BKOPS, thus preventing your host driver from doing
>> mmc_suspend_host while BKOPS is running"
>>
>> [Sujit] In addition, probably we can allow host to turn off the clocks while
>> carrying out BKOPS. But, how can we know whether card is done with BKOPS and
>> is idle so that we can call mmc_suspend_host()?
>
> We are then going into details about how to implement IDLE BKOPS,
> which is a bit out of scope for this patch, but let me try to comment
> anyway.
>
> The initial patch for BKOPS could skip your consideration, and just
> check for BKOPS complete once runtime suspend callback gets called.
> This will then be rather simple to implement and work for all cases
> but yours. I realize that a new blk request will be needed to move out
> from BKOPS state then.
>
> The next step could be to schedule a timer/work when issuing BKOPS,
> that polls for completion. I belive it should be rather simple to
> extend the runtime pm callbacks with this support, right?

Thanks for the details. It looks clear to me now.

>
>>
>> "Move mmc_suspend|resume_host from your host runtime callbacks, into
>> the bus_ops runtime callbacks. Typically, when no BKOPS is needed
>> mmc_suspend_host can be done."
>>
>> [Sujit] Doesn't it look like we are establishing parent child relationship
>> here? If the card has nothing to do, suspend the host?
>
> Well, the naming of these functions are not correct. It is not the
> host that actually gets suspended, it is the card.
>
> Right now these functions happens to be called when a host enters
> suspend though, which indeed is also kind of strange. It would make
> more sense to handle card suspend from the mmc/sdio bus instead; but
> let's leave that for a separate discussion. :-)

I agree.

>
> I also assume that if your host driver runtime pm callbacks calls
> mmc_suspend|resume_host, your host driver system suspend|resume
> callbacks must not - otherwise you will have races! Instead upper
> layers like a power domain, must force your device into runtime
> suspend when entering system suspend and vice verse when doing system
> resume. These issues exists with or without my patches.
>

Possibly, the races can be avoided using pm_runtime_suspended() check, 
but I am not sure if it is the clean way.


-- 
Regards,
Sujit

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Kevin Liu]

> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Wednesday, March 27, 2013 9:32 PM
> To: Ulf Hansson
> Cc: linux-mmc@vger.kernel.org; Johan Rudholm; Ulf Hansson; Maya Erez; Kevin Liu; Seungwon Jeon; jh80.chung@samsung.com
> Subject: Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>
> Hi Maya, Kevin, Seungwon, Jaehoon,
>
> On Fri, Mar 01 2013, Ulf Hansson wrote:
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> SDIO has been using runtime pm for a while to handle runtime power save
>> operations. This patchset is enabling the option to make the sd/mmc
>> blockdevices to use runtime pm as well.
>>
>> The runtime pm implementation for the block device will make use of
>> autosuspend to defer power save operation to after request inactivty for
>> a certain time.
>>
>> To actually perform some power save operations the corresponding bus ops
>> for mmc and sd shall be implemented. Typically it could make sense to do
>> BKOPS for eMMC in here.
>>
>> Ulf Hansson (3):
>>   mmc: core: Remove power_restore bus_ops for mmc and sd
>>   mmc: core: Add bus_ops for runtime pm callbacks
>>   mmc: block: Enable runtime pm for mmc blkdevice
>>
>>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>  drivers/mmc/core/bus.c   |   14 ++++++++++++--
>>  drivers/mmc/core/core.h  |    2 ++
>>  drivers/mmc/core/mmc.c   |   14 --------------
>>  drivers/mmc/core/sd.c    |   14 --------------
>>  drivers/mmc/core/sdio.c  |   20 ++++++++++++++++++++
>>  6 files changed, 60 insertions(+), 32 deletions(-)
>
> This looks good to me, but I'd like to make sure the design works for
> you before we decide to merge it.  Any concerns?
>

Chris,

This patchset is ok for me.
We can discuss/review how sd/mmc runtime callback function implemented later.

Thanks
Kevin

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[merez]

Hi Chris,

Sorry for the late response.
I am currently on vacation and would get back to work next week.
We would like to have an internal discussion to make sure there are no
additional concerns with this patch.
Can you please wait with this patch approval until the end of next week?

Thanks,
Maya

>> -----Original Message-----
>> From: Chris Ball [mailto:cjb@laptop.org]
>> Sent: Wednesday, March 27, 2013 9:32 PM
>> To: Ulf Hansson
>> Cc: linux-mmc@vger.kernel.org; Johan Rudholm; Ulf Hansson; Maya Erez;
>> Kevin Liu; Seungwon Jeon; jh80.chung@samsung.com
>> Subject: Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>
>> Hi Maya, Kevin, Seungwon, Jaehoon,
>>
>> On Fri, Mar 01 2013, Ulf Hansson wrote:
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> SDIO has been using runtime pm for a while to handle runtime power save
>>> operations. This patchset is enabling the option to make the sd/mmc
>>> blockdevices to use runtime pm as well.
>>>
>>> The runtime pm implementation for the block device will make use of
>>> autosuspend to defer power save operation to after request inactivty
>>> for
>>> a certain time.
>>>
>>> To actually perform some power save operations the corresponding bus
>>> ops
>>> for mmc and sd shall be implemented. Typically it could make sense to
>>> do
>>> BKOPS for eMMC in here.
>>>
>>> Ulf Hansson (3):
>>>   mmc: core: Remove power_restore bus_ops for mmc and sd
>>>   mmc: core: Add bus_ops for runtime pm callbacks
>>>   mmc: block: Enable runtime pm for mmc blkdevice
>>>
>>>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>  drivers/mmc/core/bus.c   |   14 ++++++++++++--
>>>  drivers/mmc/core/core.h  |    2 ++
>>>  drivers/mmc/core/mmc.c   |   14 --------------
>>>  drivers/mmc/core/sd.c    |   14 --------------
>>>  drivers/mmc/core/sdio.c  |   20 ++++++++++++++++++++
>>>  6 files changed, 60 insertions(+), 32 deletions(-)
>>
>> This looks good to me, but I'd like to make sure the design works for
>> you before we decide to merge it.  Any concerns?
>>
>
> Chris,
>
> This patchset is ok for me.
> We can discuss/review how sd/mmc runtime callback function implemented
> later.
>
> Thanks
> Kevin
> --
> 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
>


-- 
Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Ulf Hansson]

On 28 March 2013 22:05,  <merez@codeaurora.org> wrote:
> Hi Chris,
>
> Sorry for the late response.
> I am currently on vacation and would get back to work next week.
> We would like to have an internal discussion to make sure there are no
> additional concerns with this patch.
> Can you please wait with this patch approval until the end of next week?

Hi Maya,

Hope you have had a good vacation. I was thinking of the BKOPS patches
I assume you are working on. If you need some assistance to "rebase"
you work on top of this patchset, just ping me.

Kind regards
Ulf Hansson

>
> Thanks,
> Maya
>
>>> -----Original Message-----
>>> From: Chris Ball [mailto:cjb@laptop.org]
>>> Sent: Wednesday, March 27, 2013 9:32 PM
>>> To: Ulf Hansson
>>> Cc: linux-mmc@vger.kernel.org; Johan Rudholm; Ulf Hansson; Maya Erez;
>>> Kevin Liu; Seungwon Jeon; jh80.chung@samsung.com
>>> Subject: Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>>
>>> Hi Maya, Kevin, Seungwon, Jaehoon,
>>>
>>> On Fri, Mar 01 2013, Ulf Hansson wrote:
>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> SDIO has been using runtime pm for a while to handle runtime power save
>>>> operations. This patchset is enabling the option to make the sd/mmc
>>>> blockdevices to use runtime pm as well.
>>>>
>>>> The runtime pm implementation for the block device will make use of
>>>> autosuspend to defer power save operation to after request inactivty
>>>> for
>>>> a certain time.
>>>>
>>>> To actually perform some power save operations the corresponding bus
>>>> ops
>>>> for mmc and sd shall be implemented. Typically it could make sense to
>>>> do
>>>> BKOPS for eMMC in here.
>>>>
>>>> Ulf Hansson (3):
>>>>   mmc: core: Remove power_restore bus_ops for mmc and sd
>>>>   mmc: core: Add bus_ops for runtime pm callbacks
>>>>   mmc: block: Enable runtime pm for mmc blkdevice
>>>>
>>>>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>  drivers/mmc/core/bus.c   |   14 ++++++++++++--
>>>>  drivers/mmc/core/core.h  |    2 ++
>>>>  drivers/mmc/core/mmc.c   |   14 --------------
>>>>  drivers/mmc/core/sd.c    |   14 --------------
>>>>  drivers/mmc/core/sdio.c  |   20 ++++++++++++++++++++
>>>>  6 files changed, 60 insertions(+), 32 deletions(-)
>>>
>>> This looks good to me, but I'd like to make sure the design works for
>>> you before we decide to merge it.  Any concerns?
>>>
>>
>> Chris,
>>
>> This patchset is ok for me.
>> We can discuss/review how sd/mmc runtime callback function implemented
>> later.
>>
>> Thanks
>> Kevin
>> --
>> 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
>>
>
>
> --
> Maya Erez
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>

Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice

[Maya Erez]

בתאריך 4/2/2013 1:45 PM, ציטוט Ulf Hansson:
> On 28 March 2013 22:05,  <merez@codeaurora.org> wrote:
>> Hi Chris,
>>
>> Sorry for the late response.
>> I am currently on vacation and would get back to work next week.
>> We would like to have an internal discussion to make sure there are no
>> additional concerns with this patch.
>> Can you please wait with this patch approval until the end of next week?
> Hi Maya,
>
> Hope you have had a good vacation. I was thinking of the BKOPS patches
> I assume you are working on. If you need some assistance to "rebase"
> you work on top of this patchset, just ping me.
>
> Kind regards
> Ulf Hansson
>
>> Thanks,
>> Maya
>>
>>>> -----Original Message-----
>>>> From: Chris Ball [mailto:cjb@laptop.org]
>>>> Sent: Wednesday, March 27, 2013 9:32 PM
>>>> To: Ulf Hansson
>>>> Cc: linux-mmc@vger.kernel.org; Johan Rudholm; Ulf Hansson; Maya Erez;
>>>> Kevin Liu; Seungwon Jeon; jh80.chung@samsung.com
>>>> Subject: Re: [PATCH 0/3] mmc: Use runtime pm for blkdevice
>>>>
>>>> Hi Maya, Kevin, Seungwon, Jaehoon,
>>>>
>>>> On Fri, Mar 01 2013, Ulf Hansson wrote:
>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>
>>>>> SDIO has been using runtime pm for a while to handle runtime power save
>>>>> operations. This patchset is enabling the option to make the sd/mmc
>>>>> blockdevices to use runtime pm as well.
>>>>>
>>>>> The runtime pm implementation for the block device will make use of
>>>>> autosuspend to defer power save operation to after request inactivty
>>>>> for
>>>>> a certain time.
>>>>>
>>>>> To actually perform some power save operations the corresponding bus
>>>>> ops
>>>>> for mmc and sd shall be implemented. Typically it could make sense to
>>>>> do
>>>>> BKOPS for eMMC in here.
>>>>>
>>>>> Ulf Hansson (3):
>>>>>    mmc: core: Remove power_restore bus_ops for mmc and sd
>>>>>    mmc: core: Add bus_ops for runtime pm callbacks
>>>>>    mmc: block: Enable runtime pm for mmc blkdevice
>>>>>
>>>>>   drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>>   drivers/mmc/core/bus.c   |   14 ++++++++++++--
>>>>>   drivers/mmc/core/core.h  |    2 ++
>>>>>   drivers/mmc/core/mmc.c   |   14 --------------
>>>>>   drivers/mmc/core/sd.c    |   14 --------------
>>>>>   drivers/mmc/core/sdio.c  |   20 ++++++++++++++++++++
>>>>>   6 files changed, 60 insertions(+), 32 deletions(-)
>>>> This looks good to me, but I'd like to make sure the design works for
>>>> you before we decide to merge it.  Any concerns?
>>>>
>>> Chris,
>>>
>>> This patchset is ok for me.
>>> We can discuss/review how sd/mmc runtime callback function implemented
>>> later.
>>>
>>> Thanks
>>> Kevin
>>> --
>>> 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
>>>
>>
>> --
>> Maya Erez
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
> --
> 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

Hi Ulf,

Thanks for your offer.

The current plan is to start the BKOPS in the runtime suspend API 
(triggerred 3 secs after idle time) although it may be less efficient 
than the current approach which starts the BKOPS after 200ms from idle time.
I will try to send the BKOPS patch next week.

Chris - we are OK with this patch, you can continue with its merge.

Thanks,
Maya

-- 
Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation