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/[email protected]/
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 <[email protected]>
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
From: Jeffy Chen <[email protected]>
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 <[email protected]>
Signed-off-by: Manikanta Maddireddy <[email protected]>
---
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
From: Jeffy Chen <[email protected]>
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 <[email protected]>
Signed-off-by: Manikanta Maddireddy <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
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
From: Jeffy Chen <[email protected]>
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 <[email protected]>
Signed-off-by: Manikanta Maddireddy <[email protected]>
---
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 <[email protected]>
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
Add PCIe port node under the PCIe controller-1 device tree node to support
PCIe WAKE# interrupt for WiFi.
Signed-off-by: Manikanta Maddireddy <[email protected]>
---
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
Enable PCIe wake event for the Tegra234 SoC.
Signed-off-by: Manikanta Maddireddy <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
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 <[email protected]>
> ---
>
> 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
On Wed, Feb 08, 2023 at 04:46:42PM +0530, Manikanta Maddireddy wrote:
> From: Jeffy Chen <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Manikanta Maddireddy <[email protected]>
> ---
>
> 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 <[email protected]>
On Wed, Feb 08, 2023 at 04:46:43PM +0530, Manikanta Maddireddy wrote:
> From: Jeffy Chen <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Manikanta Maddireddy <[email protected]>
> ---
>
> 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 <[email protected]>
>
> 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
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 <[email protected]>
>> ---
>>
>> 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
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 <[email protected]>
>> ---
>>
>> 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
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Manikanta Maddireddy <[email protected]>
>> ---
>>
>> 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 <[email protected]>
>>
>> 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
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Manikanta Maddireddy <[email protected]>
>> ---
>>
>> 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 <[email protected]>
Ack, I will take care of it next version.
Manikanta
On Wed, Feb 8, 2023 at 5:17 AM Manikanta Maddireddy
<[email protected]> wrote:
>
> From: Jeffy Chen <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Manikanta Maddireddy <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
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
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
> <[email protected]> wrote:
>> From: Jeffy Chen <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Manikanta Maddireddy <[email protected]>
>> Reviewed-by: Rob Herring <[email protected]>
> 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
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 <[email protected]>
> > > ---
> > >
> > > 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
>
> 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 <[email protected]>
> > > > ---
> > > >
> > > > 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.
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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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
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/[email protected]/
>
> 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 <[email protected]>
>
> 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(-)
>
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 <[email protected]>
> ---
>
> 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
>
--
மணிவண்ணன் சதாசிவம்
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/[email protected]/
>>
>> 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
>> <[email protected]>
>>
>> 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(-)
>>
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 <[email protected]>
>> ---
>>
>> 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/[email protected]/
I carried wakeup name defined in previous version, but wake seems to be
sufficient.
Thanks,
Manikanta
>
>> + };
>> };
>>
>> pcie@14160000 {
>> --
>> 2.25.1
>>
> --
> மணிவண்ணன் சதாசிவம்
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 <[email protected]>
> > > ---
> > >
> > > 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/[email protected]/
>
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
> > >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
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 <[email protected]>
>>>> ---
>>>>
>>>> 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/[email protected]/
>>
> 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/[email protected]/
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
>>>>
>>> --
>>> மணிவண்ணன் சதாசிவம்
> --
> மணிவண்ணன் சதாசிவம்
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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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/[email protected]/
> > >
> > 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/[email protected]/
>
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
> > > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்