[PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend

[PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend

[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

[PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend

[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

[PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280

[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

[PATCH 3/3] nvme-pci: Make use of "suspend_poweroff" flag during system suspend

[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

Re: [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend

[Christoph Hellwig]

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>

Re: [PATCH 0/3] PCI: Notify PCI drivers about powerdown during suspend

[Bjorn Helgaas]

[+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
> 

Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend

[Bjorn Helgaas]

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
> 

Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280

[Bjorn Helgaas]

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
> 

Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend

[Manivannan Sadhasivam]

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
> > 

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

Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280

[Manivannan Sadhasivam]

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
> > 

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

Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280

[Bjorn Helgaas]

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

Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend

[Bjorn Helgaas]

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,

Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280

[Manivannan Sadhasivam]

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

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

Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend

[Manivannan Sadhasivam]

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,

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

Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280

[Christoph Hellwig]

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.

Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280

[Bjorn Helgaas]

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

Re: [PATCH 1/3] PCI: Add a flag to notify PCI drivers about powerdown during suspend

[Rob Herring]

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