[PATCH v3 0/2] PCI: Add support for PCIe wake interrupt

[PATCH v3 0/2] PCI: Add support for PCIe wake interrupt

[Krishna Chaitanya Chundru]

PCIe wake interrupt is needed for bringing back PCIe device state from
D3cold to D0.

This is pending from long time, there was two attempts done previously to
add wake support[1], [2]. Those series tried to add support for legacy
interrupts along with wake. Legacy interrupts are already available in
the latest kernel and we can ignore them. For the wake IRQ the series is
trying to use interrupts property define in the device tree.

This series is using gpio property instead of interrupts, from
gpio desc driver will allocate the dedicate IRQ and initiate the wake
IRQ from the port bus driver instead of pcie framework as adding in the
pcie framework will be applicable to the endpoint devices also. As the
port bus driver is for bridges, portbus driver is correct place to invoke
them.

Add two new functions, of_pci_slot_setup_wake_irq() and
of_pci_slot_teardown_wake_irq(), to manage wake interrupts for PCI devices
using the Device Tree.

The series depend on the following series:
https://lore.kernel.org/all/20250515090517.3506772-1-krishna.chundru@oss.qualcomm.com/

[1]: https://lore.kernel.org/all/b2b91240-95fe-145d-502c-d52225497a34@nvidia.com/T/
[2]: https://lore.kernel.org/all/20171226023646.17722-1-jeffy.chen@rock-chips.com/

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Changes in v3:
- Update the commit messages, function names etc as suggested by Mani.
- return wake_irq if returns error (Neil).
- Link to v2: https://lore.kernel.org/r/20250419-wake_irq_support-v2-0-06baed9a87a1@oss.qualcomm.com

Changes in v2:
- Move the wake irq teardown after pcie_port_device_remove
  and move of_pci_setup_wake_irq before pcie_link_rcec (Lukas)
- teardown wake irq in shutdown also.
- Link to v1: https://lore.kernel.org/r/20250401-wake_irq_support-v1-0-d2e22f4a0efd@oss.qualcomm.com

---
Krishna Chaitanya Chundru (2):
      arm64: dts: qcom: sc7280: Add wake GPIO
      PCI/portdrv: Add support for PCIe wake interrupt

 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   |  1 +
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  1 +
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       |  1 +
 drivers/pci/of.c                               | 67 ++++++++++++++++++++++++++
 drivers/pci/pci.h                              |  6 +++
 drivers/pci/pcie/portdrv.c                     | 12 ++++-
 6 files changed, 87 insertions(+), 1 deletion(-)
---
base-commit: 88d324e69ea9f3ae1c1905ea75d717c08bdb8e15
change-id: 20250329-wake_irq_support-79772fc8cd44
prerequisite-change-id: 20250101-perst-cb885b5a6129:v1
prerequisite-patch-id: 3cff2ef415ec12c8ddb7ce7193035ce546081243
prerequisite-patch-id: 820dbf5dc092c32c8394fbc33f9fe6b8da6e6eab
prerequisite-patch-id: 7f87f54386a87b39ca346b53d3c34ff0d0cb7911

Best regards,
-- 
krishnachaitanya-linux <krishna.chundru@oss.qualcomm.com>

[PATCH v3 1/2] arm64: dts: qcom: sc7280: Add wake GPIO

[Krishna Chaitanya Chundru]

Add WAKE# gpio which is needed to bring PCIe device state
from D3cold to D0.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts   | 1 +
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 +
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
index 10c152ac03c874df5f1dc386d9079d3db1c55362..27cd94cb48d91e57d07cb38e008859d0c354b8fc 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -810,6 +810,7 @@ &mdss_edp_phy {
 
 &pcieport1 {
 	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
+	wake-gpios = <&tlmm 3 GPIO_ACTIVE_LOW>;
 };
 
 &pcie1 {
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 60b3cf50ea1d61dd5e8b573b5f1c6faa1c291eee..d435db860625d52842bf8e92d6223f67343121db 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -477,6 +477,7 @@ &pcie1 {
 
 &pcieport1 {
 	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
+	wake-gpios = <&tlmm 3 GPIO_ACTIVE_LOW>;
 };
 
 &pm8350c_pwm {
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 0b0212b670797a364d7f0e7a458fc73245fff8db..762615a61e046c711b248579abcbc58b86e428c1 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -418,6 +418,7 @@ &lpass_va_macro {
 
 &pcieport1 {
 	reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
+	wake-gpios = <&tlmm 3 GPIO_ACTIVE_LOW>;
 };
 
 &pcie1 {

-- 
2.34.1

[PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Krishna Chaitanya Chundru]

PCIe wake interrupt is needed for bringing back PCIe device state
from D3cold to D0.

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.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Tested-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/pci/of.c           | 67 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h          |  6 +++++
 drivers/pci/pcie/portdrv.c | 12 ++++++++-
 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index ab7a8252bf4137a17971c3eb8ab70ce78ca70969..3487cd62d81f0a66e7408e286475e8d91c2e410a 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -7,6 +7,7 @@
 #define pr_fmt(fmt)	"PCI: OF: " fmt
 
 #include <linux/cleanup.h>
+#include <linux/gpio/consumer.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/pci.h>
@@ -15,6 +16,7 @@
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
 #include "pci.h"
 
 #ifdef CONFIG_PCI
@@ -966,3 +968,68 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
 	return slot_power_limit_mw;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
+
+/**
+ * of_pci_slot_setup_wake_irq - Set up wake interrupt for PCI device
+ * @pdev: The PCI device structure
+ *
+ * This function sets up the wake interrupt for a PCI device by getting the
+ * corresponding WAKE# gpio from the device tree, and configuring it as a
+ * dedicated wake interrupt.
+ *
+ * Return: 0 if the WAKE# gpio is not available or successfully parsed else
+ * errno otherwise.
+ */
+int of_pci_slot_setup_wake_irq(struct pci_dev *pdev)
+{
+	struct gpio_desc *wake;
+	struct device_node *dn;
+	int ret, wake_irq;
+
+	dn = pci_device_to_OF_node(pdev);
+	if (!dn)
+		return 0;
+
+	wake = devm_fwnode_gpiod_get(&pdev->dev, of_fwnode_handle(dn),
+				     "wake", GPIOD_IN, NULL);
+	if (IS_ERR(wake) && PTR_ERR(wake) != -ENOENT) {
+		dev_err(&pdev->dev, "Failed to get wake GPIO: %ld\n", PTR_ERR(wake));
+		return PTR_ERR(wake);
+	}
+	if (IS_ERR(wake))
+		return 0;
+
+	wake_irq = gpiod_to_irq(wake);
+	if (wake_irq < 0) {
+		dev_err(&pdev->dev, "Dailed to get wake irq: %d\n", wake_irq);
+		return wake_irq;
+	}
+
+	device_init_wakeup(&pdev->dev, true);
+
+	ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, wake_irq);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to set wake IRQ: %d\n", ret);
+		device_init_wakeup(&pdev->dev, false);
+		return ret;
+	}
+	irq_set_irq_type(wake_irq, IRQ_TYPE_EDGE_FALLING);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_slot_setup_wake_irq);
+
+/**
+ * of_pci_slot_teardown_wake_irq - Teardown wake interrupt setup for PCI device
+ *
+ * @pdev: The PCI device structure
+ *
+ * This function tears down the wake interrupt setup for a PCI device,
+ * clearing the dedicated wake interrupt and disabling device wake-up.
+ */
+void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev)
+{
+	dev_pm_clear_wake_irq(&pdev->dev);
+	device_init_wakeup(&pdev->dev, false);
+}
+EXPORT_SYMBOL_GPL(of_pci_slot_teardown_wake_irq);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 39f368d2f26de872f6484c6cb4e12752abfff7bc..dd7a4da1225bbdb1dff82707b580e7e0a95a5abf 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -888,6 +888,9 @@ void pci_release_of_node(struct pci_dev *dev);
 void pci_set_bus_of_node(struct pci_bus *bus);
 void pci_release_bus_of_node(struct pci_bus *bus);
 
+int of_pci_slot_setup_wake_irq(struct pci_dev *pdev);
+void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev);
+
 int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
 bool of_pci_supply_present(struct device_node *np);
 
@@ -931,6 +934,9 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
 	return 0;
 }
 
+static int of_pci_slot_setup_wake_irq(struct pci_dev *pdev) { return 0; }
+static void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev) { }
+
 static inline bool of_pci_supply_present(struct device_node *np)
 {
 	return false;
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index e8318fd5f6ed537a1b236a3a0f054161d5710abd..9a6beec87e4523a33ecace684109cd44e025c97b 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -694,12 +694,18 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	     (type != PCI_EXP_TYPE_RC_EC)))
 		return -ENODEV;
 
+	status = of_pci_slot_setup_wake_irq(dev);
+	if (status)
+		return status;
+
 	if (type == PCI_EXP_TYPE_RC_EC)
 		pcie_link_rcec(dev);
 
 	status = pcie_port_device_register(dev);
-	if (status)
+	if (status) {
+		of_pci_slot_teardown_wake_irq(dev);
 		return status;
+	}
 
 	pci_save_state(dev);
 
@@ -732,6 +738,8 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
 
 	pcie_port_device_remove(dev);
 
+	of_pci_slot_teardown_wake_irq(dev);
+
 	pci_disable_device(dev);
 }
 
@@ -744,6 +752,8 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev)
 	}
 
 	pcie_port_device_remove(dev);
+
+	of_pci_slot_teardown_wake_irq(dev);
 }
 
 static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,

-- 
2.34.1

Re: [PATCH v3 1/2] arm64: dts: qcom: sc7280: Add wake GPIO

[Konrad Dybcio]

On 6/5/25 7:24 AM, Krishna Chaitanya Chundru wrote:
> Add WAKE# gpio which is needed to bring PCIe device state
> from D3cold to D0.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Bjorn Helgaas]

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?

> 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.

Unless there's some related functionality in a Root Port, RCEC, or
Switch Port, maybe this code should be elsewhere, like
set_pcie_port_type(), so we could set this up for any PCIe device that
has a WAKE# description?

> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Tested-by: Sherry Sun <sherry.sun@nxp.com>
> ---
>  drivers/pci/of.c           | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h          |  6 +++++
>  drivers/pci/pcie/portdrv.c | 12 ++++++++-
>  3 files changed, 84 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index ab7a8252bf4137a17971c3eb8ab70ce78ca70969..3487cd62d81f0a66e7408e286475e8d91c2e410a 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -7,6 +7,7 @@
>  #define pr_fmt(fmt)	"PCI: OF: " fmt
>  
>  #include <linux/cleanup.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
> @@ -15,6 +16,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
>  #include "pci.h"
>  
>  #ifdef CONFIG_PCI
> @@ -966,3 +968,68 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>  	return slot_power_limit_mw;
>  }
>  EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> +
> +/**
> + * of_pci_slot_setup_wake_irq - Set up wake interrupt for PCI device
> + * @pdev: The PCI device structure
> + *
> + * This function sets up the wake interrupt for a PCI device by getting the
> + * corresponding WAKE# gpio from the device tree, and configuring it as a
> + * dedicated wake interrupt.
> + *
> + * Return: 0 if the WAKE# gpio is not available or successfully parsed else
> + * errno otherwise.
> + */
> +int of_pci_slot_setup_wake_irq(struct pci_dev *pdev)
> +{
> +	struct gpio_desc *wake;
> +	struct device_node *dn;
> +	int ret, wake_irq;
> +
> +	dn = pci_device_to_OF_node(pdev);
> +	if (!dn)
> +		return 0;
> +
> +	wake = devm_fwnode_gpiod_get(&pdev->dev, of_fwnode_handle(dn),
> +				     "wake", GPIOD_IN, NULL);

I guess this finds "wake-gpio" or "wake-gpios", as used in
Documentation/devicetree/bindings/pci/qcom,pcie.yaml,
qcom,pcie-sa8775p.yaml, etc?  Are these names specified in any generic
place, e.g.,
https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/pci?

> +	if (IS_ERR(wake) && PTR_ERR(wake) != -ENOENT) {
> +		dev_err(&pdev->dev, "Failed to get wake GPIO: %ld\n", PTR_ERR(wake));
> +		return PTR_ERR(wake);
> +	}
> +	if (IS_ERR(wake))
> +		return 0;
> +
> +	wake_irq = gpiod_to_irq(wake);
> +	if (wake_irq < 0) {
> +		dev_err(&pdev->dev, "Dailed to get wake irq: %d\n", wake_irq);

s/Dailed/Failed/

> +		return wake_irq;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, true);
> +
> +	ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, wake_irq);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to set wake IRQ: %d\n", ret);
> +		device_init_wakeup(&pdev->dev, false);
> +		return ret;
> +	}
> +	irq_set_irq_type(wake_irq, IRQ_TYPE_EDGE_FALLING);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_slot_setup_wake_irq);
> +
> +/**
> + * of_pci_slot_teardown_wake_irq - Teardown wake interrupt setup for PCI device
> + *
> + * @pdev: The PCI device structure
> + *
> + * This function tears down the wake interrupt setup for a PCI device,
> + * clearing the dedicated wake interrupt and disabling device wake-up.
> + */
> +void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev)
> +{
> +	dev_pm_clear_wake_irq(&pdev->dev);
> +	device_init_wakeup(&pdev->dev, false);
> +}
> +EXPORT_SYMBOL_GPL(of_pci_slot_teardown_wake_irq);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 39f368d2f26de872f6484c6cb4e12752abfff7bc..dd7a4da1225bbdb1dff82707b580e7e0a95a5abf 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -888,6 +888,9 @@ void pci_release_of_node(struct pci_dev *dev);
>  void pci_set_bus_of_node(struct pci_bus *bus);
>  void pci_release_bus_of_node(struct pci_bus *bus);
>  
> +int of_pci_slot_setup_wake_irq(struct pci_dev *pdev);
> +void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev);
> +
>  int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
>  bool of_pci_supply_present(struct device_node *np);
>  
> @@ -931,6 +934,9 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>  	return 0;
>  }
>  
> +static int of_pci_slot_setup_wake_irq(struct pci_dev *pdev) { return 0; }
> +static void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev) { }
> +
>  static inline bool of_pci_supply_present(struct device_node *np)
>  {
>  	return false;
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index e8318fd5f6ed537a1b236a3a0f054161d5710abd..9a6beec87e4523a33ecace684109cd44e025c97b 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -694,12 +694,18 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	     (type != PCI_EXP_TYPE_RC_EC)))
>  		return -ENODEV;
>  
> +	status = of_pci_slot_setup_wake_irq(dev);
> +	if (status)
> +		return status;
> +
>  	if (type == PCI_EXP_TYPE_RC_EC)
>  		pcie_link_rcec(dev);
>  
>  	status = pcie_port_device_register(dev);
> -	if (status)
> +	if (status) {
> +		of_pci_slot_teardown_wake_irq(dev);
>  		return status;
> +	}
>  
>  	pci_save_state(dev);
>  
> @@ -732,6 +738,8 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
>  
>  	pcie_port_device_remove(dev);
>  
> +	of_pci_slot_teardown_wake_irq(dev);
> +
>  	pci_disable_device(dev);
>  }
>  
> @@ -744,6 +752,8 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev)
>  	}
>  
>  	pcie_port_device_remove(dev);
> +
> +	of_pci_slot_teardown_wake_irq(dev);
>  }
>  
>  static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,
> 
> -- 
> 2.34.1
> 

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Krishna Chaitanya Chundru]

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.
> 
The wake# is used by the endpoints to wake host to bring PCIe device
state to D0 again, in direct attach root port wake# will be connected
to the root port and in switch cases the wake# will connected to the
switch Downstream ports and switch will consolidate wake# from different
downstream ports and sends to the root port. The wake# will be used by
root port bridges only. portdrv is the driver for root port.
> Unless there's some related functionality in a Root Port, RCEC, or
> Switch Port, maybe this code should be elsewhere, like
> set_pcie_port_type(), so we could set this up for any PCIe device that
> has a WAKE# description?
> 
As this is only used by root port, I am ok to change it to there to
enable this only in case of rootport.
But we need to make some changes in the flow as of node is not assigned
yet pci dev and also if wake registration fails we can't stop the driver
from probing as driver is not yet in the picture yet.
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> Tested-by: Sherry Sun <sherry.sun@nxp.com>
>> ---
>>   drivers/pci/of.c           | 67 ++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci.h          |  6 +++++
>>   drivers/pci/pcie/portdrv.c | 12 ++++++++-
>>   3 files changed, 84 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index ab7a8252bf4137a17971c3eb8ab70ce78ca70969..3487cd62d81f0a66e7408e286475e8d91c2e410a 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -7,6 +7,7 @@
>>   #define pr_fmt(fmt)	"PCI: OF: " fmt
>>   
>>   #include <linux/cleanup.h>
>> +#include <linux/gpio/consumer.h>
>>   #include <linux/irqdomain.h>
>>   #include <linux/kernel.h>
>>   #include <linux/pci.h>
>> @@ -15,6 +16,7 @@
>>   #include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_wakeirq.h>
>>   #include "pci.h"
>>   
>>   #ifdef CONFIG_PCI
>> @@ -966,3 +968,68 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>   	return slot_power_limit_mw;
>>   }
>>   EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>> +
>> +/**
>> + * of_pci_slot_setup_wake_irq - Set up wake interrupt for PCI device
>> + * @pdev: The PCI device structure
>> + *
>> + * This function sets up the wake interrupt for a PCI device by getting the
>> + * corresponding WAKE# gpio from the device tree, and configuring it as a
>> + * dedicated wake interrupt.
>> + *
>> + * Return: 0 if the WAKE# gpio is not available or successfully parsed else
>> + * errno otherwise.
>> + */
>> +int of_pci_slot_setup_wake_irq(struct pci_dev *pdev)
>> +{
>> +	struct gpio_desc *wake;
>> +	struct device_node *dn;
>> +	int ret, wake_irq;
>> +
>> +	dn = pci_device_to_OF_node(pdev);
>> +	if (!dn)
>> +		return 0;
>> +
>> +	wake = devm_fwnode_gpiod_get(&pdev->dev, of_fwnode_handle(dn),
>> +				     "wake", GPIOD_IN, NULL);
> 
> I guess this finds "wake-gpio" or "wake-gpios", as used in
> Documentation/devicetree/bindings/pci/qcom,pcie.yaml,
> qcom,pcie-sa8775p.yaml, etc?  Are these names specified in any generic
> place, e.g.,
> https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/pci?
> 
I created a patch to add them in common schemas:
https://lore.kernel.org/all/20250515090517.3506772-1-krishna.chundru@oss.qualcomm.com/

- Krishna Chaitanya.
>> +	if (IS_ERR(wake) && PTR_ERR(wake) != -ENOENT) {
>> +		dev_err(&pdev->dev, "Failed to get wake GPIO: %ld\n", PTR_ERR(wake));
>> +		return PTR_ERR(wake);
>> +	}
>> +	if (IS_ERR(wake))
>> +		return 0;
>> +
>> +	wake_irq = gpiod_to_irq(wake);
>> +	if (wake_irq < 0) {
>> +		dev_err(&pdev->dev, "Dailed to get wake irq: %d\n", wake_irq);
> 
> s/Dailed/Failed/
> 
>> +		return wake_irq;
>> +	}
>> +
>> +	device_init_wakeup(&pdev->dev, true);
>> +
>> +	ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, wake_irq);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Failed to set wake IRQ: %d\n", ret);
>> +		device_init_wakeup(&pdev->dev, false);
>> +		return ret;
>> +	}
>> +	irq_set_irq_type(wake_irq, IRQ_TYPE_EDGE_FALLING);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_pci_slot_setup_wake_irq);
>> +
>> +/**
>> + * of_pci_slot_teardown_wake_irq - Teardown wake interrupt setup for PCI device
>> + *
>> + * @pdev: The PCI device structure
>> + *
>> + * This function tears down the wake interrupt setup for a PCI device,
>> + * clearing the dedicated wake interrupt and disabling device wake-up.
>> + */
>> +void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev)
>> +{
>> +	dev_pm_clear_wake_irq(&pdev->dev);
>> +	device_init_wakeup(&pdev->dev, false);
>> +}
>> +EXPORT_SYMBOL_GPL(of_pci_slot_teardown_wake_irq);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 39f368d2f26de872f6484c6cb4e12752abfff7bc..dd7a4da1225bbdb1dff82707b580e7e0a95a5abf 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -888,6 +888,9 @@ void pci_release_of_node(struct pci_dev *dev);
>>   void pci_set_bus_of_node(struct pci_bus *bus);
>>   void pci_release_bus_of_node(struct pci_bus *bus);
>>   
>> +int of_pci_slot_setup_wake_irq(struct pci_dev *pdev);
>> +void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev);
>> +
>>   int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
>>   bool of_pci_supply_present(struct device_node *np);
>>   
>> @@ -931,6 +934,9 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>>   	return 0;
>>   }
>>   
>> +static int of_pci_slot_setup_wake_irq(struct pci_dev *pdev) { return 0; }
>> +static void of_pci_slot_teardown_wake_irq(struct pci_dev *pdev) { }
>> +
>>   static inline bool of_pci_supply_present(struct device_node *np)
>>   {
>>   	return false;
>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
>> index e8318fd5f6ed537a1b236a3a0f054161d5710abd..9a6beec87e4523a33ecace684109cd44e025c97b 100644
>> --- a/drivers/pci/pcie/portdrv.c
>> +++ b/drivers/pci/pcie/portdrv.c
>> @@ -694,12 +694,18 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>>   	     (type != PCI_EXP_TYPE_RC_EC)))
>>   		return -ENODEV;
>>   
>> +	status = of_pci_slot_setup_wake_irq(dev);
>> +	if (status)
>> +		return status;
>> +
>>   	if (type == PCI_EXP_TYPE_RC_EC)
>>   		pcie_link_rcec(dev);
>>   
>>   	status = pcie_port_device_register(dev);
>> -	if (status)
>> +	if (status) {
>> +		of_pci_slot_teardown_wake_irq(dev);
>>   		return status;
>> +	}
>>   
>>   	pci_save_state(dev);
>>   
>> @@ -732,6 +738,8 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
>>   
>>   	pcie_port_device_remove(dev);
>>   
>> +	of_pci_slot_teardown_wake_irq(dev);
>> +
>>   	pci_disable_device(dev);
>>   }
>>   
>> @@ -744,6 +752,8 @@ static void pcie_portdrv_shutdown(struct pci_dev *dev)
>>   	}
>>   
>>   	pcie_port_device_remove(dev);
>> +
>> +	of_pci_slot_teardown_wake_irq(dev);
>>   }
>>   
>>   static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Manivannan Sadhasivam]

+ 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.

In this case, the PCI core somehow needs to know the IRQ number corresponding to
the WAKE# GPIO, so that it can register an IRQ handler for it to wakeup the
endpoint when an interrupt happens. Previous attempts [1], tried to add a new DT
property for the interrupts:
https://lore.kernel.org/linux-pci/20171225114742.18920-2-jeffy.chen@rock-chips.com

But that doesn't seem to fly. So Krishna came with the proposal to reuse the
WAKE# GPIO defined in the Root Port node (for DTs that have moved the properties
out of the Host Bridge node) and extract the IRQ number from it using
gpiod_to_irq() API.

And he used portdrv as the placeholder for the irq setup code, because the WAKE#
GPIO is going to be defined in the Root Port node irrespective of a PCIe Slot or
an endpoint is connected to the Root Port. And the portdrv driver is the one
controlling the Root Port. Though, this placeholder part can be subject to
discussion.

But from the previous discussions, I could infer that the PCIe Root Port/Bridge
DT node is going to be the placeholder for the WAKE# GPIOs:
https://lore.kernel.org/linux-pci/2806186.IK6EZBC0cX@aspire.rjw.lan

It also sounds sensible to me since we do not want to define the wake-gpios
property in the endpoint node as that would mean that for each device connected,
a separate DT endpoint node needs to be created just for defining the WAKE#
GPIO. Also, if there is only a PCIe slot in the board, then the property has to
be defined in the PCIe Root Port node itself as there is no separate DT node for
PCIe slot.

> The wake# is used by the endpoints to wake host to bring PCIe device
> state to D0 again, in direct attach root port wake# will be connected
> to the root port and in switch cases the wake# will connected to the
> switch Downstream ports and switch will consolidate wake# from different
> downstream ports and sends to the root port. The wake# will be used by
> root port bridges only. portdrv is the driver for root port.

This is not correct. Root Port doesn't have anything to do with the WAKE#
interrupt. We are just merely trying to reuse the Root Port driver because of
how the WAKE# GPIO is wired up in the DT. It might not be applicable for ACPI as
the FW raises GPE for the Wake interrupt and kernel doesn't parse WAKE#, afaik.

> > Unless there's some related functionality in a Root Port, RCEC, or
> > Switch Port, maybe this code should be elsewhere, like
> > set_pcie_port_type(), so we could set this up for any PCIe device that
> > has a WAKE# description?
> > 

I think it should work if we tie the Wake interrupt to the PCI device instead of
the Root Port/Bridge in the kernel, even though the DT representation is
different. In that case, PCI core should parse the Root Port node for each
device to get the WAKE# GPIO and setup an IRQ handler for it. When the Wake
interrupt happens, the PCI core should power ON the PCI hierarchy starting from
the host bridge, till the endpoint (top down approach).

I think we all agree that the WAKE# GPIO should be handled by the PCI core
instead of the PCI controller or client drivers. We should agree on the best
possible place though.

- Mani

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

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Bjorn Helgaas]

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.

> > > +	wake = devm_fwnode_gpiod_get(&pdev->dev, of_fwnode_handle(dn),
> > > +				     "wake", GPIOD_IN, NULL);
> > 
> > I guess this finds "wake-gpio" or "wake-gpios", as used in
> > Documentation/devicetree/bindings/pci/qcom,pcie.yaml,
> > qcom,pcie-sa8775p.yaml, etc?  Are these names specified in any generic
> > place, e.g.,
> > https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/pci?
> > 
> I created a patch to add them in common schemas:
> https://lore.kernel.org/all/20250515090517.3506772-1-krishna.chundru@oss.qualcomm.com/

Thanks.  I think it will help other DT writers if we can include a
link to the relevant dtschema commit in this commit log.

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Bjorn Helgaas]

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.

> In this case, the PCI core somehow needs to know the IRQ number
> corresponding to the WAKE# GPIO, so that it can register an IRQ
> handler for it to wakeup the endpoint when an interrupt happens.
> Previous attempts [1], tried to add a new DT property for the
> interrupts:
> https://lore.kernel.org/linux-pci/20171225114742.18920-2-jeffy.chen@rock-chips.com
> 
> But that doesn't seem to fly. So Krishna came with the proposal to
> reuse the WAKE# GPIO defined in the Root Port node (for DTs that
> have moved the properties out of the Host Bridge node) and extract
> the IRQ number from it using gpiod_to_irq() API.

I don't think we can assume a single WAKE# GPIO in a Root Port, as
above.  I think we'll have to start looking at the endpoint and search
upward till we find one.

Bjorn

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Krishna Chaitanya Chundru]

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.

 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.

Please let me know your opinion's on this.

- Krishna Chaitanya.
> >> In this case, the PCI core somehow needs to know the IRQ number
>> corresponding to the WAKE# GPIO, so that it can register an IRQ
>> handler for it to wakeup the endpoint when an interrupt happens.
>> Previous attempts [1], tried to add a new DT property for the
>> interrupts:
>> https://lore.kernel.org/linux-pci/20171225114742.18920-2-jeffy.chen@rock-chips.com
>>
>> But that doesn't seem to fly. So Krishna came with the proposal to
>> reuse the WAKE# GPIO defined in the Root Port node (for DTs that
>> have moved the properties out of the Host Bridge node) and extract
>> the IRQ number from it using gpiod_to_irq() API.
> 
> I don't think we can assume a single WAKE# GPIO in a Root Port, as
> above.  I think we'll have to start looking at the endpoint and search
> upward till we find one.
>


> Bjorn

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Krishna Chaitanya Chundru]

On 6/9/2025 9:59 PM, Bjorn Helgaas wrote:
> 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.
> 
>>>> +	wake = devm_fwnode_gpiod_get(&pdev->dev, of_fwnode_handle(dn),
>>>> +				     "wake", GPIOD_IN, NULL);
>>>
>>> I guess this finds "wake-gpio" or "wake-gpios", as used in
>>> Documentation/devicetree/bindings/pci/qcom,pcie.yaml,
>>> qcom,pcie-sa8775p.yaml, etc?  Are these names specified in any generic
>>> place, e.g.,
>>> https://github.com/devicetree-org/dt-schema/tree/main/dtschema/schemas/pci?
>>>
>> I created a patch to add them in common schemas:
>> https://lore.kernel.org/all/20250515090517.3506772-1-krishna.chundru@oss.qualcomm.com/
> 
> Thanks.  I think it will help other DT writers if we can include a
> link to the relevant dtschema commit in this commit log.
Hi Bjorn,

I have added this link to the cover letter, I will add the link to
this commit in my next patch as this make sense to be added here.

- Krishna Chaitanya.

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Bjorn Helgaas]

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

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Krishna Chaitanya Chundru]

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

Re: [PATCH v3 2/2] PCI/portdrv: Add support for PCIe wake interrupt

[Manivannan Sadhasivam]

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

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