Message-ID: <9ede83ab-f494-4975-b896-da14958f727d@oss.qualcomm.com>
Date: Thu, 3 Jul 2025 16:21:59 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: Bjorn Helgaas <helgaas@...nel.org>
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 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.

Please correct me if I was wrong.

- Krishna Chaitanya.
> 
> Bjorn

Powered by blists - more mailing lists