[PATCH 0/1] PCI: qcom: Add support for system suspend and resume

[PATCH 0/1] PCI: qcom: Add support for system suspend and resume

[Manivannan Sadhasivam]

Hello,

This series (a single patch) adds the system suspend and resume support
to the Qualcomm PCIe RC controller.

Background
==========

There were previous attempts [1][2] to add system suspend and resume
support to this driver.

In previous versions, the controller was put into low power mode by turning
OFF the resources even if there were active PCIe devices connected. Thanks
to Qualcomm's internal power topology, the link did not enter L2/L3 state
and the devices were still powered ON. But during very late end of suspend
cycle, kernel tried to disable MSIs of the PCIe devices. This caused access
violations as the resources needed to access the PCIe devices config space
were turned OFF. Series [1] worked around this issue by not accessing the
PCIe config space if the link was down in dw_msi_{un}mask_irq() functions.
But that approach was not accepted.

Then, series [2] implemented the suspend and resume operations using the
syscore framework that disabled the resources at the end of the suspend
cycle. But that approach also did not get much acceptance.

Proposal
========

So the proposal here is to just vote for minimal interconnect bandwidth and
not turn OFF the resources if there are active PCIe devices connected to
the controllers. This avoids the access violation issue during suspend and
also saves some power due to the lower interconnect bandwidth used.

Then if there are no active PCIe devices connected to the controller,
the resources are turned OFF completely and brought back during resume.
This also saves power if there are controllers in a system without any
devices connected.

Testing
=======

This series has been tested on Lenovo Thinkpad X13s.

Dependency
==========

This series depends on the host_deinit() callback patch [3] submitted by
Johan.

Thanks,
Mani

[1] https://lore.kernel.org/linux-pci/1656055682-18817-1-git-send-email-quic_krichai@quicinc.com/
[2] https://lore.kernel.org/linux-pci/1663669347-29308-1-git-send-email-quic_krichai@quicinc.com/
[3] https://lore.kernel.org/linux-pci/20230102174313.GC16638@thinkpad/

Manivannan Sadhasivam (1):
  PCI: qcom: Add support for system suspend and resume

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

-- 
2.25.1

[PATCH 1/1] PCI: qcom: Add support for system suspend and resume

[Manivannan Sadhasivam]

During the system suspend, vote for minimal interconnect bandwidth and
also turn OFF the resources like clock and PHY if there are no active
devices connected to the controller. For the controllers with active
devices, the resources are kept ON as removing the resources will
trigger access violation during the late end of suspend cycle as kernel
tries to access the config space of PCIe devices to mask the MSIs.

Also, it is not desirable to put the link into L2/L3 state as that
implies VDD supply will be removed and the devices may go into powerdown
state. This will affect the lifetime of storage devices like NVMe.

And finally, during resume, turn ON the resources if the controller was
truly suspended (resources OFF) and update the interconnect bandwidth
based on PCIe Gen speed.

Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 5696e327795b..48810f1f2dba 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -227,6 +227,7 @@ struct qcom_pcie {
 	struct gpio_desc *reset;
 	struct icc_path *icc_mem;
 	const struct qcom_pcie_cfg *cfg;
+	bool suspended;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
@@ -1835,6 +1836,52 @@ static int qcom_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int qcom_pcie_suspend_noirq(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	int ret;
+
+	ret = icc_set_bw(pcie->icc_mem, 0, 0);
+	if (ret) {
+		dev_err(pcie->pci->dev, "Failed to set interconnect bandwidth: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Turn OFF the resources only for controllers without active PCIe devices. For controllers
+	 * with active devices, the resources are kept ON and the link is expected to be in L0/L1
+	 * (sub)states.
+	 *
+	 * Turning OFF the resources for controllers with active PCIe devices will trigger access
+	 * violation during the end of the suspend cycle, as kernel tries to access the PCIe devices
+	 * config space for masking MSIs.
+	 *
+	 * Also, it is not desirable to put the link into L2/L3 state as that implies VDD supply
+	 * will be removed and the devices may go into powerdown state. This will affect the
+	 * lifetime of the storage devices like NVMe.
+	 */
+	if (!dw_pcie_link_up(pcie->pci)) {
+		qcom_pcie_host_deinit(&pcie->pci->pp);
+		pcie->suspended = true;
+	}
+
+	return 0;
+}
+
+static int qcom_pcie_resume_noirq(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+
+	if (pcie->suspended) {
+		qcom_pcie_host_init(&pcie->pci->pp);
+		pcie->suspended = false;
+	}
+
+	qcom_pcie_icc_update(pcie);
+
+	return 0;
+}
+
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
 	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
@@ -1870,12 +1917,17 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
 
+static const struct dev_pm_ops qcom_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_suspend_noirq, qcom_pcie_resume_noirq)
+};
+
 static struct platform_driver qcom_pcie_driver = {
 	.probe = qcom_pcie_probe,
 	.remove = qcom_pcie_remove,
 	.driver = {
 		.name = "qcom-pcie",
 		.of_match_table = qcom_pcie_match,
+		.pm = &qcom_pcie_pm_ops,
 	},
 };
 module_platform_driver(qcom_pcie_driver);
-- 
2.25.1

Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume

[Dhruva Gole]

On 03/01/23 13:19, Manivannan Sadhasivam wrote:
> During the system suspend, vote for minimal interconnect bandwidth and
> also turn OFF the resources like clock and PHY if there are no active
> devices connected to the controller. For the controllers with active
> devices, the resources are kept ON as removing the resources will
> trigger access violation during the late end of suspend cycle as kernel
> tries to access the config space of PCIe devices to mask the MSIs.
> 
> Also, it is not desirable to put the link into L2/L3 state as that
> implies VDD supply will be removed and the devices may go into powerdown
> state. This will affect the lifetime of storage devices like NVMe.
> 
> And finally, during resume, turn ON the resources if the controller was
> truly suspended (resources OFF) and update the interconnect bandwidth
> based on PCIe Gen speed.
> 
> Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Nice to have another driver added to the list of system suspend
support!

Acked-by: Dhruva Gole <d-gole@ti.com>

>   drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 5696e327795b..48810f1f2dba 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -227,6 +227,7 @@ struct qcom_pcie {
>   	struct gpio_desc *reset;
>   	struct icc_path *icc_mem;
>   	const struct qcom_pcie_cfg *cfg;qcom_pcie_icc_update
> +	bool suspended;
>   };
>   
>   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> @@ -1835,6 +1836,52 @@ static int qcom_pcie_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static int qcom_pcie_suspend_noirq(struct device *dev)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = icc_set_bw(pcie->icc_mem, 0, 0);
> +	if (ret) {
> +		dev_err(pcie->pci->dev, "Failed to set interconnect bandwidth: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Turn OFF the resources only for controllers without active PCIe devices. For controllers
> +	 * with active devices, the resources are kept ON and the link is expected to be in L0/L1
> +	 * (sub)states.
> +	 *
> +	 * Turning OFF the resources for controllers with active PCIe devices will trigger access
> +	 * violation during the end of the suspend cycle, as kernel tries to access the PCIe devices
> +	 * config space for masking MSIs.
> +	 *
> +	 * Also, it is not desirable to put the link into L2/L3 state as that implies VDD supply
> +	 * will be removed and the devices may go into powerdown state. This will affect the
> +	 * lifetime of the storage devices like NVMe.
> +	 */
> +	if (!dw_pcie_link_up(pcie->pci)) {
> +		qcom_pcie_host_deinit(&pcie->pci->pp);
> +		pcie->suspended = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_resume_noirq(struct device *dev)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +
> +	if (pcie->suspended) {
> +		qcom_pcie_host_init(&pcie->pci->pp);
> +		pcie->suspended = false;
> +	}
> +
> +	qcom_pcie_icc_update(pcie);
> +
> +	return 0;
> +}
> +
>   static const struct of_device_id qcom_pcie_match[] = {
>   	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
>   	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> @@ -1870,12 +1917,17 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
>   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
>   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
>   
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_suspend_noirq, qcom_pcie_resume_noirq)
> +};
> +
>   static struct platform_driver qcom_pcie_driver = {
>   	.probe = qcom_pcie_probe,
>   	.remove = qcom_pcie_remove,
>   	.driver = {
>   		.name = "qcom-pcie",
>   		.of_match_table = qcom_pcie_match,
> +		.pm = &qcom_pcie_pm_ops,
>   	},
>   };
>   module_platform_driver(qcom_pcie_driver);

Out of curiosity, were you able to measure how much power you were able
to save after adding suspend support for PCIe? I don't know if clock
gating really saves much amount of power, but yeah its true that we
can't really cut off the power domain entirely in this case.

-- 
Thanks and Regards,
Dhruva Gole

Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume

[Manivannan Sadhasivam]

On Tue, Jan 03, 2023 at 04:46:11PM +0530, Dhruva Gole wrote:
> 
> 
> On 03/01/23 13:19, Manivannan Sadhasivam wrote:
> > During the system suspend, vote for minimal interconnect bandwidth and
> > also turn OFF the resources like clock and PHY if there are no active
> > devices connected to the controller. For the controllers with active
> > devices, the resources are kept ON as removing the resources will
> > trigger access violation during the late end of suspend cycle as kernel
> > tries to access the config space of PCIe devices to mask the MSIs.
> > 
> > Also, it is not desirable to put the link into L2/L3 state as that
> > implies VDD supply will be removed and the devices may go into powerdown
> > state. This will affect the lifetime of storage devices like NVMe.
> > 
> > And finally, during resume, turn ON the resources if the controller was
> > truly suspended (resources OFF) and update the interconnect bandwidth
> > based on PCIe Gen speed.
> > 
> > Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> 
> Nice to have another driver added to the list of system suspend
> support!
> 
> Acked-by: Dhruva Gole <d-gole@ti.com>
> 
> >   drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
> >   1 file changed, 52 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 5696e327795b..48810f1f2dba 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -227,6 +227,7 @@ struct qcom_pcie {
> >   	struct gpio_desc *reset;
> >   	struct icc_path *icc_mem;
> >   	const struct qcom_pcie_cfg *cfg;qcom_pcie_icc_update
> > +	bool suspended;
> >   };
> >   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> > @@ -1835,6 +1836,52 @@ static int qcom_pcie_remove(struct platform_device *pdev)
> >   	return 0;
> >   }
> > +static int qcom_pcie_suspend_noirq(struct device *dev)
> > +{
> > +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = icc_set_bw(pcie->icc_mem, 0, 0);
> > +	if (ret) {
> > +		dev_err(pcie->pci->dev, "Failed to set interconnect bandwidth: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * Turn OFF the resources only for controllers without active PCIe devices. For controllers
> > +	 * with active devices, the resources are kept ON and the link is expected to be in L0/L1
> > +	 * (sub)states.
> > +	 *
> > +	 * Turning OFF the resources for controllers with active PCIe devices will trigger access
> > +	 * violation during the end of the suspend cycle, as kernel tries to access the PCIe devices
> > +	 * config space for masking MSIs.
> > +	 *
> > +	 * Also, it is not desirable to put the link into L2/L3 state as that implies VDD supply
> > +	 * will be removed and the devices may go into powerdown state. This will affect the
> > +	 * lifetime of the storage devices like NVMe.
> > +	 */
> > +	if (!dw_pcie_link_up(pcie->pci)) {
> > +		qcom_pcie_host_deinit(&pcie->pci->pp);
> > +		pcie->suspended = true;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_resume_noirq(struct device *dev)
> > +{
> > +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > +
> > +	if (pcie->suspended) {
> > +		qcom_pcie_host_init(&pcie->pci->pp);
> > +		pcie->suspended = false;
> > +	}
> > +
> > +	qcom_pcie_icc_update(pcie);
> > +
> > +	return 0;
> > +}
> > +
> >   static const struct of_device_id qcom_pcie_match[] = {
> >   	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
> >   	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> > @@ -1870,12 +1917,17 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
> >   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
> >   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
> > +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> > +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_suspend_noirq, qcom_pcie_resume_noirq)
> > +};
> > +
> >   static struct platform_driver qcom_pcie_driver = {
> >   	.probe = qcom_pcie_probe,
> >   	.remove = qcom_pcie_remove,
> >   	.driver = {
> >   		.name = "qcom-pcie",
> >   		.of_match_table = qcom_pcie_match,
> > +		.pm = &qcom_pcie_pm_ops,
> >   	},
> >   };
> >   module_platform_driver(qcom_pcie_driver);
> 
> Out of curiosity, were you able to measure how much power you were able
> to save after adding suspend support for PCIe? I don't know if clock
> gating really saves much amount of power, but yeah its true that we
> can't really cut off the power domain entirely in this case.
> 

I did not measure the power consumption and I agree that we won't save much
power with setting icc bandwidth to 0. But it is better to have something
than nothing. And in the coming days, I have plans to look into other power
saving measures also.

Thanks,
Mani

> -- 
> Thanks and Regards,
> Dhruva Gole

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

Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume

[Matthias Kaehlcke]

On Thu, Jan 05, 2023 at 07:06:39PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jan 03, 2023 at 04:46:11PM +0530, Dhruva Gole wrote:
> > 
> > 
> > On 03/01/23 13:19, Manivannan Sadhasivam wrote:
> > > During the system suspend, vote for minimal interconnect bandwidth and
> > > also turn OFF the resources like clock and PHY if there are no active
> > > devices connected to the controller. For the controllers with active
> > > devices, the resources are kept ON as removing the resources will
> > > trigger access violation during the late end of suspend cycle as kernel
> > > tries to access the config space of PCIe devices to mask the MSIs.
> > > 
> > > Also, it is not desirable to put the link into L2/L3 state as that
> > > implies VDD supply will be removed and the devices may go into powerdown
> > > state. This will affect the lifetime of storage devices like NVMe.
> > > 
> > > And finally, during resume, turn ON the resources if the controller was
> > > truly suspended (resources OFF) and update the interconnect bandwidth
> > > based on PCIe Gen speed.
> > > 
> > > Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > 
> > Nice to have another driver added to the list of system suspend
> > support!
> > 
> > Acked-by: Dhruva Gole <d-gole@ti.com>
> > 
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
> > >   1 file changed, 52 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 5696e327795b..48810f1f2dba 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -227,6 +227,7 @@ struct qcom_pcie {
> > >   	struct gpio_desc *reset;
> > >   	struct icc_path *icc_mem;
> > >   	const struct qcom_pcie_cfg *cfg;qcom_pcie_icc_update
> > > +	bool suspended;
> > >   };
> > >   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> > > @@ -1835,6 +1836,52 @@ static int qcom_pcie_remove(struct platform_device *pdev)
> > >   	return 0;
> > >   }
> > > +static int qcom_pcie_suspend_noirq(struct device *dev)
> > > +{
> > > +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > +	int ret;
> > > +
> > > +	ret = icc_set_bw(pcie->icc_mem, 0, 0);
> > > +	if (ret) {
> > > +		dev_err(pcie->pci->dev, "Failed to set interconnect bandwidth: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Turn OFF the resources only for controllers without active PCIe devices. For controllers
> > > +	 * with active devices, the resources are kept ON and the link is expected to be in L0/L1
> > > +	 * (sub)states.
> > > +	 *
> > > +	 * Turning OFF the resources for controllers with active PCIe devices will trigger access
> > > +	 * violation during the end of the suspend cycle, as kernel tries to access the PCIe devices
> > > +	 * config space for masking MSIs.
> > > +	 *
> > > +	 * Also, it is not desirable to put the link into L2/L3 state as that implies VDD supply
> > > +	 * will be removed and the devices may go into powerdown state. This will affect the
> > > +	 * lifetime of the storage devices like NVMe.
> > > +	 */
> > > +	if (!dw_pcie_link_up(pcie->pci)) {
> > > +		qcom_pcie_host_deinit(&pcie->pci->pp);
> > > +		pcie->suspended = true;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int qcom_pcie_resume_noirq(struct device *dev)
> > > +{
> > > +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > +
> > > +	if (pcie->suspended) {
> > > +		qcom_pcie_host_init(&pcie->pci->pp);
> > > +		pcie->suspended = false;
> > > +	}
> > > +
> > > +	qcom_pcie_icc_update(pcie);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static const struct of_device_id qcom_pcie_match[] = {
> > >   	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
> > >   	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> > > @@ -1870,12 +1917,17 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
> > >   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
> > >   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
> > > +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> > > +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_suspend_noirq, qcom_pcie_resume_noirq)
> > > +};
> > > +
> > >   static struct platform_driver qcom_pcie_driver = {
> > >   	.probe = qcom_pcie_probe,
> > >   	.remove = qcom_pcie_remove,
> > >   	.driver = {
> > >   		.name = "qcom-pcie",
> > >   		.of_match_table = qcom_pcie_match,
> > > +		.pm = &qcom_pcie_pm_ops,
> > >   	},
> > >   };
> > >   module_platform_driver(qcom_pcie_driver);
> > 
> > Out of curiosity, were you able to measure how much power you were able
> > to save after adding suspend support for PCIe? I don't know if clock
> > gating really saves much amount of power, but yeah its true that we
> > can't really cut off the power domain entirely in this case.
> > 
> 
> I did not measure the power consumption and I agree that we won't save much
> power with setting icc bandwidth to 0. But it is better to have something
> than nothing. And in the coming days, I have plans to look into other power
> saving measures also.

On a sc7280 system I see a reduction of ~30mW with this patch when no PCI
card is plugged in. The reduction seems to come from powering the PHY down.

Interestingly on that system power consumption during suspend (without this
patch) is ~30mW higher *without* a PCI card vs. with a card. Maybe the PHY
doesn't enter a low power mode when no card is plugged in?

Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume

[Manivannan Sadhasivam]

On Fri, Jan 06, 2023 at 06:17:19PM +0000, Matthias Kaehlcke wrote:
> On Thu, Jan 05, 2023 at 07:06:39PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jan 03, 2023 at 04:46:11PM +0530, Dhruva Gole wrote:
> > > 
> > > 
> > > On 03/01/23 13:19, Manivannan Sadhasivam wrote:
> > > > During the system suspend, vote for minimal interconnect bandwidth and
> > > > also turn OFF the resources like clock and PHY if there are no active
> > > > devices connected to the controller. For the controllers with active
> > > > devices, the resources are kept ON as removing the resources will
> > > > trigger access violation during the late end of suspend cycle as kernel
> > > > tries to access the config space of PCIe devices to mask the MSIs.
> > > > 
> > > > Also, it is not desirable to put the link into L2/L3 state as that
> > > > implies VDD supply will be removed and the devices may go into powerdown
> > > > state. This will affect the lifetime of storage devices like NVMe.
> > > > 
> > > > And finally, during resume, turn ON the resources if the controller was
> > > > truly suspended (resources OFF) and update the interconnect bandwidth
> > > > based on PCIe Gen speed.
> > > > 
> > > > Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > 
> > > Nice to have another driver added to the list of system suspend
> > > support!
> > > 
> > > Acked-by: Dhruva Gole <d-gole@ti.com>
> > > 
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
> > > >   1 file changed, 52 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index 5696e327795b..48810f1f2dba 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -227,6 +227,7 @@ struct qcom_pcie {
> > > >   	struct gpio_desc *reset;
> > > >   	struct icc_path *icc_mem;
> > > >   	const struct qcom_pcie_cfg *cfg;qcom_pcie_icc_update
> > > > +	bool suspended;
> > > >   };
> > > >   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> > > > @@ -1835,6 +1836,52 @@ static int qcom_pcie_remove(struct platform_device *pdev)
> > > >   	return 0;
> > > >   }
> > > > +static int qcom_pcie_suspend_noirq(struct device *dev)
> > > > +{
> > > > +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > > +	int ret;
> > > > +
> > > > +	ret = icc_set_bw(pcie->icc_mem, 0, 0);
> > > > +	if (ret) {
> > > > +		dev_err(pcie->pci->dev, "Failed to set interconnect bandwidth: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Turn OFF the resources only for controllers without active PCIe devices. For controllers
> > > > +	 * with active devices, the resources are kept ON and the link is expected to be in L0/L1
> > > > +	 * (sub)states.
> > > > +	 *
> > > > +	 * Turning OFF the resources for controllers with active PCIe devices will trigger access
> > > > +	 * violation during the end of the suspend cycle, as kernel tries to access the PCIe devices
> > > > +	 * config space for masking MSIs.
> > > > +	 *
> > > > +	 * Also, it is not desirable to put the link into L2/L3 state as that implies VDD supply
> > > > +	 * will be removed and the devices may go into powerdown state. This will affect the
> > > > +	 * lifetime of the storage devices like NVMe.
> > > > +	 */
> > > > +	if (!dw_pcie_link_up(pcie->pci)) {
> > > > +		qcom_pcie_host_deinit(&pcie->pci->pp);
> > > > +		pcie->suspended = true;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int qcom_pcie_resume_noirq(struct device *dev)
> > > > +{
> > > > +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > > +
> > > > +	if (pcie->suspended) {
> > > > +		qcom_pcie_host_init(&pcie->pci->pp);
> > > > +		pcie->suspended = false;
> > > > +	}
> > > > +
> > > > +	qcom_pcie_icc_update(pcie);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >   static const struct of_device_id qcom_pcie_match[] = {
> > > >   	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
> > > >   	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> > > > @@ -1870,12 +1917,17 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
> > > >   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
> > > >   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
> > > > +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> > > > +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_suspend_noirq, qcom_pcie_resume_noirq)
> > > > +};
> > > > +
> > > >   static struct platform_driver qcom_pcie_driver = {
> > > >   	.probe = qcom_pcie_probe,
> > > >   	.remove = qcom_pcie_remove,
> > > >   	.driver = {
> > > >   		.name = "qcom-pcie",
> > > >   		.of_match_table = qcom_pcie_match,
> > > > +		.pm = &qcom_pcie_pm_ops,
> > > >   	},
> > > >   };
> > > >   module_platform_driver(qcom_pcie_driver);
> > > 
> > > Out of curiosity, were you able to measure how much power you were able
> > > to save after adding suspend support for PCIe? I don't know if clock
> > > gating really saves much amount of power, but yeah its true that we
> > > can't really cut off the power domain entirely in this case.
> > > 
> > 
> > I did not measure the power consumption and I agree that we won't save much
> > power with setting icc bandwidth to 0. But it is better to have something
> > than nothing. And in the coming days, I have plans to look into other power
> > saving measures also.
> 
> On a sc7280 system I see a reduction of ~30mW with this patch when no PCI
> card is plugged in. The reduction seems to come from powering the PHY down.
> 

Thanks a lot for testing!

> Interestingly on that system power consumption during suspend (without this
> patch) is ~30mW higher *without* a PCI card vs. with a card. Maybe the PHY
> doesn't enter a low power mode when no card is plugged in?

Yeah, both PHY and controllers are never put into low power mode even if there
are no devices connected. I don't know if the low power mode is possible at
all with PHY.

Thanks,
Mani

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

Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume

[Matthias Kaehlcke]

On Sat, Jan 07, 2023 at 12:32:52AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jan 06, 2023 at 06:17:19PM +0000, Matthias Kaehlcke wrote:
> > On Thu, Jan 05, 2023 at 07:06:39PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Jan 03, 2023 at 04:46:11PM +0530, Dhruva Gole wrote:
> > > > 
> > > > 
> > > > On 03/01/23 13:19, Manivannan Sadhasivam wrote:
> > > > > During the system suspend, vote for minimal interconnect bandwidth and
> > > > > also turn OFF the resources like clock and PHY if there are no active
> > > > > devices connected to the controller. For the controllers with active
> > > > > devices, the resources are kept ON as removing the resources will
> > > > > trigger access violation during the late end of suspend cycle as kernel
> > > > > tries to access the config space of PCIe devices to mask the MSIs.
> > > > > 
> > > > > Also, it is not desirable to put the link into L2/L3 state as that
> > > > > implies VDD supply will be removed and the devices may go into powerdown
> > > > > state. This will affect the lifetime of storage devices like NVMe.
> > > > > 
> > > > > And finally, during resume, turn ON the resources if the controller was
> > > > > truly suspended (resources OFF) and update the interconnect bandwidth
> > > > > based on PCIe Gen speed.
> > > > > 
> > > > > Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > ---
> > > > 
> > > > Nice to have another driver added to the list of system suspend
> > > > support!
> > > > 
> > > > Acked-by: Dhruva Gole <d-gole@ti.com>
> > > > 
> > > > >   drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
> > > > >   1 file changed, 52 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > index 5696e327795b..48810f1f2dba 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > @@ -227,6 +227,7 @@ struct qcom_pcie {
> > > > >   	struct gpio_desc *reset;
> > > > >   	struct icc_path *icc_mem;
> > > > >   	const struct qcom_pcie_cfg *cfg;qcom_pcie_icc_update
> > > > > +	bool suspended;
> > > > >   };
> > > > >   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> > > > > @@ -1835,6 +1836,52 @@ static int qcom_pcie_remove(struct platform_device *pdev)
> > > > >   	return 0;
> > > > >   }
> > > > > +static int qcom_pcie_suspend_noirq(struct device *dev)
> > > > > +{
> > > > > +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = icc_set_bw(pcie->icc_mem, 0, 0);
> > > > > +	if (ret) {
> > > > > +		dev_err(pcie->pci->dev, "Failed to set interconnect bandwidth: %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Turn OFF the resources only for controllers without active PCIe devices. For controllers
> > > > > +	 * with active devices, the resources are kept ON and the link is expected to be in L0/L1
> > > > > +	 * (sub)states.
> > > > > +	 *
> > > > > +	 * Turning OFF the resources for controllers with active PCIe devices will trigger access
> > > > > +	 * violation during the end of the suspend cycle, as kernel tries to access the PCIe devices
> > > > > +	 * config space for masking MSIs.
> > > > > +	 *
> > > > > +	 * Also, it is not desirable to put the link into L2/L3 state as that implies VDD supply
> > > > > +	 * will be removed and the devices may go into powerdown state. This will affect the
> > > > > +	 * lifetime of the storage devices like NVMe.
> > > > > +	 */
> > > > > +	if (!dw_pcie_link_up(pcie->pci)) {
> > > > > +		qcom_pcie_host_deinit(&pcie->pci->pp);
> > > > > +		pcie->suspended = true;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int qcom_pcie_resume_noirq(struct device *dev)
> > > > > +{
> > > > > +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > > > +
> > > > > +	if (pcie->suspended) {
> > > > > +		qcom_pcie_host_init(&pcie->pci->pp);
> > > > > +		pcie->suspended = false;
> > > > > +	}
> > > > > +
> > > > > +	qcom_pcie_icc_update(pcie);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >   static const struct of_device_id qcom_pcie_match[] = {
> > > > >   	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
> > > > >   	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> > > > > @@ -1870,12 +1917,17 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
> > > > >   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
> > > > >   DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
> > > > > +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> > > > > +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_suspend_noirq, qcom_pcie_resume_noirq)
> > > > > +};
> > > > > +
> > > > >   static struct platform_driver qcom_pcie_driver = {
> > > > >   	.probe = qcom_pcie_probe,
> > > > >   	.remove = qcom_pcie_remove,
> > > > >   	.driver = {
> > > > >   		.name = "qcom-pcie",
> > > > >   		.of_match_table = qcom_pcie_match,
> > > > > +		.pm = &qcom_pcie_pm_ops,
> > > > >   	},
> > > > >   };
> > > > >   module_platform_driver(qcom_pcie_driver);
> > > > 
> > > > Out of curiosity, were you able to measure how much power you were able
> > > > to save after adding suspend support for PCIe? I don't know if clock
> > > > gating really saves much amount of power, but yeah its true that we
> > > > can't really cut off the power domain entirely in this case.
> > > > 
> > > 
> > > I did not measure the power consumption and I agree that we won't save much
> > > power with setting icc bandwidth to 0. But it is better to have something
> > > than nothing. And in the coming days, I have plans to look into other power
> > > saving measures also.
> > 
> > On a sc7280 system I see a reduction of ~30mW with this patch when no PCI
> > card is plugged in. The reduction seems to come from powering the PHY down.
> > 
> 
> Thanks a lot for testing!
> 
> > Interestingly on that system power consumption during suspend (without this
> > patch) is ~30mW higher *without* a PCI card vs. with a card. Maybe the PHY
> > doesn't enter a low power mode when no card is plugged in?
> 
> Yeah, both PHY and controllers are never put into low power mode even if there
> are no devices connected. I don't know if the low power mode is possible at
> all with PHY.

It's still interesting that the PHY apparently at least enters a *lower* power
mode when a card is plugged in, the extra 30mW are only seen without a card.

Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume

[Johan Hovold]

On Tue, Jan 03, 2023 at 01:19:07PM +0530, Manivannan Sadhasivam wrote:
> During the system suspend, vote for minimal interconnect bandwidth and
> also turn OFF the resources like clock and PHY if there are no active
> devices connected to the controller. For the controllers with active
> devices, the resources are kept ON as removing the resources will
> trigger access violation during the late end of suspend cycle as kernel
> tries to access the config space of PCIe devices to mask the MSIs.
> 
> Also, it is not desirable to put the link into L2/L3 state as that
> implies VDD supply will be removed and the devices may go into powerdown
> state. This will affect the lifetime of storage devices like NVMe.
> 
> And finally, during resume, turn ON the resources if the controller was
> truly suspended (resources OFF) and update the interconnect bandwidth
> based on PCIe Gen speed.
> 
> Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)

I just gave this a quick spin on the sc8280xp-crd, and unfortunately
this change appears to break suspend (e.g. hangs during suspend or
resume). Setting a non-zero (250 MBps) peak bandwidth during suspend
makes things work again.

Presumably something is relying on these interconnect clocks to remain
enabled. And isn't that expected as we need to set a non-zero icc bw to
enable the interconnect clocks during probe?

I'm afraid I won't have time to look into this for a while myself, but
have you tried this on the CRD, Mani? 

One obvious difference is the modem on the CRD which I believe neither
of our X13s have, but this seems like more of a general problem.

Johan

Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume

[Manivannan Sadhasivam]

On Tue, Jan 03, 2023 at 02:16:47PM +0100, Johan Hovold wrote:
> On Tue, Jan 03, 2023 at 01:19:07PM +0530, Manivannan Sadhasivam wrote:
> > During the system suspend, vote for minimal interconnect bandwidth and
> > also turn OFF the resources like clock and PHY if there are no active
> > devices connected to the controller. For the controllers with active
> > devices, the resources are kept ON as removing the resources will
> > trigger access violation during the late end of suspend cycle as kernel
> > tries to access the config space of PCIe devices to mask the MSIs.
> > 
> > Also, it is not desirable to put the link into L2/L3 state as that
> > implies VDD supply will be removed and the devices may go into powerdown
> > state. This will affect the lifetime of storage devices like NVMe.
> > 
> > And finally, during resume, turn ON the resources if the controller was
> > truly suspended (resources OFF) and update the interconnect bandwidth
> > based on PCIe Gen speed.
> > 
> > Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> 
> I just gave this a quick spin on the sc8280xp-crd, and unfortunately
> this change appears to break suspend (e.g. hangs during suspend or
> resume). Setting a non-zero (250 MBps) peak bandwidth during suspend
> makes things work again.
> 
> Presumably something is relying on these interconnect clocks to remain
> enabled. And isn't that expected as we need to set a non-zero icc bw to
> enable the interconnect clocks during probe?
> 

After suspend, I assumed that there won't be any access to the controller
specific registers, so thought it should be fine. And it works on X13s too.
Maybe, the access to device config space is triggering issues on CRD? I will
check with Qcom.

> I'm afraid I won't have time to look into this for a while myself, but
> have you tried this on the CRD, Mani? 
> 

Thanks for testing, Johan!

I did not test this on CRD. Since both X13s and CRD are sharing the same
SoC, I thought it would work on CRD too. But since you have tested and
reported the issue, I will look into it.

> One obvious difference is the modem on the CRD which I believe neither
> of our X13s have, but this seems like more of a general problem.
> 

Yeah, this seems to be a platform issue. I will check on this behaviour and
report back.

Thanks,
Mani

> Johan

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