Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

On 20 December 2017 at 13:08, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On 20 December 2017 at 10:02, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
>> Hi Ulf,
>>
>> On Wednesday 20 December 2017 02:05 PM, Ulf Hansson wrote:
>>> On 20 December 2017 at 07:42, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
>>>> Hi Ulf,
>>>>
>>>> On Wednesday 20 December 2017 02:52 AM, Ulf Hansson wrote:
>>>>> The runtime PM deployment in the phy core is a bit unnecessary complicated
>>>>> and the main reason is because it operates on the phy device, which is
>>>>> created by the phy core and assigned as a child device of the phy provider
>>>>> device.
>>>>>
>>>>> Let's simplify the code, by replacing the existing calls to
>>>>> phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
>>>>> pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
>>>>> change to give the phy provider device as the parameter to the runtime PM
>>>>> calls. This together with adding error paths, that allows the phy
>>>>> provider device to be runtime PM disabled, enables further clean up the
>>>>> code. More precisely, we can simply avoid to enable runtime PM for the phy
>>>>> device altogether, so let's do that as well.
>>>>>
>>>>> More importantly, this change also fixes an issue for system suspend.
>>>>> Especially in those cases when the phy provider device gets put into a low
>>>>> power state via calling the pm_runtime_force_suspend() helper, as is the
>>>>> case for a Renesas SoC, which has the phy provider device attached to the
>>>>> generic PM domain.
>>>>>
>>>>> The problem in this case, is that pm_runtime_force_suspend() expects the
>>>>> child device of the provider device to be runtime suspended, else this will
>>>>> trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
>>>>> resume.
>>>>>
>>>>> In the current case, even if phy_power_off() triggers a pm_runtime_put()
>>>>> during system suspend the phy device (child) doesn't get runtime suspended,
>>>>> because that is prevented in the system suspend phases. However, by
>>>>> avoiding to enable runtime PM, this problem goes away.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>>>>> ---
>>>>>  drivers/phy/phy-core.c | 33 +++++++++++++--------------------
>>>>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>>> index b4964b0..9fa3f13 100644
>>>>> --- a/drivers/phy/phy-core.c
>>>>> +++ b/drivers/phy/phy-core.c
>>>>> @@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
>>>>>       if (!phy)
>>>>>               return 0;
>>>>>
>>>>> -     ret = phy_pm_runtime_get_sync(phy);
>>>>> -     if (ret < 0 && ret != -ENOTSUPP)
>>>>> +     ret = pm_runtime_get_sync(phy->dev.parent);
>>>>
>>>> Won't this make phy-core manage pm_runtime of phy_provider even though the
>>>> phy_provider might not intend it?
>>>
>>> No it shouldn't.
>>>
>>> There are two cases to consider around this.
>>>
>>> 1) CONFIG_PM is unset. In this case pm_runtime_get_sync() will return
>>> 1, which is treated as succeeds by the error path.
>>>
>>> 2) CONFIG_PM is set, but the phy provider don't use runtime PM, thus
>>> it hasn't called pm_runtime_enable() for its device. In this case,
>>> pm_runtime_get_sync() returns -EACCES, which is also treated as
>>> success by the error path.
>>
>> There can be a case where the phy_provider uses runtime PM but doesn't want
>> phy-core to manage it.
>
> Ah, so you mean there are cases when the provider driver calls
> pm_runtime_enable() *after* it calls phy_create()/dev_phy_create()
> instead of before?

Okay so I found an example, thanks for pointing it out!

drivers/phy/ti/phy-twl4030-usb.c

>
> I am not really sure I understand *why* a provider driver wants to do
> that though, do you have more details?
> I mean, even if the phy core handles runtime PM, additional management
> can be done on top in the phy provider, there is nothing preventing
> that, but I guess that isn't sufficient?

In the above example, I guess the reason is related to the use of
usb_add_phy_dev().

I must say, the code in drivers/phy/ti/phy-twl4030-usb.c dealing with
these things looks a bit fragile, from a runtime PM point view.
However that's different story. :-)

Let me re-spin this, taking care of the problem you pointed out!

Kind regards
Uffe