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

From: Manivannan Sadhasivam
Date: Mon Sep 12 2022 - 13:09:58 EST

Next message: Rob Herring: "Re: [PATCH v3 03/10] dt-bindings: misc: fastrpc: Document memory-region property"
Previous message: Rob Herring: "Re: [PATCH v3 00/10] misc: fastrpc: Add audiopd support"
In reply to: Matthias Kaehlcke: "Re: [PATCH v6 1/5] PCI: qcom: Add system suspend and resume support"
Next in thread: Krishna chaitanya chundru: "[PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Sep 09, 2022 at 05:31:18PM +0000, Matthias Kaehlcke wrote:
> On Fri, Sep 09, 2022 at 02:14:40PM +0530, Krishna chaitanya chundru wrote:
> > Add suspend and resume syscore ops.
> >
> > When system suspends and if the link is in L1ss, disable the clocks
> > and power down the phy so that system enters into low power state to
> > save the maximum power. And when the system resumes, enable the clocks
> > back and power on phy if they are disabled in the suspend path.
> >
> > we are doing this only when link is in l1ss but not in L2/L3 as
> > nowhere we are forcing link to L2/L3 by sending PME turn off.
> >
> > is_suspended flag indicates if the clocks are disabled in the suspend
> > path or not.
> >
> > There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> > (getting hit by affinity changes while making CPUs offline during suspend,
> > this will happen after devices are suspended (all phases of suspend ops)).
> > When registered with pm ops there is a crash due to un-clocked access,
> > as in the pm suspend op clocks are disabled. So, registering with syscore
> > ops which will called after making CPUs offline.
>
> My knowledge of PCI is limited, but given the issues you are seeing which
> don't seem to impact other DWC drivers I wonder if keeping the link in
> l1ss is the right thing to do. The intel, tegra and imx6 drivers all turn
> the PME off, which IIUC results in the link to transition ot L2 or L3.
> Shouldn't the QC driver do the same?

Some PCI endpoint drivers (NVMe, WLAN) expects the device to be powered ON
during system suspend. So transitioning the link to L2/L3 will put them in
power down mode (if Vaux is not supported) and will break those drivers during
resume.

IIRC, Tegra is also facing this issue with NVMe.

Thanks,
Mani

>
> Some more comments inline, for if the current approach moves forward.
>
> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx>
> > ---
> > changes since v5:
> > - Rebasing the code and replaced pm ops with syscore ops as
> > we are getting acciess to pci region after pm ops. syscore ops
> > will called after disabling non boot cpus and there is no pci
> > access after that.
> > Changes since v4:
> > - Rebasing the code and removed the supports_system_suspend flag
> > - in the resume path as is_suspended will serve its purpose.
> > Changes since v3:
> > - Powering down the phy in suspend and powering it on resume to
> > acheive maximum power savings.
> > Changes since v2:
> > - Replaced the enable, disable clks ops with suspend and resume
> > - Renamed support_pm_opsi flag with supports_system_suspend.
> > Changes since v1:
> > - Fixed compilation errors.
> > ---
> > drivers/pci/controller/dwc/pcie-qcom.c | 140 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 139 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 39ca06f..6e04d0d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -27,6 +27,7 @@
> > #include <linux/reset.h>
> > #include <linux/slab.h>
> > #include <linux/types.h>
> > +#include <linux/syscore_ops.h>
> >
> > #include "../../pci.h"
> > #include "pcie-designware.h"
> > @@ -44,6 +45,9 @@
> > #define PCIE20_PARF_PM_CTRL 0x20
> > #define REQ_NOT_ENTR_L1 BIT(5)
> >
> > +#define PCIE20_PARF_PM_STTS 0x24
> > +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB BIT(8)
> > +
> > #define PCIE20_PARF_PHY_CTRL 0x40
> > #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK GENMASK(20, 16)
> > #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) ((x) << 16)
> > @@ -122,6 +126,8 @@
> >
> > #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
> >
> > +static LIST_HEAD(qcom_pcie_list);
> > +
> > struct qcom_pcie_resources_2_1_0 {
> > struct clk_bulk_data clks[QCOM_PCIE_2_1_0_MAX_CLOCKS];
> > struct reset_control *pci_reset;
> > @@ -211,13 +217,21 @@ struct qcom_pcie_ops {
> > void (*post_deinit)(struct qcom_pcie *pcie);
> > void (*ltssm_enable)(struct qcom_pcie *pcie);
> > int (*config_sid)(struct qcom_pcie *pcie);
> > + int (*suspend)(struct qcom_pcie *pcie);
> > + int (*resume)(struct qcom_pcie *pcie);
> > };
> >
> > struct qcom_pcie_cfg {
> > const struct qcom_pcie_ops *ops;
> > + /*
> > + * Flag ensures which devices will turn off clks, phy
> > + * in system suspend.
> > + */
> > + unsigned int supports_system_suspend:1;
> > };
> >
> > struct qcom_pcie {
> > + struct list_head list; /* list to probed instances */
> > struct dw_pcie *pci;
> > void __iomem *parf; /* DT parf */
> > void __iomem *elbi; /* DT elbi */
> > @@ -225,10 +239,14 @@ struct qcom_pcie {
> > struct phy *phy;
> > struct gpio_desc *reset;
> > const struct qcom_pcie_cfg *cfg;
> > + unsigned int is_suspended:1;
> > };
> >
> > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
> >
> > +static int __maybe_unused qcom_pcie_syscore_op_suspend(void);
> > +static void __maybe_unused qcom_pcie_syscore_op_resume(void);
> > +
> > static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > {
> > gpiod_set_value_cansleep(pcie->reset, 1);
> > @@ -1301,6 +1319,28 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > }
> >
> > +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
> > +{
> > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > + int ret;
> > +
> > + ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > +
> > + phy_power_on(pcie->phy);
> > +
> > + return ret;
> > +}
> > +
> > +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
> > +{
> > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > +
> > + phy_power_off(pcie->phy);
> > +
> > + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > + return 0;
> > +}
> > +
> > static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> > {
> > struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > @@ -1594,6 +1634,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> > .deinit = qcom_pcie_deinit_2_7_0,
> > .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > .config_sid = qcom_pcie_config_sid_sm8250,
> > + .suspend = qcom_pcie_suspend_2_7_0,
> > + .resume = qcom_pcie_resume_2_7_0,
> > };
> >
> > /* Qcom IP rev.: 2.9.0 Synopsys IP rev.: 5.00a */
> > @@ -1613,6 +1655,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
> > .ops = &ops_1_9_0,
> > };
> >
> > +static const struct qcom_pcie_cfg sc7280_cfg = {
> > + .ops = &ops_1_9_0,
> > + .supports_system_suspend = true,
> > +};
> > +
> > static const struct qcom_pcie_cfg cfg_2_1_0 = {
> > .ops = &ops_2_1_0,
> > };
> > @@ -1642,6 +1689,23 @@ static const struct dw_pcie_ops dw_pcie_ops = {
> > .start_link = qcom_pcie_start_link,
> > };
> >
> > +/*
> > + * There is access to Ep PCIe space to mask MSI/MSIX after pm suspend
> > + * ops.(getting hit by affinity changes while making CPUs offline during
> > + * suspend, this will happen after devices are suspended
> > + * (all phases of suspend ops)).
> > + *
> > + * When registered with pm ops there is a crash due to un-clocked access,
> > + * as in the pm suspend op clocks are disabled.
> > + *
> > + * So, registering with syscore ops which will called after making
> > + * CPU's offline.
> > + */
> > +static struct syscore_ops qcom_pcie_syscore_ops = {
> > + .suspend = qcom_pcie_syscore_op_suspend,
> > + .resume = qcom_pcie_syscore_op_resume,
> > +};
> > +
> > static int qcom_pcie_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > @@ -1720,6 +1784,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > goto err_phy_exit;
> > }
> >
> > + /* Register for syscore ops only when first instance probed */
> > + if (list_empty(&qcom_pcie_list))
> > + register_syscore_ops(&qcom_pcie_syscore_ops);
> > +
> > + /*
> > + * Add the qcom_pcie list of each PCIe instance probed to
> > + * the global list so that we use it iterate through each PCIe
> > + * instance in the syscore ops.
> > + */
> > + list_add_tail(&pcie->list, &qcom_pcie_list);
> > +
> > return 0;
> >
> > err_phy_exit:
> > @@ -1731,6 +1806,69 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > +static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
> > +{
> > + u32 val;
> > + struct dw_pcie *pci = pcie->pci;
> > + struct device *dev = pci->dev;
> > +
> > + if (!pcie->cfg->supports_system_suspend)
> > + return 0;
>
> This check could be done in qcom_pcie_syscore_op_suspend()
>
> > +
> > + /* if the link is not active turn off clocks */
> > + if (!dw_pcie_link_up(pci)) {
> > + dev_info(dev, "Link is not active\n");
>
> level info seems to verbose, it's not particularly interesting that the
> link is not active, except for debugging.
>
> > + goto suspend;
> > + }
> > +
> > + /* if the link is not in l1ss don't turn off clocks */
> > + val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > + if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > + dev_warn(dev, "Link is not in L1ss\n");
> > + return 0;
> > + }
>
> I think the following would be clearer:
>
> if (dw_pcie_link_up(pci)) {
> /* if the link is not in l1ss don't turn off clocks */
> val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> dev_warn(dev, "Link is not in L1ss\n");
> return 0;
> }
> } else {
> dev_dbg(dev, "Link is not active\n");
> }
>
> > +
> > +suspend:
> > + if (pcie->cfg->ops->suspend)
> > + pcie->cfg->ops->suspend(pcie);
> > +
> > + pcie->is_suspended = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused qcom_pcie_pm_resume(struct qcom_pcie *pcie)
> > +{
> > + if (!pcie->is_suspended)
> > + return 0;
> > +
> > + if (pcie->cfg->ops->resume)
> > + pcie->cfg->ops->resume(pcie);
> > +
> > + pcie->is_suspended = false;
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused qcom_pcie_syscore_op_suspend(void)
> > +{
> > + struct qcom_pcie *qcom_pcie;
> > +
> > + list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> > + qcom_pcie_pm_suspend(qcom_pcie);
> > + }
> > + return 0;
> > +}
> > +
> > +static void __maybe_unused qcom_pcie_syscore_op_resume(void)
> > +{
> > + struct qcom_pcie *qcom_pcie;
> > +
> > + list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> > + qcom_pcie_pm_resume(qcom_pcie);
> > + }
> > +}
> > +
> > 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 },
> > @@ -1742,7 +1880,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> > { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
> > { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
> > { .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> > - { .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
> > + { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> > { .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
> > { .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
> > { .compatible = "qcom,pcie-sdm845", .data = &cfg_2_7_0 },
> > --
> > 2.7.4
> >

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

Next message: Rob Herring: "Re: [PATCH v3 03/10] dt-bindings: misc: fastrpc: Document memory-region property"
Previous message: Rob Herring: "Re: [PATCH v3 00/10] misc: fastrpc: Add audiopd support"
In reply to: Matthias Kaehlcke: "Re: [PATCH v6 1/5] PCI: qcom: Add system suspend and resume support"
Next in thread: Krishna chaitanya chundru: "[PATCH v6 2/5] PCI: qcom: Add retry logic for link to be stable in L1ss"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]