[PATCH RESEND] i2c/nomadik: runtime PM support

[PATCH RESEND] 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>
Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@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>
---
So I'm resending this, after discussion with Mark Brown and others
I can only conclude that the question of whether to handle
clocks and/or regulators centrally in say
drivers/base/power/clock_ops.c or distributed in drivers is not
simple to determine.

When I discussed the matter with Mark I think we concluded that
the approach for his systems will be to add calls in the drivers
to begin with, then consolidate to the system-wide handlers
when/if it makes sense.

The key reason is that we have hardware blocks with different
characteristics on the order to do things, whereas in some other
systems (like shmobile I guess) every IP block shall be twisted
the same way. We have a mixed legacy of ARM PrimeCells and IP
blocks from different corners of the world and just cannot do
it in one single way. ARM PrimeCells already have some
centralized runtime PM callbacks.
---
 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 5267ab9..3c9803d 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.8

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

[Mark Brown]

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

On Thu, Mar 08, 2012 at 02:30:07PM +0100, Linus Walleij wrote:

> The key reason is that we have hardware blocks with different
> characteristics on the order to do things, whereas in some other
> systems (like shmobile I guess) every IP block shall be twisted
> the same way. We have a mixed legacy of ARM PrimeCells and IP
> blocks from different corners of the world and just cannot do
> it in one single way. ARM PrimeCells already have some
> centralized runtime PM callbacks.

Yeah, the other issue on some systems with long histories is that if
you've got many generations of SoC it can be easier to roll updates out
over drivers than get all the SoCs updated to the latest infrastructure
along with supporting new ones.

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

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

[Linus Walleij]

On Thu, Mar 8, 2012 at 2:30 PM, 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>
> Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@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>

Broonie, are you OK with merging this patch now?

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, Mar 19, 2012 at 12:35 PM, Linus Walleij
<linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Mar 8, 2012 at 2:30 PM, 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>
>> Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@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>
>
> Broonie, are you OK with merging this patch now?

Sorry malfunction in my parser. Should be DOOKS of course...
Broonie already agreed on the approach I think.

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, Mar 19, 2012 at 12:35 PM, Linus Walleij
<linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Mar 8, 2012 at 2:30 PM, 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>
>> Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@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>
>
> Dooks, are you OK with merging this patch now?

Ping on this...

Yours,
Linus Walleij

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

[Wolfram Sang]

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


> >> 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>
> >> Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@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>
> >
> > Dooks, are you OK with merging this patch now?
> 

As I understood, Mark acked it? I didn't dive into the details, so it would be
nice for me to have an ACK (or at least a not NACK) from Magnus.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

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

[Mark Brown]

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

On Tue, Apr 17, 2012 at 04:39:01PM +0200, Wolfram Sang wrote:

> > > Dooks, are you OK with merging this patch now?

> As I understood, Mark acked it? I didn't dive into the details, so it would be
> nice for me to have an ACK (or at least a not NACK) from Magnus.

I probably ignored it - I try to minimise my involvement with I2C on
account of the general problems with getting things merged.

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

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

[Magnus Damm]

On Tue, Apr 17, 2012 at 11:39 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@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>
>> >> Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@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>
>> >
>> > Dooks, are you OK with merging this patch now?
>>
>
> As I understood, Mark acked it? I didn't dive into the details, so it would be
> nice for me to have an ACK (or at least a not NACK) from Magnus.

I'm a bit hesitant to ack because the runtime suspend/resume callbacks
are used differently than we would do on arch/sh and
arch/arm/mach-shmobile. This difference may or may not be a good
thing. I'm afraid I know too little about the nomadik platform to say
anything wise. =)

Our drivers assume that the ->runtime_suspend() and ->runtime_resume()
callbacks are executed before/after the power is turned off/on for a
certain power domain. The way they are used in this patch more seems
like they are expected to be called regardless of the state of the
rest of the devices sharing the underlying power domain. You probably
want to control the clocks and the regulators directly - independently
of the rest of the devices.You may want to look into struct
gpd_dev_ops for various ways to interface to the pm domain code.

Exactly what is a good fit depends on how the underlying SoC code maps
the runtime pm clock and domain code to the actual hardware.

Cheers,

/ magnus

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

[Wolfram Sang]

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


> I'm a bit hesitant to ack because the runtime suspend/resume callbacks
> are used differently than we would do on arch/sh and
> arch/arm/mach-shmobile. This difference may or may not be a good
> thing. I'm afraid I know too little about the nomadik platform to say
> anything wise. =)

I take this as a not-NACK ;) I don't want to dive too much into the
nomadik power management as well, so I will trust the authors here.
Since the driver is only used on this platform, I guess a different PM
strategy can be fix later if needed.

Linus, can you resend the patch another time, please?

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

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

[Linus Walleij]

On Thu, Apr 19, 2012 at 1:16 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:

> I take this as a not-NACK ;) I don't want to dive too much into the
> nomadik power management as well, so I will trust the authors here.
> Since the driver is only used on this platform, I guess a different PM
> strategy can be fix later if needed.
>
> Linus, can you resend the patch another time, please?

I think I have second thoughts about this now, after talking to
some smart people. The base problem is that the I2C "regulator"
in this driver is a voltage domain and this should be handled by
power domains instead. (Some pain about that recently...)

So now I think we should remove the regulator handling
altogether, keep clk handling (which is currently distributed) and
respin on top of that.

Yours,
Linus Walleij

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

[Linus Walleij]

On Wed, Apr 18, 2012 at 8:39 AM, Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I'm a bit hesitant to ack because the runtime suspend/resume callbacks
> are used differently than we would do on arch/sh and
> arch/arm/mach-shmobile. This difference may or may not be a good
> thing. I'm afraid I know too little about the nomadik platform to say
> anything wise. =)
>
> Our drivers assume that the ->runtime_suspend() and ->runtime_resume()
> callbacks are executed before/after the power is turned off/on for a
> certain power domain. The way they are used in this patch more seems
> like they are expected to be called regardless of the state of the
> rest of the devices sharing the underlying power domain.

After discussing this internally a bit I think this is the central point here,
and yes that is indeed how it is written.

We are not using runtime_[suspend|resume]() like that but want
this to be handled on each and every driver in isolation.

So our handler kicks in at the pm_domain level, then calls the
driver hook from the domain by a simple
pm_generic_runtime_suspend(dev) *every* time, not just
if the power domain was switched on/off.

The reason is mainly that we want register save/restore to be done
as soon as a driver needs it, not before/after its power domain is cut
as mentioned here.

Example: the system is sleeping. We need to wake up and do
some sporadic task in one driver, then go back to sleep.

If *all* runtime_resume() hooks for *all* devices in the same power
domain are called at this time on the way up/down we get a major
overhead as our primary power domain is pretty big.

We prefer to be able to control on a per-driver instance how this
is handled. That may include e.g. clock handling as part of the
pm_runtime hooks, but not necessarily, register save/restore is
the big bulk of work.

Another thing that sort of mandates this approach is that we
have drivers that are used on multiple systems beside ux500.
E.g the UART PL011 is used on a plethora of systems.

Some of these may use the bus hooks (or type, class...)
calling into the drivers runtime_pm() hooks, if we start to
encode semantics into the driver regarding how these hooks
get called or assume they are always called from a power
domain we're inviting disaster I'm afraid :-/

On systems where we know the driver is always used with
power domains the world is easier, but this is not the case
here. This piece of code in rumtime PM is the source of the
confusion:

        if (dev->pm_domain)
                callback = dev->pm_domain->ops.runtime_suspend;
        else if (dev->type && dev->type->pm)
                callback = dev->type->pm->runtime_suspend;
        else if (dev->class && dev->class->pm)
                callback = dev->class->pm->runtime_suspend;
        else if (dev->bus && dev->bus->pm)
                callback = dev->bus->pm->runtime_suspend;
        else
                callback = NULL;

        if (!callback && dev->driver && dev->driver->pm)
                callback = dev->driver->pm->runtime_suspend;

So for a particular driver we don't know which one it's
going to be. But we need to assume they all call down
to the runtime_suspend/resume hooks with something like
pm_generic_runtime_suspend(dev) regardless.

For example the bus code for AMBA does this:

static int amba_pm_runtime_suspend(struct device *dev)
{
        struct amba_device *pcdev = to_amba_device(dev);
        int ret = pm_generic_runtime_suspend(dev);

        if (ret == 0 && dev->driver)
                clk_disable(pcdev->pclk);

        return ret;
}

Platform devices does this:

static const struct dev_pm_ops platform_dev_pm_ops = {
        .runtime_suspend = pm_generic_runtime_suspend,
        .runtime_resume = pm_generic_runtime_resume,
        .runtime_idle = pm_generic_runtime_idle,
        USE_PLATFORM_PM_SLEEP_OPS
};

So ... we assume pm_generic_runtime_suspend() is always
called one way or another. How else can we have code that
work with both bus code like this and out power domains?

I see that shmobile does something totally different, but its
drivers are not used by others are they?

> You probably
> want to control the clocks and the regulators directly - independently
> of the rest of the devices.You may want to look into struct
> gpd_dev_ops for various ways to interface to the pm domain code.

In this specific driver we may want to keep the clock control
in the main code path, and move the regulator, which is actually
a power domain switch, to the power domain (genpd) framework.

But that is not the case with all drivers, so while I can device a
way forward in this case it doesn't play well in the generic sense.

Yours,
Linus Walleij

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

[Mark Brown]

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

On Thu, May 03, 2012 at 11:03:12AM +0200, Linus Walleij wrote:
> On Wed, Apr 18, 2012 at 8:39 AM, Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> > Our drivers assume that the ->runtime_suspend() and ->runtime_resume()
> > callbacks are executed before/after the power is turned off/on for a
> > certain power domain. The way they are used in this patch more seems
> > like they are expected to be called regardless of the state of the
> > rest of the devices sharing the underlying power domain.

...

> On systems where we know the driver is always used with
> power domains the world is easier, but this is not the case
> here. This piece of code in rumtime PM is the source of the
> confusion:

One thing that the regulator framework has which might be useful here is
that it can notify drivers when power is actually removed.  This allows
drivers to reduce the amount of work they need to do to bring the device
back up if they know power was retained while they were suspended.

It feels like some of the confusion is over the difference between
quiescing the device when idle and actually removing power from the
device.

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

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

[Rafael J. Wysocki]

On Thursday, May 03, 2012, Linus Walleij wrote:
> On Wed, Apr 18, 2012 at 8:39 AM, Magnus Damm <magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > I'm a bit hesitant to ack because the runtime suspend/resume callbacks
> > are used differently than we would do on arch/sh and
> > arch/arm/mach-shmobile. This difference may or may not be a good
> > thing. I'm afraid I know too little about the nomadik platform to say
> > anything wise. =)
> >
> > Our drivers assume that the ->runtime_suspend() and ->runtime_resume()
> > callbacks are executed before/after the power is turned off/on for a
> > certain power domain. The way they are used in this patch more seems
> > like they are expected to be called regardless of the state of the
> > rest of the devices sharing the underlying power domain.
> 
> After discussing this internally a bit I think this is the central point here,
> and yes that is indeed how it is written.
> 
> We are not using runtime_[suspend|resume]() like that but want
> this to be handled on each and every driver in isolation.
> 
> So our handler kicks in at the pm_domain level, then calls the
> driver hook from the domain by a simple
> pm_generic_runtime_suspend(dev) *every* time, not just
> if the power domain was switched on/off.
> 
> The reason is mainly that we want register save/restore to be done
> as soon as a driver needs it, not before/after its power domain is cut
> as mentioned here.
> 
> Example: the system is sleeping. We need to wake up and do
> some sporadic task in one driver, then go back to sleep.
> 
> If *all* runtime_resume() hooks for *all* devices in the same power
> domain are called at this time on the way up/down we get a major
> overhead as our primary power domain is pretty big.

FWIW, in the generic PM domains framework used on the sh7372 platform we have
a need_restore flag whose meaning is whether or not to call .runtime_suspend()
(or its domain-specific counterpart) when the domain is about to be powered
off and .runtime_resume() when the device is resumed by the runtime PM
framework.  Those two callbacks are meant to save and restore the device's
state, respectively, and there are two more domain-specific callbacks,
.stop() and .start() that (in the case of sh7372) manipiulate the devices'
clocks.

So, if pm_runtime_suspend() is called for a device, we first check if it's OK
to call .stop() for it (according to PM QoS constraints), but we don't call
.runtime_suspend() for it yet at this point.  Next, we check if it's OK to
power off the domain containing the device (taking PM QoS constraints and
subdomains into consideration) and if we decide to do that, we call
.runtime_suspend() for the devices in the domain, but only those whose
need_restore flags are unset.  Then, if pm_runtime_resume() is called for
one of the devices, we check its need_restore flag and call .runtime_resume()
for the device if the flag set and .start() is called subsequently.

> We prefer to be able to control on a per-driver instance how this
> is handled. That may include e.g. clock handling as part of the
> pm_runtime hooks, but not necessarily, register save/restore is
> the big bulk of work.
> 
> Another thing that sort of mandates this approach is that we
> have drivers that are used on multiple systems beside ux500.
> E.g the UART PL011 is used on a plethora of systems.
> 
> Some of these may use the bus hooks (or type, class...)
> calling into the drivers runtime_pm() hooks, if we start to
> encode semantics into the driver regarding how these hooks
> get called or assume they are always called from a power
> domain we're inviting disaster I'm afraid :-/

The generic PM domains framework allows you to use different callbacks
with domains in general (ie. it is possible to use two different sets of
PM callbacks depending on whether the device is in a domain or not).

There may be a couple of things still missing depending on the use case, but
the general support is there.

> On systems where we know the driver is always used with
> power domains the world is easier, but this is not the case
> here. This piece of code in rumtime PM is the source of the
> confusion:
> 
>         if (dev->pm_domain)
>                 callback = dev->pm_domain->ops.runtime_suspend;
>         else if (dev->type && dev->type->pm)
>                 callback = dev->type->pm->runtime_suspend;
>         else if (dev->class && dev->class->pm)
>                 callback = dev->class->pm->runtime_suspend;
>         else if (dev->bus && dev->bus->pm)
>                 callback = dev->bus->pm->runtime_suspend;
>         else
>                 callback = NULL;
> 
>         if (!callback && dev->driver && dev->driver->pm)
>                 callback = dev->driver->pm->runtime_suspend;
> 
> So for a particular driver we don't know which one it's
> going to be.

This doesn't seem to be particularly difficult to figure out in advance, though.

The diffucult part is to know what the domain/subsystem will do in addition to
executing your callbacks, but the general idea is that drivers shouldn't need
to know that.  I know that that's kind of not in agreement with reality, however.

> But we need to assume they all call down to the runtime_suspend/resume hooks
> with something like pm_generic_runtime_suspend(dev) regardless.
> 
> For example the bus code for AMBA does this:
> 
> static int amba_pm_runtime_suspend(struct device *dev)
> {
>         struct amba_device *pcdev = to_amba_device(dev);
>         int ret = pm_generic_runtime_suspend(dev);
> 
>         if (ret == 0 && dev->driver)
>                 clk_disable(pcdev->pclk);
> 
>         return ret;
> }
> 
> Platform devices does this:
> 
> static const struct dev_pm_ops platform_dev_pm_ops = {
>         .runtime_suspend = pm_generic_runtime_suspend,
>         .runtime_resume = pm_generic_runtime_resume,
>         .runtime_idle = pm_generic_runtime_idle,
>         USE_PLATFORM_PM_SLEEP_OPS
> };
> 
> So ... we assume pm_generic_runtime_suspend() is always
> called one way or another. How else can we have code that
> work with both bus code like this and out power domains?
> 
> I see that shmobile does something totally different, but its
> drivers are not used by others are they?

Yes, they are, by the sh architecture.

There are a few things missing here still, as I said above, but in the end
a driver should be able to learn whether or not it is used along with a PM
domain (as in the generic PM domains framework) and adjust to that.

> > You probably
> > want to control the clocks and the regulators directly - independently
> > of the rest of the devices.You may want to look into struct
> > gpd_dev_ops for various ways to interface to the pm domain code.
> 
> In this specific driver we may want to keep the clock control
> in the main code path, and move the regulator, which is actually
> a power domain switch, to the power domain (genpd) framework.
> 
> But that is not the case with all drivers, so while I can device a
> way forward in this case it doesn't play well in the generic sense.

I think the number of different use cases is limited, though, so it
should be possible to cover them all somehow. :-)

Thanks,
Rafael

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

[Mark Brown]

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

On Thu, May 03, 2012 at 02:59:25PM +0200, Rafael J. Wysocki wrote:
> On Thursday, May 03, 2012, Linus Walleij wrote:

> > If *all* runtime_resume() hooks for *all* devices in the same power
> > domain are called at this time on the way up/down we get a major
> > overhead as our primary power domain is pretty big.

> FWIW, in the generic PM domains framework used on the sh7372 platform we have
> a need_restore flag whose meaning is whether or not to call .runtime_suspend()
> (or its domain-specific counterpart) when the domain is about to be powered
> off and .runtime_resume() when the device is resumed by the runtime PM
> framework.  Those two callbacks are meant to save and restore the device's
> state, respectively, and there are two more domain-specific callbacks,
> .stop() and .start() that (in the case of sh7372) manipiulate the devices'
> clocks.

> So, if pm_runtime_suspend() is called for a device, we first check if it's OK
> to call .stop() for it (according to PM QoS constraints), but we don't call
> .runtime_suspend() for it yet at this point.  Next, we check if it's OK to
> power off the domain containing the device (taking PM QoS constraints and
> subdomains into consideration) and if we decide to do that, we call
> .runtime_suspend() for the devices in the domain, but only those whose
> need_restore flags are unset.  Then, if pm_runtime_resume() is called for
> one of the devices, we check its need_restore flag and call .runtime_resume()
> for the device if the flag set and .start() is called subsequently.

This seems like a really useful idiom in general - might it be worth
supporting as a standard framework feature?

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

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

[Rafael J. Wysocki]

On Thursday, May 03, 2012, Mark Brown wrote:
> On Thu, May 03, 2012 at 02:59:25PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, May 03, 2012, Linus Walleij wrote:
> 
> > > If *all* runtime_resume() hooks for *all* devices in the same power
> > > domain are called at this time on the way up/down we get a major
> > > overhead as our primary power domain is pretty big.
> 
> > FWIW, in the generic PM domains framework used on the sh7372 platform we have
> > a need_restore flag whose meaning is whether or not to call .runtime_suspend()
> > (or its domain-specific counterpart) when the domain is about to be powered
> > off and .runtime_resume() when the device is resumed by the runtime PM
> > framework.  Those two callbacks are meant to save and restore the device's
> > state, respectively, and there are two more domain-specific callbacks,
> > .stop() and .start() that (in the case of sh7372) manipiulate the devices'
> > clocks.
> 
> > So, if pm_runtime_suspend() is called for a device, we first check if it's OK
> > to call .stop() for it (according to PM QoS constraints), but we don't call
> > .runtime_suspend() for it yet at this point.  Next, we check if it's OK to
> > power off the domain containing the device (taking PM QoS constraints and
> > subdomains into consideration) and if we decide to do that, we call
> > .runtime_suspend() for the devices in the domain, but only those whose
> > need_restore flags are unset.  Then, if pm_runtime_resume() is called for
> > one of the devices, we check its need_restore flag and call .runtime_resume()
> > for the device if the flag set and .start() is called subsequently.
> 
> This seems like a really useful idiom in general - might it be worth
> supporting as a standard framework feature?

The generic PM domains framework does this, it's not platform-specific
(if I understood your question correctly).

Thanks,
Rafael

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

[Mark Brown]

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

On Thu, May 03, 2012 at 09:25:37PM +0200, Rafael J. Wysocki wrote:
> On Thursday, May 03, 2012, Mark Brown wrote:

> > This seems like a really useful idiom in general - might it be worth
> > supporting as a standard framework feature?

> The generic PM domains framework does this, it's not platform-specific
> (if I understood your question correctly).

AIUI it only works if the device gets placed into a doman, though?

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

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

[Rafael J. Wysocki]

On Friday, May 04, 2012, Mark Brown wrote:
> On Thu, May 03, 2012 at 09:25:37PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, May 03, 2012, Mark Brown wrote:
> 
> > > This seems like a really useful idiom in general - might it be worth
> > > supporting as a standard framework feature?
> 
> > The generic PM domains framework does this, it's not platform-specific
> > (if I understood your question correctly).
> 
> AIUI it only works if the device gets placed into a doman, though?

Yes, it does, because otherwise the runtime PM framework avoids calling
.runtime_suspend() or .runtime_resume() twice in a row.

So the flag is only when the callbacks are not always run through the runtime
PM framework, like when domains are involved.

Thanks,
Rafael