[PATCH v2 0/3] PCI: Notify PCI drivers about powerdown during suspend

[PATCH v2 0/3] PCI: Notify PCI drivers about powerdown during suspend

[Manivannan Sadhasivam]

Hi,

This series adds support for notifying the PCI drivers like NVMe about the
transition of PCI devices into powerdown mode during system suspend.

Background
----------

On Qcom SC7280 based Chrome platforms, the RPMh will turn off the power to all
PCIe devices during system suspend for aggressive powersaving. Currently, there
is no way for the PCI device drivers to learn about this situation. Some of the
drivers assume that the power will be retained and some others assume that the
power may be taken down.

We faced the issue with NVMe PCI driver, where the driver expects the NVMe
device to be in APST (Autonomous Power State Transition) state for power saving
during system suspend. So when the power goes down, the NVMe driver fails to
bringup the device during resume.

Previous work
-------------

We tried to fix this issue in a couple of ways:

1. The NVMe PCI driver checks for the existence of "StorageD3Enable" ACPI
property in the suspend path. If the property is found, the driver assumes that
the device may go to poweroff state and shutdowns the device accordingly.

As like the ACPI based systems, we also tried to get the support in place for
DT based systems. But that didn't get accepted:
https://lore.kernel.org/all/Yl+6V3pWuyRYuVV8@infradead.org/T/

2. Keith Busch proposed a module params based approach. The parameter when set,
will allow the driver to support APST during suspend. Absence of that parameter
will let the driver shutdown the device.

This also did not get accepted:
https://lore.kernel.org/linux-nvme/20220201165006.3074615-1-kbusch@kernel.org/

Proposal
--------

Christoph suggested to add a notification in the PCI/PM core to let the NVMe
driver know that the device will go into powerdown state during suspend.
https://lore.kernel.org/all/Yg0wklcJ3ed76Jbk@infradead.org/

Hence in this series, a "suspend_poweroff" flag is introduced in the host bridge
struct. When this flag is set by the PCI RC drivers, the PCI device driver like
NVMe can shutdown the device during suspend.

In the coming days, the usage of this flag could be extended to other PCI
drivers as well.

In this series, the system suspend/resume support is also added to the Qcom
PCIe RC driver for SC7280. During the suspend time, the RC driver will put the
device into D3cold and recover it during resume. So even though RPMh is cutting
off the power to PCIe domain, it is necessary to put the device in D3cold by
the PCIe RC driver for proper working.

Testing
-------

This series has been tested on SC7280 IDP board connected to a NVMe PCI device.

Thanks,
Mani

Manivannan Sadhasivam (2):
  PCI: Add a flag to notify PCI drivers about powerdown during suspend
  nvme-pci: Make use of "suspend_poweroff" flag during system suspend

Prasad Malisetty (1):
  PCI: qcom: Add system PM support

 drivers/nvme/host/pci.c                |   3 +-
 drivers/pci/controller/dwc/pcie-qcom.c | 108 +++++++++++++++++++++++++
 include/linux/pci.h                    |   2 +
 3 files changed, 112 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v2 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend

[Manivannan Sadhasivam]

On some systems like Chromebooks based on Qcom SC7280, the PCIe RC
driver or a hardware entity like RPMh may powerdown all PCIe devices
during system suspend for aggressive powersaving. In that case, the
PCI host controller drivers need to notify the PCI device drivers that
the power will be taken off during system suspend so that the drivers
can prepare the devices accordingly.

One prime example is the PCI NVMe driver. This flag can be used by the
driver to shutdown the NVMe device during suspend and recover it during
resume.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

Changes in v2:

* Changed the comment for the flag to mention the usecase.
* Reworded the commit message to convey how the poweroff happens

 include/linux/pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 60adf42460ab..3e3a1c4f4559 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -578,6 +578,8 @@ struct pci_host_bridge {
 	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
 	unsigned int	size_windows:1;		/* Enable root bus sizing */
 	unsigned int	msi_domain:1;		/* Bridge wants MSI domain */
+	unsigned int	suspend_poweroff:1;	/* Some platforms like Qcom SC7280 may poweroff
+						   devices during system suspend for power saving */
 
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
-- 
2.25.1

[PATCH v2 2/3] nvme-pci: Make use of "suspend_poweroff" flag during system suspend

[Manivannan Sadhasivam]

On some platforms like Chromebooks based on Qcom SC7280, the power to the
PCI devices will be taken off during system suspend. For these platforms,
the PCI RC will set the "system_poweroff" flag to notify the PCI device
drivers of the poweroff scenario.

Hence, make use of the flag in the system suspend path and if set, properly
shutdown the device.

Acked-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

Changes in v2:

* Added Ack from Christoph
* Reworded the commit message to include SC7280

 drivers/nvme/host/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d817ca17463e..381bf0c7cf8d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3238,6 +3238,7 @@ static int nvme_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct nvme_dev *ndev = pci_get_drvdata(pdev);
+	struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
 	struct nvme_ctrl *ctrl = &ndev->ctrl;
 	int ret = -EBUSY;
 
@@ -3257,7 +3258,7 @@ static int nvme_suspend(struct device *dev)
 	 * state (which may not be possible if the link is up).
 	 */
 	if (pm_suspend_via_firmware() || !ctrl->npss ||
-	    !pcie_aspm_enabled(pdev) ||
+	    !pcie_aspm_enabled(pdev) || bridge->suspend_poweroff ||
 	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND))
 		return nvme_disable_prepare_reset(ndev, true);
 
-- 
2.25.1

[PATCH v2 3/3] PCI: qcom: Add system PM support

[Manivannan Sadhasivam]

From: Prasad Malisetty <quic_pmaliset@quicinc.com>

Add suspend and resume callbacks to handle system suspend and resume in
the Qcom PCIe controller driver. When the system suspends, PME turnoff
message will be sent to the device and the RC driver will wait for the
device to enter L23 Ready state. After that, the PHY will be powered down
and clocks/regulators will be disabled.

When the system resumes, RC driver gets initialized and re-establishes the
link.

The system suspend and resume are only handled when the underlying platform
supports the "suspend_poweroff" flag that notifies the PCI drivers about
powerdown during suspend.

Otherwise, these callbacks are NO-OPs. Currently, this flag is only enabled
for SC7280 SoC, where aggressive powersaving is required during system
suspend. It should be noted that on this platform, RPMh will also cutoff
the power to PCIe domain during suspend, independent of the RC driver as
rest of the RPMh clients de-vote for the resources. But ideally, the PCIe
devices should be put into D3cold state during suspend by the PCIe RC
driver and that's what this commit also does.

Signed-off-by: Prasad Malisetty <quic_pmaliset@quicinc.com>
[mani: Reworded commit message, removed pipe_clk & added "suspend_poweroff" flag]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

Changes in v2:

* Picked the suspend/resume patch from Prasad and squashed the suspend_poweroff
  patch
* Rebased on top of pipe_clk handling series from Dmitry
  https://lore.kernel.org/all/20220513175339.2981959-1-dmitry.baryshkov@linaro.org/

 drivers/pci/controller/dwc/pcie-qcom.c | 108 +++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index b48c899bcc97..bfcc79058b3f 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
 
@@ -193,6 +198,7 @@ struct qcom_pcie_cfg {
 	unsigned int has_ddrss_sf_tbu_clk:1;
 	unsigned int has_aggre0_clk:1;
 	unsigned int has_aggre1_clk:1;
+	unsigned int suspend_poweroff:1;
 };
 
 struct qcom_pcie {
@@ -1171,6 +1177,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
+	/* Indicate PCI device drivers that the power will be taken off during system suspend */
+	if (pcie->cfg->suspend_poweroff)
+		pci->pp.bridge->suspend_poweroff = true;
+
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)
 		goto err_disable_regulators;
@@ -1467,6 +1477,7 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
 static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
+	.suspend_poweroff = true,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
@@ -1564,6 +1575,102 @@ 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);
+
+	/* Disable PCIe clocks and regulators */
+	pcie->cfg->ops->deinit(pcie);
+}
+
+static int qcom_pcie_pm_suspend(struct device *dev)
+{
+	int ret;
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	struct dw_pcie *pci = pcie->pci;
+
+	/* If the platform doesn't support powerdown during suspend, just return */
+	if (!pcie->cfg->suspend_poweroff)
+		return 0;
+
+	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 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;
+
+	/* If the platform doesn't support powerdown during suspend, just return */
+	if (!pcie->cfg->suspend_poweroff)
+		return 0;
+
+	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 },
@@ -1600,6 +1707,7 @@ static struct platform_driver qcom_pcie_driver = {
 		.name = "qcom-pcie",
 		.suppress_bind_attrs = true,
 		.of_match_table = qcom_pcie_match,
+		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
 	},
 };
 builtin_platform_driver(qcom_pcie_driver);
-- 
2.25.1

Re: [PATCH v2 3/3] PCI: qcom: Add system PM support

[Christoph Hellwig]

On Wed, May 18, 2022 at 06:49:13PM +0530, Manivannan Sadhasivam wrote:
> From: Prasad Malisetty <quic_pmaliset@quicinc.com>
> 
> Add suspend and resume callbacks to handle system suspend and resume in
> the Qcom PCIe controller driver. When the system suspends, PME turnoff
> message will be sent to the device and the RC driver will wait for the
> device to enter L23 Ready state. After that, the PHY will be powered down
> and clocks/regulators will be disabled.

So what about just not doing this stupid power disabling to start
with?  Unlike x86 where we do not have choice due to the BIOS, we
apparently do here.  And disabling power is the wrong thing to do at
least for SSDs as it massively increases the wear on the NAND.

Re: [PATCH v2 3/3] PCI: qcom: Add system PM support

[Manivannan Sadhasivam]

On Wed, May 18, 2022 at 03:23:58PM +0200, Christoph Hellwig wrote:
> On Wed, May 18, 2022 at 06:49:13PM +0530, Manivannan Sadhasivam wrote:
> > From: Prasad Malisetty <quic_pmaliset@quicinc.com>
> > 
> > Add suspend and resume callbacks to handle system suspend and resume in
> > the Qcom PCIe controller driver. When the system suspends, PME turnoff
> > message will be sent to the device and the RC driver will wait for the
> > device to enter L23 Ready state. After that, the PHY will be powered down
> > and clocks/regulators will be disabled.
> 
> So what about just not doing this stupid power disabling to start
> with?  Unlike x86 where we do not have choice due to the BIOS, we
> apparently do here.  And disabling power is the wrong thing to do at
> least for SSDs as it massively increases the wear on the NAND.

That's the requirement the Chromebook based on SC7280 has. I will check
internally and get back.

Thanks,
Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 0/3] PCI: Notify PCI drivers about powerdown during suspend

[Rafael J. Wysocki]

On Wed, May 18, 2022 at 3:19 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Hi,
>
> This series adds support for notifying the PCI drivers like NVMe about the
> transition of PCI devices into powerdown mode during system suspend.

What kind of system suspend are you referring to?  Suspend-to-idle or
suspend-to-RAM?

For suspend-to-RAM this series should not be necessary at all, because
drivers cannot assume that the state of devices will be preserved
anyway in this case.

For suspend-to-idle, drivers can expect the preservation of the device
state, so perhaps the firmware should be prevented from powering them
off in the suspend-to-idle path instead.