[PATCH 2/2] i2c/nomadik: runtime PM support

[PATCH 2/2] i2c/nomadik: runtime PM support

[Linus Walleij]

From: Jonas Aaberg <jonas.aberg-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>

Turn off the clock and regulator to the I2C block using runtime
PM.

Cc: Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
Signed-off-by: Jonas Aaberg <jonas.aberg-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   53 ++++++++++++++++++++++++++++----------
 1 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index ce4fd80..9ac9870 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -628,12 +628,8 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 
 	dev->busy = true;
 
-	if (dev->regulator)
-		regulator_enable(dev->regulator);
 	pm_runtime_get_sync(&dev->pdev->dev);
 
-	clk_enable(dev->clk);
-
 	status = init_hw(dev);
 	if (status)
 		goto out;
@@ -666,10 +662,8 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 	}
 
 out:
-	clk_disable(dev->clk);
-	pm_runtime_put_sync(&dev->pdev->dev);
-	if (dev->regulator)
-		regulator_disable(dev->regulator);
+
+	pm_runtime_put(&dev->pdev->dev);
 
 	dev->busy = false;
 
@@ -859,9 +853,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 
 
 #ifdef CONFIG_PM
-static int nmk_i2c_suspend(struct device *dev)
+
+static int nmk_i2c_suspend(struct platform_device *pdev, pm_message_t state)
 {
-	struct platform_device *pdev = to_platform_device(dev);
 	struct nmk_i2c_dev *nmk_i2c = platform_get_drvdata(pdev);
 
 	if (nmk_i2c->busy)
@@ -870,23 +864,53 @@ static int nmk_i2c_suspend(struct device *dev)
 	return 0;
 }
 
-static int nmk_i2c_resume(struct device *dev)
+static int nmk_i2c_suspend_noirq(struct device *dev)
 {
+	struct nmk_i2c_dev *nmk_i2c =
+		platform_get_drvdata(to_platform_device(dev));
+
+	if (nmk_i2c->busy)
+		return -EBUSY;
+
 	return 0;
 }
+
 #else
 #define nmk_i2c_suspend	NULL
-#define nmk_i2c_resume	NULL
+#define nmk_i2c_suspend_noirq NULL
 #endif
 
+static int nmk_i2c_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct nmk_i2c_dev *nmk_i2c = platform_get_drvdata(pdev);
+
+	clk_disable(nmk_i2c->clk);
+	if (nmk_i2c->regulator)
+		regulator_disable(nmk_i2c->regulator);
+	return 0;
+}
+
+static int nmk_i2c_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct nmk_i2c_dev *nmk_i2c = platform_get_drvdata(pdev);
+
+	if (nmk_i2c->regulator)
+		regulator_enable(nmk_i2c->regulator);
+	clk_enable(nmk_i2c->clk);
+	return 0;
+}
+
 /*
  * We use noirq so that we suspend late and resume before the wakeup interrupt
  * to ensure that we do the !pm_runtime_suspended() check in resume before
  * there has been a regular pm runtime resume (via pm_runtime_get_sync()).
  */
 static const struct dev_pm_ops nmk_i2c_pm = {
-	.suspend_noirq	= nmk_i2c_suspend,
-	.resume_noirq	= nmk_i2c_resume,
+	SET_RUNTIME_PM_OPS(nmk_i2c_runtime_suspend, nmk_i2c_runtime_resume,
+			   NULL)
+	.suspend_noirq	= nmk_i2c_suspend_noirq,
 };
 
 static unsigned int nmk_i2c_functionality(struct i2c_adapter *adap)
@@ -1047,6 +1071,7 @@ static struct platform_driver nmk_i2c_driver = {
 	},
 	.probe = nmk_i2c_probe,
 	.remove = __devexit_p(nmk_i2c_remove),
+	.suspend = nmk_i2c_suspend,
 };
 
 static int __init nmk_i2c_init(void)
-- 
1.7.3.2

Re: [PATCH 2/2] i2c/nomadik: runtime PM support

[Magnus Damm]

On Fri, Oct 21, 2011 at 1:23 AM, Linus Walleij
<linus.walleij-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org> wrote:
> From: Jonas Aaberg <jonas.aberg-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
>
> Turn off the clock and regulator to the I2C block using runtime
> PM.
>
> Cc: Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> Signed-off-by: Jonas Aaberg <jonas.aberg-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-nomadik.c |   53 ++++++++++++++++++++++++++++----------
>  1 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index ce4fd80..9ac9870 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c

[snip]

> +static int nmk_i2c_runtime_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct nmk_i2c_dev *nmk_i2c = platform_get_drvdata(pdev);
> +
> +       clk_disable(nmk_i2c->clk);
> +       if (nmk_i2c->regulator)
> +               regulator_disable(nmk_i2c->regulator);
> +       return 0;
> +}
> +
> +static int nmk_i2c_runtime_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct nmk_i2c_dev *nmk_i2c = platform_get_drvdata(pdev);
> +
> +       if (nmk_i2c->regulator)
> +               regulator_enable(nmk_i2c->regulator);
> +       clk_enable(nmk_i2c->clk);
> +       return 0;
> +}
> +

Hm, on SH-Mobile ARM we never control any clocks from the driver
runtime pm callbacks.

In our case the SoC code may associate various clocks with the struct
device and those clocks will be disabled and enabled automatically
when the driver does runtime pm put() and get() touch zero use count
for the device. The driver is happily unaware how many and which
clocks that are associated with the struct device.

So the clocks are started and stopped depending on the use count value
for a single device, but the actual driver callbacks are not however
invoked. In our case the runtime suspend callbacks are only invoked
when all the devices in the power domain happen to have a zero runtime
pm use count. So if you control your clocks from those callbacks then
they will be mostly left on which may not be what you want.

Perhaps your soc-specific code invokes the runtime pm suspend / resume
callbacks regardless of the state of all devices in a certain power
domain?

Cheers,

/ magnus

Re: [PATCH 2/2] i2c/nomadik: runtime PM support

[Linus Walleij]

On Thu, Oct 20, 2011 at 6:44 PM, Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Oct 21, 2011 at 1:23 AM, Linus Walleij
> <linus.walleij-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org> wrote:
>> From: Jonas Aaberg <jonas.aberg-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
...
>> +static int nmk_i2c_runtime_suspend(struct device *dev)
>> +{
>> +       clk_disable(nmk_i2c->clk);
(...)
>> +}
>> +
>> +static int nmk_i2c_runtime_resume(struct device *dev)
>> +{
(...)
>> +       clk_enable(nmk_i2c->clk);
>> +       return 0;
>> +}
>> +
>
> Hm, on SH-Mobile ARM we never control any clocks from the driver
> runtime pm callbacks.

OK so you're using pm_clk_add_notifier() and the standard
implementation from drivers/base/power/clock_ops.c?

> (...) In our case the runtime suspend callbacks are only invoked
> when all the devices in the power domain happen to have a zero runtime
> pm use count. So if you control your clocks from those callbacks then
> they will be mostly left on which may not be what you want.

We have power domains implemented as well, but currently these
handle power domain regulators and pin control only,
not clocking.

They are registered to the platform and amba bus using
bus_register_notifier() rather than pm_*() notifiers though,
but we listen to the same events
BUS_NOTIFY_[BIND|UNBOUND]_DRIVER

So your suggestion is to move clock control for all
platform devices to the overarching platform code where
we also handle the power domain regulators and pin
control?

Currently we have something similar for the AMBA
bus here, since it actually has code in place to grab
the block clock on probe() and its own runtime PM
callbacks. Since the bus driver holds the actual reference
to the clock, we have to call down into the bus callbacks
from the platform runtime PM implementation since our
implementation takes precedence. I guess this is expected
way of doing it?

I see three possible paradigms here:

(A) dev_power_domain implementation in
  platform/board code handle silicon clocks and regulators.

(B) Bus (as AMBA) driver handle silicon clocks and
  regulators

(C) Devices handle clocks and regulators in their runtime
  PM callbacks themselves

So now ARM SHmobile does (A), PrimeCell drivers do
(B) and this patch attempted to do (C).

And our way of handling AMBA is a mixture calling
the bus callbacks from the platform like (A)+(B)

I guess (A) and (C) won't mix very well since both
handle platform devices and things will screw up if
say our platform and SHmobile would use the same
driver.

Yours,
Linus Walleij

Re: [PATCH 2/2] i2c/nomadik: runtime PM support

[Magnus Damm]

On Fri, Oct 21, 2011 at 8:19 PM, Linus Walleij <linus.walleij-QSEj5FYQhm5QFI55V6+gNQ@public.gmane.orgg> wrote:
> On Thu, Oct 20, 2011 at 6:44 PM, Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Fri, Oct 21, 2011 at 1:23 AM, Linus Walleij
>> <linus.walleij-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org> wrote:
>>> From: Jonas Aaberg <jonas.aberg-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> ...
>>> +static int nmk_i2c_runtime_suspend(struct device *dev)
>>> +{
>>> +       clk_disable(nmk_i2c->clk);
> (...)
>>> +}
>>> +
>>> +static int nmk_i2c_runtime_resume(struct device *dev)
>>> +{
> (...)
>>> +       clk_enable(nmk_i2c->clk);
>>> +       return 0;
>>> +}
>>> +
>>
>> Hm, on SH-Mobile ARM we never control any clocks from the driver
>> runtime pm callbacks.
>
> OK so you're using pm_clk_add_notifier() and the standard
> implementation from drivers/base/power/clock_ops.c?

We are using the standard implementation together with
pm_clk_add_notifier() to point out our platform_bus_notifier in
arch/arm/mach-shmobile/pm_runtime.c. My intention with the comment
above was not to so focused on the notifier, more that soc-specific
code can use pm_clk_add() to add multiple clocks to a certain struct
device.

>> (...) In our case the runtime suspend callbacks are only invoked
>> when all the devices in the power domain happen to have a zero runtime
>> pm use count. So if you control your clocks from those callbacks then
>> they will be mostly left on which may not be what you want.
>
> We have power domains implemented as well, but currently these
> handle power domain regulators and pin control only,
> not clocking.
>
> They are registered to the platform and amba bus using
> bus_register_notifier() rather than pm_*() notifiers though,
> but we listen to the same events
> BUS_NOTIFY_[BIND|UNBOUND]_DRIVER

I probably mentioned this earlier, but on SH-Mobile ARM drivers we
control both clocks and power domains using the same
pm_runtime_get()/put() functions. The clocks are turned off
immediately, but the power domains will only be turned off when all
included devices are idle according to Runtime PM. At this
all-devices-in-one-power-domain-are-idle time we actually invoke the
->runtime_suspend() callbacks. This is handled by generic code and can
easily be used by anyone.

> So your suggestion is to move clock control for all
> platform devices to the overarching platform code where
> we also handle the power domain regulators and pin
> control?

No, I wouldn't go that far to suggest that. =) I'm just saying that
using Runtime PM for clock and power domain control on a per-device
granularity in the driver is working well for us. Especially since we
can let the soc-code associate multiple clocks to each struct device
with pm_clk_add().

I'm soon going to hack on trying to tie in voltage control in the
soc-specific Runtime PM code. Right now the voltage is static but we
gate off various parts of the SoC. After prototyping with the voltage
control code then perhaps it would be easier for me to recommend you
something. =)

The reason why I replied to the email initially was to point out that
your way of using Runtime PM seems quite different from our way. It
looked like this driver wanted to have the ->runtime_suspend()
callback executed as soon as possible to control the clock. So if you
intend to execute the ->runtime_suspend() callbacks directly then it
may become difficult for you to use the common pm domain code base
that Rafael has been working on.

> Currently we have something similar for the AMBA
> bus here, since it actually has code in place to grab
> the block clock on probe() and its own runtime PM
> callbacks. Since the bus driver holds the actual reference
> to the clock, we have to call down into the bus callbacks
> from the platform runtime PM implementation since our
> implementation takes precedence. I guess this is expected
> way of doing it?

I'm not sure actually. We should sit down together and discuss this
over an afternoon - hopefully together with Rafael. I could probably
find time during next week in Orlando if you're around.

> I see three possible paradigms here:
>
> (A) dev_power_domain implementation in
>  platform/board code handle silicon clocks and regulators.
>
> (B) Bus (as AMBA) driver handle silicon clocks and
>  regulators
>
> (C) Devices handle clocks and regulators in their runtime
>  PM callbacks themselves
>
> So now ARM SHmobile does (A), PrimeCell drivers do
> (B) and this patch attempted to do (C).
>
> And our way of handling AMBA is a mixture calling
> the bus callbacks from the platform like (A)+(B)
>
> I guess (A) and (C) won't mix very well since both
> handle platform devices and things will screw up if
> say our platform and SHmobile would use the same
> driver.

Exactly my point.

Thanks,

/ magnus

Re: [PATCH 2/2] i2c/nomadik: runtime PM support

[Linus Walleij]

On Mon, Oct 24, 2011 at 4:32 PM, Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Oct 21, 2011 at 8:19 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> Currently we have something similar for the AMBA
>> bus here, since it actually has code in place to grab
>> the block clock on probe() and its own runtime PM
>> callbacks. Since the bus driver holds the actual reference
>> to the clock, we have to call down into the bus callbacks
>> from the platform runtime PM implementation since our
>> implementation takes precedence. I guess this is expected
>> way of doing it?
>
> I'm not sure actually. We should sit down together and discuss this
> over an afternoon - hopefully together with Rafael. I could probably
> find time during next week in Orlando if you're around.

OK sounds like a plan, I'll bring our code and we'll atleast try
to understand each others' thinking!

Med vänlig hälsning
Linus Walleij