| Message ID | 1513778960-10073-2-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > The runtime PM deployment in the phy core is deployed using the phy core > device, which is created by the phy core and assigned as a child device of > the phy provider device. > > The behaviour around the runtime PM deployment cause some issues during > system suspend, in cases when the phy provider device is put into a low > power state via a call to the pm_runtime_force_suspend() helper, as is the > case for a Renesas SoC, which has its phy provider device attached to the > generic PM domain. > > In more detail, the problem in this case is that pm_runtime_force_suspend() > expects the child device of the provider device, which is the phy core > device, to be runtime suspended, else a WARN splat will be printed > (correctly) when runtime PM gets re-enabled at system resume. So we are now trying to work around issues with pm_runtime_force_suspend(). Lovely. :-/ > In the current scenario, even if a call to phy_power_off() triggers it to > invoke pm_runtime_put() during system suspend, the phy core device doesn't > get runtime suspended, because this is prevented in the system suspend > phases by the PM core. > > To solve this problem, let's move the runtime PM deployment from the phy > core device to the phy provider device, as this provides the similar > behaviour. Changing this makes it redundant to enable runtime PM for the > phy core device, so let's avoid doing that. I'm not really convinced that this approach is the best one to be honest. I'll have a deeper look at this in the next few days, stay tuned. Thanks, Rafael
On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> The runtime PM deployment in the phy core is deployed using the phy core >> device, which is created by the phy core and assigned as a child device of >> the phy provider device. >> >> The behaviour around the runtime PM deployment cause some issues during >> system suspend, in cases when the phy provider device is put into a low >> power state via a call to the pm_runtime_force_suspend() helper, as is the >> case for a Renesas SoC, which has its phy provider device attached to the >> generic PM domain. >> >> In more detail, the problem in this case is that pm_runtime_force_suspend() >> expects the child device of the provider device, which is the phy core >> device, to be runtime suspended, else a WARN splat will be printed >> (correctly) when runtime PM gets re-enabled at system resume. > > So we are now trying to work around issues with > pm_runtime_force_suspend(). Lovely. :-/ Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or are you saying we should just ignore all issues related to it? Of course, if we had something that could replace pm_runtime_force_suspend(), that would be great, but there isn't. > >> In the current scenario, even if a call to phy_power_off() triggers it to >> invoke pm_runtime_put() during system suspend, the phy core device doesn't >> get runtime suspended, because this is prevented in the system suspend >> phases by the PM core. >> >> To solve this problem, let's move the runtime PM deployment from the phy >> core device to the phy provider device, as this provides the similar >> behaviour. Changing this makes it redundant to enable runtime PM for the >> phy core device, so let's avoid doing that. > > I'm not really convinced that this approach is the best one to be honest. > > I'll have a deeper look at this in the next few days, stay tuned. There is different ways to solve this, for sure. I picked this one, because I think it's the most trivial thing to do, and it shouldn't cause any other problems. I think any other option would involve assigning ->suspend|resume() callbacks to the phy core device, but that's fine too, if you prefer that. Also, I have considered how to deal with wakeup paths for phys, although I didn't want to post changes as a part of this series, but maybe I should to give a more complete picture? Kind regards Uffe
On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> The runtime PM deployment in the phy core is deployed using the phy core >>> device, which is created by the phy core and assigned as a child device of >>> the phy provider device. >>> >>> The behaviour around the runtime PM deployment cause some issues during >>> system suspend, in cases when the phy provider device is put into a low >>> power state via a call to the pm_runtime_force_suspend() helper, as is the >>> case for a Renesas SoC, which has its phy provider device attached to the >>> generic PM domain. >>> >>> In more detail, the problem in this case is that pm_runtime_force_suspend() >>> expects the child device of the provider device, which is the phy core >>> device, to be runtime suspended, else a WARN splat will be printed >>> (correctly) when runtime PM gets re-enabled at system resume. >> >> So we are now trying to work around issues with >> pm_runtime_force_suspend(). Lovely. :-/ > > Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or > are you saying we should just ignore all issues related to it? Or get rid of it? Seriously, is the resulting pain worth it? > Of course, if we had something that could replace > pm_runtime_force_suspend(), that would be great, but there isn't. I beg to differ. At least some of it could be replaced with the driver flags. >>> In the current scenario, even if a call to phy_power_off() triggers it to >>> invoke pm_runtime_put() during system suspend, the phy core device doesn't >>> get runtime suspended, because this is prevented in the system suspend >>> phases by the PM core. >>> >>> To solve this problem, let's move the runtime PM deployment from the phy >>> core device to the phy provider device, as this provides the similar >>> behaviour. Changing this makes it redundant to enable runtime PM for the >>> phy core device, so let's avoid doing that. >> >> I'm not really convinced that this approach is the best one to be honest. >> >> I'll have a deeper look at this in the next few days, stay tuned. > > There is different ways to solve this, for sure. I picked this one, > because I think it's the most trivial thing to do, and it shouldn't > cause any other problems. > > I think any other option would involve assigning ->suspend|resume() > callbacks to the phy core device, but that's fine too, if you prefer > that. > > Also, I have considered how to deal with wakeup paths for phys, > although I didn't want to post changes as a part of this series, but > maybe I should to give a more complete picture? Yes, you should. The point is that without genpd using pm_runtime_force_suspend() the phy code could very well stay the way it is. And it is logical, because having a parent with enabled runtime PM without enabling runtime PM for its children is at least conceptually questionable. So the conclusion may be that the phy code is OK, but calling pm_runtime_force_suspend() from genpd isn't. Thanks, Rafael
On Sat, Dec 23, 2017 at 2:35 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote: >>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> The runtime PM deployment in the phy core is deployed using the phy core >>>> device, which is created by the phy core and assigned as a child device of >>>> the phy provider device. [cut] >> >> Also, I have considered how to deal with wakeup paths for phys, >> although I didn't want to post changes as a part of this series, but >> maybe I should to give a more complete picture? > > Yes, you should. > > The point is that without genpd using pm_runtime_force_suspend() the > phy code could very well stay the way it is. And it is logical, > because having a parent with enabled runtime PM without enabling > runtime PM for its children is at least conceptually questionable. Actually, I sort of agree that the phy's usage of runtime PM is too convoluted. For example, it uses pm_runtime_enabled() unnecessarily at least in some places, but that doesn't seem to be fixed by your patches. Thanks, Rafael
On 23 December 2017 at 02:35, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote: >>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> The runtime PM deployment in the phy core is deployed using the phy core >>>> device, which is created by the phy core and assigned as a child device of >>>> the phy provider device. >>>> >>>> The behaviour around the runtime PM deployment cause some issues during >>>> system suspend, in cases when the phy provider device is put into a low >>>> power state via a call to the pm_runtime_force_suspend() helper, as is the >>>> case for a Renesas SoC, which has its phy provider device attached to the >>>> generic PM domain. >>>> >>>> In more detail, the problem in this case is that pm_runtime_force_suspend() >>>> expects the child device of the provider device, which is the phy core >>>> device, to be runtime suspended, else a WARN splat will be printed >>>> (correctly) when runtime PM gets re-enabled at system resume. >>> >>> So we are now trying to work around issues with >>> pm_runtime_force_suspend(). Lovely. :-/ >> >> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or >> are you saying we should just ignore all issues related to it? > > Or get rid of it? > > Seriously, is the resulting pain worth it? I am not sure what pain you refer to? :-) So far I haven't got much errors being reported because of its use, have you? Moreover, to me, this small series fixes the problems in a rather trivial way. > >> Of course, if we had something that could replace >> pm_runtime_force_suspend(), that would be great, but there isn't. > > I beg to differ. > > At least some of it could be replaced with the driver flags. Yes, true. On the other hand that is exactly the problem, only *some*. And more importantly, genpd can't use them, because it can't cope with have system wide PM callbacks to be skipped. In other words, so far, the driver PM flags can't solve this issue. > >>>> In the current scenario, even if a call to phy_power_off() triggers it to >>>> invoke pm_runtime_put() during system suspend, the phy core device doesn't >>>> get runtime suspended, because this is prevented in the system suspend >>>> phases by the PM core. >>>> >>>> To solve this problem, let's move the runtime PM deployment from the phy >>>> core device to the phy provider device, as this provides the similar >>>> behaviour. Changing this makes it redundant to enable runtime PM for the >>>> phy core device, so let's avoid doing that. >>> >>> I'm not really convinced that this approach is the best one to be honest. >>> >>> I'll have a deeper look at this in the next few days, stay tuned. >> >> There is different ways to solve this, for sure. I picked this one, >> because I think it's the most trivial thing to do, and it shouldn't >> cause any other problems. >> >> I think any other option would involve assigning ->suspend|resume() >> callbacks to the phy core device, but that's fine too, if you prefer >> that. >> >> Also, I have considered how to deal with wakeup paths for phys, >> although I didn't want to post changes as a part of this series, but >> maybe I should to give a more complete picture? > > Yes, you should. Okay! > > The point is that without genpd using pm_runtime_force_suspend() the > phy code could very well stay the way it is. And it is logical, > because having a parent with enabled runtime PM without enabling > runtime PM for its children is at least conceptually questionable. I agree that the phy core is today logical okay. But I think that's also the case, if we pick up this small series. There are many reasons and cases where a children is runtime PM disabled, while the parent is runtime PM enabled. Moreover the runtime PM core copes fine with that. > > So the conclusion may be that the phy code is OK, but calling > pm_runtime_force_suspend() from genpd isn't. So I am worried that changing genpd in this regards, may introduce other regressions. @subject series seems far less risky - and again, to me, the changes are trivial. Anyway, I will keep your suggestion in mind and think about, how/if genpd can be changed. Kind regards Uffe
On Thu, Dec 21, 2017 at 2:39 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> The runtime PM deployment in the phy core is deployed using the phy core >> device, which is created by the phy core and assigned as a child device of >> the phy provider device. >> >> The behaviour around the runtime PM deployment cause some issues during >> system suspend, in cases when the phy provider device is put into a low >> power state via a call to the pm_runtime_force_suspend() helper, as is the >> case for a Renesas SoC, which has its phy provider device attached to the >> generic PM domain. >> >> In more detail, the problem in this case is that pm_runtime_force_suspend() >> expects the child device of the provider device, which is the phy core >> device, to be runtime suspended, else a WARN splat will be printed >> (correctly) when runtime PM gets re-enabled at system resume. > > So we are now trying to work around issues with > pm_runtime_force_suspend(). Lovely. :-/ > >> In the current scenario, even if a call to phy_power_off() triggers it to >> invoke pm_runtime_put() during system suspend, the phy core device doesn't >> get runtime suspended, because this is prevented in the system suspend >> phases by the PM core. >> >> To solve this problem, let's move the runtime PM deployment from the phy >> core device to the phy provider device, as this provides the similar >> behaviour. Changing this makes it redundant to enable runtime PM for the >> phy core device, so let's avoid doing that. > > I'm not really convinced that this approach is the best one to be honest. > > I'll have a deeper look at this in the next few days, stay tuned. So IMO the changes you are proposing make sense regardless of the genpd issue, because they generally simplify the phy code, but the additional use_runtime_pm field in struct phy represents redundant information (manipulating reference counters shouldn't matter if runtime PM is disabled), so it doesn't appear to be necessary. [On a related note, I'm not sure why phy tries to intercept runtime PM errors and "fix up" the reference counters. That doesn't look right to me at all.] That said, the current phy code is not strictly invalid. While it looks more complicated than necessary, it doesn't do things documented as invalid in principle, so saying "The behaviour around the runtime PM deployment cause some issues during system suspend" in the changelog is describing the problem from a very specific angle. Simply put, pm_runtime_force_suspend() and the current phy code cannot work together and so using them together is a bug. None of them individually is at fault, but combining them is incorrect. Fortunately enough, the phy code can be modified so that it can be used with pm_runtime_force_suspend() without problems, but picturing it as "problematic", because it cannot do that today is not entirely fair IMO. Thanks, Rafael
On Sat, Dec 23, 2017 at 1:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 23 December 2017 at 02:35, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote: >>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>>> The runtime PM deployment in the phy core is deployed using the phy core >>>>> device, which is created by the phy core and assigned as a child device of >>>>> the phy provider device. >>>>> >>>>> The behaviour around the runtime PM deployment cause some issues during >>>>> system suspend, in cases when the phy provider device is put into a low >>>>> power state via a call to the pm_runtime_force_suspend() helper, as is the >>>>> case for a Renesas SoC, which has its phy provider device attached to the >>>>> generic PM domain. >>>>> >>>>> In more detail, the problem in this case is that pm_runtime_force_suspend() >>>>> expects the child device of the provider device, which is the phy core >>>>> device, to be runtime suspended, else a WARN splat will be printed >>>>> (correctly) when runtime PM gets re-enabled at system resume. >>>> >>>> So we are now trying to work around issues with >>>> pm_runtime_force_suspend(). Lovely. :-/ >>> >>> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or >>> are you saying we should just ignore all issues related to it? >> >> Or get rid of it? >> >> Seriously, is the resulting pain worth it? > > I am not sure what pain you refer to? :-) So far I haven't got much > errors being reported because of its use, have you? > > Moreover, to me, this small series fixes the problems in a rather trivial way. Well, I agree here (please see the message regarding that I've just sent in this thread). >> >>> Of course, if we had something that could replace >>> pm_runtime_force_suspend(), that would be great, but there isn't. >> >> I beg to differ. >> >> At least some of it could be replaced with the driver flags. > > Yes, true. > > On the other hand that is exactly the problem, only *some*. And more > importantly, genpd can't use them, because it can't cope with have > system wide PM callbacks to be skipped. Its own system-wide PM callbacks cannot be skipped, but it already skips driver callbacks in some cases. :-) > In other words, so far, the driver PM flags can't solve this issue. It is unclear which issue you are talking about, but anyway, yes, they aren't equivalent to pm_runtime_force_suspend/resume() which is quite intentional, honestly ... >>>>> In the current scenario, even if a call to phy_power_off() triggers it to >>>>> invoke pm_runtime_put() during system suspend, the phy core device doesn't >>>>> get runtime suspended, because this is prevented in the system suspend >>>>> phases by the PM core. >>>>> >>>>> To solve this problem, let's move the runtime PM deployment from the phy >>>>> core device to the phy provider device, as this provides the similar >>>>> behaviour. Changing this makes it redundant to enable runtime PM for the >>>>> phy core device, so let's avoid doing that. >>>> >>>> I'm not really convinced that this approach is the best one to be honest. >>>> >>>> I'll have a deeper look at this in the next few days, stay tuned. >>> >>> There is different ways to solve this, for sure. I picked this one, >>> because I think it's the most trivial thing to do, and it shouldn't >>> cause any other problems. >>> >>> I think any other option would involve assigning ->suspend|resume() >>> callbacks to the phy core device, but that's fine too, if you prefer >>> that. >>> >>> Also, I have considered how to deal with wakeup paths for phys, >>> although I didn't want to post changes as a part of this series, but >>> maybe I should to give a more complete picture? >> >> Yes, you should. > > Okay! > >> >> The point is that without genpd using pm_runtime_force_suspend() the >> phy code could very well stay the way it is. And it is logical, >> because having a parent with enabled runtime PM without enabling >> runtime PM for its children is at least conceptually questionable. > > I agree that the phy core is today logical okay. But I think that's > also the case, if we pick up this small series. > > There are many reasons and cases where a children is runtime PM > disabled, while the parent is runtime PM enabled. Moreover the runtime > PM core copes fine with that. Fair enough. >> >> So the conclusion may be that the phy code is OK, but calling >> pm_runtime_force_suspend() from genpd isn't. > > So I am worried that changing genpd in this regards, may introduce > other regressions. @subject series seems far less risky - and again, > to me, the changes are trivial. > > Anyway, I will keep your suggestion in mind and think about, how/if > genpd can be changed. Thanks a lot! Rafael
[...] > > So IMO the changes you are proposing make sense regardless of the > genpd issue, because they generally simplify the phy code, but the > additional use_runtime_pm field in struct phy represents redundant > information (manipulating reference counters shouldn't matter if > runtime PM is disabled), so it doesn't appear to be necessary. > Actually, the first version I posted treated the return codes from pm_runtime_get_sync() according to your suggestion above. However, Kishon pointed out that it didn't work. That's because, there are phy provider drivers that enables runtime PM *after* calling phy_create(). And in those cases, that is because they want to treat runtime PM themselves. I think that's probably something we should look into to change, but I find it being a separate issue, that I didn't want to investigate as part of this series. See more about the thread here: https://www.spinics.net/lists/linux-renesas-soc/msg21711.html > [On a related note, I'm not sure why phy tries to intercept runtime PM > errors and "fix up" the reference counters. That doesn't look right > to me at all.] > > That said, the current phy code is not strictly invalid. While it > looks more complicated than necessary, it doesn't do things documented > as invalid in principle, so saying "The behaviour around the runtime > PM deployment cause some issues during system suspend" in the > changelog is describing the problem from a very specific angle. > Simply put, pm_runtime_force_suspend() and the current phy code cannot > work together and so using them together is a bug. None of them > individually is at fault, but combining them is incorrect. > > Fortunately enough, the phy code can be modified so that it can be > used with pm_runtime_force_suspend() without problems, but picturing > it as "problematic", because it cannot do that today is not entirely > fair IMO. Right, this makes sense. Let me clarify this in the changelog. Kind regards Uffe
On Saturday, December 23, 2017 4:09:33 PM CET Ulf Hansson wrote: > [...] > > > > > So IMO the changes you are proposing make sense regardless of the > > genpd issue, because they generally simplify the phy code, but the > > additional use_runtime_pm field in struct phy represents redundant > > information (manipulating reference counters shouldn't matter if > > runtime PM is disabled), so it doesn't appear to be necessary. > > > > Actually, the first version I posted treated the return codes from > pm_runtime_get_sync() according to your suggestion above. > > However, Kishon pointed out that it didn't work. That's because, there > are phy provider drivers that enables runtime PM *after* calling > phy_create(). And in those cases, that is because they want to treat > runtime PM themselves. > > I think that's probably something we should look into to change, but I > find it being a separate issue, that I didn't want to investigate as > part of this series. > > See more about the thread here: > https://www.spinics.net/lists/linux-renesas-soc/msg21711.html Even so, it shouldn't matter when the provider enables runtime PM. Calling pm_runtime_get_*()/pm_runtime_put_*() should always work as long as they are balanced properly regardless of whether or not runtime PM is enabled for the device or it is enabled in the meantime. Of course, the initial runtime PM status of the device must reflect the values of the usage counters, but that should not be too hard to ensure. The reason why it matters here is because the phy core tries to be smart about manipulating reference counters by itself and that's a mistake. So it looks to me like the whole thing needs to be reworked and yes, that should be done in the first place IMO, because it will make the issue with genpd go away automatically. [Why is phy_pm_runtime_get() there at all, for instance? It seems to have no users and I kind of don't see use cases for it. Also phy_pm_runtime_get_sync() is only used by the phy core in three places - shouldn't be too hard to fix that.] So the issue with genpd really seems to be a messenger here. :-) Thanks, Rafael
On 24 December 2017 at 13:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Saturday, December 23, 2017 4:09:33 PM CET Ulf Hansson wrote: >> [...] >> >> > >> > So IMO the changes you are proposing make sense regardless of the >> > genpd issue, because they generally simplify the phy code, but the >> > additional use_runtime_pm field in struct phy represents redundant >> > information (manipulating reference counters shouldn't matter if >> > runtime PM is disabled), so it doesn't appear to be necessary. >> > >> >> Actually, the first version I posted treated the return codes from >> pm_runtime_get_sync() according to your suggestion above. >> >> However, Kishon pointed out that it didn't work. That's because, there >> are phy provider drivers that enables runtime PM *after* calling >> phy_create(). And in those cases, that is because they want to treat >> runtime PM themselves. >> >> I think that's probably something we should look into to change, but I >> find it being a separate issue, that I didn't want to investigate as >> part of this series. >> >> See more about the thread here: >> https://www.spinics.net/lists/linux-renesas-soc/msg21711.html > > Even so, it shouldn't matter when the provider enables runtime PM. > > Calling pm_runtime_get_*()/pm_runtime_put_*() should always work as > long as they are balanced properly regardless of whether or not > runtime PM is enabled for the device or it is enabled in the meantime. > Of course, the initial runtime PM status of the device must reflect > the values of the usage counters, but that should not be too hard to > ensure. Yes, this is probably the main reason. However, as stated, I think we should look into addressing this problem more carefully, in a separate next step. > > The reason why it matters here is because the phy core tries to be smart > about manipulating reference counters by itself and that's a mistake. > > So it looks to me like the whole thing needs to be reworked and yes, > that should be done in the first place IMO, because it will make the > issue with genpd go away automatically. Sorry, but I am not fully understanding what you suggest here. Perhaps you didn't check patch2? > > [Why is phy_pm_runtime_get() there at all, for instance? It seems > to have no users and I kind of don't see use cases for it. Also > phy_pm_runtime_get_sync() is only used by the phy core in three > places - shouldn't be too hard to fix that.] Removing these APIs and functions is done in patch2, so I guess I am already doing what you suggests above? No? [...] Kind regards Uffe
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index b4964b0..09588ec 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -153,12 +153,12 @@ int phy_pm_runtime_get(struct phy *phy) { int ret; - if (!pm_runtime_enabled(&phy->dev)) + if (!phy->use_runtime_pm) return -ENOTSUPP; - ret = pm_runtime_get(&phy->dev); + ret = pm_runtime_get(phy->dev.parent); if (ret < 0 && ret != -EINPROGRESS) - pm_runtime_put_noidle(&phy->dev); + pm_runtime_put_noidle(phy->dev.parent); return ret; } @@ -168,12 +168,12 @@ int phy_pm_runtime_get_sync(struct phy *phy) { int ret; - if (!pm_runtime_enabled(&phy->dev)) + if (!phy->use_runtime_pm) return -ENOTSUPP; - ret = pm_runtime_get_sync(&phy->dev); + ret = pm_runtime_get_sync(phy->dev.parent); if (ret < 0) - pm_runtime_put_sync(&phy->dev); + pm_runtime_put_sync(phy->dev.parent); return ret; } @@ -181,37 +181,37 @@ EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync); int phy_pm_runtime_put(struct phy *phy) { - if (!pm_runtime_enabled(&phy->dev)) + if (!phy->use_runtime_pm) return -ENOTSUPP; - return pm_runtime_put(&phy->dev); + return pm_runtime_put(phy->dev.parent); } EXPORT_SYMBOL_GPL(phy_pm_runtime_put); int phy_pm_runtime_put_sync(struct phy *phy) { - if (!pm_runtime_enabled(&phy->dev)) + if (!phy->use_runtime_pm) return -ENOTSUPP; - return pm_runtime_put_sync(&phy->dev); + return pm_runtime_put_sync(phy->dev.parent); } EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync); void phy_pm_runtime_allow(struct phy *phy) { - if (!pm_runtime_enabled(&phy->dev)) + if (!phy->use_runtime_pm) return; - pm_runtime_allow(&phy->dev); + pm_runtime_allow(phy->dev.parent); } EXPORT_SYMBOL_GPL(phy_pm_runtime_allow); void phy_pm_runtime_forbid(struct phy *phy) { - if (!pm_runtime_enabled(&phy->dev)) + if (!phy->use_runtime_pm) return; - pm_runtime_forbid(&phy->dev); + pm_runtime_forbid(phy->dev.parent); } EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid); @@ -774,10 +774,7 @@ 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); - } + phy->use_runtime_pm = pm_runtime_enabled(dev); return phy; diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index 4f8423a..b4298a1 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -83,6 +83,7 @@ struct phy { int power_count; struct phy_attrs attrs; struct regulator *pwr; + bool use_runtime_pm; }; /**
The runtime PM deployment in the phy core is deployed using the phy core device, which is created by the phy core and assigned as a child device of the phy provider device. The behaviour around the runtime PM deployment cause some issues during system suspend, in cases when the phy provider device is put into a low power state via a call to the pm_runtime_force_suspend() helper, as is the case for a Renesas SoC, which has its phy provider device attached to the generic PM domain. In more detail, the problem in this case is that pm_runtime_force_suspend() expects the child device of the provider device, which is the phy core device, to be runtime suspended, else a WARN splat will be printed (correctly) when runtime PM gets re-enabled at system resume. In the current scenario, even if a call to phy_power_off() triggers it to invoke pm_runtime_put() during system suspend, the phy core device doesn't get runtime suspended, because this is prevented in the system suspend phases by the PM core. To solve this problem, let's move the runtime PM deployment from the phy core device to the phy provider device, as this provides the similar behaviour. Changing this makes it redundant to enable runtime PM for the phy core device, so let's avoid doing that. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/phy/phy-core.c | 33 +++++++++++++++------------------ include/linux/phy/phy.h | 1 + 2 files changed, 16 insertions(+), 18 deletions(-)