Message-ID: <20250610164154.GA812762@bhelgaas>
Date: Tue, 10 Jun 2025 11:41:54 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Manivannan Sadhasivam <mani@...nel.org>,
	Brian Norris <briannorris@...omium.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Tony Lindgren <tony@...mide.com>,
	JeffyChen <jeffy.chen@...k-chips.com>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	cros-qcom-dts-watchers@...omium.org,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-arm-msm@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, quic_vbadigan@...cinc.com,
	quic_mrana@...cinc.com, Sherry Sun <sherry.sun@....com>
Subject: Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

On Tue, Jun 10, 2025 at 10:00:20AM +0530, Krishna Chaitanya Chundru wrote:
> On 6/10/2025 4:04 AM, Bjorn Helgaas wrote:
> > On Mon, Jun 09, 2025 at 05:29:49PM +0530, Manivannan Sadhasivam wrote:
> > > + Brian, Rafael, Tony, Jeffy (who were part of the previous attempt to add WAKE#
> > > GPIO/interrupt support:
> > > https://lore.kernel.org/linux-pci/20171225114742.18920-1-jeffy.chen@rock-chips.com
> > > 
> > > On Mon, Jun 09, 2025 at 11:27:49AM +0530, Krishna Chaitanya Chundru wrote:
> > > > On 6/6/2025 1:56 AM, Bjorn Helgaas wrote:
> > > > > On Thu, Jun 05, 2025 at 10:54:45AM +0530, Krishna Chaitanya Chundru wrote:
> > > > > > PCIe wake interrupt is needed for bringing back PCIe device state
> > > > > > from D3cold to D0.
> > > > > 
> > > > > Does this refer to the WAKE# signal or Beacon or both?  I guess the
> > > > > comments in the patch suggest WAKE#.  Is there any spec section we can
> > > > > cite here?
> > > > > 
> > > > we are referring only WAKE# signal, I will add the PCIe spec r6.0, sec
> > > > 5.3.3.2 in next patch version.
> > > > > > Implement new functions, of_pci_setup_wake_irq() and
> > > > > > of_pci_teardown_wake_irq(), to manage wake interrupts for PCI devices
> > > > > > using the Device Tree.
> > > > > > 
> > > > > >   From the port bus driver call these functions to enable wake support
> > > > > > for bridges.
> > > > > 
> > > > > What is the connection to bridges and portdrv?  WAKE# is described in
> > > > > PCIe r6.0, sec 5.3.3.2, and PCIe CEM r6.0, sec 2.3, but AFAICS neither
> > > > > restricts it to bridges.
> > > 
> > > You are right. WAKE# is really a PCIe slot/Endpoint property and
> > > doesn't necessarily belong to a Root Port/Bridge. But the problem is
> > > with handling the Wake interrupt in the host. For instance, below is
> > > the DT representation of the PCIe hierarchy:
> > > 
> > > 	PCIe Host Bridge
> > > 		|
> > > 		v
> > > 	PCIe Root Port/Bridge
> > > 		|
> > > 		|
> > > 		v
> > > PCIe Slot <-------------> PCIe Endpoint
> > > 
> > > DTs usually define both the WAKE# and PERST# GPIOs
> > > ({wake/reset}-gpios property) in the PCIe Host Bridge node. But we
> > > have decided to move atleast the PERST# to the Root Port node since
> > > the PERST# lines are per slot and not per host bridge.
> > > 
> > > Similar interpretation applies to WAKE# as well, but the major
> > > difference is that it is controlled by the endpoints, not by the
> > > host (RC/Host Bridge/Root Port). The host only cares about the
> > > interrupt that rises from the WAKE# GPIO.  The PCIe spec, r6.0,
> > > Figure 5-4, tells us that the WAKE# is routed to the PM controller
> > > on the host. In most of the systems that tends to be true as the
> > > WAKE# is not tied to the PCIe IP itself, but to a GPIO controller in
> > > the host.
> > 
> > If WAKE# is supported at all, it's a sideband signal independent of
> > the link topology.  PCIe CEM r6.0, sec 2.3, says WAKE# from multiple
> > connectors can be wire-ORed together, or can have individual
> > connections to the PM controller.
>
> I believe they are referring to multi root port where WAKE# can
> routed to individual root port where each root port can go D3cold
> individually.

AFAICT there's no requirement that WAKE# be routed to a Root Port or a
Switch Port.  The routing is completely implementation specific.

> From endpoint perspective they will have single WAKE# signal, the
> WAKE# from endpoint will be routed to its DSP's i.e root port in
> direct attach and in case of switch they will routed to the USP from
> their again they will be connected to the root port only as there is
> noway that individual DSP's in the switch can go to D3cold from
> linux point of view as linux will not have control over switch
> firmware to control D3cold to D0 sequence.
>
> But still if the firmware in the DSP of a switch can allow device to
> go in to D3cold after moving host moving link to D3hot, the DSP in
> the switch needs to receive the WAKE# signal first to supply power
> and refclk then DSP will propagate WAKE# to host to change device
> state to D0. In this case if there is separate WAKE# signal routed
> to the host, we can define WAKE# in the device-tree assigned to the
> DSP of the switch. As the DSP's are also tied with the portdrv, the
> same existing patch will work since this patch is looking for
> wake-gpios property assigned to that particular port in the DT.

WAKE# is only defined for certain form factors, and Root Ports and
Switch Ports have no WAKE#-related behavior defined by the PCIe specs.

I don't want to make assumptions about how WAKE# is routed, whether
Switches have implementation-specific WAKE# handling, or how D3cold
transitions happen.  Those things are all implementation specific.

My main objections are:

  - Setting up a wake IRQ should be done on an endpoint, but this
    patch assumes doing it on a Root Port or Switch Port is enough.

    We can start a DT search for a wake IRQ at the endpoint and
    traverse up the hierarchy if necessary, of course.

  - The code should not be in portdrv.c.  Putting it in portdrv means
    it won't work unless CONFIG_PCIEPORTBUS is enabled, and WAKE# has
    nothing to do with the rest of portdrv.

Bjorn

Powered by blists - more mailing lists