diff mbox series

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

Message ID 20220513110027.31015-3-manivannan.sadhasivam@linaro.org (mailing list archive)
State Superseded
Headers show
Series PCI: Notify PCI drivers about powerdown during suspend | expand

Commit Message

Manivannan Sadhasivam May 13, 2022, 11 a.m. UTC
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(+)

Comments

Bjorn Helgaas May 16, 2022, 8:19 p.m. UTC | #1
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
>
Manivannan Sadhasivam May 17, 2022, 3:11 p.m. UTC | #2
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
> >
Bjorn Helgaas May 17, 2022, 5:18 p.m. UTC | #3
[+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
> > > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
Manivannan Sadhasivam May 18, 2022, 3:52 a.m. UTC | #4
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
> > > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்
Christoph Hellwig May 18, 2022, 8:43 a.m. UTC | #5
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.
Bjorn Helgaas May 18, 2022, 4:50 p.m. UTC | #6
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
diff mbox series

Patch

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