On 29.03.2023 15:02, Manivannan Sadhasivam wrote:
> 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.
I think I asked that question before, but.. Do we know what Windows/macOS do?
Konrad
>
> - 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
>>
>
