| Message ID | 1513718551-28624-2-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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@linaro.org> > --- > 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? Thanks Kishon
On 20 December 2017 at 07:42, Kishon Vijay Abraham I <kishon@ti.com> 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@linaro.org> >> --- >> 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
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@ti.com> 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@linaro.org> >>> --- >>> 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. Thanks Kishon
On 20 December 2017 at 10:02, Kishon Vijay Abraham I <kishon@ti.com> 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@ti.com> 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@linaro.org> >>>> --- >>>> 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? 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? Kind regards Uffe
On 20 December 2017 at 13:08, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 20 December 2017 at 10:02, Kishon Vijay Abraham I <kishon@ti.com> 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@ti.com> 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@linaro.org> >>>>> --- >>>>> 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
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); + if (ret < 0 && ret != -EACCES) return ret; - ret = 0; /* Override possible ret == -ENOTSUPP */ + ret = 0; mutex_lock(&phy->mutex); if (phy->init_count == 0 && phy->ops->init) { @@ -239,7 +239,7 @@ int phy_init(struct phy *phy) out: mutex_unlock(&phy->mutex); - phy_pm_runtime_put(phy); + pm_runtime_put(phy->dev.parent); return ret; } EXPORT_SYMBOL_GPL(phy_init); @@ -251,10 +251,10 @@ int phy_exit(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); + if (ret < 0 && ret != -EACCES) return ret; - ret = 0; /* Override possible ret == -ENOTSUPP */ + ret = 0; mutex_lock(&phy->mutex); if (phy->init_count == 1 && phy->ops->exit) { @@ -268,7 +268,7 @@ int phy_exit(struct phy *phy) out: mutex_unlock(&phy->mutex); - phy_pm_runtime_put(phy); + pm_runtime_put(phy->dev.parent); return ret; } EXPORT_SYMBOL_GPL(phy_exit); @@ -286,11 +286,10 @@ int phy_power_on(struct phy *phy) goto out; } - ret = phy_pm_runtime_get_sync(phy); - if (ret < 0 && ret != -ENOTSUPP) + ret = pm_runtime_get_sync(phy->dev.parent); + if (ret < 0 && ret != -EACCES) goto err_pm_sync; - - ret = 0; /* Override possible ret == -ENOTSUPP */ + ret = 0; mutex_lock(&phy->mutex); if (phy->power_count == 0 && phy->ops->power_on) { @@ -306,7 +305,7 @@ int phy_power_on(struct phy *phy) err_pwr_on: mutex_unlock(&phy->mutex); - phy_pm_runtime_put_sync(phy); + pm_runtime_put(phy->dev.parent); err_pm_sync: if (phy->pwr) regulator_disable(phy->pwr); @@ -333,7 +332,7 @@ int phy_power_off(struct phy *phy) } --phy->power_count; mutex_unlock(&phy->mutex); - phy_pm_runtime_put(phy); + pm_runtime_put(phy->dev.parent); if (phy->pwr) regulator_disable(phy->pwr); @@ -774,11 +773,6 @@ struct phy *phy_create(struct device *dev, struct device_node *node, if (ret) goto put_dev; - if (pm_runtime_enabled(dev)) { - pm_runtime_enable(&phy->dev); - pm_runtime_no_callbacks(&phy->dev); - } - return phy; put_dev: @@ -831,7 +825,6 @@ EXPORT_SYMBOL_GPL(devm_phy_create); */ void phy_destroy(struct phy *phy) { - pm_runtime_disable(&phy->dev); device_unregister(&phy->dev); } EXPORT_SYMBOL_GPL(phy_destroy);
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@linaro.org> --- drivers/phy/phy-core.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)