[PATCH v2 0/3] OMAP: add runtime PM support at bus-level

[PATCH v2 0/3] OMAP: add runtime PM support at 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.

This series applies on top v2.6.35-rc2 + Tony's omap-fixes branch and
is also available in the pm-wip/runtime branch of my linux-omap-pm git
tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git

Kevin Hilman (3):
  OMAP: PM: initial runtime PM core support
  OMAP: bus-level PM: enable use of runtime PM API for suspend/resume
  OMAP1: PM: add simple runtime PM layer to manage clocks

 arch/arm/mach-omap1/Makefile |    2 +-
 arch/arm/mach-omap1/pm_bus.c |   77 +++++++++++++++++++++++++++
 arch/arm/mach-omap2/Makefile |    7 ++-
 arch/arm/mach-omap2/pm_bus.c |  118 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 202 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-omap1/pm_bus.c
 create mode 100644 arch/arm/mach-omap2/pm_bus.c

[PATCH v2 1/3] 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 |   70 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 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 ea52b03..8ed47ea 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..9719a9f
--- /dev/null
+++ b/arch/arm/mach-omap2/pm_bus.c
@@ -0,0 +1,70 @@
+/*
+ * 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)
+{
+	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 v2 2/3] OMAP: bus-level PM: enable use of runtime PM API for suspend/resume

[Kevin Hilman]

Hook into the platform bus methods for suspend and resume and use the
runtime PM API to allow the OMAP runtime PM core (based on
omap_device) 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 |   48 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/pm_bus.c b/arch/arm/mach-omap2/pm_bus.c
index 9719a9f..5e453dc 100644
--- a/arch/arm/mach-omap2/pm_bus.c
+++ b/arch/arm/mach-omap2/pm_bus.c
@@ -68,3 +68,51 @@ 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;
+	int ret = 0;
+
+	if (!drv)
+		return 0;
+
+	if (drv->pm) {
+		if (drv->pm->suspend_noirq)
+			ret = drv->pm->suspend_noirq(dev);
+	}
+
+	/*
+	 * The DPM core has done a 'get' to prevent runtime PM
+	 * transitions during system PM.  This put is to balance
+	 * out that get so that this device can now be runtime
+	 * suspended.
+	 */
+	pm_runtime_put_sync(dev);
+
+	return ret;
+}
+
+int platform_pm_resume_noirq(struct device *dev)
+{
+	struct device_driver *drv = dev->driver;
+	int ret = 0;
+
+	/* 
+	 * This 'get' is to balance the 'put' in the above suspend_noirq
+	 * method so that the runtime PM usage counting is in the same
+	 * state it was when suspend was called.
+	 */
+	pm_runtime_get_sync(dev);
+
+	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 v2 3/3] 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>
---
 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..29d651a
--- /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, "ick");
+	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 v2 1/3] OMAP: PM: initial runtime PM core support

[Grant Likely]

On Thu, Jun 24, 2010 at 5:43 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> 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().)

Hi Kevin,

You shouldn't hijack the platform bus in this way, for a number of
reasons.  Not all platform devices in an OMAP system are internal OMAP
devices (as you know since you do an explicit check for omap devices).
 Right there that says that the abstraction is at the wrong level.
What happens when an mostly transparent bridge is added with its own
peripherals and its own special operations?  Does this routine then
need to be extended for each new special case?  It's not maintainable
in the long run.

This approach is also not multiplatform friendly (cc'ing Eric and
Nicolas who are working on ARM multiplatform).  It won't work for
building a kernel that supports, say, both versatile and OMAP.

The kernel already has the facility to do what you need.  We talked
about it briefly at ELC, and now that I look at it closer, I thing
gregkh is absolutely right.  Just create a new bus type for OMAP
devices.  It is simple to add one.  You can probably even call out to
the platform bus ops for most of the operations.  The fact that OMAP
devices have special behaviour that needs to be handled at the bus
type level means that they are not platform devices anymore.  This
also eliminates all the omap_device_is_valid and OMAP_DEVICE_MAGIC
gymnastics.

I see from the comments in omap_device.c that doing an
omap_bus/omap_device is being considered anyway.  Please don't merge
this patch and do the omap_bus_type instead.

Also, I notice that most of these hooks open-code the generic versions
of the runtime hooks.  Instead of open coding it, can the omap hooks
call the generic hooks before/after doing the omap-specific work?

Cheers,
g.

>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/Makefile |    7 +++-
>  arch/arm/mach-omap2/pm_bus.c |   70 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 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 ea52b03..8ed47ea 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..9719a9f
> --- /dev/null
> +++ b/arch/arm/mach-omap2/pm_bus.c
> @@ -0,0 +1,70 @@
> +/*
> + * 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)
> +{
> +       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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Kevin Hilman]

Grant Likely <grant.likely@secretlab.ca> writes:

> On Thu, Jun 24, 2010 at 5:43 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> 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().)
>
> Hi Kevin,

Hi Grant.  Thanks for the review and suggestions.

> You shouldn't hijack the platform bus in this way, for a number of
> reasons.  Not all platform devices in an OMAP system are internal OMAP
> devices (as you know since you do an explicit check for omap devices).
>  Right there that says that the abstraction is at the wrong level.
> What happens when an mostly transparent bridge is added with its own
> peripherals and its own special operations?  Does this routine then
> need to be extended for each new special case?  It's not maintainable
> in the long run.
>
> This approach is also not multiplatform friendly (cc'ing Eric and
> Nicolas who are working on ARM multiplatform).  It won't work for
> building a kernel that supports, say, both versatile and OMAP.

Totally agree here, but this a separate issue not specifically created
by this series (it was created when runtime PM support for SH was
added), but indeed I am continuining bad behavior. :/

The issue is that the current method to override bus methods is by
overriding weak symbols.  This clearly doesn't scale to support multiple
platforms in the same image.

What would be needed (if we continue to override the platform_bus
methods) is to have some sort of register function for overriding these
methods.  I'll look into that based on the result of discussions
below...

> The kernel already has the facility to do what you need.  We talked
> about it briefly at ELC, and now that I look at it closer, I thing
> gregkh is absolutely right.  Just create a new bus type for OMAP
> devices.  It is simple to add one.  You can probably even call out to
> the platform bus ops for most of the operations.  The fact that OMAP
> devices have special behaviour that needs to be handled at the bus
> type level means that they are not platform devices anymore.  This
> also eliminates all the omap_device_is_valid and OMAP_DEVICE_MAGIC
> gymnastics.
>
> I see from the comments in omap_device.c that doing an
> omap_bus/omap_device is being considered anyway.  Please don't merge
> this patch and do the omap_bus_type instead.

Agreed, it is logicially simpler in many ways and as you've noticed,
we've been discussing it in the OMAP community.

However, I keep coming back to extending the platform bus, primarily
since the resulting new bus code would look almost identical to the
platform bus.  All I really needed is the ability to extend a small
subset of the PM functions, so this led to me the "extend instead of
duplicate" approach.

In addition, I really don't want to duplicate all the platform_driver
and platform_device code, again because it would be identical but
especially since this would seriously impact many drivers.  For drivers
that are used on OMAP and also on other platforms, do we want drivers to
know (or care) if they are on the platform bus or on the OMAP bus?  I
think this is how it is done for OF devices, but I'm not crazy about
that approach (after our discussions at ELC, I remember thinking you'd
been through this with the OF devices as well and are moving towards
using platform_bus/platform_device for those too.  Did I understand
correctly?)

This affects many aspects of all drivers, from register and probe (for
early devices/drivers too!) to all the plaform_get_resource() usage, all
of which assumes a platform_driver and platform_device.  I didn't look
closely, but I didn't see if (or how) OF was handling early devices.

All that being said, if I could create a custom bus, but continue to use
platform_devices, that would greatly simply the changes to drivers.  Do
you think that's acceptable?  If so, I can take a stab at that and see
what it looks like.

> Also, I notice that most of these hooks open-code the generic versions
> of the runtime hooks.  Instead of open coding it, can the omap hooks
> call the generic hooks before/after doing the omap-specific work?

Ah, good point.

This patch pre-dates the creation of pm_generic_runtime_*, but certainly
should be upgraded to use those.  Thanks.

Kevin


> Cheers,
> g.
>
>>
>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> ---
>>  arch/arm/mach-omap2/Makefile |    7 +++-
>>  arch/arm/mach-omap2/pm_bus.c |   70 ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 76 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 ea52b03..8ed47ea 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..9719a9f
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/pm_bus.c
>> @@ -0,0 +1,70 @@
>> +/*
>> + * 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)
>> +{
>> +       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
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Russell King - ARM Linux]

On Fri, Jun 25, 2010 at 09:26:43AM -0600, Grant Likely wrote:
> This approach is also not multiplatform friendly (cc'ing Eric and
> Nicolas who are working on ARM multiplatform).  It won't work for
> building a kernel that supports, say, both versatile and OMAP.

And I should also point out that multiplatform is not the answer to
Linus' concerns.  There's no way such work is going to be anywhere
near complete come the next merge window.

Note that the loss of the defconfigs will make the integration of
multiplatform patches extremely time consuming and slow.

Re: [PATCH v2 1/3] OMAP: PM: initial runtime PM core support

[Grant Likely]

On Fri, Jun 25, 2010 at 12:04 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
>> On Thu, Jun 24, 2010 at 5:43 PM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>>> 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().)
>>
>> Hi Kevin,
>
> Hi Grant.  Thanks for the review and suggestions.
>
>> You shouldn't hijack the platform bus in this way, for a number of
>> reasons.  Not all platform devices in an OMAP system are internal OMAP
>> devices (as you know since you do an explicit check for omap devices).
>>  Right there that says that the abstraction is at the wrong level.
>> What happens when an mostly transparent bridge is added with its own
>> peripherals and its own special operations?  Does this routine then
>> need to be extended for each new special case?  It's not maintainable
>> in the long run.
>>
>> This approach is also not multiplatform friendly (cc'ing Eric and
>> Nicolas who are working on ARM multiplatform).  It won't work for
>> building a kernel that supports, say, both versatile and OMAP.
>
> Totally agree here, but this a separate issue not specifically created
> by this series (it was created when runtime PM support for SH was
> added), but indeed I am continuining bad behavior. :/

I think I could successfully argue that ARM is more important that SH.
 If SH messes up it's sandbox the collateral damage isn't nearly so
severe.  Don't follow the SH lead in this case.

> The issue is that the current method to override bus methods is by
> overriding weak symbols.  This clearly doesn't scale to support multiple
> platforms in the same image.

Agreed.  It scales better to change the hooks at runtime, which is
actually quite easy, but it still leaves the abstraction at the wrong
level.

> What would be needed (if we continue to override the platform_bus
> methods) is to have some sort of register function for overriding these
> methods.  I'll look into that based on the result of discussions
> below...
>
>> The kernel already has the facility to do what you need.  We talked
>> about it briefly at ELC, and now that I look at it closer, I thing
>> gregkh is absolutely right.  Just create a new bus type for OMAP
>> devices.  It is simple to add one.  You can probably even call out to
>> the platform bus ops for most of the operations.  The fact that OMAP
>> devices have special behaviour that needs to be handled at the bus
>> type level means that they are not platform devices anymore.  This
>> also eliminates all the omap_device_is_valid and OMAP_DEVICE_MAGIC
>> gymnastics.
>>
>> I see from the comments in omap_device.c that doing an
>> omap_bus/omap_device is being considered anyway.  Please don't merge
>> this patch and do the omap_bus_type instead.
>
> Agreed, it is logicially simpler in many ways and as you've noticed,
> we've been discussing it in the OMAP community.
>
> However, I keep coming back to extending the platform bus, primarily
> since the resulting new bus code would look almost identical to the
> platform bus.  All I really needed is the ability to extend a small
> subset of the PM functions, so this led to me the "extend instead of
> duplicate" approach.
>
> In addition, I really don't want to duplicate all the platform_driver
> and platform_device code, again because it would be identical but
> especially since this would seriously impact many drivers.  For drivers
> that are used on OMAP and also on other platforms, do we want drivers to
> know (or care) if they are on the platform bus or on the OMAP bus?

Some questions to make sure I understand:
Do you have a lot of these cases?
Do non-OMAP devices also have to process the omap clock enable/disable
code too, or is this only for stuff that is internal to the chip?
Or is this the case where existing non-omap device drivers can also
drive the omap SoC hardware?

> I think this is how it is done for OF devices, but I'm not crazy about
> that approach (after our discussions at ELC, I remember thinking you'd
> been through this with the OF devices as well and are moving towards
> using platform_bus/platform_device for those too.  Did I understand
> correctly?)

Yes, I've got patches which merge of_platform_bus_type with the
platform bus.  This was an easy decision to make because the
of-specific bits (specifically, matching an of_device_id table with a
device tree node) are applicable to all bus types; i2c, spi, mdio,
platform, etc).  The needed OF data structures have been moved into
struct device and struct device_driver so that of_platform_bus_type no
longer has anything different.

The drivers still need to care about OF when extra platform data needs
to be extracted from the DT node, but for IRQs and register ranges it
is automatic.

The omap case looks different.  You've got special runtime pm ops that
you want to hook in; but only for omap_devices.  It doesn't really
apply to more than one bus type in the system.

> This affects many aspects of all drivers, from register and probe (for
> early devices/drivers too!) to all the plaform_get_resource() usage, all
> of which assumes a platform_driver and platform_device.  I didn't look
> closely, but I didn't see if (or how) OF was handling early devices.

You don't have to reimplement the entire platform bus.  You could
simply create a new bus type, but reuse all the existing platform bus
code.  All that changes is the bus type that the device and the driver
gets registered on.  Then you could easily replace only the functions
that matter.  (do a git grep platform_bus_type to see how few
references there actually are.  It looks like there are only 5
references to it in drivers/base/platform.c that you'd need to work
around; in platform_device_add(), platform_driver_register(), 2 in
platform_driver_probe(), and the register in platform_bus_init().  You
may not even need to reimplement platform_driver_probe().

It might even be as simple as doing this:
-        pdev->dev.bus = &platform_bus_type;
+       if (!pdev->dev.bus)
+                pdev->dev.bus = &platform_bus_type;

So that a different bus type can be selected at device registration
time (but this might be a little on the dangerous side.  It might be
better to have a wrapper function that accepts an omap_device, and
then does the appropriate registration so that the compiler can
type-check it for you).

Another way to look at the problem is that these runtime
customizations are kind of a property of the parent device (the bus,
not the bus_type).  Would it make sense for parent devices to have
runtime ops to perform for each child that is suspended/resumed?  That
would make it simple to register another device that implements the
bus behaviour and attach it at runtime instead of compile time.

So, instead of having all the platform_bus_type devices as children of
the "platform" device (/sys/devices/platform/*), you could set the
omap devices to be children of an omap bus device
(/sys/devices/omap/*).  Different busses can also implement different
behaviour by using a different parent device.  For example:

/sys/devices/platform
/sys/devices/omap-bus-a
/sys/devices/omap-bus-a/omap-bus-b

Thoughts?

> All that being said, if I could create a custom bus, but continue to use
> platform_devices, that would greatly simply the changes to drivers.  Do
> you think that's acceptable?  If so, I can take a stab at that and see
> what it looks like.

Heh, I should read the whole email before replying.  See above.

>> Also, I notice that most of these hooks open-code the generic versions
>> of the runtime hooks.  Instead of open coding it, can the omap hooks
>> call the generic hooks before/after doing the omap-specific work?
>
> Ah, good point.
>
> This patch pre-dates the creation of pm_generic_runtime_*, but certainly
> should be upgraded to use those.  Thanks.
>
> Kevin
>
>
>> Cheers,
>> g.
>>
>>>
>>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>>> ---
>>>  arch/arm/mach-omap2/Makefile |    7 +++-
>>>  arch/arm/mach-omap2/pm_bus.c |   70 ++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 76 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 ea52b03..8ed47ea 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..9719a9f
>>> --- /dev/null
>>> +++ b/arch/arm/mach-omap2/pm_bus.c
>>> @@ -0,0 +1,70 @@
>>> +/*
>>> + * 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)
>>> +{
>>> +       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
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Grant Likely]

On Fri, Jun 25, 2010 at 2:06 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jun 25, 2010 at 09:26:43AM -0600, Grant Likely wrote:
>> This approach is also not multiplatform friendly (cc'ing Eric and
>> Nicolas who are working on ARM multiplatform).  It won't work for
>> building a kernel that supports, say, both versatile and OMAP.
>
> And I should also point out that multiplatform is not the answer to
> Linus' concerns.  There's no way such work is going to be anywhere
> near complete come the next merge window.

Yes, but solving the defconfig problem isn't the reason for multiplatform.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Russell King - ARM Linux]

On Fri, Jun 25, 2010 at 02:13:15PM -0600, Grant Likely wrote:
> On Fri, Jun 25, 2010 at 2:06 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Jun 25, 2010 at 09:26:43AM -0600, Grant Likely wrote:
> >> This approach is also not multiplatform friendly (cc'ing Eric and
> >> Nicolas who are working on ARM multiplatform).  It won't work for
> >> building a kernel that supports, say, both versatile and OMAP.
> >
> > And I should also point out that multiplatform is not the answer to
> > Linus' concerns.  There's no way such work is going to be anywhere
> > near complete come the next merge window.
> 
> Yes, but solving the defconfig problem isn't the reason for multiplatform.

Does anyone know where we are on the defconfig problem?  From what I
can see, it's mostly stalled for the time being, which is not good
news for us.
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Kevin Hilman]

Grant Likely <grant.likely@secretlab.ca> writes:

> On Fri, Jun 25, 2010 at 12:04 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> Grant Likely <grant.likely@secretlab.ca> writes:
>>
>>> On Thu, Jun 24, 2010 at 5:43 PM, Kevin Hilman
>>> <khilman@deeprootsystems.com> wrote:
>>>> 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().)
>>>
>>> Hi Kevin,
>>
>> Hi Grant.  Thanks for the review and suggestions.
>>
>>> You shouldn't hijack the platform bus in this way, for a number of
>>> reasons.  Not all platform devices in an OMAP system are internal OMAP
>>> devices (as you know since you do an explicit check for omap devices).
>>>  Right there that says that the abstraction is at the wrong level.
>>> What happens when an mostly transparent bridge is added with its own
>>> peripherals and its own special operations?  Does this routine then
>>> need to be extended for each new special case?  It's not maintainable
>>> in the long run.
>>>
>>> This approach is also not multiplatform friendly (cc'ing Eric and
>>> Nicolas who are working on ARM multiplatform).  It won't work for
>>> building a kernel that supports, say, both versatile and OMAP.
>>
>> Totally agree here, but this a separate issue not specifically created
>> by this series (it was created when runtime PM support for SH was
>> added), but indeed I am continuining bad behavior. :/
>
> I think I could successfully argue that ARM is more important that SH.
>  If SH messes up it's sandbox the collateral damage isn't nearly so
> severe.  Don't follow the SH lead in this case.
>
>> The issue is that the current method to override bus methods is by
>> overriding weak symbols.  This clearly doesn't scale to support multiple
>> platforms in the same image.
>
> Agreed.  It scales better to change the hooks at runtime, which is
> actually quite easy, but it still leaves the abstraction at the wrong
> level.
>
>> What would be needed (if we continue to override the platform_bus
>> methods) is to have some sort of register function for overriding these
>> methods.  I'll look into that based on the result of discussions
>> below...
>>
>>> The kernel already has the facility to do what you need.  We talked
>>> about it briefly at ELC, and now that I look at it closer, I thing
>>> gregkh is absolutely right.  Just create a new bus type for OMAP
>>> devices.  It is simple to add one.  You can probably even call out to
>>> the platform bus ops for most of the operations.  The fact that OMAP
>>> devices have special behaviour that needs to be handled at the bus
>>> type level means that they are not platform devices anymore.  This
>>> also eliminates all the omap_device_is_valid and OMAP_DEVICE_MAGIC
>>> gymnastics.
>>>
>>> I see from the comments in omap_device.c that doing an
>>> omap_bus/omap_device is being considered anyway.  Please don't merge
>>> this patch and do the omap_bus_type instead.
>>
>> Agreed, it is logicially simpler in many ways and as you've noticed,
>> we've been discussing it in the OMAP community.
>>
>> However, I keep coming back to extending the platform bus, primarily
>> since the resulting new bus code would look almost identical to the
>> platform bus.  All I really needed is the ability to extend a small
>> subset of the PM functions, so this led to me the "extend instead of
>> duplicate" approach.
>>
>> In addition, I really don't want to duplicate all the platform_driver
>> and platform_device code, again because it would be identical but
>> especially since this would seriously impact many drivers.  For drivers
>> that are used on OMAP and also on other platforms, do we want drivers to
>> know (or care) if they are on the platform bus or on the OMAP bus?
>
> Some questions to make sure I understand:
> Do you have a lot of these cases?

Yes, there are several.

There are several instances of hardware blocks that are re-used across
OMAP and DaVinci but where the device clocking and PM infrastructure is
totally different.  Runtime PM helps a lot here in that all the details
can be done in low-level, SoC specific code.  

There are even cases of OMAP-derivative parts that will change clocking
or change (or remove) PM internal details and require special handling.
We want to be able to use the same driver for all these, and not care
if it's an OMAP, Davinci, OMAP-derivative etc.

There's also the special case of the MUSB driver which is on OMAP,
Davinci and Blackfin.

> Do non-OMAP devices also have to process the omap clock enable/disable
> code too, or is this only for stuff that is internal to the chip?

This is only for on-chip devices.  But as I said above, there are
on-chip devices on OMAP that are used on other platforms, and the driver
should be re-usable, without OMAP-specific knowledge.

> Or is this the case where existing non-omap device drivers can also
> drive the omap SoC hardware?
>
>> I think this is how it is done for OF devices, but I'm not crazy about
>> that approach (after our discussions at ELC, I remember thinking you'd
>> been through this with the OF devices as well and are moving towards
>> using platform_bus/platform_device for those too.  Did I understand
>> correctly?)
>
> Yes, I've got patches which merge of_platform_bus_type with the
> platform bus.  This was an easy decision to make because the
> of-specific bits (specifically, matching an of_device_id table with a
> device tree node) are applicable to all bus types; i2c, spi, mdio,
> platform, etc).  The needed OF data structures have been moved into
> struct device and struct device_driver so that of_platform_bus_type no
> longer has anything different.
>
> The drivers still need to care about OF when extra platform data needs
> to be extracted from the DT node, but for IRQs and register ranges it
> is automatic.

Does that mean the drivers are still doing platform_get_resource() for
either platform devices or OF devices, or are does the driver have to
know which bus it was on and call accordingly.  It's the latter that I
want to stay away from.

> The omap case looks different.  You've got special runtime pm ops that
> you want to hook in; but only for omap_devices.  It doesn't really
> apply to more than one bus type in the system.
>
>> This affects many aspects of all drivers, from register and probe (for
>> early devices/drivers too!) to all the plaform_get_resource() usage, all
>> of which assumes a platform_driver and platform_device.  I didn't look
>> closely, but I didn't see if (or how) OF was handling early devices.
>
> You don't have to reimplement the entire platform bus.  You could
> simply create a new bus type, but reuse all the existing platform bus
> code.  All that changes is the bus type that the device and the driver
> gets registered on.  Then you could easily replace only the functions
> that matter.  (do a git grep platform_bus_type to see how few
> references there actually are.  It looks like there are only 5
> references to it in drivers/base/platform.c that you'd need to work
> around; in platform_device_add(), platform_driver_register(), 2 in
> platform_driver_probe(), and the register in platform_bus_init().  You
> may not even need to reimplement platform_driver_probe().
>
> It might even be as simple as doing this:
> -        pdev->dev.bus = &platform_bus_type;
> +       if (!pdev->dev.bus)
> +                pdev->dev.bus = &platform_bus_type;

Yeah, I found those and already started exploring that change.

> So that a different bus type can be selected at device registration
> time (but this might be a little on the dangerous side.  It might be
> better to have a wrapper function that accepts an omap_device, and
> then does the appropriate registration so that the compiler can
> type-check it for you).
>
> Another way to look at the problem is that these runtime
> customizations are kind of a property of the parent device (the bus,
> not the bus_type).  Would it make sense for parent devices to have
> runtime ops to perform for each child that is suspended/resumed?  That
> would make it simple to register another device that implements the
> bus behaviour and attach it at runtime instead of compile time.
>
> So, instead of having all the platform_bus_type devices as children of
> the "platform" device (/sys/devices/platform/*), you could set the
> omap devices to be children of an omap bus device
> (/sys/devices/omap/*).  Different busses can also implement different
> behaviour by using a different parent device.  For example:
>
> /sys/devices/platform
> /sys/devices/omap-bus-a
> /sys/devices/omap-bus-a/omap-bus-b
>
> Thoughts?

Hmm, I like this idea.  This is certainly worth exploring as a first
pass.

Thanks for the idea,

Kevin

>
>> All that being said, if I could create a custom bus, but continue to use
>> platform_devices, that would greatly simply the changes to drivers.  Do
>> you think that's acceptable?  If so, I can take a stab at that and see
>> what it looks like.
>
> Heh, I should read the whole email before replying.  See above.
>
>>> Also, I notice that most of these hooks open-code the generic versions
>>> of the runtime hooks.  Instead of open coding it, can the omap hooks
>>> call the generic hooks before/after doing the omap-specific work?
>>
>> Ah, good point.
>>
>> This patch pre-dates the creation of pm_generic_runtime_*, but certainly
>> should be upgraded to use those.  Thanks.
>>
>> Kevin
>>
>>
>>> Cheers,
>>> g.
>>>
>>>>
>>>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>>>> ---
>>>>  arch/arm/mach-omap2/Makefile |    7 +++-
>>>>  arch/arm/mach-omap2/pm_bus.c |   70 ++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 76 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 ea52b03..8ed47ea 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..9719a9f
>>>> --- /dev/null
>>>> +++ b/arch/arm/mach-omap2/pm_bus.c
>>>> @@ -0,0 +1,70 @@
>>>> +/*
>>>> + * 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)
>>>> +{
>>>> +       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
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Grant Likely]

On Fri, Jun 25, 2010 at 3:58 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>> Yes, I've got patches which merge of_platform_bus_type with the
>> platform bus.  This was an easy decision to make because the
>> of-specific bits (specifically, matching an of_device_id table with a
>> device tree node) are applicable to all bus types; i2c, spi, mdio,
>> platform, etc).  The needed OF data structures have been moved into
>> struct device and struct device_driver so that of_platform_bus_type no
>> longer has anything different.
>>
>> The drivers still need to care about OF when extra platform data needs
>> to be extracted from the DT node, but for IRQs and register ranges it
>> is automatic.
>
> Does that mean the drivers are still doing platform_get_resource() for
> either platform devices or OF devices, or are does the driver have to
> know which bus it was on and call accordingly.  It's the latter that I
> want to stay away from.

platform_get_resource() works for OF and non-OF platform devices with
no extra code needed in the OF case.

It's the other data that requires special code because it is provided
in the device tree instead of in a platform_data structure.  A driver
that normally uses platform_data will need extra code early in the
probe hook to go and parse the device tree and populate the private
data structures accordingly.  This behaviour has to be handled in the
driver because every driver uses a different platform_data structure.
It cannot be generic code.  The rest of the driver can remain
untouched.

In the old (bad) way, the bus type was entirely different, and drivers
used by both platform_bus_type and of_platform_bus_type needed to have
entirely separate device_driver structures and probe() hooks for each
bus type.

[...]
>> So, instead of having all the platform_bus_type devices as children of
>> the "platform" device (/sys/devices/platform/*), you could set the
>> omap devices to be children of an omap bus device
>> (/sys/devices/omap/*).  Different busses can also implement different
>> behaviour by using a different parent device.  For example:
>>
>> /sys/devices/platform
>> /sys/devices/omap-bus-a
>> /sys/devices/omap-bus-a/omap-bus-b
>>
>> Thoughts?
>
> Hmm, I like this idea.  This is certainly worth exploring as a first
> pass.
>
> Thanks for the idea,

:-)
g.
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Kevin Hilman]

Grant Likely <grant.likely@secretlab.ca> writes:

> Another way to look at the problem is that these runtime
> customizations are kind of a property of the parent device (the bus,
> not the bus_type).  Would it make sense for parent devices to have
> runtime ops to perform for each child that is suspended/resumed?  That
> would make it simple to register another device that implements the
> bus behaviour and attach it at runtime instead of compile time.

Maybe I didn't fully understand your idea, but the problem here is
devices do not have dev_pm_ops.  Only busses, classes, and types have
dev_pm_ops.

Though I'm horribly unfamiliar with the intended usage of 'struct
device_type', an interesing discovery is that dev->type also has
dev_pm_ops, and it takes precedence over the bus type in the
suspend/resume.  IOW, when suspending, when deciding which dev_pm_ops to
use, it checks class, type, then bus in that order.

I need to explore this 'type' feature a little more, but using that or
the 'class' might be another way to have custom PM ops for certain
platform_devices.

Kevin

Re: [PATCH v2 1/3] OMAP: PM: initial runtime PM core support

[Grant Likely]

On Fri, Jun 25, 2010 at 4:46 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
>> Another way to look at the problem is that these runtime
>> customizations are kind of a property of the parent device (the bus,
>> not the bus_type).  Would it make sense for parent devices to have
>> runtime ops to perform for each child that is suspended/resumed?  That
>> would make it simple to register another device that implements the
>> bus behaviour and attach it at runtime instead of compile time.
>
> Maybe I didn't fully understand your idea, but the problem here is
> devices do not have dev_pm_ops.  Only busses, classes, and types have
> dev_pm_ops.

Sorry, I mistyped.  What I meant was for the parent device's
device_driver to be able to have a set of child dev_pm_ops; but I'm
wading into territory (power management) I'm not particularly familiar
with, and that might be making things far too complex.

> Though I'm horribly unfamiliar with the intended usage of 'struct
> device_type', an interesing discovery is that dev->type also has
> dev_pm_ops, and it takes precedence over the bus type in the
> suspend/resume.  IOW, when suspending, when deciding which dev_pm_ops to
> use, it checks class, type, then bus in that order.

So I guess my suggestion above boils down to somehow inserting
"parent" between type and bus in that list.

> I need to explore this 'type' feature a little more, but using that or
> the 'class' might be another way to have custom PM ops for certain
> platform_devices.

Should maybe start cc'ing Greg and linux-kernel/linux-pm in this discussion.

g.
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 383 bytes --]

On Fri, 25 Jun 2010, Russell King - ARM Linux wrote:

> Does anyone know where we are on the defconfig problem?  From what I
> can see, it's mostly stalled for the time being, which is not good
> news for us.

What looked to be promizing is the work by Uwe Kleine-König according to 
the preview he posted on June 10:
  
Message-id: <20100610063234.GA22533@pengutronix.de>


Nicolas

Re: [PATCH v2 1/3] OMAP: PM: initial runtime PM core support

[Uwe Kleine-König]

Hello,

On Fri, Jun 25, 2010 at 07:46:05PM -0400, Nicolas Pitre wrote:
> On Fri, 25 Jun 2010, Russell King - ARM Linux wrote:
> 
> > Does anyone know where we are on the defconfig problem?  From what I
> > can see, it's mostly stalled for the time being, which is not good
> > news for us.
> 
> What looked to be promizing is the work by Uwe Kleine-König according to 
> the preview he posted on June 10:
>   
> Message-id: <20100610063234.GA22533@pengutronix.de>
I think Linus is still away?!  I plan to ping him again asking for an
opinion regarding my idea when he will be back.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Russell King - ARM Linux]

On Sat, Jun 26, 2010 at 12:51:33PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Jun 25, 2010 at 07:46:05PM -0400, Nicolas Pitre wrote:
> > On Fri, 25 Jun 2010, Russell King - ARM Linux wrote:
> > 
> > > Does anyone know where we are on the defconfig problem?  From what I
> > > can see, it's mostly stalled for the time being, which is not good
> > > news for us.
> > 
> > What looked to be promizing is the work by Uwe Kleine-König according to 
> > the preview he posted on June 10:
> >   
> > Message-id: <20100610063234.GA22533@pengutronix.de>
> I think Linus is still away?!  I plan to ping him again asking for an
> opinion regarding my idea when he will be back.

It looks that way...
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Kevin Hilman]

Grant Likely <grant.likely@secretlab.ca> writes:

> On Fri, Jun 25, 2010 at 4:46 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> Grant Likely <grant.likely@secretlab.ca> writes:
>>
>>> Another way to look at the problem is that these runtime
>>> customizations are kind of a property of the parent device (the bus,
>>> not the bus_type).  Would it make sense for parent devices to have
>>> runtime ops to perform for each child that is suspended/resumed?  That
>>> would make it simple to register another device that implements the
>>> bus behaviour and attach it at runtime instead of compile time.
>>
>> Maybe I didn't fully understand your idea, but the problem here is
>> devices do not have dev_pm_ops.  Only busses, classes, and types have
>> dev_pm_ops.
>
> Sorry, I mistyped.  What I meant was for the parent device's
> device_driver to be able to have a set of child dev_pm_ops; but I'm
> wading into territory (power management) I'm not particularly familiar
> with, and that might be making things far too complex.

I see your point now, but I think this indeed might complicate things
too much.  

Also, I'm not crazy about how this would delay the per-device PM hooks
to be essentially batched until all devices under the parent are
"ready."

But anyways, it just needs some more research on my part.
Unfortunately, I'll be away from this work on vacation for most of July,
so this won't get any attention from me until August.

>> Though I'm horribly unfamiliar with the intended usage of 'struct
>> device_type', an interesing discovery is that dev->type also has
>> dev_pm_ops, and it takes precedence over the bus type in the
>> suspend/resume.  IOW, when suspending, when deciding which dev_pm_ops to
>> use, it checks class, type, then bus in that order.
>
> So I guess my suggestion above boils down to somehow inserting
> "parent" between type and bus in that list.
>
>> I need to explore this 'type' feature a little more, but using that or
>> the 'class' might be another way to have custom PM ops for certain
>> platform_devices.
>
> Should maybe start cc'ing Greg and linux-kernel/linux-pm in this discussion.

They were involved in the early discussions about overriding the
platform_bus dev_pm_ops methods, which led us to the current
implementation.

But as I explore the custom bus approach a bit more (in August) I'll
broaden the audience.

Thanks again for all your helpful suggestions,

Kevin
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Grant Likely]

On Mon, Jun 28, 2010 at 2:49 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
>> On Fri, Jun 25, 2010 at 4:46 PM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>>> Grant Likely <grant.likely@secretlab.ca> writes:
>>>
>>>> Another way to look at the problem is that these runtime
>>>> customizations are kind of a property of the parent device (the bus,
>>>> not the bus_type).  Would it make sense for parent devices to have
>>>> runtime ops to perform for each child that is suspended/resumed?  That
>>>> would make it simple to register another device that implements the
>>>> bus behaviour and attach it at runtime instead of compile time.
>>>
>>> Maybe I didn't fully understand your idea, but the problem here is
>>> devices do not have dev_pm_ops.  Only busses, classes, and types have
>>> dev_pm_ops.
>>
>> Sorry, I mistyped.  What I meant was for the parent device's
>> device_driver to be able to have a set of child dev_pm_ops; but I'm
>> wading into territory (power management) I'm not particularly familiar
>> with, and that might be making things far too complex.
>
> I see your point now, but I think this indeed might complicate things
> too much.
>
> Also, I'm not crazy about how this would delay the per-device PM hooks
> to be essentially batched until all devices under the parent are
> "ready."

No, delaying until all children are ready wasn't my intent.  I was
thinking about a set of childs_dev_pm_ops() that would be called once
for each child as each child suspends/resumes...  but probably a moot
point since we both agree that it adds a lot of complexity.  :-)

>
> But anyways, it just needs some more research on my part.
> Unfortunately, I'll be away from this work on vacation for most of July,
> so this won't get any attention from me until August.
>
>>> Though I'm horribly unfamiliar with the intended usage of 'struct
>>> device_type', an interesing discovery is that dev->type also has
>>> dev_pm_ops, and it takes precedence over the bus type in the
>>> suspend/resume.  IOW, when suspending, when deciding which dev_pm_ops to
>>> use, it checks class, type, then bus in that order.
>>
>> So I guess my suggestion above boils down to somehow inserting
>> "parent" between type and bus in that list.
>>
>>> I need to explore this 'type' feature a little more, but using that or
>>> the 'class' might be another way to have custom PM ops for certain
>>> platform_devices.
>>
>> Should maybe start cc'ing Greg and linux-kernel/linux-pm in this discussion.
>
> They were involved in the early discussions about overriding the
> platform_bus dev_pm_ops methods, which led us to the current
> implementation.
>
> But as I explore the custom bus approach a bit more (in August) I'll
> broaden the audience.
>
> Thanks again for all your helpful suggestions,

np.  Talk you you later.  Have a great vacation.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Kevin Hilman]

Grant Likely <grant.likely@secretlab.ca> writes:

[...]

>
>> This affects many aspects of all drivers, from register and probe (for
>> early devices/drivers too!) to all the plaform_get_resource() usage, all
>> of which assumes a platform_driver and platform_device.  I didn't look
>> closely, but I didn't see if (or how) OF was handling early devices.
>
> You don't have to reimplement the entire platform bus.  You could
> simply create a new bus type, but reuse all the existing platform bus
> code.  All that changes is the bus type that the device and the driver
> gets registered on.  Then you could easily replace only the functions
> that matter.  (do a git grep platform_bus_type to see how few
> references there actually are.  It looks like there are only 5
> references to it in drivers/base/platform.c that you'd need to work
> around; in platform_device_add(), platform_driver_register(), 2 in
> platform_driver_probe(), and the register in platform_bus_init().  You
> may not even need to reimplement platform_driver_probe().
>
> It might even be as simple as doing this:
> -        pdev->dev.bus = &platform_bus_type;
> +       if (!pdev->dev.bus)
> +                pdev->dev.bus = &platform_bus_type;
>
> So that a different bus type can be selected at device registration
> time 

just FYI...

as a quick proof of concept, I've done a quick hack just to prove to
myself that I could use platform_devices on a custom bus, and it indeed
works.  The small patch below[1] shows the changes required to the
platform code.

Next step was to hack up minimal custom bus code.  The quickest (and
dirtiest) way was to simply memcpy platform_bus_type into my new
omap_bus_type and then override the few dev_pm_ops functions I needed[2].

So, with these in place, and using the dev_pm_ops functions from
$SUBJECT patch, I was able register a platform_device and
platform_driver onto my custom bus and see my custom dev_pm_ops
functions being used as expected.

While admittedly a bit hacky, at least this paves the way in my head
that this is indeed do-able, and I can take my vacation in peace without
this particular problem haunting me (too much.)

Kevin

[1]
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4d99c8b..2cf55e2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -241,7 +241,8 @@ int platform_device_add(struct platform_device *pdev)
 	if (!pdev->dev.parent)
 		pdev->dev.parent = &platform_bus;
 
-	pdev->dev.bus = &platform_bus_type;
+	if (!pdev->dev.bus)
+		pdev->dev.bus = &platform_bus_type;
 
 	if (pdev->id != -1)
 		dev_set_name(&pdev->dev, "%s.%d", pdev->name,  pdev->id);
@@ -482,7 +483,8 @@ static void platform_drv_shutdown(struct device *_dev)
  */
 int platform_driver_register(struct platform_driver *drv)
 {
-	drv->driver.bus = &platform_bus_type;
+	if (!drv->driver.bus)
+		drv->driver.bus = &platform_bus_type;
 	if (drv->probe)
 		drv->driver.probe = platform_drv_probe;
 	if (drv->remove)
@@ -539,12 +541,12 @@ int __init_or_module platform_driver_probe(struct platform_driver *drv,
 	 * if the probe was successful, and make sure any forced probes of
 	 * new devices fail.
 	 */
-	spin_lock(&platform_bus_type.p->klist_drivers.k_lock);
+	spin_lock(&drv->driver.bus->p->klist_drivers.k_lock);
 	drv->probe = NULL;
 	if (code == 0 && list_empty(&drv->driver.p->klist_devices.k_list))
 		retval = -ENODEV;
 	drv->driver.probe = platform_drv_probe_fail;
-	spin_unlock(&platform_bus_type.p->klist_drivers.k_lock);
+	spin_unlock(&drv->driver.bus->p->klist_drivers.k_lock);
 
 	if (code != retval)
 		platform_driver_unregister(drv);


[2] 
struct bus_type omap_bus_type;
EXPORT_SYMBOL_GPL(omap_bus_type);

static int __init omap_bus_init(void)
{
	int error;
	struct bus_type *bus = &omap_bus_type;

	pr_debug("%s\n", __func__);

	/*
	 * We're just a copy of platform_bus_type with special dev_pm_ops.
	 */
	memcpy(bus, &platform_bus_type, sizeof(struct bus_type));
	bus->name = "omap";
	bus->pm->suspend_noirq = omap_pm_suspend_noirq,
	bus->pm->resume_noirq = omap_pm_resume_noirq,
	bus->pm->runtime_suspend = omap_pm_runtime_suspend,
	bus->pm->runtime_resume = omap_pm_runtime_resume,
	bus->pm->runtime_idle = omap_pm_runtime_idle,

	error = device_register(&omap_bus);
	if (error)
		return error;
	error =  bus_register(&omap_bus_type);
	if (error)
		device_unregister(&omap_bus);

	return error;
}
arch_initcall(omap_bus_init);
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Grant Likely]

On Mon, Jun 28, 2010 at 4:27 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
> [...]
>
>>
>>> This affects many aspects of all drivers, from register and probe (for
>>> early devices/drivers too!) to all the plaform_get_resource() usage, all
>>> of which assumes a platform_driver and platform_device.  I didn't look
>>> closely, but I didn't see if (or how) OF was handling early devices.
>>
>> You don't have to reimplement the entire platform bus.  You could
>> simply create a new bus type, but reuse all the existing platform bus
>> code.  All that changes is the bus type that the device and the driver
>> gets registered on.  Then you could easily replace only the functions
>> that matter.  (do a git grep platform_bus_type to see how few
>> references there actually are.  It looks like there are only 5
>> references to it in drivers/base/platform.c that you'd need to work
>> around; in platform_device_add(), platform_driver_register(), 2 in
>> platform_driver_probe(), and the register in platform_bus_init().  You
>> may not even need to reimplement platform_driver_probe().
>>
>> It might even be as simple as doing this:
>> -        pdev->dev.bus = &platform_bus_type;
>> +       if (!pdev->dev.bus)
>> +                pdev->dev.bus = &platform_bus_type;
>>
>> So that a different bus type can be selected at device registration
>> time
>
> just FYI...
>
> as a quick proof of concept, I've done a quick hack just to prove to
> myself that I could use platform_devices on a custom bus, and it indeed
> works.  The small patch below[1] shows the changes required to the
> platform code.
>
> Next step was to hack up minimal custom bus code.  The quickest (and
> dirtiest) way was to simply memcpy platform_bus_type into my new
> omap_bus_type and then override the few dev_pm_ops functions I needed[2].
>
> So, with these in place, and using the dev_pm_ops functions from
> $SUBJECT patch, I was able register a platform_device and
> platform_driver onto my custom bus and see my custom dev_pm_ops
> functions being used as expected.

Nice!

> While admittedly a bit hacky, at least this paves the way in my head
> that this is indeed do-able, and I can take my vacation in peace without
> this particular problem haunting me (too much.)

indeed.  It could be less hacky also by adding a
platform_bus_type_init() function to the common platform code.

Cheers,
g.
--
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 v2 1/3] OMAP: PM: initial runtime PM core support

[Grant Likely]

On Thu, Jun 24, 2010 at 5:43 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> 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.
[...]
> +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);

For the record, I should note here that this is *really* dangerous.
When handed a random platform_device pointer, it is not safe to use
to_omap_device() and dereference it with omap_device_is_valid().
There are no guarantees that the dereference is actually valid,
particularly so when the platform_device has been dynamically
allocated.

It is only okay to use to_omap_device() when the code is already
absolutely sure that the platform_device is in fact contained by a
struct omap_device.  There needs to be a different method to test if
it is contained by an omap_device before doing the dereference.
--
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