| Message ID | 20230302083625.188482-2-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: qcom: Add support for system suspend and resume | expand |
On Thu, Mar 02, 2023 at 02:06:25PM +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> > Acked-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 53 ++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index a232b04af048..7147f0103026 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) > @@ -1820,6 +1821,53 @@ static int qcom_pcie_probe(struct platform_device *pdev) > return ret; > } > > +static int qcom_pcie_suspend_noirq(struct device *dev) > +{ > + struct qcom_pcie *pcie = dev_get_drvdata(dev); > + int ret; > + > + /* Set minimum bandwidth required to keep data path functional during suspend */ > + ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250)); I can again confirm that this now passes basic smoke testing on my CRD (unlike v1 which set bw to 0) and X13s. > + if (ret) { > + dev_err(pcie->pci->dev, "Failed to set interconnect bandwidth: %d\n", ret); You already have a pointer to dev. > + return ret; > + } And please try to stay within 80 columns width unless breaking it improves readability (generally does not apply comments). > + > + /* > + * 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. > + */ Same here. > + 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); Looks like error handling is missing here. > + 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 }, > @@ -1856,12 +1904,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, > .driver = { > .name = "qcom-pcie", > .suppress_bind_attrs = true, > .of_match_table = qcom_pcie_match, > + .pm = &qcom_pcie_pm_ops, > }, > }; > builtin_platform_driver(qcom_pcie_driver); Johan
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index a232b04af048..7147f0103026 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) @@ -1820,6 +1821,53 @@ static int qcom_pcie_probe(struct platform_device *pdev) return ret; } +static int qcom_pcie_suspend_noirq(struct device *dev) +{ + struct qcom_pcie *pcie = dev_get_drvdata(dev); + int ret; + + /* Set minimum bandwidth required to keep data path functional during suspend */ + ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250)); + 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 }, @@ -1856,12 +1904,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, .driver = { .name = "qcom-pcie", .suppress_bind_attrs = true, .of_match_table = qcom_pcie_match, + .pm = &qcom_pcie_pm_ops, }, }; builtin_platform_driver(qcom_pcie_driver);