[PATCH 0/4] runtime PM support at the bus level

[PATCH 0/4] runtime PM support at the bus level

[Kevin Hilman]

This series introduces runtime PM support at the platform bus level
for OMAP.

In a nutshell, when using the runtime PM API for any device with an
assocated omap_device (and hwmod), the omap device API will be used to
handle the hardware-level power management of that device, including
managing clocks, etc.

Today, most drivers handle this by manually enabling/disabling their
clocks when needed.  With this series (and an omap_device/hwmod for
each device) direct clock managment can be removed from the driver in
favor of using the runtime PM API.

NOTE: the 1st patch is FYI only, it has already been merged into
      mainline for 2.6.35 via the driver core.

This applies on v2.6.34 and has been tested along with Benoit's two
hwmod fixes/cleanup series and the work-in-progress UART and MMC hwmod
conversions and has been tested on OMAP3430 (omap3evm) and OMAP2
(n810).

This series is also available in my git tree as the 'pm-wip/runtime'
branch.

Kevin Hilman (4):
  platform_bus: allow custom extensions to system PM methods
  OMAP: PM: initial runtime PM core support
  OMAP: PM: use omap_device API for suspend/resume
  OMAP: omap_device: add flag to disable automatic bus-level
    suspend/resume

 arch/arm/mach-omap2/Makefile                  |    7 +-
 arch/arm/mach-omap2/pm_bus.c                  |  133 +++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/omap_device.h |    5 +
 drivers/base/platform.c                       |    8 +-
 4 files changed, 148 insertions(+), 5 deletions(-)
 create mode 100644 arch/arm/mach-omap2/pm_bus.c

[PATCH 1/4] platform_bus: allow custom extensions to system PM methods

[Kevin Hilman]

When runtime PM for platform_bus was added, it allowed for platforms
to customize the runtime PM methods since they are defined as weak
symbols.

This patch allows platforms to also extend the system PM methods with
custom hooks so runtime PM and system PM extensions can be managed
together by custom platform-specific code.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Magnus Damm <damm@opensource.se>
Cc: Rafael Wysocki <rjw@sisk.pl>
Cc: Dmitry Torokhov <dtor@mail.ru>
Cc: Eric Miao <eric.y.miao@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/base/platform.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4b4b565..0bc478a 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -735,7 +735,7 @@ static void platform_pm_complete(struct device *dev)
 
 #ifdef CONFIG_SUSPEND
 
-static int platform_pm_suspend(struct device *dev)
+int __weak platform_pm_suspend(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
 	int ret = 0;
@@ -753,7 +753,7 @@ static int platform_pm_suspend(struct device *dev)
 	return ret;
 }
 
-static int platform_pm_suspend_noirq(struct device *dev)
+int __weak platform_pm_suspend_noirq(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
 	int ret = 0;
@@ -769,7 +769,7 @@ static int platform_pm_suspend_noirq(struct device *dev)
 	return ret;
 }
 
-static int platform_pm_resume(struct device *dev)
+int __weak platform_pm_resume(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
 	int ret = 0;
@@ -787,7 +787,7 @@ static int platform_pm_resume(struct device *dev)
 	return ret;
 }
 
-static int platform_pm_resume_noirq(struct device *dev)
+int __weak platform_pm_resume_noirq(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
 	int ret = 0;
-- 
1.7.0.2

[PATCH 2/4] OMAP: PM: initial runtime PM core support

[Kevin Hilman]

Implement the new runtime PM framework as a thin layer on top of the
omap_device API.  Since we don't have an OMAP-specific bus, override
the runtime PM hooks for the platform_bus for the OMAP specific
implementation.

While the runtime PM API has three main states (idle, suspend, resume)
This version treats idle and suspend the same way by implementing both
on top of omap_device_disable(), which follows closely with how driver
are currently using clock enable/disable calls. Longer-termm
pm_runtime_idle() could take other constraints into consideration to
make the decision, but the current

Device driver ->runtime_suspend() hooks are called just before the
device is disabled (via omap_device_idle()), and device driver
->runtime_resume() hooks are called just after device has been
enabled (via omap_device_enable().)

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/Makefile |    7 +++-
 arch/arm/mach-omap2/pm_bus.c |   72 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap2/pm_bus.c

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 4b9fc57..58a0474 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -46,12 +46,17 @@ obj-$(CONFIG_ARCH_OMAP2)		+= sdrc2xxx.o
 ifeq ($(CONFIG_PM),y)
 obj-$(CONFIG_ARCH_OMAP2)		+= pm24xx.o
 obj-$(CONFIG_ARCH_OMAP2)		+= sleep24xx.o
-obj-$(CONFIG_ARCH_OMAP3)		+= pm34xx.o sleep34xx.o cpuidle34xx.o
+obj-$(CONFIG_ARCH_OMAP3)		+= pm34xx.o sleep34xx.o cpuidle34xx.o \
+					   pm_bus.o
 obj-$(CONFIG_PM_DEBUG)			+= pm-debug.o
 
 AFLAGS_sleep24xx.o			:=-Wa,-march=armv6
 AFLAGS_sleep34xx.o			:=-Wa,-march=armv7-a
 
+ifeq ($(CONFIG_PM_VERBOSE),y)
+CFLAGS_pm_bus.o				+= -DDEBUG
+endif
+
 endif
 
 # PRCM
diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c
new file mode 100644
index 0000000..69acaa5
--- /dev/null
+++ b/arch/arm/mach-omap2/pm_bus.c
@@ -0,0 +1,72 @@
+/*
+ * Runtime PM support code for OMAP
+ *
+ * Author: Kevin Hilman, Deep Root Systems, LLC
+ *
+ * Copyright (C) 2010 Texas Instruments, Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+
+#include <plat/omap_device.h>
+#include <plat/omap-pm.h>
+
+#ifdef CONFIG_PM_RUNTIME
+int platform_pm_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *odev = to_omap_device(pdev);
+	int r, ret = 0;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (dev->driver->pm && dev->driver->pm->runtime_suspend)
+		ret = dev->driver->pm->runtime_suspend(dev);
+	if (!ret && omap_device_is_valid(odev)) {
+		r = omap_device_idle(pdev);
+		WARN_ON(r);
+	}
+
+	return ret;
+};
+
+int platform_pm_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *odev = to_omap_device(pdev);
+	int r, ret = 0;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (omap_device_is_valid(odev)) {
+		r = omap_device_enable(pdev);
+		WARN_ON(r);
+	}
+
+	if (dev->driver->pm && dev->driver->pm->runtime_resume)
+		ret = dev->driver->pm->runtime_resume(dev);
+
+	return ret;
+};
+
+int platform_pm_runtime_idle(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *odev = to_omap_device(pdev);
+	int ret;
+
+	ret = pm_runtime_suspend(dev);
+	dev_dbg(dev, "%s [%d]\n", __func__, ret);
+
+	return 0;
+};
+#endif /* CONFIG_PM_RUNTIME */
+
-- 
1.7.0.2

[PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Kevin Hilman]

Hook into the platform bus methods for suspend and resume and
use the omap_device API to automatically idle and enable the
device on suspend and resume.

This allows device drivers to get rid of direct management of their
clocks in their suspend/resume paths, and let omap_device do it for
them.

We currently use the _noirq (late suspend, early resume) versions of
the suspend/resume methods to ensure that the device is not disabled
too early for any drivers also using the _noirq methods.

NOTE: only works for devices with omap_hwmod support.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/pm_bus.c |   61 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c
index 69acaa5..3787da8 100644
--- a/arch/arm/mach-omap2/pm_bus.c
+++ b/arch/arm/mach-omap2/pm_bus.c
@@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev)
 };
 #endif /* CONFIG_PM_RUNTIME */
 
+#ifdef CONFIG_SUSPEND
+int platform_pm_suspend_noirq(struct device *dev)
+{
+	struct device_driver *drv = dev->driver;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *odev = to_omap_device(pdev);
+	int ret = 0;
+
+	if (!drv)
+		return 0;
+
+	if (drv->pm) {
+		if (drv->pm->suspend_noirq)
+			ret = drv->pm->suspend_noirq(dev);
+	}
+
+	if (omap_device_is_valid(odev)) {
+		if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) {
+			dev_dbg(dev, "no automatic bus-level system resume.\n");
+			return 0;
+		}
+
+		dev_dbg(dev, "%s\n", __func__);
+		omap_device_idle(pdev);
+	} else {
+		dev_dbg(dev, "not an omap_device\n");
+	}
+
+	return ret;
+}
+
+int platform_pm_resume_noirq(struct device *dev)
+{
+	struct device_driver *drv = dev->driver;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *odev = to_omap_device(pdev);
+	int ret = 0;
+
+	if (omap_device_is_valid(odev)) {
+		if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) {
+			dev_dbg(dev, "no automatic bus-level system resume.\n");
+			return 0;
+		}
+
+		dev_dbg(dev, "%s\n", __func__);
+		omap_device_enable(pdev);
+	} else {
+		dev_dbg(dev, "not an omap_device\n");
+	}
+
+	if (!drv)
+		return 0;
+
+	if (drv->pm) {
+		if (drv->pm->resume_noirq)
+			ret = drv->pm->resume_noirq(dev);
+	}
+
+	return ret;
+}
+#endif /* CONFIG_SUSPEND */
-- 
1.7.0.2

[PATCH 4/4] OMAP: omap_device: add flag to disable automatic bus-level suspend/resume

[Kevin Hilman]

As part of the runtime PM support, bus-level code can automatically
handle the enable/idle for each omap_device.  There are, however, some
special cases where we don't want the bus-level layer to handle this,
and instead handle it manually.

Specific use cases are for omap_devices that are controlled
inside the idle path (like UART.)

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/plat-omap/include/plat/omap_device.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index 3694b62..2cdbcdd 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -68,12 +68,16 @@ struct omap_device {
 	struct omap_device_pm_latency	*pm_lats;
 	u32				dev_wakeup_lat;
 	u32				_dev_wakeup_lat_limit;
+	u32                             flags;
 	u8				pm_lats_cnt;
 	s8				pm_lat_level;
 	u8				hwmods_cnt;
 	u8				_state;
 };
 
+/* flags for struct omap_device */
+#define OMAP_DEVICE_NO_BUS_SUSPEND     BIT(0)
+
 /* Device driver interface (call via platform_data fn ptrs) */
 
 int omap_device_enable(struct platform_device *pdev);
@@ -142,6 +146,7 @@ struct omap_device_pm_latency {
 	u32 flags;
 };
 
+/* flags for struct omap_device_pm_latency */
 #define OMAP_DEVICE_LATENCY_AUTO_ADJUST BIT(1)
 
 /* Get omap_device pointer from platform_device pointer */
-- 
1.7.0.2

RE: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Nayak, Rajendra]

 

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Friday, May 28, 2010 2:43 AM
> To: linux-omap@vger.kernel.org
> Subject: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume
> 
> Hook into the platform bus methods for suspend and resume and
> use the omap_device API to automatically idle and enable the
> device on suspend and resume.
> 
> This allows device drivers to get rid of direct management of their
> clocks in their suspend/resume paths, and let omap_device do it for
> them.
> 
> We currently use the _noirq (late suspend, early resume) versions of
> the suspend/resume methods to ensure that the device is not disabled
> too early for any drivers also using the _noirq methods.
> 
> NOTE: only works for devices with omap_hwmod support.
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/pm_bus.c |   61 
> ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 61 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/pm_bus.c 
> b/arch/arm/mach-omap2/pm_bus.c
> index 69acaa5..3787da8 100644
> --- a/arch/arm/mach-omap2/pm_bus.c
> +++ b/arch/arm/mach-omap2/pm_bus.c
> @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev)
>  };
>  #endif /* CONFIG_PM_RUNTIME */
>  
> +#ifdef CONFIG_SUSPEND
> +int platform_pm_suspend_noirq(struct device *dev)
> +{
> +	struct device_driver *drv = dev->driver;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_device *odev = to_omap_device(pdev);
> +	int ret = 0;
> +
> +	if (!drv)
> +		return 0;
> +
> +	if (drv->pm) {
> +		if (drv->pm->suspend_noirq)
> +			ret = drv->pm->suspend_noirq(dev);
> +	}
> +
> +	if (omap_device_is_valid(odev)) {
> +		if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) {
> +			dev_dbg(dev, "no automatic bus-level 
> system resume.\n");
> +			return 0;
> +		}
> +
> +		dev_dbg(dev, "%s\n", __func__);
> +		omap_device_idle(pdev);

Is it expected that a device is always in enabled state at this point?
If the device is already in idle a call to omap_device_idle unconditionally
throws up warnings from the omap_device api.

> +	} else {
> +		dev_dbg(dev, "not an omap_device\n");
> +	}
> +
> +	return ret;
> +}
> +
> +int platform_pm_resume_noirq(struct device *dev)
> +{
> +	struct device_driver *drv = dev->driver;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_device *odev = to_omap_device(pdev);
> +	int ret = 0;
> +
> +	if (omap_device_is_valid(odev)) {
> +		if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) {
> +			dev_dbg(dev, "no automatic bus-level 
> system resume.\n");
> +			return 0;
> +		}
> +
> +		dev_dbg(dev, "%s\n", __func__);
> +		omap_device_enable(pdev);
> +	} else {
> +		dev_dbg(dev, "not an omap_device\n");
> +	}
> +
> +	if (!drv)
> +		return 0;
> +
> +	if (drv->pm) {
> +		if (drv->pm->resume_noirq)
> +			ret = drv->pm->resume_noirq(dev);
> +	}
> +
> +	return ret;
> +}
> +#endif /* CONFIG_SUSPEND */
> -- 
> 1.7.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Kevin Hilman]

"Nayak, Rajendra" <rnayak@ti.com> writes:

[...]

>> diff --git a/arch/arm/mach-omap2/pm_bus.c 
>> b/arch/arm/mach-omap2/pm_bus.c
>> index 69acaa5..3787da8 100644
>> --- a/arch/arm/mach-omap2/pm_bus.c
>> +++ b/arch/arm/mach-omap2/pm_bus.c
>> @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev)
>>  };
>>  #endif /* CONFIG_PM_RUNTIME */
>>  
>> +#ifdef CONFIG_SUSPEND
>> +int platform_pm_suspend_noirq(struct device *dev)
>> +{
>> +	struct device_driver *drv = dev->driver;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct omap_device *odev = to_omap_device(pdev);
>> +	int ret = 0;
>> +
>> +	if (!drv)
>> +		return 0;
>> +
>> +	if (drv->pm) {
>> +		if (drv->pm->suspend_noirq)
>> +			ret = drv->pm->suspend_noirq(dev);
>> +	}
>> +
>> +	if (omap_device_is_valid(odev)) {
>> +		if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) {
>> +			dev_dbg(dev, "no automatic bus-level 
>> system resume.\n");
>> +			return 0;
>> +		}
>> +
>> +		dev_dbg(dev, "%s\n", __func__);
>> +		omap_device_idle(pdev);
>
> Is it expected that a device is always in enabled state at this point?
> If the device is already in idle a call to omap_device_idle unconditionally
> throws up warnings from the omap_device api.

Hmm, good point.  The device may already be idled (via runtime PM, or
maybe because it was never enabled.)

There are two options:

1. fixup the warnings in the omap_device_idle() to allow multiple
   calls to _idle()

2. Add an omap_device_is_idle() check before calling _idle()

I much prefer (1).  

Paul?

Kevin

[PATCH] OMAP1: PM: add simple runtime PM layer to manage clocks

[Kevin Hilman]

On OMAP1, we do not have omap_device + omap_hwmod to manage the
device-specific idle, enable and shutdown.  Instead, just
enable/disable device clocks automatically at the runtime PM level.

This allows drivers to not have any OMAP1 specific clock management
and allows them to simply use the runtime PM API to manage clocks.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
This patch applies on top of the initial runtime PM support series:

  [PATCH 0/4] runtime PM support at the bus level

posted to linux-omap on May 27th.  It is also now included in
my pm-wip/runtime branch in my git tree.

 arch/arm/mach-omap1/Makefile |    2 +-
 arch/arm/mach-omap1/pm_bus.c |   77 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap1/pm_bus.c

diff --git a/arch/arm/mach-omap1/Makefile b/arch/arm/mach-omap1/Makefile
index ea231c7..fd4df71 100644
--- a/arch/arm/mach-omap1/Makefile
+++ b/arch/arm/mach-omap1/Makefile
@@ -12,7 +12,7 @@ obj-$(CONFIG_OMAP_MPU_TIMER)	+= time.o
 obj-$(CONFIG_OMAP_32K_TIMER)	+= timer32k.o
 
 # Power Management
-obj-$(CONFIG_PM) += pm.o sleep.o
+obj-$(CONFIG_PM) += pm.o sleep.o pm_bus.o
 
 # DSP
 obj-$(CONFIG_OMAP_MBOX_FWK)	+= mailbox_mach.o
diff --git a/arch/arm/mach-omap1/pm_bus.c b/arch/arm/mach-omap1/pm_bus.c
new file mode 100644
index 0000000..46a34fc
--- /dev/null
+++ b/arch/arm/mach-omap1/pm_bus.c
@@ -0,0 +1,77 @@
+/*
+ * Runtime PM support code for OMAP1
+ *
+ * Author: Kevin Hilman, Deep Root Systems, LLC
+ *
+ * Copyright (C) 2010 Texas Instruments, Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+
+#include <plat/omap_device.h>
+#include <plat/omap-pm.h>
+
+#ifdef CONFIG_PM_RUNTIME
+int platform_pm_runtime_suspend(struct device *dev)
+{
+	struct clk *iclk, *fclk;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	if (dev->driver->pm && dev->driver->pm->runtime_suspend)
+		ret = dev->driver->pm->runtime_suspend(dev);
+
+	fclk = clk_get(dev, "fck");
+	if (!IS_ERR(fclk)) {
+		clk_disable(fclk);
+		clk_put(fclk);
+	}
+
+	iclk = clk_get(dev, "ick");
+	if (!IS_ERR(iclk)) {
+		clk_disable(iclk);
+		clk_put(iclk);
+	}
+
+	return 0;
+};
+
+int platform_pm_runtime_resume(struct device *dev)
+{
+	int r, ret = 0;
+	struct clk *iclk, *fclk;
+
+	iclk = clk_get(dev, "fck");
+	if (!IS_ERR(iclk)) {
+		clk_enable(iclk);
+		clk_put(iclk);
+	}
+
+	fclk = clk_get(dev, "fck");
+	if (!IS_ERR(fclk)) {
+		clk_enable(fclk);
+		clk_put(fclk);
+	}
+
+	if (dev->driver->pm && dev->driver->pm->runtime_resume)
+		ret = dev->driver->pm->runtime_resume(dev);
+
+	return ret;
+};
+
+int platform_pm_runtime_idle(struct device *dev)
+{
+	ret = pm_runtime_suspend(dev);
+	dev_dbg(dev, "%s [%d]\n", __func__, ret);
+
+	return 0;
+};
+#endif /* CONFIG_PM_RUNTIME */
-- 
1.7.0.2

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Paul Walmsley]

On Tue, 1 Jun 2010, Kevin Hilman wrote:

> "Nayak, Rajendra" <rnayak@ti.com> writes:
> 
> [...]
> 
> >> diff --git a/arch/arm/mach-omap2/pm_bus.c 
> >> b/arch/arm/mach-omap2/pm_bus.c
> >> index 69acaa5..3787da8 100644
> >> --- a/arch/arm/mach-omap2/pm_bus.c
> >> +++ b/arch/arm/mach-omap2/pm_bus.c
> >> @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev)
> >>  };
> >>  #endif /* CONFIG_PM_RUNTIME */
> >>  
> >> +#ifdef CONFIG_SUSPEND
> >> +int platform_pm_suspend_noirq(struct device *dev)
> >> +{
> >> +	struct device_driver *drv = dev->driver;
> >> +	struct platform_device *pdev = to_platform_device(dev);
> >> +	struct omap_device *odev = to_omap_device(pdev);
> >> +	int ret = 0;
> >> +
> >> +	if (!drv)
> >> +		return 0;
> >> +
> >> +	if (drv->pm) {
> >> +		if (drv->pm->suspend_noirq)
> >> +			ret = drv->pm->suspend_noirq(dev);
> >> +	}
> >> +
> >> +	if (omap_device_is_valid(odev)) {
> >> +		if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) {
> >> +			dev_dbg(dev, "no automatic bus-level 
> >> system resume.\n");
> >> +			return 0;
> >> +		}
> >> +
> >> +		dev_dbg(dev, "%s\n", __func__);
> >> +		omap_device_idle(pdev);
> >
> > Is it expected that a device is always in enabled state at this point?
> > If the device is already in idle a call to omap_device_idle unconditionally
> > throws up warnings from the omap_device api.
> 
> Hmm, good point.  The device may already be idled (via runtime PM, or
> maybe because it was never enabled.)
> 
> There are two options:
> 
> 1. fixup the warnings in the omap_device_idle() to allow multiple
>    calls to _idle()
> 
> 2. Add an omap_device_is_idle() check before calling _idle()
> 
> I much prefer (1).  
> 
> Paul?

As far as I can tell, it's not safe for upper-layer code to idle a device 
like this.  The driver itself needs to be aware of the device's idle 
state.  For example, if the driver has exported some symbols, and some 
other code is calling one of those functions, it's the driver code that 
needs to be aware of its own idle-state and to return some kind of error 
if the device is quiesced.  I don't think there's an easy way for 
upper-layer code to do that.

The runtime PM-related code, however, should be safe, since it's initiated 
by the driver itself.


- Paul

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> On Tue, 1 Jun 2010, Kevin Hilman wrote:
>
>> "Nayak, Rajendra" <rnayak@ti.com> writes:
>> 
>> [...]
>> 
>> >> diff --git a/arch/arm/mach-omap2/pm_bus.c 
>> >> b/arch/arm/mach-omap2/pm_bus.c
>> >> index 69acaa5..3787da8 100644
>> >> --- a/arch/arm/mach-omap2/pm_bus.c
>> >> +++ b/arch/arm/mach-omap2/pm_bus.c
>> >> @@ -70,3 +70,64 @@ int platform_pm_runtime_idle(struct device *dev)
>> >>  };
>> >>  #endif /* CONFIG_PM_RUNTIME */
>> >>  
>> >> +#ifdef CONFIG_SUSPEND
>> >> +int platform_pm_suspend_noirq(struct device *dev)
>> >> +{
>> >> +	struct device_driver *drv = dev->driver;
>> >> +	struct platform_device *pdev = to_platform_device(dev);
>> >> +	struct omap_device *odev = to_omap_device(pdev);
>> >> +	int ret = 0;
>> >> +
>> >> +	if (!drv)
>> >> +		return 0;
>> >> +
>> >> +	if (drv->pm) {
>> >> +		if (drv->pm->suspend_noirq)
>> >> +			ret = drv->pm->suspend_noirq(dev);
>> >> +	}
>> >> +
>> >> +	if (omap_device_is_valid(odev)) {
>> >> +		if (odev->flags & OMAP_DEVICE_NO_BUS_SUSPEND) {
>> >> +			dev_dbg(dev, "no automatic bus-level 
>> >> system resume.\n");
>> >> +			return 0;
>> >> +		}
>> >> +
>> >> +		dev_dbg(dev, "%s\n", __func__);
>> >> +		omap_device_idle(pdev);
>> >
>> > Is it expected that a device is always in enabled state at this point?
>> > If the device is already in idle a call to omap_device_idle unconditionally
>> > throws up warnings from the omap_device api.
>> 
>> Hmm, good point.  The device may already be idled (via runtime PM, or
>> maybe because it was never enabled.)
>> 
>> There are two options:
>> 
>> 1. fixup the warnings in the omap_device_idle() to allow multiple
>>    calls to _idle()
>> 
>> 2. Add an omap_device_is_idle() check before calling _idle()
>> 
>> I much prefer (1).  
>> 
>> Paul?
>
> As far as I can tell, it's not safe for upper-layer code to idle a device 
> like this.  The driver itself needs to be aware of the device's idle 
> state.  

The driver is made aware using the standard dev_pm_ops callback for
suspend and resume.

Note that this is not just any upper-layer code that is blindly idling
a device.  This is only happening at the system-suspend time, and in
particular this is happening using the _noirq() versions of the
dev_pm_ops hooks.

> For example, if the driver has exported some symbols, and some 
> other code is calling one of those functions, it's the driver code that 
> needs to be aware of its own idle-state and to return some kind of error 
> if the device is quiesced.  I don't think there's an easy way for 
> upper-layer code to do that.

Drivers already have to handle this today.  If they have been
suspended and their functions have been called, they have to return
errors.  This patch doesn't change that.

Kevin

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Paul Walmsley]

On Mon, 21 Jun 2010, Kevin Hilman wrote:

> Paul Walmsley <paul@pwsan.com> writes:
> 
> > As far as I can tell, it's not safe for upper-layer code to idle a device 
> > like this.  The driver itself needs to be aware of the device's idle 
> > state.  
> 
> The driver is made aware using the standard dev_pm_ops callback for
> suspend and resume.

I guess the intent of your patch is to avoid having to patch in 
omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume 
callbacks?  If so, some comments:

- dev_pm_ops->suspend_noirq() is intended for use on devices with
  shared interrupts.  Right now the OMAP2+ codebase doesn't use any
  shared interrupts for on-chip devices, as far as I can see.  It
  looks like you use ->suspend_noirq() to ensure your code runs after
  the existing driver suspend functions.  Using ->suspend_noirq() for
  this purpose seems like a hack.  A better place for that code would
  be in a bus->pm->suspend() callback, which will run after the
  device's dev_pm_ops->suspend() callback.

- It is not safe to call omap_device_{idle,enable}() from DPM
  callbacks as the patch does, because they will race with runtime PM
  calls to omap_device_{idle,enable}().  (This is part of the reason
  why it's important to be anal about enforcing the
  omap_device_{enable,idle,shutdown}() state transition limitations -
  so we get warned early when these types of conflicts appear.)

  omap_device*() calls should either be in runtime PM functions, or
  DPM ->suspend/resume() callbacks, but not both.  Since we've decided
  to focus on runtime PM power management as our primary driver power
  management approach, and we expect drivers to aggressively idle
  their devices, it seems best to keep the omap_device*() functions in
  runtime PM code.

  At that point, the common DPM suspend/idle callbacks that you
  propose can simply block until runtime PM indicates that the device
  is idle.  For example, you could use a per-omap_device mutex.
  A sketch of a sample implementation follows this message.


- Paul


In omap_device.c, add

int omap_device_suspend_mpu_irqs(struct omap_device *od)
{
	/*
         * For each hwmod, for each MPU IRQ, save current state,
         * disable_irq(irq)
         */
}

int omap_device_resume_mpu_irqs(struct omap_device *od)
{
	/*
         * For each hwmod, for each MPU IRQ, enable_irq(irq) if
         * it was previously
         */
}


In your bus->pm->suspend()/resume() functions:

int omap_bus_suspend(struct device *dev, pm_message_t state)
{
        dev_dbg("waiting for device %s to go idle\n", dev_name(dev));

        mutex_lock(&od->active_mutex);

        /* Device guaranteed to be idle at this point */

        /*
         * do anything else necessary to ensure that driver code is not
         * entered after the unlock()
         */
        omap_device_suspend_mpu_irqs(od);

        mutex_unlock(&od->active_mutex);

        return 0;
}

int omap_bus_resume(struct device *dev, pm_message_t state)
{
        mutex_lock(&od->active_mutex);

        /* Device guaranteed to be idle at this point */
    
        /*
         * do anything else necessary to ensure that driver code can
         * be entered after the unlock()
         */
        omap_device_resume_mpu_irqs(od);

        mutex_unlock(&od->active_mutex);

        return 0;
}


Then in the omap_device code, add a mutex active_mutex in struct
omap_device, init the mutex in omap_device_build_ss(), then:

int omap_device_enable(struct platform_device *pdev)
{
       struct omap_device *od;

       od = _find_by_pdev(pdev);

       ...
       WARN(!mutex_trylock(&od->active_mutex),
            "State transition violation - should never happen\n");
       mutex_lock(&od->active_mutex);
       ...
}
 
int omap_device_idle(struct platform_device *pdev)
{
        struct omap_device *od;

        od = _find_by_pdev(pdev);

        ...
        mutex_unlock(&od->active_mutex);
        ...
}

int omap_device_shutdown(struct platform_device *pdev)
{
        struct omap_device *od;

        od = _find_by_pdev(pdev);

        ...
        mutex_unlock(&od->active_mutex);
        ...
}


The driver needs the usual: 

- add a DPM ->suspend() function that tries to stop whatever the
  device is doing if it's in the middle of something that can be
  stopped;

- ensure all paths that start new I/O check dev->power.status, and
  return an error if it is DPM_SUSPENDING



- Paul

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> On Mon, 21 Jun 2010, Kevin Hilman wrote:
>
>> Paul Walmsley <paul@pwsan.com> writes:
>> 
>> > As far as I can tell, it's not safe for upper-layer code to idle a device 
>> > like this.  The driver itself needs to be aware of the device's idle 
>> > state.  
>> 
>> The driver is made aware using the standard dev_pm_ops callback for
>> suspend and resume.
>
> I guess the intent of your patch is to avoid having to patch in 
> omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume 
> callbacks?  

Partially.  Actually, I think many (most?) drivers can get rid of
static suspend/resume paths all together and just use runtime PM.

There are some cases though (MMC comes to mind, more on that below)
where static suspend has some extra steps necessary and behaves
differently than runtime PM.

For those cases, the goal is as you stated.  Basically, to avoid having
all the drivers having to call omap_device_idle().

> If so, some comments:
> - dev_pm_ops->suspend_noirq() is intended for use on devices with
>   shared interrupts.  

Just to clarify.  The functions I'm overriding here is the bus
functions (bus->pm->[suspend|resume]_noirq(), not any driver functions

Indeed, shared IRQs were an intended usage, but I don't see that as a
requirement.  Indeed, Documentation/power/devices.txt even seems to
suggest that the _noirq version is the place to turn the device "as
off as possible:

    "For simple drivers, suspend might quiesce the device using class code
    and then turn its hardware as "off" as possible during suspend_noirq"

>   Right now the OMAP2+ codebase doesn't use any
>   shared interrupts for on-chip devices, as far as I can see.  It
>   looks like you use ->suspend_noirq() to ensure your code runs after
>   the existing driver suspend functions.  

No. The primary reason for using _noirq() is that if any driver ever
did use the _noirq() hooks (for any atomic activity, or late wakeup
detection, etc.)  the device will still be accessible.

>   Using ->suspend_noirq() for this purpose seems like a hack.  
>   A better place for that code would be in a bus->pm->suspend()
>   callback, which will run after the device's dev_pm_ops->suspend()
>   callback.

I originally put it in bus->pm->suspend, but moved it to
bus->pm->suspend_noirq() since I didn't do a full audit so see
if anything was using the _noirq hooks.

If we want to have a requirement that no on-chip devices can use the
_noirq hooks (or at least they can't touch hardware in those hooks)
then I can move it back to bus->pm->suspend().  But personally, I
would see that as an artificial requirement based on a very
restrictive definiton of the _noirq() methods.

> - It is not safe to call omap_device_{idle,enable}() from DPM
>   callbacks as the patch does, because they will race with runtime PM
>   calls to omap_device_{idle,enable}().  

No, they cannot race.  

Runtime PM transitions are prevented by the system PM core during a
system PM transition.  The system suspend path does a pm_runtime_get()
for each device being suspended, and then does a _put() upon resume.
(see drivers/base/power/main.c, grep for pm_runtime_)

>   (This is part of the reason
>   why it's important to be anal about enforcing the
>   omap_device_{enable,idle,shutdown}() state transition limitations -
>   so we get warned early when these types of conflicts appear.)
>
>   omap_device*() calls should either be in runtime PM functions, or
>   DPM ->suspend/resume() callbacks, but not both.  

Why not both?  I don't follow the reason for this restriction.  We
certainly did the equivalent clock enable/disable calls in both cases
in the past.

>   Since we've decided
>   to focus on runtime PM power management as our primary driver power
>   management approach, and we expect drivers to aggressively idle
>   their devices, it seems best to keep the omap_device*() functions in
>   runtime PM code.

I agree, mostly.

As I mentioned above, I expect most drivers not to even have system PM
methods implemented.  They should implement everything as runtime PM.
However, there will be some exceptions.

The use case that brought this patch into existence was the MMC driver
where the suspend hook had to do some extra "stuff" before suspending.
Even if already runtime suspended, the MMC suspend hook has to
re-enable the device do "stuff" and then re-idle the device.

Initially, I tried to just use the same runtime PM calls in the
suspend hook, but that doesn't work since runtime PM transitions are
prevented during system PM.  So, I was forced to call
omap_device_idle() in the suspend path, which led to the decision to
move that into common code.

>   At that point, the common DPM suspend/idle callbacks that you
>   propose can simply block until runtime PM indicates that the device
>   is idle.  
>
>   For example, you could use a per-omap_device mutex.
>   A sketch of a sample implementation follows this message.

That is an option, but since there is no potential racing between
runtime PM and system PM, I don't see the need for extra locking.

Also, in your example, by manually [dis|en]abling IRQs, you're
re-inventing the _noirq versions of the hooks, even though the DPM
core is going to do it (again) soon.  

I'd rather just use the noirq hooks that are already in place, and let
the driver manage IRQs itself.  Especially when it comes to wakeup
IRQs, I think it problematic to start managing IRQs in common code.

Kevin


>
>
> - Paul
>
>
> In omap_device.c, add
>
> int omap_device_suspend_mpu_irqs(struct omap_device *od)
> {
> 	/*
>          * For each hwmod, for each MPU IRQ, save current state,
>          * disable_irq(irq)
>          */
> }
>
> int omap_device_resume_mpu_irqs(struct omap_device *od)
> {
> 	/*
>          * For each hwmod, for each MPU IRQ, enable_irq(irq) if
>          * it was previously
>          */
> }
>
>
> In your bus->pm->suspend()/resume() functions:
>
> int omap_bus_suspend(struct device *dev, pm_message_t state)
> {
>         dev_dbg("waiting for device %s to go idle\n", dev_name(dev));
>
>         mutex_lock(&od->active_mutex);
>
>         /* Device guaranteed to be idle at this point */
>
>         /*
>          * do anything else necessary to ensure that driver code is not
>          * entered after the unlock()
>          */
>         omap_device_suspend_mpu_irqs(od);
>
>         mutex_unlock(&od->active_mutex);
>
>         return 0;
> }
>
> int omap_bus_resume(struct device *dev, pm_message_t state)
> {
>         mutex_lock(&od->active_mutex);
>
>         /* Device guaranteed to be idle at this point */
>     
>         /*
>          * do anything else necessary to ensure that driver code can
>          * be entered after the unlock()
>          */
>         omap_device_resume_mpu_irqs(od);
>
>         mutex_unlock(&od->active_mutex);
>
>         return 0;
> }
>
>
> Then in the omap_device code, add a mutex active_mutex in struct
> omap_device, init the mutex in omap_device_build_ss(), then:
>
> int omap_device_enable(struct platform_device *pdev)
> {
>        struct omap_device *od;
>
>        od = _find_by_pdev(pdev);
>
>        ...
>        WARN(!mutex_trylock(&od->active_mutex),
>             "State transition violation - should never happen\n");
>        mutex_lock(&od->active_mutex);
>        ...
> }
>  
> int omap_device_idle(struct platform_device *pdev)
> {
>         struct omap_device *od;
>
>         od = _find_by_pdev(pdev);
>
>         ...
>         mutex_unlock(&od->active_mutex);
>         ...
> }
>
> int omap_device_shutdown(struct platform_device *pdev)
> {
>         struct omap_device *od;
>
>         od = _find_by_pdev(pdev);
>
>         ...
>         mutex_unlock(&od->active_mutex);
>         ...
> }
>
>
> The driver needs the usual: 
>
> - add a DPM ->suspend() function that tries to stop whatever the
>   device is doing if it's in the middle of something that can be
>   stopped;
>
> - ensure all paths that start new I/O check dev->power.status, and
>   return an error if it is DPM_SUSPENDING
>
>
>
> - Paul

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Paul Walmsley]

Hi Kevin,

On Mon, 21 Jun 2010, Kevin Hilman wrote:

> Paul Walmsley <paul@pwsan.com> writes:
> 
> > I guess the intent of your patch is to avoid having to patch in 
> > omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume 
> > callbacks?  
> 
> Partially.  Actually, I think many (most?) drivers can get rid of
> static suspend/resume paths all together and just use runtime PM.
> 
> There are some cases though (MMC comes to mind, more on that below)
> where static suspend has some extra steps necessary and behaves
> differently than runtime PM.

It's not just MMC, any driver that can be in the middle of doing something 
during DPM ->suspend() will need to have DPM suspend code to stop what 
it's doing and quiesce the device before returning from the suspend 
callback.  Either that, or ->suspend() needs to return an error and block 
the system suspend process...

> > If so, some comments:
> > - dev_pm_ops->suspend_noirq() is intended for use on devices with
> >   shared interrupts.  
> 
> Just to clarify.  The functions I'm overriding here is the bus
> functions (bus->pm->[suspend|resume]_noirq(), not any driver functions

OK, I see that now - this code was confusing in the patch's 
platform_pm_suspend_noirq():

+       if (drv->pm) {
+               if (drv->pm->suspend_noirq)
+                       ret = drv->pm->suspend_noirq(dev);
+       }

This is already done by the DPM core in 
drivers/base/power/main.c:device_suspend_noirq() and will result in 
re-executing the driver's suspend_noirq function, which is potentially 
quite bad.  Same thing for platform_pm_resume_noirq() in the patch.

> Indeed, shared IRQs were an intended usage, but I don't see that as a
> requirement.  Indeed, Documentation/power/devices.txt even seems to
> suggest that the _noirq version is the place to turn the device "as
> off as possible:
> 
>     "For simple drivers, suspend might quiesce the device using class code
>     and then turn its hardware as "off" as possible during suspend_noirq"

Further down in that file, it says:

"2.  The suspend methods should quiesce the device to stop it from 
     performing I/O.  They also may save the device registers and put it into the
     appropriate low-power state, depending on the bus type the device
     is on, and they may enable wakeup events."

... and then:

"The majority of subsystems and device drivers need not implement 
 [the suspend_noirq] callback.  However, bus types allowing devices to 
 share interrupt vectors, like PCI, generally need it; otherwise a driver 
 might encounter an error during the suspend phase by fielding a shared 
 interrupt generated by some other device after its own device had been 
 set to low power."

> >   Right now the OMAP2+ codebase doesn't use any
> >   shared interrupts for on-chip devices, as far as I can see.  It
> >   looks like you use ->suspend_noirq() to ensure your code runs after
> >   the existing driver suspend functions.  
> 
> No. The primary reason for using _noirq() is that if any driver ever
> did use the _noirq() hooks (for any atomic activity, or late wakeup
> detection, etc.)  the device will still be accessible.
>
> >   Using ->suspend_noirq() for this purpose seems like a hack.  
> >   A better place for that code would be in a bus->pm->suspend()
> >   callback, which will run after the device's dev_pm_ops->suspend()
> >   callback.
> 
> I originally put it in bus->pm->suspend, but moved it to
> bus->pm->suspend_noirq() since I didn't do a full audit so see
> if anything was using the _noirq hooks.
> 
> If we want to have a requirement that no on-chip devices can use the
> _noirq hooks (or at least they can't touch hardware in those hooks)
> then I can move it back to bus->pm->suspend().

That sounds like the best argument for keeping these hooks in 
_noirq() -- some driver writer may be likely to use suspend_noirq() 
without understanding that they shouldn't... maybe we should add some code 
that looks for this and warns.

But thinking about it further, it also seems possible that a handful of 
OMAP drivers might use shared IRQs at some point in the future.  DSS comes 
to mind -- that really is a shared IRQ.  So, _noirq() is fine, then.

> But personally, I would see that as an artificial requirement based on 
> a very restrictive definiton of the _noirq() methods.

It's just the definition from the kernel documentation.

> > - It is not safe to call omap_device_{idle,enable}() from DPM
> >   callbacks as the patch does, because they will race with runtime PM
> >   calls to omap_device_{idle,enable}().  
> 
> No, they cannot race.  
> 
> Runtime PM transitions are prevented by the system PM core during a
> system PM transition.  The system suspend path does a pm_runtime_get()
> for each device being suspended, and then does a _put() upon resume.
> (see drivers/base/power/main.c, grep for pm_runtime_)

Yes, you are correct in terms of my original statement.  But the problem 
-- and the race -- that I did a poor job of describing is still present.

What if this pm_bus ->suspend_noirq() calls omap_device_idle() while other 
code in the driver is still in the middle of interacting with the device?  
Although that would certainly be a driver bug, I certainly wouldn't trust 
all of our existing driver DPM suspend* functions to adequately wait for 
in-progress operations to complete and block new operations from starting 
before returning.

We shouldn't idle (and potentially power off) a device unless we know its 
driver is done with it.  In an ideal world, this would always be taken 
care of by driver ->suspend()/->suspend_noirq() functions.  But we know 
this isn't always the case -- perhaps it's not even the case for most of 
the OMAP drivers.

We can use the device's runtime PM state to figure out whether the driver 
thinks it's done with the device.  Unfortunately, the existing Linux DPM 
suspend code makes it difficult to deal with this by calling 
pm_runtime_get_noresume() before entering suspend and never calling 
pm_runtime_put() until after resume -- no idea why.  That strikes me as a 
bug.  From a semantic perspective it is certainly confusing: the PM 
runtime implementation will think the device is still active after it's 
been suspended.  I wouldn't want the full Linux system suspend code to 
enter some low power state while some driver still thought its device 
should stay powered.

Anyway, given this strange behavior, I think there is probably a simple 
workaround.  Rather than calling omap_device_idle() in 
platform_pm_suspend_noirq(), how about calling pm_runtime_put_sync()?  It 
probably needs a comment to indicate that it's intended to balance the 
pm_runtime_get_noresume() that's in dpm_prepare().  Similarly it should be 
possible to replace the omap_device_enable() in platform_pm_resume_noirq() 
with pm_runtime_get_sync().  That way the pm_bus code will not call 
omap_device_idle() on a device that the driver has not yet indicated is 
safe to enter idle.

> >   (This is part of the reason
> >   why it's important to be anal about enforcing the
> >   omap_device_{enable,idle,shutdown}() state transition limitations -
> >   so we get warned early when these types of conflicts appear.)
> >
> >   omap_device*() calls should either be in runtime PM functions, or
> >   DPM ->suspend/resume() callbacks, but not both.  
> 
> Why not both?  I don't follow the reason for this restriction.  We
> certainly did the equivalent clock enable/disable calls in both cases
> in the past.

clk_enable()/clk_disable() use-count, unlike the omap_device_idle() 
function.  So if an existing driver calls clk_enable()/clk_disable() in 
its suspend function, if other driver code had been previously entered 
that called clk_enable(), the kernel will not try to disable the clock 
until that code completes.

Also, clk_disable()/clk_enable() never touched the OCP_SYSCONFIG.*IDLEMODE 
bits.  So even if clk_disable() told the PRCM to turn the device clock 
off, the PRCM might not actually turn the clock off if the device doesn't 
idle-ack, which would prevent the enclosing powerdomain from actually 
turning off and destroying device context.  But with omap_device_idle(), 
if we have any hwmods that have the HWMOD_SWSUP_SIDLE/HWMOD_SWSUP_MSTANDBY 
flags set, those hwmods are going to be force-idled.  The PRCM will shut 
off their clocks and potentially cause device context to be lost if the 
driver wasn't expecting it.

> >   Since we've decided
> >   to focus on runtime PM power management as our primary driver power
> >   management approach, and we expect drivers to aggressively idle
> >   their devices, it seems best to keep the omap_device*() functions in
> >   runtime PM code.
> 
> I agree, mostly.
> 
> As I mentioned above, I expect most drivers not to even have system PM
> methods implemented.  They should implement everything as runtime PM.
> However, there will be some exceptions.
> 
> The use case that brought this patch into existence was the MMC driver
> where the suspend hook had to do some extra "stuff" before suspending.
> Even if already runtime suspended, the MMC suspend hook has to
> re-enable the device do "stuff" and then re-idle the device.

I don't think it's just MMC; any driver that does external I/O will 
probably need a DPM ->suspend() function to ensure that its external I/O 
is completed before returning from ->suspend().  Even drivers that are 
just doing some internal device work will need to know somehow that the 
device is finished before completing ->suspend().

> Initially, I tried to just use the same runtime PM calls in the suspend 
> hook, but that doesn't work since runtime PM transitions are prevented 
> during system PM.  So, I was forced to call omap_device_idle() in the 
> suspend path, which led to the decision to move that into common code.

Thanks, I see that now.  I think as long as the pm_bus common code ensures 
that the PM runtime system thinks that the device is idle, it should be 
okay to have this in common code.

> >   At that point, the common DPM suspend/idle callbacks that you
> >   propose can simply block until runtime PM indicates that the device
> >   is idle.  
> >
> >   For example, you could use a per-omap_device mutex.
> >   A sketch of a sample implementation follows this message.
> 
> That is an option, but since there is no potential racing between
> runtime PM and system PM, I don't see the need for extra locking.

Yep, but as mentioned above, we still need to check to see if the driver 
thinks that the device is in use before calling omap_device_idle().  
(Preferably it should just use the PM runtime put code for this rather 
than calling omap_device_idle() directly).

> Also, in your example, by manually [dis|en]abling IRQs, you're
> re-inventing the _noirq versions of the hooks, even though the DPM
> core is going to do it (again) soon.
>
> I'd rather just use the noirq hooks that are already in place, and let
> the driver manage IRQs itself.  Especially when it comes to wakeup
> IRQs, I think it problematic to start managing IRQs in common code.

Yeah, that part of the suggestion wasn't right.


- Paul

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Paul Walmsley]

One correction here:

On Wed, 23 Jun 2010, Paul Walmsley wrote:

> On Mon, 21 Jun 2010, Kevin Hilman wrote:
> 
> > Just to clarify.  The functions I'm overriding here is the bus
> > functions (bus->pm->[suspend|resume]_noirq(), not any driver functions
> 
> OK, I see that now - this code was confusing in the patch's 
> platform_pm_suspend_noirq():
> 
> +       if (drv->pm) {
> +               if (drv->pm->suspend_noirq)
> +                       ret = drv->pm->suspend_noirq(dev);
> +       }
> 
> This is already done by the DPM core in 
> drivers/base/power/main.c:device_suspend_noirq() and will result in 
> re-executing the driver's suspend_noirq function, which is potentially 
> quite bad.  Same thing for platform_pm_resume_noirq() in the patch.

Sorry, misread this also.  Indeed, something like this is necessary in 
your platform bus override code - so please ignore this part of the 
comments.


- Paul

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> On Mon, 21 Jun 2010, Kevin Hilman wrote:
>
>> Paul Walmsley <paul@pwsan.com> writes:
>> 
>> > I guess the intent of your patch is to avoid having to patch in 
>> > omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume 
>> > callbacks?  
>> 
>> Partially.  Actually, I think many (most?) drivers can get rid of
>> static suspend/resume paths all together and just use runtime PM.
>> 
>> There are some cases though (MMC comes to mind, more on that below)
>> where static suspend has some extra steps necessary and behaves
>> differently than runtime PM.
>
> It's not just MMC, any driver that can be in the middle of doing something 
> during DPM ->suspend() will need to have DPM suspend code to stop what 
> it's doing and quiesce the device before returning from the suspend 
> callback.  

I don't think we do a very good job of that today in most drivers, but
your point is valid.

Probably best to "trick" the runtime PM core by doing a runtime PM
put/get in the bus code as you suggest below to avoid this kind of
potential conflict.

> Either that, or ->suspend() needs to return an error and block 
> the system suspend process...
>

[...]

>> >   Right now the OMAP2+ codebase doesn't use any
>> >   shared interrupts for on-chip devices, as far as I can see.  It
>> >   looks like you use ->suspend_noirq() to ensure your code runs after
>> >   the existing driver suspend functions.  
>> 
>> No. The primary reason for using _noirq() is that if any driver ever
>> did use the _noirq() hooks (for any atomic activity, or late wakeup
>> detection, etc.)  the device will still be accessible.
>>
>> >   Using ->suspend_noirq() for this purpose seems like a hack.  
>> >   A better place for that code would be in a bus->pm->suspend()
>> >   callback, which will run after the device's dev_pm_ops->suspend()
>> >   callback.
>> 
>> I originally put it in bus->pm->suspend, but moved it to
>> bus->pm->suspend_noirq() since I didn't do a full audit so see
>> if anything was using the _noirq hooks.
>> 
>> If we want to have a requirement that no on-chip devices can use the
>> _noirq hooks (or at least they can't touch hardware in those hooks)
>> then I can move it back to bus->pm->suspend().
>
> That sounds like the best argument for keeping these hooks in 
> _noirq() -- some driver writer may be likely to use suspend_noirq() 
> without understanding that they shouldn't... maybe we should add some code 
> that looks for this and warns.
>
> But thinking about it further, it also seems possible that a handful of 
> OMAP drivers might use shared IRQs at some point in the future.  DSS comes 
> to mind -- that really is a shared IRQ.  So, _noirq() is fine, then.

OK.

[...]

>> > - It is not safe to call omap_device_{idle,enable}() from DPM
>> >   callbacks as the patch does, because they will race with runtime PM
>> >   calls to omap_device_{idle,enable}().  
>> 
>> No, they cannot race.  
>> 
>> Runtime PM transitions are prevented by the system PM core during a
>> system PM transition.  The system suspend path does a pm_runtime_get()
>> for each device being suspended, and then does a _put() upon resume.
>> (see drivers/base/power/main.c, grep for pm_runtime_)
>
> Yes, you are correct in terms of my original statement.  But the problem 
> -- and the race -- that I did a poor job of describing is still present.
>
> What if this pm_bus ->suspend_noirq() calls omap_device_idle() while other 
> code in the driver is still in the middle of interacting with the device?  
> Although that would certainly be a driver bug, I certainly wouldn't trust 
> all of our existing driver DPM suspend* functions to adequately wait for 
> in-progress operations to complete and block new operations from starting 
> before returning.

Yes, I see the point now, and I agree that this is a problem.

> We shouldn't idle (and potentially power off) a device unless we know its 
> driver is done with it.  In an ideal world, this would always be taken 
> care of by driver ->suspend()/->suspend_noirq() functions.  But we know 
> this isn't always the case -- perhaps it's not even the case for most of 
> the OMAP drivers.

Yeah, I don't think we handle this very well currently in most drivers.

> We can use the device's runtime PM state to figure out whether the driver 
> thinks it's done with the device.  Unfortunately, the existing Linux DPM 
> suspend code makes it difficult to deal with this by calling 
> pm_runtime_get_noresume() before entering suspend and never calling 
> pm_runtime_put() until after resume -- no idea why.  

I gathered it was intended to minimize potential conflicts between
system PM and runtime PM, but not sure.  I may ask on linux-pm.

> That strikes me as a bug.  From a semantic perspective it is certainly
> confusing: the PM runtime implementation will think the device is
> still active after it's been suspended.  I wouldn't want the full
> Linux system suspend code to enter some low power state while some
> driver still thought its device should stay powered.

Completely agree here, and this is the root cause of all this funny
business in the first place.

If I we could just put pm_runtime_put_sync() in the driver's suspend
routine (and _get_sync() in the resume routine) this patch would be
totally unncessary.

> Anyway, given this strange behavior, I think there is probably a simple 
> workaround.  Rather than calling omap_device_idle() in 
> platform_pm_suspend_noirq(), how about calling pm_runtime_put_sync()?  It 
> probably needs a comment to indicate that it's intended to balance the 
> pm_runtime_get_noresume() that's in dpm_prepare().  Similarly it should be 
> possible to replace the omap_device_enable() in platform_pm_resume_noirq() 
> with pm_runtime_get_sync().  That way the pm_bus code will not call 
> omap_device_idle() on a device that the driver has not yet indicated is 
> safe to enter idle.

Hehe.  This was actually the original implementation.    

I decided I didn't like having to put those comments to admit defeat
against the DPM core. ;) So, I decided to change it to directly use
omap_device*.  But now, in light of the potential conflicts you raised,
I will go back to this implementation.

Thanks for the thorough feedback,

Kevin

Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Paul Walmsley]

On Thu, 24 Jun 2010, Paul Walmsley wrote:

> Sorry, misread this also.  Indeed, something like this is necessary in 
> your platform bus override code - so please ignore this part of the 
> comments.

By the way, I owe you a more general apology, Kevin, that some of these 
comments have been erroneous and that you have had to spend time 
addressing the half-baked ones.  They haven't measured up to my personal 
technical standards for comments...


- Paul

RE: [PATCH] OMAP1: PM: add simple runtime PM layer to manage clocks

[DebBarma, Tarun Kanti]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Friday, June 18, 2010 4:38 AM
> To: linux-omap@vger.kernel.org
> Subject: [PATCH] OMAP1: PM: add simple runtime PM layer to manage clocks
> 
> On OMAP1, we do not have omap_device + omap_hwmod to manage the
> device-specific idle, enable and shutdown.  Instead, just
> enable/disable device clocks automatically at the runtime PM level.
> 
> This allows drivers to not have any OMAP1 specific clock management
> and allows them to simply use the runtime PM API to manage clocks.
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
> This patch applies on top of the initial runtime PM support series:
> 
>   [PATCH 0/4] runtime PM support at the bus level
> 
> posted to linux-omap on May 27th.  It is also now included in
> my pm-wip/runtime branch in my git tree.
> 
>  arch/arm/mach-omap1/Makefile |    2 +-
>  arch/arm/mach-omap1/pm_bus.c |   77
> ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/mach-omap1/pm_bus.c
> 
> diff --git a/arch/arm/mach-omap1/Makefile b/arch/arm/mach-omap1/Makefile
> index ea231c7..fd4df71 100644
> --- a/arch/arm/mach-omap1/Makefile
> +++ b/arch/arm/mach-omap1/Makefile
> @@ -12,7 +12,7 @@ obj-$(CONFIG_OMAP_MPU_TIMER)	+= time.o
>  obj-$(CONFIG_OMAP_32K_TIMER)	+= timer32k.o
> 
>  # Power Management
> -obj-$(CONFIG_PM) += pm.o sleep.o
> +obj-$(CONFIG_PM) += pm.o sleep.o pm_bus.o
> 
>  # DSP
>  obj-$(CONFIG_OMAP_MBOX_FWK)	+= mailbox_mach.o
> diff --git a/arch/arm/mach-omap1/pm_bus.c b/arch/arm/mach-omap1/pm_bus.c
> new file mode 100644
> index 0000000..46a34fc
> --- /dev/null
> +++ b/arch/arm/mach-omap1/pm_bus.c
> @@ -0,0 +1,77 @@
> +/*
> + * Runtime PM support code for OMAP1
> + *
> + * Author: Kevin Hilman, Deep Root Systems, LLC
> + *
> + * Copyright (C) 2010 Texas Instruments, Inc.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +
> +#include <plat/omap_device.h>
> +#include <plat/omap-pm.h>
> +
> +#ifdef CONFIG_PM_RUNTIME
> +int platform_pm_runtime_suspend(struct device *dev)
> +{
> +	struct clk *iclk, *fclk;
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	if (dev->driver->pm && dev->driver->pm->runtime_suspend)
> +		ret = dev->driver->pm->runtime_suspend(dev);
I do not see "ret" declaration. I hope this is not a global variable. 

> +
> +	fclk = clk_get(dev, "fck");
> +	if (!IS_ERR(fclk)) {
> +		clk_disable(fclk);
> +		clk_put(fclk);
> +	}
> +
> +	iclk = clk_get(dev, "ick");
> +	if (!IS_ERR(iclk)) {
> +		clk_disable(iclk);
> +		clk_put(iclk);
> +	}
> +
> +	return 0;
Why not return "ret" instead of 0? 

> +};
> +
> +int platform_pm_runtime_resume(struct device *dev)
> +{
> +	int r, ret = 0;
> +	struct clk *iclk, *fclk;
> +
> +	iclk = clk_get(dev, "fck");
> +	if (!IS_ERR(iclk)) {
> +		clk_enable(iclk);
> +		clk_put(iclk);
> +	}
> +
> +	fclk = clk_get(dev, "fck");
> +	if (!IS_ERR(fclk)) {
> +		clk_enable(fclk);
> +		clk_put(fclk);
> +	}
> +
> +	if (dev->driver->pm && dev->driver->pm->runtime_resume)
> +		ret = dev->driver->pm->runtime_resume(dev);
> +
> +	return ret;
> +};
> +
> +int platform_pm_runtime_idle(struct device *dev)
> +{
> +	ret = pm_runtime_suspend(dev);
Unless "ret" is global, we have to declare this locally?

> +	dev_dbg(dev, "%s [%d]\n", __func__, ret);
> +
> +	return 0;
How about returning "ret" instead of 0? 

> +};
> +#endif /* CONFIG_PM_RUNTIME */
> --
> 1.7.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html