* [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend @ 2022-05-13 11:00 Manivannan Sadhasivam 2022-05-13 11:00 ` [PATCH 1/3] PCI: Add a flag to notify " Manivannan Sadhasivam ` (4 more replies) 0 siblings, 5 replies; 17+ messages in thread From: Manivannan Sadhasivam @ 2022-05-13 11:00 UTC (permalink / raw) To: bhelgaas, lorenzo.pieralisi, kbusch, hch Cc: linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi, Manivannan Sadhasivam Hi, This series adds support for notifying the PCI drivers like NVMe about the transition of PCI devices into powerdown mode during system suspend. Background ---------- On Qcom SC7280 based Chrome platforms, the OS will turn off the power to all PCIe devices during system suspend for aggressive powersaving. Currently, there is no way for the PCI device drivers to learn about this situation. Some of the drivers assume that the power will be retained and some others assume that the power may be taken down. We faced the issue with NVMe PCI driver, where the driver expects the NVMe device to be in APST (Autonomous Power State Transition) state for power saving during system suspend. So when the power goes down, the NVMe driver fails to bringup the device during resume. Previous work ------------- We tried to fix this issue in a couple of ways: 1. The NVMe PCI driver checks for the existence of "StorageD3Enable" ACPI property in the suspend path. If the property is found, the driver assumes that the device may go to poweroff state and shutdowns the device accordingly. As like the ACPI based systems, we also tried to get the support in place for DT based systems. But that didn't get accepted: https://lore.kernel.org/all/Yl+6V3pWuyRYuVV8@infradead.org/T/ 2. Keith Busch proposed a module params based approach. The parameter when set, will allow the driver to support APST during suspend. Absence of that parameter will let the driver shutdown the device. This also did not get accepted: https://lore.kernel.org/linux-nvme/20220201165006.3074615-1-kbusch@kernel.org/ Proposal -------- Christoph suggested to add a notification in the PCI/PM core to let the NVMe driver know that the device will go into powerdown state during suspend. https://lore.kernel.org/all/Yg0wklcJ3ed76Jbk@infradead.org/ Hence in this series, a "suspend_poweroff" flag is introduced in the host bridge struct. When this flag is set by the PCI RC drivers, the PCI device driver like NVMe can shutdown the device during suspend. In the coming days, the usage of this flag could be extended to other PCI drivers as well. Testing ------- This series has been tested on SC7280 IDP board connected to a NVMe PCI device. Thanks, Mani Manivannan Sadhasivam (3): PCI: Add a flag to notify PCI drivers about powerdown during suspend PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 nvme-pci: Make use of "suspend_poweroff" flag during system suspend drivers/nvme/host/pci.c | 3 ++- drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++ include/linux/pci.h | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend 2022-05-13 11:00 [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Manivannan Sadhasivam @ 2022-05-13 11:00 ` Manivannan Sadhasivam 2022-05-16 20:18 ` Bjorn Helgaas 2022-05-13 11:00 ` [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 Manivannan Sadhasivam ` (3 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Manivannan Sadhasivam @ 2022-05-13 11:00 UTC (permalink / raw) To: bhelgaas, lorenzo.pieralisi, kbusch, hch Cc: linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi, Manivannan Sadhasivam On some systems like Chromebooks based on Qcom chipsets, the OS may powerdown all PCIe devices during system suspend for aggressive powersaving. In that case, the PCI host controller drivers need to notify the PCI device drivers that the power will be taken off during system suspend so that the drivers can prepare the devices accordingly. One prime example is the PCI NVMe driver. This flag can be used by the driver to shutdown the NVMe device during suspend and recover it during resume. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- include/linux/pci.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/pci.h b/include/linux/pci.h index 60adf42460ab..069caf1fe88d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -578,6 +578,7 @@ struct pci_host_bridge { unsigned int preserve_config:1; /* Preserve FW resource setup */ unsigned int size_windows:1; /* Enable root bus sizing */ unsigned int msi_domain:1; /* Bridge wants MSI domain */ + unsigned int suspend_poweroff:1; /* OS may poweroff devices during system suspend */ /* Resource alignment requirements */ resource_size_t (*align_resource)(struct pci_dev *dev, -- 2.25.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend 2022-05-13 11:00 ` [PATCH 1/3] PCI: Add a flag to notify " Manivannan Sadhasivam @ 2022-05-16 20:18 ` Bjorn Helgaas 2022-05-17 15:09 ` Manivannan Sadhasivam 2022-05-26 20:48 ` Rob Herring 0 siblings, 2 replies; 17+ messages in thread From: Bjorn Helgaas @ 2022-05-16 20:18 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: bhelgaas, lorenzo.pieralisi, kbusch, hch, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi On Fri, May 13, 2022 at 04:30:25PM +0530, Manivannan Sadhasivam wrote: > On some systems like Chromebooks based on Qcom chipsets, the OS may > powerdown all PCIe devices during system suspend for aggressive > powersaving. In that case, the PCI host controller drivers need to notify > the PCI device drivers that the power will be taken off during system > suspend so that the drivers can prepare the devices accordingly. "The OS may powerdown all PCIe devices ..." makes it sound like this is an OS policy decision. Where exactly (what function) is that? Or if it's not an OS policy decision, but rather some property of the hardware, say that specifically. > One prime example is the PCI NVMe driver. This flag can be used by the > driver to shutdown the NVMe device during suspend and recover it during > resume. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > include/linux/pci.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 60adf42460ab..069caf1fe88d 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -578,6 +578,7 @@ struct pci_host_bridge { > unsigned int preserve_config:1; /* Preserve FW resource setup */ > unsigned int size_windows:1; /* Enable root bus sizing */ > unsigned int msi_domain:1; /* Bridge wants MSI domain */ > + unsigned int suspend_poweroff:1; /* OS may poweroff devices during system suspend */ > > /* Resource alignment requirements */ > resource_size_t (*align_resource)(struct pci_dev *dev, > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend 2022-05-16 20:18 ` Bjorn Helgaas @ 2022-05-17 15:09 ` Manivannan Sadhasivam 2022-05-17 17:24 ` Bjorn Helgaas 2022-05-26 20:48 ` Rob Herring 1 sibling, 1 reply; 17+ messages in thread From: Manivannan Sadhasivam @ 2022-05-17 15:09 UTC (permalink / raw) To: Bjorn Helgaas Cc: bhelgaas, lorenzo.pieralisi, kbusch, hch, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi On Mon, May 16, 2022 at 03:18:17PM -0500, Bjorn Helgaas wrote: > On Fri, May 13, 2022 at 04:30:25PM +0530, Manivannan Sadhasivam wrote: > > On some systems like Chromebooks based on Qcom chipsets, the OS may > > powerdown all PCIe devices during system suspend for aggressive > > powersaving. In that case, the PCI host controller drivers need to notify > > the PCI device drivers that the power will be taken off during system > > suspend so that the drivers can prepare the devices accordingly. > > "The OS may powerdown all PCIe devices ..." makes it sound like this > is an OS policy decision. Where exactly (what function) is that? > > Or if it's not an OS policy decision, but rather some property of the > hardware, say that specifically. > On SC7280, it is the Resource Power Manager(RPMh) that's powering the devices down by cutting off the PCIe voltage domain. But the SC7280 RC driver itself may put the PCIe devices into D3cold state during system suspend. https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/ So to cover both cases (one is a hardware independent of SoC and another one is the device driver), and to be generic, I've used the term "OS" after looking at the previous flags. Thanks, Mani > > One prime example is the PCI NVMe driver. This flag can be used by the > > driver to shutdown the NVMe device during suspend and recover it during > > resume. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > include/linux/pci.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 60adf42460ab..069caf1fe88d 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -578,6 +578,7 @@ struct pci_host_bridge { > > unsigned int preserve_config:1; /* Preserve FW resource setup */ > > unsigned int size_windows:1; /* Enable root bus sizing */ > > unsigned int msi_domain:1; /* Bridge wants MSI domain */ > > + unsigned int suspend_poweroff:1; /* OS may poweroff devices during system suspend */ > > > > /* Resource alignment requirements */ > > resource_size_t (*align_resource)(struct pci_dev *dev, > > -- > > 2.25.1 > > -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend 2022-05-17 15:09 ` Manivannan Sadhasivam @ 2022-05-17 17:24 ` Bjorn Helgaas 2022-05-18 3:59 ` Manivannan Sadhasivam 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2022-05-17 17:24 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Bjorn Helgaas, Lorenzo Pieralisi, Keith Busch, Christoph Hellwig, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, Stanimir Varbanov, Bjorn Andersson, Jens Axboe, Veerabhadrarao Badiganti, quic_krichai, Nitin Rawat, Vidya Sagar, Sagi Grimberg On Tue, May 17, 2022 at 08:39:08PM +0530, Manivannan Sadhasivam wrote: > On Mon, May 16, 2022 at 03:18:17PM -0500, Bjorn Helgaas wrote: > > On Fri, May 13, 2022 at 04:30:25PM +0530, Manivannan Sadhasivam wrote: > > > On some systems like Chromebooks based on Qcom chipsets, the OS may > > > powerdown all PCIe devices during system suspend for aggressive > > > powersaving. In that case, the PCI host controller drivers need to notify > > > the PCI device drivers that the power will be taken off during system > > > suspend so that the drivers can prepare the devices accordingly. > > > > "The OS may powerdown all PCIe devices ..." makes it sound like this > > is an OS policy decision. Where exactly (what function) is that? > > > > Or if it's not an OS policy decision, but rather some property of the > > hardware, say that specifically. > > On SC7280, it is the Resource Power Manager(RPMh) that's powering > the devices down by cutting off the PCIe voltage domain. But the > SC7280 RC driver itself may put the PCIe devices into D3cold state > during system suspend. > https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/ > > So to cover both cases (one is a hardware independent of SoC and > another one is the device driver), and to be generic, I've used the > term "OS" after looking at the previous flags. This sort of device-specific behavior definitely needs a pointer to an example. Otherwise it seems like it could be generic PCIe behavior that should be documented in the PCIe base spec. > > > One prime example is the PCI NVMe driver. This flag can be used by the > > > driver to shutdown the NVMe device during suspend and recover it during > > > resume. Apparently nvme is broken, or at least sub-optimal, without this flag. What other drivers will be similarly affected? > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > include/linux/pci.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > > index 60adf42460ab..069caf1fe88d 100644 > > > --- a/include/linux/pci.h > > > +++ b/include/linux/pci.h > > > @@ -578,6 +578,7 @@ struct pci_host_bridge { > > > unsigned int preserve_config:1; /* Preserve FW resource setup */ > > > unsigned int size_windows:1; /* Enable root bus sizing */ > > > unsigned int msi_domain:1; /* Bridge wants MSI domain */ > > > + unsigned int suspend_poweroff:1; /* OS may poweroff devices during system suspend */ > > > > > > /* Resource alignment requirements */ > > > resource_size_t (*align_resource)(struct pci_dev *dev, ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend 2022-05-17 17:24 ` Bjorn Helgaas @ 2022-05-18 3:59 ` Manivannan Sadhasivam 0 siblings, 0 replies; 17+ messages in thread From: Manivannan Sadhasivam @ 2022-05-18 3:59 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, Lorenzo Pieralisi, Keith Busch, Christoph Hellwig, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, Stanimir Varbanov, Bjorn Andersson, Jens Axboe, Veerabhadrarao Badiganti, quic_krichai, Nitin Rawat, Vidya Sagar, Sagi Grimberg On Tue, May 17, 2022 at 12:24:23PM -0500, Bjorn Helgaas wrote: > On Tue, May 17, 2022 at 08:39:08PM +0530, Manivannan Sadhasivam wrote: > > On Mon, May 16, 2022 at 03:18:17PM -0500, Bjorn Helgaas wrote: > > > On Fri, May 13, 2022 at 04:30:25PM +0530, Manivannan Sadhasivam wrote: > > > > On some systems like Chromebooks based on Qcom chipsets, the OS may > > > > powerdown all PCIe devices during system suspend for aggressive > > > > powersaving. In that case, the PCI host controller drivers need to notify > > > > the PCI device drivers that the power will be taken off during system > > > > suspend so that the drivers can prepare the devices accordingly. > > > > > > "The OS may powerdown all PCIe devices ..." makes it sound like this > > > is an OS policy decision. Where exactly (what function) is that? > > > > > > Or if it's not an OS policy decision, but rather some property of the > > > hardware, say that specifically. > > > > On SC7280, it is the Resource Power Manager(RPMh) that's powering > > the devices down by cutting off the PCIe voltage domain. But the > > SC7280 RC driver itself may put the PCIe devices into D3cold state > > during system suspend. > > https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/ > > > > So to cover both cases (one is a hardware independent of SoC and > > another one is the device driver), and to be generic, I've used the > > term "OS" after looking at the previous flags. > > This sort of device-specific behavior definitely needs a pointer to an > example. Otherwise it seems like it could be generic PCIe behavior > that should be documented in the PCIe base spec. > Okay. > > > > One prime example is the PCI NVMe driver. This flag can be used by the > > > > driver to shutdown the NVMe device during suspend and recover it during > > > > resume. > > Apparently nvme is broken, or at least sub-optimal, without this flag. Yes, broken on SC7280 or any other SoCs that turn off power. > What other drivers will be similarly affected? > I don't have a list but the drivers that don't expect the device to be turned off or reset during suspend may experience this issue. Right now, we have only identified the issue with NVMe because that's what used on Chromebooks. But in the coming days, we may need to fix some of the drivers also. Thanks, Mani > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > include/linux/pci.h | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > > > index 60adf42460ab..069caf1fe88d 100644 > > > > --- a/include/linux/pci.h > > > > +++ b/include/linux/pci.h > > > > @@ -578,6 +578,7 @@ struct pci_host_bridge { > > > > unsigned int preserve_config:1; /* Preserve FW resource setup */ > > > > unsigned int size_windows:1; /* Enable root bus sizing */ > > > > unsigned int msi_domain:1; /* Bridge wants MSI domain */ > > > > + unsigned int suspend_poweroff:1; /* OS may poweroff devices during system suspend */ > > > > > > > > /* Resource alignment requirements */ > > > > resource_size_t (*align_resource)(struct pci_dev *dev, -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend 2022-05-16 20:18 ` Bjorn Helgaas 2022-05-17 15:09 ` Manivannan Sadhasivam @ 2022-05-26 20:48 ` Rob Herring 1 sibling, 0 replies; 17+ messages in thread From: Rob Herring @ 2022-05-26 20:48 UTC (permalink / raw) To: Bjorn Helgaas Cc: Manivannan Sadhasivam, bhelgaas, lorenzo.pieralisi, kbusch, hch, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi On Mon, May 16, 2022 at 03:18:17PM -0500, Bjorn Helgaas wrote: > On Fri, May 13, 2022 at 04:30:25PM +0530, Manivannan Sadhasivam wrote: > > On some systems like Chromebooks based on Qcom chipsets, the OS may > > powerdown all PCIe devices during system suspend for aggressive > > powersaving. In that case, the PCI host controller drivers need to notify > > the PCI device drivers that the power will be taken off during system > > suspend so that the drivers can prepare the devices accordingly. > > "The OS may powerdown all PCIe devices ..." makes it sound like this > is an OS policy decision. Where exactly (what function) is that? > > Or if it's not an OS policy decision, but rather some property of the > hardware, say that specifically. > > > One prime example is the PCI NVMe driver. This flag can be used by the > > driver to shutdown the NVMe device during suspend and recover it during > > resume. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > include/linux/pci.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 60adf42460ab..069caf1fe88d 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -578,6 +578,7 @@ struct pci_host_bridge { > > unsigned int preserve_config:1; /* Preserve FW resource setup */ > > unsigned int size_windows:1; /* Enable root bus sizing */ > > unsigned int msi_domain:1; /* Bridge wants MSI domain */ > > + unsigned int suspend_poweroff:1; /* OS may poweroff devices during system suspend */ Why does this apply to the whole host bridge? What if you have multiple devices and some are powered off and others aren't? Rob ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 2022-05-13 11:00 [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Manivannan Sadhasivam 2022-05-13 11:00 ` [PATCH 1/3] PCI: Add a flag to notify " Manivannan Sadhasivam @ 2022-05-13 11:00 ` Manivannan Sadhasivam 2022-05-16 20:19 ` Bjorn Helgaas 2022-05-13 11:00 ` [PATCH 3/3] nvme-pci: Make use of "suspend_poweroff" flag during system suspend Manivannan Sadhasivam ` (2 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Manivannan Sadhasivam @ 2022-05-13 11:00 UTC (permalink / raw) To: bhelgaas, lorenzo.pieralisi, kbusch, hch Cc: linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi, Manivannan Sadhasivam For aggressive power saving on SC7280 SoCs, the power for the PCI devices will be taken off during system suspend. Hence, notify the same to the PCI device drivers using "suspend_poweroff" flag so that the drivers can prepare the PCI devices to handle the poweroff and recover them during resume. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 6ab90891801d..4b0ad2827f8f 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -199,6 +199,7 @@ struct qcom_pcie_cfg { unsigned int has_ddrss_sf_tbu_clk:1; unsigned int has_aggre0_clk:1; unsigned int has_aggre1_clk:1; + unsigned int suspend_poweroff:1; }; struct qcom_pcie { @@ -1220,6 +1221,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) if (pcie->cfg->pipe_clk_need_muxing) clk_set_parent(res->pipe_clk_src, res->ref_clk_src); + /* Indicate PCI device drivers that the power will be taken off during system suspend */ + if (pcie->cfg->suspend_poweroff) + pci->pp.bridge->suspend_poweroff = true; + ret = clk_bulk_prepare_enable(res->num_clks, res->clks); if (ret < 0) goto err_disable_regulators; @@ -1548,6 +1553,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = { .ops = &ops_1_9_0, .has_tbu_clk = true, .pipe_clk_need_muxing = true, + .suspend_poweroff = true, }; static const struct dw_pcie_ops dw_pcie_ops = { -- 2.25.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 2022-05-13 11:00 ` [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 Manivannan Sadhasivam @ 2022-05-16 20:19 ` Bjorn Helgaas 2022-05-17 15:11 ` Manivannan Sadhasivam 2022-05-18 8:43 ` Christoph Hellwig 0 siblings, 2 replies; 17+ messages in thread From: Bjorn Helgaas @ 2022-05-16 20:19 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: bhelgaas, lorenzo.pieralisi, kbusch, hch, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi On Fri, May 13, 2022 at 04:30:26PM +0530, Manivannan Sadhasivam wrote: > For aggressive power saving on SC7280 SoCs, the power for the PCI devices > will be taken off during system suspend. Hence, notify the same to the > PCI device drivers using "suspend_poweroff" flag so that the drivers can > prepare the PCI devices to handle the poweroff and recover them during > resume. No doubt "power ... will be taken off during system suspend" is true, but this isn't very informative. Is this a property of SC7280? A choice made by the SC7280 driver? Why is this not applicable to other systems? > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 6ab90891801d..4b0ad2827f8f 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -199,6 +199,7 @@ struct qcom_pcie_cfg { > unsigned int has_ddrss_sf_tbu_clk:1; > unsigned int has_aggre0_clk:1; > unsigned int has_aggre1_clk:1; > + unsigned int suspend_poweroff:1; > }; > > struct qcom_pcie { > @@ -1220,6 +1221,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > if (pcie->cfg->pipe_clk_need_muxing) > clk_set_parent(res->pipe_clk_src, res->ref_clk_src); > > + /* Indicate PCI device drivers that the power will be taken off during system suspend */ > + if (pcie->cfg->suspend_poweroff) > + pci->pp.bridge->suspend_poweroff = true; > + > ret = clk_bulk_prepare_enable(res->num_clks, res->clks); > if (ret < 0) > goto err_disable_regulators; > @@ -1548,6 +1553,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = { > .ops = &ops_1_9_0, > .has_tbu_clk = true, > .pipe_clk_need_muxing = true, > + .suspend_poweroff = true, > }; > > static const struct dw_pcie_ops dw_pcie_ops = { > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 2022-05-16 20:19 ` Bjorn Helgaas @ 2022-05-17 15:11 ` Manivannan Sadhasivam 2022-05-17 17:18 ` Bjorn Helgaas 2022-05-18 8:43 ` Christoph Hellwig 1 sibling, 1 reply; 17+ messages in thread From: Manivannan Sadhasivam @ 2022-05-17 15:11 UTC (permalink / raw) To: Bjorn Helgaas Cc: bhelgaas, lorenzo.pieralisi, kbusch, hch, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi On Mon, May 16, 2022 at 03:19:50PM -0500, Bjorn Helgaas wrote: > On Fri, May 13, 2022 at 04:30:26PM +0530, Manivannan Sadhasivam wrote: > > For aggressive power saving on SC7280 SoCs, the power for the PCI devices > > will be taken off during system suspend. Hence, notify the same to the > > PCI device drivers using "suspend_poweroff" flag so that the drivers can > > prepare the PCI devices to handle the poweroff and recover them during > > resume. > > No doubt "power ... will be taken off during system suspend" is true, > but this isn't very informative. Is this a property of SC7280? A > choice made by the SC7280 driver? Why is this not applicable to other > systems? > The SC7280's RPMh firmware is cutting off the PCIe power domain during system suspend. And as I explained in previous patch, the RC driver itself may put the devices in D3cold conditionally on this platform. The reason is to save power as this chipset is being used in Chromebooks. Thanks, Mani > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > index 6ab90891801d..4b0ad2827f8f 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > @@ -199,6 +199,7 @@ struct qcom_pcie_cfg { > > unsigned int has_ddrss_sf_tbu_clk:1; > > unsigned int has_aggre0_clk:1; > > unsigned int has_aggre1_clk:1; > > + unsigned int suspend_poweroff:1; > > }; > > > > struct qcom_pcie { > > @@ -1220,6 +1221,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > > if (pcie->cfg->pipe_clk_need_muxing) > > clk_set_parent(res->pipe_clk_src, res->ref_clk_src); > > > > + /* Indicate PCI device drivers that the power will be taken off during system suspend */ > > + if (pcie->cfg->suspend_poweroff) > > + pci->pp.bridge->suspend_poweroff = true; > > + > > ret = clk_bulk_prepare_enable(res->num_clks, res->clks); > > if (ret < 0) > > goto err_disable_regulators; > > @@ -1548,6 +1553,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = { > > .ops = &ops_1_9_0, > > .has_tbu_clk = true, > > .pipe_clk_need_muxing = true, > > + .suspend_poweroff = true, > > }; > > > > static const struct dw_pcie_ops dw_pcie_ops = { > > -- > > 2.25.1 > > -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 2022-05-17 15:11 ` Manivannan Sadhasivam @ 2022-05-17 17:18 ` Bjorn Helgaas 2022-05-18 3:52 ` Manivannan Sadhasivam 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2022-05-17 17:18 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Bjorn Helgaas, Lorenzo Pieralisi, Keith Busch, Christoph Hellwig, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, Stanimir Varbanov, Bjorn Andersson, Jens Axboe, Veerabhadrarao Badiganti, quic_krichai, Nitin Rawat, Vidya Sagar, Sagi Grimberg, Prasad Malisetty, Andy Gross, Rob Herring, Krzysztof Wilczyński, Rajat Jain, Saheed O. Bolarinwa, Rama Krishna, Stephen Boyd, Dmitry Baryshkov, Kalle Valo [+cc Prasad, Andy, Rob, Krzysztof, Rajat, Saheed, Rama, Stephen, Dmitry, Kalle for connection to https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/] Subject line convention for this file is "PCI: qcom:" (not "PCI: dwc: qcom:"). Find this from "git log --oneline drivers/pci/controller/dwc/pcie-qcom.c". On Tue, May 17, 2022 at 08:41:34PM +0530, Manivannan Sadhasivam wrote: > On Mon, May 16, 2022 at 03:19:50PM -0500, Bjorn Helgaas wrote: > > On Fri, May 13, 2022 at 04:30:26PM +0530, Manivannan Sadhasivam wrote: > > > For aggressive power saving on SC7280 SoCs, the power for the > > > PCI devices will be taken off during system suspend. Hence, > > > notify the same to the PCI device drivers using > > > "suspend_poweroff" flag so that the drivers can prepare the PCI > > > devices to handle the poweroff and recover them during resume. > > > > No doubt "power ... will be taken off during system suspend" is > > true, but this isn't very informative. Is this a property of > > SC7280? A choice made by the SC7280 driver? Why is this not > > applicable to other systems? > > The SC7280's RPMh firmware is cutting off the PCIe power domain > during system suspend. And as I explained in previous patch, the RC > driver itself may put the devices in D3cold conditionally on this > platform. The reason is to save power as this chipset is being used > in Chromebooks. It looks like this should be squashed into the patch you mentioned: https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/ If Prasad's patch is applied without this, devices will be powered off, but nvme will not be prepared for it. Apparently something would be broken in that case? Also, I think this patch should be reordered so the nvme driver is prepared for suspend_poweroff before the qcom driver starts setting it. Otherwise there's a window where qcom sets suspend_poweroff and powers off devices, but nvme doesn't know about it, and I assume something will be broken in that case? Please mention RPMh in the commit log, along with the specific connection with system suspend, i.e., what OS action enables RPMh to cut power. > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > index 6ab90891801d..4b0ad2827f8f 100644 > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > @@ -199,6 +199,7 @@ struct qcom_pcie_cfg { > > > unsigned int has_ddrss_sf_tbu_clk:1; > > > unsigned int has_aggre0_clk:1; > > > unsigned int has_aggre1_clk:1; > > > + unsigned int suspend_poweroff:1; > > > }; > > > > > > struct qcom_pcie { > > > @@ -1220,6 +1221,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > > > if (pcie->cfg->pipe_clk_need_muxing) > > > clk_set_parent(res->pipe_clk_src, res->ref_clk_src); > > > > > > + /* Indicate PCI device drivers that the power will be taken off during system suspend */ > > > + if (pcie->cfg->suspend_poweroff) > > > + pci->pp.bridge->suspend_poweroff = true; > > > + > > > ret = clk_bulk_prepare_enable(res->num_clks, res->clks); > > > if (ret < 0) > > > goto err_disable_regulators; > > > @@ -1548,6 +1553,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = { > > > .ops = &ops_1_9_0, > > > .has_tbu_clk = true, > > > .pipe_clk_need_muxing = true, > > > + .suspend_poweroff = true, > > > }; > > > > > > static const struct dw_pcie_ops dw_pcie_ops = { > > > -- > > > 2.25.1 > > > > > -- > மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 2022-05-17 17:18 ` Bjorn Helgaas @ 2022-05-18 3:52 ` Manivannan Sadhasivam 2022-05-18 16:50 ` Bjorn Helgaas 0 siblings, 1 reply; 17+ messages in thread From: Manivannan Sadhasivam @ 2022-05-18 3:52 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, Lorenzo Pieralisi, Keith Busch, Christoph Hellwig, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, Stanimir Varbanov, Bjorn Andersson, Jens Axboe, Veerabhadrarao Badiganti, quic_krichai, Nitin Rawat, Vidya Sagar, Sagi Grimberg, Prasad Malisetty, Andy Gross, Rob Herring, Krzysztof Wilczyński, Rajat Jain, Saheed O. Bolarinwa, Rama Krishna, Stephen Boyd, Dmitry Baryshkov, Kalle Valo On Tue, May 17, 2022 at 12:18:57PM -0500, Bjorn Helgaas wrote: > [+cc Prasad, Andy, Rob, Krzysztof, Rajat, Saheed, Rama, Stephen, > Dmitry, Kalle for connection to https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/] > > Subject line convention for this file is "PCI: qcom:" (not "PCI: dwc: > qcom:"). > > Find this from "git log --oneline drivers/pci/controller/dwc/pcie-qcom.c". > > On Tue, May 17, 2022 at 08:41:34PM +0530, Manivannan Sadhasivam wrote: > > On Mon, May 16, 2022 at 03:19:50PM -0500, Bjorn Helgaas wrote: > > > On Fri, May 13, 2022 at 04:30:26PM +0530, Manivannan Sadhasivam wrote: > > > > For aggressive power saving on SC7280 SoCs, the power for the > > > > PCI devices will be taken off during system suspend. Hence, > > > > notify the same to the PCI device drivers using > > > > "suspend_poweroff" flag so that the drivers can prepare the PCI > > > > devices to handle the poweroff and recover them during resume. > > > > > > No doubt "power ... will be taken off during system suspend" is > > > true, but this isn't very informative. Is this a property of > > > SC7280? A choice made by the SC7280 driver? Why is this not > > > applicable to other systems? > > > > The SC7280's RPMh firmware is cutting off the PCIe power domain > > during system suspend. And as I explained in previous patch, the RC > > driver itself may put the devices in D3cold conditionally on this > > platform. The reason is to save power as this chipset is being used > > in Chromebooks. > > It looks like this should be squashed into the patch you mentioned: > https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/ > > If Prasad's patch is applied without this, devices will be powered > off, but nvme will not be prepared for it. Apparently something would > be broken in that case? > Yes, but Prasad's patch is not yet reviewed so likely not get merged until further respins. > Also, I think this patch should be reordered so the nvme driver is > prepared for suspend_poweroff before the qcom driver starts setting > it. Otherwise there's a window where qcom sets suspend_poweroff and > powers off devices, but nvme doesn't know about it, and I assume > something will be broken in that case? > As per my understanding, patches in a series should not have build dependency but they may depend on each other for functionality. But I don't have any issue in reordering the patches. Will do. > Please mention RPMh in the commit log, along with the specific > connection with system suspend, i.e., what OS action enables RPMh to > cut power. > Okay, will do. Thanks, Mani > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > > index 6ab90891801d..4b0ad2827f8f 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > > @@ -199,6 +199,7 @@ struct qcom_pcie_cfg { > > > > unsigned int has_ddrss_sf_tbu_clk:1; > > > > unsigned int has_aggre0_clk:1; > > > > unsigned int has_aggre1_clk:1; > > > > + unsigned int suspend_poweroff:1; > > > > }; > > > > > > > > struct qcom_pcie { > > > > @@ -1220,6 +1221,10 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > > > > if (pcie->cfg->pipe_clk_need_muxing) > > > > clk_set_parent(res->pipe_clk_src, res->ref_clk_src); > > > > > > > > + /* Indicate PCI device drivers that the power will be taken off during system suspend */ > > > > + if (pcie->cfg->suspend_poweroff) > > > > + pci->pp.bridge->suspend_poweroff = true; > > > > + > > > > ret = clk_bulk_prepare_enable(res->num_clks, res->clks); > > > > if (ret < 0) > > > > goto err_disable_regulators; > > > > @@ -1548,6 +1553,7 @@ static const struct qcom_pcie_cfg sc7280_cfg = { > > > > .ops = &ops_1_9_0, > > > > .has_tbu_clk = true, > > > > .pipe_clk_need_muxing = true, > > > > + .suspend_poweroff = true, > > > > }; > > > > > > > > static const struct dw_pcie_ops dw_pcie_ops = { > > > > -- > > > > 2.25.1 > > > > > > > > -- > > மணிவண்ணன் சதாசிவம் -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 2022-05-18 3:52 ` Manivannan Sadhasivam @ 2022-05-18 16:50 ` Bjorn Helgaas 0 siblings, 0 replies; 17+ messages in thread From: Bjorn Helgaas @ 2022-05-18 16:50 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Bjorn Helgaas, Lorenzo Pieralisi, Keith Busch, Christoph Hellwig, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, Stanimir Varbanov, Bjorn Andersson, Jens Axboe, Veerabhadrarao Badiganti, quic_krichai, Nitin Rawat, Vidya Sagar, Sagi Grimberg, Prasad Malisetty, Andy Gross, Rob Herring, Krzysztof Wilczyński, Rajat Jain, Saheed O. Bolarinwa, Rama Krishna, Stephen Boyd, Dmitry Baryshkov, Kalle Valo On Wed, May 18, 2022 at 09:22:11AM +0530, Manivannan Sadhasivam wrote: > On Tue, May 17, 2022 at 12:18:57PM -0500, Bjorn Helgaas wrote: > > [+cc Prasad, Andy, Rob, Krzysztof, Rajat, Saheed, Rama, Stephen, > > Dmitry, Kalle for connection to https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/] > > > > Subject line convention for this file is "PCI: qcom:" (not "PCI: dwc: > > qcom:"). > > > > Find this from "git log --oneline drivers/pci/controller/dwc/pcie-qcom.c". > > > > On Tue, May 17, 2022 at 08:41:34PM +0530, Manivannan Sadhasivam wrote: > > > On Mon, May 16, 2022 at 03:19:50PM -0500, Bjorn Helgaas wrote: > > > > On Fri, May 13, 2022 at 04:30:26PM +0530, Manivannan Sadhasivam wrote: > > > > > For aggressive power saving on SC7280 SoCs, the power for the > > > > > PCI devices will be taken off during system suspend. Hence, > > > > > notify the same to the PCI device drivers using > > > > > "suspend_poweroff" flag so that the drivers can prepare the PCI > > > > > devices to handle the poweroff and recover them during resume. > > > > > > > > No doubt "power ... will be taken off during system suspend" is > > > > true, but this isn't very informative. Is this a property of > > > > SC7280? A choice made by the SC7280 driver? Why is this not > > > > applicable to other systems? > > > > > > The SC7280's RPMh firmware is cutting off the PCIe power domain > > > during system suspend. And as I explained in previous patch, the RC > > > driver itself may put the devices in D3cold conditionally on this > > > platform. The reason is to save power as this chipset is being used > > > in Chromebooks. > > > > It looks like this should be squashed into the patch you mentioned: > > https://lore.kernel.org/lkml/CAE-0n53ho2DX2rqQMvvKAuDCfsWW62TceTaNPzv5Mn_NQ-U6dA@mail.gmail.com/T/ > > > > If Prasad's patch is applied without this, devices will be powered > > off, but nvme will not be prepared for it. Apparently something would > > be broken in that case? > > > > Yes, but Prasad's patch is not yet reviewed so likely not get merged until > further respins. Ok. Please work with Prasad to squash these as needed so there are no regressions along the way. > > Also, I think this patch should be reordered so the nvme driver is > > prepared for suspend_poweroff before the qcom driver starts setting > > it. Otherwise there's a window where qcom sets suspend_poweroff and > > powers off devices, but nvme doesn't know about it, and I assume > > something will be broken in that case? > > As per my understanding, patches in a series should not have build dependency > but they may depend on each other for functionality. Yes. But if qcom starts powering off devices when nvme isn't expecting it, it sounds like nvme will regress until the patch that adds nvme support. That temporary regression is what I want to avoid. Bjorn ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 2022-05-16 20:19 ` Bjorn Helgaas 2022-05-17 15:11 ` Manivannan Sadhasivam @ 2022-05-18 8:43 ` Christoph Hellwig 1 sibling, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2022-05-18 8:43 UTC (permalink / raw) To: Bjorn Helgaas Cc: Manivannan Sadhasivam, bhelgaas, lorenzo.pieralisi, kbusch, hch, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi On Mon, May 16, 2022 at 03:19:50PM -0500, Bjorn Helgaas wrote: > No doubt "power ... will be taken off during system suspend" is true, > but this isn't very informative. Is this a property of SC7280? A > choice made by the SC7280 driver? Why is this not applicable to other > systems? This is really braindamage inflicted by microsoft with the StorageD3 property on ACPI systems. It is in general a really, really bad idea as it saves a little power but massively wears out the flash. It seems like Chromebooks and DT just want to keep up with the Jones. Which is a reminder that we should probably integrate the StorageD3 handling into this framework. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] nvme-pci: Make use of "suspend_poweroff" flag during system suspend 2022-05-13 11:00 [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Manivannan Sadhasivam 2022-05-13 11:00 ` [PATCH 1/3] PCI: Add a flag to notify " Manivannan Sadhasivam 2022-05-13 11:00 ` [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 Manivannan Sadhasivam @ 2022-05-13 11:00 ` Manivannan Sadhasivam 2022-05-16 5:44 ` [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Christoph Hellwig 2022-05-16 20:15 ` Bjorn Helgaas 4 siblings, 0 replies; 17+ messages in thread From: Manivannan Sadhasivam @ 2022-05-13 11:00 UTC (permalink / raw) To: bhelgaas, lorenzo.pieralisi, kbusch, hch Cc: linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi, Manivannan Sadhasivam On some platforms, the power to the PCI devices will be taken off during system suspend. For these platforms, the PCI RC will set the "system_poweroff" flag to notify the PCI device drivers of the poweroff scenario. Hence, make use of the flag in the system suspend path and if set, properly shutdown the device. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/nvme/host/pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index d817ca17463e..381bf0c7cf8d 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -3238,6 +3238,7 @@ static int nvme_suspend(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct nvme_dev *ndev = pci_get_drvdata(pdev); + struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus); struct nvme_ctrl *ctrl = &ndev->ctrl; int ret = -EBUSY; @@ -3257,7 +3258,7 @@ static int nvme_suspend(struct device *dev) * state (which may not be possible if the link is up). */ if (pm_suspend_via_firmware() || !ctrl->npss || - !pcie_aspm_enabled(pdev) || + !pcie_aspm_enabled(pdev) || bridge->suspend_poweroff || (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND)) return nvme_disable_prepare_reset(ndev, true); -- 2.25.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend 2022-05-13 11:00 [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Manivannan Sadhasivam ` (2 preceding siblings ...) 2022-05-13 11:00 ` [PATCH 3/3] nvme-pci: Make use of "suspend_poweroff" flag during system suspend Manivannan Sadhasivam @ 2022-05-16 5:44 ` Christoph Hellwig 2022-05-16 20:15 ` Bjorn Helgaas 4 siblings, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2022-05-16 5:44 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: bhelgaas, lorenzo.pieralisi, kbusch, hch, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi This looks workable to me, and is fine with me from the NVMe side. I could see arguments for passing the flag to the actual suspend method, but as that would require much bigger changes I'm not going to ask for it just because. Acked-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend 2022-05-13 11:00 [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Manivannan Sadhasivam ` (3 preceding siblings ...) 2022-05-16 5:44 ` [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Christoph Hellwig @ 2022-05-16 20:15 ` Bjorn Helgaas 4 siblings, 0 replies; 17+ messages in thread From: Bjorn Helgaas @ 2022-05-16 20:15 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: bhelgaas, lorenzo.pieralisi, kbusch, hch, linux-nvme, linux-pci, linux-arm-msm, linux-kernel, svarbanov, bjorn.andersson, axboe, quic_vbadigan, quic_krichai, quic_nitirawa, vidyas, sagi, Rafael J. Wysocki, linux-pm [+cc Rafael, linux-pm, since this is power management stuff https://lore.kernel.org/all/20220513110027.31015-1-manivannan.sadhasivam@linaro.org/] On Fri, May 13, 2022 at 04:30:24PM +0530, Manivannan Sadhasivam wrote: > Hi, > > This series adds support for notifying the PCI drivers like NVMe about the > transition of PCI devices into powerdown mode during system suspend. > > Background > ---------- > > On Qcom SC7280 based Chrome platforms, the OS will turn off the power to all > PCIe devices during system suspend for aggressive powersaving. Currently, there > is no way for the PCI device drivers to learn about this situation. Some of the > drivers assume that the power will be retained and some others assume that the > power may be taken down. > > We faced the issue with NVMe PCI driver, where the driver expects the NVMe > device to be in APST (Autonomous Power State Transition) state for power saving > during system suspend. So when the power goes down, the NVMe driver fails to > bringup the device during resume. > > Previous work > ------------- > > We tried to fix this issue in a couple of ways: > > 1. The NVMe PCI driver checks for the existence of "StorageD3Enable" ACPI > property in the suspend path. If the property is found, the driver assumes that > the device may go to poweroff state and shutdowns the device accordingly. > > As like the ACPI based systems, we also tried to get the support in place for > DT based systems. But that didn't get accepted: > https://lore.kernel.org/all/Yl+6V3pWuyRYuVV8@infradead.org/T/ > > 2. Keith Busch proposed a module params based approach. The parameter when set, > will allow the driver to support APST during suspend. Absence of that parameter > will let the driver shutdown the device. > > This also did not get accepted: > https://lore.kernel.org/linux-nvme/20220201165006.3074615-1-kbusch@kernel.org/ > > Proposal > -------- > > Christoph suggested to add a notification in the PCI/PM core to let the NVMe > driver know that the device will go into powerdown state during suspend. > https://lore.kernel.org/all/Yg0wklcJ3ed76Jbk@infradead.org/ > > Hence in this series, a "suspend_poweroff" flag is introduced in the host bridge > struct. When this flag is set by the PCI RC drivers, the PCI device driver like > NVMe can shutdown the device during suspend. > > In the coming days, the usage of this flag could be extended to other PCI > drivers as well. > > Testing > ------- > > This series has been tested on SC7280 IDP board connected to a NVMe PCI device. > > Thanks, > Mani > > Manivannan Sadhasivam (3): > PCI: Add a flag to notify PCI drivers about powerdown during suspend > PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 > nvme-pci: Make use of "suspend_poweroff" flag during system suspend > > drivers/nvme/host/pci.c | 3 ++- > drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++ > include/linux/pci.h | 1 + > 3 files changed, 9 insertions(+), 1 deletion(-) > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-05-26 20:48 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-13 11:00 [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Manivannan Sadhasivam 2022-05-13 11:00 ` [PATCH 1/3] PCI: Add a flag to notify " Manivannan Sadhasivam 2022-05-16 20:18 ` Bjorn Helgaas 2022-05-17 15:09 ` Manivannan Sadhasivam 2022-05-17 17:24 ` Bjorn Helgaas 2022-05-18 3:59 ` Manivannan Sadhasivam 2022-05-26 20:48 ` Rob Herring 2022-05-13 11:00 ` [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280 Manivannan Sadhasivam 2022-05-16 20:19 ` Bjorn Helgaas 2022-05-17 15:11 ` Manivannan Sadhasivam 2022-05-17 17:18 ` Bjorn Helgaas 2022-05-18 3:52 ` Manivannan Sadhasivam 2022-05-18 16:50 ` Bjorn Helgaas 2022-05-18 8:43 ` Christoph Hellwig 2022-05-13 11:00 ` [PATCH 3/3] nvme-pci: Make use of "suspend_poweroff" flag during system suspend Manivannan Sadhasivam 2022-05-16 5:44 ` [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend Christoph Hellwig 2022-05-16 20:15 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).