Re: [PATCH] i2c-s3c2410: Add stub runtime power management

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Sylwester Nawrocki]

On 01/21/2012 07:46 PM, Mark Brown wrote:
> Add stub runtime_pm calls which go through the flow of enabling and
> disabling but don't actually do anything with the device itself as
> there's nothing useful we can do. This provides the core PM framework
> with information about when the device is idle, enabling chip wide
> power savings.
> 
> Signed-off-by: Mark Brown<broonie@opensource.wolfsonmicro.com>
> Acked-by: Heiko Stuebner<heiko@sntech.de>
> ---
>   drivers/i2c/busses/i2c-s3c2410.c |    8 ++++++++
>   1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index e6f982b..3d80bab 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -31,6 +31,7 @@
>   #include<linux/errno.h>
>   #include<linux/err.h>
>   #include<linux/platform_device.h>
> +#include<linux/pm_runtime.h>
>   #include<linux/clk.h>
>   #include<linux/cpufreq.h>
>   #include<linux/slab.h>
> @@ -564,6 +565,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>   	int retry;
>   	int ret;
> 
> +	pm_runtime_get_sync(&adap->dev);

IMHO it's more appropriate to use i2c->dev here instead, i.e. to reference
the platform device we've enabled runtime PM for.

>   	clk_enable(i2c->clk);
> 
>   	for (retry = 0; retry<  adap->retries; retry++) {
> @@ -572,6 +574,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
> 
>   		if (ret != -EAGAIN) {
>   			clk_disable(i2c->clk);
> +			pm_runtime_put(&adap->dev);

Ditto.

>   			return ret;
>   		}
> 
> @@ -581,6 +584,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>   	}
> 
>   	clk_disable(i2c->clk);
> +	pm_runtime_put(&adap->dev);

Ditto.

>   	return -EREMOTEIO;
>   }
> 
> @@ -1013,6 +1017,8 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>   	of_i2c_register_devices(&i2c->adap);
>   	platform_set_drvdata(pdev, i2c);
> 
> +	pm_runtime_enable(&pdev->dev);
> +
>   	dev_info(&pdev->dev, "%s: S3C I2C adapter\n", dev_name(&i2c->adap.dev));
>   	clk_disable(i2c->clk);
>   	return 0;
> @@ -1047,6 +1053,8 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
>   {
>   	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
> 
> +	pm_runtime_disable(&pdev->dev);
> +
>   	s3c24xx_i2c_deregister_cpufreq(i2c);
> 
>   	i2c_del_adapter(&i2c->adap);

How about the following patch (untested) ? It might be a better start for 
proper power management implementation and would still allow the driver 
to work on platforms that don't support runtime PM. 

8<----------------------------------------------

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 4c17180..0f588bb 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -31,6 +31,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/slab.h>
@@ -551,6 +552,26 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	return ret;
 }
 
+static inline int s3c24xx_pm_runtime_get(struct s3c24xx_i2c *i2c)
+{
+	if (!pm_runtime_enabled(i2c->dev)) {
+		clk_enable(i2c->clk);
+		return 0;
+	}
+
+	return pm_runtime_get_sync(i2c->dev);
+}
+
+static inline int s3c24xx_pm_runtime_put(struct s3c24xx_i2c *i2c)
+{
+	if (!pm_runtime_enabled(i2c->dev)) {
+		clk_disable(i2c->clk);
+		return 0;
+	}
+
+	return pm_runtime_put(i2c->dev);
+}
+
 /* s3c24xx_i2c_xfer
  *
  * first port of call from the i2c bus code when an message needs
@@ -564,14 +585,14 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	int retry;
 	int ret;
 
-	clk_enable(i2c->clk);
+	s3c24xx_pm_runtime_get(i2c);
 
 	for (retry = 0; retry < adap->retries; retry++) {
 
 		ret = s3c24xx_i2c_doxfer(i2c, msgs, num);
 
 		if (ret != -EAGAIN) {
-			clk_disable(i2c->clk);
+			s3c24xx_pm_runtime_put(i2c);
 			return ret;
 		}
 
@@ -580,7 +601,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 		udelay(100);
 	}
 
-	clk_disable(i2c->clk);
+	s3c24xx_pm_runtime_put(i2c);
 	return -EREMOTEIO;
 }
 
@@ -929,7 +950,9 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "clock source %p\n", i2c->clk);
 
-	clk_enable(i2c->clk);
+	platform_set_drvdata(pdev, i2c);
+	pm_runtime_enable(i2c->dev);
+	s3c24xx_pm_runtime_get(i2c);
 
 	/* map the registers */
 
@@ -1011,10 +1034,9 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	}
 
 	of_i2c_register_devices(&i2c->adap);
-	platform_set_drvdata(pdev, i2c);
 
 	dev_info(&pdev->dev, "%s: S3C I2C adapter\n", dev_name(&i2c->adap.dev));
-	clk_disable(i2c->clk);
+	s3c24xx_pm_runtime_put(i2c);
 	return 0;
 
  err_cpufreq:
@@ -1048,6 +1070,8 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
 {
 	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
 
+	pm_runtime_disable(&pdev->dev);
+
 	s3c24xx_i2c_deregister_cpufreq(i2c);
 
 	i2c_del_adapter(&i2c->adap);
@@ -1066,7 +1090,28 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_RUNTIME
+static int s3c24xx_i2c_runtime_suspend(struct device *dev)
+{
+	struct s3c24xx_i2c *i2c = dev_get_drvdata(dev);
+
+	clk_disable(i2c->clk);
+	return 0;
+}
+
+static int s3c24xx_i2c_runtime_resume(struct device *dev)
+{
+	struct s3c24xx_i2c *i2c = dev_get_drvdata(dev);
+
+	clk_enable(i2c->clk);
+	return 0;
+}
+#else
+#define s3c24xx_i2c_runtime_suspend NULL
+#define s3c24xx_i2c_runtime_resume NULL
+#endif
+
+#ifdef CONFIG_PM_SLEEP
 static int s3c24xx_i2c_suspend_noirq(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -1089,17 +1134,18 @@ static int s3c24xx_i2c_resume(struct device *dev)
 
 	return 0;
 }
+#else
+#define s3c24xx_i2c_suspend_noirq NULL
+#define s3c24xx_i2c_resume NULL
+#endif
 
 static const struct dev_pm_ops s3c24xx_i2c_dev_pm_ops = {
 	.suspend_noirq = s3c24xx_i2c_suspend_noirq,
 	.resume = s3c24xx_i2c_resume,
+	.runtime_suspend = s3c24xx_i2c_runtime_suspend,
+	.runtime_resume = s3c24xx_i2c_runtime_resume,
 };
 
-#define S3C24XX_DEV_PM_OPS (&s3c24xx_i2c_dev_pm_ops)
-#else
-#define S3C24XX_DEV_PM_OPS NULL
-#endif
-
 /* device driver for platform bus bits */
 
 static struct platform_device_id s3c24xx_driver_ids[] = {
@@ -1131,7 +1177,7 @@ static struct platform_driver s3c24xx_i2c_driver = {
 	.driver		= {
 		.owner	= THIS_MODULE,
 		.name	= "s3c-i2c",
-		.pm	= S3C24XX_DEV_PM_OPS,
+		.pm	= &s3c24xx_i2c_dev_pm_ops,
 		.of_match_table = s3c24xx_i2c_match,
 	},
 };

8>----------------------------------------------


-- 
Thanks,
Sylwester

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

On Sun, Jan 22, 2012 at 01:59:25PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 07:46 PM, Mark Brown wrote:

> > +	pm_runtime_get_sync(&adap->dev);

> IMHO it's more appropriate to use i2c->dev here instead, i.e. to reference
> the platform device we've enabled runtime PM for.

Oh, bah humbug.  Now that I notice this I think that's the reason I
enabled runtime PM for the adaptor - it's easier to get hold of.

> How about the following patch (untested) ? It might be a better start for 
> proper power management implementation and would still allow the driver 
> to work on platforms that don't support runtime PM. 

It's not really a platform issue - the platform bus by default does the
right thing and the runtime PM core is pure software, the only platform
dependency is if users have bothered truning runtime PM on.

> +static inline int s3c24xx_pm_runtime_put(struct s3c24xx_i2c *i2c)
> +{
> +	if (!pm_runtime_enabled(i2c->dev)) {
> +		clk_disable(i2c->clk);
> +		return 0;
> +	}
> +
> +	return pm_runtime_put(i2c->dev);
> +}

To be honest I don't think this is worth it.  Either we just keep the
clock management outside of runtime PM or we push it in but trying to
support both simultaneously adds complication and doesn't actually do
all that much in practical terms.

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Sylwester Nawrocki]

On 01/22/2012 04:22 PM, Mark Brown wrote:
> On Sun, Jan 22, 2012 at 01:59:25PM +0100, Sylwester Nawrocki wrote:
>> How about the following patch (untested) ? It might be a better start for
>> proper power management implementation and would still allow the driver
>> to work on platforms that don't support runtime PM.
> 
> It's not really a platform issue - the platform bus by default does the
> right thing and the runtime PM core is pure software, the only platform
> dependency is if users have bothered truning runtime PM on.

It is a platform (SoC) issue in terms of enabling PM_RUNTIME, which is 
pure software, as you're pointing out. I think we agree here that the 
only issue is that some (ill) platforms refuse to enable PM_RUNTIME.

And clock gating belongs to runtime PM, which this I2C controller driver
chooses to implement without runtime PM awareness. 

>> +static inline int s3c24xx_pm_runtime_put(struct s3c24xx_i2c *i2c)
>> +{
>> +	if (!pm_runtime_enabled(i2c->dev)) {
>> +		clk_disable(i2c->clk);
>> +		return 0;
>> +	}
>> +
>> +	return pm_runtime_put(i2c->dev);
>> +}
> 
> To be honest I don't think this is worth it.  Either we just keep the
> clock management outside of runtime PM or we push it in but trying to
> support both simultaneously adds complication and doesn't actually do
> all that much in practical terms.

It's not pretty, I agree. However it is supposed to be more a temporary
solution, rather than anything final. And it probably is better at 
avoiding any races and mismatch between real device state and the PM core
knowledge of it. I'm not going to persuade my version too much, it's far
from prefect, similarly as the original patch in this thread. The best 
solution would be to force platform to enable PM_RUNTIME if they want to
use a driver which does runtime PM. But this has to wait, yet, probably
infinitely.. :)

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Bill Gatliff]

Guys:

On Sun, Jan 22, 2012 at 6:22 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> The best solution would be to force platform to enable PM_RUNTIME if they want to
> use a driver which does runtime PM. But this has to wait, yet, probably
> infinitely.. :)

Probably easier to motivate platforms to enable PM_RUNTIME if there
are drivers that require it.  :)

I for one would rather see in-kernel drivers that require it, and then
patches in mailing lists that show how to un-require it.  Make it more
painful to avoid PM_RUNTIME, and less painful to use it.

b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Sylwester Nawrocki]

Hi,

On 01/22/2012 06:27 PM, Bill Gatliff wrote:
> On Sun, Jan 22, 2012 at 6:22 PM, Sylwester Nawrocki
> <sylvester.nawrocki@gmail.com>  wrote:
>> The best solution would be to force platform to enable PM_RUNTIME if 
>> they want to use a driver which does runtime PM. But this has to wait, 
>> yet, probably infinitely.. :)
> 
> Probably easier to motivate platforms to enable PM_RUNTIME if there
> are drivers that require it.  :)
>
> I for one would rather see in-kernel drivers that require it, and then

In fact we have to deal with the opposite now, as some existing drivers
have been used for multiple generations of SoC, where almost unchanged
device IPs are deployed. Those drivers were originally written for the
simplest SoCs.

> patches in mailing lists that show how to un-require it.  Make it more
> painful to avoid PM_RUNTIME, and less painful to use it.

Yeah, makes a lot of sense. With new code there is no issue, only the code
with long history is sort of troublesome.

-- 
Regards,
Sylwester

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

On Sun, Jan 22, 2012 at 06:48:36PM +0100, Sylwester Nawrocki wrote:
> On 01/22/2012 06:27 PM, Bill Gatliff wrote:

> > I for one would rather see in-kernel drivers that require it, and then

> In fact we have to deal with the opposite now, as some existing drivers
> have been used for multiple generations of SoC, where almost unchanged
> device IPs are deployed. Those drivers were originally written for the
> simplest SoCs.

TBH I think most of the devices for which people are running these days
will be able to get some win from the system wide stuff - the WFI modes
aren't exactly the latest thing in hardware terms, it's just been a long
road to getting them supported.  Infrastructure like Mark's PM QoS work
and Raphael's PM domains work has really helped a lot here.

> > patches in mailing lists that show how to un-require it.  Make it more
> > painful to avoid PM_RUNTIME, and less painful to use it.

> Yeah, makes a lot of sense. With new code there is no issue, only the code
> with long history is sort of troublesome.

It's mostly a transition management issue I think.  When I repost I'll
add an additional patch on top which moves the clock gating into the
runtime PM callbacks, that way the decision on that doesn't block the
system wide work.

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Sylwester Nawrocki]

On 01/22/2012 10:39 PM, Mark Brown wrote:
> On Sun, Jan 22, 2012 at 06:48:36PM +0100, Sylwester Nawrocki wrote:
>> On 01/22/2012 06:27 PM, Bill Gatliff wrote:
> 
>>> I for one would rather see in-kernel drivers that require it, and then
> 
>> In fact we have to deal with the opposite now, as some existing drivers
>> have been used for multiple generations of SoC, where almost unchanged
>> device IPs are deployed. Those drivers were originally written for the
>> simplest SoCs.
> 
> TBH I think most of the devices for which people are running these days
> will be able to get some win from the system wide stuff - the WFI modes
> aren't exactly the latest thing in hardware terms, it's just been a long
> road to getting them supported.  Infrastructure like Mark's PM QoS work
> and Raphael's PM domains work has really helped a lot here.

Indeed, now that I checked some earliest SoC TRMs even the simplest systems
have some sort of intermediate Idle states.

>>> patches in mailing lists that show how to un-require it.  Make it more
>>> painful to avoid PM_RUNTIME, and less painful to use it.
> 
>> Yeah, makes a lot of sense. With new code there is no issue, only the code
>> with long history is sort of troublesome.
> 
> It's mostly a transition management issue I think.  When I repost I'll
> add an additional patch on top which moves the clock gating into the
> runtime PM callbacks, that way the decision on that doesn't block the
> system wide work.

Sounds like a great resolution to the problem, thanks for considering it.
And sorry for disturbing. Let's see what's Kukjin's opinion on that.

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Ben Dooks]

On Sun, Jan 22, 2012 at 09:39:53PM +0000, Mark Brown wrote:
> On Sun, Jan 22, 2012 at 06:48:36PM +0100, Sylwester Nawrocki wrote:
> > On 01/22/2012 06:27 PM, Bill Gatliff wrote:
> 
> > > I for one would rather see in-kernel drivers that require it, and then
> 
> > In fact we have to deal with the opposite now, as some existing drivers
> > have been used for multiple generations of SoC, where almost unchanged
> > device IPs are deployed. Those drivers were originally written for the
> > simplest SoCs.
> 
> TBH I think most of the devices for which people are running these days
> will be able to get some win from the system wide stuff - the WFI modes
> aren't exactly the latest thing in hardware terms, it's just been a long
> road to getting them supported.  Infrastructure like Mark's PM QoS work
> and Raphael's PM domains work has really helped a lot here.

The WFI stuff's been around for ages, and is generally just the core that
is affected. The deep sleep options might be useful, so I don't see any
reason not to do this. I also approve that it might allow people to shut
down I2C units they're not using at the time to save power.

-- 
Ben

Re: [PATCH] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

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

On Mon, Feb 13, 2012 at 11:31:39PM +0000, Ben Dooks wrote:
> On Sun, Jan 22, 2012 at 09:39:53PM +0000, Mark Brown wrote:

> > TBH I think most of the devices for which people are running these days
> > will be able to get some win from the system wide stuff - the WFI modes
> > aren't exactly the latest thing in hardware terms, it's just been a long
> > road to getting them supported.  Infrastructure like Mark's PM QoS work
> > and Raphael's PM domains work has really helped a lot here.

> The WFI stuff's been around for ages, and is generally just the core that
> is affected. The deep sleep options might be useful, so I don't see any
> reason not to do this. I also approve that it might allow people to shut
> down I2C units they're not using at the time to save power.

On the more recent devices they've got wider effects if you configure
the device to enter one of the lower power states on WFI - things will
break quite badly if you enter even SLEEP mode on s3c64xx right now for
example as we don't make sure things like the UARTs are quiesced enough.
The power savings are nice, though.

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