On 06/28/2017 09:31 AM, Ulf Hansson wrote:
> 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.
Yeah.
>
>>
>> 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.
>
I'm not sure about benefits with this particular driver - everything
depend on power on/off latencies, if they are big direct_complete make sense.
The main point of my comment was based on fact that dw_i2c driver uses
the same function for PM runtime and System suspend callbacks, but this is
definitely wrong (ACPI or not), because PM runtime and System suspend
are not synchronized.
--
regards,
-grygorii
