Re: [PATCH] i2c: exynos5: Properly use the "noirq" variants of suspend/resume

[Kevin Hilman <khilman@linaro.org>]

Doug Anderson <dianders@chromium.org> writes:

> Kevin,
>
> On Fri, Jun 20, 2014 at 4:13 PM, Kevin Hilman <khilman@linaro.org> wrote:
>> Doug Anderson <dianders@chromium.org> writes:
>>
>>> Kevin,
>>>
>>> On Fri, Jun 20, 2014 at 2:48 PM, Kevin Hilman <khilman@linaro.org> wrote:
>>>> Hi Doug,
>>>>
>>>> Doug Anderson <dianders@chromium.org> writes:
>>>>
>>>>> On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman <khilman@linaro.org> wrote:
>>>>>> Doug Anderson <dianders@chromium.org> writes:
>>>>>>
>>>>>>> The original code for the exynos i2c controller registered for the
>>>>>>> "noirq" variants.  However during review feedback it was moved to
>>>>>>> SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no
>>>>>>> longer actually "noirq" (despite functions named
>>>>>>> exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq).
>>>>>>>
>>>>>>> i2c controllers that might have wakeup sources on them seem to need to
>>>>>>> resume at noirq time so that the individual drivers can actually read
>>>>>>> the i2c bus to handle their wakeup.
>>>>>>
>>>>>> I suspect usage of the noirq variants pre-dates the existence of the
>>>>>> late/early callbacks in the PM core, but based on the description above,
>>>>>> I suspect what you actually want is the late/early callbacks.
>>>>>
>>>>> I think it actually really needs noirq.  ;)
>>>>
>>>> Yes, it appears it does.   Objection withdrawn.
>>>>
>>>> I just wanted to be sure because since the introduction of late/early,
>>>> the need for noirq should be pretty rare, but there certainly are needs.
>>>>
>>>> <tangent>
>>>> In this case though, the need for it has more to do with the
>>>> lack of a way for us to describe non parent-child device dependencies
>>>> than whether or not IRQs are enabled or not.
>>>> </tangent>
>>>
>>> Actually, I'm not sure that's true, but I'll talk through it and you
>>> can point to where I'm wrong (I often am!)
>>>
>>> If you're a wakeup device then you need to be ready to handle
>>> interrupts as soon as the "noirq" phase of resume is done, right?
>>
>> As soon as the noirq phase of your own driver is done, correct.
>>
>>> Said another way: you need to be ready to handle interrupts _before_
>>> the normal resume code is called and be ready to handle interrupts
>>> even _before_ the early resume code is called.
>>
>> Correct.
>>
>>> That means if you are implementing a bus that's needed by any devices
>>> with wakeup interrupts then it's your responsibility to also be
>>> prepared to run this early.
>>>
>>> In this particular case the max77686 driver doesn't need to do
>>> anything at all to be ready to handle interrupts.  It's suspend and
>>> resume code is just boilerplate "enable wakeups / disable wakeups" and
>>> it has no "noirq" code.  The max77686 driver doesn't have any "noirq"
>>> wake call because it would just be empty.
>>>
>>> Said another way: the problem isn't that the max77686 wakeup gets
>>> called before the i2c wakeup.  The problem is that i2c is needed ASAP
>>> once IRQs are enabled and thus needs to be run noirq.
>>>
>>> Does that sound semi-correct?
>>
>> Yes that's correct.
>>
>> My point above was (trying to be) that ultimately this is an ordering
>> issue.  e.g. the bus device needs to be "ready" before wakeup devices on
>> that bus can handle wakeup interrupts etc.  The way we're handling that
>> ordering is by the implied ordering of noirq, late/early and "normal"
>> callbacks.  That's convenient, but not exactly obvious.
>>
>> It works because we dont' typically need too many layers here, but it
>> would be much more understandable if we could describe this kind of
>> dependency in a way that the suspend/resume code would suspend/resume
>> things in the right order rather than by tinkering with callback levels
>> (since otherwise suspend/resume ordering just depends on probe order.)
>>
>> This issue then usually gets me headed down my usual rant path about how
>> I think runtime PM is much better suited for handling ordering and
>> dependencies becuase it automatically handles parent/child dependencies
>> and non parent/child dependencies can be handled by taking advantage of
>> the get/put APIs which are refcounted, ect etc. but that's another can
>> worms.
>
> Ah, I gotcha.  Yes, I'm a fan of having explicit dependency orderings too.
>
> So I guess in this case the truly correct way to handle it is:
>
> 1. i2c controller should have Runtime PM even though (as per the code
> now) there's nothing you can do to it to save power under normal
> circumstances.  So the runtime "suspend" code would be a no-op.
>
> 2. When the i2c controller is told to runtime resume, it should
> double-check if a full SoC poweroff has happened since the last time
> it checked.  In this case it should reinit its hardware.
>
> 3. If the i2c controller gets a full "resume" callback then it should
> also reinit the hardware just so it's not sitting in a half-configured
> state until the first peripheral uses it.
>
> If later someone finds a way to power gate the i2c controller when no
> active transfers are going (and we actually save non-trivial power
> doing this) then we've got a nice place to put that code.
>
> NOTE: Unless we can actually save power by power gating the i2c
> peripheral when there are no active transfers, we would also just have
> the i2c_xfer() init the hardware if needed.  Maybe that's kinda gross,
> though.

Yes, this is how we manage the i2c controller on OMAP.

Essentially, between every xfer, the hw is disabled and can potentially
lose context, so eveery xfer requires a hw init.  We use the runtime PM
"autosuspend" feature so that it stays alive for X milliseconds so
bursty i2c xfers are not punished.   

Have a look at drivers/i2c/busses/i2c-omap.c.

You'll notice there are not callbacks for system suspend/resume, it's
only doing runtime PM.

Kevin