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. Does it make sense? Kind regards Uffe
