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?
And, I think, direct_complete path should still work after this also.
--
regards,
-grygorii
