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 <[email protected]>
---
Changes since v3:
- Replaced noirq hooks with normal suspend/resume hooks.
- Removed local variable and placed in function itself.
Changes since v2:
- Removed unnecessary variable initializations and comments.
- Removed platform specific variables declarations.
- Added MACRO names for the BIT shiftings.
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 6ab9089..4d29c80 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -48,6 +48,7 @@
#define PCIE20_PARF_PHY_REFCLK 0x4C
#define PHY_REFCLK_SSP_EN BIT(16)
#define PHY_REFCLK_USE_PAD BIT(12)
+#define PHY_POWER_DOWN 0x1
#define PCIE20_PARF_DBI_BASE_ADDR 0x168
#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C
@@ -62,6 +63,8 @@
#define PCIE20_ELBI_SYS_CTRL 0x04
#define PCIE20_ELBI_SYS_CTRL_LT_ENABLE BIT(0)
+#define PCIE_PME_TURNOFF_MSG BIT(4)
+#define PCIE_PM_LINKST_IN_L2 BIT(5)
#define PCIE20_AXI_MSTR_RESP_COMP_CTRL0 0x818
#define CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K 0x4
@@ -73,6 +76,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
@@ -1645,6 +1650,97 @@ 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;
+ u32 val, poll_val;
+ struct dw_pcie *pci = pcie->pci;
+ struct device *dev = pci->dev;
+
+ val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
+ val |= PCIE_PME_TURNOFF_MSG;
+ writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
+
+ ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val,
+ (poll_val & PCIE_PM_LINKST_IN_L2),
+ 10000, 100000);
+ if (!ret)
+ dev_dbg(dev, "Device entered L23_Ready state\n");
+ else
+ dev_err(dev, "Device failed to enter L23_Ready. 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)
+{
+ qcom_ep_reset_assert(pcie);
+
+ /* Put PHY into POWER DOWN state */
+ phy_power_off(pcie->phy);
+
+ writel(PHY_POWER_DOWN, pcie->parf + PCIE20_PARF_PHY_CTRL);
+
+ if (pcie->cfg->ops->post_deinit)
+ pcie->cfg->ops->post_deinit(pcie);
+
+ /* Disable PCIe clocks and regulators */
+ pcie->cfg->ops->deinit(pcie);
+}
+
+static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
+{
+ int ret;
+ 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 0;
+ }
+
+ 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 0;
+}
+
+/* Resume the PCIe link */
+static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
+{
+ int ret;
+ struct qcom_pcie *pcie = dev_get_drvdata(dev);
+ struct dw_pcie *pci = pcie->pci;
+ struct pcie_port *pp = &pci->pp;
+
+ ret = qcom_pcie_host_init(pp);
+ if (ret) {
+ dev_err(dev, "cannot initialize host\n");
+ return ret;
+ }
+
+ dw_pcie_setup_rc(pp);
+
+ 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;
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops qcom_pcie_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
+};
+
static const struct of_device_id qcom_pcie_match[] = {
{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
@@ -1679,6 +1775,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
Quoting Prasad Malisetty (2022-03-07 10:55:06)
> Add suspend_noirq and resume_noirq callbacks to handle
Nitpick: They're no longer noirq so this should say "Add suspend and
resume callbacks"
> 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.
I think Bjorn A. and Dmitry are trying to avoid this whole parking thing
in another series? Can you take a look at that?
>
> When system resumes, PCIe link will be re-established and
> setup rc settings.
>
> Signed-off-by: Prasad Malisetty <[email protected]>
>
> ---
> Changes since v3:
> - Replaced noirq hooks with normal suspend/resume hooks.
> - Removed local variable and placed in function itself.
>
> Changes since v2:
> - Removed unnecessary variable initializations and comments.
> - Removed platform specific variables declarations.
> - Added MACRO names for the BIT shiftings.
>
> 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 6ab9089..4d29c80 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -48,6 +48,7 @@
> #define PCIE20_PARF_PHY_REFCLK 0x4C
> #define PHY_REFCLK_SSP_EN BIT(16)
> #define PHY_REFCLK_USE_PAD BIT(12)
> +#define PHY_POWER_DOWN 0x1
>
> #define PCIE20_PARF_DBI_BASE_ADDR 0x168
> #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C
> @@ -62,6 +63,8 @@
>
> #define PCIE20_ELBI_SYS_CTRL 0x04
> #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE BIT(0)
> +#define PCIE_PME_TURNOFF_MSG BIT(4)
> +#define PCIE_PM_LINKST_IN_L2 BIT(5)
>
> #define PCIE20_AXI_MSTR_RESP_COMP_CTRL0 0x818
> #define CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K 0x4
> @@ -73,6 +76,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
>
> @@ -1645,6 +1650,97 @@ 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;
> + u32 val, poll_val;
> + struct dw_pcie *pci = pcie->pci;
> + struct device *dev = pci->dev;
> +
> + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> + val |= PCIE_PME_TURNOFF_MSG;
> + writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> +
> + ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val,
> + (poll_val & PCIE_PM_LINKST_IN_L2),
> + 10000, 100000);
> + if (!ret)
> + dev_dbg(dev, "Device entered L23_Ready state\n");
> + else
> + dev_err(dev, "Device failed to enter L23_Ready. 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)
> +{
> + qcom_ep_reset_assert(pcie);
> +
> + /* Put PHY into POWER DOWN state */
> + phy_power_off(pcie->phy);
> +
> + writel(PHY_POWER_DOWN, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
> + if (pcie->cfg->ops->post_deinit)
> + pcie->cfg->ops->post_deinit(pcie);
> +
> + /* Disable PCIe clocks and regulators */
> + pcie->cfg->ops->deinit(pcie);
> +}
> +
> +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
> +{
> + int ret;
> + 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 0;
> + }
> +
> + 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 0;
> +}
> +
> +/* Resume the PCIe link */
> +static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
> +{
> + int ret;
> + struct qcom_pcie *pcie = dev_get_drvdata(dev);
> + struct dw_pcie *pci = pcie->pci;
> + struct pcie_port *pp = &pci->pp;
> +
> + ret = qcom_pcie_host_init(pp);
> + if (ret) {
> + dev_err(dev, "cannot initialize host\n");
Capitalize cannot?
> + return ret;
> + }
> +
> + dw_pcie_setup_rc(pp);
> +
> + 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;
Drop return and braces.
> + }
> +
> + return 0;
return ret;
> +}
> +
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> +};
> +
On Tue, Mar 08, 2022 at 12:25:06AM +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 <[email protected]>
>
> ---
> Changes since v3:
> - Replaced noirq hooks with normal suspend/resume hooks.
> - Removed local variable and placed in function itself.
>
> Changes since v2:
> - Removed unnecessary variable initializations and comments.
> - Removed platform specific variables declarations.
> - Added MACRO names for the BIT shiftings.
>
> 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 6ab9089..4d29c80 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -48,6 +48,7 @@
> #define PCIE20_PARF_PHY_REFCLK 0x4C
> #define PHY_REFCLK_SSP_EN BIT(16)
> #define PHY_REFCLK_USE_PAD BIT(12)
> +#define PHY_POWER_DOWN 0x1
BIT(0)
>
> #define PCIE20_PARF_DBI_BASE_ADDR 0x168
> #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16C
> @@ -62,6 +63,8 @@
>
> #define PCIE20_ELBI_SYS_CTRL 0x04
> #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE BIT(0)
> +#define PCIE_PME_TURNOFF_MSG BIT(4)
> +#define PCIE_PM_LINKST_IN_L2 BIT(5)
>
> #define PCIE20_AXI_MSTR_RESP_COMP_CTRL0 0x818
> #define CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K 0x4
> @@ -73,6 +76,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
>
> @@ -1645,6 +1650,97 @@ 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;
> + u32 val, poll_val;
> + struct dw_pcie *pci = pcie->pci;
> + struct device *dev = pci->dev;
> +
> + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> + val |= PCIE_PME_TURNOFF_MSG;
> + writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> +
> + ret = readl_poll_timeout((pcie->parf + PCIE20_PARF_PM_STTS), poll_val,
> + (poll_val & PCIE_PM_LINKST_IN_L2),
> + 10000, 100000);
> + if (!ret)
> + dev_dbg(dev, "Device entered L23_Ready state\n");
> + else
> + dev_err(dev, "Device failed to enter L23_Ready. 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)
> +{
> + qcom_ep_reset_assert(pcie);
> +
In the modem usecase, the endpoint uses the LINK_DOWN event for freeing up the
resources. If PERST# gets asserted before turing off the PHY, the LINK_DOWN
event will be missed in the endpoint. And moreover, modem cannot free up the
resources in PERST# handler as it is a hard IRQ handler and the cleanup action
might sleep. The deferring also doesn't work because, once PERST# event get's
handled, the host will turn off the refclk and the access to DBI region will
not be allowed.
For this reason, I'd prefer to move this PERST# assertion to the end of the
funtion.
> + /* Put PHY into POWER DOWN state */
> + phy_power_off(pcie->phy);
> +
> + writel(PHY_POWER_DOWN, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
> + if (pcie->cfg->ops->post_deinit)
> + pcie->cfg->ops->post_deinit(pcie);
> +
> + /* Disable PCIe clocks and regulators */
> + pcie->cfg->ops->deinit(pcie);
It is also required to add a 100ms delay here as PERST# reaches quickly before
LINK_DOWN.
/*
* Allow the LINK_DOWN event to reach the endpoint before PERST# assert.
* This is required for resource cleanup in the endpoint for modem usecase.
*/
usleep_range(50000, 100000);
qcom_ep_reset_assert(pcie);
> +}
> +
> +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
> +{
> + int ret;
> + 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");
I think this debug message doesn't add any value, so just skip it.
> + return 0;
> + }
> +
> + ret = qcom_pcie_send_pme_turnoff_msg(pcie);
> + if (ret)
> + return ret;
> +
> + /* Power down the PHY, disable clock and regulators */
You are also asserting PERST#.
> + qcom_pcie_host_disable(pcie);
> +
> + return 0;
> +}
> +
> +/* Resume the PCIe link */
> +static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
> +{
> + int ret;
> + struct qcom_pcie *pcie = dev_get_drvdata(dev);
> + struct dw_pcie *pci = pcie->pci;
> + struct pcie_port *pp = &pci->pp;
> +
> + ret = qcom_pcie_host_init(pp);
> + if (ret) {
> + dev_err(dev, "cannot initialize host\n");
> + return ret;
> + }
> +
> + dw_pcie_setup_rc(pp);
> +
> + qcom_pcie_start_link(pci);
> +
> + ret = dw_pcie_wait_for_link(pci);
> + if (ret) {
> + dev_err(dev, "Link never came up, Resume failed\n");
dw_pcie_wait_for_link() itself prints the error in failure case. So just return
directly.
return dw_pcie_wait_for_link(pci);
Thanks,
Mani
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> +};
> +
> static const struct of_device_id qcom_pcie_match[] = {
> { .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
> { .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> @@ -1679,6 +1775,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 Wed, Apr 13, 2022 at 09:36:30AM +0300, Dmitry Baryshkov wrote:
> On Wed, 13 Apr 2022 at 08:49, Manivannan Sadhasivam
> <[email protected]> wrote:
> >
> > On Tue, Apr 12, 2022 at 01:40:08PM +0300, Kalle Valo wrote:
> > > + ath11k
> > >
> > > Manivannan Sadhasivam <[email protected]> writes:
> > >
> > > > +Kalle, linux-wireless
> > > >
> > > > On Tue, Mar 08, 2022 at 12:25:06AM +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.
> > > >>
> > > >
> > > > Kalle, is this behaviour appropriate for WLAN devices as well? The devices
> > > > will be put into poweroff state (assuming no Vaux provided in D3cold) during
> > > > system suspend.
> > >
> > > ath11k leaves the firmware running during suspend. I don't fully
> > > understand what the patch is doing, but if it cuts the power from the
> > > WLAN chip during suspend that will break ath11k.
> > >
> >
> > Thanks Kalle for the confirmation. Yes the device will be put into D3cold state
> > and that will most likely equal to poweroff state.
>
> Just to remind that ath11k on Qualcomm boards has a separate power
> supply, not directly tied to the PCIe power supply.
>
It may change in the future or on a different OEM setup. Irrespective of that,
this patch sends the PME_Turn_Off event to the ep devices. The devices in turn
will shutdown the internal resources for poweroff/refclk removal. Therefore the
device state shall be lost.
Thanks,
Mani
> > Prasad, you should try to just turn off the host resources like clocks and
> > regulators (not refclk) and let the device be in the default state
> > (D3hot/L{0/1}?) during suspend.
>
>
> --
> With best wishes
> Dmitry