[RFC PATCH 0/4] Enable setting of sdio device power state with ACPI

[RFC PATCH 0/4] Enable setting of sdio device power state with ACPI

[Aaron Lu]

This patchset enables setting of acpi power state for sdio device
when it is runtime suspended/resumed. SDIO function drivers can use
the interfaces too introduced in patch 3 when there is such a need.

Patch 1 adds slot number information to the sdhci_host structure,
the slot number will be used to bind the sdio device with acpi node.

Patch 2 does the actual binding.

Patch 3 introduces a platform power management operation structure,
in which 4 callbacks are defined. The reason this structure is used is
that in addition to ACPI, there may be other platform mechanisms to
place a device into a low power state. And a implementation of this
structure is done for ACPI in this patch. The idea and the definition
of this structure is heavily based on the one used in PCI subsystem.

Patch 4 utilizes the newly added platform pm api to set the device
into a low power state after its driver's runtime suspend callback.
Its runtime wakeup capability is also enabled, and for this to work,
driver must call device_set_run_wake(dev, true) somewhere when wakeup
is desired. Please note that this wakeup mentioned here is realized
through a sideband signal, it's not done by a SDIO interrupt. So
platform support code is needed.

Aaron Lu (4):
  sdhci: add slot number into sdhci_host structure
  mmc: sdio: bind sdio device with acpi device
  sdio: introduce sdio_platform_pm_ops
  sdio: pm: set device's power state after driver runtime suspended it

 drivers/mmc/core/Makefile      |  1 +
 drivers/mmc/core/sdio.c        | 37 +++++++++++++++++++
 drivers/mmc/core/sdio.h        | 28 +++++++++++++++
 drivers/mmc/core/sdio_acpi.c   | 81 ++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/sdio_bus.c    | 63 +++++++++++++++++++++++++++++---
 drivers/mmc/core/sdio_bus.h    |  2 ++
 drivers/mmc/host/sdhci-pci.c   |  2 +-
 drivers/mmc/host/sdhci-pltfm.c |  4 +--
 drivers/mmc/host/sdhci-s3c.c   |  2 +-
 drivers/mmc/host/sdhci-spear.c |  4 +--
 drivers/mmc/host/sdhci.c       |  3 +-
 drivers/mmc/host/sdhci.h       |  2 +-
 include/linux/mmc/sdhci.h      |  1 +
 13 files changed, 218 insertions(+), 12 deletions(-)
 create mode 100644 drivers/mmc/core/sdio.h
 create mode 100644 drivers/mmc/core/sdio_acpi.c

-- 
1.7.12.21.g871e293

[RFC PATCH 1/4] sdhci: add slot number into sdhci_host structure

[Aaron Lu]

Per the SDHCI spec, a SD host controller can have multiple slots, and
each slot corresponds to a sdhci_host structure.

This patch adds the slot number information to the sdhci_host structure,
this will be used in the following patch to bind a sdio card to an acpi
node.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/mmc/host/sdhci-pci.c   | 2 +-
 drivers/mmc/host/sdhci-pltfm.c | 4 ++--
 drivers/mmc/host/sdhci-s3c.c   | 2 +-
 drivers/mmc/host/sdhci-spear.c | 4 ++--
 drivers/mmc/host/sdhci.c       | 3 ++-
 drivers/mmc/host/sdhci.h       | 2 +-
 include/linux/mmc/sdhci.h      | 1 +
 7 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 3f0794e..f9da09f 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1211,7 +1211,7 @@ static struct sdhci_pci_slot * __devinit sdhci_pci_probe_slot(
 		return ERR_PTR(-ENODEV);
 	}
 
-	host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pci_slot));
+	host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pci_slot), slotno);
 	if (IS_ERR(host)) {
 		dev_err(&pdev->dev, "cannot allocate host\n");
 		return ERR_CAST(host);
diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 65551a9..9a07b86 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -116,9 +116,9 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 
 	/* Some PCI-based MFD need the parent here */
 	if (pdev->dev.parent != &platform_bus && !np)
-		host = sdhci_alloc_host(pdev->dev.parent, sizeof(*pltfm_host));
+		host = sdhci_alloc_host(pdev->dev.parent, sizeof(*pltfm_host), 0);
 	else
-		host = sdhci_alloc_host(&pdev->dev, sizeof(*pltfm_host));
+		host = sdhci_alloc_host(&pdev->dev, sizeof(*pltfm_host), 0);
 
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 2903949..d689a8a 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -569,7 +569,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 		return irq;
 	}
 
-	host = sdhci_alloc_host(dev, sizeof(struct sdhci_s3c));
+	host = sdhci_alloc_host(dev, sizeof(struct sdhci_s3c), 0);
 	if (IS_ERR(host)) {
 		dev_err(dev, "sdhci_alloc_host() failed\n");
 		return PTR_ERR(host);
diff --git a/drivers/mmc/host/sdhci-spear.c b/drivers/mmc/host/sdhci-spear.c
index 423da81..4825bef 100644
--- a/drivers/mmc/host/sdhci-spear.c
+++ b/drivers/mmc/host/sdhci-spear.c
@@ -115,9 +115,9 @@ static int __devinit sdhci_probe(struct platform_device *pdev)
 	pdev->dev.platform_data = sdhci;
 
 	if (pdev->dev.parent)
-		host = sdhci_alloc_host(pdev->dev.parent, 0);
+		host = sdhci_alloc_host(pdev->dev.parent, 0, 0);
 	else
-		host = sdhci_alloc_host(&pdev->dev, 0);
+		host = sdhci_alloc_host(&pdev->dev, 0, 0);
 
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0e15c79..f4f51d1a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2622,7 +2622,7 @@ EXPORT_SYMBOL_GPL(sdhci_runtime_resume_host);
 \*****************************************************************************/
 
 struct sdhci_host *sdhci_alloc_host(struct device *dev,
-	size_t priv_size)
+	size_t priv_size, int slotno)
 {
 	struct mmc_host *mmc;
 	struct sdhci_host *host;
@@ -2635,6 +2635,7 @@ struct sdhci_host *sdhci_alloc_host(struct device *dev,
 
 	host = mmc_priv(mmc);
 	host->mmc = mmc;
+	host->slotno = slotno;
 
 	return host;
 }
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 97653ea..86d7ec6 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -365,7 +365,7 @@ static inline u8 sdhci_readb(struct sdhci_host *host, int reg)
 #endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS */
 
 extern struct sdhci_host *sdhci_alloc_host(struct device *dev,
-	size_t priv_size);
+	size_t priv_size, int slotno);
 extern void sdhci_free_host(struct sdhci_host *host);
 
 static inline void *sdhci_priv(struct sdhci_host *host)
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index fa8529a..3f3a508 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -18,6 +18,7 @@
 #include <linux/mmc/host.h>
 
 struct sdhci_host {
+	int slotno;	/* slot number of this host */
 	/* Data set by hardware interface driver */
 	const char *hw_name;	/* Hardware bus name */
 
-- 
1.7.12.21.g871e293

[RFC PATCH 2/4] mmc: sdio: bind sdio device with acpi device

[Aaron Lu]

ACPI spec defines the _ADDR encoding for sdio bus as:
High word - slot number (0 based)
Low word  - function number

This patch adds support for sdio device binding with acpi using the
generic ACPI glue framework.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/mmc/core/Makefile    |  1 +
 drivers/mmc/core/sdio_acpi.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/mmc/core/sdio_bus.c  | 14 ++++++++++++--
 drivers/mmc/core/sdio_bus.h  |  2 ++
 4 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mmc/core/sdio_acpi.c

diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index 38ed210..c8300aa 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -10,3 +10,4 @@ mmc_core-y			:= core.o bus.o host.o \
 				   quirks.o slot-gpio.o
 
 mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
+mmc_core-$(CONFIG_ACPI)		+= sdio_acpi.o
diff --git a/drivers/mmc/core/sdio_acpi.c b/drivers/mmc/core/sdio_acpi.c
new file mode 100644
index 0000000..0f92e90
--- /dev/null
+++ b/drivers/mmc/core/sdio_acpi.c
@@ -0,0 +1,35 @@
+#include <linux/mmc/host.h>
+#include <linux/mmc/card.h>
+#include <linux/mmc/sdhci.h>
+#include <linux/mmc/sdio_func.h>
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+#include "sdio_bus.h"
+
+static int acpi_sdio_find_device(struct device *dev, acpi_handle *handle)
+{
+	struct sdio_func *func;
+	struct sdhci_host *host;
+	u64 addr;
+
+	func = dev_to_sdio_func(dev);
+	host = mmc_priv(func->card->host);
+
+	addr = (host->slotno << 16) | func->num;
+
+	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev), addr);
+	if (!*handle)
+		return -ENODEV;
+
+	return 0;
+}
+
+static struct acpi_bus_type acpi_sdio_bus = {
+	.bus = &sdio_bus_type,
+	.find_device = acpi_sdio_find_device,
+};
+
+int sdio_acpi_register(void)
+{
+	return register_acpi_bus_type(&acpi_sdio_bus);
+}
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 6bf6879..aaec9e2 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -23,6 +23,7 @@
 
 #include "sdio_cis.h"
 #include "sdio_bus.h"
+#include "sdio_acpi.h"
 
 /* show configuration fields */
 #define sdio_config_attr(field, format_string)				\
@@ -209,7 +210,7 @@ static const struct dev_pm_ops sdio_bus_pm_ops = {
 
 #endif /* !CONFIG_PM */
 
-static struct bus_type sdio_bus_type = {
+struct bus_type sdio_bus_type = {
 	.name		= "sdio",
 	.dev_attrs	= sdio_dev_attrs,
 	.match		= sdio_bus_match,
@@ -221,7 +222,16 @@ static struct bus_type sdio_bus_type = {
 
 int sdio_register_bus(void)
 {
-	return bus_register(&sdio_bus_type);
+	int ret;
+
+	ret = bus_register(&sdio_bus_type);
+	if (ret)
+		goto out;
+
+	ret = sdio_acpi_register();
+
+out:
+	return ret;
 }
 
 void sdio_unregister_bus(void)
diff --git a/drivers/mmc/core/sdio_bus.h b/drivers/mmc/core/sdio_bus.h
index 567a768..91c0e93 100644
--- a/drivers/mmc/core/sdio_bus.h
+++ b/drivers/mmc/core/sdio_bus.h
@@ -18,5 +18,7 @@ void sdio_remove_func(struct sdio_func *func);
 int sdio_register_bus(void);
 void sdio_unregister_bus(void);
 
+extern struct bus_type sdio_bus_type;
+
 #endif
 
-- 
1.7.12.21.g871e293

[RFC PATCH 3/4] sdio: introduce sdio_platform_pm_ops

[Aaron Lu]

Some platform has the ability to set the sdio device into a low power
state with some specific method, e.g. ACPI on x86 based system can use
acpi control methods to change the device's power state.

Considering there may be different platforms utilizing different
mechanisms to achieve this, a new structure is introduced to let
individual platform to use these callbacks to do the job.

The structure contains 4 callbacks:
- is_manageable
  return true when the platform can manage the device's power;
  return false otherwise.
- choose_state
  Choose a proper power state for the device
- set_state
  Set the device's power state
- run_wake
  Enable the device's runtime wakeup capability from the platform's
  perspective.

And 4 functions to wrap these callbacks:
- bool platform_sdio_power_manageable(struct device *dev)
- int platform_sdio_choose_power_state(struct device *dev)
- int platform_sdio_set_power_state(struct device *dev, int state)
- int platform_sdio_run_wake(struct device *dev, bool enable)
So when these callbacks are desired, these wrapper functions should be
used. And if someday some sdio function driver which lives out of the
mmc subsystem has a need to use these wrapper functions, they can be
exported.

sdio_acpi.c implements these callbacks utilizing ACPI code.

The idea of this patch and the definition/wrapper of these callbacks
are heavily based on the one used in PCI subsystem.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/mmc/core/sdio.c      | 37 ++++++++++++++++++++++++++++++++++
 drivers/mmc/core/sdio.h      | 28 ++++++++++++++++++++++++++
 drivers/mmc/core/sdio_acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mmc/core/sdio.h

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index d4619e2..84b01b2 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -27,6 +27,7 @@
 #include "sd_ops.h"
 #include "sdio_ops.h"
 #include "sdio_cis.h"
+#include "sdio.h"
 
 static int sdio_read_fbr(struct sdio_func *func)
 {
@@ -1178,3 +1179,39 @@ err:
 	return err;
 }
 
+static struct sdio_platform_pm_ops *sdio_platform_pm;
+
+int sdio_set_platform_pm(struct sdio_platform_pm_ops *ops)
+{
+	if (!ops->is_manageable || !ops->choose_state ||
+			!ops->set_state || !ops->run_wake)
+		return -EINVAL;
+
+	sdio_platform_pm = ops;
+
+	return 0;
+}
+
+bool platform_sdio_power_manageable(struct device *dev)
+{
+	return sdio_platform_pm ?
+		sdio_platform_pm->is_manageable(dev) : false;
+}
+
+int platform_sdio_run_wake(struct device *dev, bool enable)
+{
+	return sdio_platform_pm ?
+		sdio_platform_pm->run_wake(dev, enable) : -ENODEV;
+}
+
+int platform_sdio_choose_power_state(struct device *dev)
+{
+	return sdio_platform_pm ?
+		sdio_platform_pm->choose_state(dev) : SDIO_POWER_ERROR;
+}
+
+int platform_sdio_set_power_state(struct device *dev, int state)
+{
+	return sdio_platform_pm ?
+		sdio_platform_pm->set_state(dev, state) : -ENOSYS;
+}
diff --git a/drivers/mmc/core/sdio.h b/drivers/mmc/core/sdio.h
new file mode 100644
index 0000000..a95929e
--- /dev/null
+++ b/drivers/mmc/core/sdio.h
@@ -0,0 +1,28 @@
+#ifndef __SDIO_H
+#define __SDIO_H
+
+typedef int __bitwise sdio_power_t;
+
+#define SDIO_D0          ((sdio_power_t __force) 0)
+#define SDIO_D1          ((sdio_power_t __force) 1)
+#define SDIO_D2          ((sdio_power_t __force) 2)
+#define SDIO_D3hot       ((sdio_power_t __force) 3)
+#define SDIO_D3cold      ((sdio_power_t __force) 4)
+#define SDIO_UNKNOWN     ((sdio_power_t __force) 5)
+#define SDIO_POWER_ERROR ((sdio_power_t __force) -1)
+
+struct sdio_platform_pm_ops {
+	bool (*is_manageable)(struct device *dev);
+	int (*choose_state)(struct device *dev);
+	int (*set_state)(struct device *dev, sdio_power_t state);
+	int (*run_wake)(struct device *dev, bool enabel);
+};
+
+int sdio_set_platform_pm(struct sdio_platform_pm_ops *ops);
+
+bool platform_sdio_power_manageable(struct device *dev);
+sdio_power_t platform_sdio_choose_power_state(struct device *dev);
+int platform_sdio_set_power_state(struct device *dev, sdio_power_t state);
+int platform_sdio_run_wake(struct device *dev, bool enable);
+
+#endif
diff --git a/drivers/mmc/core/sdio_acpi.c b/drivers/mmc/core/sdio_acpi.c
index 0f92e90..a5b3012 100644
--- a/drivers/mmc/core/sdio_acpi.c
+++ b/drivers/mmc/core/sdio_acpi.c
@@ -4,8 +4,45 @@
 #include <linux/mmc/sdio_func.h>
 #include <linux/acpi.h>
 #include <acpi/acpi_bus.h>
+#include "sdio.h"
 #include "sdio_bus.h"
 
+static bool acpi_sdio_power_manageable(struct device *dev)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
+	return handle ? acpi_bus_power_manageable(handle) : false;
+}
+
+static int acpi_sdio_choose_power_state(struct device *dev)
+{
+	return acpi_pm_device_sleep_state(dev, NULL, ACPI_STATE_D3);
+}
+
+static int acpi_sdio_set_power_state(struct device *dev, int state)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
+
+	if (state < ACPI_STATE_D0 || state > ACPI_STATE_D3_COLD)
+		return -EINVAL;
+
+	if (!handle)
+		return -ENODEV;
+
+	return acpi_bus_set_power(handle, state);
+}
+
+static int acpi_sdio_run_wake(struct device *dev, bool enable)
+{
+	return acpi_pm_device_run_wake(dev, enable);
+}
+
+struct sdio_platform_pm_ops acpi_sdio_platform_pm = {
+	.is_manageable = acpi_sdio_power_manageable,
+	.choose_state = acpi_sdio_choose_power_state,
+	.set_state = acpi_sdio_set_power_state,
+	.run_wake = acpi_sdio_run_wake,
+};
+
 static int acpi_sdio_find_device(struct device *dev, acpi_handle *handle)
 {
 	struct sdio_func *func;
@@ -31,5 +68,14 @@ static struct acpi_bus_type acpi_sdio_bus = {
 
 int sdio_acpi_register(void)
 {
-	return register_acpi_bus_type(&acpi_sdio_bus);
+	int ret;
+
+	ret = register_acpi_bus_type(&acpi_sdio_bus);
+	if (ret)
+		goto out;
+
+	sdio_set_platform_pm(&acpi_sdio_platform_pm);
+
+out:
+	return ret;
 }
-- 
1.7.12.21.g871e293

[RFC PATCH 4/4] sdio: pm: set device's power state after driver runtime suspended it

[Aaron Lu]

In sdio bus level runtime callback function, after call the driver's
runtime suspend callback, we will check if the device supports a
platform level power management, and if so, a proper power state is
chosen by the corresponding platform callback and then set.

Platform level runtime wakeup is also set, if device is enabled for
runtime wakeup by its driver, it will be armed the ability to generate
a wakeup event by the platform.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index aaec9e2..d83dea8 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -23,6 +23,7 @@
 
 #include "sdio_cis.h"
 #include "sdio_bus.h"
+#include "sdio.h"
 #include "sdio_acpi.h"
 
 /* show configuration fields */
@@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev)
 }
 
 #ifdef CONFIG_PM
+
+static int sdio_bus_runtime_suspend(struct device *dev)
+{
+	int ret;
+	sdio_power_t state;
+
+	ret = pm_generic_runtime_suspend(dev);
+	if (ret)
+		goto out;
+
+	if (!platform_sdio_power_manageable(dev))
+		goto out;
+
+	platform_sdio_run_wake(dev, true);
+
+	state = platform_sdio_choose_power_state(dev);
+	if (state == SDIO_POWER_ERROR) {
+		ret = -EIO;
+		goto out;
+	}
+
+	ret = platform_sdio_set_power_state(dev, state);
+
+out:
+	return ret;
+}
+
+static int sdio_bus_runtime_resume(struct device *dev)
+{
+	int ret;
+
+	if (platform_sdio_power_manageable(dev)) {
+		platform_sdio_run_wake(dev, false);
+		ret = platform_sdio_set_power_state(dev, SDIO_D0);
+		if (ret)
+			goto out;
+	}
+
+	ret = pm_generic_runtime_resume(dev);
+
+out:
+	return ret;
+}
+
 static const struct dev_pm_ops sdio_bus_pm_ops = {
 	SET_RUNTIME_PM_OPS(
-		pm_generic_runtime_suspend,
-		pm_generic_runtime_resume,
+		sdio_bus_runtime_suspend,
+		sdio_bus_runtime_resume,
 		pm_generic_runtime_idle
 	)
 };
-- 
1.7.12.21.g871e293

Re: [RFC PATCH 2/4] mmc: sdio: bind sdio device with acpi device

[Rafael J. Wysocki]

On Friday 12 of October 2012 11:12:39 Aaron Lu wrote:
> ACPI spec defines the _ADDR encoding for sdio bus as:
> High word - slot number (0 based)
> Low word  - function number
> 
> This patch adds support for sdio device binding with acpi using the
> generic ACPI glue framework.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/mmc/core/Makefile    |  1 +
>  drivers/mmc/core/sdio_acpi.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/mmc/core/sdio_bus.c  | 14 ++++++++++++--
>  drivers/mmc/core/sdio_bus.h  |  2 ++
>  4 files changed, 50 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/mmc/core/sdio_acpi.c
> 
> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
> index 38ed210..c8300aa 100644
> --- a/drivers/mmc/core/Makefile
> +++ b/drivers/mmc/core/Makefile
> @@ -10,3 +10,4 @@ mmc_core-y			:= core.o bus.o host.o \
>  				   quirks.o slot-gpio.o
>  
>  mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
> +mmc_core-$(CONFIG_ACPI)		+= sdio_acpi.o
> diff --git a/drivers/mmc/core/sdio_acpi.c b/drivers/mmc/core/sdio_acpi.c
> new file mode 100644
> index 0000000..0f92e90
> --- /dev/null
> +++ b/drivers/mmc/core/sdio_acpi.c
> @@ -0,0 +1,35 @@
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/sdhci.h>
> +#include <linux/mmc/sdio_func.h>
> +#include <linux/acpi.h>
> +#include <acpi/acpi_bus.h>
> +#include "sdio_bus.h"
> +
> +static int acpi_sdio_find_device(struct device *dev, acpi_handle *handle)
> +{
> +	struct sdio_func *func;
> +	struct sdhci_host *host;
> +	u64 addr;
> +
> +	func = dev_to_sdio_func(dev);
> +	host = mmc_priv(func->card->host);
> +
> +	addr = (host->slotno << 16) | func->num;
> +
> +	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev), addr);
> +	if (!*handle)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static struct acpi_bus_type acpi_sdio_bus = {
> +	.bus = &sdio_bus_type,
> +	.find_device = acpi_sdio_find_device,
> +};
> +
> +int sdio_acpi_register(void)

I'd call it sdio_acpi_register_bus().

> +{
> +	return register_acpi_bus_type(&acpi_sdio_bus);
> +}
> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> index 6bf6879..aaec9e2 100644
> --- a/drivers/mmc/core/sdio_bus.c
> +++ b/drivers/mmc/core/sdio_bus.c
> @@ -23,6 +23,7 @@
>  
>  #include "sdio_cis.h"
>  #include "sdio_bus.h"
> +#include "sdio_acpi.h"
>  
>  /* show configuration fields */
>  #define sdio_config_attr(field, format_string)				\
> @@ -209,7 +210,7 @@ static const struct dev_pm_ops sdio_bus_pm_ops = {
>  
>  #endif /* !CONFIG_PM */
>  
> -static struct bus_type sdio_bus_type = {
> +struct bus_type sdio_bus_type = {
>  	.name		= "sdio",
>  	.dev_attrs	= sdio_dev_attrs,
>  	.match		= sdio_bus_match,
> @@ -221,7 +222,16 @@ static struct bus_type sdio_bus_type = {
>  
>  int sdio_register_bus(void)
>  {
> -	return bus_register(&sdio_bus_type);
> +	int ret;
> +
> +	ret = bus_register(&sdio_bus_type);
> +	if (ret)
> +		goto out;
> +
> +	ret = sdio_acpi_register();

What happens if this fails?

> +
> +out:
> +	return ret;
>  }
>  
>  void sdio_unregister_bus(void)
> diff --git a/drivers/mmc/core/sdio_bus.h b/drivers/mmc/core/sdio_bus.h
> index 567a768..91c0e93 100644
> --- a/drivers/mmc/core/sdio_bus.h
> +++ b/drivers/mmc/core/sdio_bus.h
> @@ -18,5 +18,7 @@ void sdio_remove_func(struct sdio_func *func);
>  int sdio_register_bus(void);
>  void sdio_unregister_bus(void);
>  
> +extern struct bus_type sdio_bus_type;
> +
>  #endif

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 3/4] sdio: introduce sdio_platform_pm_ops

[Rafael J. Wysocki]

On Friday 12 of October 2012 11:12:40 Aaron Lu wrote:
> Some platform has the ability to set the sdio device into a low power
> state with some specific method, e.g. ACPI on x86 based system can use
> acpi control methods to change the device's power state.
> 
> Considering there may be different platforms utilizing different
> mechanisms to achieve this, a new structure is introduced to let
> individual platform to use these callbacks to do the job.
> 
> The structure contains 4 callbacks:
> - is_manageable
>   return true when the platform can manage the device's power;
>   return false otherwise.
> - choose_state
>   Choose a proper power state for the device
> - set_state
>   Set the device's power state
> - run_wake
>   Enable the device's runtime wakeup capability from the platform's
>   perspective.
> 
> And 4 functions to wrap these callbacks:
> - bool platform_sdio_power_manageable(struct device *dev)
> - int platform_sdio_choose_power_state(struct device *dev)
> - int platform_sdio_set_power_state(struct device *dev, int state)
> - int platform_sdio_run_wake(struct device *dev, bool enable)
> So when these callbacks are desired, these wrapper functions should be
> used. And if someday some sdio function driver which lives out of the
> mmc subsystem has a need to use these wrapper functions, they can be
> exported.
> 
> sdio_acpi.c implements these callbacks utilizing ACPI code.
> 
> The idea of this patch and the definition/wrapper of these callbacks
> are heavily based on the one used in PCI subsystem.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

I'm not really sure if we need to make it so complicated.

Please see my comments on the next patch.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 4/4] sdio: pm: set device's power state after driver runtime suspended it

[Rafael J. Wysocki]

On Friday 12 of October 2012 11:12:41 Aaron Lu wrote:
> In sdio bus level runtime callback function, after call the driver's
> runtime suspend callback, we will check if the device supports a
> platform level power management, and if so, a proper power state is
> chosen by the corresponding platform callback and then set.
> 
> Platform level runtime wakeup is also set, if device is enabled for
> runtime wakeup by its driver, it will be armed the ability to generate
> a wakeup event by the platform.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> index aaec9e2..d83dea8 100644
> --- a/drivers/mmc/core/sdio_bus.c
> +++ b/drivers/mmc/core/sdio_bus.c
> @@ -23,6 +23,7 @@
>  
>  #include "sdio_cis.h"
>  #include "sdio_bus.h"
> +#include "sdio.h"
>  #include "sdio_acpi.h"
>  
>  /* show configuration fields */
> @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev)
>  }
>  
>  #ifdef CONFIG_PM
> +
> +static int sdio_bus_runtime_suspend(struct device *dev)
> +{
> +	int ret;
> +	sdio_power_t state;
> +
> +	ret = pm_generic_runtime_suspend(dev);
> +	if (ret)
> +		goto out;
> +
> +	if (!platform_sdio_power_manageable(dev))
> +		goto out;
> +
> +	platform_sdio_run_wake(dev, true);
> +
> +	state = platform_sdio_choose_power_state(dev);
> +	if (state == SDIO_POWER_ERROR) {
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	ret = platform_sdio_set_power_state(dev, state);
> +
> +out:
> +	return ret;
> +}
> +
> +static int sdio_bus_runtime_resume(struct device *dev)
> +{
> +	int ret;
> +
> +	if (platform_sdio_power_manageable(dev)) {
> +		platform_sdio_run_wake(dev, false);
> +		ret = platform_sdio_set_power_state(dev, SDIO_D0);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	ret = pm_generic_runtime_resume(dev);
> +
> +out:
> +	return ret;
> +}
> +

Most likely we will need to make analogous changes for other bus types that
don't support power management natively, like platform, SPI, I2C etc.  In all
of them the _runtime_suspend() and _runtime_resume() routine will look
almost exactly the same except for the platform_sdio_ prefix.

For this reason, I think it would be better to simply define two functions
acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of
the ACPI-specific operations related to runtime suspend/resume.  Then, we
will be able to use these functions for all of the bus types in question
in the same way (we may also need to add analogous functions for system
suspend/resume handling).

If we do that, then the sdio_platform_pm_ops structure from the previous
patch won't be necessary any more.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 4/4] sdio: pm: set device's power state after driver runtime suspended it

[Rafael J. Wysocki]

On Friday 19 of October 2012 01:39:25 Rafael J. Wysocki wrote:
> On Friday 12 of October 2012 11:12:41 Aaron Lu wrote:
> > In sdio bus level runtime callback function, after call the driver's
> > runtime suspend callback, we will check if the device supports a
> > platform level power management, and if so, a proper power state is
> > chosen by the corresponding platform callback and then set.
> > 
> > Platform level runtime wakeup is also set, if device is enabled for
> > runtime wakeup by its driver, it will be armed the ability to generate
> > a wakeup event by the platform.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 47 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> > index aaec9e2..d83dea8 100644
> > --- a/drivers/mmc/core/sdio_bus.c
> > +++ b/drivers/mmc/core/sdio_bus.c
> > @@ -23,6 +23,7 @@
> >  
> >  #include "sdio_cis.h"
> >  #include "sdio_bus.h"
> > +#include "sdio.h"
> >  #include "sdio_acpi.h"
> >  
> >  /* show configuration fields */
> > @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev)
> >  }
> >  
> >  #ifdef CONFIG_PM
> > +
> > +static int sdio_bus_runtime_suspend(struct device *dev)
> > +{
> > +	int ret;
> > +	sdio_power_t state;
> > +
> > +	ret = pm_generic_runtime_suspend(dev);
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (!platform_sdio_power_manageable(dev))
> > +		goto out;
> > +
> > +	platform_sdio_run_wake(dev, true);
> > +
> > +	state = platform_sdio_choose_power_state(dev);
> > +	if (state == SDIO_POWER_ERROR) {
> > +		ret = -EIO;
> > +		goto out;
> > +	}
> > +
> > +	ret = platform_sdio_set_power_state(dev, state);
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int sdio_bus_runtime_resume(struct device *dev)
> > +{
> > +	int ret;
> > +
> > +	if (platform_sdio_power_manageable(dev)) {
> > +		platform_sdio_run_wake(dev, false);
> > +		ret = platform_sdio_set_power_state(dev, SDIO_D0);
> > +		if (ret)
> > +			goto out;
> > +	}
> > +
> > +	ret = pm_generic_runtime_resume(dev);
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> 
> Most likely we will need to make analogous changes for other bus types that
> don't support power management natively, like platform, SPI, I2C etc.  In all
> of them the _runtime_suspend() and _runtime_resume() routine will look
> almost exactly the same except for the platform_sdio_ prefix.
> 
> For this reason, I think it would be better to simply define two functions
> acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of
> the ACPI-specific operations related to runtime suspend/resume.  Then, we
> will be able to use these functions for all of the bus types in question
> in the same way (we may also need to add analogous functions for system
> suspend/resume handling).

Something like in the (totally untested) patch below.

With this code the SDIO bus type should be able to use
acpi_subsys_runtime_suspend() and acpi_subsys_runtime_resume() as its
bus-type-level runtime suspend/resume callback routines, respectively.

Thanks,
Rafael


Prototype, no sign-off.
---
 drivers/acpi/dev_pm.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h  |   12 +++++++++++
 2 files changed, 66 insertions(+)

Index: linux-pm/drivers/acpi/dev_pm.c
===================================================================
--- /dev/null
+++ linux-pm/drivers/acpi/dev_pm.c
@@ -0,0 +1,54 @@
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/pm_qos.h>
+#include <linux/pm_runtime.h>
+
+#include <acpi/acpi_bus.h>
+
+int acpi_dev_runtime_suspend(struct device *dev)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
+	bool enable_wakeup;
+	int power_state;
+
+	if (!handle || !acpi_bus_power_manageable(handle))
+		return 0;
+
+	enable_wakeup = dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP))
+				!= PM_QOS_FLAGS_NONE;
+	acpi_pm_device_run_wake(dev, enable_wakeup);
+
+	power_state = acpi_pm_device_sleep_state(dev, NULL, ACPI_STATE_D3);
+	if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3)
+		return -EIO;
+
+	return acpi_bus_set_power(handle, power_state);
+}
+EXPORT_SYMBOL_GPL(acpi_dev_runtime_suspend);
+
+int acpi_dev_runtime_resume(struct device *dev)
+{
+	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
+
+	if (!handle || !acpi_bus_power_manageable(handle))
+		return 0;
+
+	acpi_pm_device_run_wake(dev, false);
+	return acpi_bus_set_power(handle, ACPI_STATE_D0);
+}
+EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume);
+
+int acpi_subsys_runtime_suspend(struct device *dev)
+{
+	int ret = pm_generic_runtime_suspend(dev);
+	return ret ? ret : acpi_dev_runtime_suspend(dev);
+}
+EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend);
+
+int acpi_subsys_runtime_resume(struct device *dev)
+{
+	int ret = acpi_dev_runtime_resume(dev);
+	return ret ? ret : pm_generic_runtime_resume(dev);
+}
+EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -434,4 +434,16 @@ acpi_status acpi_os_prepare_sleep(u8 sle
 #define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0)
 #endif
 
+#if defined(CONFIG_ACPI) && defined (CONFIG_PM_RUNTIME)
+int acpi_dev_runtime_suspend(struct device *dev);
+int acpi_dev_runtime_resume(struct device *dev);
+int acpi_subsys_runtime_suspend(struct device *dev);
+int acpi_subsys_runtime_resume(struct device *dev);
+#else
+static inline acpi_dev_runtime_suspend(struct device *dev) { return 0; }
+static inline acpi_dev_runtime_resume(struct device *dev) { return 0; }
+static inline acpi_subsys_runtime_suspend(struct device *dev) { return 0; }
+static inline acpi_subsys_runtime_resume(struct device *dev) { return 0; }
+#endif
+
 #endif	/*_LINUX_ACPI_H*/


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 2/4] mmc: sdio: bind sdio device with acpi device

[Aaron Lu]

On Fri, Oct 19, 2012 at 01:25:46AM +0200, Rafael J. Wysocki wrote:
> On Friday 12 of October 2012 11:12:39 Aaron Lu wrote:
> > ACPI spec defines the _ADDR encoding for sdio bus as:
> > High word - slot number (0 based)
> > Low word  - function number
> > 
> > This patch adds support for sdio device binding with acpi using the
> > generic ACPI glue framework.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  drivers/mmc/core/Makefile    |  1 +
> >  drivers/mmc/core/sdio_acpi.c | 35 +++++++++++++++++++++++++++++++++++
> >  drivers/mmc/core/sdio_bus.c  | 14 ++++++++++++--
> >  drivers/mmc/core/sdio_bus.h  |  2 ++
> >  4 files changed, 50 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/mmc/core/sdio_acpi.c
> > 
> > diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
> > index 38ed210..c8300aa 100644
> > --- a/drivers/mmc/core/Makefile
> > +++ b/drivers/mmc/core/Makefile
> > @@ -10,3 +10,4 @@ mmc_core-y			:= core.o bus.o host.o \
> >  				   quirks.o slot-gpio.o
> >  
> >  mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
> > +mmc_core-$(CONFIG_ACPI)		+= sdio_acpi.o
> > diff --git a/drivers/mmc/core/sdio_acpi.c b/drivers/mmc/core/sdio_acpi.c
> > new file mode 100644
> > index 0000000..0f92e90
> > --- /dev/null
> > +++ b/drivers/mmc/core/sdio_acpi.c
> > @@ -0,0 +1,35 @@
> > +#include <linux/mmc/host.h>
> > +#include <linux/mmc/card.h>
> > +#include <linux/mmc/sdhci.h>
> > +#include <linux/mmc/sdio_func.h>
> > +#include <linux/acpi.h>
> > +#include <acpi/acpi_bus.h>
> > +#include "sdio_bus.h"
> > +
> > +static int acpi_sdio_find_device(struct device *dev, acpi_handle *handle)
> > +{
> > +	struct sdio_func *func;
> > +	struct sdhci_host *host;
> > +	u64 addr;
> > +
> > +	func = dev_to_sdio_func(dev);
> > +	host = mmc_priv(func->card->host);
> > +
> > +	addr = (host->slotno << 16) | func->num;
> > +
> > +	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev), addr);
> > +	if (!*handle)
> > +		return -ENODEV;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct acpi_bus_type acpi_sdio_bus = {
> > +	.bus = &sdio_bus_type,
> > +	.find_device = acpi_sdio_find_device,
> > +};
> > +
> > +int sdio_acpi_register(void)
> 
> I'd call it sdio_acpi_register_bus().

OK, will update this in v2.

> 
> > +{
> > +	return register_acpi_bus_type(&acpi_sdio_bus);
> > +}
> > diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> > index 6bf6879..aaec9e2 100644
> > --- a/drivers/mmc/core/sdio_bus.c
> > +++ b/drivers/mmc/core/sdio_bus.c
> > @@ -23,6 +23,7 @@
> >  
> >  #include "sdio_cis.h"
> >  #include "sdio_bus.h"
> > +#include "sdio_acpi.h"
> >  
> >  /* show configuration fields */
> >  #define sdio_config_attr(field, format_string)				\
> > @@ -209,7 +210,7 @@ static const struct dev_pm_ops sdio_bus_pm_ops = {
> >  
> >  #endif /* !CONFIG_PM */
> >  
> > -static struct bus_type sdio_bus_type = {
> > +struct bus_type sdio_bus_type = {
> >  	.name		= "sdio",
> >  	.dev_attrs	= sdio_dev_attrs,
> >  	.match		= sdio_bus_match,
> > @@ -221,7 +222,16 @@ static struct bus_type sdio_bus_type = {
> >  
> >  int sdio_register_bus(void)
> >  {
> > -	return bus_register(&sdio_bus_type);
> > +	int ret;
> > +
> > +	ret = bus_register(&sdio_bus_type);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = sdio_acpi_register();
> 
> What happens if this fails?

Oh, yes, it shouldn't affect the sdio_register_bus function. So I'll not
check its return value in v2.

Thanks,
Aaron

> 
> > +
> > +out:
> > +	return ret;
> >  }
> >  
> >  void sdio_unregister_bus(void)
> > diff --git a/drivers/mmc/core/sdio_bus.h b/drivers/mmc/core/sdio_bus.h
> > index 567a768..91c0e93 100644
> > --- a/drivers/mmc/core/sdio_bus.h
> > +++ b/drivers/mmc/core/sdio_bus.h
> > @@ -18,5 +18,7 @@ void sdio_remove_func(struct sdio_func *func);
> >  int sdio_register_bus(void);
> >  void sdio_unregister_bus(void);
> >  
> > +extern struct bus_type sdio_bus_type;
> > +
> >  #endif
> 
> Thanks,
> Rafael
> 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 4/4] sdio: pm: set device's power state after driver runtime suspended it

[Aaron Lu]

On Fri, Oct 19, 2012 at 08:08:38PM +0200, Rafael J. Wysocki wrote:
> On Friday 19 of October 2012 01:39:25 Rafael J. Wysocki wrote:
> > On Friday 12 of October 2012 11:12:41 Aaron Lu wrote:
> > > In sdio bus level runtime callback function, after call the driver's
> > > runtime suspend callback, we will check if the device supports a
> > > platform level power management, and if so, a proper power state is
> > > chosen by the corresponding platform callback and then set.
> > > 
> > > Platform level runtime wakeup is also set, if device is enabled for
> > > runtime wakeup by its driver, it will be armed the ability to generate
> > > a wakeup event by the platform.
> > > 
> > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > ---
> > >  drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 47 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> > > index aaec9e2..d83dea8 100644
> > > --- a/drivers/mmc/core/sdio_bus.c
> > > +++ b/drivers/mmc/core/sdio_bus.c
> > > @@ -23,6 +23,7 @@
> > >  
> > >  #include "sdio_cis.h"
> > >  #include "sdio_bus.h"
> > > +#include "sdio.h"
> > >  #include "sdio_acpi.h"
> > >  
> > >  /* show configuration fields */
> > > @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev)
> > >  }
> > >  
> > >  #ifdef CONFIG_PM
> > > +
> > > +static int sdio_bus_runtime_suspend(struct device *dev)
> > > +{
> > > +	int ret;
> > > +	sdio_power_t state;
> > > +
> > > +	ret = pm_generic_runtime_suspend(dev);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	if (!platform_sdio_power_manageable(dev))
> > > +		goto out;
> > > +
> > > +	platform_sdio_run_wake(dev, true);
> > > +
> > > +	state = platform_sdio_choose_power_state(dev);
> > > +	if (state == SDIO_POWER_ERROR) {
> > > +		ret = -EIO;
> > > +		goto out;
> > > +	}
> > > +
> > > +	ret = platform_sdio_set_power_state(dev, state);
> > > +
> > > +out:
> > > +	return ret;
> > > +}
> > > +
> > > +static int sdio_bus_runtime_resume(struct device *dev)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (platform_sdio_power_manageable(dev)) {
> > > +		platform_sdio_run_wake(dev, false);
> > > +		ret = platform_sdio_set_power_state(dev, SDIO_D0);
> > > +		if (ret)
> > > +			goto out;
> > > +	}
> > > +
> > > +	ret = pm_generic_runtime_resume(dev);
> > > +
> > > +out:
> > > +	return ret;
> > > +}
> > > +
> > 
> > Most likely we will need to make analogous changes for other bus types that
> > don't support power management natively, like platform, SPI, I2C etc.  In all
> > of them the _runtime_suspend() and _runtime_resume() routine will look
> > almost exactly the same except for the platform_sdio_ prefix.
> > 
> > For this reason, I think it would be better to simply define two functions
> > acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of
> > the ACPI-specific operations related to runtime suspend/resume.  Then, we
> > will be able to use these functions for all of the bus types in question
> > in the same way (we may also need to add analogous functions for system
> > suspend/resume handling).
> 
> Something like in the (totally untested) patch below.

Looks good to me.
I'll test the code and put it into v2 of the patchset with your
sign-off, is it OK?

Thanks,
Aaron

> 
> With this code the SDIO bus type should be able to use
> acpi_subsys_runtime_suspend() and acpi_subsys_runtime_resume() as its
> bus-type-level runtime suspend/resume callback routines, respectively.
> 
> Thanks,
> Rafael
> 
> 
> Prototype, no sign-off.
> ---
>  drivers/acpi/dev_pm.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h  |   12 +++++++++++
>  2 files changed, 66 insertions(+)
> 
> Index: linux-pm/drivers/acpi/dev_pm.c
> ===================================================================
> --- /dev/null
> +++ linux-pm/drivers/acpi/dev_pm.c
> @@ -0,0 +1,54 @@
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/pm_qos.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <acpi/acpi_bus.h>
> +
> +int acpi_dev_runtime_suspend(struct device *dev)
> +{
> +	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> +	bool enable_wakeup;
> +	int power_state;
> +
> +	if (!handle || !acpi_bus_power_manageable(handle))
> +		return 0;
> +
> +	enable_wakeup = dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP))
> +				!= PM_QOS_FLAGS_NONE;
> +	acpi_pm_device_run_wake(dev, enable_wakeup);
> +
> +	power_state = acpi_pm_device_sleep_state(dev, NULL, ACPI_STATE_D3);
> +	if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3)
> +		return -EIO;
> +
> +	return acpi_bus_set_power(handle, power_state);
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_runtime_suspend);
> +
> +int acpi_dev_runtime_resume(struct device *dev)
> +{
> +	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> +
> +	if (!handle || !acpi_bus_power_manageable(handle))
> +		return 0;
> +
> +	acpi_pm_device_run_wake(dev, false);
> +	return acpi_bus_set_power(handle, ACPI_STATE_D0);
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume);
> +
> +int acpi_subsys_runtime_suspend(struct device *dev)
> +{
> +	int ret = pm_generic_runtime_suspend(dev);
> +	return ret ? ret : acpi_dev_runtime_suspend(dev);
> +}
> +EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend);
> +
> +int acpi_subsys_runtime_resume(struct device *dev)
> +{
> +	int ret = acpi_dev_runtime_resume(dev);
> +	return ret ? ret : pm_generic_runtime_resume(dev);
> +}
> +EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
> Index: linux-pm/include/linux/acpi.h
> ===================================================================
> --- linux-pm.orig/include/linux/acpi.h
> +++ linux-pm/include/linux/acpi.h
> @@ -434,4 +434,16 @@ acpi_status acpi_os_prepare_sleep(u8 sle
>  #define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0)
>  #endif
>  
> +#if defined(CONFIG_ACPI) && defined (CONFIG_PM_RUNTIME)
> +int acpi_dev_runtime_suspend(struct device *dev);
> +int acpi_dev_runtime_resume(struct device *dev);
> +int acpi_subsys_runtime_suspend(struct device *dev);
> +int acpi_subsys_runtime_resume(struct device *dev);
> +#else
> +static inline acpi_dev_runtime_suspend(struct device *dev) { return 0; }
> +static inline acpi_dev_runtime_resume(struct device *dev) { return 0; }
> +static inline acpi_subsys_runtime_suspend(struct device *dev) { return 0; }
> +static inline acpi_subsys_runtime_resume(struct device *dev) { return 0; }
> +#endif
> +
>  #endif	/*_LINUX_ACPI_H*/
> 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 4/4] sdio: pm: set device's power state after driver runtime suspended it

[Rafael J. Wysocki]

On Saturday 20 of October 2012 15:15:41 Aaron Lu wrote:
> On Fri, Oct 19, 2012 at 08:08:38PM +0200, Rafael J. Wysocki wrote:
> > On Friday 19 of October 2012 01:39:25 Rafael J. Wysocki wrote:
> > > On Friday 12 of October 2012 11:12:41 Aaron Lu wrote:
> > > > In sdio bus level runtime callback function, after call the driver's
> > > > runtime suspend callback, we will check if the device supports a
> > > > platform level power management, and if so, a proper power state is
> > > > chosen by the corresponding platform callback and then set.
> > > > 
> > > > Platform level runtime wakeup is also set, if device is enabled for
> > > > runtime wakeup by its driver, it will be armed the ability to generate
> > > > a wakeup event by the platform.
> > > > 
> > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > > > ---
> > > >  drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 47 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> > > > index aaec9e2..d83dea8 100644
> > > > --- a/drivers/mmc/core/sdio_bus.c
> > > > +++ b/drivers/mmc/core/sdio_bus.c
> > > > @@ -23,6 +23,7 @@
> > > >  
> > > >  #include "sdio_cis.h"
> > > >  #include "sdio_bus.h"
> > > > +#include "sdio.h"
> > > >  #include "sdio_acpi.h"
> > > >  
> > > >  /* show configuration fields */
> > > > @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev)
> > > >  }
> > > >  
> > > >  #ifdef CONFIG_PM
> > > > +
> > > > +static int sdio_bus_runtime_suspend(struct device *dev)
> > > > +{
> > > > +	int ret;
> > > > +	sdio_power_t state;
> > > > +
> > > > +	ret = pm_generic_runtime_suspend(dev);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	if (!platform_sdio_power_manageable(dev))
> > > > +		goto out;
> > > > +
> > > > +	platform_sdio_run_wake(dev, true);
> > > > +
> > > > +	state = platform_sdio_choose_power_state(dev);
> > > > +	if (state == SDIO_POWER_ERROR) {
> > > > +		ret = -EIO;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	ret = platform_sdio_set_power_state(dev, state);
> > > > +
> > > > +out:
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int sdio_bus_runtime_resume(struct device *dev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (platform_sdio_power_manageable(dev)) {
> > > > +		platform_sdio_run_wake(dev, false);
> > > > +		ret = platform_sdio_set_power_state(dev, SDIO_D0);
> > > > +		if (ret)
> > > > +			goto out;
> > > > +	}
> > > > +
> > > > +	ret = pm_generic_runtime_resume(dev);
> > > > +
> > > > +out:
> > > > +	return ret;
> > > > +}
> > > > +
> > > 
> > > Most likely we will need to make analogous changes for other bus types that
> > > don't support power management natively, like platform, SPI, I2C etc.  In all
> > > of them the _runtime_suspend() and _runtime_resume() routine will look
> > > almost exactly the same except for the platform_sdio_ prefix.
> > > 
> > > For this reason, I think it would be better to simply define two functions
> > > acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of
> > > the ACPI-specific operations related to runtime suspend/resume.  Then, we
> > > will be able to use these functions for all of the bus types in question
> > > in the same way (we may also need to add analogous functions for system
> > > suspend/resume handling).
> > 
> > Something like in the (totally untested) patch below.
> 
> Looks good to me.
> I'll test the code and put it into v2 of the patchset with your
> sign-off, is it OK?

I'd rather do it a bit differently in the signed-off version (I'm working
on these patches, they should be ready around Tuesday), but if you can test
it in its current form, that'd be useful too.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH 4/4] sdio: pm: set device's power state after driver runtime suspended it

[Aaron Lu]

On 10/22/2012 03:57 AM, Rafael J. Wysocki wrote:
> On Saturday 20 of October 2012 15:15:41 Aaron Lu wrote:
>> On Fri, Oct 19, 2012 at 08:08:38PM +0200, Rafael J. Wysocki wrote:
>>> On Friday 19 of October 2012 01:39:25 Rafael J. Wysocki wrote:
>>>> On Friday 12 of October 2012 11:12:41 Aaron Lu wrote:
>>>>> In sdio bus level runtime callback function, after call the driver's
>>>>> runtime suspend callback, we will check if the device supports a
>>>>> platform level power management, and if so, a proper power state is
>>>>> chosen by the corresponding platform callback and then set.
>>>>>
>>>>> Platform level runtime wakeup is also set, if device is enabled for
>>>>> runtime wakeup by its driver, it will be armed the ability to generate
>>>>> a wakeup event by the platform.
>>>>>
>>>>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>>>>> ---
>>>>>  drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++--
>>>>>  1 file changed, 47 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>>>>> index aaec9e2..d83dea8 100644
>>>>> --- a/drivers/mmc/core/sdio_bus.c
>>>>> +++ b/drivers/mmc/core/sdio_bus.c
>>>>> @@ -23,6 +23,7 @@
>>>>>  
>>>>>  #include "sdio_cis.h"
>>>>>  #include "sdio_bus.h"
>>>>> +#include "sdio.h"
>>>>>  #include "sdio_acpi.h"
>>>>>  
>>>>>  /* show configuration fields */
>>>>> @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev)
>>>>>  }
>>>>>  
>>>>>  #ifdef CONFIG_PM
>>>>> +
>>>>> +static int sdio_bus_runtime_suspend(struct device *dev)
>>>>> +{
>>>>> +	int ret;
>>>>> +	sdio_power_t state;
>>>>> +
>>>>> +	ret = pm_generic_runtime_suspend(dev);
>>>>> +	if (ret)
>>>>> +		goto out;
>>>>> +
>>>>> +	if (!platform_sdio_power_manageable(dev))
>>>>> +		goto out;
>>>>> +
>>>>> +	platform_sdio_run_wake(dev, true);
>>>>> +
>>>>> +	state = platform_sdio_choose_power_state(dev);
>>>>> +	if (state == SDIO_POWER_ERROR) {
>>>>> +		ret = -EIO;
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>> +	ret = platform_sdio_set_power_state(dev, state);
>>>>> +
>>>>> +out:
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static int sdio_bus_runtime_resume(struct device *dev)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	if (platform_sdio_power_manageable(dev)) {
>>>>> +		platform_sdio_run_wake(dev, false);
>>>>> +		ret = platform_sdio_set_power_state(dev, SDIO_D0);
>>>>> +		if (ret)
>>>>> +			goto out;
>>>>> +	}
>>>>> +
>>>>> +	ret = pm_generic_runtime_resume(dev);
>>>>> +
>>>>> +out:
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>
>>>> Most likely we will need to make analogous changes for other bus types that
>>>> don't support power management natively, like platform, SPI, I2C etc.  In all
>>>> of them the _runtime_suspend() and _runtime_resume() routine will look
>>>> almost exactly the same except for the platform_sdio_ prefix.
>>>>
>>>> For this reason, I think it would be better to simply define two functions
>>>> acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of
>>>> the ACPI-specific operations related to runtime suspend/resume.  Then, we
>>>> will be able to use these functions for all of the bus types in question
>>>> in the same way (we may also need to add analogous functions for system
>>>> suspend/resume handling).
>>>
>>> Something like in the (totally untested) patch below.
>>
>> Looks good to me.
>> I'll test the code and put it into v2 of the patchset with your
>> sign-off, is it OK?
> 
> I'd rather do it a bit differently in the signed-off version (I'm working
> on these patches, they should be ready around Tuesday), but if you can test

OK, thanks.

> it in its current form, that'd be useful too.

I was planning to test it some time later, so looks like I can directly
test your signed-off version :-)

Thanks,
Aaron