[PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

Hi all,

The goal of this patchset is to allow user space to control the
responsiveness of the MMC stack related to runtime power management.

Namely, on systems that contain power domains, the time necessary
to bring an MMC host up after it has been runtime-suspended may
depend not only on the MMC core and host driver, but also on the platform
and other devices in the same power domain(s) that contain(s) the MMC
host.  Although that obviously may influence the MMC performance,
currently, there is no way to control it through the MMC subsystem.
Moreover, the only available way to control it at all is by using PM QoS
latency requests, so it is necessary to add some kind of support for that
to MMC.

Patch [1/3] adds PM QoS-related fields to struct mmc_host and introduces
a new sysfs attribute for MMC hosts, pm_latency_limit_ms, allowing user
space to influence the MMC host's runtime PM via PM QoS.  Whether or not
this attribute will be created (and PM QoS will be used for the given host)
depends on the host driver, so host drivers that don't (plan) to support
PM QoS don't need to do anything about that.

Patches [2/3] and [3/3] add the corresponding host driver bits to the
tmio_mmc and sh_mmcif drivers, respectively.

Thanks,
Rafael

[PATCH 1/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

A runtime suspend of an MMC controller belonging to a power domain
or, in a more complicated scenario, a runtime suspend of another
device in the same power domain, may cause power to be removed from
the entire domain.  In that case, the amount of time necessary to
runtime-resume the MMC controller is often substantially greater
than the time needed to enable its clock.  That may hurt performance
in some situations, because user data may need to wait for the
controller to become operational, so we should make it possible to
prevent that from happening.

For this reason, introduce a new sysfs attribute for MMC hosts,
pm_latency_limit_ms, allowing user space to specify the upper bound
of the time necessary to bring the host up after it has been
runtime-suspended.  However, make it appear ony for the hosts whose
drivers declare support for PM QoS by populating the pm_qos_req
member of struct mmc_host before registering the host.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/mmc/mmc-dev-attrs.txt |   22 +++++++++++
 drivers/mmc/core/host.c             |   67 ++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h            |    5 ++
 3 files changed, 94 insertions(+)

Index: linux/drivers/mmc/core/host.c
===================================================================
--- linux.orig/drivers/mmc/core/host.c
+++ linux/drivers/mmc/core/host.c
@@ -293,6 +293,68 @@ static inline void mmc_host_clk_sysfs_in
 
 #endif
 
+static ssize_t pm_qos_val_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+	return snprintf(buf, PAGE_SIZE, "%d\n", host->pm_qos_val);
+}
+
+static ssize_t pm_qos_val_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+	s32 value;
+	int ret;
+
+	if (kstrtos32(buf, 0, &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	host->pm_qos_val = value;
+	ret = dev_pm_qos_update_request(host->pm_qos_req, host->pm_qos_val);
+	return ret < 0 ? ret : count;
+}
+
+static void mmc_host_pm_qos_init(struct mmc_host *host)
+{
+	int ret;
+
+	if (!host->pm_qos_req)
+		return;
+
+	ret = dev_pm_qos_add_request(host->parent, host->pm_qos_req,
+				     host->pm_qos_val);
+	if (ret < 0) {
+		dev_err(host->parent, "Unable to use PM QoS: %d\n", ret);
+		return;
+	}
+
+	host->pm_qos_val_attr.show = pm_qos_val_show;
+	host->pm_qos_val_attr.store = pm_qos_val_store;
+	sysfs_attr_init(&host->pm_qos_val_attr.attr);
+	host->pm_qos_val_attr.attr.name = "pm_latency_limit_ms";
+	host->pm_qos_val_attr.attr.mode = S_IRUGO | S_IWUSR;
+	if (device_create_file(&host->class_dev, &host->pm_qos_val_attr)) {
+		pr_err("%s: Failed to create PM latency limit sysfs entry\n",
+				mmc_hostname(host));
+		host->pm_qos_val_attr.attr.name = NULL;
+	}
+}
+
+static void mmc_host_pm_qos_exit(struct mmc_host *host)
+{
+	if (!host->pm_qos_req)
+		return;
+
+	if (host->pm_qos_val_attr.attr.name)
+		device_remove_file(&host->class_dev, &host->pm_qos_val_attr);
+
+	dev_pm_qos_remove_request(host->pm_qos_req);
+}
+
 /**
  *	mmc_alloc_host - initialise the per-host structure.
  *	@extra: sizeof private data structure
@@ -381,6 +443,8 @@ int mmc_add_host(struct mmc_host *host)
 #endif
 	mmc_host_clk_sysfs_init(host);
 
+	mmc_host_pm_qos_init(host);
+
 	mmc_start_host(host);
 	register_pm_notifier(&host->pm_notify);
 
@@ -400,8 +464,11 @@ EXPORT_SYMBOL(mmc_add_host);
 void mmc_remove_host(struct mmc_host *host)
 {
 	unregister_pm_notifier(&host->pm_notify);
+
 	mmc_stop_host(host);
 
+	mmc_host_pm_qos_exit(host);
+
 #ifdef CONFIG_DEBUG_FS
 	mmc_remove_host_debugfs(host);
 #endif
Index: linux/include/linux/mmc/host.h
===================================================================
--- linux.orig/include/linux/mmc/host.h
+++ linux/include/linux/mmc/host.h
@@ -13,6 +13,7 @@
 #include <linux/leds.h>
 #include <linux/sched.h>
 #include <linux/fault-inject.h>
+#include <linux/pm_qos.h>
 
 #include <linux/mmc/core.h>
 #include <linux/mmc/pm.h>
@@ -191,6 +192,10 @@ struct mmc_host {
 	u32			ocr_avail_mmc;	/* MMC-specific OCR */
 	struct notifier_block	pm_notify;
 
+	struct dev_pm_qos_request	*pm_qos_req;
+	s32				pm_qos_val;
+	struct device_attribute		pm_qos_val_attr;
+
 #define MMC_VDD_165_195		0x00000080	/* VDD voltage 1.65 - 1.95 */
 #define MMC_VDD_20_21		0x00000100	/* VDD voltage 2.0 ~ 2.1 */
 #define MMC_VDD_21_22		0x00000200	/* VDD voltage 2.1 ~ 2.2 */
Index: linux/Documentation/mmc/mmc-dev-attrs.txt
===================================================================
--- linux.orig/Documentation/mmc/mmc-dev-attrs.txt
+++ linux/Documentation/mmc/mmc-dev-attrs.txt
@@ -74,3 +74,25 @@ This attribute appears only if CONFIG_MM
 	clkgate_delay	Tune the clock gating delay with desired value in milliseconds.
 
 echo <desired delay> > /sys/class/mmc_host/mmcX/clkgate_delay
+
+MMC PM QoS Support
+==================
+
+If the MMC host driver supports PM QoS, it should populate the pm_qos_req member
+of struct mmc_host before registering the host.  In that case, read and write
+access if provided to the following attribute:
+
+	pm_latency_limit_ms	Specify the upper bound of the time necessary
+				to bring the host up after it has been
+				runtime-suspended (in milliseconds).
+
+echo <desired latency limit> > /sys/class/mmc_host/mmcX/pm_latency_limit_ms
+
+The precise meaning of this attribute is "whenever the host is runtime-suspended,
+make sure that it will be possible to bring it up within the given time" (e.g.
+if the limit is set to 10 ms, it should always be possible to make the host
+operational at most 10 ms after it has been runtime-suspended).  Note, however,
+that if the limit is set too small, the only physically possible way to respect
+it may be to keep the host permanently operational (i.e. never suspend it).
+
+If 0 is written to pm_latency_limit_ms, it means that there is no limit.

Re: [PATCH 1/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Linus Walleij]

On Sun, Mar 4, 2012 at 1:02 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:

> A runtime suspend of an MMC controller belonging to a power domain
> or, in a more complicated scenario, a runtime suspend of another
> device in the same power domain, may cause power to be removed from
> the entire domain.  In that case, the amount of time necessary to
> runtime-resume the MMC controller is often substantially greater
> than the time needed to enable its clock.  That may hurt performance
> in some situations, because user data may need to wait for the
> controller to become operational, so we should make it possible to
> prevent that from happening.
>
> For this reason, introduce a new sysfs attribute for MMC hosts,
> pm_latency_limit_ms, allowing user space to specify the upper bound
> of the time necessary to bring the host up after it has been
> runtime-suspended.  However, make it appear ony for the hosts whose
> drivers declare support for PM QoS by populating the pm_qos_req
> member of struct mmc_host before registering the host.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Overall I've already ACKed this I think, looking good...
Acked-by: Linus Walleij <linus.walleij@linaro.org>

> @@ -191,6 +192,10 @@ struct mmc_host {
>        u32                     ocr_avail_mmc;  /* MMC-specific OCR */
>        struct notifier_block   pm_notify;
>
> +       struct dev_pm_qos_request       *pm_qos_req;
> +       s32                             pm_qos_val;
> +       struct device_attribute         pm_qos_val_attr;
> +
>  #define MMC_VDD_165_195                0x00000080      /* VDD voltage 1.65 - 1.95 */
>  #define MMC_VDD_20_21          0x00000100      /* VDD voltage 2.0 ~ 2.1 */
>  #define MMC_VDD_21_22          0x00000200      /* VDD voltage 2.1 ~ 2.2 */

As you can see from the members around there we try to document
the members whenever the meaning is non-trivial.

So pm_qos_val could need
/* QoS maximum power-on delay in ms */

(The entire struct host should of course be converted to use kerneldoc
but right now the pattern is like that.)

> Index: linux/Documentation/mmc/mmc-dev-attrs.txt

Well documented!

Yours,
Linus Walleij

Re: [PATCH 1/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

On Sunday, March 04, 2012, Linus Walleij wrote:
> On Sun, Mar 4, 2012 at 1:02 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > A runtime suspend of an MMC controller belonging to a power domain
> > or, in a more complicated scenario, a runtime suspend of another
> > device in the same power domain, may cause power to be removed from
> > the entire domain.  In that case, the amount of time necessary to
> > runtime-resume the MMC controller is often substantially greater
> > than the time needed to enable its clock.  That may hurt performance
> > in some situations, because user data may need to wait for the
> > controller to become operational, so we should make it possible to
> > prevent that from happening.
> >
> > For this reason, introduce a new sysfs attribute for MMC hosts,
> > pm_latency_limit_ms, allowing user space to specify the upper bound
> > of the time necessary to bring the host up after it has been
> > runtime-suspended.  However, make it appear ony for the hosts whose
> > drivers declare support for PM QoS by populating the pm_qos_req
> > member of struct mmc_host before registering the host.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Overall I've already ACKed this I think, looking good...
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

I have a new version, though, (will be sent in a while), but hopefully
your ACK will still apply. :-)

Rafael

[PATCH 2/3] tmio_mmc / PM: Use PM QoS requests

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make tmio_mmc populate the pm_qos_req member of struct mmc_host, to
let the core know that it should create the pm_latency_limit_ms
host attribute for it, and set the defaul value of that attribute
to 100 ms.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/mmc/host/tmio_mmc_pio.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux/drivers/mmc/host/tmio_mmc_pio.c
===================================================================
--- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
+++ linux/drivers/mmc/host/tmio_mmc_pio.c
@@ -918,6 +918,12 @@ int __devinit tmio_mmc_host_probe(struct
 	if (ret < 0)
 		goto pm_disable;
 
+	mmc->pm_qos_req = kzalloc(sizeof(*mmc->pm_qos_req), GFP_KERNEL);
+	if (mmc->pm_qos_req)
+		mmc->pm_qos_val = 100;
+	else
+		dev_err(&pdev->dev, "Not enough memory for PM QoS.\n");
+
 	/*
 	 * There are 4 different scenarios for the card detection:
 	 *  1) an external gpio irq handles the cd (best for power savings)
@@ -998,6 +1004,8 @@ void tmio_mmc_host_remove(struct tmio_mm
 	cancel_delayed_work_sync(&host->delayed_reset_work);
 	tmio_mmc_release_dma(host);
 
+	kfree(host->mmc->pm_qos_req);
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 

[PATCH 3/3] sh_mmcif / PM: Use PM QoS requests

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make sh_mmcif populate the pm_qos_req member of struct mmc_host, to
let the core know that it should create the pm_latency_limit_ms
host attribute for it, and set the defaul value of that attribute
to 100 ms.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/mmc/host/sh_mmcif.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux/drivers/mmc/host/sh_mmcif.c
===================================================================
--- linux.orig/drivers/mmc/host/sh_mmcif.c
+++ linux/drivers/mmc/host/sh_mmcif.c
@@ -57,6 +57,7 @@
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/module.h>
 
@@ -1327,6 +1328,12 @@ static int __devinit sh_mmcif_probe(stru
 	if (ret < 0)
 		goto clean_up2;
 
+	mmc->pm_qos_req = kzalloc(sizeof(*mmc->pm_qos_req), GFP_KERNEL);
+	if (mmc->pm_qos_req)
+		mmc->pm_qos_val = 100;
+	else
+		dev_err(&pdev->dev, "Not enough memory for PM QoS.\n");
+
 	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
 
 	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
@@ -1397,6 +1404,8 @@ static int __devexit sh_mmcif_remove(str
 
 	platform_set_drvdata(pdev, NULL);
 
+	kfree(host->mmc->pm_qos_req);
+
 	clk_disable(host->hclk);
 	mmc_free_host(host->mmc);
 	pm_runtime_put_sync(&pdev->dev);

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

Hi all,

On Sunday, March 04, 2012, Rafael J. Wysocki wrote:
> Hi all,
> 
> The goal of this patchset is to allow user space to control the
> responsiveness of the MMC stack related to runtime power management.
> 
> Namely, on systems that contain power domains, the time necessary
> to bring an MMC host up after it has been runtime-suspended may
> depend not only on the MMC core and host driver, but also on the platform
> and other devices in the same power domain(s) that contain(s) the MMC
> host.  Although that obviously may influence the MMC performance,
> currently, there is no way to control it through the MMC subsystem.
> Moreover, the only available way to control it at all is by using PM QoS
> latency requests, so it is necessary to add some kind of support for that
> to MMC.
> 
> Patch [1/3] adds PM QoS-related fields to struct mmc_host and introduces
> a new sysfs attribute for MMC hosts, pm_latency_limit_ms, allowing user
> space to influence the MMC host's runtime PM via PM QoS.  Whether or not
> this attribute will be created (and PM QoS will be used for the given host)
> depends on the host driver, so host drivers that don't (plan) to support
> PM QoS don't need to do anything about that.
> 
> Patches [2/3] and [3/3] add the corresponding host driver bits to the
> tmio_mmc and sh_mmcif drivers, respectively.

After posting the patches I noticed that the changelog of patch [1/3] and
the documentation of the new sysfs attribute were not 100% accurate, because
the PM QoS request really applies to the time between a resume request and
the moment the device is operational again and not the time from runtime
suspend (the latter would imply some kind of autoresume mechanism, which isn't
there).

I also thought it would be cleaner to allocate the val and attr fields along
with the request, because in the previous version of the patchset they weren't
used when req was NULL, resulting in (a little) wasted memory.

Updated patches follow.

Thanks,
Rafael

[PATCH 1/3] MMC / PM: Make it possible to use PM QoS latency constraints, v2

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

A runtime suspend of an MMC controller belonging to a power domain
or, in a more complicated scenario, a runtime suspend of another
device in the same power domain, may cause power to be removed from
the entire domain.  In that case, the amount of time necessary to
runtime-resume the MMC controller is often substantially greater
than the time needed to enable its clock.  That may hurt performance
in some situations, because user data may need to wait for the
controller to become operational, so we should make it possible to
prevent that from happening.

For this reason, introduce a new sysfs attribute for MMC hosts,
pm_latency_limit_ms, allowing user space to specify the upper bound
of the time necessary to bring the (runtime-suspended) host up after
the resume of it has been requested.  However, make that attribute
appear ony for the hosts whose drivers declare support for PM QoS by
populating the pm_qos member of struct mmc_host before registering
the host.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/mmc/mmc-dev-attrs.txt |   24 ++++++++++++
 drivers/mmc/core/host.c             |   67 ++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h            |    9 ++++
 3 files changed, 100 insertions(+)

Index: linux/drivers/mmc/core/host.c
===================================================================
--- linux.orig/drivers/mmc/core/host.c
+++ linux/drivers/mmc/core/host.c
@@ -293,6 +293,68 @@ static inline void mmc_host_clk_sysfs_in
 
 #endif
 
+static ssize_t pm_qos_val_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+	return snprintf(buf, PAGE_SIZE, "%d\n", host->pm_qos->val);
+}
+
+static ssize_t pm_qos_val_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct mmc_host *host = cls_dev_to_mmc_host(dev);
+	s32 value;
+	int ret;
+
+	if (kstrtos32(buf, 0, &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	host->pm_qos->val = value;
+	ret = dev_pm_qos_update_request(&host->pm_qos->req, value);
+	return ret < 0 ? ret : count;
+}
+
+static void mmc_host_pm_qos_init(struct mmc_host *host)
+{
+	int ret;
+
+	if (!host->pm_qos)
+		return;
+
+	ret = dev_pm_qos_add_request(host->parent, &host->pm_qos->req,
+				     host->pm_qos->val);
+	if (ret < 0) {
+		dev_err(host->parent, "Unable to use PM QoS: %d\n", ret);
+		return;
+	}
+
+	host->pm_qos->val_attr.show = pm_qos_val_show;
+	host->pm_qos->val_attr.store = pm_qos_val_store;
+	sysfs_attr_init(&host->pm_qos->val_attr.attr);
+	host->pm_qos->val_attr.attr.name = "pm_latency_limit_ms";
+	host->pm_qos->val_attr.attr.mode = S_IRUGO | S_IWUSR;
+	if (device_create_file(&host->class_dev, &host->pm_qos->val_attr)) {
+		pr_err("%s: Failed to create PM latency limit sysfs entry\n",
+				mmc_hostname(host));
+		host->pm_qos->val_attr.attr.name = NULL;
+	}
+}
+
+static void mmc_host_pm_qos_exit(struct mmc_host *host)
+{
+	if (!host->pm_qos)
+		return;
+
+	if (host->pm_qos->val_attr.attr.name)
+		device_remove_file(&host->class_dev, &host->pm_qos->val_attr);
+
+	dev_pm_qos_remove_request(&host->pm_qos->req);
+}
+
 /**
  *	mmc_alloc_host - initialise the per-host structure.
  *	@extra: sizeof private data structure
@@ -381,6 +443,8 @@ int mmc_add_host(struct mmc_host *host)
 #endif
 	mmc_host_clk_sysfs_init(host);
 
+	mmc_host_pm_qos_init(host);
+
 	mmc_start_host(host);
 	register_pm_notifier(&host->pm_notify);
 
@@ -400,8 +464,11 @@ EXPORT_SYMBOL(mmc_add_host);
 void mmc_remove_host(struct mmc_host *host)
 {
 	unregister_pm_notifier(&host->pm_notify);
+
 	mmc_stop_host(host);
 
+	mmc_host_pm_qos_exit(host);
+
 #ifdef CONFIG_DEBUG_FS
 	mmc_remove_host_debugfs(host);
 #endif
Index: linux/include/linux/mmc/host.h
===================================================================
--- linux.orig/include/linux/mmc/host.h
+++ linux/include/linux/mmc/host.h
@@ -13,6 +13,7 @@
 #include <linux/leds.h>
 #include <linux/sched.h>
 #include <linux/fault-inject.h>
+#include <linux/pm_qos.h>
 
 #include <linux/mmc/core.h>
 #include <linux/mmc/pm.h>
@@ -177,6 +178,12 @@ struct mmc_hotplug {
 	void *handler_priv;
 };
 
+struct mmc_pm_qos {
+	struct dev_pm_qos_request req;
+	s32 val;
+	struct device_attribute val_attr;
+};
+
 struct mmc_host {
 	struct device		*parent;
 	struct device		class_dev;
@@ -191,6 +198,8 @@ struct mmc_host {
 	u32			ocr_avail_mmc;	/* MMC-specific OCR */
 	struct notifier_block	pm_notify;
 
+	struct mmc_pm_qos	*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 */
 #define MMC_VDD_21_22		0x00000200	/* VDD voltage 2.1 ~ 2.2 */
Index: linux/Documentation/mmc/mmc-dev-attrs.txt
===================================================================
--- linux.orig/Documentation/mmc/mmc-dev-attrs.txt
+++ linux/Documentation/mmc/mmc-dev-attrs.txt
@@ -74,3 +74,27 @@ This attribute appears only if CONFIG_MM
 	clkgate_delay	Tune the clock gating delay with desired value in milliseconds.
 
 echo <desired delay> > /sys/class/mmc_host/mmcX/clkgate_delay
+
+MMC PM QoS Support
+==================
+
+If the MMC host driver supports PM QoS, it should populate the pm_qos member
+of struct mmc_host before registering the host.  In that case, read and write
+access if provided to the following attribute:
+
+	pm_latency_limit_ms	Specify the upper bound of the time, since a
+				resume request, necessary to bring the host up
+				after it has been runtime-suspended
+				(in milliseconds).
+
+echo <desired latency limit> > /sys/class/mmc_host/mmcX/pm_latency_limit_ms
+
+The precise meaning of this attribute is "whenever the host is runtime-suspended,
+make sure that it will be possible to bring it up within the given time from
+a resume request" (e.g. if the limit is set to 10 ms, it should always be
+possible to make the runtime-suspended host operational at most 10 ms from a
+resume request).  Note, however, that if the limit is set too small, the only
+physically possible way to respect it may be to keep the host permanently
+operational (i.e. to avoid suspending it).
+
+If 0 is written to pm_latency_limit_ms, it means that there is no limit.

Re: [PATCH 1/3] MMC / PM: Make it possible to use PM QoS latency constraints, v2

[Linus Walleij]

On Sun, Mar 4, 2012 at 8:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:

> A runtime suspend of an MMC controller belonging to a power domain
> or, in a more complicated scenario, a runtime suspend of another
> device in the same power domain, may cause power to be removed from
> the entire domain.  In that case, the amount of time necessary to
> runtime-resume the MMC controller is often substantially greater
> than the time needed to enable its clock.  That may hurt performance
> in some situations, because user data may need to wait for the
> controller to become operational, so we should make it possible to
> prevent that from happening.
>
> For this reason, introduce a new sysfs attribute for MMC hosts,
> pm_latency_limit_ms, allowing user space to specify the upper bound
> of the time necessary to bring the (runtime-suspended) host up after
> the resume of it has been requested.  However, make that attribute
> appear ony for the hosts whose drivers declare support for PM QoS by
> populating the pm_qos member of struct mmc_host before registering
> the host.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

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

Yours,
Linus Walleij

Re: [PATCH 1/3] MMC / PM: Make it possible to use PM QoS latency constraints, v2

[Guennadi Liakhovetski]

Hi Rafael

On Sun, 4 Mar 2012, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> A runtime suspend of an MMC controller belonging to a power domain
> or, in a more complicated scenario, a runtime suspend of another
> device in the same power domain, may cause power to be removed from
> the entire domain.  In that case, the amount of time necessary to
> runtime-resume the MMC controller is often substantially greater
> than the time needed to enable its clock.  That may hurt performance
> in some situations, because user data may need to wait for the
> controller to become operational, so we should make it possible to
> prevent that from happening.
> 
> For this reason, introduce a new sysfs attribute for MMC hosts,
> pm_latency_limit_ms, allowing user space to specify the upper bound
> of the time necessary to bring the (runtime-suspended) host up after
> the resume of it has been requested.  However, make that attribute
> appear ony for the hosts whose drivers declare support for PM QoS by
> populating the pm_qos member of struct mmc_host before registering
> the host.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  Documentation/mmc/mmc-dev-attrs.txt |   24 ++++++++++++
>  drivers/mmc/core/host.c             |   67 ++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h            |    9 ++++
>  3 files changed, 100 insertions(+)
> 
> Index: linux/drivers/mmc/core/host.c
> ===================================================================
> --- linux.orig/drivers/mmc/core/host.c
> +++ linux/drivers/mmc/core/host.c
> @@ -293,6 +293,68 @@ static inline void mmc_host_clk_sysfs_in
>  
>  #endif
>  
> +static ssize_t pm_qos_val_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct mmc_host *host = cls_dev_to_mmc_host(dev);
> +	return snprintf(buf, PAGE_SIZE, "%d\n", host->pm_qos->val);
> +}
> +
> +static ssize_t pm_qos_val_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct mmc_host *host = cls_dev_to_mmc_host(dev);
> +	s32 value;
> +	int ret;
> +
> +	if (kstrtos32(buf, 0, &value))
> +		return -EINVAL;
> +
> +	if (value < 0)
> +		return -EINVAL;
> +
> +	host->pm_qos->val = value;
> +	ret = dev_pm_qos_update_request(&host->pm_qos->req, value);

This seems strange, is this really needed? First assign the new value to 
the request, and then pass it to dev_pm_qos_update_request() with the 
request and as a separate parameter additionally?

Further, isn't value, used in dev_pm_qos*() calls in microseconds, whereas 
your patch is supposed to operate in miliseconds?

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

Re: [PATCH 1/3] MMC / PM: Make it possible to use PM QoS latency constraints, v2

[Rafael J. Wysocki]

On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
> Hi Rafael
> 
> On Sun, 4 Mar 2012, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > A runtime suspend of an MMC controller belonging to a power domain
> > or, in a more complicated scenario, a runtime suspend of another
> > device in the same power domain, may cause power to be removed from
> > the entire domain.  In that case, the amount of time necessary to
> > runtime-resume the MMC controller is often substantially greater
> > than the time needed to enable its clock.  That may hurt performance
> > in some situations, because user data may need to wait for the
> > controller to become operational, so we should make it possible to
> > prevent that from happening.
> > 
> > For this reason, introduce a new sysfs attribute for MMC hosts,
> > pm_latency_limit_ms, allowing user space to specify the upper bound
> > of the time necessary to bring the (runtime-suspended) host up after
> > the resume of it has been requested.  However, make that attribute
> > appear ony for the hosts whose drivers declare support for PM QoS by
> > populating the pm_qos member of struct mmc_host before registering
> > the host.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  Documentation/mmc/mmc-dev-attrs.txt |   24 ++++++++++++
> >  drivers/mmc/core/host.c             |   67 ++++++++++++++++++++++++++++++++++++
> >  include/linux/mmc/host.h            |    9 ++++
> >  3 files changed, 100 insertions(+)
> > 
> > Index: linux/drivers/mmc/core/host.c
> > ===================================================================
> > --- linux.orig/drivers/mmc/core/host.c
> > +++ linux/drivers/mmc/core/host.c
> > @@ -293,6 +293,68 @@ static inline void mmc_host_clk_sysfs_in
> >  
> >  #endif
> >  
> > +static ssize_t pm_qos_val_show(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	struct mmc_host *host = cls_dev_to_mmc_host(dev);
> > +	return snprintf(buf, PAGE_SIZE, "%d\n", host->pm_qos->val);
> > +}
> > +
> > +static ssize_t pm_qos_val_store(struct device *dev,
> > +		struct device_attribute *attr, const char *buf, size_t count)
> > +{
> > +	struct mmc_host *host = cls_dev_to_mmc_host(dev);
> > +	s32 value;
> > +	int ret;
> > +
> > +	if (kstrtos32(buf, 0, &value))
> > +		return -EINVAL;
> > +
> > +	if (value < 0)
> > +		return -EINVAL;
> > +
> > +	host->pm_qos->val = value;
> > +	ret = dev_pm_qos_update_request(&host->pm_qos->req, value);
> 
> This seems strange, is this really needed? First assign the new value to 
> the request,

Not to the request, but to the val field which is in the same structure.

This field is needed anyway for initialization (the initial value is
passed through it from the driver to the core) and I don't see why not to
update it later (which allows it to be used by pm_qos_val_show without
looking into pm_qos->req).

However, I should update it only if the result returned by
dev_pm_qos_update_request() is not an error code.  Will fix.

> and then pass it to dev_pm_qos_update_request() with the 
> request and as a separate parameter additionally?
>
> Further, isn't value, used in dev_pm_qos*() calls in microseconds, whereas 
> your patch is supposed to operate in miliseconds?

Yes, it is in microseconds, sorry for the confusion.

Thanks,
Rafael

[PATCH 2/3] tmio_mmc / PM: Use PM QoS requests, v2

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make tmio_mmc populate the pm_qos member of struct mmc_host, to
let the core know that it should create the pm_latency_limit_ms
host attribute for it, and set the default value of that attribute
to 100 ms.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/mmc/host/tmio_mmc_pio.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux/drivers/mmc/host/tmio_mmc_pio.c
===================================================================
--- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
+++ linux/drivers/mmc/host/tmio_mmc_pio.c
@@ -918,6 +918,12 @@ int __devinit tmio_mmc_host_probe(struct
 	if (ret < 0)
 		goto pm_disable;
 
+	mmc->pm_qos = kzalloc(sizeof(*mmc->pm_qos), GFP_KERNEL);
+	if (mmc->pm_qos)
+		mmc->pm_qos->val = 100;
+	else
+		dev_err(&pdev->dev, "Not enough memory for PM QoS.\n");
+
 	/*
 	 * There are 4 different scenarios for the card detection:
 	 *  1) an external gpio irq handles the cd (best for power savings)
@@ -998,6 +1004,8 @@ void tmio_mmc_host_remove(struct tmio_mm
 	cancel_delayed_work_sync(&host->delayed_reset_work);
 	tmio_mmc_release_dma(host);
 
+	kfree(host->mmc->pm_qos);
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 

Re: [PATCH 2/3] tmio_mmc / PM: Use PM QoS requests, v2

[Guennadi Liakhovetski]

On Sun, 4 Mar 2012, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Make tmio_mmc populate the pm_qos member of struct mmc_host, to
> let the core know that it should create the pm_latency_limit_ms
> host attribute for it, and set the default value of that attribute
> to 100 ms.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/mmc/host/tmio_mmc_pio.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> Index: linux/drivers/mmc/host/tmio_mmc_pio.c
> ===================================================================
> --- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
> +++ linux/drivers/mmc/host/tmio_mmc_pio.c
> @@ -918,6 +918,12 @@ int __devinit tmio_mmc_host_probe(struct
>  	if (ret < 0)
>  		goto pm_disable;
>  
> +	mmc->pm_qos = kzalloc(sizeof(*mmc->pm_qos), GFP_KERNEL);

Why don't you just add pm_qos to struct tmio_mmc_host?

> +	if (mmc->pm_qos)
> +		mmc->pm_qos->val = 100;

100ms... that seems way too long for me, wouldn't that allow the 
runtime-pm to power down and up the domain on each request?...

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

Re: [PATCH 2/3] tmio_mmc / PM: Use PM QoS requests, v2

[Rafael J. Wysocki]

On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
> On Sun, 4 Mar 2012, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Make tmio_mmc populate the pm_qos member of struct mmc_host, to
> > let the core know that it should create the pm_latency_limit_ms
> > host attribute for it, and set the default value of that attribute
> > to 100 ms.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/mmc/host/tmio_mmc_pio.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > Index: linux/drivers/mmc/host/tmio_mmc_pio.c
> > ===================================================================
> > --- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
> > +++ linux/drivers/mmc/host/tmio_mmc_pio.c
> > @@ -918,6 +918,12 @@ int __devinit tmio_mmc_host_probe(struct
> >  	if (ret < 0)
> >  		goto pm_disable;
> >  
> > +	mmc->pm_qos = kzalloc(sizeof(*mmc->pm_qos), GFP_KERNEL);
> 
> Why don't you just add pm_qos to struct tmio_mmc_host?

Because the core has to see it.

> > +	if (mmc->pm_qos)
> > +		mmc->pm_qos->val = 100;
> 
> 100ms... that seems way too long for me, wouldn't that allow the 
> runtime-pm to power down and up the domain on each request?...

That should be 100 us, sorry for the confusion.

Thanks,
Rafael

Re: [PATCH 2/3] tmio_mmc / PM: Use PM QoS requests, v2

[Guennadi Liakhovetski]

On Tue, 6 Mar 2012, Rafael J. Wysocki wrote:

> On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
> > On Sun, 4 Mar 2012, Rafael J. Wysocki wrote:
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Make tmio_mmc populate the pm_qos member of struct mmc_host, to
> > > let the core know that it should create the pm_latency_limit_ms
> > > host attribute for it, and set the default value of that attribute
> > > to 100 ms.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  drivers/mmc/host/tmio_mmc_pio.c |    8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > Index: linux/drivers/mmc/host/tmio_mmc_pio.c
> > > ===================================================================
> > > --- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
> > > +++ linux/drivers/mmc/host/tmio_mmc_pio.c
> > > @@ -918,6 +918,12 @@ int __devinit tmio_mmc_host_probe(struct
> > >  	if (ret < 0)
> > >  		goto pm_disable;
> > >  
> > > +	mmc->pm_qos = kzalloc(sizeof(*mmc->pm_qos), GFP_KERNEL);
> > 
> > Why don't you just add pm_qos to struct tmio_mmc_host?
> 
> Because the core has to see it.

Yes, I understand that. So, you just assign a pointer to it to 
mmc->pm_qos, just avoiding an extra allocation / freeing / error handling.

> > > +	if (mmc->pm_qos)
> > > +		mmc->pm_qos->val = 100;
> > 
> > 100ms... that seems way too long for me, wouldn't that allow the 
> > runtime-pm to power down and up the domain on each request?...
> 
> That should be 100 us, sorry for the confusion.

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

Re: [PATCH 2/3] tmio_mmc / PM: Use PM QoS requests, v2

[Rafael J. Wysocki]

On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
> On Tue, 6 Mar 2012, Rafael J. Wysocki wrote:
> 
> > On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
> > > On Sun, 4 Mar 2012, Rafael J. Wysocki wrote:
> > > 
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > Make tmio_mmc populate the pm_qos member of struct mmc_host, to
> > > > let the core know that it should create the pm_latency_limit_ms
> > > > host attribute for it, and set the default value of that attribute
> > > > to 100 ms.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > ---
> > > >  drivers/mmc/host/tmio_mmc_pio.c |    8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > Index: linux/drivers/mmc/host/tmio_mmc_pio.c
> > > > ===================================================================
> > > > --- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
> > > > +++ linux/drivers/mmc/host/tmio_mmc_pio.c
> > > > @@ -918,6 +918,12 @@ int __devinit tmio_mmc_host_probe(struct
> > > >  	if (ret < 0)
> > > >  		goto pm_disable;
> > > >  
> > > > +	mmc->pm_qos = kzalloc(sizeof(*mmc->pm_qos), GFP_KERNEL);
> > > 
> > > Why don't you just add pm_qos to struct tmio_mmc_host?
> > 
> > Because the core has to see it.
> 
> Yes, I understand that. So, you just assign a pointer to it to 
> mmc->pm_qos, just avoiding an extra allocation / freeing / error handling.

Well, I think this is a very minor point.

I'm going to move the sysfs attribute creation to the PM core anyway.

Thanks,
Rafael

[PATCH 3/3] sh_mmcif / PM: Use PM QoS requests, v2

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make sh_mmcif populate the pm_qos member of struct mmc_host, to
let the core know that it should create the pm_latency_limit_ms
host attribute for it, and set the default value of that attribute
to 100 ms.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/mmc/host/sh_mmcif.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux/drivers/mmc/host/sh_mmcif.c
===================================================================
--- linux.orig/drivers/mmc/host/sh_mmcif.c
+++ linux/drivers/mmc/host/sh_mmcif.c
@@ -57,6 +57,7 @@
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/module.h>
 
@@ -1327,6 +1328,12 @@ static int __devinit sh_mmcif_probe(stru
 	if (ret < 0)
 		goto clean_up2;
 
+	mmc->pm_qos = kzalloc(sizeof(*mmc->pm_qos), GFP_KERNEL);
+	if (mmc->pm_qos)
+		mmc->pm_qos->val = 100;
+	else
+		dev_err(&pdev->dev, "Not enough memory for PM QoS.\n");
+
 	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
 
 	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
@@ -1356,6 +1363,7 @@ clean_up5:
 clean_up4:
 	free_irq(irq[0], host);
 clean_up3:
+	kfree(mmc->pm_qos);
 	pm_runtime_suspend(&pdev->dev);
 clean_up2:
 	pm_runtime_disable(&pdev->dev);
@@ -1397,6 +1405,8 @@ static int __devexit sh_mmcif_remove(str
 
 	platform_set_drvdata(pdev, NULL);
 
+	kfree(host->mmc->pm_qos);
+
 	clk_disable(host->hclk);
 	mmc_free_host(host->mmc);
 	pm_runtime_put_sync(&pdev->dev);

Re: [PATCH 3/3] sh_mmcif / PM: Use PM QoS requests, v2

[Guennadi Liakhovetski]

On Sun, 4 Mar 2012, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Make sh_mmcif populate the pm_qos member of struct mmc_host, to
> let the core know that it should create the pm_latency_limit_ms
> host attribute for it, and set the default value of that attribute
> to 100 ms.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/mmc/host/sh_mmcif.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> Index: linux/drivers/mmc/host/sh_mmcif.c
> ===================================================================
> --- linux.orig/drivers/mmc/host/sh_mmcif.c
> +++ linux/drivers/mmc/host/sh_mmcif.c
> @@ -57,6 +57,7 @@
>  #include <linux/pagemap.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/module.h>
>  
> @@ -1327,6 +1328,12 @@ static int __devinit sh_mmcif_probe(stru
>  	if (ret < 0)
>  		goto clean_up2;
>  
> +	mmc->pm_qos = kzalloc(sizeof(*mmc->pm_qos), GFP_KERNEL);
> +	if (mmc->pm_qos)
> +		mmc->pm_qos->val = 100;

Same comments as for tmio_mmc: embed pm_qos and reduce the value?

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

Re: [PATCH 3/3] sh_mmcif / PM: Use PM QoS requests, v2

[Rafael J. Wysocki]

On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
> On Sun, 4 Mar 2012, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Make sh_mmcif populate the pm_qos member of struct mmc_host, to
> > let the core know that it should create the pm_latency_limit_ms
> > host attribute for it, and set the default value of that attribute
> > to 100 ms.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/mmc/host/sh_mmcif.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > Index: linux/drivers/mmc/host/sh_mmcif.c
> > ===================================================================
> > --- linux.orig/drivers/mmc/host/sh_mmcif.c
> > +++ linux/drivers/mmc/host/sh_mmcif.c
> > @@ -57,6 +57,7 @@
> >  #include <linux/pagemap.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/module.h>
> >  
> > @@ -1327,6 +1328,12 @@ static int __devinit sh_mmcif_probe(stru
> >  	if (ret < 0)
> >  		goto clean_up2;
> >  
> > +	mmc->pm_qos = kzalloc(sizeof(*mmc->pm_qos), GFP_KERNEL);
> > +	if (mmc->pm_qos)
> > +		mmc->pm_qos->val = 100;
> 
> Same comments as for tmio_mmc: embed pm_qos

That wouldn't work.

> and reduce the value?

The value will be fine when the units are correct.

Thanks,
Rafael

[PATCH 0/3] PM: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

Hi all,

On Sunday, March 04, 2012, Rafael J. Wysocki wrote:
> On Sunday, March 04, 2012, Rafael J. Wysocki wrote:
> > 
> > The goal of this patchset is to allow user space to control the
> > responsiveness of the MMC stack related to runtime power management.
> > 
> > Namely, on systems that contain power domains, the time necessary
> > to bring an MMC host up after it has been runtime-suspended may
> > depend not only on the MMC core and host driver, but also on the platform
> > and other devices in the same power domain(s) that contain(s) the MMC
> > host.  Although that obviously may influence the MMC performance,
> > currently, there is no way to control it through the MMC subsystem.
> > Moreover, the only available way to control it at all is by using PM QoS
> > latency requests, so it is necessary to add some kind of support for that
> > to MMC.
> > 
> > Patch [1/3] adds PM QoS-related fields to struct mmc_host and introduces
> > a new sysfs attribute for MMC hosts, pm_latency_limit_ms, allowing user
> > space to influence the MMC host's runtime PM via PM QoS.  Whether or not
> > this attribute will be created (and PM QoS will be used for the given host)
> > depends on the host driver, so host drivers that don't (plan) to support
> > PM QoS don't need to do anything about that.
> > 
> > Patches [2/3] and [3/3] add the corresponding host driver bits to the
> > tmio_mmc and sh_mmcif drivers, respectively.
> 
> After posting the patches I noticed that the changelog of patch [1/3] and
> the documentation of the new sysfs attribute were not 100% accurate, because
> the PM QoS request really applies to the time between a resume request and
> the moment the device is operational again and not the time from runtime
> suspend (the latter would imply some kind of autoresume mechanism, which isn't
> there).
> 
> I also thought it would be cleaner to allocate the val and attr fields along
> with the request, because in the previous version of the patchset they weren't
> used when req was NULL, resulting in (a little) wasted memory.
> 
> Updated patches follow.

Taking the feedback so far into account, I decided to move the exposing of the
PM QoS latency limit from the MMC layer to the PM core sysfs code, so that
every driver (not only MMC host drivers) can use it.

New patches follow, details are in the changelogs:

[1/3] - Make it possible to expose PM QoS latency constraints
[2/3] - Make tmio_mmc use the new interface.
[3/3] - Make sh_mmcif use the new interface.

Thanks,
Rafael

[PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

A runtime suspend of a device (e.g. an MMC controller) belonging to
a power domain or, in a more complicated scenario, a runtime suspend
of another device in the same power domain, may cause power to be
removed from the entire domain.  In that case, the amount of time
necessary to runtime-resume the given device (e.g. the MMC
controller) is often substantially greater than the time needed to
run its driver's runtime resume callback.  That may hurt performance
in some situations, because user data may need to wait for the
device to become operational, so we should make it possible to
prevent that from happening.

For this reason, introduce a new sysfs attribute for devices,
power/pm_qos_latency_us, allowing user space to specify the upper
bound of the time necessary to bring the (runtime-suspended) device
up after the resume of it has been requested.  However, make that
attribute appear ony for the devices whose drivers declare support
for by calling the (new) dev_pm_qos_expose_latency_limit() helper
function with the appropriate initial value of the attribute.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/ABI/testing/sysfs-devices-power |   15 +++++++
 drivers/base/power/power.h                    |    4 +
 drivers/base/power/qos.c                      |   53 ++++++++++++++++++++++++++
 drivers/base/power/sysfs.c                    |   46 ++++++++++++++++++++++
 include/linux/pm.h                            |    1 
 include/linux/pm_qos.h                        |    5 ++
 6 files changed, 124 insertions(+)

Index: linux/include/linux/pm.h
===================================================================
--- linux.orig/include/linux/pm.h
+++ linux/include/linux/pm.h
@@ -549,6 +549,7 @@ struct dev_pm_info {
 #endif
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	struct pm_qos_constraints *constraints;
+	struct dev_pm_qos_request *pq_req;
 };
 
 extern void update_pm_runtime_accounting(struct device *dev);
Index: linux/drivers/base/power/sysfs.c
===================================================================
--- linux.orig/drivers/base/power/sysfs.c
+++ linux/drivers/base/power/sysfs.c
@@ -5,6 +5,7 @@
 #include <linux/device.h>
 #include <linux/string.h>
 #include <linux/export.h>
+#include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/atomic.h>
 #include <linux/jiffies.h>
@@ -455,6 +456,32 @@ static ssize_t async_store(struct device
 static DEVICE_ATTR(async, 0644, async_show, async_store);
 #endif /* CONFIG_PM_ADVANCED_DEBUG */
 
+static ssize_t pm_qos_latency_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", dev->power.pq_req->node.prio);
+}
+
+static ssize_t pm_qos_latency_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t n)
+{
+	s32 value;
+	int ret;
+
+	if (kstrtos32(buf, 0, &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	ret = dev_pm_qos_update_request(dev->power.pq_req, value);
+	return ret < 0 ? ret : n;
+}
+
+static DEVICE_ATTR(pm_qos_latency_us, 0644,
+		   pm_qos_latency_show, pm_qos_latency_store);
+
 static struct attribute *power_attrs[] = {
 #ifdef CONFIG_PM_ADVANCED_DEBUG
 #ifdef CONFIG_PM_SLEEP
@@ -510,6 +537,15 @@ static struct attribute_group pm_runtime
 	.attrs	= runtime_attrs,
 };
 
+static struct attribute *pm_qos_attrs[] = {
+	&dev_attr_pm_qos_latency_us.attr,
+	NULL,
+};
+static struct attribute_group pm_qos_attr_group = {
+	.name	= power_group_name,
+	.attrs	= pm_qos_attrs,
+};
+
 int dpm_sysfs_add(struct device *dev)
 {
 	int rc;
@@ -550,6 +586,16 @@ void wakeup_sysfs_remove(struct device *
 	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
 }
 
+int pm_qos_sysfs_add(struct device *dev)
+{
+	return sysfs_merge_group(&dev->kobj, &pm_qos_attr_group);
+}
+
+void pm_qos_sysfs_remove(struct device *dev)
+{
+	sysfs_unmerge_group(&dev->kobj, &pm_qos_attr_group);
+}
+
 void rpm_sysfs_remove(struct device *dev)
 {
 	sysfs_unmerge_group(&dev->kobj, &pm_runtime_attr_group);
Index: linux/drivers/base/power/power.h
===================================================================
--- linux.orig/drivers/base/power/power.h
+++ linux/drivers/base/power/power.h
@@ -71,6 +71,8 @@ extern void dpm_sysfs_remove(struct devi
 extern void rpm_sysfs_remove(struct device *dev);
 extern int wakeup_sysfs_add(struct device *dev);
 extern void wakeup_sysfs_remove(struct device *dev);
+extern int pm_qos_sysfs_add(struct device *dev);
+extern void pm_qos_sysfs_remove(struct device *dev);
 
 #else /* CONFIG_PM */
 
@@ -79,5 +81,7 @@ static inline void dpm_sysfs_remove(stru
 static inline void rpm_sysfs_remove(struct device *dev) {}
 static inline int wakeup_sysfs_add(struct device *dev) { return 0; }
 static inline void wakeup_sysfs_remove(struct device *dev) {}
+static inline int pm_qos_sysfs_add(struct device *dev) { return 0; }
+static inline void pm_qos_sysfs_remove(struct device *dev) {}
 
 #endif
Index: linux/drivers/base/power/qos.c
===================================================================
--- linux.orig/drivers/base/power/qos.c
+++ linux/drivers/base/power/qos.c
@@ -41,6 +41,7 @@
 #include <linux/mutex.h>
 #include <linux/export.h>
 
+#include "power.h"
 
 static DEFINE_MUTEX(dev_pm_qos_mtx);
 
@@ -445,3 +446,55 @@ int dev_pm_qos_add_ancestor_request(stru
 	return error;
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_add_ancestor_request);
+
+static void __dev_pm_qos_drop_user_request(struct device *dev)
+{
+	dev_pm_qos_remove_request(dev->power.pq_req);
+	dev->power.pq_req = 0;
+}
+
+/**
+ * dev_pm_qos_expose_latency_limit - Expose PM QoS latency limit to user space.
+ * @dev: Device whose PM QoS latency limit is to be exposed to user space.
+ * @value: Initial value of the latency limit.
+ */
+int dev_pm_qos_expose_latency_limit(struct device *dev, s32 value)
+{
+	struct dev_pm_qos_request *req;
+	int ret;
+
+	if (!device_is_registered(dev) || value < 0)
+		return -EINVAL;
+
+	if (dev->power.pq_req)
+		return -EEXIST;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	ret = dev_pm_qos_add_request(dev, req, value);
+	if (ret < 0)
+		return ret;
+
+	dev->power.pq_req = req;
+	ret = pm_qos_sysfs_add(dev);
+	if (ret)
+		__dev_pm_qos_drop_user_request(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_qos_expose_latency_limit);
+
+/**
+ * dev_pm_qos_hide_latency_limit - Hide PM QoS latency limit from user space.
+ * @dev: Device whose PM QoS latency limit is to be hidden from user space.
+ */
+void dev_pm_qos_hide_latency_limit(struct device *dev)
+{
+	if (dev->power.pq_req) {
+		pm_qos_sysfs_remove(dev);
+		__dev_pm_qos_drop_user_request(dev);
+	}
+}
+EXPORT_SYMBOL_GPL(dev_pm_qos_hide_latency_limit);
Index: linux/include/linux/pm_qos.h
===================================================================
--- linux.orig/include/linux/pm_qos.h
+++ linux/include/linux/pm_qos.h
@@ -98,6 +98,8 @@ void dev_pm_qos_constraints_init(struct
 void dev_pm_qos_constraints_destroy(struct device *dev);
 int dev_pm_qos_add_ancestor_request(struct device *dev,
 				    struct dev_pm_qos_request *req, s32 value);
+int dev_pm_qos_expose_latency_limit(struct device *dev, s32 value);
+void dev_pm_qos_hide_latency_limit(struct device *dev);
 #else
 static inline s32 __dev_pm_qos_read_value(struct device *dev)
 			{ return 0; }
@@ -135,6 +137,9 @@ static inline void dev_pm_qos_constraint
 static inline int dev_pm_qos_add_ancestor_request(struct device *dev,
 				    struct dev_pm_qos_request *req, s32 value)
 			{ return 0; }
+int dev_pm_qos_expose_latency_limit(struct device *dev, s32 value)
+			{ return 0; }
+void dev_pm_qos_hide_latency_limit(struct device *dev) {}
 #endif
 
 #endif
Index: linux/Documentation/ABI/testing/sysfs-devices-power
===================================================================
--- linux.orig/Documentation/ABI/testing/sysfs-devices-power
+++ linux/Documentation/ABI/testing/sysfs-devices-power
@@ -175,3 +175,18 @@ Description:
 
 		Not all drivers support this attribute.  If it isn't supported,
 		attempts to read or write it will yield I/O errors.
+
+What:		/sys/devices/.../power/pm_qos_latency_us
+Date:		March 2012
+Contact:	Rafael J. Wysocki <rjw@sisk.pl>
+Description:
+		The /sys/devices/.../power/pm_qos_latency_us attribute contains
+		the PM QoS resume latency limit for the given device, which is
+		the maximum allowed time it can take to resume the device, after
+		it has been suspended at run time, from a resume request to the
+		moment the device will be ready to process I/O, in microseconds.
+		If it is equal to 0, however, this means that the PM QoS resume
+		latency may be arbitrary.
+
+		Not all drivers support this attribute.  If it isn't supported,
+		it is not present.

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> A runtime suspend of a device (e.g. an MMC controller) belonging to
> a power domain or, in a more complicated scenario, a runtime suspend
> of another device in the same power domain, may cause power to be
> removed from the entire domain.  In that case, the amount of time
> necessary to runtime-resume the given device (e.g. the MMC
> controller) is often substantially greater than the time needed to
> run its driver's runtime resume callback.  That may hurt performance
> in some situations, because user data may need to wait for the
> device to become operational, so we should make it possible to
> prevent that from happening.
>
> For this reason, introduce a new sysfs attribute for devices,
> power/pm_qos_latency_us, allowing user space to specify the upper

If we're expecting to have more of these knobs, maybe having a pm_qos
subdir under power will keep down the clutter in /sys/devices/.../power.
This knob would then be /sys/devices/.../power/pm_qos/pm_qos_latency_us.

I think 'latency' alone is a bit too vague (wakeup latency?  interrupt
latency?  I think wakeup latency is clearer.  Another possibility is
resume latency, IMO, that will lead to confusion about whether this
field also affects system suspend/resume.

That brings up another point: I think the docs should be very clear
about how this affects system suspend/resume.  From my understanding, it
is only intended to affect runtime suspend/resume but I think the
docs/comments need to be very clear about this since as you know the
overlap between system PM and runtime PM has been a source of
confusion.

> bound of the time necessary to bring the (runtime-suspended) device
> up after the resume of it has been requested.  However, make that
> attribute appear ony for the devices whose drivers declare support

s/ony/only/

> for by calling the (new) dev_pm_qos_expose_latency_limit() helper
> function with the appropriate initial value of the attribute.

Yes.  I really like the ability to hide/expose this feature, and that
the default is that it's hidden.

That feature addresses my primary concern about exposing too much to
userspace for certain subsystems.

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Since I've objected to this kind of feature in the past, I'll just say
for the record that I'm fine with selectively exposing this particular
knob.

Reviewed-by: Kevin Hilman <khilman@ti.com>

Kevin

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

On Thu, Mar 08, 2012 at 09:49:24AM -0800, Kevin Hilman wrote:

> Since I've objected to this kind of feature in the past, I'll just say
> for the record that I'm fine with selectively exposing this particular
> knob.

> Reviewed-by: Kevin Hilman <khilman@ti.com>

I agree with everything Kevin said:

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

On Thursday, March 08, 2012, Mark Brown wrote:
> On Thu, Mar 08, 2012 at 09:49:24AM -0800, Kevin Hilman wrote:
> 
> > Since I've objected to this kind of feature in the past, I'll just say
> > for the record that I'm fine with selectively exposing this particular
> > knob.
> 
> > Reviewed-by: Kevin Hilman <khilman@ti.com>
> 
> I agree with everything Kevin said:
> 
> Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Thanks!

Rafael

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Guennadi Liakhovetski]

On Thu, 8 Mar 2012, Kevin Hilman wrote:

> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > A runtime suspend of a device (e.g. an MMC controller) belonging to
> > a power domain or, in a more complicated scenario, a runtime suspend
> > of another device in the same power domain, may cause power to be
> > removed from the entire domain.  In that case, the amount of time
> > necessary to runtime-resume the given device (e.g. the MMC
> > controller) is often substantially greater than the time needed to
> > run its driver's runtime resume callback.  That may hurt performance
> > in some situations, because user data may need to wait for the
> > device to become operational, so we should make it possible to
> > prevent that from happening.
> >
> > For this reason, introduce a new sysfs attribute for devices,
> > power/pm_qos_latency_us, allowing user space to specify the upper
> 
> If we're expecting to have more of these knobs, maybe having a pm_qos
> subdir under power will keep down the clutter in /sys/devices/.../power.
> This knob would then be /sys/devices/.../power/pm_qos/pm_qos_latency_us.

If this change does have to be made, I'd propose

.../power/qos/latency...

removing redundancy.

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

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

On Thursday, March 08, 2012, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > A runtime suspend of a device (e.g. an MMC controller) belonging to
> > a power domain or, in a more complicated scenario, a runtime suspend
> > of another device in the same power domain, may cause power to be
> > removed from the entire domain.  In that case, the amount of time
> > necessary to runtime-resume the given device (e.g. the MMC
> > controller) is often substantially greater than the time needed to
> > run its driver's runtime resume callback.  That may hurt performance
> > in some situations, because user data may need to wait for the
> > device to become operational, so we should make it possible to
> > prevent that from happening.
> >
> > For this reason, introduce a new sysfs attribute for devices,
> > power/pm_qos_latency_us, allowing user space to specify the upper
> 
> If we're expecting to have more of these knobs, maybe having a pm_qos
> subdir under power will keep down the clutter in /sys/devices/.../power.
> This knob would then be /sys/devices/.../power/pm_qos/pm_qos_latency_us.

I'm not sure how difficult it is to create a subdir in sysfs under something
that is not a kobject.

Besides, this follows the convention already used by wakeup and runtime PM
attributes that don't have their own subdirs (although there may be a number
of them in each category).

> I think 'latency' alone is a bit too vague (wakeup latency?  interrupt
> latency?  I think wakeup latency is clearer.  Another possibility is
> resume latency, IMO, that will lead to confusion about whether this
> field also affects system suspend/resume.

I think "wakeup latency" will lead to more confusion because of the
wakeup-related attributes.  I'll go for "resume_latency" if you don't
mind. :-)

> That brings up another point: I think the docs should be very clear
> about how this affects system suspend/resume.  From my understanding, it
> is only intended to affect runtime suspend/resume

That's correct.

> but I think the
> docs/comments need to be very clear about this since as you know the
> overlap between system PM and runtime PM has been a source of
> confusion.

OK

> > bound of the time necessary to bring the (runtime-suspended) device
> > up after the resume of it has been requested.  However, make that
> > attribute appear ony for the devices whose drivers declare support
> 
> s/ony/only/

Yup, thanks.

> > for by calling the (new) dev_pm_qos_expose_latency_limit() helper
> > function with the appropriate initial value of the attribute.
> 
> Yes.  I really like the ability to hide/expose this feature, and that
> the default is that it's hidden.
> 
> That feature addresses my primary concern about exposing too much to
> userspace for certain subsystems.
> 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Since I've objected to this kind of feature in the past, I'll just say
> for the record that I'm fine with selectively exposing this particular
> knob.
> 
> Reviewed-by: Kevin Hilman <khilman@ti.com>

Thanks!

I'll update the patches and post a new version shortly.

Rafael

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Thursday, March 08, 2012, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> >
>> > A runtime suspend of a device (e.g. an MMC controller) belonging to
>> > a power domain or, in a more complicated scenario, a runtime suspend
>> > of another device in the same power domain, may cause power to be
>> > removed from the entire domain.  In that case, the amount of time
>> > necessary to runtime-resume the given device (e.g. the MMC
>> > controller) is often substantially greater than the time needed to
>> > run its driver's runtime resume callback.  That may hurt performance
>> > in some situations, because user data may need to wait for the
>> > device to become operational, so we should make it possible to
>> > prevent that from happening.
>> >
>> > For this reason, introduce a new sysfs attribute for devices,
>> > power/pm_qos_latency_us, allowing user space to specify the upper
>> 
>> If we're expecting to have more of these knobs, maybe having a pm_qos
>> subdir under power will keep down the clutter in /sys/devices/.../power.
>> This knob would then be /sys/devices/.../power/pm_qos/pm_qos_latency_us.
>
> I'm not sure how difficult it is to create a subdir in sysfs under something
> that is not a kobject.
>
> Besides, this follows the convention already used by wakeup and runtime PM
> attributes that don't have their own subdirs (although there may be a number
> of them in each category).

OK

>> I think 'latency' alone is a bit too vague (wakeup latency?  interrupt
>> latency?  I think wakeup latency is clearer.  Another possibility is
>> resume latency, IMO, that will lead to confusion about whether this
>> field also affects system suspend/resume.
>
> I think "wakeup latency" will lead to more confusion because of the
> wakeup-related attributes.  

What confusion?  All of those are related to device wakeups from some
low power state, and so is this proposed latency attribute.  So I don't
understand the potential confusion.

> I'll go for "resume_latency" if you don't mind. :-)

Most people think of resume as coming back from system PM.  If this is
called resume_latency, I would expect confusion about why setting this
attribute has no effect on how fast their system returns from system
suspend.

Kevin

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

On Thursday, March 08, 2012, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Thursday, March 08, 2012, Kevin Hilman wrote:
> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> >> 
> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >> >
> >> > A runtime suspend of a device (e.g. an MMC controller) belonging to
> >> > a power domain or, in a more complicated scenario, a runtime suspend
> >> > of another device in the same power domain, may cause power to be
> >> > removed from the entire domain.  In that case, the amount of time
> >> > necessary to runtime-resume the given device (e.g. the MMC
> >> > controller) is often substantially greater than the time needed to
> >> > run its driver's runtime resume callback.  That may hurt performance
> >> > in some situations, because user data may need to wait for the
> >> > device to become operational, so we should make it possible to
> >> > prevent that from happening.
> >> >
> >> > For this reason, introduce a new sysfs attribute for devices,
> >> > power/pm_qos_latency_us, allowing user space to specify the upper
> >> 
> >> If we're expecting to have more of these knobs, maybe having a pm_qos
> >> subdir under power will keep down the clutter in /sys/devices/.../power.
> >> This knob would then be /sys/devices/.../power/pm_qos/pm_qos_latency_us.
> >
> > I'm not sure how difficult it is to create a subdir in sysfs under something
> > that is not a kobject.
> >
> > Besides, this follows the convention already used by wakeup and runtime PM
> > attributes that don't have their own subdirs (although there may be a number
> > of them in each category).
> 
> OK
> 
> >> I think 'latency' alone is a bit too vague (wakeup latency?  interrupt
> >> latency?  I think wakeup latency is clearer.  Another possibility is
> >> resume latency, IMO, that will lead to confusion about whether this
> >> field also affects system suspend/resume.
> >
> > I think "wakeup latency" will lead to more confusion because of the
> > wakeup-related attributes.  
> 
> What confusion?  All of those are related to device wakeups from some
> low power state, and so is this proposed latency attribute.  So I don't
> understand the potential confusion.

The word "wakeup" may refer to many different things, as well as the word
"resume". :-)

> > I'll go for "resume_latency" if you don't mind. :-)
> 
> Most people think of resume as coming back from system PM.  If this is
> called resume_latency, I would expect confusion about why setting this
> attribute has no effect on how fast their system returns from system
> suspend.

I'll make it depend on CONFIG_PM_RUNTIME _and_ write in the documentation
that this attribute has no effect on system-wide suspend/resume.  That should
be sufficient for the majority of people and if it's not for someone, well,
I guess that's really a problem of that person. :-)

Thanks,
Rafael

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Thursday, March 08, 2012, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> > On Thursday, March 08, 2012, Kevin Hilman wrote:
>> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> >> 
>> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> >> >
>> >> > A runtime suspend of a device (e.g. an MMC controller) belonging to
>> >> > a power domain or, in a more complicated scenario, a runtime suspend
>> >> > of another device in the same power domain, may cause power to be
>> >> > removed from the entire domain.  In that case, the amount of time
>> >> > necessary to runtime-resume the given device (e.g. the MMC
>> >> > controller) is often substantially greater than the time needed to
>> >> > run its driver's runtime resume callback.  That may hurt performance
>> >> > in some situations, because user data may need to wait for the
>> >> > device to become operational, so we should make it possible to
>> >> > prevent that from happening.
>> >> >
>> >> > For this reason, introduce a new sysfs attribute for devices,
>> >> > power/pm_qos_latency_us, allowing user space to specify the upper
>> >> 
>> >> If we're expecting to have more of these knobs, maybe having a pm_qos
>> >> subdir under power will keep down the clutter in /sys/devices/.../power.
>> >> This knob would then be /sys/devices/.../power/pm_qos/pm_qos_latency_us.
>> >
>> > I'm not sure how difficult it is to create a subdir in sysfs under something
>> > that is not a kobject.
>> >
>> > Besides, this follows the convention already used by wakeup and runtime PM
>> > attributes that don't have their own subdirs (although there may be a number
>> > of them in each category).
>> 
>> OK
>> 
>> >> I think 'latency' alone is a bit too vague (wakeup latency?  interrupt
>> >> latency?  I think wakeup latency is clearer.  Another possibility is
>> >> resume latency, IMO, that will lead to confusion about whether this
>> >> field also affects system suspend/resume.
>> >
>> > I think "wakeup latency" will lead to more confusion because of the
>> > wakeup-related attributes.  
>> 
>> What confusion?  All of those are related to device wakeups from some
>> low power state, and so is this proposed latency attribute.  So I don't
>> understand the potential confusion.
>
> The word "wakeup" may refer to many different things, as well as the word
> "resume". :-)

Yes, but what's the confusion in this case?

IMO, The existing /sys/devices/.../power/wakeup* meaning is the same
meaning as as for the wakeup latency in this patch, so I don't
understand where the confusion would be.

Kevin

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

On Friday, March 09, 2012, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Thursday, March 08, 2012, Kevin Hilman wrote:
> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> >> 
> >> > On Thursday, March 08, 2012, Kevin Hilman wrote:
> >> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> >> >> 
> >> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >> >> >
> >> >> > A runtime suspend of a device (e.g. an MMC controller) belonging to
> >> >> > a power domain or, in a more complicated scenario, a runtime suspend
> >> >> > of another device in the same power domain, may cause power to be
> >> >> > removed from the entire domain.  In that case, the amount of time
> >> >> > necessary to runtime-resume the given device (e.g. the MMC
> >> >> > controller) is often substantially greater than the time needed to
> >> >> > run its driver's runtime resume callback.  That may hurt performance
> >> >> > in some situations, because user data may need to wait for the
> >> >> > device to become operational, so we should make it possible to
> >> >> > prevent that from happening.
> >> >> >
> >> >> > For this reason, introduce a new sysfs attribute for devices,
> >> >> > power/pm_qos_latency_us, allowing user space to specify the upper
> >> >> 
> >> >> If we're expecting to have more of these knobs, maybe having a pm_qos
> >> >> subdir under power will keep down the clutter in /sys/devices/.../power.
> >> >> This knob would then be /sys/devices/.../power/pm_qos/pm_qos_latency_us.
> >> >
> >> > I'm not sure how difficult it is to create a subdir in sysfs under something
> >> > that is not a kobject.
> >> >
> >> > Besides, this follows the convention already used by wakeup and runtime PM
> >> > attributes that don't have their own subdirs (although there may be a number
> >> > of them in each category).
> >> 
> >> OK
> >> 
> >> >> I think 'latency' alone is a bit too vague (wakeup latency?  interrupt
> >> >> latency?  I think wakeup latency is clearer.  Another possibility is
> >> >> resume latency, IMO, that will lead to confusion about whether this
> >> >> field also affects system suspend/resume.
> >> >
> >> > I think "wakeup latency" will lead to more confusion because of the
> >> > wakeup-related attributes.  
> >> 
> >> What confusion?  All of those are related to device wakeups from some
> >> low power state, and so is this proposed latency attribute.  So I don't
> >> understand the potential confusion.
> >
> > The word "wakeup" may refer to many different things, as well as the word
> > "resume". :-)
> 
> Yes, but what's the confusion in this case?
> 
> IMO, The existing /sys/devices/.../power/wakeup* meaning is the same
> meaning as as for the wakeup latency in this patch,

No, it is not.  They refer to system wakeup. :-)

> so I don't understand where the confusion would be.

See above. ;-)

Rafael

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Friday, March 09, 2012, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> > On Thursday, March 08, 2012, Kevin Hilman wrote:
>> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> >> 
>> >> > On Thursday, March 08, 2012, Kevin Hilman wrote:
>> >> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> >> >> 
>> >> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> >> >> >
>> >> >> > A runtime suspend of a device (e.g. an MMC controller) belonging to
>> >> >> > a power domain or, in a more complicated scenario, a runtime suspend
>> >> >> > of another device in the same power domain, may cause power to be
>> >> >> > removed from the entire domain.  In that case, the amount of time
>> >> >> > necessary to runtime-resume the given device (e.g. the MMC
>> >> >> > controller) is often substantially greater than the time needed to
>> >> >> > run its driver's runtime resume callback.  That may hurt performance
>> >> >> > in some situations, because user data may need to wait for the
>> >> >> > device to become operational, so we should make it possible to
>> >> >> > prevent that from happening.
>> >> >> >
>> >> >> > For this reason, introduce a new sysfs attribute for devices,
>> >> >> > power/pm_qos_latency_us, allowing user space to specify the upper
>> >> >> 
>> >> >> If we're expecting to have more of these knobs, maybe having a pm_qos
>> >> >> subdir under power will keep down the clutter in /sys/devices/.../power.
>> >> >> This knob would then be /sys/devices/.../power/pm_qos/pm_qos_latency_us.
>> >> >
>> >> > I'm not sure how difficult it is to create a subdir in sysfs under something
>> >> > that is not a kobject.
>> >> >
>> >> > Besides, this follows the convention already used by wakeup and runtime PM
>> >> > attributes that don't have their own subdirs (although there may be a number
>> >> > of them in each category).
>> >> 
>> >> OK
>> >> 
>> >> >> I think 'latency' alone is a bit too vague (wakeup latency?  interrupt
>> >> >> latency?  I think wakeup latency is clearer.  Another possibility is
>> >> >> resume latency, IMO, that will lead to confusion about whether this
>> >> >> field also affects system suspend/resume.
>> >> >
>> >> > I think "wakeup latency" will lead to more confusion because of the
>> >> > wakeup-related attributes.  
>> >> 
>> >> What confusion?  All of those are related to device wakeups from some
>> >> low power state, and so is this proposed latency attribute.  So I don't
>> >> understand the potential confusion.
>> >
>> > The word "wakeup" may refer to many different things, as well as the word
>> > "resume". :-)
>> 
>> Yes, but what's the confusion in this case?
>> 
>> IMO, The existing /sys/devices/.../power/wakeup* meaning is the same
>> meaning as as for the wakeup latency in this patch,
>
> No, it is not.  They refer to system wakeup. :-)

OK, now I'm confused (again).  I thought those could be used for runtime
PM wakeups also.  At least I was planning on using them for any kind of
wakeup.

>> so I don't understand where the confusion would be.
>
> See above. ;-)

Sheesh, this is getting ugly.

So wakeup*  attributes refer to system resume and resume* attribues
refer to runtime PM.

Yuck.

Kevin

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Alan Stern]

On Thu, 8 Mar 2012, Kevin Hilman wrote:

> >> > The word "wakeup" may refer to many different things, as well as the word
> >> > "resume". :-)
> >> 
> >> Yes, but what's the confusion in this case?
> >> 
> >> IMO, The existing /sys/devices/.../power/wakeup* meaning is the same
> >> meaning as as for the wakeup latency in this patch,
> >
> > No, it is not.  They refer to system wakeup. :-)
> 
> OK, now I'm confused (again).  I thought those could be used for runtime
> PM wakeups also.  At least I was planning on using them for any kind of
> wakeup.
> 
> >> so I don't understand where the confusion would be.
> >
> > See above. ;-)
> 
> Sheesh, this is getting ugly.
> 
> So wakeup*  attributes refer to system resume and resume* attribues
> refer to runtime PM.
> 
> Yuck.

How about calling it "runtime latency"?  Or "runtime wakeup latency" in 
case people think there might be some other sort of latency associated 
with runtime power management.

Alan Stern

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

[...]

> How about calling it "runtime latency"?  Or "runtime wakeup latency" in 
> case people think there might be some other sort of latency associated 
> with runtime power management.

Either is better than just latency, but I would vote for runtime wakeup
latency.

Kevin

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

On Friday, March 09, 2012, Kevin Hilman wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> [...]
> 
> > How about calling it "runtime latency"?  Or "runtime wakeup latency" in 
> > case people think there might be some other sort of latency associated 
> > with runtime power management.
> 
> Either is better than just latency, but I would vote for runtime wakeup
> latency.

Well, that would be pm_qos_runtime_wakeup_latency_us.  Kind of long, IMHO.
Apart from this "wakeup" may be thought to refer to "remote wakeup", which
is when a device is resumed as a result of an external signal.

pm_qos_resume_latency_us is shorter and since it is referred to in the
documentation as "resume latency", I don't see any problems with that name.

Thanks,
Rafael

Re: [PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Friday, March 09, 2012, Kevin Hilman wrote:
>> Alan Stern <stern@rowland.harvard.edu> writes:
>> 
>> [...]
>> 
>> > How about calling it "runtime latency"?  Or "runtime wakeup latency" in 
>> > case people think there might be some other sort of latency associated 
>> > with runtime power management.
>> 
>> Either is better than just latency, but I would vote for runtime wakeup
>> latency.
>
> Well, that would be pm_qos_runtime_wakeup_latency_us.  Kind of long, IMHO.

Yeah, the long names are why I initially suggested having a 'qos'
subdir.  

> Apart from this "wakeup" may be thought to refer to "remote wakeup", which
> is when a device is resumed as a result of an external signal.
>
> pm_qos_resume_latency_us is shorter and since it is referred to in the
> documentation as "resume latency", I don't see any problems with that name.

OK

Kevin

[PATCH 2/3] tmio_mmc / PM: Use PM QoS latency constraint

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make tmio_mmc call dev_pm_qos_expose_latency_limit() to expose
the PM QoS latency limit to user space and specify the initial
value of it as 100 microseconds.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/mmc/host/tmio_mmc_pio.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux/drivers/mmc/host/tmio_mmc_pio.c
===================================================================
--- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
+++ linux/drivers/mmc/host/tmio_mmc_pio.c
@@ -39,6 +39,7 @@
 #include <linux/module.h>
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
+#include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/scatterlist.h>
 #include <linux/spinlock.h>
@@ -955,6 +956,8 @@ int __devinit tmio_mmc_host_probe(struct
 
 	mmc_add_host(mmc);
 
+	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
+
 	/* Unmask the IRQs we want to know about */
 	if (!_host->chan_rx)
 		irq_mask |= TMIO_MASK_READOP;
@@ -993,6 +996,8 @@ void tmio_mmc_host_remove(struct tmio_mm
 		|| host->mmc->caps & MMC_CAP_NONREMOVABLE)
 		pm_runtime_get_sync(&pdev->dev);
 
+	dev_pm_qos_hide_latency_limit(&pdev->dev);
+
 	mmc_remove_host(host->mmc);
 	cancel_work_sync(&host->done);
 	cancel_delayed_work_sync(&host->delayed_reset_work);

Re: [PATCH 2/3] tmio_mmc / PM: Use PM QoS latency constraint

[Adrian Hunter]

On 08/03/12 01:29, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Make tmio_mmc call dev_pm_qos_expose_latency_limit() to expose
> the PM QoS latency limit to user space and specify the initial
> value of it as 100 microseconds.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/mmc/host/tmio_mmc_pio.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> Index: linux/drivers/mmc/host/tmio_mmc_pio.c
> ===================================================================
> --- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
> +++ linux/drivers/mmc/host/tmio_mmc_pio.c
> @@ -39,6 +39,7 @@
>  #include <linux/module.h>
>  #include <linux/pagemap.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_qos.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/scatterlist.h>
>  #include <linux/spinlock.h>
> @@ -955,6 +956,8 @@ int __devinit tmio_mmc_host_probe(struct
>  
>  	mmc_add_host(mmc);
>  
> +	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
> +
>  	/* Unmask the IRQs we want to know about */
>  	if (!_host->chan_rx)
>  		irq_mask |= TMIO_MASK_READOP;
> @@ -993,6 +996,8 @@ void tmio_mmc_host_remove(struct tmio_mm
>  		|| host->mmc->caps & MMC_CAP_NONREMOVABLE)
>  		pm_runtime_get_sync(&pdev->dev);
>  
> +	dev_pm_qos_hide_latency_limit(&pdev->dev);

Is it really necessary to hide the latency limit before destroying the
device?  Presumably QoS code could (or does) take care of it.

> +
>  	mmc_remove_host(host->mmc);
>  	cancel_work_sync(&host->done);
>  	cancel_delayed_work_sync(&host->delayed_reset_work);
> 
> 

Re: [PATCH 2/3] tmio_mmc / PM: Use PM QoS latency constraint

[Rafael J. Wysocki]

On Thursday, March 08, 2012, Adrian Hunter wrote:
> On 08/03/12 01:29, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Make tmio_mmc call dev_pm_qos_expose_latency_limit() to expose
> > the PM QoS latency limit to user space and specify the initial
> > value of it as 100 microseconds.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/mmc/host/tmio_mmc_pio.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > Index: linux/drivers/mmc/host/tmio_mmc_pio.c
> > ===================================================================
> > --- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
> > +++ linux/drivers/mmc/host/tmio_mmc_pio.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/module.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_qos.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/spinlock.h>
> > @@ -955,6 +956,8 @@ int __devinit tmio_mmc_host_probe(struct
> >  
> >  	mmc_add_host(mmc);
> >  
> > +	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
> > +
> >  	/* Unmask the IRQs we want to know about */
> >  	if (!_host->chan_rx)
> >  		irq_mask |= TMIO_MASK_READOP;
> > @@ -993,6 +996,8 @@ void tmio_mmc_host_remove(struct tmio_mm
> >  		|| host->mmc->caps & MMC_CAP_NONREMOVABLE)
> >  		pm_runtime_get_sync(&pdev->dev);
> >  
> > +	dev_pm_qos_hide_latency_limit(&pdev->dev);
> 
> Is it really necessary to hide the latency limit before destroying the
> device?  Presumably QoS code could (or does) take care of it.

Yes, it can do that.  I'll modify patch [1/3] this way.

Thanks,
Rafael

[PATCH 3/3] sh_mmcif / PM: Use PM QoS latency constraint

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make sh_mmcif call dev_pm_qos_expose_latency_limit() to expose
the PM QoS latency limit to user space and specify the initial
value of it as 100 microseconds.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/mmc/host/sh_mmcif.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux/drivers/mmc/host/sh_mmcif.c
===================================================================
--- linux.orig/drivers/mmc/host/sh_mmcif.c
+++ linux/drivers/mmc/host/sh_mmcif.c
@@ -56,6 +56,7 @@
 #include <linux/mmc/sh_mmcif.h>
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
+#include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/spinlock.h>
 #include <linux/module.h>
@@ -1346,6 +1347,8 @@ static int __devinit sh_mmcif_probe(stru
 	if (ret < 0)
 		goto clean_up5;
 
+	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
+
 	dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
 	dev_dbg(&pdev->dev, "chip ver H'%04x\n",
 		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
@@ -1376,6 +1379,8 @@ static int __devexit sh_mmcif_remove(str
 	host->dying = true;
 	pm_runtime_get_sync(&pdev->dev);
 
+	dev_pm_qos_hide_latency_limit(&pdev->dev);
+
 	mmc_remove_host(host->mmc);
 	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
 

Re: [PATCH 3/3] sh_mmcif / PM: Use PM QoS latency constraint

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

On Thu, Mar 08, 2012 at 12:30:28AM +0100, Rafael J. Wysocki wrote:

> +	dev_pm_qos_hide_latency_limit(&pdev->dev);
> +

Shouldn't the core be able to cope with cleaning things up by itself
here?  It seems like it'd be easier/less error prone to do that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] sh_mmcif / PM: Use PM QoS latency constraint

[Rafael J. Wysocki]

On Thursday, March 08, 2012, Mark Brown wrote:
> On Thu, Mar 08, 2012 at 12:30:28AM +0100, Rafael J. Wysocki wrote:
> 
> > +	dev_pm_qos_hide_latency_limit(&pdev->dev);
> > +
> 
> Shouldn't the core be able to cope with cleaning things up by itself
> here?  It seems like it'd be easier/less error prone to do that.

Yes, I think this is a fairly reasonable expectation. :-)

[Update][PATCH 0/3] PM: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

Hi all,

On Thursday, March 08, 2012, Rafael J. Wysocki wrote:
> On Sunday, March 04, 2012, Rafael J. Wysocki wrote:
> > On Sunday, March 04, 2012, Rafael J. Wysocki wrote:
> > > 
> > > The goal of this patchset is to allow user space to control the
> > > responsiveness of the MMC stack related to runtime power management.
> > > 
> > > Namely, on systems that contain power domains, the time necessary
> > > to bring an MMC host up after it has been runtime-suspended may
> > > depend not only on the MMC core and host driver, but also on the platform
> > > and other devices in the same power domain(s) that contain(s) the MMC
> > > host.  Although that obviously may influence the MMC performance,
> > > currently, there is no way to control it through the MMC subsystem.
> > > Moreover, the only available way to control it at all is by using PM QoS
> > > latency requests, so it is necessary to add some kind of support for that
> > > to MMC.
> > > 
> > > Patch [1/3] adds PM QoS-related fields to struct mmc_host and introduces
> > > a new sysfs attribute for MMC hosts, pm_latency_limit_ms, allowing user
> > > space to influence the MMC host's runtime PM via PM QoS.  Whether or not
> > > this attribute will be created (and PM QoS will be used for the given host)
> > > depends on the host driver, so host drivers that don't (plan) to support
> > > PM QoS don't need to do anything about that.
> > > 
> > > Patches [2/3] and [3/3] add the corresponding host driver bits to the
> > > tmio_mmc and sh_mmcif drivers, respectively.
> > 
> > After posting the patches I noticed that the changelog of patch [1/3] and
> > the documentation of the new sysfs attribute were not 100% accurate, because
> > the PM QoS request really applies to the time between a resume request and
> > the moment the device is operational again and not the time from runtime
> > suspend (the latter would imply some kind of autoresume mechanism, which isn't
> > there).
> > 
> > I also thought it would be cleaner to allocate the val and attr fields along
> > with the request, because in the previous version of the patchset they weren't
> > used when req was NULL, resulting in (a little) wasted memory.
> > 
> > Updated patches follow.
> 
> Taking the feedback so far into account, I decided to move the exposing of the
> PM QoS latency limit from the MMC layer to the PM core sysfs code, so that
> every driver (not only MMC host drivers) can use it.
> 
> New patches follow, details are in the changelogs:
> 
> [1/3] - Make it possible to expose PM QoS latency constraints
> [2/3] - Make tmio_mmc use the new interface.
> [3/3] - Make sh_mmcif use the new interface.

Taking the latest feedback into account I reworked patch [1/3] in the
following way:

- The new attribute is now called pm_qos_resume_latency_us.
- It depends on CONFIG_PM_RUNTIME and the documentation says it doesn't has an
  effect on system-wide suspend/resume.
- The new attribute is hidden automatically by dev_pm_qos_constraints_destroy()
  if still exposed at this point, so the drivers that exposed it don't have to
  hide it explicitly on removal (although they still can do that).

However, I still think that it is appropriate for the MMC drivers to hide it
on _their_ removal, because otherwise the device would be left with the
meaningless attribute after that point.  So, I haven't modified patches [2-3/3],
but they follow for completness.

I've added the Reviewed-by tags from Kevin and Mark to patch [1/3], becuase
the changes made generally follow their suggestions.

Thanks,
Rafael

[Update][PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

A runtime suspend of a device (e.g. an MMC controller) belonging to
a power domain or, in a more complicated scenario, a runtime suspend
of another device in the same power domain, may cause power to be
removed from the entire domain.  In that case, the amount of time
necessary to runtime-resume the given device (e.g. the MMC
controller) is often substantially greater than the time needed to
run its driver's runtime resume callback.  That may hurt performance
in some situations, because user data may need to wait for the
device to become operational, so we should make it possible to
prevent that from happening.

For this reason, introduce a new sysfs attribute for devices,
power/pm_qos_resume_latency_us, allowing user space to specify the
upper bound of the time necessary to bring the (runtime-suspended)
device up after the resume of it has been requested.  However, make
that attribute appear only for the devices whose drivers declare
support for it by calling the (new) dev_pm_qos_expose_latency_limit()
helper function with the appropriate initial value of the attribute.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reviewed-by: Kevin Hilman <khilman@ti.com>
Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 Documentation/ABI/testing/sysfs-devices-power |   18 +++++++
 drivers/base/power/power.h                    |    4 +
 drivers/base/power/qos.c                      |   62 ++++++++++++++++++++++++++
 drivers/base/power/sysfs.c                    |   47 +++++++++++++++++++
 include/linux/pm.h                            |    1 
 include/linux/pm_qos.h                        |    9 +++
 6 files changed, 141 insertions(+)

Index: linux/include/linux/pm.h
===================================================================
--- linux.orig/include/linux/pm.h
+++ linux/include/linux/pm.h
@@ -546,6 +546,7 @@ struct dev_pm_info {
 	unsigned long		accounting_timestamp;
 	ktime_t			suspend_time;
 	s64			max_time_suspended_ns;
+	struct dev_pm_qos_request *pq_req;
 #endif
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	struct pm_qos_constraints *constraints;
Index: linux/drivers/base/power/sysfs.c
===================================================================
--- linux.orig/drivers/base/power/sysfs.c
+++ linux/drivers/base/power/sysfs.c
@@ -5,6 +5,7 @@
 #include <linux/device.h>
 #include <linux/string.h>
 #include <linux/export.h>
+#include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/atomic.h>
 #include <linux/jiffies.h>
@@ -217,6 +218,31 @@ static ssize_t autosuspend_delay_ms_stor
 static DEVICE_ATTR(autosuspend_delay_ms, 0644, autosuspend_delay_ms_show,
 		autosuspend_delay_ms_store);
 
+static ssize_t pm_qos_latency_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", dev->power.pq_req->node.prio);
+}
+
+static ssize_t pm_qos_latency_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t n)
+{
+	s32 value;
+	int ret;
+
+	if (kstrtos32(buf, 0, &value))
+		return -EINVAL;
+
+	if (value < 0)
+		return -EINVAL;
+
+	ret = dev_pm_qos_update_request(dev->power.pq_req, value);
+	return ret < 0 ? ret : n;
+}
+
+static DEVICE_ATTR(pm_qos_resume_latency_us, 0644,
+		   pm_qos_latency_show, pm_qos_latency_store);
 #endif /* CONFIG_PM_RUNTIME */
 
 #ifdef CONFIG_PM_SLEEP
@@ -510,6 +536,17 @@ static struct attribute_group pm_runtime
 	.attrs	= runtime_attrs,
 };
 
+static struct attribute *pm_qos_attrs[] = {
+#ifdef CONFIG_PM_RUNTIME
+	&dev_attr_pm_qos_resume_latency_us.attr,
+#endif /* CONFIG_PM_RUNTIME */
+	NULL,
+};
+static struct attribute_group pm_qos_attr_group = {
+	.name	= power_group_name,
+	.attrs	= pm_qos_attrs,
+};
+
 int dpm_sysfs_add(struct device *dev)
 {
 	int rc;
@@ -550,6 +587,16 @@ void wakeup_sysfs_remove(struct device *
 	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
 }
 
+int pm_qos_sysfs_add(struct device *dev)
+{
+	return sysfs_merge_group(&dev->kobj, &pm_qos_attr_group);
+}
+
+void pm_qos_sysfs_remove(struct device *dev)
+{
+	sysfs_unmerge_group(&dev->kobj, &pm_qos_attr_group);
+}
+
 void rpm_sysfs_remove(struct device *dev)
 {
 	sysfs_unmerge_group(&dev->kobj, &pm_runtime_attr_group);
Index: linux/drivers/base/power/power.h
===================================================================
--- linux.orig/drivers/base/power/power.h
+++ linux/drivers/base/power/power.h
@@ -71,6 +71,8 @@ extern void dpm_sysfs_remove(struct devi
 extern void rpm_sysfs_remove(struct device *dev);
 extern int wakeup_sysfs_add(struct device *dev);
 extern void wakeup_sysfs_remove(struct device *dev);
+extern int pm_qos_sysfs_add(struct device *dev);
+extern void pm_qos_sysfs_remove(struct device *dev);
 
 #else /* CONFIG_PM */
 
@@ -79,5 +81,7 @@ static inline void dpm_sysfs_remove(stru
 static inline void rpm_sysfs_remove(struct device *dev) {}
 static inline int wakeup_sysfs_add(struct device *dev) { return 0; }
 static inline void wakeup_sysfs_remove(struct device *dev) {}
+static inline int pm_qos_sysfs_add(struct device *dev) { return 0; }
+static inline void pm_qos_sysfs_remove(struct device *dev) {}
 
 #endif
Index: linux/drivers/base/power/qos.c
===================================================================
--- linux.orig/drivers/base/power/qos.c
+++ linux/drivers/base/power/qos.c
@@ -41,6 +41,7 @@
 #include <linux/mutex.h>
 #include <linux/export.h>
 
+#include "power.h"
 
 static DEFINE_MUTEX(dev_pm_qos_mtx);
 
@@ -166,6 +167,12 @@ void dev_pm_qos_constraints_destroy(stru
 	struct dev_pm_qos_request *req, *tmp;
 	struct pm_qos_constraints *c;
 
+	/*
+	 * If the device's PM QoS resume latency limit has been exposed to user
+	 * space, it has to be hidden at this point.
+	 */
+	dev_pm_qos_hide_latency_limit(dev);
+
 	mutex_lock(&dev_pm_qos_mtx);
 
 	dev->power.power_state = PMSG_INVALID;
@@ -445,3 +452,58 @@ int dev_pm_qos_add_ancestor_request(stru
 	return error;
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_add_ancestor_request);
+
+#ifdef CONFIG_PM_RUNTIME
+static void __dev_pm_qos_drop_user_request(struct device *dev)
+{
+	dev_pm_qos_remove_request(dev->power.pq_req);
+	dev->power.pq_req = 0;
+}
+
+/**
+ * dev_pm_qos_expose_latency_limit - Expose PM QoS latency limit to user space.
+ * @dev: Device whose PM QoS latency limit is to be exposed to user space.
+ * @value: Initial value of the latency limit.
+ */
+int dev_pm_qos_expose_latency_limit(struct device *dev, s32 value)
+{
+	struct dev_pm_qos_request *req;
+	int ret;
+
+	if (!device_is_registered(dev) || value < 0)
+		return -EINVAL;
+
+	if (dev->power.pq_req)
+		return -EEXIST;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	ret = dev_pm_qos_add_request(dev, req, value);
+	if (ret < 0)
+		return ret;
+
+	dev->power.pq_req = req;
+	ret = pm_qos_sysfs_add(dev);
+	if (ret)
+		__dev_pm_qos_drop_user_request(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_qos_expose_latency_limit);
+
+/**
+ * dev_pm_qos_hide_latency_limit - Hide PM QoS latency limit from user space.
+ * @dev: Device whose PM QoS latency limit is to be hidden from user space.
+ */
+void dev_pm_qos_hide_latency_limit(struct device *dev)
+{
+	if (dev->power.pq_req) {
+		pm_qos_sysfs_remove(dev);
+		__dev_pm_qos_drop_user_request(dev);
+	}
+}
+EXPORT_SYMBOL_GPL(dev_pm_qos_hide_latency_limit);
+#endif /* CONFIG_PM_RUNTIME */
+
Index: linux/include/linux/pm_qos.h
===================================================================
--- linux.orig/include/linux/pm_qos.h
+++ linux/include/linux/pm_qos.h
@@ -137,4 +137,13 @@ static inline int dev_pm_qos_add_ancesto
 			{ return 0; }
 #endif
 
+#ifdef CONFIG_PM_RUNTIME
+int dev_pm_qos_expose_latency_limit(struct device *dev, s32 value);
+void dev_pm_qos_hide_latency_limit(struct device *dev);
+#else
+static inline int dev_pm_qos_expose_latency_limit(struct device *dev, s32 value)
+			{ return 0; }
+static inline void dev_pm_qos_hide_latency_limit(struct device *dev) {}
+#endif
+
 #endif
Index: linux/Documentation/ABI/testing/sysfs-devices-power
===================================================================
--- linux.orig/Documentation/ABI/testing/sysfs-devices-power
+++ linux/Documentation/ABI/testing/sysfs-devices-power
@@ -175,3 +175,21 @@ Description:
 
 		Not all drivers support this attribute.  If it isn't supported,
 		attempts to read or write it will yield I/O errors.
+
+What:		/sys/devices/.../power/pm_qos_latency_us
+Date:		March 2012
+Contact:	Rafael J. Wysocki <rjw@sisk.pl>
+Description:
+		The /sys/devices/.../power/pm_qos_resume_latency_us attribute
+		contains the PM QoS resume latency limit for the given device,
+		which is the maximum allowed time it can take to resume the
+		device, after it has been suspended at run time, from a resume
+		request to the moment the device will be ready to process I/O,
+		in microseconds.  If it is equal to 0, however, this means that
+		the PM QoS resume latency may be arbitrary.
+
+		Not all drivers support this attribute.  If it isn't supported,
+		it is not present.
+
+		This attribute has no effect on system-wide suspend/resume and
+		hibernation.

Re: [Update][PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Linus Walleij]

On Fri, Mar 9, 2012 at 12:01 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:

> A runtime suspend of a device (e.g. an MMC controller) belonging to
> a power domain or, in a more complicated scenario, a runtime suspend
> of another device in the same power domain, may cause power to be
> removed from the entire domain.  In that case, the amount of time
> necessary to runtime-resume the given device (e.g. the MMC
> controller) is often substantially greater than the time needed to
> run its driver's runtime resume callback.  That may hurt performance
> in some situations, because user data may need to wait for the
> device to become operational, so we should make it possible to
> prevent that from happening.
>
> For this reason, introduce a new sysfs attribute for devices,
> power/pm_qos_resume_latency_us, allowing user space to specify the
> upper bound of the time necessary to bring the (runtime-suspended)
> device up after the resume of it has been requested.  However, make
> that attribute appear only for the devices whose drivers declare
> support for it by calling the (new) dev_pm_qos_expose_latency_limit()
> helper function with the appropriate initial value of the attribute.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

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

Yours,
Linus Walleij

Re: [Update][PATCH 1/3] PM / QoS: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

On Monday, March 12, 2012, Linus Walleij wrote:
> On Fri, Mar 9, 2012 at 12:01 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > A runtime suspend of a device (e.g. an MMC controller) belonging to
> > a power domain or, in a more complicated scenario, a runtime suspend
> > of another device in the same power domain, may cause power to be
> > removed from the entire domain.  In that case, the amount of time
> > necessary to runtime-resume the given device (e.g. the MMC
> > controller) is often substantially greater than the time needed to
> > run its driver's runtime resume callback.  That may hurt performance
> > in some situations, because user data may need to wait for the
> > device to become operational, so we should make it possible to
> > prevent that from happening.
> >
> > For this reason, introduce a new sysfs attribute for devices,
> > power/pm_qos_resume_latency_us, allowing user space to specify the
> > upper bound of the time necessary to bring the (runtime-suspended)
> > device up after the resume of it has been requested.  However, make
> > that attribute appear only for the devices whose drivers declare
> > support for it by calling the (new) dev_pm_qos_expose_latency_limit()
> > helper function with the appropriate initial value of the attribute.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > Reviewed-by: Kevin Hilman <khilman@ti.com>
> > Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

Rafael

[Update][PATCH 2/3] tmio_mmc / PM: Use PM QoS latency constraint

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make tmio_mmc call dev_pm_qos_expose_latency_limit() to expose
the PM QoS latency limit to user space and specify the initial
value of it as 100 microseconds.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/mmc/host/tmio_mmc_pio.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux/drivers/mmc/host/tmio_mmc_pio.c
===================================================================
--- linux.orig/drivers/mmc/host/tmio_mmc_pio.c
+++ linux/drivers/mmc/host/tmio_mmc_pio.c
@@ -39,6 +39,7 @@
 #include <linux/module.h>
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
+#include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/scatterlist.h>
 #include <linux/spinlock.h>
@@ -955,6 +956,8 @@ int __devinit tmio_mmc_host_probe(struct
 
 	mmc_add_host(mmc);
 
+	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
+
 	/* Unmask the IRQs we want to know about */
 	if (!_host->chan_rx)
 		irq_mask |= TMIO_MASK_READOP;
@@ -993,6 +996,8 @@ void tmio_mmc_host_remove(struct tmio_mm
 		|| host->mmc->caps & MMC_CAP_NONREMOVABLE)
 		pm_runtime_get_sync(&pdev->dev);
 
+	dev_pm_qos_hide_latency_limit(&pdev->dev);
+
 	mmc_remove_host(host->mmc);
 	cancel_work_sync(&host->done);
 	cancel_delayed_work_sync(&host->delayed_reset_work);

[Update][PATCH 3/3] sh_mmcif / PM: Use PM QoS latency constraint

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make sh_mmcif call dev_pm_qos_expose_latency_limit() to expose
the PM QoS latency limit to user space and specify the initial
value of it as 100 microseconds.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/mmc/host/sh_mmcif.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux/drivers/mmc/host/sh_mmcif.c
===================================================================
--- linux.orig/drivers/mmc/host/sh_mmcif.c
+++ linux/drivers/mmc/host/sh_mmcif.c
@@ -56,6 +56,7 @@
 #include <linux/mmc/sh_mmcif.h>
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
+#include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/spinlock.h>
 #include <linux/module.h>
@@ -1346,6 +1347,8 @@ static int __devinit sh_mmcif_probe(stru
 	if (ret < 0)
 		goto clean_up5;
 
+	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
+
 	dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
 	dev_dbg(&pdev->dev, "chip ver H'%04x\n",
 		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
@@ -1376,6 +1379,8 @@ static int __devexit sh_mmcif_remove(str
 	host->dying = true;
 	pm_runtime_get_sync(&pdev->dev);
 
+	dev_pm_qos_hide_latency_limit(&pdev->dev);
+
 	mmc_remove_host(host->mmc);
 	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
 

Re: [Update][PATCH 0/3] PM: Make it possible to expose PM QoS latency constraints

[Rafael J. Wysocki]

On Friday, March 09, 2012, Rafael J. Wysocki wrote:
> Hi all,
> 
> On Thursday, March 08, 2012, Rafael J. Wysocki wrote:
> > On Sunday, March 04, 2012, Rafael J. Wysocki wrote:
> > > On Sunday, March 04, 2012, Rafael J. Wysocki wrote:
> > > > 
> > > > The goal of this patchset is to allow user space to control the
> > > > responsiveness of the MMC stack related to runtime power management.
> > > > 
> > > > Namely, on systems that contain power domains, the time necessary
> > > > to bring an MMC host up after it has been runtime-suspended may
> > > > depend not only on the MMC core and host driver, but also on the platform
> > > > and other devices in the same power domain(s) that contain(s) the MMC
> > > > host.  Although that obviously may influence the MMC performance,
> > > > currently, there is no way to control it through the MMC subsystem.
> > > > Moreover, the only available way to control it at all is by using PM QoS
> > > > latency requests, so it is necessary to add some kind of support for that
> > > > to MMC.
> > > > 
> > > > Patch [1/3] adds PM QoS-related fields to struct mmc_host and introduces
> > > > a new sysfs attribute for MMC hosts, pm_latency_limit_ms, allowing user
> > > > space to influence the MMC host's runtime PM via PM QoS.  Whether or not
> > > > this attribute will be created (and PM QoS will be used for the given host)
> > > > depends on the host driver, so host drivers that don't (plan) to support
> > > > PM QoS don't need to do anything about that.
> > > > 
> > > > Patches [2/3] and [3/3] add the corresponding host driver bits to the
> > > > tmio_mmc and sh_mmcif drivers, respectively.
> > > 
> > > After posting the patches I noticed that the changelog of patch [1/3] and
> > > the documentation of the new sysfs attribute were not 100% accurate, because
> > > the PM QoS request really applies to the time between a resume request and
> > > the moment the device is operational again and not the time from runtime
> > > suspend (the latter would imply some kind of autoresume mechanism, which isn't
> > > there).
> > > 
> > > I also thought it would be cleaner to allocate the val and attr fields along
> > > with the request, because in the previous version of the patchset they weren't
> > > used when req was NULL, resulting in (a little) wasted memory.
> > > 
> > > Updated patches follow.
> > 
> > Taking the feedback so far into account, I decided to move the exposing of the
> > PM QoS latency limit from the MMC layer to the PM core sysfs code, so that
> > every driver (not only MMC host drivers) can use it.
> > 
> > New patches follow, details are in the changelogs:
> > 
> > [1/3] - Make it possible to expose PM QoS latency constraints
> > [2/3] - Make tmio_mmc use the new interface.
> > [3/3] - Make sh_mmcif use the new interface.
> 
> Taking the latest feedback into account I reworked patch [1/3] in the
> following way:
> 
> - The new attribute is now called pm_qos_resume_latency_us.

It seems that the name of the new attribute has been agreed on, so I wonder
if anyone still has any problems with patches [1/3]?  If not and if there is
v3.3-rc7, I'd like to include them into my v3.4 material.

> - It depends on CONFIG_PM_RUNTIME and the documentation says it doesn't has an
>   effect on system-wide suspend/resume.
> - The new attribute is hidden automatically by dev_pm_qos_constraints_destroy()
>   if still exposed at this point, so the drivers that exposed it don't have to
>   hide it explicitly on removal (although they still can do that).
> 
> However, I still think that it is appropriate for the MMC drivers to hide it
> on _their_ removal, because otherwise the device would be left with the
> meaningless attribute after that point.  So, I haven't modified patches [2-3/3],
> but they follow for completness.
> 
> I've added the Reviewed-by tags from Kevin and Mark to patch [1/3], becuase
> the changes made generally follow their suggestions.

Thanks,
Rafael

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Adrian Hunter]

On 04/03/12 02:01, Rafael J. Wysocki wrote:
> Hi all,
> 
> The goal of this patchset is to allow user space to control the
> responsiveness of the MMC stack related to runtime power management.

I wonder why this is build into mmc and not just a generic runtime pm
facility. e.g.

	/* Set maximum resume latency target to 100ms */
	pm_runtime_set_max_latency(dev, 100);

And then runtime pm will create sysfs attributes etc

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Guennadi Liakhovetski]

On Tue, 6 Mar 2012, Adrian Hunter wrote:

> On 04/03/12 02:01, Rafael J. Wysocki wrote:
> > Hi all,
> > 
> > The goal of this patchset is to allow user space to control the
> > responsiveness of the MMC stack related to runtime power management.
> 
> I wonder why this is build into mmc and not just a generic runtime pm
> facility. e.g.
> 
> 	/* Set maximum resume latency target to 100ms */
> 	pm_runtime_set_max_latency(dev, 100);
> 
> And then runtime pm will create sysfs attributes etc

+1. Having to do essentially exactly the same for each driver subsystem 
seems counterproductive.

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

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
> On Tue, 6 Mar 2012, Adrian Hunter wrote:
> 
> > On 04/03/12 02:01, Rafael J. Wysocki wrote:
> > > Hi all,
> > > 
> > > The goal of this patchset is to allow user space to control the
> > > responsiveness of the MMC stack related to runtime power management.
> > 
> > I wonder why this is build into mmc and not just a generic runtime pm
> > facility. e.g.

Because of the user space interface (it doesn't necessarily make sense
for all devices) and to allow drivers to opt-in (if they don't, the interface
won't be created), which is not possible at the core level (we don't know in
advance what drivers will handle the given devices and if they will support
PM QoS).

> > 	/* Set maximum resume latency target to 100ms */
> > 	pm_runtime_set_max_latency(dev, 100);
> > 
> > And then runtime pm will create sysfs attributes etc
> 
> +1. Having to do essentially exactly the same for each driver subsystem 
> seems counterproductive.

Other subsystems need not do that in the same way.

Thanks,
Rafael

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Adrian Hunter]

On 06/03/12 23:14, Rafael J. Wysocki wrote:
> On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
>> On Tue, 6 Mar 2012, Adrian Hunter wrote:
>>
>>> On 04/03/12 02:01, Rafael J. Wysocki wrote:
>>>> Hi all,
>>>>
>>>> The goal of this patchset is to allow user space to control the
>>>> responsiveness of the MMC stack related to runtime power management.
>>>
>>> I wonder why this is build into mmc and not just a generic runtime pm
>>> facility. e.g.
> 
> Because of the user space interface (it doesn't necessarily make sense
> for all devices) and to allow drivers to opt-in (if they don't, the interface
> won't be created), which is not possible at the core level (we don't know in
> advance what drivers will handle the given devices and if they will support
> PM QoS).

Even "opt-in" is undesirable, because it is really up to userspace not the
driver.

> 
>>> 	/* Set maximum resume latency target to 100ms */
>>> 	pm_runtime_set_max_latency(dev, 100);
>>>
>>> And then runtime pm will create sysfs attributes etc
>>
>> +1. Having to do essentially exactly the same for each driver subsystem 
>> seems counterproductive.
> 
> Other subsystems need not do that in the same way.

Maybe this needs to be re-thought.  Userspace needs a simple, consistent and
understandable set of pm controls across the entire kernel, not piecemeal
across different subsystems.

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

On Wednesday, March 07, 2012, Adrian Hunter wrote:
> On 06/03/12 23:14, Rafael J. Wysocki wrote:
> > On Tuesday, March 06, 2012, Guennadi Liakhovetski wrote:
> >> On Tue, 6 Mar 2012, Adrian Hunter wrote:
> >>
> >>> On 04/03/12 02:01, Rafael J. Wysocki wrote:
> >>>> Hi all,
> >>>>
> >>>> The goal of this patchset is to allow user space to control the
> >>>> responsiveness of the MMC stack related to runtime power management.
> >>>
> >>> I wonder why this is build into mmc and not just a generic runtime pm
> >>> facility. e.g.
> > 
> > Because of the user space interface (it doesn't necessarily make sense
> > for all devices) and to allow drivers to opt-in (if they don't, the interface
> > won't be created), which is not possible at the core level (we don't know in
> > advance what drivers will handle the given devices and if they will support
> > PM QoS).
> 
> Even "opt-in" is undesirable, because it is really up to userspace not the
> driver.
> 
> > 
> >>> 	/* Set maximum resume latency target to 100ms */
> >>> 	pm_runtime_set_max_latency(dev, 100);
> >>>
> >>> And then runtime pm will create sysfs attributes etc
> >>
> >> +1. Having to do essentially exactly the same for each driver subsystem 
> >> seems counterproductive.
> > 
> > Other subsystems need not do that in the same way.
> 
> Maybe this needs to be re-thought.  Userspace needs a simple, consistent and
> understandable set of pm controls across the entire kernel, not piecemeal
> across different subsystems.

Well, that's my opinion too, but other people don't seem to agree with it.

Thanks,
Rafael

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Wed, Mar 07, 2012 at 10:05:54AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, March 07, 2012, Adrian Hunter wrote:

> > Maybe this needs to be re-thought.  Userspace needs a simple, consistent and
> > understandable set of pm controls across the entire kernel, not piecemeal
> > across different subsystems.

> Well, that's my opinion too, but other people don't seem to agree with it.

Well, it depends on what the knob is.  Some things definitely make sense
being generic (like the existing support for disabling runtime PM) but
people have suggested stuff like fiddling with IRQ latencies which really 
seems far too detailed a thing to be twiddling from userspace and fairly
obscure in the system level impact so difficult for applications to take
advantage of usefully.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

>> Maybe this needs to be re-thought.  Userspace needs a simple, consistent and
>> understandable set of pm controls across the entire kernel, not piecemeal
>> across different subsystems.
>
> Well, that's my opinion too, but other people don't seem to agree with it.

I don't agree because when it comes to PM, subsystems can be quite
different in what they want to expose to userspace.  

IMO, it's the subsystems/drivers that should decide what to expose to
userspace for PM, just like they decide what gets exposed to userspace
for the rest of their functionality.

In other words, in my view, keeping PM knobs/controls outside the
management of the subsystem is creating a strange boundary for
userspace.  Applications have to do all their "normal" interactions with
the subsystem/driver, but for PM, they have to find the right sysfs
magic and twiddle that.    I would much rather see the
subsystems/drivers grow their own PM functionality and expose it to
userspace as they see fit.

One of the examples used to discuss this in the past has been the
touchscreen sample rate.  Touchscreens can save power by having a lower
sample rate at the expense of less precision.  For finger/thumb type
interface, a lower sample rate might be fine, but for handwriting
recognition with a stylus, a higher sample rate could be required.

Using a subsystem-generic (presumably sysfs-based) interface, the
application would be required to find the right sysfs magic in addition
to its interactions with tslib.  (is there really a generic "sampling
rate" knob that would make sense for all subsystems?)

To me it seems more logical for the touchscreen/input subystem to expose
this "sampling rate" knob in a subsystem-specific way to userspace,
which could then be handled by tslib.

Kevin

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

On Wednesday, March 07, 2012, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> >> Maybe this needs to be re-thought.  Userspace needs a simple, consistent and
> >> understandable set of pm controls across the entire kernel, not piecemeal
> >> across different subsystems.
> >
> > Well, that's my opinion too, but other people don't seem to agree with it.
> 
> I don't agree because when it comes to PM, subsystems can be quite
> different in what they want to expose to userspace.  
> 
> IMO, it's the subsystems/drivers that should decide what to expose to
> userspace for PM, just like they decide what gets exposed to userspace
> for the rest of their functionality.
> 
> In other words, in my view, keeping PM knobs/controls outside the
> management of the subsystem is creating a strange boundary for
> userspace.  Applications have to do all their "normal" interactions with
> the subsystem/driver, but for PM, they have to find the right sysfs
> magic and twiddle that.    I would much rather see the
> subsystems/drivers grow their own PM functionality and expose it to
> userspace as they see fit.
> 
> One of the examples used to discuss this in the past has been the
> touchscreen sample rate.  Touchscreens can save power by having a lower
> sample rate at the expense of less precision.  For finger/thumb type
> interface, a lower sample rate might be fine, but for handwriting
> recognition with a stylus, a higher sample rate could be required.
> 
> Using a subsystem-generic (presumably sysfs-based) interface, the
> application would be required to find the right sysfs magic in addition
> to its interactions with tslib.  (is there really a generic "sampling
> rate" knob that would make sense for all subsystems?)
> 
> To me it seems more logical for the touchscreen/input subystem to expose
> this "sampling rate" knob in a subsystem-specific way to userspace,
> which could then be handled by tslib.

That would be fine, but it doesn't _conflict_ with a more direct (so to
speak) knob in sysfs, does it?

Rafael

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Wed, Mar 07, 2012 at 09:51:07PM +0100, Rafael J. Wysocki wrote:

> That would be fine, but it doesn't _conflict_ with a more direct (so to
> speak) knob in sysfs, does it?

Well, perhaps - one of the issues I remember from last time we discussed
this was that there's a tension between the embedded space where the
idea of userspace charging around and playing with low level parameters
to make things hang together is scary and the PC space where the kernel
just isn't going to be able to cope with everyone's systems reliably as
it's got too little information about the hardware.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

On Wednesday, March 07, 2012, Mark Brown wrote:
> On Wed, Mar 07, 2012 at 09:51:07PM +0100, Rafael J. Wysocki wrote:
> 
> > That would be fine, but it doesn't _conflict_ with a more direct (so to
> > speak) knob in sysfs, does it?
> 
> Well, perhaps - one of the issues I remember from last time we discussed
> this was that there's a tension between the embedded space where the
> idea of userspace charging around and playing with low level parameters
> to make things hang together is scary and the PC space where the kernel
> just isn't going to be able to cope with everyone's systems reliably as
> it's got too little information about the hardware.

Definitely, on the PC side we would like user space to have access to
low-level settings.

Anyway, I suggest that we wait a bit until I post updated patches and then
you'll tell me why they are wrong, OK?

Rafael

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

On Tuesday, March 06, 2012, Adrian Hunter wrote:
> On 04/03/12 02:01, Rafael J. Wysocki wrote:
> > Hi all,
> > 
> > The goal of this patchset is to allow user space to control the
> > responsiveness of the MMC stack related to runtime power management.
> 
> I wonder why this is build into mmc and not just a generic runtime pm
> facility. e.g.
> 
> 	/* Set maximum resume latency target to 100ms */
> 	pm_runtime_set_max_latency(dev, 100);

That actually is

 	dev_pm_qos_add_request(dev, req, 100);

where req is used as a handle for your request (it may be used, for
example, to remove the request or update it).

> And then runtime pm will create sysfs attributes etc

Well, there may be an interface for drivers analogous to
device_wakeup_enable()/device_wakeup_disable() allowing them
to add/remove a sysfs attribute for user space to control
a single PM QoS constraint.  That even sounds like a good idea. :-)

Thanks,
Rafael

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Adrian Hunter]

On 06/03/12 23:47, Rafael J. Wysocki wrote:
> On Tuesday, March 06, 2012, Adrian Hunter wrote:
>> On 04/03/12 02:01, Rafael J. Wysocki wrote:
>>> Hi all,
>>>
>>> The goal of this patchset is to allow user space to control the
>>> responsiveness of the MMC stack related to runtime power management.
>>
>> I wonder why this is build into mmc and not just a generic runtime pm
>> facility. e.g.
>>
>> 	/* Set maximum resume latency target to 100ms */
>> 	pm_runtime_set_max_latency(dev, 100);
> 
> That actually is
> 
>  	dev_pm_qos_add_request(dev, req, 100);
> 
> where req is used as a handle for your request (it may be used, for
> example, to remove the request or update it).
> 
>> And then runtime pm will create sysfs attributes etc
> 
> Well, there may be an interface for drivers analogous to
> device_wakeup_enable()/device_wakeup_disable() allowing them
> to add/remove a sysfs attribute for user space to control
> a single PM QoS constraint.  That even sounds like a good idea. :-)

Does that mean you are going to change your patch?

Re: [PATCH 0/3] MMC / PM: Make it possible to use PM QoS latency constraints

[Rafael J. Wysocki]

On Wednesday, March 07, 2012, Adrian Hunter wrote:
> On 06/03/12 23:47, Rafael J. Wysocki wrote:
> > On Tuesday, March 06, 2012, Adrian Hunter wrote:
> >> On 04/03/12 02:01, Rafael J. Wysocki wrote:
> >>> Hi all,
> >>>
> >>> The goal of this patchset is to allow user space to control the
> >>> responsiveness of the MMC stack related to runtime power management.
> >>
> >> I wonder why this is build into mmc and not just a generic runtime pm
> >> facility. e.g.
> >>
> >> 	/* Set maximum resume latency target to 100ms */
> >> 	pm_runtime_set_max_latency(dev, 100);
> > 
> > That actually is
> > 
> >  	dev_pm_qos_add_request(dev, req, 100);
> > 
> > where req is used as a handle for your request (it may be used, for
> > example, to remove the request or update it).
> > 
> >> And then runtime pm will create sysfs attributes etc
> > 
> > Well, there may be an interface for drivers analogous to
> > device_wakeup_enable()/device_wakeup_disable() allowing them
> > to add/remove a sysfs attribute for user space to control
> > a single PM QoS constraint.  That even sounds like a good idea. :-)
> 
> Does that mean you are going to change your patch?

Yes, it does.

Thanks,
Rafael