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

[Frank Li]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Sent: Wednesday, March 29, 2023 8:03 AM
> To: Frank Li <frank.li@xxxxxxx>
> Cc: lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; robh@xxxxxxxxxx;
> 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; Dhruva Gole <d-gole@xxxxxx>
> Subject: Re: [EXT] [PATCH v3 1/1] PCI: qcom: Add support for system suspend
> and resume
> 
> Caution: EXT Email
> 
> 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.

It appears that not every device is compatible with L1.2 ASPM.

The PCI controller driver should manage this situation by transitioning devices to L2/3
when the system is suspended. However, I am unsure of the appropriate method for
handling this case.. 

> 
> > 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
> >
> 
> --
> மணிவண்ணன் சதாசிவம்