Re: [PATCH v1 3/3] i2c: tegra: Fix suspending in active runtime PM state

[Dmitry Osipenko]

13.12.2019 21:01, Dmitry Osipenko ÐÐÑÐÑ:
> 13.12.2019 17:04, Dmitry Osipenko ÐÐÑÐÑ:
>> 13.12.2019 16:47, Thierry Reding ÐÐÑÐÑ:
>>> On Fri, Dec 13, 2019 at 02:34:28AM +0300, Dmitry Osipenko wrote:
>>>> I noticed that sometime I2C clock is kept enabled during suspend-resume.
>>>> This happens because runtime PM defers dynamic suspension and thus it may
>>>> happen that runtime PM is in active state when system enters into suspend.
>>>> In particular I2C controller that is used for CPU's DVFS is often kept ON
>>>> during suspend because CPU's voltage scaling happens quite often.
>>>>
>>>> Note: we marked runtime PM as IRQ-safe during the driver's probe in the
>>>> "Support atomic transfers" patch, thus it's okay to enforce runtime PM
>>>> suspend/resume in the NOIRQ phase which is used for the system-level
>>>> suspend/resume of the driver.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>> ---
>>>>  drivers/i2c/busses/i2c-tegra.c | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>
>>> I've recently discussed this with Rafael in the context of runtime PM
>>> support in the Tegra DRM driver and my understanding is that you're not
>>> supposed to force runtime PM suspension like this.
>>>
>>> I had meant to send out an alternative patch to fix this, which I've
>>> done now:
>>>
>>> 	http://patchwork.ozlabs.org/patch/1209148/
>>>
>>> That's more in line with what Rafael and I had discussed in the other
>>> thread and should address the issue that you're seeing as well.
>>
>> Well, either me or you are still having some misunderstanding of the
>> runtime PM :) To my knowledge there are a lot of drivers that enforce
>> suspension of the runtime PM during system's suspend, it should be a
>> right thing to do especially in a context of the Tegra I2C driver
>> because we're using asynchronous pm_runtime_put() and thus at the time
>> of system's suspending, the runtime PM could be ON (as I wrote in the
>> commit message) and then Terga's I2C driver manually disables the clock
>> on resume (woopsie).
> 
> Actually, looks like it's not the asynchronous pm_runtime_put() is the
> cause of suspending in active state. I see that only one of three I2C
> controllers is suspended in the enabled state, maybe some child (I2C
> client) device keeps it awake, will try to find out.
> 
>> By invoking pm_runtime_force_suspend() on systems's suspend, the runtime
>> PM executes tegra_i2c_runtime_suspend() if device is in active state. On
>> system resume, pm_runtime_force_resume() either keeps device in a
>> suspended state or resumes it, say if for userspace disabled the runtime
>> PM for the I2C controller.
>>
>> Rafael, could you please clarify whether my patch is doing a wrong thing?

[snip]

I'm now thinking that it will be not very worthwhile to spend time on
trying to understand why runtime PM isn't working as expected here. It
will be better to simply remove runtime PM from the I2C driver because
it is used only for clock-gating/pinmuxing and it is a very light
operation in comparison to I2C transfer performance. Thus it should be
better to avoid the runtime PM overhead by enabling/disabling the I2C
clocks before/after the transfer, I think that's what driver did before
the runtime PM addition.

Thierry / Jon / Mikko, any objections?