On 26 June 2017 at 21:39, Grygorii Strashko <grygorii.strashko@xxxxxx> wrote:
>
>
> On 06/26/2017 11:49 AM, Ulf Hansson wrote:
>> On 23 June 2017 at 00:01, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>>> On Thu, Jun 22, 2017 at 11:37 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>>>> On 22 June 2017 at 16:41, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>>>>> On Thursday, June 22, 2017 01:49:33 PM Mika Westerberg wrote:
>>>>>> On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
>>>>>>> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>>>>>>>> The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
>>>>>>>> during system suspend"), may suggest to the PM core to try out the so
>>>>>>>> called direct_complete path for system sleep. In this path, the PM core
>>>>>>>> treats a runtime suspended device as it's already in a proper low power
>>>>>>>> state for system sleep, which makes it skip calling the system sleep
>>>>>>>> callbacks for the device, except for the ->prepare() and the ->complete()
>>>>>>>> callback.
>>>>>>>>
>>>>>>>> Moreover, under certain circumstances the PM core may unset the
>>>>>>>> direct_complete flag for a parent device, in case its child device are
>>>>>>>> being system suspended before. In other words, the PM core doesn't skip
>>>>>>>> calling the system sleep callbacks, no matter if the device is runtime
>>>>>>>> suspended or not.
>>>>>>>>
>>>>>>>> In cases of an i2c slave device, the above situation is triggered.
>>>>>>>> Unfortunate, this also breaks the assumption that the i2c device is always
>>>>>>>> runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
>>>>>>>> invoked, which then leads to a regression.
>>>>>>>>
>>>>>>>> More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
>>>>>>>> clk_core_unprepare(), for an already disabled/unprepared clock, leading to
>>>>>>>> complaints about clocks calls being wrongly balanced.
>>>>>>>>
>>>>>>>> In cases when the i2c device is attached to the ACPI PM domain, the problem
>>>>>>>> doesn't occur. That's because ACPI's ->suspend() callback, assigned to
>>>>>>>> acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>>>>>>>
>>>>>>> Which really is expected to happen, so direct_complete should only be
>>>>>>> used along with the ACPI PM domain in this case.
>>>>>>>
>>>>>>> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
>>>>>>> to do the right thing without dw_i2c_plat_prepare() and the return
>>>>>>> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
>>>>>>> will only have effect without ACPI PM domain AFAICS.
>>>>>>>
>>>>>>> It looks like commit 8503ff166504 is entirely misguided.
>>>>>>
>>>>>> Indeed it is. At the time I suggested that change I did not really
>>>>>> understand how the direct complete is supposed to be used :-/
>>>>>
>>>>> So can we go for a full revert, please, and then fix up things properly?
>>>>
>>>> Unfortunate I think a revert is going to make it worse, for the non ACPI case.
>>>>
>>>> Because in that case, unless I am reading the code wrong, I think when
>>>> the device is runtime suspended during system sleep, then the system
>>>> suspend callback will also be called (because the direct_complete
>>>> isn't run), again causing clock unbalance issues.
>>>>
>>>> This makes it worse in that sense, that then you don't even need an
>>>> i2c slave to trigger the problem.
>>>
>>> In that case your changelog is a bit misleading.
>>
>> Yeah, it is! I didn't realize that until I fully investigated the revert option.
>>
>>>
>>> It looks like the commit in question attempted to fix exactly this
>>> issue, but it failed, so it should be replaced with something else
>>> which is what your patch is effectively doing.
>>>
>>> IMO you should describe the original problem, explain why that commit
>>> is not sufficient to fix it and then describe the final fix.
>>>
>>> Anyway, after reading the changelog it should be clear that things
>>> were broken before the commit in question.
>>>
>>> And BTW I'm not really sure how the rest of the series is related to this?
>>
>> Going back in history, I realize the system sleep support in this
>> driver has been broken even before the commit $subject patch intends
>> to fix.
>> However it has been working fine for the ACPI case, because of how the
>> ACPI PM domain manages its devices during system sleep.
>>
>> The commit in question, adds an improvement to the driver, because it
>> enables the direct_complete path. For ACPI, that was already working,
>> but not for the other cases. So to be able to support the similar
>> improvement as the direct_complete path offers, as that isn't working
>> for this driver, I tried out using the runtime PM centric path
>> instead. That is what the rest of the changes in this series takes
>> care of.
>>
>> Now, as the system sleep support is broken I wanted to make a simple
>> fix for that first, via $subject patch. I guess what makes this a bit
>> confusing is that I shouldn't point to a certain commit, but rather
>> just add a stable tag and update the changelog accordingly.
>>
>
> Wouldn't it fix suspend for this driver if you will just replace
> dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as
> you've done in patch 9?
For the non-ACPI case - yes.
For the ACPI case - no. Here we need to adapt the ACPI PM domain
first, as it currently doesn't support the runtime PM centric path for
system sleep.
>
> And, I think, direct_complete path should still work after this also.
I don't understand why we want or need that?
To me it would only make the code in the ACPI PM domain more
complicated, comparing to the changes I have posted in rest of this
series.
Kind regards
Uffe
