| Message ID | 1662713084-8106-1-git-send-email-quic_krichai@quicinc.com |
|---|---|
| Headers | show |
| Series | PCI: qcom: Add system suspend & resume support | expand |
On Fri, Sep 09, 2022 at 02:14:39PM +0530, Krishna chaitanya chundru wrote: > Add suspend and resume syscore ops. > > When system suspends, and if the link is in L1ss, disable the clocks > and power down the phy so that system enters into low power state by > parking link in L1ss to save the maximum power. And when the system > resumes, enable the clocks back and power on phy if they are disabled > in the suspend path. > You need to mention that you are only turning off the PCIe controller clocks and PHY is still powered by a separate domain (MX) so the link statys intact. > we are doing this only when link is in l1ss but not in L2/L3 as > nowhere we are forcing link to L2/L3 by sending PME turn off. > > is_suspended flag indicates if the clocks are disabled in the suspend > path or not. > > There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops > (getting hit by affinity changes while making CPUs offline during suspend, > this will happen after devices are suspended (all phases of suspend ops)). > When registered with pm ops there is a crash due to un-clocked access, > as in the pm suspend op clocks are disabled. So, registering with syscore > ops which will called after making CPUs offline. > > Make GDSC always on to ensure controller and its dependent clocks > won't go down during system suspend. > Where is the changelog? You seem to have added PHY and CLK patches to this series. You need to comment on that. Thanks, Mani > Krishna chaitanya chundru (5): > PCI: qcom: Add system suspend and resume support > PCI: qcom: Add retry logic for link to be stable in L1ss > phy: core: Add support for phy power down & power up > phy: qcom: Add power down/up callbacks to pcie phy > clk: qcom: Alwaya on pcie gdsc > > drivers/clk/qcom/gcc-sc7280.c | 2 +- > drivers/pci/controller/dwc/pcie-qcom.c | 156 ++++++++++++++++++++++++++++++- > drivers/phy/phy-core.c | 30 ++++++ > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 50 ++++++++++ > include/linux/phy/phy.h | 20 ++++ > 5 files changed, 256 insertions(+), 2 deletions(-) > > -- > 2.7.4 >
On 9/12/2022 10:34 PM, Manivannan Sadhasivam wrote: > + Rajendra > > On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote: >> Make GDSC always on to ensure controller and its dependent clocks >> won't go down during system suspend. >> > > You need to mention the SoC name in subject, otherwise one cannot know for > which platform this patch applies to. > >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> drivers/clk/qcom/gcc-sc7280.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c >> index 7ff64d4..2f781a2 100644 >> --- a/drivers/clk/qcom/gcc-sc7280.c >> +++ b/drivers/clk/qcom/gcc-sc7280.c >> @@ -3109,7 +3109,7 @@ static struct gdsc gcc_pcie_1_gdsc = { >> .name = "gcc_pcie_1_gdsc", >> }, >> .pwrsts = PWRSTS_OFF_ON, >> - .flags = VOTABLE, >> + .flags = ALWAYS_ON, > > Rajendra, should we also put PCIe GDSC into retention state as you have done for > USB [1]? Yes, it looks like we should handle this the same way as we did with usb. Why are we removing the VOTABLE flag anyway? > > Thanks, > Mani > > [1] https://lore.kernel.org/all/20220901101756.28164-2-quic_rjendra@quicinc.com/ > >> }; >> >> static struct gdsc gcc_ufs_phy_gdsc = { >> -- >> 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 12:12:32PM +0530, Rajendra Nayak wrote: > > On 9/12/2022 10:34 PM, Manivannan Sadhasivam wrote: > > + Rajendra > > > > On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote: > > > Make GDSC always on to ensure controller and its dependent clocks > > > won't go down during system suspend. > > > > > > > You need to mention the SoC name in subject, otherwise one cannot know for > > which platform this patch applies to. > > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > > > --- > > > drivers/clk/qcom/gcc-sc7280.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c > > > index 7ff64d4..2f781a2 100644 > > > --- a/drivers/clk/qcom/gcc-sc7280.c > > > +++ b/drivers/clk/qcom/gcc-sc7280.c > > > @@ -3109,7 +3109,7 @@ static struct gdsc gcc_pcie_1_gdsc = { > > > .name = "gcc_pcie_1_gdsc", > > > }, > > > .pwrsts = PWRSTS_OFF_ON, > > > - .flags = VOTABLE, > > > + .flags = ALWAYS_ON, > > > > Rajendra, should we also put PCIe GDSC into retention state as you have done for > > USB [1]? > > Yes, it looks like we should handle this the same way as we did with usb. Okay, thanks for the confirmation. > Why are we removing the VOTABLE flag anyway? Yeah, I don't see a reason for doing that. Chaitanya, please follow the patch from Rajendra I mentioned above and adapt it for PCIe GDSC. Thanks, Mani > > > > > Thanks, > > Mani > > > > [1] https://lore.kernel.org/all/20220901101756.28164-2-quic_rjendra@quicinc.com/ > > > > > }; > > > static struct gdsc gcc_ufs_phy_gdsc = { > > > -- > > > 2.7.4 > > > > >
On 9/12/2022 11:07 PM, Manivannan Sadhasivam wrote: > On Fri, Sep 09, 2022 at 02:14:39PM +0530, Krishna chaitanya chundru wrote: >> Add suspend and resume syscore ops. >> >> When system suspends, and if the link is in L1ss, disable the clocks >> and power down the phy so that system enters into low power state by >> parking link in L1ss to save the maximum power. And when the system >> resumes, enable the clocks back and power on phy if they are disabled >> in the suspend path. >> > You need to mention that you are only turning off the PCIe controller > clocks and PHY is still powered by a separate domain (MX) so the link > statys intact. sure I will update the commit in next series. >> we are doing this only when link is in l1ss but not in L2/L3 as >> nowhere we are forcing link to L2/L3 by sending PME turn off. >> >> is_suspended flag indicates if the clocks are disabled in the suspend >> path or not. >> >> There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops >> (getting hit by affinity changes while making CPUs offline during suspend, >> this will happen after devices are suspended (all phases of suspend ops)). >> When registered with pm ops there is a crash due to un-clocked access, >> as in the pm suspend op clocks are disabled. So, registering with syscore >> ops which will called after making CPUs offline. >> >> Make GDSC always on to ensure controller and its dependent clocks >> won't go down during system suspend. >> > Where is the changelog? You seem to have added PHY and CLK patches to > this series. You need to comment on that. > > Thanks, > Mani I will update that in next patch. >> Krishna chaitanya chundru (5): >> PCI: qcom: Add system suspend and resume support >> PCI: qcom: Add retry logic for link to be stable in L1ss >> phy: core: Add support for phy power down & power up >> phy: qcom: Add power down/up callbacks to pcie phy >> clk: qcom: Alwaya on pcie gdsc >> >> drivers/clk/qcom/gcc-sc7280.c | 2 +- >> drivers/pci/controller/dwc/pcie-qcom.c | 156 ++++++++++++++++++++++++++++++- >> drivers/phy/phy-core.c | 30 ++++++ >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 50 ++++++++++ >> include/linux/phy/phy.h | 20 ++++ >> 5 files changed, 256 insertions(+), 2 deletions(-) >> >> -- >> 2.7.4 >>
On 9/13/2022 10:12 PM, Manivannan Sadhasivam wrote: > On Tue, Sep 13, 2022 at 12:12:32PM +0530, Rajendra Nayak wrote: >> On 9/12/2022 10:34 PM, Manivannan Sadhasivam wrote: >>> + Rajendra >>> >>> On Fri, Sep 09, 2022 at 02:14:44PM +0530, Krishna chaitanya chundru wrote: >>>> Make GDSC always on to ensure controller and its dependent clocks >>>> won't go down during system suspend. >>>> >>> You need to mention the SoC name in subject, otherwise one cannot know for >>> which platform this patch applies to. >>> >>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>> --- >>>> drivers/clk/qcom/gcc-sc7280.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c >>>> index 7ff64d4..2f781a2 100644 >>>> --- a/drivers/clk/qcom/gcc-sc7280.c >>>> +++ b/drivers/clk/qcom/gcc-sc7280.c >>>> @@ -3109,7 +3109,7 @@ static struct gdsc gcc_pcie_1_gdsc = { >>>> .name = "gcc_pcie_1_gdsc", >>>> }, >>>> .pwrsts = PWRSTS_OFF_ON, >>>> - .flags = VOTABLE, >>>> + .flags = ALWAYS_ON, >>> Rajendra, should we also put PCIe GDSC into retention state as you have done for >>> USB [1]? >> Yes, it looks like we should handle this the same way as we did with usb. > Okay, thanks for the confirmation. > >> Why are we removing the VOTABLE flag anyway? > Yeah, I don't see a reason for doing that. > > Chaitanya, please follow the patch from Rajendra I mentioned above and adapt it > for PCIe GDSC. > > Thanks, > Mani ok I will try to adapt that. >>> Thanks, >>> Mani >>> >>> [1] https://lore.kernel.org/all/20220901101756.28164-2-quic_rjendra@quicinc.com/ >>> >>>> }; >>>> static struct gdsc gcc_ufs_phy_gdsc = { >>>> -- >>>> 2.7.4 >>>>
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. > >