linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] PCI: qcom: Add system PM support
@ 2022-03-07 18:55 Prasad Malisetty
  2022-03-17 22:07 ` Stephen Boyd
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Prasad Malisetty @ 2022-03-07 18:55 UTC (permalink / raw)
  To: agross, bjorn.andersson, lorenzo.pieralisi, robh, kw, bhelgaas,
	linux-pci, linux-arm-msm, linux-kernel, rajatja, refactormyself
  Cc: quic_vbadigan, quic_ramkri, manivannan.sadhasivam, swboyd,
	Prasad Malisetty

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>

---
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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] PCI: qcom: Add system PM support
  2022-03-07 18:55 [PATCH v4] PCI: qcom: Add system PM support Prasad Malisetty
@ 2022-03-17 22:07 ` Stephen Boyd
  2022-03-18 14:34 ` Manivannan Sadhasivam
  2022-04-12  6:01 ` Manivannan Sadhasivam
  2 siblings, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2022-03-17 22:07 UTC (permalink / raw)
  To: Prasad Malisetty, agross, bhelgaas, bjorn.andersson, kw,
	linux-arm-msm, linux-kernel, linux-pci, lorenzo.pieralisi,
	rajatja, refactormyself, robh
  Cc: quic_vbadigan, quic_ramkri, manivannan.sadhasivam

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 <quic_pmaliset@quicinc.com>
>
> ---
> 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)
> +};
> +

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] PCI: qcom: Add system PM support
  2022-03-07 18:55 [PATCH v4] PCI: qcom: Add system PM support Prasad Malisetty
  2022-03-17 22:07 ` Stephen Boyd
@ 2022-03-18 14:34 ` Manivannan Sadhasivam
  2022-04-12  6:01 ` Manivannan Sadhasivam
  2 siblings, 0 replies; 8+ messages in thread
From: Manivannan Sadhasivam @ 2022-03-18 14:34 UTC (permalink / raw)
  To: Prasad Malisetty
  Cc: agross, bjorn.andersson, lorenzo.pieralisi, robh, kw, bhelgaas,
	linux-pci, linux-arm-msm, linux-kernel, rajatja, refactormyself,
	quic_vbadigan, quic_ramkri, swboyd

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 <quic_pmaliset@quicinc.com>
> 
> ---
> 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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] PCI: qcom: Add system PM support
  2022-03-07 18:55 [PATCH v4] PCI: qcom: Add system PM support Prasad Malisetty
  2022-03-17 22:07 ` Stephen Boyd
  2022-03-18 14:34 ` Manivannan Sadhasivam
@ 2022-04-12  6:01 ` Manivannan Sadhasivam
  2022-04-12 10:40   ` Kalle Valo
  2 siblings, 1 reply; 8+ messages in thread
From: Manivannan Sadhasivam @ 2022-04-12  6:01 UTC (permalink / raw)
  To: Prasad Malisetty
  Cc: agross, bjorn.andersson, lorenzo.pieralisi, robh, kw, bhelgaas,
	linux-pci, linux-arm-msm, linux-kernel, rajatja, refactormyself,
	quic_vbadigan, quic_ramkri, swboyd, kvalo, linux-wireless

+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.

Thanks,
Mani

> When system resumes, PCIe link will be re-established and
> setup rc settings.
> 
> Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com>
> 
> ---
> 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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] PCI: qcom: Add system PM support
  2022-04-12  6:01 ` Manivannan Sadhasivam
@ 2022-04-12 10:40   ` Kalle Valo
  2022-04-13  5:49     ` Manivannan Sadhasivam
  0 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2022-04-12 10:40 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Prasad Malisetty, agross, bjorn.andersson, lorenzo.pieralisi,
	robh, kw, bhelgaas, linux-pci, linux-arm-msm, linux-kernel,
	rajatja, refactormyself, quic_vbadigan, quic_ramkri, swboyd,
	linux-wireless, ath11k

+ ath11k

Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] PCI: qcom: Add system PM support
  2022-04-12 10:40   ` Kalle Valo
@ 2022-04-13  5:49     ` Manivannan Sadhasivam
  2022-04-13  6:36       ` Dmitry Baryshkov
  0 siblings, 1 reply; 8+ messages in thread
From: Manivannan Sadhasivam @ 2022-04-13  5:49 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Prasad Malisetty, agross, bjorn.andersson, lorenzo.pieralisi,
	robh, kw, bhelgaas, linux-pci, linux-arm-msm, linux-kernel,
	rajatja, refactormyself, quic_vbadigan, quic_ramkri, swboyd,
	linux-wireless, ath11k

On Tue, Apr 12, 2022 at 01:40:08PM +0300, Kalle Valo wrote:
> + ath11k
> 
> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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.

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.

Thanks,
Mani

> -- 
> https://patchwork.kernel.org/project/linux-wireless/list/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] PCI: qcom: Add system PM support
  2022-04-13  5:49     ` Manivannan Sadhasivam
@ 2022-04-13  6:36       ` Dmitry Baryshkov
  2022-04-13  7:05         ` Manivannan Sadhasivam
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2022-04-13  6:36 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Kalle Valo, Prasad Malisetty, agross, bjorn.andersson,
	lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-arm-msm,
	linux-kernel, rajatja, refactormyself, quic_vbadigan,
	quic_ramkri, swboyd, linux-wireless, ath11k

On Wed, 13 Apr 2022 at 08:49, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Apr 12, 2022 at 01:40:08PM +0300, Kalle Valo wrote:
> > + ath11k
> >
> > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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.

> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] PCI: qcom: Add system PM support
  2022-04-13  6:36       ` Dmitry Baryshkov
@ 2022-04-13  7:05         ` Manivannan Sadhasivam
  0 siblings, 0 replies; 8+ messages in thread
From: Manivannan Sadhasivam @ 2022-04-13  7:05 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Kalle Valo, Prasad Malisetty, agross, bjorn.andersson,
	lorenzo.pieralisi, robh, kw, bhelgaas, linux-pci, linux-arm-msm,
	linux-kernel, rajatja, refactormyself, quic_vbadigan,
	quic_ramkri, swboyd, linux-wireless, ath11k

On Wed, Apr 13, 2022 at 09:36:30AM +0300, Dmitry Baryshkov wrote:
> On Wed, 13 Apr 2022 at 08:49, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Apr 12, 2022 at 01:40:08PM +0300, Kalle Valo wrote:
> > > + ath11k
> > >
> > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-04-13  7:06 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-07 18:55 [PATCH v4] PCI: qcom: Add system PM support Prasad Malisetty
2022-03-17 22:07 ` Stephen Boyd
2022-03-18 14:34 ` Manivannan Sadhasivam
2022-04-12  6:01 ` Manivannan Sadhasivam
2022-04-12 10:40   ` Kalle Valo
2022-04-13  5:49     ` Manivannan Sadhasivam
2022-04-13  6:36       ` Dmitry Baryshkov
2022-04-13  7:05         ` Manivannan Sadhasivam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).