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

[Linus Walleij <linus.walleij@linaro.org>]

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