[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

[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

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

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