Message-ID: <aapdte7bfbkqnftrpl5twrwrzzptm5djg2lqanmpqrxgbmb5n2@fjhjavynjt6l>
Date: Tue, 12 Aug 2025 21:40:33 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Bjorn Helgaas <helgaas@...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 Thu, Jul 03, 2025 at 04:21:59PM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 6/10/2025 10:11 PM, Bjorn Helgaas wrote:
> > 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.
> I went through the SPEC again and you are right the spec hasn't
> mentioned about wake# routing properly.
> 
> I will move the code from portdrv to pci core framework and for your
> 1st objection, you are suggesting to search for wake IRQ in the endpoint
> DT and then traverse up. I believe you are suggesting this because we
> may more than one wake# routed to root port from multiple endpoints.
> if this is the case then we need to register for more than one wake
> IRQ. For this case I feel better to check for wake# gpio in the DT
> when ever there is a new device is detected in the pci core and create
> the wake IRQ with the dev associated with the pci_dev.
> 

I think it makes sense to have the wake-gpios property in the endpoint node as
that will clearly describe whether the device using WAKE# or not. If we have it
in the bridge node, then the host wouldn't know to which endpoint the WAKE# is
routed to. It will lead to incorrect assumptions.

But if we define wake-gpios in the endpoint node, then host will know which
endpoint supports WAKE# and it can register the WAKE# IRQ only for that device.
Even if the WAKE# is shared with multiple endpoints, all of them could define
the same wake-gpios property and the host could use a shared IRQ with the help
of IRQF_SHARED.

But this requires the dt-binding change though. I will submit a PR for that.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists