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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: "Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Keith Busch" <kbusch@kernel.org>,
	"Christoph Hellwig" <hch@lst.de>,
	linux-nvme@lists.infradead.org, linux-pci@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Stanimir Varbanov" <svarbanov@mm-sol.com>,
	"Bjorn Andersson" <bjorn.andersson@linaro.org>,
	"Jens Axboe" <axboe@fb.com>,
	"Veerabhadrarao Badiganti" <quic_vbadigan@quicinc.com>,
	quic_krichai@quicinc.com,
	"Nitin Rawat" <quic_nitirawa@quicinc.com>,
	"Vidya Sagar" <vidyas@nvidia.com>,
	"Sagi Grimberg" <sagi@grimberg.me>,
	"Prasad Malisetty" <quic_pmaliset@quicinc.com>,
	"Andy Gross" <agross@kernel.org>, "Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rajat Jain" <rajatja@google.com>,
	"Saheed O. Bolarinwa" <refactormyself@gmail.com>,
	"Rama Krishna" <quic_ramkri@quicinc.com>,
	"Stephen Boyd" <swboyd@chromium.org>,
	"Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>,
	"Kalle Valo" <kvalo@kernel.org>
Subject: Re: [PATCH 2/3] PCI: dwc: qcom: Set suspend_poweroff flag for SC7280
Date: Wed, 18 May 2022 11:50:17 -0500	[thread overview]
Message-ID: <20220518165017.GA1145045@bhelgaas> (raw)
In-Reply-To: <20220518035211.GA4791@thinkpad>

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