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
