| Message ID | 1643738876-18572-1-git-send-email-quic_pmaliset@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] PCI: qcom: Add system PM support | expand |
On Tue, Feb 01, 2022 at 11:37:56PM +0530, Prasad Malisetty wrote: > Add suspend_noirq and resume_noirq callbacks to handle > System suspend and resume in dwc pcie controller driver. s/System/system/ s/pcie/PCIe/ as you did below. > When system suspends, send PME turnoff message to enter > link into L2 state. Along with powerdown the PHY, disable > pipe clock, switch gcc_pcie_1_pipe_clk_src to XO if mux is > supported and disable the pcie clocks, regulators. > > When system resumes, PCIe link will be re-established and > setup rc settings. > > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > Reported-by: kernel test robot <lkp@intel.com> The kernel test robot reported the lack of system power management? I'm impressed ;) I doubt this "Reported-by" is useful. If it *is*, please include a link to the report. > --- > Changes since v1: > - Removed unnecessary logs and modified log level suggested by Manivannan. > - Removed platform specific callbacks as PM support is generic. > --- > drivers/pci/controller/dwc/pcie-qcom.c | 97 ++++++++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index c19cd506..d1dd6c7 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -73,6 +73,8 @@ > > #define PCIE20_PARF_Q2A_FLUSH 0x1AC > > +#define PCIE20_PARF_PM_STTS 0x24 Indent with tabs (not spaces) as the surrounding code does. > #define PCIE20_MISC_CONTROL_1_REG 0x8BC > #define DBI_RO_WR_EN 1 > > @@ -1616,6 +1618,100 @@ static int qcom_pcie_probe(struct platform_device *pdev) > return ret; > } > > +static int qcom_pcie_send_pme_turnoff_msg(struct qcom_pcie *pcie) > +{ > + int ret = 0; Drop unnecessary init. > + u32 val = 0, poll_val = 0; Drop unnecessary "val" init. > + u64 l23_rdy_poll_timeout = 100000; Add "microseconds" comment. > + struct dw_pcie *pci = pcie->pci; > + struct device *dev = pci->dev; > + > + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL); > + val |= BIT(4); > + writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL); > + > + ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val, > + (poll_val & BIT(5)), 10000, l23_rdy_poll_timeout); > + if (!ret) > + dev_info(dev, "PM_Enter_L23 is received\n"); > + else > + dev_err(dev, "PM_Enter_L23 is NOT received.PARF_PM_STTS 0x%x\n", > + readl_relaxed(pcie->parf + PCIE20_PARF_PM_STTS)); > + > + return ret; > +} > + > +static void qcom_pcie_host_disable(struct qcom_pcie *pcie) > +{ > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0; qcom_pcie_host_disable() is called from qcom_pcie_pm_suspend_noirq(), which is used for all qcom devices. But it looks like not all qcom devices have 2_7_0 resources. Is this supposed to be used only on certain revisions? > + > + /* Assert the reset of endpoint */ Superfluous comment, since the function name says the same thing. > + qcom_ep_reset_assert(pcie); > + > + /* Put PHY into POWER DOWN state */ > + phy_power_off(pcie->phy); > + > + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL); > + > + pcie->ops->post_deinit(pcie); > + > + /* Disable PCIe clocks and regulators */ > + pcie->ops->deinit(pcie); > +} > + > +static int __maybe_unused qcom_pcie_pm_suspend_noirq(struct device *dev) > +{ > + int ret = 0; > + struct qcom_pcie *pcie = dev_get_drvdata(dev); > + struct dw_pcie *pci = pcie->pci; > + > + if (!dw_pcie_link_up(pci)) { > + dev_dbg(dev, "Power has been turned off already\n"); > + return ret; "return 0" here and drop the init above. > + } > + > + /* Send PME turnoff msg */ Superfluous comment. > + ret = qcom_pcie_send_pme_turnoff_msg(pcie); > + if (ret) > + return ret; > + > + /* Power down the PHY, disable clock and regulators */ > + qcom_pcie_host_disable(pcie); > + > + return ret; "return 0" here. > +} > + > +/* Resume the PCIe link */ > +static int __maybe_unused qcom_pcie_pm_resume_noirq(struct device *dev) > +{ > + int ret = 0; Drop unnecessary init. > + struct qcom_pcie *pcie = dev_get_drvdata(dev); > + struct dw_pcie *pci = pcie->pci; > + struct pcie_port *pp = &pci->pp; > + > + /* Initialize PCIe host */ Superfluous comment. > + ret = qcom_pcie_host_init(pp); > + if (ret) { > + dev_err(dev, "cannot initialize host\n"); > + return ret; > + } > + > + dw_pcie_setup_rc(pp); > + > + /* Start the PCIe link */ Superfluous comment. > + qcom_pcie_start_link(pci); > + > + ret = dw_pcie_wait_for_link(pci); > + if (ret) > + dev_err(dev, "Link never came up, Resume failed\n"); > + > + return ret; > +} > + > +static const struct dev_pm_ops qcom_pcie_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend_noirq, qcom_pcie_pm_resume_noirq) qcom_pcie_pm_suspend_noirq() and qcom_pcie_pm_resume_noirq() look nothing like their counterparts in dra7xx, exynos, imx6, intel-gw. Any chance you could make them more similar? > +}; > + > static const struct of_device_id qcom_pcie_match[] = { > { .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg }, > { .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg }, > @@ -1648,6 +1744,7 @@ static struct platform_driver qcom_pcie_driver = { > .probe = qcom_pcie_probe, > .driver = { > .name = "qcom-pcie", > + .pm = &qcom_pcie_pm_ops, > .suppress_bind_attrs = true, > .of_match_table = qcom_pcie_match, > }, > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
On top of Bjorn's review: On Tue, Feb 01, 2022 at 11:37:56PM +0530, Prasad Malisetty wrote: > Add suspend_noirq and resume_noirq callbacks to handle > System suspend and resume in dwc pcie controller driver. > > When system suspends, send PME turnoff message to enter > link into L2 state. Along with powerdown the PHY, disable > pipe clock, switch gcc_pcie_1_pipe_clk_src to XO if mux is > supported and disable the pcie clocks, regulators. > > When system resumes, PCIe link will be re-established and > setup rc settings. > > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > Reported-by: kernel test robot <lkp@intel.com> > > --- > Changes since v1: > - Removed unnecessary logs and modified log level suggested by Manivannan. > - Removed platform specific callbacks as PM support is generic. This is not still generic... Please see below. > --- > drivers/pci/controller/dwc/pcie-qcom.c | 97 ++++++++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index c19cd506..d1dd6c7 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -73,6 +73,8 @@ > > #define PCIE20_PARF_Q2A_FLUSH 0x1AC > > +#define PCIE20_PARF_PM_STTS 0x24 > + > #define PCIE20_MISC_CONTROL_1_REG 0x8BC > #define DBI_RO_WR_EN 1 > > @@ -1616,6 +1618,100 @@ static int qcom_pcie_probe(struct platform_device *pdev) > return ret; > } > > +static int qcom_pcie_send_pme_turnoff_msg(struct qcom_pcie *pcie) > +{ > + int ret = 0; > + u32 val = 0, poll_val = 0; > + u64 l23_rdy_poll_timeout = 100000; > + struct dw_pcie *pci = pcie->pci; > + struct device *dev = pci->dev; > + > + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL); > + val |= BIT(4); Define BIT(4) > + writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL); > + > + ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val, > + (poll_val & BIT(5)), 10000, l23_rdy_poll_timeout); Define BIT(5) > + if (!ret) > + dev_info(dev, "PM_Enter_L23 is received\n"); Maybe print, "Device entered L23_Ready state"? Also this should be dev_dbg(). > + else > + dev_err(dev, "PM_Enter_L23 is NOT received.PARF_PM_STTS 0x%x\n", Maybe print, "Device failed to enter L23_Ready state"? > + readl_relaxed(pcie->parf + PCIE20_PARF_PM_STTS)); > + > + return ret; > +} > + > +static void qcom_pcie_host_disable(struct qcom_pcie *pcie) > +{ > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0; > + As Bjorn said this would only work for platforms supporting v2.7.0 ops. Please make it generic. > + /* Assert the reset of endpoint */ > + qcom_ep_reset_assert(pcie); > + > + /* Put PHY into POWER DOWN state */ > + phy_power_off(pcie->phy); > + > + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL); Define "1". Thanks, Mani
Hi Manivannan, Thanks for review and comments. Please see my comments inline. Thanks -Prasad -----Original Message----- From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Sent: Friday, February 11, 2022 2:44 PM To: Prasad Malisetty (Temp) (QUIC) <quic_pmaliset@quicinc.com> Cc: agross@kernel.org; bjorn.andersson@linaro.org; lorenzo.pieralisi@arm.com; robh@kernel.org; kw@linux.com; bhelgaas@google.com; linux-pci@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; Veerabhadrarao Badiganti (QUIC) <quic_vbadigan@quicinc.com>; Rama Krishna (QUIC) <quic_ramkri@quicinc.com>; swboyd@chromium.org Subject: Re: [PATCH v2] PCI: qcom: Add system PM support On top of Bjorn's review: On Tue, Feb 01, 2022 at 11:37:56PM +0530, Prasad Malisetty wrote: > Add suspend_noirq and resume_noirq callbacks to handle System suspend > and resume in dwc pcie controller driver. > > When system suspends, send PME turnoff message to enter link into L2 > state. Along with powerdown the PHY, disable pipe clock, switch > gcc_pcie_1_pipe_clk_src to XO if mux is supported and disable the pcie > clocks, regulators. > > When system resumes, PCIe link will be re-established and setup rc > settings. > > Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> > Reported-by: kernel test robot <lkp@intel.com> > > --- > Changes since v1: > - Removed unnecessary logs and modified log level suggested by Manivannan. > - Removed platform specific callbacks as PM support is generic. This is not still generic... Please see below. > --- > drivers/pci/controller/dwc/pcie-qcom.c | 97 > ++++++++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c > b/drivers/pci/controller/dwc/pcie-qcom.c > index c19cd506..d1dd6c7 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -73,6 +73,8 @@ > > #define PCIE20_PARF_Q2A_FLUSH 0x1AC > > +#define PCIE20_PARF_PM_STTS 0x24 > + > #define PCIE20_MISC_CONTROL_1_REG 0x8BC > #define DBI_RO_WR_EN 1 > > @@ -1616,6 +1618,100 @@ static int qcom_pcie_probe(struct platform_device *pdev) > return ret; > } > > +static int qcom_pcie_send_pme_turnoff_msg(struct qcom_pcie *pcie) { > + int ret = 0; > + u32 val = 0, poll_val = 0; > + u64 l23_rdy_poll_timeout = 100000; > + struct dw_pcie *pci = pcie->pci; > + struct device *dev = pci->dev; > + > + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL); > + val |= BIT(4); Define BIT(4) >> Okay sure. > + writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL); > + > + ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val, > + (poll_val & BIT(5)), 10000, l23_rdy_poll_timeout); Define BIT(5) >> Okay, I will update in next patch version. > + if (!ret) > + dev_info(dev, "PM_Enter_L23 is received\n"); Maybe print, "Device entered L23_Ready state"? Also this should be dev_dbg(). > + else > + dev_err(dev, "PM_Enter_L23 is NOT received.PARF_PM_STTS 0x%x\n", Maybe print, "Device failed to enter L23_Ready state"? >> Sure, I will update in coming patch version. > + readl_relaxed(pcie->parf + PCIE20_PARF_PM_STTS)); > + > + return ret; > +} > + > +static void qcom_pcie_host_disable(struct qcom_pcie *pcie) { > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0; > + As Bjorn said this would only work for platforms supporting v2.7.0 ops. Please make it generic. >>Sure, I removed the platform specific code but forgot to remove above line. I will update in next patch version. > + /* Assert the reset of endpoint */ > + qcom_ep_reset_assert(pcie); > + > + /* Put PHY into POWER DOWN state */ > + phy_power_off(pcie->phy); > + > + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL); Define "1". >> Sure Manivannan. Thanks, Mani
Hi Mani, Thanks for the review and comments. I updated my mail client. sending the same responses. I will post the new patch version. Testing is in progress. Thanks -Prasad On 2/11/2022 2:44 PM, Manivannan Sadhasivam wrote: > On top of Bjorn's review: > > On Tue, Feb 01, 2022 at 11:37:56PM +0530, Prasad Malisetty wrote: >> Add suspend_noirq and resume_noirq callbacks to handle >> System suspend and resume in dwc pcie controller driver. >> >> When system suspends, send PME turnoff message to enter >> link into L2 state. Along with powerdown the PHY, disable >> pipe clock, switch gcc_pcie_1_pipe_clk_src to XO if mux is >> supported and disable the pcie clocks, regulators. >> >> When system resumes, PCIe link will be re-established and >> setup rc settings. >> >> Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> >> Reported-by: kernel test robot <lkp@intel.com> >> >> --- >> Changes since v1: >> - Removed unnecessary logs and modified log level suggested by Manivannan. >> - Removed platform specific callbacks as PM support is generic. > This is not still generic... Please see below. > >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 97 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 97 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index c19cd506..d1dd6c7 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -73,6 +73,8 @@ >> >> #define PCIE20_PARF_Q2A_FLUSH 0x1AC >> >> +#define PCIE20_PARF_PM_STTS 0x24 >> + >> #define PCIE20_MISC_CONTROL_1_REG 0x8BC >> #define DBI_RO_WR_EN 1 >> >> @@ -1616,6 +1618,100 @@ static int qcom_pcie_probe(struct platform_device *pdev) >> return ret; >> } >> >> +static int qcom_pcie_send_pme_turnoff_msg(struct qcom_pcie *pcie) >> +{ >> + int ret = 0; >> + u32 val = 0, poll_val = 0; >> + u64 l23_rdy_poll_timeout = 100000; >> + struct dw_pcie *pci = pcie->pci; >> + struct device *dev = pci->dev; >> + >> + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL); >> + val |= BIT(4); > Define BIT(4) Sure, I will update in next patch version. > >> + writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL); >> + >> + ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val, >> + (poll_val & BIT(5)), 10000, l23_rdy_poll_timeout); > Define BIT(5) Sure, I will update in next patch version. > >> + if (!ret) >> + dev_info(dev, "PM_Enter_L23 is received\n"); > Maybe print, "Device entered L23_Ready state"? Also this should be dev_dbg(). > >> + else >> + dev_err(dev, "PM_Enter_L23 is NOT received.PARF_PM_STTS 0x%x\n", > Maybe print, "Device failed to enter L23_Ready state"? Agree, I will update in next patch version. > >> + readl_relaxed(pcie->parf + PCIE20_PARF_PM_STTS)); >> + >> + return ret; >> +} >> + >> +static void qcom_pcie_host_disable(struct qcom_pcie *pcie) >> +{ >> + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0; >> + > As Bjorn said this would only work for platforms supporting v2.7.0 ops. Please > make it generic. I removed platform specific code but forgot to remove above one. will update in next patch version. >> + /* Assert the reset of endpoint */ >> + qcom_ep_reset_assert(pcie); >> + >> + /* Put PHY into POWER DOWN state */ >> + phy_power_off(pcie->phy); >> + >> + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL); > Define "1". Sure, I will update in next patch version. > > Thanks, > Mani
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index c19cd506..d1dd6c7 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -73,6 +73,8 @@ #define PCIE20_PARF_Q2A_FLUSH 0x1AC +#define PCIE20_PARF_PM_STTS 0x24 + #define PCIE20_MISC_CONTROL_1_REG 0x8BC #define DBI_RO_WR_EN 1 @@ -1616,6 +1618,100 @@ static int qcom_pcie_probe(struct platform_device *pdev) return ret; } +static int qcom_pcie_send_pme_turnoff_msg(struct qcom_pcie *pcie) +{ + int ret = 0; + u32 val = 0, poll_val = 0; + u64 l23_rdy_poll_timeout = 100000; + struct dw_pcie *pci = pcie->pci; + struct device *dev = pci->dev; + + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL); + val |= BIT(4); + writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL); + + ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val, + (poll_val & BIT(5)), 10000, l23_rdy_poll_timeout); + if (!ret) + dev_info(dev, "PM_Enter_L23 is received\n"); + else + dev_err(dev, "PM_Enter_L23 is NOT received.PARF_PM_STTS 0x%x\n", + readl_relaxed(pcie->parf + PCIE20_PARF_PM_STTS)); + + return ret; +} + +static void qcom_pcie_host_disable(struct qcom_pcie *pcie) +{ + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0; + + /* Assert the reset of endpoint */ + qcom_ep_reset_assert(pcie); + + /* Put PHY into POWER DOWN state */ + phy_power_off(pcie->phy); + + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL); + + pcie->ops->post_deinit(pcie); + + /* Disable PCIe clocks and regulators */ + pcie->ops->deinit(pcie); +} + +static int __maybe_unused qcom_pcie_pm_suspend_noirq(struct device *dev) +{ + int ret = 0; + struct qcom_pcie *pcie = dev_get_drvdata(dev); + struct dw_pcie *pci = pcie->pci; + + if (!dw_pcie_link_up(pci)) { + dev_dbg(dev, "Power has been turned off already\n"); + return ret; + } + + /* Send PME turnoff msg */ + ret = qcom_pcie_send_pme_turnoff_msg(pcie); + if (ret) + return ret; + + /* Power down the PHY, disable clock and regulators */ + qcom_pcie_host_disable(pcie); + + return ret; +} + +/* Resume the PCIe link */ +static int __maybe_unused qcom_pcie_pm_resume_noirq(struct device *dev) +{ + int ret = 0; + struct qcom_pcie *pcie = dev_get_drvdata(dev); + struct dw_pcie *pci = pcie->pci; + struct pcie_port *pp = &pci->pp; + + /* Initialize PCIe host */ + ret = qcom_pcie_host_init(pp); + if (ret) { + dev_err(dev, "cannot initialize host\n"); + return ret; + } + + dw_pcie_setup_rc(pp); + + /* Start the PCIe link */ + qcom_pcie_start_link(pci); + + ret = dw_pcie_wait_for_link(pci); + if (ret) + dev_err(dev, "Link never came up, Resume failed\n"); + + return ret; +} + +static const struct dev_pm_ops qcom_pcie_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend_noirq, qcom_pcie_pm_resume_noirq) +}; + static const struct of_device_id qcom_pcie_match[] = { { .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg }, { .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg }, @@ -1648,6 +1744,7 @@ static struct platform_driver qcom_pcie_driver = { .probe = qcom_pcie_probe, .driver = { .name = "qcom-pcie", + .pm = &qcom_pcie_pm_ops, .suppress_bind_attrs = true, .of_match_table = qcom_pcie_match, },
Add suspend_noirq and resume_noirq callbacks to handle System suspend and resume in dwc pcie controller driver. When system suspends, send PME turnoff message to enter link into L2 state. Along with powerdown the PHY, disable pipe clock, switch gcc_pcie_1_pipe_clk_src to XO if mux is supported and disable the pcie clocks, regulators. When system resumes, PCIe link will be re-established and setup rc settings. Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com> Reported-by: kernel test robot <lkp@intel.com> --- Changes since v1: - Removed unnecessary logs and modified log level suggested by Manivannan. - Removed platform specific callbacks as PM support is generic. --- drivers/pci/controller/dwc/pcie-qcom.c | 97 ++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+)