| Message ID | 1662713084-8106-1-git-send-email-quic_krichai@quicinc.com (mailing list archive) |
|---|---|
| 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. > > 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. > > 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 This seems fairly ugly because it doesn't fit nicely into the PM framework. Why is this a qcom-specific thing? What about other DWC-based controllers? > 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/10/2022 1:21 AM, Bjorn Helgaas 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. >> >> 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. >> >> 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 > This seems fairly ugly because it doesn't fit nicely into the PM > framework. Why is this a qcom-specific thing? What about other > DWC-based controllers? We wanted to allow system S3 state by turning off all PCIe clocks but at the same time retaining NVMe device in D0 state and PCIe link in l1ss state. Here nothing really specific to DWC as PCIe controller remains intact. And the Qcom PHY allows this scheme (that is to retain the link state in l1ss even though all pcie clocks are turned off). Since clocks are completely managed by qcom platform driver, we are trying to manage them during S3/S0 transitions with PM callbacks. >> 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 Mon, Sep 12, 2022 at 09:40:30PM +0530, Krishna Chaitanya Chundru wrote: > On 9/10/2022 1:21 AM, Bjorn Helgaas 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. > > > > > > 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. > > > > > > 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 > > > > This seems fairly ugly because it doesn't fit nicely into the PM > > framework. Why is this a qcom-specific thing? What about other > > DWC-based controllers? > > We wanted to allow system S3 state by turning off all PCIe clocks > but at the same time retaining NVMe device in D0 state and PCIe link > in l1ss state. > > Here nothing really specific to DWC as PCIe controller remains intact. > > And the Qcom PHY allows this scheme (that is to retain the link > state in l1ss even though all pcie clocks are turned off). Is there somewhere in the PCIe spec I can read about how a link with clocks turned off can remain in L1.1 or L1.2? > Since clocks are completely managed by qcom platform driver, we are > trying to manage them during S3/S0 transitions with PM callbacks. I'm looking at this text in PCIe r6.0, sec 5.4.1: Components in the D0 state (i.e., fully active state) normally keep their Upstream Link in the active L0 state, as defined in § Section 5.3.2 . ASPM defines a protocol for components in the D0 state to reduce Link power by placing their Links into a low power state and instructing the other end of the Link to do likewise. This capability allows hardware-autonomous, dynamic Link power reduction beyond what is achievable by software-only controlled (i.e., PCI-PM software driven) power management. How does this qcom software management of clocks fit into this scheme? It seems to me that if you need software to turn clocks off and on, that is no longer ASPM. Bjorn
On Mon, Sep 12, 2022 at 12:08:06PM -0500, Bjorn Helgaas wrote: > On Mon, Sep 12, 2022 at 09:40:30PM +0530, Krishna Chaitanya Chundru wrote: > > On 9/10/2022 1:21 AM, Bjorn Helgaas 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. > > > > > > > > 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. > > > > > > > > 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 > > > > > > This seems fairly ugly because it doesn't fit nicely into the PM > > > framework. Why is this a qcom-specific thing? What about other > > > DWC-based controllers? > > > > We wanted to allow system S3 state by turning off all PCIe clocks > > but at the same time retaining NVMe device in D0 state and PCIe link > > in l1ss state. > > > > Here nothing really specific to DWC as PCIe controller remains intact. > > > > And the Qcom PHY allows this scheme (that is to retain the link > > state in l1ss even though all pcie clocks are turned off). > > Is there somewhere in the PCIe spec I can read about how a link with > clocks turned off can remain in L1.1 or L1.2? > This part is Qcom specific. On Qcom platforms there are two power domains used, CX and MX. CX domain is sourcing the PCIe controller and MX is sourcing PCIe PHY. Both PCIe and PHY drivers don't control MX domain and only control CX. So even though this patch is turning off all of the PCIe clocks, that only helps in powering down the CX domain for achieving the low power usecase while still keeping MX domain powered on. So at the end of suspend, the PCIe controller would've turned off but the link stays in L1SS state due to it being backed by MX. > > Since clocks are completely managed by qcom platform driver, we are > > trying to manage them during S3/S0 transitions with PM callbacks. > > I'm looking at this text in PCIe r6.0, sec 5.4.1: > > Components in the D0 state (i.e., fully active state) normally keep > their Upstream Link in the active L0 state, as defined in § Section > 5.3.2 . ASPM defines a protocol for components in the D0 state to > reduce Link power by placing their Links into a low power state and > instructing the other end of the Link to do likewise. This > capability allows hardware-autonomous, dynamic Link power reduction > beyond what is achievable by software-only controlled (i.e., PCI-PM > software driven) power management. > > How does this qcom software management of clocks fit into this scheme? > It seems to me that if you need software to turn clocks off and on, > that is no longer ASPM. > The PCIe link automatically transitions to L1SS if there is no activity on the link but still the clocks sourced to the PCIe controller needs to be turned off. PCIe spec only covers the link specifics and the controller is platform dependent. Thanks, Mani > Bjorn
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 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 >>