Re: [PATCH] i2c: nomadik: move runtime suspend of hw to _noirq

Re: [PATCH] i2c: nomadik: move runtime suspend of hw to _noirq

[Ulf Hansson]

+ linux-pm, Rafael, Kevin

On 23 May 2016 at 14:59, Linus Walleij <linus.walleij@linaro.org> wrote:
> The runtime suspend of the hardware (declocking and pin control
> resting) needs to happen in the *_noirq callbacks after all
> hardware interrupts are disabled and we know there will be no
> more I2C traffic in the system.

As you need this change, it means some other device requires the I2C
device to be runtime resumed, while a *_late callback tries to suspend
the other device. Right!?

Can you tell which these devices are?

Moving the suspend operations to the *_noirq callbacks could improve
the situation, but unfortunate it's comes with a limitation.
That's because runtime PM has been disabled (by genpd or the PM core)
before the *_late callbacks is invoked, which means the I2C device
needs to be runtime resumed in an earlier phase to make it
operational.

This is indeed a generic problem, as we don't have a way to describe
these kind of dependences between devices. In other words, we need to
control the order of how devices becomes suspended/resumed. If that
could be done, we could likely avoid runtime resuming devices
unnecessarily.

Kind regards
Uffe

>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/i2c/busses/i2c-nomadik.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index bcd17e8cbcb4..c96a3e331bed 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -877,7 +877,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
>  }
>
>  #ifdef CONFIG_PM_SLEEP
> -static int nmk_i2c_suspend_late(struct device *dev)
> +static int nmk_i2c_suspend_noirq(struct device *dev)
>  {
>         int ret;
>
> @@ -889,7 +889,7 @@ static int nmk_i2c_suspend_late(struct device *dev)
>         return 0;
>  }
>
> -static int nmk_i2c_resume_early(struct device *dev)
> +static int nmk_i2c_resume_noirq(struct device *dev)
>  {
>         return pm_runtime_force_resume(dev);
>  }
> @@ -931,7 +931,8 @@ static int nmk_i2c_runtime_resume(struct device *dev)
>  #endif
>
>  static const struct dev_pm_ops nmk_i2c_pm = {
> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(nmk_i2c_suspend_late, nmk_i2c_resume_early)
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(nmk_i2c_suspend_noirq,
> +                                     nmk_i2c_resume_noirq)
>         SET_RUNTIME_PM_OPS(nmk_i2c_runtime_suspend,
>                         nmk_i2c_runtime_resume,
>                         NULL)
> --
> 2.4.11
>

Re: [PATCH] i2c: nomadik: move runtime suspend of hw to _noirq

[Linus Walleij]

On Wed, May 25, 2016 at 12:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 23 May 2016 at 14:59, Linus Walleij <linus.walleij@linaro.org> wrote:
>> The runtime suspend of the hardware (declocking and pin control
>> resting) needs to happen in the *_noirq callbacks after all
>> hardware interrupts are disabled and we know there will be no
>> more I2C traffic in the system.
>
> As you need this change, it means some other device requires the I2C
> device to be runtime resumed, while a *_late callback tries to suspend
> the other device. Right!?

Yep.

> Can you tell which these devices are?

Those I have seen colliding in suspend/resume cycles:
drivers/leds/leds-lp5521.c
drivers/iio/light/bh1780.c
(note that I have fixes pending for the bh1780 driver)

> Moving the suspend operations to the *_noirq callbacks could improve
> the situation, but unfortunate it's comes with a limitation.
> That's because runtime PM has been disabled (by genpd or the PM core)
> before the *_late callbacks is invoked, which means the I2C device
> needs to be runtime resumed in an earlier phase to make it
> operational.

Hm uncertain what you mean here, but happy to test.
The macro SET_NOIRQ_SYSTEM_SLEEP_PM_OPS makes
you think the hooks should be paired up and has this nice
symmetry, do you mean I should not do this but keep
the resume hook where SET_LATE_SYSTEM_SLEEP_PM_OPS()
puts it?

> This is indeed a generic problem, as we don't have a way to describe
> these kind of dependences between devices. In other words, we need to
> control the order of how devices becomes suspended/resumed. If that
> could be done, we could likely avoid runtime resuming devices
> unnecessarily.

Correct, keep me in the loop and I can test some patches.

Yours,
Linus Walleij

Re: [PATCH] i2c: nomadik: move runtime suspend of hw to _noirq

[Wolfram Sang]

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


> > This is indeed a generic problem, as we don't have a way to describe
> > these kind of dependences between devices. In other words, we need to
> > control the order of how devices becomes suspended/resumed. If that
> > could be done, we could likely avoid runtime resuming devices
> > unnecessarily.
> 
> Correct, keep me in the loop and I can test some patches.

So, this patch is discarded in favor of a generic solution?


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

Re: [PATCH] i2c: nomadik: move runtime suspend of hw to _noirq

[Wolfram Sang]

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

On Sun, Jun 19, 2016 at 07:12:21PM +0200, Wolfram Sang wrote:
> 
> > > This is indeed a generic problem, as we don't have a way to describe
> > > these kind of dependences between devices. In other words, we need to
> > > control the order of how devices becomes suspended/resumed. If that
> > > could be done, we could likely avoid runtime resuming devices
> > > unnecessarily.
> > 
> > Correct, keep me in the loop and I can test some patches.
> 
> So, this patch is discarded in favor of a generic solution?

Yes?


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

Re: [PATCH] i2c: nomadik: move runtime suspend of hw to _noirq

[Linus Walleij]

On Thu, Jul 14, 2016 at 3:40 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Sun, Jun 19, 2016 at 07:12:21PM +0200, Wolfram Sang wrote:
>>
>> > > This is indeed a generic problem, as we don't have a way to describe
>> > > these kind of dependences between devices. In other words, we need to
>> > > control the order of how devices becomes suspended/resumed. If that
>> > > could be done, we could likely avoid runtime resuming devices
>> > > unnecessarily.
>> >
>> > Correct, keep me in the loop and I can test some patches.
>>
>> So, this patch is discarded in favor of a generic solution?
>
> Yes?

I'm reading Ulf's comment here again:

> Moving the suspend operations to the *_noirq callbacks could improve
> the situation, but unfortunate it's comes with a limitation.
> That's because runtime PM has been disabled (by genpd or the PM core)
> before the *_late callbacks is invoked, which means the I2C device
> needs to be runtime resumed in an earlier phase to make it
> operational.

And I mean it means that in the .resume hook, i.e. this:

static int nmk_i2c_resume_noirq(struct device *dev)
{
        return pm_runtime_force_resume(dev);
}

The force call will have no effect in *_noirq() because the runtime
PM callbacks are turned off. The device will not be runtime resumed
by the _force_ call as it should, it will still be runtime suspended
when the device comes back up, and that makes it behave badly
unless it goes through another suspend/resume cycle befor it is
actually used. However we are saved by the fact that the driver
does not use autosuspend, so what happens after the force resume
is that the driver will just be runtime suspended again at late
since there are no users.

It will instead be properly resumed when the next I2C transaction
comes in.

It looks like this if I put prints in the runtime suspend/resume
calls:

[   28.863128] PM: early resume of devices complete after 0.854 msecs
[   28.866302] nmk-i2c 80128000.i2c: nmk_i2c_runtime_resume
[   29.132904] PM: resume of devices complete after 269.744 msecs
[   29.363861] Restarting tasks ...
[   29.373016] nmk-i2c 80128000.i2c: nmk_i2c_runtime_suspend
[   29.384124] done.

Then comes some real traffic (LED heartbeat):
[   29.391113] nmk-i2c 80128000.i2c: nmk_i2c_runtime_resume
[   29.396972] nmk-i2c 80128000.i2c: nmk_i2c_runtime_suspend
(...)

So I think the patch is fine.

Yours,
Linus Walleij