Re: [PATCH v7 1/5] PCI: qcom: Add system suspend and resume support

From: Bjorn Helgaas <helgaas@kernel.org>
To: Krishna Chaitanya Chundru <quic_krichai@quicinc.com>
Cc: linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, mka@chromium.org,
	quic_vbadigan@quicinc.com, quic_hemantk@quicinc.com,
	quic_nitegupt@quicinc.com, quic_skananth@quicinc.com,
	quic_ramkri@quicinc.com, manivannan.sadhasivam@linaro.org,
	swboyd@chromium.org, dmitry.baryshkov@linaro.org,
	svarbanov@mm-sol.com, agross@kernel.org, andersson@kernel.org,
	konrad.dybcio@somainline.org, lpieralisi@kernel.org,
	robh@kernel.org, kw@linux.com, bhelgaas@google.com,
	linux-phy@lists.infradead.org, vkoul@kernel.org, kishon@ti.com,
	mturquette@baylibre.com, linux-clk@vger.kernel.org,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	linux-pm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	Kevin Rowland <kevin.p.rowland@gmail.com>
Subject: Re: [PATCH v7 1/5] PCI: qcom: Add system suspend and resume support
Date: Wed, 12 Oct 2022 19:44:44 -0500	[thread overview]
Message-ID: <20221013004444.GA3135316@bhelgaas> (raw)
In-Reply-To: <04ace1ae-26d0-4157-b7eb-8dff29895180@quicinc.com>

[+cc Marc, Kevin]

On Wed, Oct 12, 2022 at 07:36:52PM +0530, Krishna Chaitanya Chundru wrote:
> On 10/6/2022 2:43 AM, Bjorn Helgaas wrote:

[I'm declaring quote text bankruptcy and dropping the huge wall of
text.  The IRQ affinity change you mention seems to be the critical
issue :)]

> > The PCIe spec clearly envisions Refclk being turned off
> > (sec 5.5.3.3.1) and PHYs being powered off (sec 5.5.3.2) while in
> > L1.2.
> > 
> > I've been assuming L1.2 exit (which includes Refclk being turned on
> > and PHYs being powered up) is completely handled by hardware, but it
> > sounds like the Qcom controller needs software assistance which fields
> > an interrupt when CLKREQ# is asserted and turns on Refclk and the
> > PHYs?
> > 
> > 5.5.3 does say "All Link and PHY state must be maintained during L1.2,
> > or must be restored upon exit using implementation specific means",
> > and maybe Qcom counts as using implementation specific means.
> > 
> > I *am* concerned about whether software can do the L1.2 exit fast
> > enough, but the biggest reason I'm struggling with this is because
> > using the syscore framework to work around IRQ affinity changes that
> > happen late in suspend just seems kind of kludgy and it doesn't seem
> > like it fits cleanly in the power management model.
> 
> Can you please suggest any another way to work around IRQ affinity
> changes.

One of your earlier patches [1] made dw_msi_mask_irq() look like this:

  static void dw_msi_mask_irq(struct irq_data *d)
  {
    struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
    struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

    if (dw_pcie_link_up(pci))
	    pci_msi_mask_irq(d);

    irq_chip_mask_parent(d);
  }

That was an awful lot like Marc's suggestion [2] that the
pci_msi_mask_irq() should be redundant.

If it's truly redundant, maybe pci_msi_mask_irq() can be removed
from dw_msi_mask_irq() (and other similar *_mask_irq()
implementations) completely?

Bjorn

[1] https://lore.kernel.org/r/1659526134-22978-3-git-send-email-quic_krichai@quicinc.com
[2] https://lore.kernel.org/linux-pci/86k05m7dkr.wl-maz@kernel.org/