[RFC,v14 0/5] Add DT based PCIe wake support in PCI core driver

[RFC,v14 0/5] Add DT based PCIe wake support in PCI core driver

[Manikanta Maddireddy]

Below series [1] attempted to support DT based PCIe wake feature in generic
PCI core driver. This series was left at v13 and final comments are not
addressed. I am continuing this series from v14 by addressing all comments
in v13. I dropped rockchip device tree patch because I don't have hardware
to verify it. Instead, I verified these patches on NVIDIA Jetson AGX Orin
Developer Kit and included its device tree changes in this series.

[1] https://lore.kernel.org/all/20171226023646.17722-1-jeffy.chen@rock-chips.com/

Changes in v14:
Updated commit message for DT bindings patch to reflect that DT properties
are tied to PCI-PCI Bridge.
Addressed review comments on PCI interrupt parsing patch.
Dropped rockchip device tree patch.
Added Jetson AGX OrinDeveloper Kit device tree and Tegra PMC patches.

Changes in v13:
Fix compiler error reported by kbuild test robot <fengguang.wu@intel.com>

Changes in v12:
Only add irq definitions for PCI devices and rewrite the commit message.
Enable the wake irq in noirq stage to avoid possible irq storm.

Changes in v11:
Address Brian's comments.
Only support 1-per-device PCIe WAKE# pin as suggested.
Move to pcie port as Brian suggested.

Changes in v10:
Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
since dedicated wakeirq will be lost in device_set_wakeup_enable(false).

Changes in v9:
Add section for PCI devices and rewrite the commit message.
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.
Rewrite the commit message.

Changes in v8:
Add optional "pci", and rewrite commit message.
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
Rewrite the commit message.

Changes in v7:
Move PCIE_WAKE handling into pci core.

Changes in v6:
Fix device_init_wake error handling, and add some comments.

Changes in v5:
Move to pci.txt
Rebase.
Use "wakeup" instead of "wake"

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (3):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  PCI / PM: Add support for the PCIe WAKE# signal for OF

Manikanta Maddireddy (2):
  arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller
  soc/tegra: pmc: Add Tegra234 PCIe wake event

 Documentation/devicetree/bindings/pci/pci.txt |  8 +++
 .../nvidia/tegra234-p3737-0000+p3701-0000.dts | 11 ++++
 drivers/pci/of.c                              | 63 ++++++++++++++++++-
 drivers/pci/pci-driver.c                      | 10 +++
 drivers/pci/pci.c                             |  7 +++
 drivers/pci/pci.h                             |  8 +++
 drivers/soc/tegra/pmc.c                       |  1 +
 7 files changed, 105 insertions(+), 3 deletions(-)

-- 
2.25.1

[RFC,v14 1/5] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq

[Manikanta Maddireddy]

From: Jeffy Chen <jeffy.chen@rock-chips.com>

Add device tree support to pass PCIe WAKE# pin information to PCI core
driver. To support PCIe WAKE# and PCI irqs, add definition of the optional
properties "interrupts" and "interrupt-names". These properties should be
defined by the PCIe port to which wake capable Endpoint is connected,
so the definition is added under "PCI-PCI Bridge properties" section.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---

Changes in v14:
Move the device tree properties definition to "PCI-PCI Bridge properties"
section and also update commit message.

Changes in v13: None
Changes in v12:
Only add irq definitions for PCI devices and rewrite the commit message.

Changes in v11: None
Changes in v10: None
Changes in v9:
Add section for PCI devices and rewrite the commit message.

Changes in v8:
Add optional "pci", and rewrite commit message.

Changes in v7: None
Changes in v6: None
Changes in v5:
Move to pci.txt

Changes in v3: None
Changes in v2: None

 Documentation/devicetree/bindings/pci/pci.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index 6a8f2874a24d..53bd559a7305 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -71,6 +71,14 @@ Optional properties:
    trusted with relaxed DMA protection, as users could easily attach
    malicious devices to this port.
 
+- interrupts: Interrupt specifier for each name in interrupt-names.
+- interrupt-names:
+    May contain "wakeup" for PCIe WAKE# interrupt and "pci" for PCI interrupt.
+    The PCI devices may optionally include an 'interrupts' property that
+    represents the legacy PCI interrupt. And when we try to specify the PCIe
+    WAKE# pin, a corresponding 'interrupt-names' property is required to
+    distinguish them.
+
 Example:
 
 pcie@10000000 {
-- 
2.25.1

[RFC,v14 2/5] of/irq: Adjust of_pci_irq parsing for multiple interrupts

[Manikanta Maddireddy]

From: Jeffy Chen <jeffy.chen@rock-chips.com>

Currently we are considering the first irq as the PCI interrupt pin,
but a PCI device may have multiple interrupts(e.g. PCIe WAKE# pin).

Only parse the PCI interrupt pin when the irq is unnamed or named as
"pci".

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---

Changes in v14:
Address Rob's comment on using of_property_match_string().

Changes in v13: None
Changes in v12: None
Changes in v11:
Address Brian's comments.

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None

 drivers/pci/of.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 196834ed44fe..ff897c40ed71 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -429,9 +429,17 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
 	 */
 	dn = pci_device_to_OF_node(pdev);
 	if (dn) {
-		rc = of_irq_parse_one(dn, 0, out_irq);
-		if (!rc)
-			return rc;
+		int index = 0;
+
+		index = of_property_match_string(dn, "interrupt-names", "pci");
+		if (index == -EINVAL)	/* Property doesn't exist */
+			index = 0;
+
+		if (index >= 0) {
+			rc = of_irq_parse_one(dn, index, out_irq);
+			if (!rc)
+				return rc;
+		}
 	}
 
 	/*
-- 
2.25.1

[RFC,v14 3/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Manikanta Maddireddy]

From: Jeffy Chen <jeffy.chen@rock-chips.com>

Add of_pci_setup_wake_irq() to parse the PCIe WAKE# interrupt from the
device tree and set the wake irq. Add of_pci_teardown_wake_irq() to clear
the wake irq.

Call of_pci_setup_wake_irq() in pci_device_probe() to setup PCIe WAKE#
interrupt during PCIe Endpoint enumeration.

Enable or disable PCIe WAKE# interrupt in platform_pci_set_wakeup().

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---

Changes in v14:
pci_platform_pm_ops structure is removed in latest kernel, so dropped
pci-of driver. Instead, enable wake in platform_pci_set_wakeup().

Changes in v13:
Fix compiler error reported by kbuild test robot <fengguang.wu@intel.com>

Changes in v12:
Enable the wake irq in noirq stage to avoid possible irq storm.

Changes in v11:
Only support 1-per-device PCIe WAKE# pin as suggested.

Changes in v10:
Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
since dedicated wakeirq will be lost in device_set_wakeup_enable(false).

Changes in v9:
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.

Changes in v8:
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.

Changes in v7:
Move PCIE_WAKE handling into pci core.

Changes in v6:
Fix device_init_wake error handling, and add some comments.

Changes in v5:
Rebase.

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

 drivers/pci/of.c         | 49 ++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-driver.c | 10 ++++++++
 drivers/pci/pci.c        |  7 ++++++
 drivers/pci/pci.h        |  8 +++++++
 4 files changed, 74 insertions(+)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index ff897c40ed71..1c348e63f175 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -13,6 +13,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
+#include <linux/pm_wakeirq.h>
 #include "pci.h"
 
 #ifdef CONFIG_PCI
@@ -705,3 +706,51 @@ 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);
+
+int of_pci_setup_wake_irq(struct pci_dev *pdev)
+{
+	struct pci_dev *ppdev;
+	struct device_node *dn;
+	int ret, irq;
+
+	/* Get the pci_dev of our parent. Hopefully it's a port. */
+	ppdev = pdev->bus->self;
+	/* Nope, it's a host bridge. */
+	if (!ppdev)
+		return 0;
+
+	dn = pci_device_to_OF_node(ppdev);
+	if (!dn)
+		return 0;
+
+	irq = of_irq_get_byname(dn, "wakeup");
+	if (irq == -EPROBE_DEFER) {
+		return irq;
+	} else if (irq < 0) {
+		/* Ignore other errors, since a missing wakeup is non-fatal. */
+		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
+		return 0;
+	}
+
+	device_init_wakeup(&pdev->dev, true);
+
+	ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, irq);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to set wake IRQ: %d\n", ret);
+		device_init_wakeup(&pdev->dev, false);
+		return ret;
+	}
+
+	/* Start out disabled to avoid irq storm */
+	dev_pm_disable_wake_irq(&pdev->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
+
+void of_pci_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_teardown_wake_irq);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index d934c27491c4..fca966137fac 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -14,6 +14,7 @@
 #include <linux/sched.h>
 #include <linux/sched/isolation.h>
 #include <linux/cpu.h>
+#include <linux/of_pci.h>
 #include <linux/pm_runtime.h>
 #include <linux/suspend.h>
 #include <linux/kexec.h>
@@ -456,10 +457,17 @@ static int pci_device_probe(struct device *dev)
 	if (error < 0)
 		return error;
 
+	error = of_pci_setup_wake_irq(pci_dev);
+	if (error < 0) {
+		pcibios_free_irq(pci_dev);
+		return error;
+	}
+
 	pci_dev_get(pci_dev);
 	error = __pci_device_probe(drv, pci_dev);
 	if (error) {
 		pcibios_free_irq(pci_dev);
+		of_pci_teardown_wake_irq(pci_dev);
 		pci_dev_put(pci_dev);
 	}
 
@@ -471,6 +479,8 @@ static void pci_device_remove(struct device *dev)
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct pci_driver *drv = pci_dev->driver;
 
+	of_pci_teardown_wake_irq(pci_dev);
+
 	if (drv->remove) {
 		pm_runtime_get_sync(dev);
 		drv->remove(pci_dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index fba95486caaf..332a0b98b6c3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -27,6 +27,7 @@
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/pci_hotplug.h>
 #include <linux/vmalloc.h>
 #include <asm/dma.h>
@@ -1052,6 +1053,12 @@ static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
 	if (pci_use_mid_pm())
 		return PCI_POWER_ERROR;
 
+	/* Enable or disable wakeirq if set via device tree. */
+	if (enable)
+		dev_pm_enable_wake_irq(&dev->dev);
+	else
+		dev_pm_disable_wake_irq(&dev->dev);
+
 	return acpi_pci_wakeup(dev, enable);
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9ed3b5550043..83a2af148631 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -631,6 +631,8 @@ int of_pci_get_max_link_speed(struct device_node *node);
 u32 of_pci_get_slot_power_limit(struct device_node *node,
 				u8 *slot_power_limit_value,
 				u8 *slot_power_limit_scale);
+int of_pci_setup_wake_irq(struct pci_dev *pdev);
+void of_pci_teardown_wake_irq(struct pci_dev *pdev);
 void pci_set_of_node(struct pci_dev *dev);
 void pci_release_of_node(struct pci_dev *dev);
 void pci_set_bus_of_node(struct pci_bus *bus);
@@ -669,6 +671,12 @@ of_pci_get_slot_power_limit(struct device_node *node,
 	return 0;
 }
 
+static inline int of_pci_setup_wake_irq(struct pci_dev *pdev)
+{
+	return 0;
+}
+
+static inline void of_pci_teardown_wake_irq(struct pci_dev *pdev) { }
 static inline void pci_set_of_node(struct pci_dev *dev) { }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
-- 
2.25.1

[RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Manikanta Maddireddy]

Add PCIe port node under the PCIe controller-1 device tree node to support
PCIe WAKE# interrupt for WiFi.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---

Changes in v14:
New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.

 .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
index 8a9747855d6b..9c89be263141 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
@@ -2147,6 +2147,17 @@ pcie@14100000 {
 
 			phys = <&p2u_hsio_3>;
 			phy-names = "p2u-0";
+
+			pci@0,0 {
+				reg = <0x0000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+
+				interrupt-parent = <&gpio>;
+				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
+				interrupt-names = "wakeup";
+			};
 		};
 
 		pcie@14160000 {
-- 
2.25.1

[RFC,v14 5/5] soc/tegra: pmc: Add Tegra234 PCIe wake event

[Manikanta Maddireddy]

Enable PCIe wake event for the Tegra234 SoC.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---

Changes in v14:
New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.

 drivers/soc/tegra/pmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index cf4cfbf9f7c5..139ee853c32b 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -4225,6 +4225,7 @@ static const char * const tegra234_reset_sources[] = {
 };
 
 static const struct tegra_wake_event tegra234_wake_events[] = {
+	TEGRA_WAKE_GPIO("pcie", 1, 0, TEGRA234_MAIN_GPIO(L, 2)),
 	TEGRA_WAKE_GPIO("power", 29, 1, TEGRA234_AON_GPIO(EE, 4)),
 	TEGRA_WAKE_IRQ("rtc", 73, 10),
 };
-- 
2.25.1

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> Add PCIe port node under the PCIe controller-1 device tree node to support
> PCIe WAKE# interrupt for WiFi.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> 
> Changes in v14:
> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> 
>  .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> index 8a9747855d6b..9c89be263141 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>  
>  			phys = <&p2u_hsio_3>;
>  			phy-names = "p2u-0";
> +
> +			pci@0,0 {
> +				reg = <0x0000 0 0 0 0>;
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +
> +				interrupt-parent = <&gpio>;
> +				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> +				interrupt-names = "wakeup";
> +			};

Don't we need to wire this to the PMC interrupt controller and the wake
event corresponding to the L2 GPIO? Otherwise none of the wake logic in
PMC will get invoked.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v14 5/5] soc/tegra: pmc: Add Tegra234 PCIe wake event

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On Wed, Feb 08, 2023 at 04:46:45PM +0530, Manikanta Maddireddy wrote:
> Enable PCIe wake event for the Tegra234 SoC.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> 
> Changes in v14:
> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> 
>  drivers/soc/tegra/pmc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index cf4cfbf9f7c5..139ee853c32b 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -4225,6 +4225,7 @@ static const char * const tegra234_reset_sources[] = {
>  };
>  
>  static const struct tegra_wake_event tegra234_wake_events[] = {
> +	TEGRA_WAKE_GPIO("pcie", 1, 0, TEGRA234_MAIN_GPIO(L, 2)),

What about wake events for other PCIe controllers? Do we need/want to
distinguish by name for those?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v14 2/5] of/irq: Adjust of_pci_irq parsing for multiple interrupts

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

On Wed, Feb 08, 2023 at 04:46:42PM +0530, Manikanta Maddireddy wrote:
> From: Jeffy Chen <jeffy.chen@rock-chips.com>
> 
> Currently we are considering the first irq as the PCI interrupt pin,
> but a PCI device may have multiple interrupts(e.g. PCIe WAKE# pin).
> 
> Only parse the PCI interrupt pin when the irq is unnamed or named as
> "pci".
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> 
> Changes in v14:
> Address Rob's comment on using of_property_match_string().
> 
> Changes in v13: None
> Changes in v12: None
> Changes in v11:
> Address Brian's comments.
> 
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/pci/of.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 196834ed44fe..ff897c40ed71 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -429,9 +429,17 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
>  	 */
>  	dn = pci_device_to_OF_node(pdev);
>  	if (dn) {
> -		rc = of_irq_parse_one(dn, 0, out_irq);
> -		if (!rc)
> -			return rc;
> +		int index = 0;

No need to initialize to 0 here since you're assigning to it immediately
below.

Otherwise, looks good, so with that initialization dropped, this is:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v14 3/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]

On Wed, Feb 08, 2023 at 04:46:43PM +0530, Manikanta Maddireddy wrote:
> From: Jeffy Chen <jeffy.chen@rock-chips.com>
> 
> Add of_pci_setup_wake_irq() to parse the PCIe WAKE# interrupt from the
> device tree and set the wake irq. Add of_pci_teardown_wake_irq() to clear
> the wake irq.
> 
> Call of_pci_setup_wake_irq() in pci_device_probe() to setup PCIe WAKE#
> interrupt during PCIe Endpoint enumeration.
> 
> Enable or disable PCIe WAKE# interrupt in platform_pci_set_wakeup().
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> 
> Changes in v14:
> pci_platform_pm_ops structure is removed in latest kernel, so dropped
> pci-of driver. Instead, enable wake in platform_pci_set_wakeup().
> 
> Changes in v13:
> Fix compiler error reported by kbuild test robot <fengguang.wu@intel.com>
> 
> Changes in v12:
> Enable the wake irq in noirq stage to avoid possible irq storm.
> 
> Changes in v11:
> Only support 1-per-device PCIe WAKE# pin as suggested.
> 
> Changes in v10:
> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
> since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
> 
> Changes in v9:
> Fix check error in .cleanup().
> Move dedicated wakeirq setup to setup() callback and use
> device_set_wakeup_enable() to enable/disable.
> 
> Changes in v8:
> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
> 
> Changes in v7:
> Move PCIE_WAKE handling into pci core.
> 
> Changes in v6:
> Fix device_init_wake error handling, and add some comments.
> 
> Changes in v5:
> Rebase.
> 
> Changes in v3:
> Fix error handling.
> 
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq.
> 
>  drivers/pci/of.c         | 49 ++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-driver.c | 10 ++++++++
>  drivers/pci/pci.c        |  7 ++++++
>  drivers/pci/pci.h        |  8 +++++++
>  4 files changed, 74 insertions(+)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index ff897c40ed71..1c348e63f175 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -13,6 +13,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> +#include <linux/pm_wakeirq.h>
>  #include "pci.h"
>  
>  #ifdef CONFIG_PCI
> @@ -705,3 +706,51 @@ 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);
> +
> +int of_pci_setup_wake_irq(struct pci_dev *pdev)
> +{
> +	struct pci_dev *ppdev;

Perhaps "parent" since that's what it is referring to? ppdev is a bit
vague.

> +	struct device_node *dn;
> +	int ret, irq;
> +
> +	/* Get the pci_dev of our parent. Hopefully it's a port. */
> +	ppdev = pdev->bus->self;
> +	/* Nope, it's a host bridge. */
> +	if (!ppdev)
> +		return 0;
> +
> +	dn = pci_device_to_OF_node(ppdev);
> +	if (!dn)
> +		return 0;
> +
> +	irq = of_irq_get_byname(dn, "wakeup");
> +	if (irq == -EPROBE_DEFER) {
> +		return irq;
> +	} else if (irq < 0) {
> +		/* Ignore other errors, since a missing wakeup is non-fatal. */
> +		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);

dev_dbg() maybe? As it is this would add an annoying info message for
basically every PCI controller on every DT-based board out there.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v14 5/5] soc/tegra: pmc: Add Tegra234 PCIe wake event

[Manikanta Maddireddy]

On 2/8/2023 5:08 PM, Thierry Reding wrote:
> On Wed, Feb 08, 2023 at 04:46:45PM +0530, Manikanta Maddireddy wrote:
>> Enable PCIe wake event for the Tegra234 SoC.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>
>> Changes in v14:
>> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
>>
>>   drivers/soc/tegra/pmc.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index cf4cfbf9f7c5..139ee853c32b 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -4225,6 +4225,7 @@ static const char * const tegra234_reset_sources[] = {
>>   };
>>   
>>   static const struct tegra_wake_event tegra234_wake_events[] = {
>> +	TEGRA_WAKE_GPIO("pcie", 1, 0, TEGRA234_MAIN_GPIO(L, 2)),
> What about wake events for other PCIe controllers? Do we need/want to
> distinguish by name for those?
>
> Thierry
Only one wake gpio is defined for PCIe in Tegra234. There is no 
implementation
withinPCIe controller, so any wake capable gpio can be used for PCIe wake.
As of now wedon't have a platform where different wake pins connected to 
PCIe slot.
If any platform use new pins for PCIe wake then we can add to this list.


Manikanta

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Manikanta Maddireddy]

On 2/8/2023 5:07 PM, Thierry Reding wrote:
> On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
>> Add PCIe port node under the PCIe controller-1 device tree node to support
>> PCIe WAKE# interrupt for WiFi.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>
>> Changes in v14:
>> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
>>
>>   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> index 8a9747855d6b..9c89be263141 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>>   
>>   			phys = <&p2u_hsio_3>;
>>   			phy-names = "p2u-0";
>> +
>> +			pci@0,0 {
>> +				reg = <0x0000 0 0 0 0>;
>> +				#address-cells = <3>;
>> +				#size-cells = <2>;
>> +				ranges;
>> +
>> +				interrupt-parent = <&gpio>;
>> +				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
>> +				interrupt-names = "wakeup";
>> +			};
> Don't we need to wire this to the PMC interrupt controller and the wake
> event corresponding to the L2 GPIO? Otherwise none of the wake logic in
> PMC will get invoked.
>
> Thierry
PCIe wake is gpio based not pmc, only wake support is provided by PMC 
controller.
I verified this patch and able to wake up Tegra from suspend.
Petlozu, correct me if my understanding is wrong.


Manikanta

Re: [RFC,v14 3/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Manikanta Maddireddy]

On 2/8/2023 5:20 PM, Thierry Reding wrote:
> On Wed, Feb 08, 2023 at 04:46:43PM +0530, Manikanta Maddireddy wrote:
>> From: Jeffy Chen <jeffy.chen@rock-chips.com>
>>
>> Add of_pci_setup_wake_irq() to parse the PCIe WAKE# interrupt from the
>> device tree and set the wake irq. Add of_pci_teardown_wake_irq() to clear
>> the wake irq.
>>
>> Call of_pci_setup_wake_irq() in pci_device_probe() to setup PCIe WAKE#
>> interrupt during PCIe Endpoint enumeration.
>>
>> Enable or disable PCIe WAKE# interrupt in platform_pci_set_wakeup().
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>
>> Changes in v14:
>> pci_platform_pm_ops structure is removed in latest kernel, so dropped
>> pci-of driver. Instead, enable wake in platform_pci_set_wakeup().
>>
>> Changes in v13:
>> Fix compiler error reported by kbuild test robot <fengguang.wu@intel.com>
>>
>> Changes in v12:
>> Enable the wake irq in noirq stage to avoid possible irq storm.
>>
>> Changes in v11:
>> Only support 1-per-device PCIe WAKE# pin as suggested.
>>
>> Changes in v10:
>> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
>> since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
>>
>> Changes in v9:
>> Fix check error in .cleanup().
>> Move dedicated wakeirq setup to setup() callback and use
>> device_set_wakeup_enable() to enable/disable.
>>
>> Changes in v8:
>> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
>>
>> Changes in v7:
>> Move PCIE_WAKE handling into pci core.
>>
>> Changes in v6:
>> Fix device_init_wake error handling, and add some comments.
>>
>> Changes in v5:
>> Rebase.
>>
>> Changes in v3:
>> Fix error handling.
>>
>> Changes in v2:
>> Use dev_pm_set_dedicated_wake_irq.
>>
>>   drivers/pci/of.c         | 49 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci-driver.c | 10 ++++++++
>>   drivers/pci/pci.c        |  7 ++++++
>>   drivers/pci/pci.h        |  8 +++++++
>>   4 files changed, 74 insertions(+)
>>
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index ff897c40ed71..1c348e63f175 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/of_irq.h>
>>   #include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>> +#include <linux/pm_wakeirq.h>
>>   #include "pci.h"
>>   
>>   #ifdef CONFIG_PCI
>> @@ -705,3 +706,51 @@ 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);
>> +
>> +int of_pci_setup_wake_irq(struct pci_dev *pdev)
>> +{
>> +	struct pci_dev *ppdev;
> Perhaps "parent" since that's what it is referring to? ppdev is a bit
> vague.
ppdev is already used in of_irq_parse_pci(), I think it mean parent pci_dev.
I see that parent is mostly used for pci_bus parent. Let me know if it 
is fine
to leave it as ppdev or need to rename it.
>
>> +	struct device_node *dn;
>> +	int ret, irq;
>> +
>> +	/* Get the pci_dev of our parent. Hopefully it's a port. */
>> +	ppdev = pdev->bus->self;
>> +	/* Nope, it's a host bridge. */
>> +	if (!ppdev)
>> +		return 0;
>> +
>> +	dn = pci_device_to_OF_node(ppdev);
>> +	if (!dn)
>> +		return 0;
>> +
>> +	irq = of_irq_get_byname(dn, "wakeup");
>> +	if (irq == -EPROBE_DEFER) {
>> +		return irq;
>> +	} else if (irq < 0) {
>> +		/* Ignore other errors, since a missing wakeup is non-fatal. */
>> +		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
> dev_dbg() maybe? As it is this would add an annoying info message for
> basically every PCI controller on every DT-based board out there.
>
> Thierry
Ack. I will wait for few days for other review this series before 
sending new patch set.

Manikanta

Re: [RFC,v14 2/5] of/irq: Adjust of_pci_irq parsing for multiple interrupts

[Manikanta Maddireddy]

On 2/8/2023 5:14 PM, Thierry Reding wrote:
> On Wed, Feb 08, 2023 at 04:46:42PM +0530, Manikanta Maddireddy wrote:
>> From: Jeffy Chen <jeffy.chen@rock-chips.com>
>>
>> Currently we are considering the first irq as the PCI interrupt pin,
>> but a PCI device may have multiple interrupts(e.g. PCIe WAKE# pin).
>>
>> Only parse the PCI interrupt pin when the irq is unnamed or named as
>> "pci".
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>
>> Changes in v14:
>> Address Rob's comment on using of_property_match_string().
>>
>> Changes in v13: None
>> Changes in v12: None
>> Changes in v11:
>> Address Brian's comments.
>>
>> Changes in v10: None
>> Changes in v9: None
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/pci/of.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index 196834ed44fe..ff897c40ed71 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -429,9 +429,17 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
>>   	 */
>>   	dn = pci_device_to_OF_node(pdev);
>>   	if (dn) {
>> -		rc = of_irq_parse_one(dn, 0, out_irq);
>> -		if (!rc)
>> -			return rc;
>> +		int index = 0;
> No need to initialize to 0 here since you're assigning to it immediately
> below.
>
> Otherwise, looks good, so with that initialization dropped, this is:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
Ack, I will take care of it next version.

Manikanta

Re: [RFC,v14 1/5] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq

[Rob Herring]

On Wed, Feb 8, 2023 at 5:17 AM Manikanta Maddireddy
<mmaddireddy@nvidia.com> wrote:
>
> From: Jeffy Chen <jeffy.chen@rock-chips.com>
>
> Add device tree support to pass PCIe WAKE# pin information to PCI core
> driver. To support PCIe WAKE# and PCI irqs, add definition of the optional
> properties "interrupts" and "interrupt-names". These properties should be
> defined by the PCIe port to which wake capable Endpoint is connected,
> so the definition is added under "PCI-PCI Bridge properties" section.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

I did? 5 years ago it seems. Times change and evolve. Don't add to
pci.txt. This must be a schema now. PCI schema lives in dtschema.

Rob

Re: [RFC,v14 1/5] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq

[Manikanta Maddireddy]

On 2/8/2023 7:23 PM, Rob Herring wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Feb 8, 2023 at 5:17 AM Manikanta Maddireddy
> <mmaddireddy@nvidia.com> wrote:
>> From: Jeffy Chen <jeffy.chen@rock-chips.com>
>>
>> Add device tree support to pass PCIe WAKE# pin information to PCI core
>> driver. To support PCIe WAKE# and PCI irqs, add definition of the optional
>> properties "interrupts" and "interrupt-names". These properties should be
>> defined by the PCIe port to which wake capable Endpoint is connected,
>> so the definition is added under "PCI-PCI Bridge properties" section.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
> I did? 5 years ago it seems. Times change and evolve. Don't add to
> pci.txt. This must be a schema now. PCI schema lives in dtschema.
>
> Rob
I will prepare new patch in dtschema and send in next version.

Manikanta

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2468 bytes --]

On Wed, Feb 08, 2023 at 05:43:35PM +0530, Manikanta Maddireddy wrote:
> 
> On 2/8/2023 5:07 PM, Thierry Reding wrote:
> > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> > > Add PCIe port node under the PCIe controller-1 device tree node to support
> > > PCIe WAKE# interrupt for WiFi.
> > > 
> > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > ---
> > > 
> > > Changes in v14:
> > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> > > 
> > >   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > index 8a9747855d6b..9c89be263141 100644
> > > --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > >   			phys = <&p2u_hsio_3>;
> > >   			phy-names = "p2u-0";
> > > +
> > > +			pci@0,0 {
> > > +				reg = <0x0000 0 0 0 0>;
> > > +				#address-cells = <3>;
> > > +				#size-cells = <2>;
> > > +				ranges;
> > > +
> > > +				interrupt-parent = <&gpio>;
> > > +				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> > > +				interrupt-names = "wakeup";
> > > +			};
> > Don't we need to wire this to the PMC interrupt controller and the wake
> > event corresponding to the L2 GPIO? Otherwise none of the wake logic in
> > PMC will get invoked.
> > 
> > Thierry
> PCIe wake is gpio based not pmc, only wake support is provided by PMC
> controller.
> I verified this patch and able to wake up Tegra from suspend.
> Petlozu, correct me if my understanding is wrong.

The way that this usually works is that you need to use something like
this:

	interrupt-parent = <&pmc>;
	interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
	interrupt-names = "wakeup";

This will then cause the PMC's interrupt chip callbacks to setup all the
wake-related interrupts and use the internal wake event tables to
forward the GPIO/IRQ corresponding to the PMC wake event to the GPIO
controller or GIC, respectively.

If you use &gpio as the interrupt parent, none of the PMC logic will be
invoked, so unless this is somehow set up correctly by default, the PMC
wouldn't be able to wake up the system.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Petlozu Pravareshwar]

> 
> On Wed, Feb 08, 2023 at 05:43:35PM +0530, Manikanta Maddireddy wrote:
> >
> > On 2/8/2023 5:07 PM, Thierry Reding wrote:
> > > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy
> wrote:
> > > > Add PCIe port node under the PCIe controller-1 device tree node to
> > > > support PCIe WAKE# interrupt for WiFi.
> > > >
> > > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > > ---
> > > >
> > > > Changes in v14:
> > > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX
> Orin.
> > > >
> > > >   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11
> +++++++++++
> > > >   1 file changed, 11 insertions(+)
> > > >
> > > > diff --git
> > > > a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > index 8a9747855d6b..9c89be263141 100644
> > > > ---
> > > > a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-
> 0000.dt
> > > > +++ s
> > > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > > >   			phys = <&p2u_hsio_3>;
> > > >   			phy-names = "p2u-0";
> > > > +
> > > > +			pci@0,0 {
> > > > +				reg = <0x0000 0 0 0 0>;
> > > > +				#address-cells = <3>;
> > > > +				#size-cells = <2>;
> > > > +				ranges;
> > > > +
> > > > +				interrupt-parent = <&gpio>;
> > > > +				interrupts = <TEGRA234_MAIN_GPIO(L, 2)
> IRQ_TYPE_LEVEL_LOW>;
> > > > +				interrupt-names = "wakeup";
> > > > +			};
> > > Don't we need to wire this to the PMC interrupt controller and the
> > > wake event corresponding to the L2 GPIO? Otherwise none of the wake
> > > logic in PMC will get invoked.
> > >
> > > Thierry
> > PCIe wake is gpio based not pmc, only wake support is provided by PMC
> > controller.
> > I verified this patch and able to wake up Tegra from suspend.
> > Petlozu, correct me if my understanding is wrong.
> 
> The way that this usually works is that you need to use something like
> this:
> 
> 	interrupt-parent = <&pmc>;
> 	interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
> 	interrupt-names = "wakeup";
> 
> This will then cause the PMC's interrupt chip callbacks to setup all the wake-
> related interrupts and use the internal wake event tables to forward the
> GPIO/IRQ corresponding to the PMC wake event to the GPIO controller or
> GIC, respectively.
> 
> If you use &gpio as the interrupt parent, none of the PMC logic will be
> invoked, so unless this is somehow set up correctly by default, the PMC
> wouldn't be able to wake up the system.
> 
> Thierry
Thierry,
Since PMC's IRQ domain is made as parent of GPIO controller's IRQ domain,
I think, for GPIO based wakes setting &gpio as the interrupt parent can still
invoke PMC logic to program the required registers to enable such wakes.
Related commit in this regard:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpio/gpio-tegra186.c?id=2a36550567307b881ce570a81189682ae1c9d08d

Thanks.

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3557 bytes --]

On Thu, Feb 09, 2023 at 10:53:25AM +0000, Petlozu Pravareshwar wrote:
> > 
> > On Wed, Feb 08, 2023 at 05:43:35PM +0530, Manikanta Maddireddy wrote:
> > >
> > > On 2/8/2023 5:07 PM, Thierry Reding wrote:
> > > > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy
> > wrote:
> > > > > Add PCIe port node under the PCIe controller-1 device tree node to
> > > > > support PCIe WAKE# interrupt for WiFi.
> > > > >
> > > > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > > > ---
> > > > >
> > > > > Changes in v14:
> > > > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX
> > Orin.
> > > > >
> > > > >   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11
> > +++++++++++
> > > > >   1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git
> > > > > a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > index 8a9747855d6b..9c89be263141 100644
> > > > > ---
> > > > > a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-
> > 0000.dt
> > > > > +++ s
> > > > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > > > >   			phys = <&p2u_hsio_3>;
> > > > >   			phy-names = "p2u-0";
> > > > > +
> > > > > +			pci@0,0 {
> > > > > +				reg = <0x0000 0 0 0 0>;
> > > > > +				#address-cells = <3>;
> > > > > +				#size-cells = <2>;
> > > > > +				ranges;
> > > > > +
> > > > > +				interrupt-parent = <&gpio>;
> > > > > +				interrupts = <TEGRA234_MAIN_GPIO(L, 2)
> > IRQ_TYPE_LEVEL_LOW>;
> > > > > +				interrupt-names = "wakeup";
> > > > > +			};
> > > > Don't we need to wire this to the PMC interrupt controller and the
> > > > wake event corresponding to the L2 GPIO? Otherwise none of the wake
> > > > logic in PMC will get invoked.
> > > >
> > > > Thierry
> > > PCIe wake is gpio based not pmc, only wake support is provided by PMC
> > > controller.
> > > I verified this patch and able to wake up Tegra from suspend.
> > > Petlozu, correct me if my understanding is wrong.
> > 
> > The way that this usually works is that you need to use something like
> > this:
> > 
> > 	interrupt-parent = <&pmc>;
> > 	interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
> > 	interrupt-names = "wakeup";
> > 
> > This will then cause the PMC's interrupt chip callbacks to setup all the wake-
> > related interrupts and use the internal wake event tables to forward the
> > GPIO/IRQ corresponding to the PMC wake event to the GPIO controller or
> > GIC, respectively.
> > 
> > If you use &gpio as the interrupt parent, none of the PMC logic will be
> > invoked, so unless this is somehow set up correctly by default, the PMC
> > wouldn't be able to wake up the system.
> > 
> > Thierry
> Thierry,
> Since PMC's IRQ domain is made as parent of GPIO controller's IRQ domain,
> I think, for GPIO based wakes setting &gpio as the interrupt parent can still
> invoke PMC logic to program the required registers to enable such wakes.
> Related commit in this regard:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpio/gpio-tegra186.c?id=2a36550567307b881ce570a81189682ae1c9d08d

Heh... nicely self-owned =). You're right, no need for the detour in DT
with those, the GPIO driver will hook up the IRQ hierarchy itself. We
already do this for the "power" key in the various gpio-keys, so it
should work fine.

Sorry for the noise,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC,v14 0/5] Add DT based PCIe wake support in PCI core driver

[Krishna Chaitanya Chundru]

Hi Manikanta,

I don't see any update on this series after comments.

Is there  any plans to take up this series.

Thanks & Regards,

Krishna Chaitanya.

On 2/8/2023 4:46 PM, Manikanta Maddireddy wrote:
> Below series [1] attempted to support DT based PCIe wake feature in generic
> PCI core driver. This series was left at v13 and final comments are not
> addressed. I am continuing this series from v14 by addressing all comments
> in v13. I dropped rockchip device tree patch because I don't have hardware
> to verify it. Instead, I verified these patches on NVIDIA Jetson AGX Orin
> Developer Kit and included its device tree changes in this series.
>
> [1] https://lore.kernel.org/all/20171226023646.17722-1-jeffy.chen@rock-chips.com/
>
> Changes in v14:
> Updated commit message for DT bindings patch to reflect that DT properties
> are tied to PCI-PCI Bridge.
> Addressed review comments on PCI interrupt parsing patch.
> Dropped rockchip device tree patch.
> Added Jetson AGX OrinDeveloper Kit device tree and Tegra PMC patches.
>
> Changes in v13:
> Fix compiler error reported by kbuild test robot <fengguang.wu@intel.com>
>
> Changes in v12:
> Only add irq definitions for PCI devices and rewrite the commit message.
> Enable the wake irq in noirq stage to avoid possible irq storm.
>
> Changes in v11:
> Address Brian's comments.
> Only support 1-per-device PCIe WAKE# pin as suggested.
> Move to pcie port as Brian suggested.
>
> Changes in v10:
> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
> since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
>
> Changes in v9:
> Add section for PCI devices and rewrite the commit message.
> Fix check error in .cleanup().
> Move dedicated wakeirq setup to setup() callback and use
> device_set_wakeup_enable() to enable/disable.
> Rewrite the commit message.
>
> Changes in v8:
> Add optional "pci", and rewrite commit message.
> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
> Rewrite the commit message.
>
> Changes in v7:
> Move PCIE_WAKE handling into pci core.
>
> Changes in v6:
> Fix device_init_wake error handling, and add some comments.
>
> Changes in v5:
> Move to pci.txt
> Rebase.
> Use "wakeup" instead of "wake"
>
> Changes in v3:
> Fix error handling.
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq.
>
> Jeffy Chen (3):
>    dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
>    of/irq: Adjust of_pci_irq parsing for multiple interrupts
>    PCI / PM: Add support for the PCIe WAKE# signal for OF
>
> Manikanta Maddireddy (2):
>    arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller
>    soc/tegra: pmc: Add Tegra234 PCIe wake event
>
>   Documentation/devicetree/bindings/pci/pci.txt |  8 +++
>   .../nvidia/tegra234-p3737-0000+p3701-0000.dts | 11 ++++
>   drivers/pci/of.c                              | 63 ++++++++++++++++++-
>   drivers/pci/pci-driver.c                      | 10 +++
>   drivers/pci/pci.c                             |  7 +++
>   drivers/pci/pci.h                             |  8 +++
>   drivers/soc/tegra/pmc.c                       |  1 +
>   7 files changed, 105 insertions(+), 3 deletions(-)
>

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Manivannan Sadhasivam]

On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> Add PCIe port node under the PCIe controller-1 device tree node to support
> PCIe WAKE# interrupt for WiFi.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> 
> Changes in v14:
> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> 
>  .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> index 8a9747855d6b..9c89be263141 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>  
>  			phys = <&p2u_hsio_3>;
>  			phy-names = "p2u-0";
> +
> +			pci@0,0 {
> +				reg = <0x0000 0 0 0 0>;
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +
> +				interrupt-parent = <&gpio>;
> +				interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> +				interrupt-names = "wakeup";

WAKE# should be part of the PCIe controller, not device. And the interrupt name
should be "wake".

- Mani

> +			};
>  		};
>  
>  		pcie@14160000 {
> -- 
> 2.25.1
> 

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

Re: [RFC,v14 0/5] Add DT based PCIe wake support in PCI core driver

[Manikanta Maddireddy]

Hi Krishna Chaitanya,

Unfortunately I cannot follow up on this series now.
If you have a platform with same requirement, please verify and publish 
new version.

Thanks,
Manikanta


On 06-12-2023 20:14, Krishna Chaitanya Chundru wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Manikanta,
>
> I don't see any update on this series after comments.
>
> Is there  any plans to take up this series.
>
> Thanks & Regards,
>
> Krishna Chaitanya.
>
> On 2/8/2023 4:46 PM, Manikanta Maddireddy wrote:
>> Below series [1] attempted to support DT based PCIe wake feature in 
>> generic
>> PCI core driver. This series was left at v13 and final comments are not
>> addressed. I am continuing this series from v14 by addressing all 
>> comments
>> in v13. I dropped rockchip device tree patch because I don't have 
>> hardware
>> to verify it. Instead, I verified these patches on NVIDIA Jetson AGX 
>> Orin
>> Developer Kit and included its device tree changes in this series.
>>
>> [1] 
>> https://lore.kernel.org/all/20171226023646.17722-1-jeffy.chen@rock-chips.com/
>>
>> Changes in v14:
>> Updated commit message for DT bindings patch to reflect that DT 
>> properties
>> are tied to PCI-PCI Bridge.
>> Addressed review comments on PCI interrupt parsing patch.
>> Dropped rockchip device tree patch.
>> Added Jetson AGX OrinDeveloper Kit device tree and Tegra PMC patches.
>>
>> Changes in v13:
>> Fix compiler error reported by kbuild test robot 
>> <fengguang.wu@intel.com>
>>
>> Changes in v12:
>> Only add irq definitions for PCI devices and rewrite the commit message.
>> Enable the wake irq in noirq stage to avoid possible irq storm.
>>
>> Changes in v11:
>> Address Brian's comments.
>> Only support 1-per-device PCIe WAKE# pin as suggested.
>> Move to pcie port as Brian suggested.
>>
>> Changes in v10:
>> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
>> since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
>>
>> Changes in v9:
>> Add section for PCI devices and rewrite the commit message.
>> Fix check error in .cleanup().
>> Move dedicated wakeirq setup to setup() callback and use
>> device_set_wakeup_enable() to enable/disable.
>> Rewrite the commit message.
>>
>> Changes in v8:
>> Add optional "pci", and rewrite commit message.
>> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
>> Rewrite the commit message.
>>
>> Changes in v7:
>> Move PCIE_WAKE handling into pci core.
>>
>> Changes in v6:
>> Fix device_init_wake error handling, and add some comments.
>>
>> Changes in v5:
>> Move to pci.txt
>> Rebase.
>> Use "wakeup" instead of "wake"
>>
>> Changes in v3:
>> Fix error handling.
>>
>> Changes in v2:
>> Use dev_pm_set_dedicated_wake_irq.
>>
>> Jeffy Chen (3):
>>    dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
>>    of/irq: Adjust of_pci_irq parsing for multiple interrupts
>>    PCI / PM: Add support for the PCIe WAKE# signal for OF
>>
>> Manikanta Maddireddy (2):
>>    arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller
>>    soc/tegra: pmc: Add Tegra234 PCIe wake event
>>
>>   Documentation/devicetree/bindings/pci/pci.txt |  8 +++
>>   .../nvidia/tegra234-p3737-0000+p3701-0000.dts | 11 ++++
>>   drivers/pci/of.c                              | 63 ++++++++++++++++++-
>>   drivers/pci/pci-driver.c                      | 10 +++
>>   drivers/pci/pci.c                             |  7 +++
>>   drivers/pci/pci.h                             |  8 +++
>>   drivers/soc/tegra/pmc.c                       |  1 +
>>   7 files changed, 105 insertions(+), 3 deletions(-)
>>

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Manikanta Maddireddy]

On 06-12-2023 21:06, Manivannan Sadhasivam wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
>> Add PCIe port node under the PCIe controller-1 device tree node to support
>> PCIe WAKE# interrupt for WiFi.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>
>> Changes in v14:
>> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
>>
>>   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> index 8a9747855d6b..9c89be263141 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>>
>>                        phys = <&p2u_hsio_3>;
>>                        phy-names = "p2u-0";
>> +
>> +                     pci@0,0 {
>> +                             reg = <0x0000 0 0 0 0>;
>> +                             #address-cells = <3>;
>> +                             #size-cells = <2>;
>> +                             ranges;
>> +
>> +                             interrupt-parent = <&gpio>;
>> +                             interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
>> +                             interrupt-names = "wakeup";
> WAKE# should be part of the PCIe controller, not device. And the interrupt name
> should be "wake".
>
> - Mani
Hi,

Please refer to the discussion in below link, WAKE# is per PCI bridge.
https://patchwork.ozlabs.org/project/linux-pci/patch/20171226020806.32710-2-jeffy.chen@rock-chips.com/

I carried wakeup name defined in previous version, but wake seems to be 
sufficient.

Thanks,
Manikanta
>
>> +                     };
>>                };
>>
>>                pcie@14160000 {
>> --
>> 2.25.1
>>
> --
> மணிவண்ணன் சதாசிவம்

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Manivannan Sadhasivam]

On Thu, Dec 07, 2023 at 12:54:04PM +0530, Manikanta Maddireddy wrote:
> 
> On 06-12-2023 21:06, Manivannan Sadhasivam wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> > > Add PCIe port node under the PCIe controller-1 device tree node to support
> > > PCIe WAKE# interrupt for WiFi.
> > > 
> > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > ---
> > > 
> > > Changes in v14:
> > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> > > 
> > >   .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > index 8a9747855d6b..9c89be263141 100644
> > > --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > > 
> > >                        phys = <&p2u_hsio_3>;
> > >                        phy-names = "p2u-0";
> > > +
> > > +                     pci@0,0 {
> > > +                             reg = <0x0000 0 0 0 0>;
> > > +                             #address-cells = <3>;
> > > +                             #size-cells = <2>;
> > > +                             ranges;
> > > +
> > > +                             interrupt-parent = <&gpio>;
> > > +                             interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> > > +                             interrupt-names = "wakeup";
> > WAKE# should be part of the PCIe controller, not device. And the interrupt name
> > should be "wake".
> > 
> > - Mani
> Hi,
> 
> Please refer to the discussion in below link, WAKE# is per PCI bridge.
> https://patchwork.ozlabs.org/project/linux-pci/patch/20171226020806.32710-2-jeffy.chen@rock-chips.com/
> 

PCIe Host controller (RC) usually represents host bridge + PCI-PCI bridge. We do
not represent the PCI-PCI bridge in devicetree for any platforms, but only RC as
a whole.

Moreover, PERST# is already defined in RC node. So it becomes confusing if
WAKE# is defined in a child node representing bridge.

So please move WAKE# to RC node.

- Mani

> I carried wakeup name defined in previous version, but wake seems to be
> sufficient.
> 
> Thanks,
> Manikanta
> > 
> > > +                     };
> > >                };
> > > 
> > >                pcie@14160000 {
> > > --
> > > 2.25.1
> > > 
> > --
> > மணிவண்ணன் சதாசிவம்

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

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Manikanta Maddireddy]

On 07-12-2023 13:29, Manivannan Sadhasivam wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, Dec 07, 2023 at 12:54:04PM +0530, Manikanta Maddireddy wrote:
>> On 06-12-2023 21:06, Manivannan Sadhasivam wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
>>>> Add PCIe port node under the PCIe controller-1 device tree node to support
>>>> PCIe WAKE# interrupt for WiFi.
>>>>
>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>> ---
>>>>
>>>> Changes in v14:
>>>> New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
>>>>
>>>>    .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> index 8a9747855d6b..9c89be263141 100644
>>>> --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
>>>> @@ -2147,6 +2147,17 @@ pcie@14100000 {
>>>>
>>>>                         phys = <&p2u_hsio_3>;
>>>>                         phy-names = "p2u-0";
>>>> +
>>>> +                     pci@0,0 {
>>>> +                             reg = <0x0000 0 0 0 0>;
>>>> +                             #address-cells = <3>;
>>>> +                             #size-cells = <2>;
>>>> +                             ranges;
>>>> +
>>>> +                             interrupt-parent = <&gpio>;
>>>> +                             interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
>>>> +                             interrupt-names = "wakeup";
>>> WAKE# should be part of the PCIe controller, not device. And the interrupt name
>>> should be "wake".
>>>
>>> - Mani
>> Hi,
>>
>> Please refer to the discussion in below link, WAKE# is per PCI bridge.
>> https://patchwork.ozlabs.org/project/linux-pci/patch/20171226020806.32710-2-jeffy.chen@rock-chips.com/
>>
> PCIe Host controller (RC) usually represents host bridge + PCI-PCI bridge. We do
> not represent the PCI-PCI bridge in devicetree for any platforms, but only RC as
> a whole.
>
> Moreover, PERST# is already defined in RC node. So it becomes confusing if
> WAKE# is defined in a child node representing bridge.
>
> So please move WAKE# to RC node.
>
> - Mani

Hi,

We can define PCI-PCI bridge in device tree, refer to below device tree 
which has 3 ports under a controller,
with PERST#(reset-gpios) defined per port.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/apple/t8103.dtsi#n749

Also, of_pci_setup_wake_irq() in below patch is parsing "wakeup" from 
PCI bridge, not from the host bridge.
https://patchwork.ozlabs.org/project/linux-pci/patch/20230208111645.3863534-4-mmaddireddy@nvidia.com/

If a controller has only one port it has to define a PCI bridge under 
controller device tree node and
add wakeup interrupt property, refer to below patch from original author.

https://www.spinics.net/lists/linux-pci/msg135569.html

Thanks,
Manikanta
>
>> I carried wakeup name defined in previous version, but wake seems to be
>> sufficient.
>>
>> Thanks,
>> Manikanta
>>>> +                     };
>>>>                 };
>>>>
>>>>                 pcie@14160000 {
>>>> --
>>>> 2.25.1
>>>>
>>> --
>>> மணிவண்ணன் சதாசிவம்
> --
> மணிவண்ணன் சதாசிவம்

Re: [RFC,v14 4/5] arm64: tegra: Add PCIe port node with PCIe WAKE# for C1 controller

[Manivannan Sadhasivam]

On Thu, Dec 07, 2023 at 02:23:46PM +0530, Manikanta Maddireddy wrote:
> 
> On 07-12-2023 13:29, Manivannan Sadhasivam wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Thu, Dec 07, 2023 at 12:54:04PM +0530, Manikanta Maddireddy wrote:
> > > On 06-12-2023 21:06, Manivannan Sadhasivam wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Wed, Feb 08, 2023 at 04:46:44PM +0530, Manikanta Maddireddy wrote:
> > > > > Add PCIe port node under the PCIe controller-1 device tree node to support
> > > > > PCIe WAKE# interrupt for WiFi.
> > > > > 
> > > > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > > > ---
> > > > > 
> > > > > Changes in v14:
> > > > > New patch in the series to support PCIe WAKE# in NVIDIA Jetson AGX Orin.
> > > > > 
> > > > >    .../dts/nvidia/tegra234-p3737-0000+p3701-0000.dts     | 11 +++++++++++
> > > > >    1 file changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > index 8a9747855d6b..9c89be263141 100644
> > > > > --- a/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > +++ b/arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dts
> > > > > @@ -2147,6 +2147,17 @@ pcie@14100000 {
> > > > > 
> > > > >                         phys = <&p2u_hsio_3>;
> > > > >                         phy-names = "p2u-0";
> > > > > +
> > > > > +                     pci@0,0 {
> > > > > +                             reg = <0x0000 0 0 0 0>;
> > > > > +                             #address-cells = <3>;
> > > > > +                             #size-cells = <2>;
> > > > > +                             ranges;
> > > > > +
> > > > > +                             interrupt-parent = <&gpio>;
> > > > > +                             interrupts = <TEGRA234_MAIN_GPIO(L, 2) IRQ_TYPE_LEVEL_LOW>;
> > > > > +                             interrupt-names = "wakeup";
> > > > WAKE# should be part of the PCIe controller, not device. And the interrupt name
> > > > should be "wake".
> > > > 
> > > > - Mani
> > > Hi,
> > > 
> > > Please refer to the discussion in below link, WAKE# is per PCI bridge.
> > > https://patchwork.ozlabs.org/project/linux-pci/patch/20171226020806.32710-2-jeffy.chen@rock-chips.com/
> > > 
> > PCIe Host controller (RC) usually represents host bridge + PCI-PCI bridge. We do
> > not represent the PCI-PCI bridge in devicetree for any platforms, but only RC as
> > a whole.
> > 
> > Moreover, PERST# is already defined in RC node. So it becomes confusing if
> > WAKE# is defined in a child node representing bridge.
> > 
> > So please move WAKE# to RC node.
> > 
> > - Mani
> 
> Hi,
> 
> We can define PCI-PCI bridge in device tree, refer to below device tree
> which has 3 ports under a controller,
> with PERST#(reset-gpios) defined per port.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/apple/t8103.dtsi#n749
> 

Hmm. For RCs with single bridge, we never defined a DT node (atleast on Qcom
platforms). But I think it is the time to fix them.

> Also, of_pci_setup_wake_irq() in below patch is parsing "wakeup" from PCI
> bridge, not from the host bridge.
> https://patchwork.ozlabs.org/project/linux-pci/patch/20230208111645.3863534-4-mmaddireddy@nvidia.com/
> 

I didn't say that WAKE# should be parsed from host bridge, it doesn't make
sense. But I get your point.

> If a controller has only one port it has to define a PCI bridge under
> controller device tree node and
> add wakeup interrupt property, refer to below patch from original author.
> 
> https://www.spinics.net/lists/linux-pci/msg135569.html
> 

Yes, I agree. Thanks for the clarification.

- Mani

> Thanks,
> Manikanta
> > 
> > > I carried wakeup name defined in previous version, but wake seems to be
> > > sufficient.
> > > 
> > > Thanks,
> > > Manikanta
> > > > > +                     };
> > > > >                 };
> > > > > 
> > > > >                 pcie@14160000 {
> > > > > --
> > > > > 2.25.1
> > > > > 
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> > --
> > மணிவண்ணன் சதாசிவம்

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