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

[Tomasz Figa]

On 21.06.2014 01:53, Doug Anderson wrote:
> Kevin,
> 
> On Fri, Jun 20, 2014 at 4:13 PM, Kevin Hilman <khilman at linaro.org> wrote:
>> Doug Anderson <dianders at chromium.org> writes:
>>
>>> Kevin,
>>>
>>> On Fri, Jun 20, 2014 at 2:48 PM, Kevin Hilman <khilman at linaro.org> wrote:
>>>> Hi Doug,
>>>>
>>>> Doug Anderson <dianders at chromium.org> writes:
>>>>
>>>>> On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman <khilman at linaro.org> wrote:
>>>>>> Doug Anderson <dianders at 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.
> 
> 
> -Doug
> 
> 
> P.S. Just to confirm: you're not opposed to landing the noirq code
> as-is.  This discussion is about how someone could make it better in
> the future...
> 

I'm not sure noirq is going to work correctly, at least not with current
callbacks. I can see a call to clk_prepare_enable() there which needs to
acquire a mutex.

Would disabling the irq in suspend callback of your wakeup device and
reenabling it in resume, while keeping their wakeup capability enabled,
work for you? Then you could resume i2c in .resume_early callback
without any issues.

Best regards,
Tomasz