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