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

[Manivannan Sadhasivam]

On Mon, Mar 27, 2023 at 03:29:54PM +0000, Frank Li wrote:
> 
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > Sent: Monday, March 27, 2023 8:38 AM
> > To: lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; robh@xxxxxxxxxx
> > Cc: andersson@xxxxxxxxxx; konrad.dybcio@xxxxxxxxxx;
> > bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-arm-
> > msm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > quic_krichai@xxxxxxxxxxx; johan+linaro@xxxxxxxxxx; steev@xxxxxxxx;
> > mka@xxxxxxxxxxxx; Manivannan Sadhasivam
> > <manivannan.sadhasivam@xxxxxxxxxx>; Dhruva Gole <d-gole@xxxxxx>
> > Subject: [EXT] [PATCH v3 1/1] PCI: qcom: Add support for system suspend
> > and resume
> > 
> > Caution: EXT Email
> > 
> > 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.
> 
> I remember I met similar problem before. It is relate ASPM settings of NVME.
> NVME try to use L1.2 at suspend to save restore time. 
> 
> It should be user decided if PCI enter L1.2( for better resume time) or L2
> For batter power saving.  If NVME disable ASPM,  NVME driver will free
> Msi irq before enter suspend,  so not issue access config space by MSI
> Irq disable function. 
> 

The NVMe driver will only shutdown the device if ASPM is completely disabled in
the kernel. They also take powerdown path for some Intel platforms though. For
others, they keep the device in power on state and expect power saving with
ASPM.

> This is just general comment. It is not specific for this patches.  Many platform
> Will face the similar problem.  Maybe need better solution to handle
> L2/L3 for better power saving in future. 
> 

The only argument I hear from them is that, when the NVMe device gets powered
down during suspend, then it may detoriate the life time of it as the suspend
cycle is going to be high.

- Mani

> Frank Li
>  
> > 
> > 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@xxxxxxxxxxx>
> > Acked-by: Dhruva Gole <d-gole@xxxxxx>
> > Signed-off-by: Manivannan Sadhasivam
> > <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 62 ++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> > b/drivers/pci/controller/dwc/pcie-qcom.c
> > index a232b04af048..f33df536d9be 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,62 @@ 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(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);
> > +       int ret;
> > +
> > +       if (pcie->suspended) {
> > +               ret = qcom_pcie_host_init(&pcie->pci->pp);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               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 +1913,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);
> > --
> > 2.25.1
> 

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