| 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 |
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 >
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 > >
[+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 > > > > > -- > மணிவண்ணன் சதாசிவம்
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 > > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
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.
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 --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 = {
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(+)