| Message ID | 1662713084-8106-4-git-send-email-quic_krichai@quicinc.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru <quic_krichai@quicinc.com> wrote: > > Introducing phy power down/up callbacks for allowing to park the > link-state in L1ss without holding any PCIe resources during > system suspend. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ > include/linux/phy/phy.h | 20 ++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index d93ddf1..1b0b757 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed) > } > EXPORT_SYMBOL_GPL(phy_set_speed); > > +int phy_power_down(struct phy *phy) > +{ > + int ret; > + > + if (!phy || !phy->ops->power_down) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->power_down(phy); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_power_down); > + > +int phy_power_up(struct phy *phy) > +{ > + int ret; > + > + if (!phy || !phy->ops->power_up) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->power_up(phy); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} As it can be seen from the phy_power_off(), the PHY can be a shared resource, with the power_count counting the number of users that requested the PHY to be powered up. By introducing suc calls you break directly into this by allowing a single user to power down the PHY, no matter how many other users have requested the PHY to stay alive. > +EXPORT_SYMBOL_GPL(phy_power_up); > + > int phy_reset(struct phy *phy) > { > int ret; > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index b141375..3a45f4d 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -76,6 +76,8 @@ union phy_configure_opts { > * @set_mode: set the mode of the phy > * @set_media: set the media type of the phy (optional) > * @set_speed: set the speed of the phy (optional) > + * @power_down: parking the phy in power down state > + * @power_up: pulling back the phy from power down > * @reset: resetting the phy > * @calibrate: calibrate the phy > * @release: ops to be performed while the consumer relinquishes the PHY > @@ -89,6 +91,8 @@ struct phy_ops { > int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); > int (*set_media)(struct phy *phy, enum phy_media media); > int (*set_speed)(struct phy *phy, int speed); > + int (*power_down)(struct phy *phy); > + int (*power_up)(struct phy *phy); > > /** > * @configure: > @@ -226,6 +230,8 @@ int phy_init(struct phy *phy); > int phy_exit(struct phy *phy); > int phy_power_on(struct phy *phy); > int phy_power_off(struct phy *phy); > +int phy_power_down(struct phy *phy); > +int phy_power_up(struct phy *phy); > int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > @@ -349,6 +355,20 @@ static inline int phy_power_off(struct phy *phy) > return -ENOSYS; > } > > +static inline int phy_power_down(struct phy *phy) > +{ > + if (!phy) > + return 0; > + return -ENOSYS; > +} > + > +static inline int phy_power_up(struct phy *phy) > +{ > + if (!phy) > + return 0; > + return -ENOSYS; > +} > + > static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, > int submode) > { > -- > 2.7.4 >
On 09-09-22, 14:14, Krishna chaitanya chundru wrote: > Introducing phy power down/up callbacks for allowing to park the > link-state in L1ss without holding any PCIe resources during > system suspend. where is the rest of the series, pls cc relevant folks on cover at least! > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ > include/linux/phy/phy.h | 20 ++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index d93ddf1..1b0b757 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed) > } > EXPORT_SYMBOL_GPL(phy_set_speed); > > +int phy_power_down(struct phy *phy) > +{ > + int ret; > + > + if (!phy || !phy->ops->power_down) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->power_down(phy); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_power_down); I dont think this is a good idea as Dmitry already updated... > + > +int phy_power_up(struct phy *phy) > +{ > + int ret; > + > + if (!phy || !phy->ops->power_up) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->power_up(phy); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_power_up); > + > int phy_reset(struct phy *phy) > { > int ret; > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index b141375..3a45f4d 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -76,6 +76,8 @@ union phy_configure_opts { > * @set_mode: set the mode of the phy > * @set_media: set the media type of the phy (optional) > * @set_speed: set the speed of the phy (optional) > + * @power_down: parking the phy in power down state > + * @power_up: pulling back the phy from power down > * @reset: resetting the phy > * @calibrate: calibrate the phy > * @release: ops to be performed while the consumer relinquishes the PHY > @@ -89,6 +91,8 @@ struct phy_ops { > int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); > int (*set_media)(struct phy *phy, enum phy_media media); > int (*set_speed)(struct phy *phy, int speed); > + int (*power_down)(struct phy *phy); > + int (*power_up)(struct phy *phy); > > /** > * @configure: > @@ -226,6 +230,8 @@ int phy_init(struct phy *phy); > int phy_exit(struct phy *phy); > int phy_power_on(struct phy *phy); > int phy_power_off(struct phy *phy); > +int phy_power_down(struct phy *phy); > +int phy_power_up(struct phy *phy); > int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); > #define phy_set_mode(phy, mode) \ > phy_set_mode_ext(phy, mode, 0) > @@ -349,6 +355,20 @@ static inline int phy_power_off(struct phy *phy) > return -ENOSYS; > } > > +static inline int phy_power_down(struct phy *phy) > +{ > + if (!phy) > + return 0; > + return -ENOSYS; > +} > + > +static inline int phy_power_up(struct phy *phy) > +{ > + if (!phy) > + return 0; > + return -ENOSYS; > +} > + > static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, > int submode) > { > -- > 2.7.4
On Tue, Sep 13, 2022 at 08:28:15PM +0530, Vinod Koul wrote: > On 09-09-22, 14:14, Krishna chaitanya chundru wrote: > > Introducing phy power down/up callbacks for allowing to park the > > link-state in L1ss without holding any PCIe resources during > > system suspend. > > where is the rest of the series, pls cc relevant folks on cover at > least! Would be best to cc relevant folks, but in the meantime, it's easy to find via lore, e.g., 3/5 has: Message-Id: <1662713084-8106-4-git-send-email-quic_krichai@quicinc.com> so the lore URL is: https://lore.kernel.org/r/1662713084-8106-4-git-send-email-quic_krichai@quicinc.com and the thread overview is at the bottom. I use this incredibly handy mutt hook that adds lore URLs to emails directly when viewing them: https://github.com/danrue/lorifier Bjorn
On 9/9/2022 2:34 PM, Dmitry Baryshkov wrote: > On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru > <quic_krichai@quicinc.com> wrote: >> Introducing phy power down/up callbacks for allowing to park the >> link-state in L1ss without holding any PCIe resources during >> system suspend. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ >> include/linux/phy/phy.h | 20 ++++++++++++++++++++ >> 2 files changed, 50 insertions(+) >> >> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >> index d93ddf1..1b0b757 100644 >> --- a/drivers/phy/phy-core.c >> +++ b/drivers/phy/phy-core.c >> @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed) >> } >> EXPORT_SYMBOL_GPL(phy_set_speed); >> >> +int phy_power_down(struct phy *phy) >> +{ >> + int ret; >> + >> + if (!phy || !phy->ops->power_down) >> + return 0; >> + >> + mutex_lock(&phy->mutex); >> + ret = phy->ops->power_down(phy); >> + mutex_unlock(&phy->mutex); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(phy_power_down); >> + >> +int phy_power_up(struct phy *phy) >> +{ >> + int ret; >> + >> + if (!phy || !phy->ops->power_up) >> + return 0; >> + >> + mutex_lock(&phy->mutex); >> + ret = phy->ops->power_up(phy); >> + mutex_unlock(&phy->mutex); >> + >> + return ret; >> +} > As it can be seen from the phy_power_off(), the PHY can be a shared > resource, with the power_count counting the number of users that > requested the PHY to be powered up. By introducing suc calls you break > directly into this by allowing a single user to power down the PHY, no > matter how many other users have requested the PHY to stay alive. can we use same power_count in this function also here and restrict the single user to power down the PHY same like phy_power_off?. >> +EXPORT_SYMBOL_GPL(phy_power_up); >> + >> int phy_reset(struct phy *phy) >> { >> int ret; >> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h >> index b141375..3a45f4d 100644 >> --- a/include/linux/phy/phy.h >> +++ b/include/linux/phy/phy.h >> @@ -76,6 +76,8 @@ union phy_configure_opts { >> * @set_mode: set the mode of the phy >> * @set_media: set the media type of the phy (optional) >> * @set_speed: set the speed of the phy (optional) >> + * @power_down: parking the phy in power down state >> + * @power_up: pulling back the phy from power down >> * @reset: resetting the phy >> * @calibrate: calibrate the phy >> * @release: ops to be performed while the consumer relinquishes the PHY >> @@ -89,6 +91,8 @@ struct phy_ops { >> int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); >> int (*set_media)(struct phy *phy, enum phy_media media); >> int (*set_speed)(struct phy *phy, int speed); >> + int (*power_down)(struct phy *phy); >> + int (*power_up)(struct phy *phy); >> >> /** >> * @configure: >> @@ -226,6 +230,8 @@ int phy_init(struct phy *phy); >> int phy_exit(struct phy *phy); >> int phy_power_on(struct phy *phy); >> int phy_power_off(struct phy *phy); >> +int phy_power_down(struct phy *phy); >> +int phy_power_up(struct phy *phy); >> int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); >> #define phy_set_mode(phy, mode) \ >> phy_set_mode_ext(phy, mode, 0) >> @@ -349,6 +355,20 @@ static inline int phy_power_off(struct phy *phy) >> return -ENOSYS; >> } >> >> +static inline int phy_power_down(struct phy *phy) >> +{ >> + if (!phy) >> + return 0; >> + return -ENOSYS; >> +} >> + >> +static inline int phy_power_up(struct phy *phy) >> +{ >> + if (!phy) >> + return 0; >> + return -ENOSYS; >> +} >> + >> static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, >> int submode) >> { >> -- >> 2.7.4 >> >
On 14/09/2022 17:50, Krishna Chaitanya Chundru wrote: > > On 9/9/2022 2:34 PM, Dmitry Baryshkov wrote: >> On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru >> <quic_krichai@quicinc.com> wrote: >>> Introducing phy power down/up callbacks for allowing to park the >>> link-state in L1ss without holding any PCIe resources during >>> system suspend. >>> >>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>> --- >>> drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ >>> include/linux/phy/phy.h | 20 ++++++++++++++++++++ >>> 2 files changed, 50 insertions(+) >>> >>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >>> index d93ddf1..1b0b757 100644 >>> --- a/drivers/phy/phy-core.c >>> +++ b/drivers/phy/phy-core.c >>> @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed) >>> } >>> EXPORT_SYMBOL_GPL(phy_set_speed); >>> >>> +int phy_power_down(struct phy *phy) >>> +{ >>> + int ret; >>> + >>> + if (!phy || !phy->ops->power_down) >>> + return 0; >>> + >>> + mutex_lock(&phy->mutex); >>> + ret = phy->ops->power_down(phy); >>> + mutex_unlock(&phy->mutex); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(phy_power_down); >>> + >>> +int phy_power_up(struct phy *phy) >>> +{ >>> + int ret; >>> + >>> + if (!phy || !phy->ops->power_up) >>> + return 0; >>> + >>> + mutex_lock(&phy->mutex); >>> + ret = phy->ops->power_up(phy); >>> + mutex_unlock(&phy->mutex); >>> + >>> + return ret; >>> +} >> As it can be seen from the phy_power_off(), the PHY can be a shared >> resource, with the power_count counting the number of users that >> requested the PHY to be powered up. By introducing suc calls you break >> directly into this by allowing a single user to power down the PHY, no >> matter how many other users have requested the PHY to stay alive. > > can we use same power_count in this function also here and restrict the > single user to > > power down the PHY same like phy_power_off?. What is the difference between power_off() and power_down()?
On 9/19/2022 10:59 PM, Dmitry Baryshkov wrote: > On 14/09/2022 17:50, Krishna Chaitanya Chundru wrote: >> >> On 9/9/2022 2:34 PM, Dmitry Baryshkov wrote: >>> On Fri, 9 Sept 2022 at 11:45, Krishna chaitanya chundru >>> <quic_krichai@quicinc.com> wrote: >>>> Introducing phy power down/up callbacks for allowing to park the >>>> link-state in L1ss without holding any PCIe resources during >>>> system suspend. >>>> >>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>> --- >>>> drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ >>>> include/linux/phy/phy.h | 20 ++++++++++++++++++++ >>>> 2 files changed, 50 insertions(+) >>>> >>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >>>> index d93ddf1..1b0b757 100644 >>>> --- a/drivers/phy/phy-core.c >>>> +++ b/drivers/phy/phy-core.c >>>> @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed) >>>> } >>>> EXPORT_SYMBOL_GPL(phy_set_speed); >>>> >>>> +int phy_power_down(struct phy *phy) >>>> +{ >>>> + int ret; >>>> + >>>> + if (!phy || !phy->ops->power_down) >>>> + return 0; >>>> + >>>> + mutex_lock(&phy->mutex); >>>> + ret = phy->ops->power_down(phy); >>>> + mutex_unlock(&phy->mutex); >>>> + >>>> + return ret; >>>> +} >>>> +EXPORT_SYMBOL_GPL(phy_power_down); >>>> + >>>> +int phy_power_up(struct phy *phy) >>>> +{ >>>> + int ret; >>>> + >>>> + if (!phy || !phy->ops->power_up) >>>> + return 0; >>>> + >>>> + mutex_lock(&phy->mutex); >>>> + ret = phy->ops->power_up(phy); >>>> + mutex_unlock(&phy->mutex); >>>> + >>>> + return ret; >>>> +} >>> As it can be seen from the phy_power_off(), the PHY can be a shared >>> resource, with the power_count counting the number of users that >>> requested the PHY to be powered up. By introducing suc calls you break >>> directly into this by allowing a single user to power down the PHY, no >>> matter how many other users have requested the PHY to stay alive. >> >> can we use same power_count in this function also here and restrict >> the single user to >> >> power down the PHY same like phy_power_off?. > > What is the difference between power_off() and power_down()? In power_off we are turning off PCIe PHY-specific clocks, and also resetting the PHY due to this PCIe link also will go down. To retain, the PCIe link state in l1ss with PHY clocks turned off, we need park PCIe PHY in the power-down state and skip the resets of the PHY so that it can maintain the link state in l1ss with the help of the always-on power domain aka MX). To support this PHY Power-down state PHY driver has been updated with new interface APIs. Initially we added phy_suspend & phy_resume to phy but as this API's are already used by drivers/net/phy/phy_device.c we are getting compilation errors. So we used these power_down & power up. As power_off & power_down is confusing we will change new api's power_down & power_up to phy_pm_suspend & phy_pm_resume in the next patch series. > >
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index d93ddf1..1b0b757 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -441,6 +441,36 @@ int phy_set_speed(struct phy *phy, int speed) } EXPORT_SYMBOL_GPL(phy_set_speed); +int phy_power_down(struct phy *phy) +{ + int ret; + + if (!phy || !phy->ops->power_down) + return 0; + + mutex_lock(&phy->mutex); + ret = phy->ops->power_down(phy); + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_power_down); + +int phy_power_up(struct phy *phy) +{ + int ret; + + if (!phy || !phy->ops->power_up) + return 0; + + mutex_lock(&phy->mutex); + ret = phy->ops->power_up(phy); + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_power_up); + int phy_reset(struct phy *phy) { int ret; diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index b141375..3a45f4d 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -76,6 +76,8 @@ union phy_configure_opts { * @set_mode: set the mode of the phy * @set_media: set the media type of the phy (optional) * @set_speed: set the speed of the phy (optional) + * @power_down: parking the phy in power down state + * @power_up: pulling back the phy from power down * @reset: resetting the phy * @calibrate: calibrate the phy * @release: ops to be performed while the consumer relinquishes the PHY @@ -89,6 +91,8 @@ struct phy_ops { int (*set_mode)(struct phy *phy, enum phy_mode mode, int submode); int (*set_media)(struct phy *phy, enum phy_media media); int (*set_speed)(struct phy *phy, int speed); + int (*power_down)(struct phy *phy); + int (*power_up)(struct phy *phy); /** * @configure: @@ -226,6 +230,8 @@ int phy_init(struct phy *phy); int phy_exit(struct phy *phy); int phy_power_on(struct phy *phy); int phy_power_off(struct phy *phy); +int phy_power_down(struct phy *phy); +int phy_power_up(struct phy *phy); int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode); #define phy_set_mode(phy, mode) \ phy_set_mode_ext(phy, mode, 0) @@ -349,6 +355,20 @@ static inline int phy_power_off(struct phy *phy) return -ENOSYS; } +static inline int phy_power_down(struct phy *phy) +{ + if (!phy) + return 0; + return -ENOSYS; +} + +static inline int phy_power_up(struct phy *phy) +{ + if (!phy) + return 0; + return -ENOSYS; +} + static inline int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode) {
Introducing phy power down/up callbacks for allowing to park the link-state in L1ss without holding any PCIe resources during system suspend. Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ include/linux/phy/phy.h | 20 ++++++++++++++++++++ 2 files changed, 50 insertions(+)