[PATCH/RFC] mmc: add a device PM QoS constraint when a host is first

[PATCH/RFC] mmc: add a device PM QoS constraint when a host is first

[Guennadi Liakhovetski]

Some MMC hosts implement a fine-grained runtime PM, whereby they
runtime-suspend and -resume the host interface on each transfer. This can
negatively affect performance, if the user was trying to transfer data
blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
throughput reduction. This constraint prevents runtime-suspending the
device, if the expected wakeup latency is larger than 100us.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

This patch has been tested on SuperH and ARM sh-mobile hosts with 
sh_mobile_sdhi and sh_mmcif drivers. Would be good to also have it tested 
on other platforms, enabling PM QoS, e.g., those, using the simple QoS 
PM domain governor.

 drivers/mmc/core/core.c  |   16 +++++++++++++++-
 include/linux/mmc/host.h |    2 ++
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b27b940..40f666f 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -22,6 +22,7 @@
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/suspend.h>
 
@@ -589,8 +590,16 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
 		wake_up(&host->wq);
 	spin_unlock_irqrestore(&host->lock, flags);
 	remove_wait_queue(&host->wq, &wait);
-	if (!stop)
+	if (!stop) {
+		if (host->claim_cnt = 1) {
+			int ret = dev_pm_qos_add_request(host->parent,
+							 &host->pm_qos, 100);
+			if (ret < 0)
+				dev_err(host->parent,
+					"PM QoS request failed: %d\n", ret);
+		}
 		mmc_host_enable(host);
+	}
 	return stop;
 }
 
@@ -615,6 +624,10 @@ int mmc_try_claim_host(struct mmc_host *host)
 		claimed_host = 1;
 	}
 	spin_unlock_irqrestore(&host->lock, flags);
+
+	if (claimed_host && host->claim_cnt = 1)
+		dev_pm_qos_add_request(host->parent, &host->pm_qos, 100);
+
 	return claimed_host;
 }
 EXPORT_SYMBOL(mmc_try_claim_host);
@@ -639,6 +652,7 @@ void mmc_do_release_host(struct mmc_host *host)
 		host->claimer = NULL;
 		spin_unlock_irqrestore(&host->lock, flags);
 		wake_up(&host->wq);
+		dev_pm_qos_remove_request(&host->pm_qos);
 	}
 }
 EXPORT_SYMBOL(mmc_do_release_host);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 1d09562..3e9d2f3 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -11,6 +11,7 @@
 #define LINUX_MMC_HOST_H
 
 #include <linux/leds.h>
+#include <linux/pm_qos.h>
 #include <linux/sched.h>
 
 #include <linux/mmc/core.h>
@@ -175,6 +176,7 @@ struct mmc_host {
 	u32			ocr_avail_sd;	/* SD-specific OCR */
 	u32			ocr_avail_mmc;	/* MMC-specific OCR */
 	struct notifier_block	pm_notify;
+	struct dev_pm_qos_request pm_qos;
 
 #define MMC_VDD_165_195		0x00000080	/* VDD voltage 1.65 - 1.95 */
 #define MMC_VDD_20_21		0x00000100	/* VDD voltage 2.0 ~ 2.1 */
-- 
1.7.2.5

[PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Guennadi Liakhovetski]

Some MMC hosts implement a fine-grained runtime PM, whereby they
runtime-suspend and -resume the host interface on each transfer. This can
negatively affect performance, if the user was trying to transfer data
blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
throughput reduction. This constraint prevents runtime-suspending the
device, if the expected wakeup latency is larger than 100us.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

This patch has been tested on SuperH and ARM sh-mobile hosts with 
sh_mobile_sdhi and sh_mmcif drivers. Would be good to also have it tested 
on other platforms, enabling PM QoS, e.g., those, using the simple QoS 
PM domain governor.

 drivers/mmc/core/core.c  |   16 +++++++++++++++-
 include/linux/mmc/host.h |    2 ++
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b27b940..40f666f 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -22,6 +22,7 @@
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/suspend.h>
 
@@ -589,8 +590,16 @@ int __mmc_claim_host(struct mmc_host *host, atomic_t *abort)
 		wake_up(&host->wq);
 	spin_unlock_irqrestore(&host->lock, flags);
 	remove_wait_queue(&host->wq, &wait);
-	if (!stop)
+	if (!stop) {
+		if (host->claim_cnt == 1) {
+			int ret = dev_pm_qos_add_request(host->parent,
+							 &host->pm_qos, 100);
+			if (ret < 0)
+				dev_err(host->parent,
+					"PM QoS request failed: %d\n", ret);
+		}
 		mmc_host_enable(host);
+	}
 	return stop;
 }
 
@@ -615,6 +624,10 @@ int mmc_try_claim_host(struct mmc_host *host)
 		claimed_host = 1;
 	}
 	spin_unlock_irqrestore(&host->lock, flags);
+
+	if (claimed_host && host->claim_cnt == 1)
+		dev_pm_qos_add_request(host->parent, &host->pm_qos, 100);
+
 	return claimed_host;
 }
 EXPORT_SYMBOL(mmc_try_claim_host);
@@ -639,6 +652,7 @@ void mmc_do_release_host(struct mmc_host *host)
 		host->claimer = NULL;
 		spin_unlock_irqrestore(&host->lock, flags);
 		wake_up(&host->wq);
+		dev_pm_qos_remove_request(&host->pm_qos);
 	}
 }
 EXPORT_SYMBOL(mmc_do_release_host);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 1d09562..3e9d2f3 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -11,6 +11,7 @@
 #define LINUX_MMC_HOST_H
 
 #include <linux/leds.h>
+#include <linux/pm_qos.h>
 #include <linux/sched.h>
 
 #include <linux/mmc/core.h>
@@ -175,6 +176,7 @@ struct mmc_host {
 	u32			ocr_avail_sd;	/* SD-specific OCR */
 	u32			ocr_avail_mmc;	/* MMC-specific OCR */
 	struct notifier_block	pm_notify;
+	struct dev_pm_qos_request pm_qos;
 
 #define MMC_VDD_165_195		0x00000080	/* VDD voltage 1.65 - 1.95 */
 #define MMC_VDD_20_21		0x00000100	/* VDD voltage 2.0 ~ 2.1 */
-- 
1.7.2.5

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is

[Ulf Hansson]

Guennadi Liakhovetski wrote:
> Some MMC hosts implement a fine-grained runtime PM, whereby they
> runtime-suspend and -resume the host interface on each transfer. This can
> negatively affect performance, if the user was trying to transfer data
> blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> throughput reduction. This constraint prevents runtime-suspending the
> device, if the expected wakeup latency is larger than 100us.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

I think host drivers can use autosuspend with some ms delay for this 
instead. This will mean that requests coming in bursts will not be 
affected (well only the first request in the burst will suffer from the 
runtime resume latency).

I believe that runtime resume callback should ofcourse be optimized so 
they are executed as fast as possible. But moreover, if they take more 
100us, is that really a reason for not executing them at all?

Br
Ulf Hansson

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Ulf Hansson]

Guennadi Liakhovetski wrote:
> Some MMC hosts implement a fine-grained runtime PM, whereby they
> runtime-suspend and -resume the host interface on each transfer. This can
> negatively affect performance, if the user was trying to transfer data
> blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> throughput reduction. This constraint prevents runtime-suspending the
> device, if the expected wakeup latency is larger than 100us.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

I think host drivers can use autosuspend with some ms delay for this 
instead. This will mean that requests coming in bursts will not be 
affected (well only the first request in the burst will suffer from the 
runtime resume latency).

I believe that runtime resume callback should ofcourse be optimized so 
they are executed as fast as possible. But moreover, if they take more 
100us, is that really a reason for not executing them at all?

Br
Ulf Hansson

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is

[Guennadi Liakhovetski]

Hi Ulf

On Tue, 13 Dec 2011, Ulf Hansson wrote:

> Guennadi Liakhovetski wrote:
> > Some MMC hosts implement a fine-grained runtime PM, whereby they
> > runtime-suspend and -resume the host interface on each transfer. This can
> > negatively affect performance, if the user was trying to transfer data
> > blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> > throughput reduction. This constraint prevents runtime-suspending the
> > device, if the expected wakeup latency is larger than 100us.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> I think host drivers can use autosuspend with some ms delay for this instead.
> This will mean that requests coming in bursts will not be affected (well only
> the first request in the burst will suffer from the runtime resume latency).

I think, Rafael is the best person to explain, why exactly this is not 
desired. In short, this is the wrong location to make such decisions and 
to define these criteria. The only thing, that the driver may be aware of 
is how quickly it wants to be able to wake up, if it got suspended. And 
it's already the PM subsystem, that has to decide, whether it can satisfy 
this requirement or not. Rafael will correct me, if my explanation is 
wrong.

> I believe that runtime resume callback should ofcourse be optimized so they
> are executed as fast as possible. But moreover, if they take more 100us, is
> that really a reason for not executing them at all?

I think it is a reason not to execute them during an intensive IO, yes. I 
cannot imagine a case, where if you have multiple IO requests waiting in 
the queue to your medium, you would want to switch off and immediately on 
again. Well, of course, such situations might exist, but then you just 
have to define and use a different governor on your system. This is also 
the flexibility, that this API is giving you.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Guennadi Liakhovetski]

Hi Ulf

On Tue, 13 Dec 2011, Ulf Hansson wrote:

> Guennadi Liakhovetski wrote:
> > Some MMC hosts implement a fine-grained runtime PM, whereby they
> > runtime-suspend and -resume the host interface on each transfer. This can
> > negatively affect performance, if the user was trying to transfer data
> > blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> > throughput reduction. This constraint prevents runtime-suspending the
> > device, if the expected wakeup latency is larger than 100us.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> I think host drivers can use autosuspend with some ms delay for this instead.
> This will mean that requests coming in bursts will not be affected (well only
> the first request in the burst will suffer from the runtime resume latency).

I think, Rafael is the best person to explain, why exactly this is not 
desired. In short, this is the wrong location to make such decisions and 
to define these criteria. The only thing, that the driver may be aware of 
is how quickly it wants to be able to wake up, if it got suspended. And 
it's already the PM subsystem, that has to decide, whether it can satisfy 
this requirement or not. Rafael will correct me, if my explanation is 
wrong.

> I believe that runtime resume callback should ofcourse be optimized so they
> are executed as fast as possible. But moreover, if they take more 100us, is
> that really a reason for not executing them at all?

I think it is a reason not to execute them during an intensive IO, yes. I 
cannot imagine a case, where if you have multiple IO requests waiting in 
the queue to your medium, you would want to switch off and immediately on 
again. Well, of course, such situations might exist, but then you just 
have to define and use a different governor on your system. This is also 
the flexibility, that this API is giving you.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

On Tuesday, December 13, 2011, Guennadi Liakhovetski wrote:
> Hi Ulf
> 
> On Tue, 13 Dec 2011, Ulf Hansson wrote:
> 
> > Guennadi Liakhovetski wrote:
> > > Some MMC hosts implement a fine-grained runtime PM, whereby they
> > > runtime-suspend and -resume the host interface on each transfer. This can
> > > negatively affect performance, if the user was trying to transfer data
> > > blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> > > throughput reduction. This constraint prevents runtime-suspending the
> > > device, if the expected wakeup latency is larger than 100us.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > I think host drivers can use autosuspend with some ms delay for this instead.
> > This will mean that requests coming in bursts will not be affected (well only
> > the first request in the burst will suffer from the runtime resume latency).
> 
> I think, Rafael is the best person to explain, why exactly this is not 
> desired. In short, this is the wrong location to make such decisions and 
> to define these criteria. The only thing, that the driver may be aware of 
> is how quickly it wants to be able to wake up, if it got suspended. And 
> it's already the PM subsystem, that has to decide, whether it can satisfy 
> this requirement or not. Rafael will correct me, if my explanation is 
> wrong.

It is correct.  More specifically, the problem is, for example, that there
may be two different low-power states of the system that may be entered
when the device is suspended, one of which is relatively shallow, but
having a low exit latency, while the other one is deep with a correspondingly
longer exit latency.  This happens when the device is a member of a power
domain that may be turned off entirely when it is suspended under certain
conditions and at the same time the device's individual clock may be turned
off and on almost instantaneously.  In that case there's no reason to avoid
suspending the device whenever possible, allowing its clock to be turned
off to save energy, but there may be a good reason to avoid turning off the
power domain due to latency constraints.  No amount of playing with
autosuspends is going to help here, unless the driver is aware of the exact
system hardware configuration it is in, which is rather impractical if you
think of universal drivers (such that may be used with different platforms,
for example).

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

On Tuesday, December 13, 2011, Guennadi Liakhovetski wrote:
> Hi Ulf
> 
> On Tue, 13 Dec 2011, Ulf Hansson wrote:
> 
> > Guennadi Liakhovetski wrote:
> > > Some MMC hosts implement a fine-grained runtime PM, whereby they
> > > runtime-suspend and -resume the host interface on each transfer. This can
> > > negatively affect performance, if the user was trying to transfer data
> > > blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> > > throughput reduction. This constraint prevents runtime-suspending the
> > > device, if the expected wakeup latency is larger than 100us.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > I think host drivers can use autosuspend with some ms delay for this instead.
> > This will mean that requests coming in bursts will not be affected (well only
> > the first request in the burst will suffer from the runtime resume latency).
> 
> I think, Rafael is the best person to explain, why exactly this is not 
> desired. In short, this is the wrong location to make such decisions and 
> to define these criteria. The only thing, that the driver may be aware of 
> is how quickly it wants to be able to wake up, if it got suspended. And 
> it's already the PM subsystem, that has to decide, whether it can satisfy 
> this requirement or not. Rafael will correct me, if my explanation is 
> wrong.

It is correct.  More specifically, the problem is, for example, that there
may be two different low-power states of the system that may be entered
when the device is suspended, one of which is relatively shallow, but
having a low exit latency, while the other one is deep with a correspondingly
longer exit latency.  This happens when the device is a member of a power
domain that may be turned off entirely when it is suspended under certain
conditions and at the same time the device's individual clock may be turned
off and on almost instantaneously.  In that case there's no reason to avoid
suspending the device whenever possible, allowing its clock to be turned
off to save energy, but there may be a good reason to avoid turning off the
power domain due to latency constraints.  No amount of playing with
autosuspends is going to help here, unless the driver is aware of the exact
system hardware configuration it is in, which is rather impractical if you
think of universal drivers (such that may be used with different platforms,
for example).

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is

[Ulf Hansson]

Guennadi Liakhovetski wrote:
> Hi Ulf
> 
> On Tue, 13 Dec 2011, Ulf Hansson wrote:
> 
>> Guennadi Liakhovetski wrote:
>>> Some MMC hosts implement a fine-grained runtime PM, whereby they
>>> runtime-suspend and -resume the host interface on each transfer. This can
>>> negatively affect performance, if the user was trying to transfer data
>>> blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
>>> throughput reduction. This constraint prevents runtime-suspending the
>>> device, if the expected wakeup latency is larger than 100us.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> I think host drivers can use autosuspend with some ms delay for this instead.
>> This will mean that requests coming in bursts will not be affected (well only
>> the first request in the burst will suffer from the runtime resume latency).
> 
> I think, Rafael is the best person to explain, why exactly this is not 
> desired. In short, this is the wrong location to make such decisions and 
> to define these criteria. The only thing, that the driver may be aware of 
> is how quickly it wants to be able to wake up, if it got suspended. And 
> it's already the PM subsystem, that has to decide, whether it can satisfy 
> this requirement or not. Rafael will correct me, if my explanation is 
> wrong.

You have a point. But I am not convinced. :-)

Some host drivers already make use of autosuspend. I think this is most 
straightforward solution to this problem right now.

However, we could also do pm_runtime_get_sync of the host device in 
claim host and vice verse in release host, thus preventing the host 
driver from triggering runtime_suspend|resume for every request. Though, 
I am not 100% sure this is really what you want either.

Using PM QoS as you propose, might prevent some hosts from doing 
runtime_suspend|resume completely and thus those might not fulfill power 
consumption requirements instead. I do not think we can take this 
decision at this level. Is performance more important than power save, 
that is kind of the question.


> 
>> I believe that runtime resume callback should ofcourse be optimized so they
>> are executed as fast as possible. But moreover, if they take more 100us, is
>> that really a reason for not executing them at all?
> 
> I think it is a reason not to execute them during an intensive IO, yes. I 
> cannot imagine a case, where if you have multiple IO requests waiting in 
> the queue to your medium, you would want to switch off and immediately on 
> again. Well, of course, such situations might exist, but then you just 
> have to define and use a different governor on your system. This is also 
> the flexibility, that this API is giving you.

I totally agree.

> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 


Br
Ulf Hansson

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Ulf Hansson]

Guennadi Liakhovetski wrote:
> Hi Ulf
> 
> On Tue, 13 Dec 2011, Ulf Hansson wrote:
> 
>> Guennadi Liakhovetski wrote:
>>> Some MMC hosts implement a fine-grained runtime PM, whereby they
>>> runtime-suspend and -resume the host interface on each transfer. This can
>>> negatively affect performance, if the user was trying to transfer data
>>> blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
>>> throughput reduction. This constraint prevents runtime-suspending the
>>> device, if the expected wakeup latency is larger than 100us.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> I think host drivers can use autosuspend with some ms delay for this instead.
>> This will mean that requests coming in bursts will not be affected (well only
>> the first request in the burst will suffer from the runtime resume latency).
> 
> I think, Rafael is the best person to explain, why exactly this is not 
> desired. In short, this is the wrong location to make such decisions and 
> to define these criteria. The only thing, that the driver may be aware of 
> is how quickly it wants to be able to wake up, if it got suspended. And 
> it's already the PM subsystem, that has to decide, whether it can satisfy 
> this requirement or not. Rafael will correct me, if my explanation is 
> wrong.

You have a point. But I am not convinced. :-)

Some host drivers already make use of autosuspend. I think this is most 
straightforward solution to this problem right now.

However, we could also do pm_runtime_get_sync of the host device in 
claim host and vice verse in release host, thus preventing the host 
driver from triggering runtime_suspend|resume for every request. Though, 
I am not 100% sure this is really what you want either.

Using PM QoS as you propose, might prevent some hosts from doing 
runtime_suspend|resume completely and thus those might not fulfill power 
consumption requirements instead. I do not think we can take this 
decision at this level. Is performance more important than power save, 
that is kind of the question.


> 
>> I believe that runtime resume callback should ofcourse be optimized so they
>> are executed as fast as possible. But moreover, if they take more 100us, is
>> that really a reason for not executing them at all?
> 
> I think it is a reason not to execute them during an intensive IO, yes. I 
> cannot imagine a case, where if you have multiple IO requests waiting in 
> the queue to your medium, you would want to switch off and immediately on 
> again. Well, of course, such situations might exist, but then you just 
> have to define and use a different governor on your system. This is also 
> the flexibility, that this API is giving you.

I totally agree.

> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 


Br
Ulf Hansson

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is

[Linus Walleij]

On Wed, Dec 14, 2011 at 10:00 AM, Ulf Hansson
<ulf.hansson@stericsson.com> wrote:
> Guennadi Liakhovetski wrote:

> Using PM QoS as you propose, might prevent some hosts from doing
> runtime_suspend|resume completely and thus those might not fulfill power
> consumption requirements instead. I do not think we can take this decision
> at this level. Is performance more important than power save, that is kind
> of the question.

I agree with this point. The problematic part of the patch (IMHO) is this:

>> + This constraint prevents runtime-suspending the
>> + device, if the expected wakeup latency is larger than 100us.
(...)
>> +                       int ret = dev_pm_qos_add_request(host->parent,
>> +                                                        &host->pm_qos, 100);

So we hardcode 100us (is that really 100us by the way? I cannot
follow this code path but usually these figures are in ms, but what
do I know) as the in-between back-to-back transfers.
But this delta is dependent on a lot of stuff that only the platform
knows, like nominal CPU frequency, bus speed etc, so certainly the
platform must be able to modify that number.

At the very least, please make this stuff optional using Kconfig
so it can be shut off, because I fear it will screw up our PM usecases.

Ulfs patch to the mmci driver actually use 50ms for back-to-back
intergap between any two hardware-affecting calls into the driver.

Yours,
Linus Walleij

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Linus Walleij]

On Wed, Dec 14, 2011 at 10:00 AM, Ulf Hansson
<ulf.hansson@stericsson.com> wrote:
> Guennadi Liakhovetski wrote:

> Using PM QoS as you propose, might prevent some hosts from doing
> runtime_suspend|resume completely and thus those might not fulfill power
> consumption requirements instead. I do not think we can take this decision
> at this level. Is performance more important than power save, that is kind
> of the question.

I agree with this point. The problematic part of the patch (IMHO) is this:

>> + This constraint prevents runtime-suspending the
>> + device, if the expected wakeup latency is larger than 100us.
(...)
>> +                       int ret = dev_pm_qos_add_request(host->parent,
>> +                                                        &host->pm_qos, 100);

So we hardcode 100us (is that really 100us by the way? I cannot
follow this code path but usually these figures are in ms, but what
do I know) as the in-between back-to-back transfers.
But this delta is dependent on a lot of stuff that only the platform
knows, like nominal CPU frequency, bus speed etc, so certainly the
platform must be able to modify that number.

At the very least, please make this stuff optional using Kconfig
so it can be shut off, because I fear it will screw up our PM usecases.

Ulfs patch to the mmci driver actually use 50ms for back-to-back
intergap between any two hardware-affecting calls into the driver.

Yours,
Linus Walleij

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

On Wednesday, December 14, 2011, Ulf Hansson wrote:
> Guennadi Liakhovetski wrote:
> > Hi Ulf
> > 
> > On Tue, 13 Dec 2011, Ulf Hansson wrote:
> > 
> >> Guennadi Liakhovetski wrote:
> >>> Some MMC hosts implement a fine-grained runtime PM, whereby they
> >>> runtime-suspend and -resume the host interface on each transfer. This can
> >>> negatively affect performance, if the user was trying to transfer data
> >>> blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> >>> throughput reduction. This constraint prevents runtime-suspending the
> >>> device, if the expected wakeup latency is larger than 100us.
> >>>
> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> I think host drivers can use autosuspend with some ms delay for this instead.
> >> This will mean that requests coming in bursts will not be affected (well only
> >> the first request in the burst will suffer from the runtime resume latency).
> > 
> > I think, Rafael is the best person to explain, why exactly this is not 
> > desired. In short, this is the wrong location to make such decisions and 
> > to define these criteria. The only thing, that the driver may be aware of 
> > is how quickly it wants to be able to wake up, if it got suspended. And 
> > it's already the PM subsystem, that has to decide, whether it can satisfy 
> > this requirement or not. Rafael will correct me, if my explanation is 
> > wrong.
> 
> You have a point. But I am not convinced. :-)
> 
> Some host drivers already make use of autosuspend. I think this is most 
> straightforward solution to this problem right now.

The problem is not about _when_ to suspend (which autosuspend is about),
but _what_ _state_ to go when suspended.  That's quite a different issue.

> However, we could also do pm_runtime_get_sync of the host device in 
> claim host and vice verse in release host, thus preventing the host 
> driver from triggering runtime_suspend|resume for every request. Though, 
> I am not 100% sure this is really what you want either.

No, I don't want that.  I want the device to be suspended when possible,
but I don't want that to cause the system to go into an overly deep power
state as a result.

> Using PM QoS as you propose, might prevent some hosts from doing 
> runtime_suspend|resume completely and thus those might not fulfill power 
> consumption requirements instead.

I'm not sure what scenario you have in mind.  Care to elaborate?

> I do not think we can take this decision at this level. Is performance more
> important than power save, that is kind of the question.

Yes, it is.  Also, the number used here is somewhat arbitrary.

However, since no one except for SH7372 is now using device PM QoS, no one
else will be affected by this change at the moment.

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

On Wednesday, December 14, 2011, Ulf Hansson wrote:
> Guennadi Liakhovetski wrote:
> > Hi Ulf
> > 
> > On Tue, 13 Dec 2011, Ulf Hansson wrote:
> > 
> >> Guennadi Liakhovetski wrote:
> >>> Some MMC hosts implement a fine-grained runtime PM, whereby they
> >>> runtime-suspend and -resume the host interface on each transfer. This can
> >>> negatively affect performance, if the user was trying to transfer data
> >>> blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> >>> throughput reduction. This constraint prevents runtime-suspending the
> >>> device, if the expected wakeup latency is larger than 100us.
> >>>
> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> I think host drivers can use autosuspend with some ms delay for this instead.
> >> This will mean that requests coming in bursts will not be affected (well only
> >> the first request in the burst will suffer from the runtime resume latency).
> > 
> > I think, Rafael is the best person to explain, why exactly this is not 
> > desired. In short, this is the wrong location to make such decisions and 
> > to define these criteria. The only thing, that the driver may be aware of 
> > is how quickly it wants to be able to wake up, if it got suspended. And 
> > it's already the PM subsystem, that has to decide, whether it can satisfy 
> > this requirement or not. Rafael will correct me, if my explanation is 
> > wrong.
> 
> You have a point. But I am not convinced. :-)
> 
> Some host drivers already make use of autosuspend. I think this is most 
> straightforward solution to this problem right now.

The problem is not about _when_ to suspend (which autosuspend is about),
but _what_ _state_ to go when suspended.  That's quite a different issue.

> However, we could also do pm_runtime_get_sync of the host device in 
> claim host and vice verse in release host, thus preventing the host 
> driver from triggering runtime_suspend|resume for every request. Though, 
> I am not 100% sure this is really what you want either.

No, I don't want that.  I want the device to be suspended when possible,
but I don't want that to cause the system to go into an overly deep power
state as a result.

> Using PM QoS as you propose, might prevent some hosts from doing 
> runtime_suspend|resume completely and thus those might not fulfill power 
> consumption requirements instead.

I'm not sure what scenario you have in mind.  Care to elaborate?

> I do not think we can take this decision at this level. Is performance more
> important than power save, that is kind of the question.

Yes, it is.  Also, the number used here is somewhat arbitrary.

However, since no one except for SH7372 is now using device PM QoS, no one
else will be affected by this change at the moment.

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

On Wednesday, December 14, 2011, Linus Walleij wrote:
> On Wed, Dec 14, 2011 at 10:00 AM, Ulf Hansson
> <ulf.hansson@stericsson.com> wrote:
> > Guennadi Liakhovetski wrote:
> 
> > Using PM QoS as you propose, might prevent some hosts from doing
> > runtime_suspend|resume completely and thus those might not fulfill power
> > consumption requirements instead. I do not think we can take this decision
> > at this level. Is performance more important than power save, that is kind
> > of the question.
> 
> I agree with this point. The problematic part of the patch (IMHO) is this:
> 
> >> + This constraint prevents runtime-suspending the
> >> + device, if the expected wakeup latency is larger than 100us.
> (...)
> >> +                       int ret = dev_pm_qos_add_request(host->parent,
> >> +                                                        &host->pm_qos, 100);
> 
> So we hardcode 100us (is that really 100us by the way? I cannot
> follow this code path but usually these figures are in ms, but what
> do I know) as the in-between back-to-back transfers.

They are in microseconds.

> But this delta is dependent on a lot of stuff that only the platform
> knows, like nominal CPU frequency, bus speed etc, so certainly the
> platform must be able to modify that number.

You seem to be confusing things.  The exact meaning of this number is:
"I may want to use the device 100 us from now (but not earlier), so please
make it possible to do that".  [It roughly means "don't put the device into
a low-power state that takes more than 100 us to resume from", but it's a bit
more complicated than that.]  It doesn't mean "don't suspend the device for
the next 100 us".

> At the very least, please make this stuff optional using Kconfig
> so it can be shut off, because I fear it will screw up our PM usecases.

I'm not sure how that's possible, at least until your platform starts
to use device PM QoS.

> Ulfs patch to the mmci driver actually use 50ms for back-to-back
> intergap between any two hardware-affecting calls into the driver.

Which is kind of independent.

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

On Wednesday, December 14, 2011, Linus Walleij wrote:
> On Wed, Dec 14, 2011 at 10:00 AM, Ulf Hansson
> <ulf.hansson@stericsson.com> wrote:
> > Guennadi Liakhovetski wrote:
> 
> > Using PM QoS as you propose, might prevent some hosts from doing
> > runtime_suspend|resume completely and thus those might not fulfill power
> > consumption requirements instead. I do not think we can take this decision
> > at this level. Is performance more important than power save, that is kind
> > of the question.
> 
> I agree with this point. The problematic part of the patch (IMHO) is this:
> 
> >> + This constraint prevents runtime-suspending the
> >> + device, if the expected wakeup latency is larger than 100us.
> (...)
> >> +                       int ret = dev_pm_qos_add_request(host->parent,
> >> +                                                        &host->pm_qos, 100);
> 
> So we hardcode 100us (is that really 100us by the way? I cannot
> follow this code path but usually these figures are in ms, but what
> do I know) as the in-between back-to-back transfers.

They are in microseconds.

> But this delta is dependent on a lot of stuff that only the platform
> knows, like nominal CPU frequency, bus speed etc, so certainly the
> platform must be able to modify that number.

You seem to be confusing things.  The exact meaning of this number is:
"I may want to use the device 100 us from now (but not earlier), so please
make it possible to do that".  [It roughly means "don't put the device into
a low-power state that takes more than 100 us to resume from", but it's a bit
more complicated than that.]  It doesn't mean "don't suspend the device for
the next 100 us".

> At the very least, please make this stuff optional using Kconfig
> so it can be shut off, because I fear it will screw up our PM usecases.

I'm not sure how that's possible, at least until your platform starts
to use device PM QoS.

> Ulfs patch to the mmci driver actually use 50ms for back-to-back
> intergap between any two hardware-affecting calls into the driver.

Which is kind of independent.

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is

[Ulf Hansson]

>> You have a point. But I am not convinced. :-)
>>
>> Some host drivers already make use of autosuspend. I think this is most 
>> straightforward solution to this problem right now.
> 
> The problem is not about _when_ to suspend (which autosuspend is about),
> but _what_ _state_ to go when suspended.  That's quite a different issue.

I was kind of taking a more simple approach, I were considering 
runtime_suspend as _one_ state. Right now there is no host driver having 
different levels of runtime_suspend state, if I am correct. But ofcourse 
that could be the future.

Moreover, I definitely do not think that a fixed timeout of 100us is 
applicable for all use cases, this must at least be configurable.

> 
>> However, we could also do pm_runtime_get_sync of the host device in 
>> claim host and vice verse in release host, thus preventing the host 
>> driver from triggering runtime_suspend|resume for every request. Though, 
>> I am not 100% sure this is really what you want either.
> 
> No, I don't want that.  I want the device to be suspended when possible,
> but I don't want that to cause the system to go into an overly deep power
> state as a result.

Before just skipping my proposal, I think you should know some more 
background to why I suggested this:

1.
mmc_claim_host is using mmc_host_enable, which kind of mean the same 
thing for a host driver as doing get_sync. Vice verse for mmc_release_host.

2.
When executing mmc/sd commands/requests the host must always be claimed 
(and thus the host is always enabled). But more important some mmc/sd 
commands must be executed as a sequence, without the host being disabled 
in between the commands (since a disable might mean that the clock to 
card gets disabled). To solve this, the mmc_claim_host is used, to make 
sure the host is enabled during the complete command sequence.

I happily continue this discussion, to see if someone more can break the 
idea about having get_sync in mmc_host_enable. :-)

> 
>> Using PM QoS as you propose, might prevent some hosts from doing 
>> runtime_suspend|resume completely and thus those might not fulfill power 
>> consumption requirements instead.
> 
> I'm not sure what scenario you have in mind.  Care to elaborate?

Well, suppose a the host drivers start considering the PM QoS 
requirement, but never can fulfill the requirement of 100us for it's 
runtime_suspend callback function...

> 
>> I do not think we can take this decision at this level. Is performance more
>> important than power save, that is kind of the question.
> 
> Yes, it is.  Also, the number used here is somewhat arbitrary.

For you maybe power management is less important, but I doubt everybody 
else agree to that. It is a more complex balance I believe.

> 
> However, since no one except for SH7372 is now using device PM QoS, no one
> else will be affected by this change at the moment.

True, but that is not a good reason for adding more stuff to the mmc core.

> 
> Thanks,
> Rafael
> 

By the way, I like to discuss with you so please take no offense from my 
comments. :-)


BR
Ulf Hansson

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Ulf Hansson]

>> You have a point. But I am not convinced. :-)
>>
>> Some host drivers already make use of autosuspend. I think this is most 
>> straightforward solution to this problem right now.
> 
> The problem is not about _when_ to suspend (which autosuspend is about),
> but _what_ _state_ to go when suspended.  That's quite a different issue.

I was kind of taking a more simple approach, I were considering 
runtime_suspend as _one_ state. Right now there is no host driver having 
different levels of runtime_suspend state, if I am correct. But ofcourse 
that could be the future.

Moreover, I definitely do not think that a fixed timeout of 100us is 
applicable for all use cases, this must at least be configurable.

> 
>> However, we could also do pm_runtime_get_sync of the host device in 
>> claim host and vice verse in release host, thus preventing the host 
>> driver from triggering runtime_suspend|resume for every request. Though, 
>> I am not 100% sure this is really what you want either.
> 
> No, I don't want that.  I want the device to be suspended when possible,
> but I don't want that to cause the system to go into an overly deep power
> state as a result.

Before just skipping my proposal, I think you should know some more 
background to why I suggested this:

1.
mmc_claim_host is using mmc_host_enable, which kind of mean the same 
thing for a host driver as doing get_sync. Vice verse for mmc_release_host.

2.
When executing mmc/sd commands/requests the host must always be claimed 
(and thus the host is always enabled). But more important some mmc/sd 
commands must be executed as a sequence, without the host being disabled 
in between the commands (since a disable might mean that the clock to 
card gets disabled). To solve this, the mmc_claim_host is used, to make 
sure the host is enabled during the complete command sequence.

I happily continue this discussion, to see if someone more can break the 
idea about having get_sync in mmc_host_enable. :-)

> 
>> Using PM QoS as you propose, might prevent some hosts from doing 
>> runtime_suspend|resume completely and thus those might not fulfill power 
>> consumption requirements instead.
> 
> I'm not sure what scenario you have in mind.  Care to elaborate?

Well, suppose a the host drivers start considering the PM QoS 
requirement, but never can fulfill the requirement of 100us for it's 
runtime_suspend callback function...

> 
>> I do not think we can take this decision at this level. Is performance more
>> important than power save, that is kind of the question.
> 
> Yes, it is.  Also, the number used here is somewhat arbitrary.

For you maybe power management is less important, but I doubt everybody 
else agree to that. It is a more complex balance I believe.

> 
> However, since no one except for SH7372 is now using device PM QoS, no one
> else will be affected by this change at the moment.

True, but that is not a good reason for adding more stuff to the mmc core.

> 
> Thanks,
> Rafael
> 

By the way, I like to discuss with you so please take no offense from my 
comments. :-)


BR
Ulf Hansson

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is

[Linus Walleij]

On Wed, Dec 14, 2011 at 11:28 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, December 14, 2011, Linus Walleij wrote:

>> But this delta is dependent on a lot of stuff that only the platform
>> knows, like nominal CPU frequency, bus speed etc, so certainly the
>> platform must be able to modify that number.
>
> You seem to be confusing things.  The exact meaning of this number is:
> "I may want to use the device 100 us from now (but not earlier), so please
> make it possible to do that".  [It roughly means "don't put the device into
> a low-power state that takes more than 100 us to resume from", but it's a bit
> more complicated than that.]  It doesn't mean "don't suspend the device for
> the next 100 us".

Yes you're right, I get it now!

Thanks for the explanation Rafael.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Linus Walleij]

On Wed, Dec 14, 2011 at 11:28 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, December 14, 2011, Linus Walleij wrote:

>> But this delta is dependent on a lot of stuff that only the platform
>> knows, like nominal CPU frequency, bus speed etc, so certainly the
>> platform must be able to modify that number.
>
> You seem to be confusing things.  The exact meaning of this number is:
> "I may want to use the device 100 us from now (but not earlier), so please
> make it possible to do that".  [It roughly means "don't put the device into
> a low-power state that takes more than 100 us to resume from", but it's a bit
> more complicated than that.]  It doesn't mean "don't suspend the device for
> the next 100 us".

Yes you're right, I get it now!

Thanks for the explanation Rafael.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

On Wednesday, December 14, 2011, Ulf Hansson wrote:
> 
> >> You have a point. But I am not convinced. :-)
> >>
> >> Some host drivers already make use of autosuspend. I think this is most 
> >> straightforward solution to this problem right now.
> > 
> > The problem is not about _when_ to suspend (which autosuspend is about),
> > but _what_ _state_ to go when suspended.  That's quite a different issue.
> 
> I was kind of taking a more simple approach, I were considering 
> runtime_suspend as _one_ state. Right now there is no host driver having 
> different levels of runtime_suspend state, if I am correct. But ofcourse 
> that could be the future.

Yes, there is, or more precisely there's going to be shortly.  We have
PM domains on SH7372 and when we enable all of them to be powered off
at run time, there will be many power states for the controller which
is located in one of them.  Hence the $subject patch. :-)

> Moreover, I definitely do not think that a fixed timeout of 100us is 
> applicable for all use cases, this must at least be configurable.

Well, this isn't a timeout.  Have you read my reply to Linus?

I agree that it should be configurable _eventually_, but no one seems to
know how to implement that configurability.  Ideally, that value should
result from some user space input, possibly after a conversion by the
driver to a useful number, but we don't seem to have any agreement as to
what the interface for passing that user space input to the driver should
look like.

> >> However, we could also do pm_runtime_get_sync of the host device in 
> >> claim host and vice verse in release host, thus preventing the host 
> >> driver from triggering runtime_suspend|resume for every request. Though, 
> >> I am not 100% sure this is really what you want either.
> > 
> > No, I don't want that.  I want the device to be suspended when possible,
> > but I don't want that to cause the system to go into an overly deep power
> > state as a result.
> 
> Before just skipping my proposal, I think you should know some more 
> background to why I suggested this:
> 
> 1.
> mmc_claim_host is using mmc_host_enable, which kind of mean the same 
> thing for a host driver as doing get_sync. Vice verse for mmc_release_host.
> 
> 2.
> When executing mmc/sd commands/requests the host must always be claimed 
> (and thus the host is always enabled). But more important some mmc/sd 
> commands must be executed as a sequence, without the host being disabled 
> in between the commands (since a disable might mean that the clock to 
> card gets disabled). To solve this, the mmc_claim_host is used, to make 
> sure the host is enabled during the complete command sequence.
> 
> I happily continue this discussion, to see if someone more can break the 
> idea about having get_sync in mmc_host_enable. :-)

I'll leave this one to Guennadi, if you don't mind. :-)

> >> Using PM QoS as you propose, might prevent some hosts from doing 
> >> runtime_suspend|resume completely and thus those might not fulfill power 
> >> consumption requirements instead.
> > 
> > I'm not sure what scenario you have in mind.  Care to elaborate?
> 
> Well, suppose a the host drivers start considering the PM QoS 
> requirement, but never can fulfill the requirement of 100us for it's 
> runtime_suspend callback function...

OK, that's a valid concern.  This means there should be a way for user space
to specify the constraint somehow, because it's a user space's role to define
policies.

> >> I do not think we can take this decision at this level. Is performance more
> >> important than power save, that is kind of the question.
> > 
> > Yes, it is.  Also, the number used here is somewhat arbitrary.
> 
> For you maybe power management is less important, but I doubt everybody 
> else agree to that. It is a more complex balance I believe.

You're right.

> > However, since no one except for SH7372 is now using device PM QoS, no one
> > else will be affected by this change at the moment.
> 
> True, but that is not a good reason for adding more stuff to the mmc core.

Good point.

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

On Wednesday, December 14, 2011, Ulf Hansson wrote:
> 
> >> You have a point. But I am not convinced. :-)
> >>
> >> Some host drivers already make use of autosuspend. I think this is most 
> >> straightforward solution to this problem right now.
> > 
> > The problem is not about _when_ to suspend (which autosuspend is about),
> > but _what_ _state_ to go when suspended.  That's quite a different issue.
> 
> I was kind of taking a more simple approach, I were considering 
> runtime_suspend as _one_ state. Right now there is no host driver having 
> different levels of runtime_suspend state, if I am correct. But ofcourse 
> that could be the future.

Yes, there is, or more precisely there's going to be shortly.  We have
PM domains on SH7372 and when we enable all of them to be powered off
at run time, there will be many power states for the controller which
is located in one of them.  Hence the $subject patch. :-)

> Moreover, I definitely do not think that a fixed timeout of 100us is 
> applicable for all use cases, this must at least be configurable.

Well, this isn't a timeout.  Have you read my reply to Linus?

I agree that it should be configurable _eventually_, but no one seems to
know how to implement that configurability.  Ideally, that value should
result from some user space input, possibly after a conversion by the
driver to a useful number, but we don't seem to have any agreement as to
what the interface for passing that user space input to the driver should
look like.

> >> However, we could also do pm_runtime_get_sync of the host device in 
> >> claim host and vice verse in release host, thus preventing the host 
> >> driver from triggering runtime_suspend|resume for every request. Though, 
> >> I am not 100% sure this is really what you want either.
> > 
> > No, I don't want that.  I want the device to be suspended when possible,
> > but I don't want that to cause the system to go into an overly deep power
> > state as a result.
> 
> Before just skipping my proposal, I think you should know some more 
> background to why I suggested this:
> 
> 1.
> mmc_claim_host is using mmc_host_enable, which kind of mean the same 
> thing for a host driver as doing get_sync. Vice verse for mmc_release_host.
> 
> 2.
> When executing mmc/sd commands/requests the host must always be claimed 
> (and thus the host is always enabled). But more important some mmc/sd 
> commands must be executed as a sequence, without the host being disabled 
> in between the commands (since a disable might mean that the clock to 
> card gets disabled). To solve this, the mmc_claim_host is used, to make 
> sure the host is enabled during the complete command sequence.
> 
> I happily continue this discussion, to see if someone more can break the 
> idea about having get_sync in mmc_host_enable. :-)

I'll leave this one to Guennadi, if you don't mind. :-)

> >> Using PM QoS as you propose, might prevent some hosts from doing 
> >> runtime_suspend|resume completely and thus those might not fulfill power 
> >> consumption requirements instead.
> > 
> > I'm not sure what scenario you have in mind.  Care to elaborate?
> 
> Well, suppose a the host drivers start considering the PM QoS 
> requirement, but never can fulfill the requirement of 100us for it's 
> runtime_suspend callback function...

OK, that's a valid concern.  This means there should be a way for user space
to specify the constraint somehow, because it's a user space's role to define
policies.

> >> I do not think we can take this decision at this level. Is performance more
> >> important than power save, that is kind of the question.
> > 
> > Yes, it is.  Also, the number used here is somewhat arbitrary.
> 
> For you maybe power management is less important, but I doubt everybody 
> else agree to that. It is a more complex balance I believe.

You're right.

> > However, since no one except for SH7372 is now using device PM QoS, no one
> > else will be affected by this change at the moment.
> 
> True, but that is not a good reason for adding more stuff to the mmc core.

Good point.

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is

[Guennadi Liakhovetski]

On Wed, 14 Dec 2011, Rafael J. Wysocki wrote:

> On Wednesday, December 14, 2011, Ulf Hansson wrote:

[snip]

> > 2.
> > When executing mmc/sd commands/requests the host must always be claimed 
> > (and thus the host is always enabled).

Why? Why cannot we save some power between IO operations - if we can do 
this quickly and safely without affecting functionality and throughput?

> > But more important some mmc/sd 
> > commands must be executed as a sequence, without the host being disabled 
> > in between the commands (since a disable might mean that the clock to 
> > card gets disabled).

Ok, there might well be such command sequences, but my question is: who 
knows about them? Is this mandated by the MMC(/SD/SDIO/...) standard or is 
it host-specific? Also "might mean" is actually interesting here. I think 
we eventually need a combination of timing-oriented PM constraints and 
"stateful" ones. During such a command sequence you would require the card 
clock to stay on.

> > To solve this, the mmc_claim_host is used, to make 
> > sure the host is enabled during the complete command sequence.
> > 
> > I happily continue this discussion, to see if someone more can break the 
> > idea about having get_sync in mmc_host_enable. :-)
> 
> I'll leave this one to Guennadi, if you don't mind. :-)

As I said above, I think, we need both - to be able to require a certain 
responsiveness / throughput and specific interface parameters like 
supplying clock, power, etc.

Also notice, that setting a constraint doesn't affect in principle, when 
the device is allowed to suspend. This is done as usual per 
pm_runtime_get*() and _put*(). I think, a reasonable solution to use PM 
QoS to impose timing constraints but at the same time to not disable the 
hardware, when this is disallowed, is to tie pm_runtime_get() and _put() 
calls to driver's .set_ios() method, like tmio_mmc and sh_mmcif drivers 
currently do. Those drivers only call pm_runtime_put() when the interface 
clock is gated. So, as long as the core is aware, that that IO sequence 
has to run uninterrupted without stopping the clock between single 
transfers, it just has to avoid gating the clock for that period and the 
interface will not enter any power-saving mode.

So, I don't think we need to enforce pm_runtime_get_sync() in 
mmc_claim_host().

Thanks
Guennadi

> > >> Using PM QoS as you propose, might prevent some hosts from doing 
> > >> runtime_suspend|resume completely and thus those might not fulfill power 
> > >> consumption requirements instead.
> > > 
> > > I'm not sure what scenario you have in mind.  Care to elaborate?
> > 
> > Well, suppose a the host drivers start considering the PM QoS 
> > requirement, but never can fulfill the requirement of 100us for it's 
> > runtime_suspend callback function...
> 
> OK, that's a valid concern.  This means there should be a way for user space
> to specify the constraint somehow, because it's a user space's role to define
> policies.
> 
> > >> I do not think we can take this decision at this level. Is performance more
> > >> important than power save, that is kind of the question.
> > > 
> > > Yes, it is.  Also, the number used here is somewhat arbitrary.
> > 
> > For you maybe power management is less important, but I doubt everybody 
> > else agree to that. It is a more complex balance I believe.
> 
> You're right.
> 
> > > However, since no one except for SH7372 is now using device PM QoS, no one
> > > else will be affected by this change at the moment.
> > 
> > True, but that is not a good reason for adding more stuff to the mmc core.
> 
> Good point.
> 
> Thanks,
> Rafael
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Guennadi Liakhovetski]

On Wed, 14 Dec 2011, Rafael J. Wysocki wrote:

> On Wednesday, December 14, 2011, Ulf Hansson wrote:

[snip]

> > 2.
> > When executing mmc/sd commands/requests the host must always be claimed 
> > (and thus the host is always enabled).

Why? Why cannot we save some power between IO operations - if we can do 
this quickly and safely without affecting functionality and throughput?

> > But more important some mmc/sd 
> > commands must be executed as a sequence, without the host being disabled 
> > in between the commands (since a disable might mean that the clock to 
> > card gets disabled).

Ok, there might well be such command sequences, but my question is: who 
knows about them? Is this mandated by the MMC(/SD/SDIO/...) standard or is 
it host-specific? Also "might mean" is actually interesting here. I think 
we eventually need a combination of timing-oriented PM constraints and 
"stateful" ones. During such a command sequence you would require the card 
clock to stay on.

> > To solve this, the mmc_claim_host is used, to make 
> > sure the host is enabled during the complete command sequence.
> > 
> > I happily continue this discussion, to see if someone more can break the 
> > idea about having get_sync in mmc_host_enable. :-)
> 
> I'll leave this one to Guennadi, if you don't mind. :-)

As I said above, I think, we need both - to be able to require a certain 
responsiveness / throughput and specific interface parameters like 
supplying clock, power, etc.

Also notice, that setting a constraint doesn't affect in principle, when 
the device is allowed to suspend. This is done as usual per 
pm_runtime_get*() and _put*(). I think, a reasonable solution to use PM 
QoS to impose timing constraints but at the same time to not disable the 
hardware, when this is disallowed, is to tie pm_runtime_get() and _put() 
calls to driver's .set_ios() method, like tmio_mmc and sh_mmcif drivers 
currently do. Those drivers only call pm_runtime_put() when the interface 
clock is gated. So, as long as the core is aware, that that IO sequence 
has to run uninterrupted without stopping the clock between single 
transfers, it just has to avoid gating the clock for that period and the 
interface will not enter any power-saving mode.

So, I don't think we need to enforce pm_runtime_get_sync() in 
mmc_claim_host().

Thanks
Guennadi

> > >> Using PM QoS as you propose, might prevent some hosts from doing 
> > >> runtime_suspend|resume completely and thus those might not fulfill power 
> > >> consumption requirements instead.
> > > 
> > > I'm not sure what scenario you have in mind.  Care to elaborate?
> > 
> > Well, suppose a the host drivers start considering the PM QoS 
> > requirement, but never can fulfill the requirement of 100us for it's 
> > runtime_suspend callback function...
> 
> OK, that's a valid concern.  This means there should be a way for user space
> to specify the constraint somehow, because it's a user space's role to define
> policies.
> 
> > >> I do not think we can take this decision at this level. Is performance more
> > >> important than power save, that is kind of the question.
> > > 
> > > Yes, it is.  Also, the number used here is somewhat arbitrary.
> > 
> > For you maybe power management is less important, but I doubt everybody 
> > else agree to that. It is a more complex balance I believe.
> 
> You're right.
> 
> > > However, since no one except for SH7372 is now using device PM QoS, no one
> > > else will be affected by this change at the moment.
> > 
> > True, but that is not a good reason for adding more stuff to the mmc core.
> 
> Good point.
> 
> Thanks,
> Rafael
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is

[Ulf Hansson]

Guennadi Liakhovetski wrote:
> On Wed, 14 Dec 2011, Rafael J. Wysocki wrote:
> 
>> On Wednesday, December 14, 2011, Ulf Hansson wrote:
> 
> [snip]
> 
>>> 2.
>>> When executing mmc/sd commands/requests the host must always be claimed 
>>> (and thus the host is always enabled).
> 
> Why? Why cannot we save some power between IO operations - if we can do 
> this quickly and safely without affecting functionality and throughput?
> 
>>> But more important some mmc/sd 
>>> commands must be executed as a sequence, without the host being disabled 
>>> in between the commands (since a disable might mean that the clock to 
>>> card gets disabled).
> 
> Ok, there might well be such command sequences, but my question is: who 
> knows about them? Is this mandated by the MMC(/SD/SDIO/...) standard or is 
> it host-specific? Also "might mean" is actually interesting here. I think 
> we eventually need a combination of timing-oriented PM constraints and 
> "stateful" ones. During such a command sequence you would require the card 
> clock to stay on.

MMC/SD/SDIO standard sets the requirement and must somehow "notify" the 
host about how to act in certain scenarios.

Just for information, I think one interesting host driver to look at is 
omap_hsmmc. For it's enable function is does pm_runtime_get_sync and for 
it's disable function it does pm_runtime_put_autosuspend, this to make 
sure the clock is enabled when it is needed.


> 
>>> To solve this, the mmc_claim_host is used, to make 
>>> sure the host is enabled during the complete command sequence.
>>>
>>> I happily continue this discussion, to see if someone more can break the 
>>> idea about having get_sync in mmc_host_enable. :-)
>> I'll leave this one to Guennadi, if you don't mind. :-)
> 
> As I said above, I think, we need both - to be able to require a certain 
> responsiveness / throughput and specific interface parameters like 
> supplying clock, power, etc.
> 
> Also notice, that setting a constraint doesn't affect in principle, when 
> the device is allowed to suspend. This is done as usual per 
> pm_runtime_get*() and _put*(). I think, a reasonable solution to use PM 
> QoS to impose timing constraints but at the same time to not disable the 
> hardware, when this is disallowed, is to tie pm_runtime_get() and _put() 
> calls to driver's .set_ios() method, like tmio_mmc and sh_mmcif drivers 
> currently do. Those drivers only call pm_runtime_put() when the interface 
> clock is gated. So, as long as the core is aware, that that IO sequence 
> has to run uninterrupted without stopping the clock between single 
> transfers, it just has to avoid gating the clock for that period and the 
> interface will not enter any power-saving mode.
> 
> So, I don't think we need to enforce pm_runtime_get_sync() in 
> mmc_claim_host().

You might be right; but still we need a way for the mmc framework to 
prevent the host driver from disabling clock etc to the card for some 
command sequences. As the omap driver solves it is to implement the 
enable/disable function and then call for get_sync and put in these 
functions, maybe that is the way how to solve this!?

Br
Ulf Hansson

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Ulf Hansson]

Guennadi Liakhovetski wrote:
> On Wed, 14 Dec 2011, Rafael J. Wysocki wrote:
> 
>> On Wednesday, December 14, 2011, Ulf Hansson wrote:
> 
> [snip]
> 
>>> 2.
>>> When executing mmc/sd commands/requests the host must always be claimed 
>>> (and thus the host is always enabled).
> 
> Why? Why cannot we save some power between IO operations - if we can do 
> this quickly and safely without affecting functionality and throughput?
> 
>>> But more important some mmc/sd 
>>> commands must be executed as a sequence, without the host being disabled 
>>> in between the commands (since a disable might mean that the clock to 
>>> card gets disabled).
> 
> Ok, there might well be such command sequences, but my question is: who 
> knows about them? Is this mandated by the MMC(/SD/SDIO/...) standard or is 
> it host-specific? Also "might mean" is actually interesting here. I think 
> we eventually need a combination of timing-oriented PM constraints and 
> "stateful" ones. During such a command sequence you would require the card 
> clock to stay on.

MMC/SD/SDIO standard sets the requirement and must somehow "notify" the 
host about how to act in certain scenarios.

Just for information, I think one interesting host driver to look at is 
omap_hsmmc. For it's enable function is does pm_runtime_get_sync and for 
it's disable function it does pm_runtime_put_autosuspend, this to make 
sure the clock is enabled when it is needed.


> 
>>> To solve this, the mmc_claim_host is used, to make 
>>> sure the host is enabled during the complete command sequence.
>>>
>>> I happily continue this discussion, to see if someone more can break the 
>>> idea about having get_sync in mmc_host_enable. :-)
>> I'll leave this one to Guennadi, if you don't mind. :-)
> 
> As I said above, I think, we need both - to be able to require a certain 
> responsiveness / throughput and specific interface parameters like 
> supplying clock, power, etc.
> 
> Also notice, that setting a constraint doesn't affect in principle, when 
> the device is allowed to suspend. This is done as usual per 
> pm_runtime_get*() and _put*(). I think, a reasonable solution to use PM 
> QoS to impose timing constraints but at the same time to not disable the 
> hardware, when this is disallowed, is to tie pm_runtime_get() and _put() 
> calls to driver's .set_ios() method, like tmio_mmc and sh_mmcif drivers 
> currently do. Those drivers only call pm_runtime_put() when the interface 
> clock is gated. So, as long as the core is aware, that that IO sequence 
> has to run uninterrupted without stopping the clock between single 
> transfers, it just has to avoid gating the clock for that period and the 
> interface will not enter any power-saving mode.
> 
> So, I don't think we need to enforce pm_runtime_get_sync() in 
> mmc_claim_host().

You might be right; but still we need a way for the mmc framework to 
prevent the host driver from disabling clock etc to the card for some 
command sequences. As the omap driver solves it is to implement the 
enable/disable function and then call for get_sync and put in these 
functions, maybe that is the way how to solve this!?

Br
Ulf Hansson

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

Hi,

Sorry for the late respones.

On Monday, December 19, 2011, Ulf Hansson wrote:
> Guennadi Liakhovetski wrote:
> > On Wed, 14 Dec 2011, Rafael J. Wysocki wrote:
> > 
> >> On Wednesday, December 14, 2011, Ulf Hansson wrote:
> > 
> > [snip]
> > 
> >>> 2.
> >>> When executing mmc/sd commands/requests the host must always be claimed 
> >>> (and thus the host is always enabled).
> > 
> > Why? Why cannot we save some power between IO operations - if we can do 
> > this quickly and safely without affecting functionality and throughput?
> > 
> >>> But more important some mmc/sd 
> >>> commands must be executed as a sequence, without the host being disabled 
> >>> in between the commands (since a disable might mean that the clock to 
> >>> card gets disabled).
> > 
> > Ok, there might well be such command sequences, but my question is: who 
> > knows about them? Is this mandated by the MMC(/SD/SDIO/...) standard or is 
> > it host-specific? Also "might mean" is actually interesting here. I think 
> > we eventually need a combination of timing-oriented PM constraints and 
> > "stateful" ones. During such a command sequence you would require the card 
> > clock to stay on.
> 
> MMC/SD/SDIO standard sets the requirement and must somehow "notify" the 
> host about how to act in certain scenarios.
> 
> Just for information, I think one interesting host driver to look at is 
> omap_hsmmc. For it's enable function is does pm_runtime_get_sync and for 
> it's disable function it does pm_runtime_put_autosuspend, this to make 
> sure the clock is enabled when it is needed.

That's OK, but that doesn't solve the issue the $subject patch tries to
address.

> >>> To solve this, the mmc_claim_host is used, to make 
> >>> sure the host is enabled during the complete command sequence.
> >>>
> >>> I happily continue this discussion, to see if someone more can break the 
> >>> idea about having get_sync in mmc_host_enable. :-)
> >> I'll leave this one to Guennadi, if you don't mind. :-)
> > 
> > As I said above, I think, we need both - to be able to require a certain 
> > responsiveness / throughput and specific interface parameters like 
> > supplying clock, power, etc.
> > 
> > Also notice, that setting a constraint doesn't affect in principle, when 
> > the device is allowed to suspend. This is done as usual per 
> > pm_runtime_get*() and _put*(). I think, a reasonable solution to use PM 
> > QoS to impose timing constraints but at the same time to not disable the 
> > hardware, when this is disallowed, is to tie pm_runtime_get() and _put() 
> > calls to driver's .set_ios() method, like tmio_mmc and sh_mmcif drivers 
> > currently do. Those drivers only call pm_runtime_put() when the interface 
> > clock is gated. So, as long as the core is aware, that that IO sequence 
> > has to run uninterrupted without stopping the clock between single 
> > transfers, it just has to avoid gating the clock for that period and the 
> > interface will not enter any power-saving mode.
> > 
> > So, I don't think we need to enforce pm_runtime_get_sync() in 
> > mmc_claim_host().
> 
> You might be right; but still we need a way for the mmc framework to 
> prevent the host driver from disabling clock etc to the card for some 
> command sequences. As the omap driver solves it is to implement the 
> enable/disable function and then call for get_sync and put in these 
> functions, maybe that is the way how to solve this!?

You still seem to be confusing preventing the clock from being disabled
at all (which may be necessary for some command sequences etc.) with preventing
the host adapter from being put into an overly deep low-power state _after_
_the_ _clock_ _has_ _been_ _disabled_.  They are two _different_ issues
and there's no way we can address them both by using timeouts.

With PM domains support on SH7372 we can easily have a situation in which
the controller's runtime PM causes a whole domain to go off (or in a more
complicated scenario, the runtime PM of some _other_ device in the same PM
domain causes it to go off while the MMC controller is in the "suspended"
state) and that leads to an overly long delay in the MMC subsystem.  So,
we need to address this issue _in_ _addition_ to ensuring that the controller
is _not_ suspended when it shouldn't be.

Thanks,
Rafael

Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed

[Rafael J. Wysocki]

Hi,

Sorry for the late respones.

On Monday, December 19, 2011, Ulf Hansson wrote:
> Guennadi Liakhovetski wrote:
> > On Wed, 14 Dec 2011, Rafael J. Wysocki wrote:
> > 
> >> On Wednesday, December 14, 2011, Ulf Hansson wrote:
> > 
> > [snip]
> > 
> >>> 2.
> >>> When executing mmc/sd commands/requests the host must always be claimed 
> >>> (and thus the host is always enabled).
> > 
> > Why? Why cannot we save some power between IO operations - if we can do 
> > this quickly and safely without affecting functionality and throughput?
> > 
> >>> But more important some mmc/sd 
> >>> commands must be executed as a sequence, without the host being disabled 
> >>> in between the commands (since a disable might mean that the clock to 
> >>> card gets disabled).
> > 
> > Ok, there might well be such command sequences, but my question is: who 
> > knows about them? Is this mandated by the MMC(/SD/SDIO/...) standard or is 
> > it host-specific? Also "might mean" is actually interesting here. I think 
> > we eventually need a combination of timing-oriented PM constraints and 
> > "stateful" ones. During such a command sequence you would require the card 
> > clock to stay on.
> 
> MMC/SD/SDIO standard sets the requirement and must somehow "notify" the 
> host about how to act in certain scenarios.
> 
> Just for information, I think one interesting host driver to look at is 
> omap_hsmmc. For it's enable function is does pm_runtime_get_sync and for 
> it's disable function it does pm_runtime_put_autosuspend, this to make 
> sure the clock is enabled when it is needed.

That's OK, but that doesn't solve the issue the $subject patch tries to
address.

> >>> To solve this, the mmc_claim_host is used, to make 
> >>> sure the host is enabled during the complete command sequence.
> >>>
> >>> I happily continue this discussion, to see if someone more can break the 
> >>> idea about having get_sync in mmc_host_enable. :-)
> >> I'll leave this one to Guennadi, if you don't mind. :-)
> > 
> > As I said above, I think, we need both - to be able to require a certain 
> > responsiveness / throughput and specific interface parameters like 
> > supplying clock, power, etc.
> > 
> > Also notice, that setting a constraint doesn't affect in principle, when 
> > the device is allowed to suspend. This is done as usual per 
> > pm_runtime_get*() and _put*(). I think, a reasonable solution to use PM 
> > QoS to impose timing constraints but at the same time to not disable the 
> > hardware, when this is disallowed, is to tie pm_runtime_get() and _put() 
> > calls to driver's .set_ios() method, like tmio_mmc and sh_mmcif drivers 
> > currently do. Those drivers only call pm_runtime_put() when the interface 
> > clock is gated. So, as long as the core is aware, that that IO sequence 
> > has to run uninterrupted without stopping the clock between single 
> > transfers, it just has to avoid gating the clock for that period and the 
> > interface will not enter any power-saving mode.
> > 
> > So, I don't think we need to enforce pm_runtime_get_sync() in 
> > mmc_claim_host().
> 
> You might be right; but still we need a way for the mmc framework to 
> prevent the host driver from disabling clock etc to the card for some 
> command sequences. As the omap driver solves it is to implement the 
> enable/disable function and then call for get_sync and put in these 
> functions, maybe that is the way how to solve this!?

You still seem to be confusing preventing the clock from being disabled
at all (which may be necessary for some command sequences etc.) with preventing
the host adapter from being put into an overly deep low-power state _after_
_the_ _clock_ _has_ _been_ _disabled_.  They are two _different_ issues
and there's no way we can address them both by using timeouts.

With PM domains support on SH7372 we can easily have a situation in which
the controller's runtime PM causes a whole domain to go off (or in a more
complicated scenario, the runtime PM of some _other_ device in the same PM
domain causes it to go off while the MMC controller is in the "suspended"
state) and that leads to an overly long delay in the MMC subsystem.  So,
we need to address this issue _in_ _addition_ to ensuring that the controller
is _not_ suspended when it shouldn't be.

Thanks,
Rafael