Re: [PATCH] i2c: rk3x: keep i2c irq ON in suspend

[Grygorii Strashko]

On 12/06/2016 09:37 PM, David.Wu wrote:
> Hi Doug,
> 
> å 2016/12/7 0:31, Doug Anderson åé:
>> Hi,
>>
>> On Tue, Dec 6, 2016 at 12:12 AM, David.Wu <david.wu@xxxxxxxxxxxxxx>
>> wrote:
>>> Hi Heiko,
>>>
>>> å 2016/12/5 18:54, Heiko Stuebner åé:
>>>>
>>>> Hi David,
>>>>
>>>> Am Montag, 5. Dezember 2016, 16:02:59 CET schrieb David Wu:
>>>>>
>>>>> During suspend there may still be some i2c access happening.
>>>>> And if we don't keep i2c irq ON, there may be i2c access timeout if
>>>>> i2c is in irq mode of operation.
>>>>
>>>>
>>>> can you describe the issue you're trying to fix a bit more please?
>>>
>>>
>>> Sometimes we could see the i2c timeout errors during suspend/resume,
>>> which
>>> makes the duration of suspend/resume too longer.
>>>
>>> [  484.171541] CPU4: Booted secondary processor [410fd082]
>>> [  485.172777] rk3x-i2c ff3c0000.i2c: timeout, ipd: 0x10, state: 1
>>> [  486.172760] rk3x-i2c ff3c0000.i2c: timeout, ipd: 0x10, state: 1
>>> [  487.172759] rk3x-i2c ff3c0000.i2c: timeout, ipd: 0x10, state: 1
>>> [  487.172840] cpu cpu4: _set_opp_voltage: failed to set voltage (800000
>>> 800000 800000 mV): -110
>>> [  487.172874] cpu cpu4: failed to set volt 800000
>>>
>>>>
>>>> I.e. I'd think the i2c-core does suspend i2c-client devices first,
>>>> so that
>>>> these should be able to finish up their ongoing transfers and not start
>>>> any
>>>> new ones instead?
>>>>
>>>> Your irq can still happen slightly after the system started going to
>>>> actually
>>>> sleep, so to me it looks like you just widened the window where irqs
>>>> can
>>>> be
>>>> handled. Especially as your irq could also just simply stem from the
>>>> start
>>>> state, so you cannot even be sure if your transaction actually is
>>>> finished.
>>>
>>>
>>> Okay, you are right. I want to give it a double insurance at first,
>>> but it
>>> may hide the unhappend issue.
>>>
>>>>
>>>> So to me it looks like the i2c-connected device driver should be fixed
>>>> instead?
>>>
>>>
>>> I tell them to fix it in rk808 driver.
>>
>> To me it seems like perhaps cpufreq should not be changing frequencies
>> until it is resumed properly.  Presumably if all the ordering is done
>> right then cpufreq should be resumed _after_ the i2c regulator so you
>> should be OK.  ...or am I somehow confused about that?
> 
> yesïthe cpufreq and regulator should start i2c job after they resume
> properly.
> 
>>
>> Also note that previous i2c busses I worked with simply returned -EIO
>> in the case where they were called when suspended.  See
>> "i2c-exynos5.c" and "i2c-s3c2410.c".
> 
> In "i2c-exynos5.c", it seems that using the "i2c->suspended" to protect
> i2c transfer works most of the time. Of course it could prevent the next
> new i2c transfer to start. But in one case, if the current i2c job was
> not finished until the i2c irq was disabled by system suspend, the i2c
> timeout error would also happen, as the current i2c job may have a large
> data to transfer and it lasts from a long time.

And this means you have bug in some of I2C client drivers which do not stop
their activities during suspend properly (most usual case - driver uses work
and this work still active during suspend and can run on one CPU while suspend
runs on another).

At the moment .suspend_noirq() callback is called there should be no active
I2C transactions in general.

> 
> So is it necessary to add a mutex lock to wait the current job to be
> finished before the "i2c->suspended" is changed in i2c_suspend_noirq()?
> 

You need to catch and fix all driver who will try to access I2C after your
I2C bus driver passes suspend_noirq stage. Smth, like [1], uses i2c_lock_adapter().
 

[1] https://git.ti.com/android-sdk/kernel-omap/commit/125ef8f7016e7b205886f93862288a45a312b1d8

-- 
regards,
-grygorii