[RFC/PATCH] base: platform: add generic clock handling for platform-bus

[RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Felipe Balbi]

Still TODO a commit log. Not for merging!!!!!

NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
---

This patch is an idea I've had recently in order to combine several different
PM implementations into the platform-bus.

This patch is bare minimum for platforms which need to handle functional and
interface clocks but the whole thing is made optional.

Note that this patch makes sure that by the time a platform_driver's probe is
called, we already have clocks enabled and pm_runtime_set_active() has been
called, thus making sure that a device driver's pm_runtime_get_sync() will
solely increase the pm usage counter.

I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause any
issues since the clock API has ref counting too.

Would really like to get some review from several folks involved with ARM SoC
PM so that's the reason for the wide audience. If I have missed anybody, please
add them to Cc.

As mentioned above, this is *NOT* meant for merging, but serves as a starting
point for discussing some convergence of several PM domain implementations on
different arch/arm/mach-* directories.

For OMAP, this has the added benefit of removing clock handling from
omap_device/omap_hwmod and, thus, dropping the need for so many DT_CLK() tables
under drivers/clk/ti/

 drivers/base/platform.c         | 169 ++++++++++++++++++++++++++++++++++++++--
 include/linux/platform_device.h |   9 +++
 2 files changed, 173 insertions(+), 5 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b79..86aeb5b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -484,6 +484,21 @@ static int platform_drv_probe(struct device *_dev)
 	if (ACPI_HANDLE(_dev))
 		acpi_dev_pm_attach(_dev, true);
 
+	dev->fck = devm_clk_get(_dev, "fck");
+	dev->ick = devm_clk_get(_dev, "ick");
+
+	if (!IS_ERR(dev->fck))
+		clk_prepare_enable(dev->fck);
+	else
+		dev->fck = NULL;
+
+	if (!IS_ERR(dev->ick))
+		clk_prepare_enable(dev->ick);
+	else
+		dev->ick = NULL;
+
+	pm_runtime_set_active(_dev);
+
 	ret = drv->probe(dev);
 	if (ret && ACPI_HANDLE(_dev))
 		acpi_dev_pm_detach(_dev, true);
@@ -507,6 +522,14 @@ static int platform_drv_remove(struct device *_dev)
 	struct platform_device *dev = to_platform_device(_dev);
 	int ret;
 
+	if (!IS_ERR(dev->ick))
+		clk_disable_unprepare(dev->ick);
+
+	if (!IS_ERR(dev->fck))
+		clk_disable_unprepare(dev->fck);
+
+	pm_runtime_set_suspended(_dev);
+
 	ret = drv->remove(dev);
 	if (ACPI_HANDLE(_dev))
 		acpi_dev_pm_detach(_dev, true);
@@ -780,6 +803,59 @@ static int platform_legacy_resume(struct device *dev)
 
 #endif /* CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_PM_RUNTIME
+int platform_pm_runtime_suspend(struct device *dev)
+{
+	struct device_driver *drv = dev->driver;
+	struct platform_driver *pdrv = to_platform_driver(drv);
+	struct platform_device *pdev = to_platform_device(dev);
+	int ret;
+
+	ret = pm_generic_runtime_suspend(dev);
+	if (ret)
+		return ret;
+
+	if (!test_bit(PLATFORM_PM_RUNTIME_KEEP_ICK, &pdrv->flags))
+		clk_disable(pdev->ick);
+
+	if (!test_bit(PLATFORM_PM_RUNTIME_KEEP_FCK, &pdrv->flags))
+		clk_disable(pdev->fck);
+
+	return ret;
+}
+
+int platform_pm_runtime_resume(struct device *dev)
+{
+	struct device_driver *drv = dev->driver;
+	struct platform_driver *pdrv = to_platform_driver(drv);
+	struct platform_device *pdev = to_platform_device(dev);
+	int ret;
+
+	if (!test_bit(PLATFORM_PM_RUNTIME_KEEP_ICK, &pdrv->flags)) {
+		ret = clk_enable(pdev->ick);
+		if (ret)
+			return ret;
+	}
+
+	if (!test_bit(PLATFORM_PM_RUNTIME_KEEP_FCK, &pdrv->flags)) {
+		ret = clk_enable(pdev->fck);
+		if (ret) {
+			clk_disable(pdev->ick);
+			return ret;
+		}
+	}
+
+	ret = pm_generic_runtime_suspend(dev);
+	if (ret) {
+		clk_disable(pdev->ick);
+		clk_disable(pdev->fck);
+		return ret;
+	}
+
+	return ret;
+}
+#endif
+
 #ifdef CONFIG_SUSPEND
 
 int platform_pm_suspend(struct device *dev)
@@ -790,6 +866,9 @@ int platform_pm_suspend(struct device *dev)
 	if (!drv)
 		return 0;
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	if (drv->pm) {
 		if (drv->pm->suspend)
 			ret = drv->pm->suspend(dev);
@@ -802,12 +881,27 @@ int platform_pm_suspend(struct device *dev)
 
 int platform_pm_resume(struct device *dev)
 {
+	struct platform_device *pdev = to_platform_device(dev);
 	struct device_driver *drv = dev->driver;
 	int ret = 0;
 
 	if (!drv)
 		return 0;
 
+	ret = clk_enable(pdev->ick);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(pdev->fck);
+	if (ret) {
+		clk_disable(pdev->ick);
+		return ret;
+	}
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	if (drv->pm) {
 		if (drv->pm->resume)
 			ret = drv->pm->resume(dev);
@@ -815,7 +909,17 @@ int platform_pm_resume(struct device *dev)
 		ret = platform_legacy_resume(dev);
 	}
 
-	return ret;
+	if (ret) {
+		clk_disable(pdev->ick);
+		clk_disable(pdev->fck);
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+
+		return ret;
+	}
+
+	return 0;
 }
 
 #endif /* CONFIG_SUSPEND */
@@ -830,6 +934,9 @@ int platform_pm_freeze(struct device *dev)
 	if (!drv)
 		return 0;
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	if (drv->pm) {
 		if (drv->pm->freeze)
 			ret = drv->pm->freeze(dev);
@@ -848,6 +955,20 @@ int platform_pm_thaw(struct device *dev)
 	if (!drv)
 		return 0;
 
+	ret = clk_enable(pdev->ick);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(pdev->fck);
+	if (ret) {
+		clk_disable(pdev->ick);
+		return ret;
+	}
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	if (drv->pm) {
 		if (drv->pm->thaw)
 			ret = drv->pm->thaw(dev);
@@ -855,7 +976,17 @@ int platform_pm_thaw(struct device *dev)
 		ret = platform_legacy_resume(dev);
 	}
 
-	return ret;
+	if (ret) {
+		clk_disable(pdev->ick);
+		clk_disable(pdev->fck);
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+
+		return ret;
+	}
+
+	return 0;
 }
 
 int platform_pm_poweroff(struct device *dev)
@@ -866,6 +997,9 @@ int platform_pm_poweroff(struct device *dev)
 	if (!drv)
 		return 0;
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	if (drv->pm) {
 		if (drv->pm->poweroff)
 			ret = drv->pm->poweroff(dev);
@@ -884,6 +1018,20 @@ int platform_pm_restore(struct device *dev)
 	if (!drv)
 		return 0;
 
+	ret = clk_enable(pdev->ick);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(pdev->fck);
+	if (ret) {
+		clk_disable(pdev->ick);
+		return ret;
+	}
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	if (drv->pm) {
 		if (drv->pm->restore)
 			ret = drv->pm->restore(dev);
@@ -891,14 +1039,25 @@ int platform_pm_restore(struct device *dev)
 		ret = platform_legacy_resume(dev);
 	}
 
-	return ret;
+	if (ret) {
+		clk_disable(pdev->ick);
+		clk_disable(pdev->fck);
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+
+		return ret;
+	}
+
+	return 0;
 }
 
 #endif /* CONFIG_HIBERNATE_CALLBACKS */
 
 static const struct dev_pm_ops platform_dev_pm_ops = {
-	.runtime_suspend = pm_generic_runtime_suspend,
-	.runtime_resume = pm_generic_runtime_resume,
+	SET_RUNTIME_PM_OPS(platform_pm_runtime_suspend,
+			platform_pm_runtime_resume,
+			NULL)
 	USE_PLATFORM_PM_SLEEP_OPS
 };
 
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..91133f5 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -12,6 +12,7 @@
 #define _PLATFORM_DEVICE_H_
 
 #include <linux/device.h>
+#include <linux/clk.h>
 #include <linux/mod_devicetable.h>
 
 #define PLATFORM_DEVID_NONE	(-1)
@@ -21,6 +22,10 @@ struct mfd_cell;
 
 struct platform_device {
 	const char	*name;
+
+	struct clk	*fck;
+	struct clk	*ick;
+
 	int		id;
 	bool		id_auto;
 	struct device	dev;
@@ -179,8 +184,12 @@ struct platform_driver {
 	struct device_driver driver;
 	const struct platform_device_id *id_table;
 	bool prevent_deferred_probe;
+	unsigned long flags;
 };
 
+#define PLATFORM_PM_RUNTIME_KEEP_ICK	BIT(0)
+#define PLATFORM_PM_RUNTIME_KEEP_FCK	BIT(1)
+
 #define to_platform_driver(drv)	(container_of((drv), struct platform_driver, \
 				 driver))
 
-- 
1.8.5.2

Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Russell King - ARM Linux]

On Fri, Jan 31, 2014 at 12:12:45PM -0600, Felipe Balbi wrote:
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 3a94b79..86aeb5b 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -484,6 +484,21 @@ static int platform_drv_probe(struct device *_dev)
>  	if (ACPI_HANDLE(_dev))
>  		acpi_dev_pm_attach(_dev, true);
>  
> +	dev->fck = devm_clk_get(_dev, "fck");
> +	dev->ick = devm_clk_get(_dev, "ick");
> +
> +	if (!IS_ERR(dev->fck))
> +		clk_prepare_enable(dev->fck);
> +	else
> +		dev->fck = NULL;
> +
> +	if (!IS_ERR(dev->ick))
> +		clk_prepare_enable(dev->ick);
> +	else
> +		dev->ick = NULL;

If people are going to continue doing this (converting error values to
NULL) can we please have a check in devm_clk_get() which prevents it
returning NULL if the implementation happens to do so?

It's either that or we force all users to conform to the API which
specifies that the error values are defined by IS_ERR() returning
true and everything else must be considered as a potential valid return.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Felipe Balbi]

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

Hi,

On Fri, Jan 31, 2014 at 08:04:35PM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 31, 2014 at 12:12:45PM -0600, Felipe Balbi wrote:
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b79..86aeb5b 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -484,6 +484,21 @@ static int platform_drv_probe(struct device *_dev)
> >  	if (ACPI_HANDLE(_dev))
> >  		acpi_dev_pm_attach(_dev, true);
> >  
> > +	dev->fck = devm_clk_get(_dev, "fck");
> > +	dev->ick = devm_clk_get(_dev, "ick");
> > +
> > +	if (!IS_ERR(dev->fck))
> > +		clk_prepare_enable(dev->fck);
> > +	else
> > +		dev->fck = NULL;
> > +
> > +	if (!IS_ERR(dev->ick))
> > +		clk_prepare_enable(dev->ick);
> > +	else
> > +		dev->ick = NULL;
> 
> If people are going to continue doing this (converting error values to
> NULL) can we please have a check in devm_clk_get() which prevents it
> returning NULL if the implementation happens to do so?
> 
> It's either that or we force all users to conform to the API which
> specifies that the error values are defined by IS_ERR() returning
> true and everything else must be considered as a potential valid return.

The idea here was just to avoid IS_ERR() checks every time we want to
enable/disable a clock since clk API already copes with NULL pointers.

This also helps with the fact that platform_bus is also used with
platforms which don't have (or otherwise don't need) any clock control
whatsoever, thus made it optional.

If everybody prefers duplication of IS_ERR() all over the place, that's
fine too.

-- 
balbi

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

Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Alan Stern]

On Fri, 31 Jan 2014, Felipe Balbi wrote:

> Still TODO a commit log. Not for merging!!!!!
> 
> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> This patch is an idea I've had recently in order to combine several different
> PM implementations into the platform-bus.
> 
> This patch is bare minimum for platforms which need to handle functional and
> interface clocks but the whole thing is made optional.
> 
> Note that this patch makes sure that by the time a platform_driver's probe is
> called, we already have clocks enabled and pm_runtime_set_active() has been
> called, thus making sure that a device driver's pm_runtime_get_sync() will
> solely increase the pm usage counter.
> 
> I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause any
> issues since the clock API has ref counting too.
> 
> Would really like to get some review from several folks involved with ARM SoC
> PM so that's the reason for the wide audience. If I have missed anybody, please
> add them to Cc.
> 
> As mentioned above, this is *NOT* meant for merging, but serves as a starting
> point for discussing some convergence of several PM domain implementations on
> different arch/arm/mach-* directories.

You might want to copy the runtime-PM approach used by the PCI 
subsystem.  It works like this:

	The core invokes a driver's probe routine with runtime PM 
	enabled, the device in the ACTIVE state, and the usage counter 
	incremented by 1.

	If the driver wants to support runtime PM, the probe routine
	can call pm_runtime_put_noidle.

	The core does pm_runtime_get_sync before invoking the driver's
	remove routine.  At this point a runtime-PM-aware driver whould 
	call pm_runtime_get_noresume, to balance the _put during probe.

	After invoking the remove routine, the core does a put_noidle
	(to balance the get_sync) and a final put_sync (to balance the
	increment before probe and to leave the device at low power.)

A nice consequence is that everything is transparent for drivers that 
don't support runtime PM.  The usage counter remains > 0 the entire 
time the driver is bound.

Conversely, drivers that do support runtime PM merely have to add one 
call during probe and one during remove.

There is one tricky aspect to all this.  The driver core sets the
dev->driver field before calling the subsystem core's probe routine.  
As a result, the subsystem has to be very careful about performing
runtime PM before invoking the driver's probe routine.  Otherwise you
will end up calling the driver's runtime_resume callback before the
driver's probe!  (And of course, the same problem exists in reverse
during remove.)

Alan Stern

Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Felipe Balbi]

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

Hi,

On Fri, Jan 31, 2014 at 04:34:27PM -0500, Alan Stern wrote:
> On Fri, 31 Jan 2014, Felipe Balbi wrote:
> 
> > Still TODO a commit log. Not for merging!!!!!
> > 
> > NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > 
> > This patch is an idea I've had recently in order to combine several different
> > PM implementations into the platform-bus.
> > 
> > This patch is bare minimum for platforms which need to handle functional and
> > interface clocks but the whole thing is made optional.
> > 
> > Note that this patch makes sure that by the time a platform_driver's probe is
> > called, we already have clocks enabled and pm_runtime_set_active() has been
> > called, thus making sure that a device driver's pm_runtime_get_sync() will
> > solely increase the pm usage counter.
> > 
> > I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause any
> > issues since the clock API has ref counting too.
> > 
> > Would really like to get some review from several folks involved with ARM SoC
> > PM so that's the reason for the wide audience. If I have missed anybody, please
> > add them to Cc.
> > 
> > As mentioned above, this is *NOT* meant for merging, but serves as a starting
> > point for discussing some convergence of several PM domain implementations on
> > different arch/arm/mach-* directories.
> 
> You might want to copy the runtime-PM approach used by the PCI 
> subsystem.  It works like this:
> 
> 	The core invokes a driver's probe routine with runtime PM 
> 	enabled, the device in the ACTIVE state, and the usage counter 
> 	incremented by 1.
> 
> 	If the driver wants to support runtime PM, the probe routine
> 	can call pm_runtime_put_noidle.
> 
> 	The core does pm_runtime_get_sync before invoking the driver's
> 	remove routine.  At this point a runtime-PM-aware driver whould 
> 	call pm_runtime_get_noresume, to balance the _put during probe.
> 
> 	After invoking the remove routine, the core does a put_noidle
> 	(to balance the get_sync) and a final put_sync (to balance the
> 	increment before probe and to leave the device at low power.)
> 
> A nice consequence is that everything is transparent for drivers that 
> don't support runtime PM.  The usage counter remains > 0 the entire 
> time the driver is bound.
> 
> Conversely, drivers that do support runtime PM merely have to add one 
> call during probe and one during remove.
> 
> There is one tricky aspect to all this.  The driver core sets the
> dev->driver field before calling the subsystem core's probe routine.  
> As a result, the subsystem has to be very careful about performing
> runtime PM before invoking the driver's probe routine.  Otherwise you
> will end up calling the driver's runtime_resume callback before the
> driver's probe!  (And of course, the same problem exists in reverse
> during remove.)

I can, certainly, do that and that would, most likely, simplify a whole
bunch of drivers. But that change, I suppose, would be a whole lot more
invasive. I'll spend some time studying PCI pm_runtime support, thanks
for the tip.

cheers

-- 
balbi

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

Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Laurent Pinchart]

Hi Felipe,

(CC'ing Geert Uytterhoeven as we happen to discuss runtime PM and clock 
handling for the Renesas SoCs at the moment)

Thank you for the patch. This is a bit of a late reply, but that's better than 
no reply I suppose. Please see below for a small comment.

On Friday 31 January 2014 12:12:45 Felipe Balbi wrote:
> Still TODO a commit log. Not for merging!!!!!
> 
> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> This patch is an idea I've had recently in order to combine several
> different PM implementations into the platform-bus.
> 
> This patch is bare minimum for platforms which need to handle functional and
> interface clocks but the whole thing is made optional.
> 
> Note that this patch makes sure that by the time a platform_driver's probe
> is called, we already have clocks enabled and pm_runtime_set_active() has
> been called, thus making sure that a device driver's pm_runtime_get_sync()
> will solely increase the pm usage counter.
> 
> I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause
> any issues since the clock API has ref counting too.
> 
> Would really like to get some review from several folks involved with ARM
> SoC PM so that's the reason for the wide audience. If I have missed
> anybody, please add them to Cc.
> 
> As mentioned above, this is *NOT* meant for merging, but serves as a
> starting point for discussing some convergence of several PM domain
> implementations on different arch/arm/mach-* directories.
> 
> For OMAP, this has the added benefit of removing clock handling from
> omap_device/omap_hwmod and, thus, dropping the need for so many DT_CLK()
> tables under drivers/clk/ti/
> 
>  drivers/base/platform.c         | 169 +++++++++++++++++++++++++++++++++++--
>  include/linux/platform_device.h |   9 +++
>  2 files changed, 173 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 3a94b79..86aeb5b 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -484,6 +484,21 @@ static int platform_drv_probe(struct device *_dev)
>  	if (ACPI_HANDLE(_dev))
>  		acpi_dev_pm_attach(_dev, true);
> 
> +	dev->fck = devm_clk_get(_dev, "fck");
> +	dev->ick = devm_clk_get(_dev, "ick");

My concern with this that some devices might want to handle their functional 
and interface clocks manually if they have exotic requirements. They would 
thus need a way to signal that the platform bus core should not try to perform 
clock management on its own.

One possible way would be to name the clocks differently for those devices, 
but we might then have a backward compatibility issue with DT.

Another concern is how to handle the DT backward compatibility for devices 
that would benefit from core management of their functional and interface 
clocks, but that currently don't name those clocks "fck" and "ick". Renaming 
those clocks in DT wouldn't be a problem for the future, but backward 
compatibility needs to be handled. Maybe platforms could provide aliases in 
that case.

> +	if (!IS_ERR(dev->fck))
> +		clk_prepare_enable(dev->fck);
> +	else
> +		dev->fck = NULL;
> +
> +	if (!IS_ERR(dev->ick))
> +		clk_prepare_enable(dev->ick);
> +	else
> +		dev->ick = NULL;
> +
> +	pm_runtime_set_active(_dev);
> +
>  	ret = drv->probe(dev);
>  	if (ret && ACPI_HANDLE(_dev))
>  		acpi_dev_pm_detach(_dev, true);
> @@ -507,6 +522,14 @@ static int platform_drv_remove(struct device *_dev)
>  	struct platform_device *dev = to_platform_device(_dev);
>  	int ret;
> 
> +	if (!IS_ERR(dev->ick))
> +		clk_disable_unprepare(dev->ick);
> +
> +	if (!IS_ERR(dev->fck))
> +		clk_disable_unprepare(dev->fck);
> +
> +	pm_runtime_set_suspended(_dev);
> +
>  	ret = drv->remove(dev);
>  	if (ACPI_HANDLE(_dev))
>  		acpi_dev_pm_detach(_dev, true);
> @@ -780,6 +803,59 @@ static int platform_legacy_resume(struct device *dev)
> 
>  #endif /* CONFIG_PM_SLEEP */
> 
> +#ifdef CONFIG_PM_RUNTIME
> +int platform_pm_runtime_suspend(struct device *dev)
> +{
> +	struct device_driver *drv = dev->driver;
> +	struct platform_driver *pdrv = to_platform_driver(drv);
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int ret;
> +
> +	ret = pm_generic_runtime_suspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	if (!test_bit(PLATFORM_PM_RUNTIME_KEEP_ICK, &pdrv->flags))
> +		clk_disable(pdev->ick);
> +
> +	if (!test_bit(PLATFORM_PM_RUNTIME_KEEP_FCK, &pdrv->flags))
> +		clk_disable(pdev->fck);
> +
> +	return ret;
> +}
> +
> +int platform_pm_runtime_resume(struct device *dev)
> +{
> +	struct device_driver *drv = dev->driver;
> +	struct platform_driver *pdrv = to_platform_driver(drv);
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int ret;
> +
> +	if (!test_bit(PLATFORM_PM_RUNTIME_KEEP_ICK, &pdrv->flags)) {
> +		ret = clk_enable(pdev->ick);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!test_bit(PLATFORM_PM_RUNTIME_KEEP_FCK, &pdrv->flags)) {
> +		ret = clk_enable(pdev->fck);
> +		if (ret) {
> +			clk_disable(pdev->ick);
> +			return ret;
> +		}
> +	}
> +
> +	ret = pm_generic_runtime_suspend(dev);
> +	if (ret) {
> +		clk_disable(pdev->ick);
> +		clk_disable(pdev->fck);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +#endif
> +
>  #ifdef CONFIG_SUSPEND
> 
>  int platform_pm_suspend(struct device *dev)
> @@ -790,6 +866,9 @@ int platform_pm_suspend(struct device *dev)
>  	if (!drv)
>  		return 0;
> 
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
>  	if (drv->pm) {
>  		if (drv->pm->suspend)
>  			ret = drv->pm->suspend(dev);
> @@ -802,12 +881,27 @@ int platform_pm_suspend(struct device *dev)
> 
>  int platform_pm_resume(struct device *dev)
>  {
> +	struct platform_device *pdev = to_platform_device(dev);
>  	struct device_driver *drv = dev->driver;
>  	int ret = 0;
> 
>  	if (!drv)
>  		return 0;
> 
> +	ret = clk_enable(pdev->ick);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable(pdev->fck);
> +	if (ret) {
> +		clk_disable(pdev->ick);
> +		return ret;
> +	}
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	if (drv->pm) {
>  		if (drv->pm->resume)
>  			ret = drv->pm->resume(dev);
> @@ -815,7 +909,17 @@ int platform_pm_resume(struct device *dev)
>  		ret = platform_legacy_resume(dev);
>  	}
> 
> -	return ret;
> +	if (ret) {
> +		clk_disable(pdev->ick);
> +		clk_disable(pdev->fck);
> +		pm_runtime_disable(dev);
> +		pm_runtime_set_suspended(dev);
> +		pm_runtime_enable(dev);
> +
> +		return ret;
> +	}
> +
> +	return 0;
>  }
> 
>  #endif /* CONFIG_SUSPEND */
> @@ -830,6 +934,9 @@ int platform_pm_freeze(struct device *dev)
>  	if (!drv)
>  		return 0;
> 
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
>  	if (drv->pm) {
>  		if (drv->pm->freeze)
>  			ret = drv->pm->freeze(dev);
> @@ -848,6 +955,20 @@ int platform_pm_thaw(struct device *dev)
>  	if (!drv)
>  		return 0;
> 
> +	ret = clk_enable(pdev->ick);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable(pdev->fck);
> +	if (ret) {
> +		clk_disable(pdev->ick);
> +		return ret;
> +	}
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	if (drv->pm) {
>  		if (drv->pm->thaw)
>  			ret = drv->pm->thaw(dev);
> @@ -855,7 +976,17 @@ int platform_pm_thaw(struct device *dev)
>  		ret = platform_legacy_resume(dev);
>  	}
> 
> -	return ret;
> +	if (ret) {
> +		clk_disable(pdev->ick);
> +		clk_disable(pdev->fck);
> +		pm_runtime_disable(dev);
> +		pm_runtime_set_suspended(dev);
> +		pm_runtime_enable(dev);
> +
> +		return ret;
> +	}
> +
> +	return 0;
>  }
> 
>  int platform_pm_poweroff(struct device *dev)
> @@ -866,6 +997,9 @@ int platform_pm_poweroff(struct device *dev)
>  	if (!drv)
>  		return 0;
> 
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
>  	if (drv->pm) {
>  		if (drv->pm->poweroff)
>  			ret = drv->pm->poweroff(dev);
> @@ -884,6 +1018,20 @@ int platform_pm_restore(struct device *dev)
>  	if (!drv)
>  		return 0;
> 
> +	ret = clk_enable(pdev->ick);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable(pdev->fck);
> +	if (ret) {
> +		clk_disable(pdev->ick);
> +		return ret;
> +	}
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	if (drv->pm) {
>  		if (drv->pm->restore)
>  			ret = drv->pm->restore(dev);
> @@ -891,14 +1039,25 @@ int platform_pm_restore(struct device *dev)
>  		ret = platform_legacy_resume(dev);
>  	}
> 
> -	return ret;
> +	if (ret) {
> +		clk_disable(pdev->ick);
> +		clk_disable(pdev->fck);
> +		pm_runtime_disable(dev);
> +		pm_runtime_set_suspended(dev);
> +		pm_runtime_enable(dev);
> +
> +		return ret;
> +	}
> +
> +	return 0;
>  }
> 
>  #endif /* CONFIG_HIBERNATE_CALLBACKS */
> 
>  static const struct dev_pm_ops platform_dev_pm_ops = {
> -	.runtime_suspend = pm_generic_runtime_suspend,
> -	.runtime_resume = pm_generic_runtime_resume,
> +	SET_RUNTIME_PM_OPS(platform_pm_runtime_suspend,
> +			platform_pm_runtime_resume,
> +			NULL)
>  	USE_PLATFORM_PM_SLEEP_OPS
>  };
> 
> diff --git a/include/linux/platform_device.h
> b/include/linux/platform_device.h index 16f6654..91133f5 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -12,6 +12,7 @@
>  #define _PLATFORM_DEVICE_H_
> 
>  #include <linux/device.h>
> +#include <linux/clk.h>
>  #include <linux/mod_devicetable.h>
> 
>  #define PLATFORM_DEVID_NONE	(-1)
> @@ -21,6 +22,10 @@ struct mfd_cell;
> 
>  struct platform_device {
>  	const char	*name;
> +
> +	struct clk	*fck;
> +	struct clk	*ick;
> +
>  	int		id;
>  	bool		id_auto;
>  	struct device	dev;
> @@ -179,8 +184,12 @@ struct platform_driver {
>  	struct device_driver driver;
>  	const struct platform_device_id *id_table;
>  	bool prevent_deferred_probe;
> +	unsigned long flags;
>  };
> 
> +#define PLATFORM_PM_RUNTIME_KEEP_ICK	BIT(0)
> +#define PLATFORM_PM_RUNTIME_KEEP_FCK	BIT(1)
> +
>  #define to_platform_driver(drv)	(container_of((drv), struct
> platform_driver, \ driver))

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Geert Uytterhoeven]

Hi Felipe,

On Wed, Mar 12, 2014 at 4:37 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Friday 31 January 2014 12:12:45 Felipe Balbi wrote:
>> Still TODO a commit log. Not for merging!!!!!
>>
>> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
>> ---
>>
>> This patch is an idea I've had recently in order to combine several
>> different PM implementations into the platform-bus.
>>
>> This patch is bare minimum for platforms which need to handle functional and
>> interface clocks but the whole thing is made optional.
>>
>> Note that this patch makes sure that by the time a platform_driver's probe
>> is called, we already have clocks enabled and pm_runtime_set_active() has
>> been called, thus making sure that a device driver's pm_runtime_get_sync()
>> will solely increase the pm usage counter.
>>
>> I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause
>> any issues since the clock API has ref counting too.
>>
>> Would really like to get some review from several folks involved with ARM
>> SoC PM so that's the reason for the wide audience. If I have missed
>> anybody, please add them to Cc.
>>
>> As mentioned above, this is *NOT* meant for merging, but serves as a
>> starting point for discussing some convergence of several PM domain
>> implementations on different arch/arm/mach-* directories.
>>
>> For OMAP, this has the added benefit of removing clock handling from
>> omap_device/omap_hwmod and, thus, dropping the need for so many DT_CLK()
>> tables under drivers/clk/ti/
>>
>>  drivers/base/platform.c         | 169 +++++++++++++++++++++++++++++++++++--
>>  include/linux/platform_device.h |   9 +++
>>  2 files changed, 173 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 3a94b79..86aeb5b 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -484,6 +484,21 @@ static int platform_drv_probe(struct device *_dev)
>>       if (ACPI_HANDLE(_dev))
>>               acpi_dev_pm_attach(_dev, true);
>>
>> +     dev->fck = devm_clk_get(_dev, "fck");
>> +     dev->ick = devm_clk_get(_dev, "ick");
>
> My concern with this that some devices might want to handle their functional
> and interface clocks manually if they have exotic requirements. They would
> thus need a way to signal that the platform bus core should not try to perform
> clock management on its own.
>
> One possible way would be to name the clocks differently for those devices,
> but we might then have a backward compatibility issue with DT.
>
> Another concern is how to handle the DT backward compatibility for devices
> that would benefit from core management of their functional and interface
> clocks, but that currently don't name those clocks "fck" and "ick". Renaming
> those clocks in DT wouldn't be a problem for the future, but backward
> compatibility needs to be handled. Maybe platforms could provide aliases in
> that case.

IIUC, on OMAP the "fck" and "ick" clock names are also not present in DT.

Device nodes have "ti,hwmods" properties, while the board code uses
struct omap_hwmod to match the "ti,hwmods" values with clock names, and
creates fck aliases for them, right?

Thanks for your answer!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Felipe Balbi]

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

Hi,

On Fri, Mar 28, 2014 at 02:20:24PM +0100, Geert Uytterhoeven wrote:
> Hi Felipe,
> 
> On Wed, Mar 12, 2014 at 4:37 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Friday 31 January 2014 12:12:45 Felipe Balbi wrote:
> >> Still TODO a commit log. Not for merging!!!!!
> >>
> >> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> >> ---
> >>
> >> This patch is an idea I've had recently in order to combine several
> >> different PM implementations into the platform-bus.
> >>
> >> This patch is bare minimum for platforms which need to handle functional and
> >> interface clocks but the whole thing is made optional.
> >>
> >> Note that this patch makes sure that by the time a platform_driver's probe
> >> is called, we already have clocks enabled and pm_runtime_set_active() has
> >> been called, thus making sure that a device driver's pm_runtime_get_sync()
> >> will solely increase the pm usage counter.
> >>
> >> I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause
> >> any issues since the clock API has ref counting too.
> >>
> >> Would really like to get some review from several folks involved with ARM
> >> SoC PM so that's the reason for the wide audience. If I have missed
> >> anybody, please add them to Cc.
> >>
> >> As mentioned above, this is *NOT* meant for merging, but serves as a
> >> starting point for discussing some convergence of several PM domain
> >> implementations on different arch/arm/mach-* directories.
> >>
> >> For OMAP, this has the added benefit of removing clock handling from
> >> omap_device/omap_hwmod and, thus, dropping the need for so many DT_CLK()
> >> tables under drivers/clk/ti/
> >>
> >>  drivers/base/platform.c         | 169 +++++++++++++++++++++++++++++++++++--
> >>  include/linux/platform_device.h |   9 +++
> >>  2 files changed, 173 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> >> index 3a94b79..86aeb5b 100644
> >> --- a/drivers/base/platform.c
> >> +++ b/drivers/base/platform.c
> >> @@ -484,6 +484,21 @@ static int platform_drv_probe(struct device *_dev)
> >>       if (ACPI_HANDLE(_dev))
> >>               acpi_dev_pm_attach(_dev, true);
> >>
> >> +     dev->fck = devm_clk_get(_dev, "fck");
> >> +     dev->ick = devm_clk_get(_dev, "ick");
> >
> > My concern with this that some devices might want to handle their functional
> > and interface clocks manually if they have exotic requirements. They would
> > thus need a way to signal that the platform bus core should not try to perform
> > clock management on its own.
> >
> > One possible way would be to name the clocks differently for those devices,
> > but we might then have a backward compatibility issue with DT.
> >
> > Another concern is how to handle the DT backward compatibility for devices
> > that would benefit from core management of their functional and interface
> > clocks, but that currently don't name those clocks "fck" and "ick". Renaming
> > those clocks in DT wouldn't be a problem for the future, but backward
> > compatibility needs to be handled. Maybe platforms could provide aliases in
> > that case.
> 
> IIUC, on OMAP the "fck" and "ick" clock names are also not present in DT.
> 
> Device nodes have "ti,hwmods" properties, while the board code uses
> struct omap_hwmod to match the "ti,hwmods" values with clock names, and
> creates fck aliases for them, right?

right, the idea was to move clock handling away from that, because that
has been creating a few issues for drivers the biggest of which is the
fact that we can't know, by probe() time, if the device is active or
suspended. Ideally, by probe() the device would be fully active with is
runtime pm usage counter incremented so that the first pm_runtime_get*
in our probe() doesn't try to call ->runtime_resume().

-- 
balbi

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

Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus

[Felipe Balbi]

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

Hi,

On Wed, Mar 12, 2014 at 04:37:19PM +0100, Laurent Pinchart wrote:
> Hi Felipe,
> 
> (CC'ing Geert Uytterhoeven as we happen to discuss runtime PM and clock 
> handling for the Renesas SoCs at the moment)
> 
> Thank you for the patch. This is a bit of a late reply, but that's better than 
> no reply I suppose. Please see below for a small comment.
> 
> On Friday 31 January 2014 12:12:45 Felipe Balbi wrote:
> > Still TODO a commit log. Not for merging!!!!!
> > 
> > NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > 
> > This patch is an idea I've had recently in order to combine several
> > different PM implementations into the platform-bus.
> > 
> > This patch is bare minimum for platforms which need to handle functional and
> > interface clocks but the whole thing is made optional.
> > 
> > Note that this patch makes sure that by the time a platform_driver's probe
> > is called, we already have clocks enabled and pm_runtime_set_active() has
> > been called, thus making sure that a device driver's pm_runtime_get_sync()
> > will solely increase the pm usage counter.
> > 
> > I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause
> > any issues since the clock API has ref counting too.
> > 
> > Would really like to get some review from several folks involved with ARM
> > SoC PM so that's the reason for the wide audience. If I have missed
> > anybody, please add them to Cc.
> > 
> > As mentioned above, this is *NOT* meant for merging, but serves as a
> > starting point for discussing some convergence of several PM domain
> > implementations on different arch/arm/mach-* directories.
> > 
> > For OMAP, this has the added benefit of removing clock handling from
> > omap_device/omap_hwmod and, thus, dropping the need for so many DT_CLK()
> > tables under drivers/clk/ti/
> > 
> >  drivers/base/platform.c         | 169 +++++++++++++++++++++++++++++++++++--
> >  include/linux/platform_device.h |   9 +++
> >  2 files changed, 173 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b79..86aeb5b 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -484,6 +484,21 @@ static int platform_drv_probe(struct device *_dev)
> >  	if (ACPI_HANDLE(_dev))
> >  		acpi_dev_pm_attach(_dev, true);
> > 
> > +	dev->fck = devm_clk_get(_dev, "fck");
> > +	dev->ick = devm_clk_get(_dev, "ick");
> 
> My concern with this that some devices might want to handle their functional 
> and interface clocks manually if they have exotic requirements. They would 
> thus need a way to signal that the platform bus core should not try to perform 
> clock management on its own.
> 
> One possible way would be to name the clocks differently for those devices, 
> but we might then have a backward compatibility issue with DT.
> 
> Another concern is how to handle the DT backward compatibility for devices 
> that would benefit from core management of their functional and interface 
> clocks, but that currently don't name those clocks "fck" and "ick". Renaming 
> those clocks in DT wouldn't be a problem for the future, but backward 
> compatibility needs to be handled. Maybe platforms could provide aliases in 
> that case.

yeah, I guess alias would be the way to go. Another possible way would
be grab the clocks by phandle if of_node is a valid pointer. At the end
of the day, the name of the clock shouldn't matter.

-- 
balbi

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