[RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core

[RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core

[Jeffy Chen]

Currently we are handling wake irq in mrvl wifi driver. Move it into
pci core.

Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).


Changes in v11:
Only add irq definitions for PCI devices and rewrite the commit message.
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 (5):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  mwifiex: Disable wakeup irq handling for pcie
  PCI / PM: Add support for the PCIe WAKE# signal for OF
  arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru

 Documentation/devicetree/bindings/pci/pci.txt | 10 ++++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  | 11 +++--
 drivers/net/wireless/marvell/mwifiex/main.c   |  4 ++
 drivers/of/of_pci_irq.c                       | 71 +++++++++++++++++++++++++--
 drivers/pci/pci-driver.c                      | 10 ++++
 include/linux/of_pci.h                        |  9 ++++
 6 files changed, 107 insertions(+), 8 deletions(-)

-- 
2.11.0

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

[Jeffy Chen]

We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
so add definitions of the optional PCIe WAKE# pin for PCI devices.

Also add an definition of the optional PCI interrupt pin for PCI
devices to distinguish it from the PCIe WAKE# pin.

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

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

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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index c77981c5dd18..3045ac452f27 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,13 @@ driver implementation may support the following properties:
    unsupported link speed, for instance, trying to do training for
    unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
    for gen2, and '1' for gen1. Any other values are invalid.
+
+PCI devices may support the following properties:
+
+- 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.
-- 
2.11.0

[RFC PATCH v11 2/5] of/irq: Adjust of_pci_irq parsing for multiple interrupts

[Jeffy Chen]

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

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/of/of_pci_irq.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index 3a05568f65df..d39565d5477b 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -27,9 +27,25 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq
 	 */
 	dn = pci_device_to_OF_node(pdev);
 	if (dn) {
-		rc = of_irq_parse_one(dn, 0, out_irq);
-		if (!rc)
-			return rc;
+		struct property *prop;
+		const char *name;
+		int index = 0;
+
+		of_property_for_each_string(dn, "interrupt-names", prop, name) {
+			if (!strcmp(name, "pci"))
+				break;
+			index++;
+		}
+
+		/*
+		 * Only parse from DT if we have no "interrupt-names",
+		 * or if we found an interrupt named "pci".
+		 */
+		if (index == 0 || name) {
+			rc = of_irq_parse_one(dn, index, out_irq);
+			if (!rc)
+				return rc;
+		}
 	}
 
 	/* Ok, we don't, time to have fun. Let's start by building up an
-- 
2.11.0

[RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Jeffy Chen]

Add of_pci_setup_wake_irq() and of_pci_teardown_wake_irq() to handle
the PCIe WAKE# interrupt.

Also use the dedicated wakeirq infrastructure to simplify it.

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

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/of/of_pci_irq.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-driver.c | 10 ++++++++++
 include/linux/of_pci.h   |  9 +++++++++
 3 files changed, 68 insertions(+)

diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index d39565d5477b..def884c1a37a 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -1,8 +1,57 @@
 #include <linux/kernel.h>
 #include <linux/of_pci.h>
 #include <linux/of_irq.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/export.h>
 
+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;
+	/* Ignore other errors, since a missing wakeup is non-fatal. */
+	else if (irq < 0) {
+		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;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
+
+void of_pci_teardown_wake_irq(struct pci_dev *pdev)
+{
+	if (!pdev->dev.power.wakeirq)
+		return;
+
+	dev_pm_clear_wake_irq(&pdev->dev);
+	device_init_wakeup(&pdev->dev, false);
+}
+EXPORT_SYMBOL_GPL(of_pci_teardown_wake_irq);
+
 /**
  * of_irq_parse_pci - Resolve the interrupt for a PCI device
  * @pdev:       the device whose interrupt is to be resolved
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index d79dbc377b9c..b4475ff35d97 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/cpu.h>
+#include <linux/of_pci.h>
 #include <linux/pm_runtime.h>
 #include <linux/suspend.h>
 #include <linux/kexec.h>
@@ -421,10 +422,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);
 	if (pci_device_can_probe(pci_dev)) {
 		error = __pci_device_probe(drv, pci_dev);
 		if (error) {
+			of_pci_teardown_wake_irq(pci_dev);
 			pcibios_free_irq(pci_dev);
 			pci_dev_put(pci_dev);
 		}
@@ -438,6 +446,8 @@ static int 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) {
 		if (drv->remove) {
 			pm_runtime_get_sync(dev);
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index bf588a05d0d0..80fe8ae3393e 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -10,6 +10,8 @@ struct of_phandle_args;
 struct device_node;
 
 #ifdef CONFIG_OF_PCI
+int of_pci_setup_wake_irq(struct pci_dev *pdev);
+void of_pci_teardown_wake_irq(struct pci_dev *pdev);
 int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq);
 struct device_node *of_pci_find_child_device(struct device_node *parent,
 					     unsigned int devfn);
@@ -23,6 +25,13 @@ int of_pci_map_rid(struct device_node *np, u32 rid,
 		   const char *map_name, const char *map_mask_name,
 		   struct device_node **target, u32 *id_out);
 #else
+static inline int of_pci_setup_wake_irq(struct pci_dev *pdev)
+{
+	return 0;
+}
+
+static void of_pci_teardown_wake_irq(struct pci_dev *pdev) { };
+
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {
 	return 0;
-- 
2.11.0

[RFC PATCH v11 5/5] arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru

[Jeffy Chen]

Currently we are handling PCIe WAKE# irq in mrvl wifi driver.

Move it to rockchip pcie port since we are going to handle it in the
pci core.

Also avoid this irq been considered as the PCI interrupt pin in the
of_irq_parse_pci().

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

Changes in v11:
Move to pcie port as Brian suggested.

Changes in v10: None
Changes in v9:
Rewrite the commit message.

Changes in v8:
Rewrite the commit message.

Changes in v7: None
Changes in v6: None
Changes in v5:
Use "wakeup" instead of "wake"

Changes in v3: None
Changes in v2: None

 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 03f195025390..be41d363efd8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -719,15 +719,16 @@ ap_i2c_audio: &i2c8 {
 		#size-cells = <2>;
 		ranges;
 
+		interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-names = "pci", "wakeup";
+		pinctrl-names = "default";
+		pinctrl-0 = <&wlan_host_wake_l>;
+		wakeup-source;
+
 		mvl_wifi: wifi@0,0 {
 			compatible = "pci1b4b,2b42";
 			reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
 			       0x83010000 0x0 0x00100000 0x0 0x00100000>;
-			interrupt-parent = <&gpio0>;
-			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&wlan_host_wake_l>;
-			wakeup-source;
 		};
 	};
 };
-- 
2.11.0

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Monday, December 25, 2017 12:47:41 PM CET Jeffy Chen wrote:
> Add of_pci_setup_wake_irq() and of_pci_teardown_wake_irq() to handle
> the PCIe WAKE# interrupt.
> 
> Also use the dedicated wakeirq infrastructure to simplify it.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> 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/of/of_pci_irq.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-driver.c | 10 ++++++++++
>  include/linux/of_pci.h   |  9 +++++++++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> index d39565d5477b..def884c1a37a 100644
> --- a/drivers/of/of_pci_irq.c
> +++ b/drivers/of/of_pci_irq.c
> @@ -1,8 +1,57 @@
>  #include <linux/kernel.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_irq.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/export.h>
>  
> +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");

Why is this a property of the bridge and not of the device itself?

> +	if (irq == -EPROBE_DEFER)

Braces here, please.

> +		return irq;
> +	/* Ignore other errors, since a missing wakeup is non-fatal. */
> +	else if (irq < 0) {
> +		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
> +		return 0;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, true);

Why do you call this before dev_pm_set_dedicated_wake_irq()?

> +
> +	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;
> +	}
> +

It would be more straightforward to call device_init_wakeup() here.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
> +
> +void of_pci_teardown_wake_irq(struct pci_dev *pdev)
> +{
> +	if (!pdev->dev.power.wakeirq)
> +		return;
> +
> +	dev_pm_clear_wake_irq(&pdev->dev);
> +	device_init_wakeup(&pdev->dev, false);
> +}
> +EXPORT_SYMBOL_GPL(of_pci_teardown_wake_irq);
> +
>  /**
>   * of_irq_parse_pci - Resolve the interrupt for a PCI device
>   * @pdev:       the device whose interrupt is to be resolved

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[JeffyChen]

Hi Rafael,

Thanks for your reply :)

On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
>> >+
>> >+	dn = pci_device_to_OF_node(ppdev);
>> >+	if (!dn)
>> >+		return 0;
>> >+
>> >+	irq = of_irq_get_byname(dn, "wakeup");
> Why is this a property of the bridge and not of the device itself?
That is suggested by Brian, because in that way, the wakeup pin would 
not "tied to what exact device is installed (or no device, if it's a slot)."

>
>> >+	if (irq == -EPROBE_DEFER)
> Braces here, please.
ok, will fix in the next version.

>
>> >+		return irq;
>> >+	/* Ignore other errors, since a missing wakeup is non-fatal. */
>> >+	else if (irq < 0) {
>> >+		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
>> >+		return 0;
>> >+	}
>> >+
>> >+	device_init_wakeup(&pdev->dev, true);
> Why do you call this before dev_pm_set_dedicated_wake_irq()?
hmmm, i thought so too, but it turns out the dedicated wake irq 
framework requires device_init_wakeup(dev, true) before attach the wake irq:

int device_wakeup_attach_irq(struct device *dev,
                              struct wake_irq *wakeirq)
{
         struct wakeup_source *ws;

         ws = dev->power.wakeup;
         if (!ws) {
                 dev_err(dev, "forgot to call device_init_wakeup?\n");
                 return -EINVAL;


>
>> >+
>> >+	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;
>> >+	}
>> >+
> It would be more straightforward to call device_init_wakeup() here.
>
>> >+	return 0;
>> >+}
>> >+EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
>> >+
>> >+void of_pci_teardown_wake_irq(struct pci_dev *pdev)
>> >+{
>> >+	if (!pdev->dev.power.wakeirq)
>> >+		return;
>> >+
>> >+	dev_pm_clear_wake_irq(&pdev->dev);
>> >+	device_init_wakeup(&pdev->dev, false);
>> >+}
>> >+EXPORT_SYMBOL_GPL(of_pci_teardown_wake_irq);
>> >+
>> >  /**
>> >   * of_irq_parse_pci - Resolve the interrupt for a PCI device
>> >   * @pdev:       the device whose interrupt is to be resolved
> Thanks,
> Rafael
>
>
>
>

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> Hi Rafael,
> 
> Thanks for your reply :)
> 
> On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
> >> >+
> >> >+	dn = pci_device_to_OF_node(ppdev);
> >> >+	if (!dn)
> >> >+		return 0;
> >> >+
> >> >+	irq = of_irq_get_byname(dn, "wakeup");
> > Why is this a property of the bridge and not of the device itself?
>
> That is suggested by Brian, because in that way, the wakeup pin would 
> not "tied to what exact device is installed (or no device, if it's a slot)."

But I don't think it works when there are two devices using different WAKE#
interrupt lines under the same bridge.  Or how does it work then?

> >> >+	if (irq == -EPROBE_DEFER)
> > Braces here, please.
> ok, will fix in the next version.
> 
> >
> >> >+		return irq;
> >> >+	/* Ignore other errors, since a missing wakeup is non-fatal. */
> >> >+	else if (irq < 0) {
> >> >+		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
> >> >+		return 0;
> >> >+	}
> >> >+
> >> >+	device_init_wakeup(&pdev->dev, true);
> > Why do you call this before dev_pm_set_dedicated_wake_irq()?
>
> hmmm, i thought so too, but it turns out the dedicated wake irq 
> framework requires device_init_wakeup(dev, true) before attach the wake irq:
> 
> int device_wakeup_attach_irq(struct device *dev,
>                               struct wake_irq *wakeirq)
> {
>          struct wakeup_source *ws;
> 
>          ws = dev->power.wakeup;
>          if (!ws) {
>                  dev_err(dev, "forgot to call device_init_wakeup?\n");
>                  return -EINVAL;
> 

Well, that's a framework issue, fair enough.

That said, what if user space removes the wakeup source from under you
concurrently via sysfs?  Tony?

Thanks,
Rafael

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

* Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]:
> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> > Hi Rafael,
> > 
> > Thanks for your reply :)
> > 
> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
> > >> >+
> > >> >+	dn = pci_device_to_OF_node(ppdev);
> > >> >+	if (!dn)
> > >> >+		return 0;
> > >> >+
> > >> >+	irq = of_irq_get_byname(dn, "wakeup");
> > > Why is this a property of the bridge and not of the device itself?
> >
> > That is suggested by Brian, because in that way, the wakeup pin would 
> > not "tied to what exact device is installed (or no device, if it's a slot)."
> 
> But I don't think it works when there are two devices using different WAKE#
> interrupt lines under the same bridge.  Or how does it work then?

It won't work currently for multiple devices but adding more than
one wakeriq per device is doable. And I think we will have other
cases where multiple wakeirqs are connected to a single device, so
that issue should be sorted out sooner or later.

And if requesting wakeirq for the PCI WAKE# lines at the PCI
controller does the job, then maybe that's all we need to start with.

Then in addition to that, we could do the following to allow
PCI devices to request the wakeirq from the PCI controller:

1. PCI controller or framework implements a chained irq for
   the WAKE# lines assuming it can mask/unmask the WAKE# lines

2. PCI devices then can just request the wakeirq from the PCI
   controller

And that's about it. Optionally we could leave out the dependency
to having PCI devices implement PM runtime and just resume the
parent (PCI controller) if PCI devices has not implemented
PM runtime.

> > >> >+	if (irq == -EPROBE_DEFER)
> > > Braces here, please.
> > ok, will fix in the next version.
> > 
> > >
> > >> >+		return irq;
> > >> >+	/* Ignore other errors, since a missing wakeup is non-fatal. */
> > >> >+	else if (irq < 0) {
> > >> >+		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
> > >> >+		return 0;
> > >> >+	}
> > >> >+
> > >> >+	device_init_wakeup(&pdev->dev, true);
> > > Why do you call this before dev_pm_set_dedicated_wake_irq()?
> >
> > hmmm, i thought so too, but it turns out the dedicated wake irq 
> > framework requires device_init_wakeup(dev, true) before attach the wake irq:
> > 
> > int device_wakeup_attach_irq(struct device *dev,
> >                               struct wake_irq *wakeirq)
> > {
> >          struct wakeup_source *ws;
> > 
> >          ws = dev->power.wakeup;
> >          if (!ws) {
> >                  dev_err(dev, "forgot to call device_init_wakeup?\n");
> >                  return -EINVAL;
> > 
> 
> Well, that's a framework issue, fair enough.
> 
> That said, what if user space removes the wakeup source from under you
> concurrently via sysfs?  Tony?

Hmm sounds racy, need to take a look. I think the only reason
to have the wakeirq pointer there was to save memory. It might
make sense to remove the wakeirq dependency here.

Regards,

Tony

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]:
>> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
>> > Hi Rafael,
>> >
>> > Thanks for your reply :)
>> >
>> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
>> > >> >+
>> > >> >+       dn = pci_device_to_OF_node(ppdev);
>> > >> >+       if (!dn)
>> > >> >+               return 0;
>> > >> >+
>> > >> >+       irq = of_irq_get_byname(dn, "wakeup");
>> > > Why is this a property of the bridge and not of the device itself?
>> >
>> > That is suggested by Brian, because in that way, the wakeup pin would
>> > not "tied to what exact device is installed (or no device, if it's a slot)."
>>
>> But I don't think it works when there are two devices using different WAKE#
>> interrupt lines under the same bridge.  Or how does it work then?
>
> It won't work currently for multiple devices but adding more than
> one wakeriq per device is doable. And I think we will have other
> cases where multiple wakeirqs are connected to a single device, so
> that issue should be sorted out sooner or later.
>
> And if requesting wakeirq for the PCI WAKE# lines at the PCI
> controller does the job, then maybe that's all we need to start with.

These are expected to be out-of-band, so not having anything to do
with the Root Complex.

In-band PME Messages go through the PCIe hierarchy, but that is a
standard mechanism and it is supported already.

WAKE# are platform-specific, pretty much by definition and I guess
that on most ARM boards they are just going to be some kind of GPIO
pins.

> Then in addition to that, we could do the following to allow
> PCI devices to request the wakeirq from the PCI controller:
>
> 1. PCI controller or framework implements a chained irq for
>    the WAKE# lines assuming it can mask/unmask the WAKE# lines
>
> 2. PCI devices then can just request the wakeirq from the PCI
>    controller
>
> And that's about it. Optionally we could leave out the dependency
> to having PCI devices implement PM runtime and just resume the
> parent (PCI controller) if PCI devices has not implemented
> PM runtime.

So if my understanding is correct, DT should give you the WAKE# IRQ
for the given endpoint PCI device and you only are expected to request
it.   The rest should just follow from the other pieces of information
in the DT.

With the quite obvious caveat that the same IRQ may be used as WAKE#
for multiple endpoint devices (which BTW need not be under the same
bridge even).

>> > >> >+       if (irq == -EPROBE_DEFER)
>> > > Braces here, please.
>> > ok, will fix in the next version.
>> >
>> > >
>> > >> >+               return irq;
>> > >> >+       /* Ignore other errors, since a missing wakeup is non-fatal. */
>> > >> >+       else if (irq < 0) {
>> > >> >+               dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
>> > >> >+               return 0;
>> > >> >+       }
>> > >> >+
>> > >> >+       device_init_wakeup(&pdev->dev, true);
>> > > Why do you call this before dev_pm_set_dedicated_wake_irq()?
>> >
>> > hmmm, i thought so too, but it turns out the dedicated wake irq
>> > framework requires device_init_wakeup(dev, true) before attach the wake irq:
>> >
>> > int device_wakeup_attach_irq(struct device *dev,
>> >                               struct wake_irq *wakeirq)
>> > {
>> >          struct wakeup_source *ws;
>> >
>> >          ws = dev->power.wakeup;
>> >          if (!ws) {
>> >                  dev_err(dev, "forgot to call device_init_wakeup?\n");
>> >                  return -EINVAL;
>> >
>>
>> Well, that's a framework issue, fair enough.
>>
>> That said, what if user space removes the wakeup source from under you
>> concurrently via sysfs?  Tony?
>
> Hmm sounds racy, need to take a look.

Not only racy, as I don't see anything to prevent user space from
making the dev->power.wakeup wakeup source go away via sysfs at any
time *after* the IRQ has been requested.

Pretty much right after dev_pm_set_dedicated_wake_irq() has returned,
device_wakeup_disable() may be called on the device via wakeup_store()
and it doesn't even check if the device has a wakeup irq.

> I think the only reason
> to have the wakeirq pointer there was to save memory. It might
> make sense to remove the wakeirq dependency here.

Well, that looks necessary to be honest.

Thanks,
Rafael

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

* Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171228 00:51]:
> On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Rafael J. Wysocki <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org> [171227 01:00]:
> >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> >> > Hi Rafael,
> >> >
> >> > Thanks for your reply :)
> >> >
> >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
> >> > >> >+
> >> > >> >+       dn = pci_device_to_OF_node(ppdev);
> >> > >> >+       if (!dn)
> >> > >> >+               return 0;
> >> > >> >+
> >> > >> >+       irq = of_irq_get_byname(dn, "wakeup");
> >> > > Why is this a property of the bridge and not of the device itself?
> >> >
> >> > That is suggested by Brian, because in that way, the wakeup pin would
> >> > not "tied to what exact device is installed (or no device, if it's a slot)."
> >>
> >> But I don't think it works when there are two devices using different WAKE#
> >> interrupt lines under the same bridge.  Or how does it work then?
> >
> > It won't work currently for multiple devices but adding more than
> > one wakeriq per device is doable. And I think we will have other
> > cases where multiple wakeirqs are connected to a single device, so
> > that issue should be sorted out sooner or later.
> >
> > And if requesting wakeirq for the PCI WAKE# lines at the PCI
> > controller does the job, then maybe that's all we need to start with.
> 
> These are expected to be out-of-band, so not having anything to do
> with the Root Complex.
> 
> In-band PME Messages go through the PCIe hierarchy, but that is a
> standard mechanism and it is supported already.
> 
> WAKE# are platform-specific, pretty much by definition and I guess
> that on most ARM boards they are just going to be some kind of GPIO
> pins.

OK. So probably supporting the following two configurations
should be enough then:

1. One or more WAKE# lines configured as a wakeirq for the PCI
   controller

   When the wakeirq calls pm_wakeup_event() for the PCI controller
   device driver, the PCI controller wakes up and can deal with
   it's child devices

2. Optionally a WAKE# line from a PCI device configured as wakeirq
   for the PCI device driver

   In this case calling the PM runtime resume in the child
   PCI device will also wake up the parent PCI controller,
   and then the PCI controller can deal with it's children

Seems like this series is pretty close to 1 above except
we need to have a list of wakeirqs per device instead of
just one. And option 2 should already work as long as the
PCI device driver parses and configures the wakeirq.

> > Then in addition to that, we could do the following to allow
> > PCI devices to request the wakeirq from the PCI controller:
> >
> > 1. PCI controller or framework implements a chained irq for
> >    the WAKE# lines assuming it can mask/unmask the WAKE# lines
> >
> > 2. PCI devices then can just request the wakeirq from the PCI
> >    controller
> >
> > And that's about it. Optionally we could leave out the dependency
> > to having PCI devices implement PM runtime and just resume the
> > parent (PCI controller) if PCI devices has not implemented
> > PM runtime.
> 
> So if my understanding is correct, DT should give you the WAKE# IRQ
> for the given endpoint PCI device and you only are expected to request
> it.   The rest should just follow from the other pieces of information
> in the DT.

Yeah and it seems that we should allow configuring both cases
1 and 2 above.

> With the quite obvious caveat that the same IRQ may be used as WAKE#
> for multiple endpoint devices (which BTW need not be under the same
> bridge even).

And with the shared interrupts we can't do the masking/unmasking
automatically..

> >> > >> >+       if (irq == -EPROBE_DEFER)
> >> > > Braces here, please.
> >> > ok, will fix in the next version.
> >> >
> >> > >
> >> > >> >+               return irq;
> >> > >> >+       /* Ignore other errors, since a missing wakeup is non-fatal. */
> >> > >> >+       else if (irq < 0) {
> >> > >> >+               dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
> >> > >> >+               return 0;
> >> > >> >+       }
> >> > >> >+
> >> > >> >+       device_init_wakeup(&pdev->dev, true);
> >> > > Why do you call this before dev_pm_set_dedicated_wake_irq()?
> >> >
> >> > hmmm, i thought so too, but it turns out the dedicated wake irq
> >> > framework requires device_init_wakeup(dev, true) before attach the wake irq:
> >> >
> >> > int device_wakeup_attach_irq(struct device *dev,
> >> >                               struct wake_irq *wakeirq)
> >> > {
> >> >          struct wakeup_source *ws;
> >> >
> >> >          ws = dev->power.wakeup;
> >> >          if (!ws) {
> >> >                  dev_err(dev, "forgot to call device_init_wakeup?\n");
> >> >                  return -EINVAL;
> >> >
> >>
> >> Well, that's a framework issue, fair enough.
> >>
> >> That said, what if user space removes the wakeup source from under you
> >> concurrently via sysfs?  Tony?
> >
> > Hmm sounds racy, need to take a look.
> 
> Not only racy, as I don't see anything to prevent user space from
> making the dev->power.wakeup wakeup source go away via sysfs at any
> time *after* the IRQ has been requested.

Currently nothing happens with wakeirqs if there's no struct
wakeup_source. On device_wakeup_enable() we call device_wakeup_attach()
that just copies dev->power.wakeirq to ws->wakeirq. And when struct
wake_source is freed the device should be active and wakeirq
disabled. Or are you seeing other issues here?

> Pretty much right after dev_pm_set_dedicated_wake_irq() has returned,
> device_wakeup_disable() may be called on the device via wakeup_store()
> and it doesn't even check if the device has a wakeup irq.
> 
> > I think the only reason
> > to have the wakeirq pointer there was to save memory. It might
> > make sense to remove the wakeirq dependency here.
> 
> Well, that looks necessary to be honest.

Seems like we're OK there except for the race. But I still wonder
if could just get rid of wakeirq in struct wakeup_source. Maybe
all we need is to see if dev->power.wakeup is allocated for the
wakeirqs.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Thu, Dec 28, 2017 at 5:22 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171228 00:51]:
>> On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> > * Rafael J. Wysocki <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org> [171227 01:00]:
>> >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
>> >> > Hi Rafael,
>> >> >
>> >> > Thanks for your reply :)
>> >> >
>> >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
>> >> > >> >+
>> >> > >> >+       dn = pci_device_to_OF_node(ppdev);
>> >> > >> >+       if (!dn)
>> >> > >> >+               return 0;
>> >> > >> >+
>> >> > >> >+       irq = of_irq_get_byname(dn, "wakeup");
>> >> > > Why is this a property of the bridge and not of the device itself?
>> >> >
>> >> > That is suggested by Brian, because in that way, the wakeup pin would
>> >> > not "tied to what exact device is installed (or no device, if it's a slot)."
>> >>
>> >> But I don't think it works when there are two devices using different WAKE#
>> >> interrupt lines under the same bridge.  Or how does it work then?
>> >
>> > It won't work currently for multiple devices but adding more than
>> > one wakeriq per device is doable. And I think we will have other
>> > cases where multiple wakeirqs are connected to a single device, so
>> > that issue should be sorted out sooner or later.
>> >
>> > And if requesting wakeirq for the PCI WAKE# lines at the PCI
>> > controller does the job, then maybe that's all we need to start with.
>>
>> These are expected to be out-of-band, so not having anything to do
>> with the Root Complex.
>>
>> In-band PME Messages go through the PCIe hierarchy, but that is a
>> standard mechanism and it is supported already.
>>
>> WAKE# are platform-specific, pretty much by definition and I guess
>> that on most ARM boards they are just going to be some kind of GPIO
>> pins.
>
> OK. So probably supporting the following two configurations
> should be enough then:
>
> 1. One or more WAKE# lines configured as a wakeirq for the PCI
>    controller
>
>    When the wakeirq calls pm_wakeup_event() for the PCI controller
>    device driver, the PCI controller wakes up and can deal with
>    it's child devices

But this shouldn't be necessary at all.  Or if it is, I wonder why
that's the case.

I'm assuming that we're talking about PCI Express here, which has two
wakeup mechanisms defined, one of which is based on using PME Messages
(Beacon) and the second one is WAKE#:

"The WAKE# mechanism uses sideband signaling to implement wakeup
functionality. WAKE# is
an “open drain” signal asserted by components requesting wakeup and
observed by the associated
power controller."

(from PCIe Base Spec 3.0).  [And there's a diagram showing the routing
of WAKE# in two cases in Figure 5-4: Conceptual Diagrams Showing Two
Example Cases of WAKE# Routing.]

Note that WAKE# is defined to be "observed by the associated power
controller", so I'm not sure what the PCI controller's role in the
handing of it is at all.

> 2. Optionally a WAKE# line from a PCI device configured as wakeirq
>    for the PCI device driver
>
>    In this case calling the PM runtime resume in the child
>    PCI device will also wake up the parent PCI controller,
>    and then the PCI controller can deal with it's children
>
> Seems like this series is pretty close to 1 above except
> we need to have a list of wakeirqs per device instead of
> just one. And option 2 should already work as long as the
> PCI device driver parses and configures the wakeirq.

Well, this is confusing, because as I said above, option 1 doesn't
look relevant even.

>> > Then in addition to that, we could do the following to allow
>> > PCI devices to request the wakeirq from the PCI controller:
>> >
>> > 1. PCI controller or framework implements a chained irq for
>> >    the WAKE# lines assuming it can mask/unmask the WAKE# lines
>> >
>> > 2. PCI devices then can just request the wakeirq from the PCI
>> >    controller
>> >
>> > And that's about it. Optionally we could leave out the dependency
>> > to having PCI devices implement PM runtime and just resume the
>> > parent (PCI controller) if PCI devices has not implemented
>> > PM runtime.
>>
>> So if my understanding is correct, DT should give you the WAKE# IRQ
>> for the given endpoint PCI device and you only are expected to request
>> it.   The rest should just follow from the other pieces of information
>> in the DT.
>
> Yeah and it seems that we should allow configuring both cases
> 1 and 2 above.
>
>> With the quite obvious caveat that the same IRQ may be used as WAKE#
>> for multiple endpoint devices (which BTW need not be under the same
>> bridge even).
>
> And with the shared interrupts we can't do the masking/unmasking
> automatically..

Or we need to use reference counting (so actually the wakeup IRQs are
not dedicated).

>> >> > >> >+       if (irq == -EPROBE_DEFER)
>> >> > > Braces here, please.
>> >> > ok, will fix in the next version.
>> >> >
>> >> > >
>> >> > >> >+               return irq;
>> >> > >> >+       /* Ignore other errors, since a missing wakeup is non-fatal. */
>> >> > >> >+       else if (irq < 0) {
>> >> > >> >+               dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
>> >> > >> >+               return 0;
>> >> > >> >+       }
>> >> > >> >+
>> >> > >> >+       device_init_wakeup(&pdev->dev, true);
>> >> > > Why do you call this before dev_pm_set_dedicated_wake_irq()?
>> >> >
>> >> > hmmm, i thought so too, but it turns out the dedicated wake irq
>> >> > framework requires device_init_wakeup(dev, true) before attach the wake irq:
>> >> >
>> >> > int device_wakeup_attach_irq(struct device *dev,
>> >> >                               struct wake_irq *wakeirq)
>> >> > {
>> >> >          struct wakeup_source *ws;
>> >> >
>> >> >          ws = dev->power.wakeup;
>> >> >          if (!ws) {
>> >> >                  dev_err(dev, "forgot to call device_init_wakeup?\n");
>> >> >                  return -EINVAL;
>> >> >
>> >>
>> >> Well, that's a framework issue, fair enough.
>> >>
>> >> That said, what if user space removes the wakeup source from under you
>> >> concurrently via sysfs?  Tony?
>> >
>> > Hmm sounds racy, need to take a look.
>>
>> Not only racy, as I don't see anything to prevent user space from
>> making the dev->power.wakeup wakeup source go away via sysfs at any
>> time *after* the IRQ has been requested.
>
> Currently nothing happens with wakeirqs if there's no struct
> wakeup_source. On device_wakeup_enable() we call device_wakeup_attach()
> that just copies dev->power.wakeirq to ws->wakeirq. And when struct
> wake_source is freed the device should be active and wakeirq
> disabled. Or are you seeing other issues here?

I'm suspicious about one thing, but I need to look deeper into the code. :-)

>> Pretty much right after dev_pm_set_dedicated_wake_irq() has returned,
>> device_wakeup_disable() may be called on the device via wakeup_store()
>> and it doesn't even check if the device has a wakeup irq.
>>
>> > I think the only reason
>> > to have the wakeirq pointer there was to save memory. It might
>> > make sense to remove the wakeirq dependency here.
>>
>> Well, that looks necessary to be honest.
>
> Seems like we're OK there except for the race. But I still wonder
> if could just get rid of wakeirq in struct wakeup_source. Maybe
> all we need is to see if dev->power.wakeup is allocated for the
> wakeirqs.

I guess "no", but let me get back to you when I have looked at things
in more detail.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

* Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171228 12:21]:
> On Thu, Dec 28, 2017 at 5:22 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171228 00:51]:
> >> On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> >> > * Rafael J. Wysocki <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org> [171227 01:00]:
> >> >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> >> >> > Hi Rafael,
> >> >> >
> >> >> > Thanks for your reply :)
> >> >> >
> >> >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
> >> >> > >> >+
> >> >> > >> >+       dn = pci_device_to_OF_node(ppdev);
> >> >> > >> >+       if (!dn)
> >> >> > >> >+               return 0;
> >> >> > >> >+
> >> >> > >> >+       irq = of_irq_get_byname(dn, "wakeup");
> >> >> > > Why is this a property of the bridge and not of the device itself?
> >> >> >
> >> >> > That is suggested by Brian, because in that way, the wakeup pin would
> >> >> > not "tied to what exact device is installed (or no device, if it's a slot)."
> >> >>
> >> >> But I don't think it works when there are two devices using different WAKE#
> >> >> interrupt lines under the same bridge.  Or how does it work then?
> >> >
> >> > It won't work currently for multiple devices but adding more than
> >> > one wakeriq per device is doable. And I think we will have other
> >> > cases where multiple wakeirqs are connected to a single device, so
> >> > that issue should be sorted out sooner or later.
> >> >
> >> > And if requesting wakeirq for the PCI WAKE# lines at the PCI
> >> > controller does the job, then maybe that's all we need to start with.
> >>
> >> These are expected to be out-of-band, so not having anything to do
> >> with the Root Complex.
> >>
> >> In-band PME Messages go through the PCIe hierarchy, but that is a
> >> standard mechanism and it is supported already.
> >>
> >> WAKE# are platform-specific, pretty much by definition and I guess
> >> that on most ARM boards they are just going to be some kind of GPIO
> >> pins.
> >
> > OK. So probably supporting the following two configurations
> > should be enough then:
> >
> > 1. One or more WAKE# lines configured as a wakeirq for the PCI
> >    controller
> >
> >    When the wakeirq calls pm_wakeup_event() for the PCI controller
> >    device driver, the PCI controller wakes up and can deal with
> >    it's child devices
> 
> But this shouldn't be necessary at all.  Or if it is, I wonder why
> that's the case.

Well Brian had a concern where we would have to implement PM runtime
for all device drivers for PCI devices.

> I'm assuming that we're talking about PCI Express here, which has two
> wakeup mechanisms defined, one of which is based on using PME Messages
> (Beacon) and the second one is WAKE#:
> 
> "The WAKE# mechanism uses sideband signaling to implement wakeup
> functionality. WAKE# is
> an “open drain” signal asserted by components requesting wakeup and
> observed by the associated
> power controller."
> 
> (from PCIe Base Spec 3.0).  [And there's a diagram showing the routing
> of WAKE# in two cases in Figure 5-4: Conceptual Diagrams Showing Two
> Example Cases of WAKE# Routing.]

Thanks for the pointer, I had not seen that :) So the use cases
I was trying to describe above are similar to the wiring in the
PCIe Base Spec 3.0 "Figure 5-4" , but numbered the other way around.

> Note that WAKE# is defined to be "observed by the associated power
> controller", so I'm not sure what the PCI controller's role in the
> handing of it is at all.

To me it seems the "switch" part stays at least partially powered
and then in-band PME messages are used after host is woken up to
figure out which WAKE# triggered?

> > 2. Optionally a WAKE# line from a PCI device configured as wakeirq
> >    for the PCI device driver
> >
> >    In this case calling the PM runtime resume in the child
> >    PCI device will also wake up the parent PCI controller,
> >    and then the PCI controller can deal with it's children
> >
> > Seems like this series is pretty close to 1 above except
> > we need to have a list of wakeirqs per device instead of
> > just one. And option 2 should already work as long as the
> > PCI device driver parses and configures the wakeirq.
> 
> Well, this is confusing, because as I said above, option 1 doesn't
> look relevant even.

So isn't my option 1 above similar to the PCIe spec "Figure 5-4"
case 2? Anyways, let's standardize on the "Figure 5-4" naming
from now to avoid confusion :)

> >> > Then in addition to that, we could do the following to allow
> >> > PCI devices to request the wakeirq from the PCI controller:
> >> >
> >> > 1. PCI controller or framework implements a chained irq for
> >> >    the WAKE# lines assuming it can mask/unmask the WAKE# lines
> >> >
> >> > 2. PCI devices then can just request the wakeirq from the PCI
> >> >    controller
> >> >
> >> > And that's about it. Optionally we could leave out the dependency
> >> > to having PCI devices implement PM runtime and just resume the
> >> > parent (PCI controller) if PCI devices has not implemented
> >> > PM runtime.
> >>
> >> So if my understanding is correct, DT should give you the WAKE# IRQ
> >> for the given endpoint PCI device and you only are expected to request
> >> it.   The rest should just follow from the other pieces of information
> >> in the DT.
> >
> > Yeah and it seems that we should allow configuring both cases
> > 1 and 2 above.
> >
> >> With the quite obvious caveat that the same IRQ may be used as WAKE#
> >> for multiple endpoint devices (which BTW need not be under the same
> >> bridge even).
> >
> > And with the shared interrupts we can't do the masking/unmasking
> > automatically..
> 
> Or we need to use reference counting (so actually the wakeup IRQs are
> not dedicated).

Yeah. FYI, for the dedicated wakeirq cases I have, we need to keep
them masked during runtime to avoid tons of interrupts as they
are often wired to the RX pins.

> >> >> > >> >+       if (irq == -EPROBE_DEFER)
> >> >> > > Braces here, please.
> >> >> > ok, will fix in the next version.
> >> >> >
> >> >> > >
> >> >> > >> >+               return irq;
> >> >> > >> >+       /* Ignore other errors, since a missing wakeup is non-fatal. */
> >> >> > >> >+       else if (irq < 0) {
> >> >> > >> >+               dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
> >> >> > >> >+               return 0;
> >> >> > >> >+       }
> >> >> > >> >+
> >> >> > >> >+       device_init_wakeup(&pdev->dev, true);
> >> >> > > Why do you call this before dev_pm_set_dedicated_wake_irq()?
> >> >> >
> >> >> > hmmm, i thought so too, but it turns out the dedicated wake irq
> >> >> > framework requires device_init_wakeup(dev, true) before attach the wake irq:
> >> >> >
> >> >> > int device_wakeup_attach_irq(struct device *dev,
> >> >> >                               struct wake_irq *wakeirq)
> >> >> > {
> >> >> >          struct wakeup_source *ws;
> >> >> >
> >> >> >          ws = dev->power.wakeup;
> >> >> >          if (!ws) {
> >> >> >                  dev_err(dev, "forgot to call device_init_wakeup?\n");
> >> >> >                  return -EINVAL;
> >> >> >
> >> >>
> >> >> Well, that's a framework issue, fair enough.
> >> >>
> >> >> That said, what if user space removes the wakeup source from under you
> >> >> concurrently via sysfs?  Tony?
> >> >
> >> > Hmm sounds racy, need to take a look.
> >>
> >> Not only racy, as I don't see anything to prevent user space from
> >> making the dev->power.wakeup wakeup source go away via sysfs at any
> >> time *after* the IRQ has been requested.
> >
> > Currently nothing happens with wakeirqs if there's no struct
> > wakeup_source. On device_wakeup_enable() we call device_wakeup_attach()
> > that just copies dev->power.wakeirq to ws->wakeirq. And when struct
> > wake_source is freed the device should be active and wakeirq
> > disabled. Or are you seeing other issues here?
> 
> I'm suspicious about one thing, but I need to look deeper into the code. :-)

OK. My response time will be laggy this week in case you find
something that needs urgent fixing :)

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:
> * Rafael J. Wysocki <rafael@kernel.org> [171228 12:21]:
> > On Thu, Dec 28, 2017 at 5:22 AM, Tony Lindgren <tony@atomide.com> wrote:
> > > * Rafael J. Wysocki <rafael@kernel.org> [171228 00:51]:
> > >> On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony@atomide.com> wrote:
> > >> > * Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]:
> > >> >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> > >> >> > Hi Rafael,
> > >> >> >
> > >> >> > Thanks for your reply :)
> > >> >> >
> > >> >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
> > >> >> > >> >+
> > >> >> > >> >+       dn = pci_device_to_OF_node(ppdev);
> > >> >> > >> >+       if (!dn)
> > >> >> > >> >+               return 0;
> > >> >> > >> >+
> > >> >> > >> >+       irq = of_irq_get_byname(dn, "wakeup");
> > >> >> > > Why is this a property of the bridge and not of the device itself?
> > >> >> >
> > >> >> > That is suggested by Brian, because in that way, the wakeup pin would
> > >> >> > not "tied to what exact device is installed (or no device, if it's a slot)."
> > >> >>
> > >> >> But I don't think it works when there are two devices using different WAKE#
> > >> >> interrupt lines under the same bridge.  Or how does it work then?
> > >> >
> > >> > It won't work currently for multiple devices but adding more than
> > >> > one wakeriq per device is doable. And I think we will have other
> > >> > cases where multiple wakeirqs are connected to a single device, so
> > >> > that issue should be sorted out sooner or later.
> > >> >
> > >> > And if requesting wakeirq for the PCI WAKE# lines at the PCI
> > >> > controller does the job, then maybe that's all we need to start with.
> > >>
> > >> These are expected to be out-of-band, so not having anything to do
> > >> with the Root Complex.
> > >>
> > >> In-band PME Messages go through the PCIe hierarchy, but that is a
> > >> standard mechanism and it is supported already.
> > >>
> > >> WAKE# are platform-specific, pretty much by definition and I guess
> > >> that on most ARM boards they are just going to be some kind of GPIO
> > >> pins.
> > >
> > > OK. So probably supporting the following two configurations
> > > should be enough then:
> > >
> > > 1. One or more WAKE# lines configured as a wakeirq for the PCI
> > >    controller
> > >
> > >    When the wakeirq calls pm_wakeup_event() for the PCI controller
> > >    device driver, the PCI controller wakes up and can deal with
> > >    it's child devices
> > 
> > But this shouldn't be necessary at all.  Or if it is, I wonder why
> > that's the case.
> 
> Well Brian had a concern where we would have to implement PM runtime
> for all device drivers for PCI devices.

Why would we?

> > I'm assuming that we're talking about PCI Express here, which has two
> > wakeup mechanisms defined, one of which is based on using PME Messages
> > (Beacon) and the second one is WAKE#:
> > 
> > "The WAKE# mechanism uses sideband signaling to implement wakeup
> > functionality. WAKE# is
> > an “open drain” signal asserted by components requesting wakeup and
> > observed by the associated
> > power controller."
> > 
> > (from PCIe Base Spec 3.0).  [And there's a diagram showing the routing
> > of WAKE# in two cases in Figure 5-4: Conceptual Diagrams Showing Two
> > Example Cases of WAKE# Routing.]
> 
> Thanks for the pointer, I had not seen that :) So the use cases
> I was trying to describe above are similar to the wiring in the
> PCIe Base Spec 3.0 "Figure 5-4" , but numbered the other way around.
> 
> > Note that WAKE# is defined to be "observed by the associated power
> > controller", so I'm not sure what the PCI controller's role in the
> > handing of it is at all.
> 
> To me it seems the "switch" part stays at least partially powered
> and then in-band PME messages are used after host is woken up to
> figure out which WAKE# triggered?

Yes, that's my understanding too.

> > > 2. Optionally a WAKE# line from a PCI device configured as wakeirq
> > >    for the PCI device driver
> > >
> > >    In this case calling the PM runtime resume in the child
> > >    PCI device will also wake up the parent PCI controller,
> > >    and then the PCI controller can deal with it's children
> > >
> > > Seems like this series is pretty close to 1 above except
> > > we need to have a list of wakeirqs per device instead of
> > > just one. And option 2 should already work as long as the
> > > PCI device driver parses and configures the wakeirq.
> > 
> > Well, this is confusing, because as I said above, option 1 doesn't
> > look relevant even.
> 
> So isn't my option 1 above similar to the PCIe spec "Figure 5-4"
> case 2?

No, it isn't, because in that case there is no practical difference
between WAKE# and an in-band PME message sent by the device (Beacon)
from the software perspective.

WAKE# causes the switch to send a PME message upstream and that is
handled by the Root Complex through the standard mechanism already
supported by our existing PME driver (drivers/pci/pcie/pme.c).

> Anyways, let's standardize on the "Figure 5-4" naming
> from now to avoid confusion :)

OK

> > >> > Then in addition to that, we could do the following to allow
> > >> > PCI devices to request the wakeirq from the PCI controller:
> > >> >
> > >> > 1. PCI controller or framework implements a chained irq for
> > >> >    the WAKE# lines assuming it can mask/unmask the WAKE# lines
> > >> >
> > >> > 2. PCI devices then can just request the wakeirq from the PCI
> > >> >    controller
> > >> >
> > >> > And that's about it. Optionally we could leave out the dependency
> > >> > to having PCI devices implement PM runtime and just resume the
> > >> > parent (PCI controller) if PCI devices has not implemented
> > >> > PM runtime.
> > >>
> > >> So if my understanding is correct, DT should give you the WAKE# IRQ
> > >> for the given endpoint PCI device and you only are expected to request
> > >> it.   The rest should just follow from the other pieces of information
> > >> in the DT.
> > >
> > > Yeah and it seems that we should allow configuring both cases
> > > 1 and 2 above.
> > >
> > >> With the quite obvious caveat that the same IRQ may be used as WAKE#
> > >> for multiple endpoint devices (which BTW need not be under the same
> > >> bridge even).
> > >
> > > And with the shared interrupts we can't do the masking/unmasking
> > > automatically..
> > 
> > Or we need to use reference counting (so actually the wakeup IRQs are
> > not dedicated).
> 
> Yeah. FYI, for the dedicated wakeirq cases I have, we need to keep
> them masked during runtime to avoid tons of interrupts as they
> are often wired to the RX pins.

OK

BTW, enable_irq_wake() should take care of the sharing, shouldn't it?

But the WAKE# thing is not just for waking up the system from sleep states,
it is for runtime PM's wakeup signaling too.

> > >> >> > >> >+       if (irq == -EPROBE_DEFER)
> > >> >> > > Braces here, please.
> > >> >> > ok, will fix in the next version.
> > >> >> >
> > >> >> > >
> > >> >> > >> >+               return irq;
> > >> >> > >> >+       /* Ignore other errors, since a missing wakeup is non-fatal. */
> > >> >> > >> >+       else if (irq < 0) {
> > >> >> > >> >+               dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
> > >> >> > >> >+               return 0;
> > >> >> > >> >+       }
> > >> >> > >> >+
> > >> >> > >> >+       device_init_wakeup(&pdev->dev, true);
> > >> >> > > Why do you call this before dev_pm_set_dedicated_wake_irq()?
> > >> >> >
> > >> >> > hmmm, i thought so too, but it turns out the dedicated wake irq
> > >> >> > framework requires device_init_wakeup(dev, true) before attach the wake irq:
> > >> >> >
> > >> >> > int device_wakeup_attach_irq(struct device *dev,
> > >> >> >                               struct wake_irq *wakeirq)
> > >> >> > {
> > >> >> >          struct wakeup_source *ws;
> > >> >> >
> > >> >> >          ws = dev->power.wakeup;
> > >> >> >          if (!ws) {
> > >> >> >                  dev_err(dev, "forgot to call device_init_wakeup?\n");
> > >> >> >                  return -EINVAL;
> > >> >> >
> > >> >>
> > >> >> Well, that's a framework issue, fair enough.
> > >> >>
> > >> >> That said, what if user space removes the wakeup source from under you
> > >> >> concurrently via sysfs?  Tony?
> > >> >
> > >> > Hmm sounds racy, need to take a look.
> > >>
> > >> Not only racy, as I don't see anything to prevent user space from
> > >> making the dev->power.wakeup wakeup source go away via sysfs at any
> > >> time *after* the IRQ has been requested.
> > >
> > > Currently nothing happens with wakeirqs if there's no struct
> > > wakeup_source. On device_wakeup_enable() we call device_wakeup_attach()
> > > that just copies dev->power.wakeirq to ws->wakeirq. And when struct
> > > wake_source is freed the device should be active and wakeirq
> > > disabled. Or are you seeing other issues here?
> > 
> > I'm suspicious about one thing, but I need to look deeper into the code. :-)

So we are fine except for the race and we need the wakeirq field in wakeup
sources to automatically arm the wakeup IRQs during suspend.

If I'm not mistaken, we only need something like the patch below (untested).

> OK. My response time will be laggy this week in case you find
> something that needs urgent fixing :)

No worries. :-)

---
 drivers/base/power/wakeirq.c |    9 ++++-----
 drivers/base/power/wakeup.c  |    2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/base/power/wakeirq.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeirq.c
+++ linux-pm/drivers/base/power/wakeirq.c
@@ -33,7 +33,6 @@ static int dev_pm_attach_wake_irq(struct
 				  struct wake_irq *wirq)
 {
 	unsigned long flags;
-	int err;
 
 	if (!dev || !wirq)
 		return -EINVAL;
@@ -45,12 +44,12 @@ static int dev_pm_attach_wake_irq(struct
 		return -EEXIST;
 	}
 
-	err = device_wakeup_attach_irq(dev, wirq);
-	if (!err)
-		dev->power.wakeirq = wirq;
+	dev->power.wakeirq = wirq;
+	if (dev->power.wakeup)
+		device_wakeup_attach_irq(dev, wirq);
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
-	return err;
+	return 0;
 }
 
 /**
Index: linux-pm/drivers/base/power/wakeup.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeup.c
+++ linux-pm/drivers/base/power/wakeup.c
@@ -303,7 +303,7 @@ int device_wakeup_attach_irq(struct devi
 	}
 
 	if (ws->wakeirq)
-		return -EEXIST;
+		dev_err(dev, "Leftover wakeup IRQ found, overriding\n");
 
 	ws->wakeirq = wakeirq;
 	return 0;

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Thu, Dec 28, 2017 at 6:29 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:
>> * Rafael J. Wysocki <rafael@kernel.org> [171228 12:21]:
>> > On Thu, Dec 28, 2017 at 5:22 AM, Tony Lindgren <tony@atomide.com> wrote:
>> > > * Rafael J. Wysocki <rafael@kernel.org> [171228 00:51]:
>> > >> On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > >> > * Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]:
>> > >> >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
>> > >> >> > Hi Rafael,
>> > >> >> >
>> > >> >> > Thanks for your reply :)
>> > >> >> >
>> > >> >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
>> > >> >> > >> >+
>> > >> >> > >> >+       dn = pci_device_to_OF_node(ppdev);
>> > >> >> > >> >+       if (!dn)
>> > >> >> > >> >+               return 0;
>> > >> >> > >> >+
>> > >> >> > >> >+       irq = of_irq_get_byname(dn, "wakeup");
>> > >> >> > > Why is this a property of the bridge and not of the device itself?
>> > >> >> >
>> > >> >> > That is suggested by Brian, because in that way, the wakeup pin would
>> > >> >> > not "tied to what exact device is installed (or no device, if it's a slot)."
>> > >> >>
>> > >> >> But I don't think it works when there are two devices using different WAKE#
>> > >> >> interrupt lines under the same bridge.  Or how does it work then?
>> > >> >
>> > >> > It won't work currently for multiple devices but adding more than
>> > >> > one wakeriq per device is doable. And I think we will have other
>> > >> > cases where multiple wakeirqs are connected to a single device, so
>> > >> > that issue should be sorted out sooner or later.
>> > >> >
>> > >> > And if requesting wakeirq for the PCI WAKE# lines at the PCI
>> > >> > controller does the job, then maybe that's all we need to start with.
>> > >>
>> > >> These are expected to be out-of-band, so not having anything to do
>> > >> with the Root Complex.
>> > >>
>> > >> In-band PME Messages go through the PCIe hierarchy, but that is a
>> > >> standard mechanism and it is supported already.
>> > >>
>> > >> WAKE# are platform-specific, pretty much by definition and I guess
>> > >> that on most ARM boards they are just going to be some kind of GPIO
>> > >> pins.
>> > >
>> > > OK. So probably supporting the following two configurations
>> > > should be enough then:
>> > >
>> > > 1. One or more WAKE# lines configured as a wakeirq for the PCI
>> > >    controller
>> > >
>> > >    When the wakeirq calls pm_wakeup_event() for the PCI controller
>> > >    device driver, the PCI controller wakes up and can deal with
>> > >    it's child devices
>> >
>> > But this shouldn't be necessary at all.  Or if it is, I wonder why
>> > that's the case.
>>
>> Well Brian had a concern where we would have to implement PM runtime
>> for all device drivers for PCI devices.
>
> Why would we?
>
>> > I'm assuming that we're talking about PCI Express here, which has two
>> > wakeup mechanisms defined, one of which is based on using PME Messages
>> > (Beacon) and the second one is WAKE#:
>> >
>> > "The WAKE# mechanism uses sideband signaling to implement wakeup
>> > functionality. WAKE# is
>> > an “open drain” signal asserted by components requesting wakeup and
>> > observed by the associated
>> > power controller."
>> >
>> > (from PCIe Base Spec 3.0).  [And there's a diagram showing the routing
>> > of WAKE# in two cases in Figure 5-4: Conceptual Diagrams Showing Two
>> > Example Cases of WAKE# Routing.]
>>
>> Thanks for the pointer, I had not seen that :) So the use cases
>> I was trying to describe above are similar to the wiring in the
>> PCIe Base Spec 3.0 "Figure 5-4" , but numbered the other way around.
>>
>> > Note that WAKE# is defined to be "observed by the associated power
>> > controller", so I'm not sure what the PCI controller's role in the
>> > handing of it is at all.
>>
>> To me it seems the "switch" part stays at least partially powered
>> and then in-band PME messages are used after host is woken up to
>> figure out which WAKE# triggered?
>
> Yes, that's my understanding too.

To be precise, it is not quite possible to figure out which WAKE#
triggered, if they are sharing the line, without looking into the
config spaces of the devices below the switch.  The switch is not
expected to do that AFAICS.  It only generates a PME message meaning
"wakeup is being signaled somewhere below" and the PME driver that
handles the Root Port receiving it should look at the PME Status bits
of the devices below the switch (the pme.c driver does that IIRC or at
least it should do that ;-)).

Still, the handling of WAKE# doesn't need to cover this case AFAICS.

Thanks,
Rafael

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

* Rafael J. Wysocki <rjw@rjwysocki.net> [171228 17:33]:
> On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:
> > 
> > Well Brian had a concern where we would have to implement PM runtime
> > for all device drivers for PCI devices.
> 
> Why would we?

Seems at least I was a bit confused. In the PCIe case the WAKE# is
owned by the PCIe slot, not the child PCIe device. So you're right,
there should be no need for the child PCIe device drivers to
implement runtime PM.

I was thinking the wakeirq case with WLAN on SDIO bus. Some WLAN
devices can have a hardwired OOB wakeirq wired to a GPIO controller.
In that case the wakeirq is owned by the child device driver
(WLAN controller) and not by the SDIO slot. I was earlier
thinking this is the same as the "Figure 5-4" case 1, but it's
not.

So in the PCIe WAKE# case for device tree, we must have the
wakeirq property for the PCIe slot for the struct device managing
that slot, and not for the child device driver. I think it's
already this way in the most recent set of patches, I need to
look again.

> > So isn't my option 1 above similar to the PCIe spec "Figure 5-4"
> > case 2?
> 
> No, it isn't, because in that case there is no practical difference
> between WAKE# and an in-band PME message sent by the device (Beacon)
> from the software perspective.
> 
> WAKE# causes the switch to send a PME message upstream and that is
> handled by the Root Complex through the standard mechanism already
> supported by our existing PME driver (drivers/pci/pcie/pme.c).

OK. So if "Figure 5-4" case 2 is already handled then and we need
to just deal with "Figure 5-4" case 1 :)

> > Yeah. FYI, for the dedicated wakeirq cases I have, we need to keep
> > them masked during runtime to avoid tons of interrupts as they
> > are often wired to the RX pins.
> 
> OK
> 
> BTW, enable_irq_wake() should take care of the sharing, shouldn't it?

That can be used to tell us which device has wakeirq enabled for
wake-up events, but only for resume not runtiem PM. We still have the
shared IRQ problem to deal with. And the PCIe subsystem still needs
to go through the child devices.

> But the WAKE# thing is not just for waking up the system from sleep states,
> it is for runtime PM's wakeup signaling too.

Yes my test cases have it working for runtime PM and for waking
up system from suspend.

> > > > Currently nothing happens with wakeirqs if there's no struct
> > > > wakeup_source. On device_wakeup_enable() we call device_wakeup_attach()
> > > > that just copies dev->power.wakeirq to ws->wakeirq. And when struct
> > > > wake_source is freed the device should be active and wakeirq
> > > > disabled. Or are you seeing other issues here?
> > > 
> > > I'm suspicious about one thing, but I need to look deeper into the code. :-)
> 
> So we are fine except for the race and we need the wakeirq field in wakeup
> sources to automatically arm the wakeup IRQs during suspend.

OK.

> If I'm not mistaken, we only need something like the patch below (untested).

Seems like it should fix the race, I'll do some testing next week.

Regards,

Tony

> ---
>  drivers/base/power/wakeirq.c |    9 ++++-----
>  drivers/base/power/wakeup.c  |    2 +-
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/base/power/wakeirq.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/wakeirq.c
> +++ linux-pm/drivers/base/power/wakeirq.c
> @@ -33,7 +33,6 @@ static int dev_pm_attach_wake_irq(struct
>  				  struct wake_irq *wirq)
>  {
>  	unsigned long flags;
> -	int err;
>  
>  	if (!dev || !wirq)
>  		return -EINVAL;
> @@ -45,12 +44,12 @@ static int dev_pm_attach_wake_irq(struct
>  		return -EEXIST;
>  	}
>  
> -	err = device_wakeup_attach_irq(dev, wirq);
> -	if (!err)
> -		dev->power.wakeirq = wirq;
> +	dev->power.wakeirq = wirq;
> +	if (dev->power.wakeup)
> +		device_wakeup_attach_irq(dev, wirq);
>  
>  	spin_unlock_irqrestore(&dev->power.lock, flags);
> -	return err;
> +	return 0;
>  }
>  
>  /**
> Index: linux-pm/drivers/base/power/wakeup.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/wakeup.c
> +++ linux-pm/drivers/base/power/wakeup.c
> @@ -303,7 +303,7 @@ int device_wakeup_attach_irq(struct devi
>  	}
>  
>  	if (ws->wakeirq)
> -		return -EEXIST;
> +		dev_err(dev, "Leftover wakeup IRQ found, overriding\n");
>  
>  	ws->wakeirq = wakeirq;
>  	return 0;
> 

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

* Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171228 17:46]:
> 
> To be precise, it is not quite possible to figure out which WAKE#
> triggered, if they are sharing the line, without looking into the
> config spaces of the devices below the switch.  The switch is not
> expected to do that AFAICS.  It only generates a PME message meaning
> "wakeup is being signaled somewhere below" and the PME driver that
> handles the Root Port receiving it should look at the PME Status bits
> of the devices below the switch (the pme.c driver does that IIRC or at
> least it should do that ;-)).
> 
> Still, the handling of WAKE# doesn't need to cover this case AFAICS.

OK makes sense now.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Fri, Dec 29, 2017 at 6:15 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Rafael J. Wysocki <rjw@rjwysocki.net> [171228 17:33]:
>> On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:
>> >
>> > Well Brian had a concern where we would have to implement PM runtime
>> > for all device drivers for PCI devices.
>>
>> Why would we?
>
> Seems at least I was a bit confused. In the PCIe case the WAKE# is
> owned by the PCIe slot, not the child PCIe device.

Well, it depends on what you mean by "slot" and "child device", but if
my understanding of it is correct, WAKE# actually is "owned" by the
latter.

First, let me clarify the terminology.  PCI slots are not really
represented in the device hierarchy in Linux.  They are represented by
kernel objects for hotplug purposes, but these objects are not based
on struct device.

Generally, there are two kinds of PCI entities represented by struct
pci_dev, bridges and endpoints (functions).  Bridges may represent
physical devices, like PCI-to-PCI bridges, or parts of physical
devices, like PCIe ports.  In PCIe, every port is logically
represented by a bridge (and a PCI config space region with a Type 1
header).  However, ports do not take actions like generating
interrupts; the pieces of hardware containing them (switches, Root
Complex) do that.

Endpoints (functions) are children of bridges (e.g. PCIe ports) and
bridges may be children of other bridges (like in a switch that is
represented by a bus segment with one upstream bridge - the upstream
port - and possibly multiple downstream bridges - downstream ports).
So in PCI a parent always is a bridge (either a PCI bridge - a bridge
between to PCI bus segments - or a host bridge) and if that is a PCIe
port, it cannot "own" anything like WAKE#, because it is not affected
by it in any way and doesn't take part in the handling of it.

In the context of "Figure 5-4" in the spec, Case 1, what matters is
that every "Slot" in the figure represents a bunch (up to 8) of
endpoints (functions), but the "Slot" is not the parent of them.  The
port of the switch the "Slot" is connected to is the parent.  WAKE#
basically comes from one of the endpoints belonging to the "Slot" and
you need to look into the config space regions for all of these
endpoints to check which one has PME Status set and clear it (to
acknowledge the PME and make the hardware stop asserting the WAKE#
signal).  So, from the software perspective, the endpoint (child) is
the source of WAKE# and that should be reflected by DT properties IMO.

> So you're right, there should be no need for the child PCIe device drivers to
> implement runtime PM.

There should be no need for that regardless.  You only need an
interrupt handler that will look for the endpoint with PME Status set,
acknowledge it and possibly invoke runtime PM for the endpoint in
question (if supported).  All of that is standard and can happen at
the bus type level and the interrupt handler I'm talking about may be
based on pci_pme_wakeup() or pci_acpi_wake_dev().

> I was thinking the wakeirq case with WLAN on SDIO bus. Some WLAN
> devices can have a hardwired OOB wakeirq wired to a GPIO controller.
> In that case the wakeirq is owned by the child device driver
> (WLAN controller) and not by the SDIO slot. I was earlier
> thinking this is the same as the "Figure 5-4" case 1, but it's
> not.

Well, it is not in the sense that the endpoint driver is not expected
to handle the wakeup interrupt by itself.  The PCI bus type is
responsible for that, but technically WAKE# comes from the endpoint
(child).

> So in the PCIe WAKE# case for device tree, we must have the
> wakeirq property for the PCIe slot for the struct device managing
> that slot,

Which doesn't exist.

> and not for the child device driver. I think it's
> already this way in the most recent set of patches, I need to
> look again.

No, you need a wakeirq properly for the child *device* and that
property will be consumed by the PCI layer.

>> > So isn't my option 1 above similar to the PCIe spec "Figure 5-4"
>> > case 2?
>>
>> No, it isn't, because in that case there is no practical difference
>> between WAKE# and an in-band PME message sent by the device (Beacon)
>> from the software perspective.
>>
>> WAKE# causes the switch to send a PME message upstream and that is
>> handled by the Root Complex through the standard mechanism already
>> supported by our existing PME driver (drivers/pci/pcie/pme.c).
>
> OK. So if "Figure 5-4" case 2 is already handled then and we need
> to just deal with "Figure 5-4" case 1 :)

Right.

>> > Yeah. FYI, for the dedicated wakeirq cases I have, we need to keep
>> > them masked during runtime to avoid tons of interrupts as they
>> > are often wired to the RX pins.
>>
>> OK
>>
>> BTW, enable_irq_wake() should take care of the sharing, shouldn't it?
>
> That can be used to tell us which device has wakeirq enabled for
> wake-up events, but only for resume not runtiem PM. We still have the
> shared IRQ problem to deal with. And the PCIe subsystem still needs
> to go through the child devices.

Right.

It actually has to walk the bus below each of them too in case it is a
bridge, like pci_acpi_wake_dev().

>> But the WAKE# thing is not just for waking up the system from sleep states,
>> it is for runtime PM's wakeup signaling too.
>
> Yes my test cases have it working for runtime PM and for waking
> up system from suspend.

OK

>> > > > Currently nothing happens with wakeirqs if there's no struct
>> > > > wakeup_source. On device_wakeup_enable() we call device_wakeup_attach()
>> > > > that just copies dev->power.wakeirq to ws->wakeirq. And when struct
>> > > > wake_source is freed the device should be active and wakeirq
>> > > > disabled. Or are you seeing other issues here?
>> > >
>> > > I'm suspicious about one thing, but I need to look deeper into the code. :-)
>>
>> So we are fine except for the race and we need the wakeirq field in wakeup
>> sources to automatically arm the wakeup IRQs during suspend.
>
> OK.
>
>> If I'm not mistaken, we only need something like the patch below (untested).
>
> Seems like it should fix the race, I'll do some testing next week.

OK, thanks!

Rafael

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Sat, Dec 30, 2017 at 12:39 AM, Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Dec 29, 2017 at 6:15 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> * Rafael J. Wysocki <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org> [171228 17:33]:
>>> On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:
>>> >
>>> > Well Brian had a concern where we would have to implement PM runtime
>>> > for all device drivers for PCI devices.
>>>
>>> Why would we?
>>
>> Seems at least I was a bit confused. In the PCIe case the WAKE# is
>> owned by the PCIe slot, not the child PCIe device.
>
> Well, it depends on what you mean by "slot" and "child device", but if
> my understanding of it is correct, WAKE# actually is "owned" by the
> latter.
>
> First, let me clarify the terminology.  PCI slots are not really
> represented in the device hierarchy in Linux.  They are represented by
> kernel objects for hotplug purposes, but these objects are not based
> on struct device.
>
> Generally, there are two kinds of PCI entities represented by struct
> pci_dev, bridges and endpoints (functions).  Bridges may represent
> physical devices, like PCI-to-PCI bridges, or parts of physical
> devices, like PCIe ports.  In PCIe, every port is logically
> represented by a bridge (and a PCI config space region with a Type 1
> header).  However, ports do not take actions like generating
> interrupts; the pieces of hardware containing them (switches, Root
> Complex) do that.
>
> Endpoints (functions) are children of bridges (e.g. PCIe ports) and
> bridges may be children of other bridges (like in a switch that is
> represented by a bus segment with one upstream bridge - the upstream
> port - and possibly multiple downstream bridges - downstream ports).
> So in PCI a parent always is a bridge (either a PCI bridge - a bridge
> between to PCI bus segments - or a host bridge) and if that is a PCIe
> port, it cannot "own" anything like WAKE#, because it is not affected
> by it in any way and doesn't take part in the handling of it.
>
> In the context of "Figure 5-4" in the spec, Case 1, what matters is
> that every "Slot" in the figure represents a bunch (up to 8) of
> endpoints (functions), but the "Slot" is not the parent of them.  The
> port of the switch the "Slot" is connected to is the parent.  WAKE#
> basically comes from one of the endpoints belonging to the "Slot" and
> you need to look into the config space regions for all of these
> endpoints to check which one has PME Status set and clear it (to
> acknowledge the PME and make the hardware stop asserting the WAKE#
> signal).  So, from the software perspective, the endpoint (child) is
> the source of WAKE# and that should be reflected by DT properties IMO.
>
>> So you're right, there should be no need for the child PCIe device drivers to
>> implement runtime PM.
>
> There should be no need for that regardless.  You only need an
> interrupt handler that will look for the endpoint with PME Status set,
> acknowledge it and possibly invoke runtime PM for the endpoint in
> question (if supported).  All of that is standard and can happen at
> the bus type level and the interrupt handler I'm talking about may be
> based on pci_pme_wakeup() or pci_acpi_wake_dev().
>
>> I was thinking the wakeirq case with WLAN on SDIO bus. Some WLAN
>> devices can have a hardwired OOB wakeirq wired to a GPIO controller.
>> In that case the wakeirq is owned by the child device driver
>> (WLAN controller) and not by the SDIO slot. I was earlier
>> thinking this is the same as the "Figure 5-4" case 1, but it's
>> not.
>
> Well, it is not in the sense that the endpoint driver is not expected
> to handle the wakeup interrupt by itself.  The PCI bus type is
> responsible for that, but technically WAKE# comes from the endpoint
> (child).
>
>> So in the PCIe WAKE# case for device tree, we must have the
>> wakeirq property for the PCIe slot for the struct device managing
>> that slot,
>
> Which doesn't exist.
>
>> and not for the child device driver. I think it's
>> already this way in the most recent set of patches, I need to
>> look again.
>
> No, you need a wakeirq properly for the child *device* and that
> property will be consumed by the PCI layer.

Or, if you use the convention mentioned in another message in this
thread, you can make the wakeirq be a property of a bridge (port) with
the clarification of the assumption that WAKE# is shared by all
functions below the bridge.  So the presence of the "wakeirq" property
for a bridge (in addition to providing the wakeup IRQ) will mean that
it applies to all devices below the bridge.

In the case of parallel PCI (not PCIe), there may be multiple "slots"
(or "PCI devices" consisting each of multiple functions) under one
bridge and in theory each of them may use a different IRQ for WAKE#
signaling, so the above convention will not work then.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

* Rafael J. Wysocki <rafael@kernel.org> [171230 00:24]:
> On Sat, Dec 30, 2017 at 12:39 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > No, you need a wakeirq properly for the child *device* and that
> > property will be consumed by the PCI layer.
> 
> Or, if you use the convention mentioned in another message in this
> thread, you can make the wakeirq be a property of a bridge (port) with
> the clarification of the assumption that WAKE# is shared by all
> functions below the bridge.  So the presence of the "wakeirq" property
> for a bridge (in addition to providing the wakeup IRQ) will mean that
> it applies to all devices below the bridge.

Yes that makes sense from device tree point of view.

> In the case of parallel PCI (not PCIe), there may be multiple "slots"
> (or "PCI devices" consisting each of multiple functions) under one
> bridge and in theory each of them may use a different IRQ for WAKE#
> signaling, so the above convention will not work then.

OK. In that case the wakeirq would have to be mapped to each
PCI device if there is no other way to describe where the slot
WAKE# line is wired to. I guess the PCI controller could go
through the child devices and map the wakeirqs that way if
needed.

Regards,

Tony

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

Hi,

* Rafael J. Wysocki <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org> [171228 17:33]:
> So we are fine except for the race and we need the wakeirq field in wakeup
> sources to automatically arm the wakeup IRQs during suspend.
> 
> If I'm not mistaken, we only need something like the patch below (untested).

Yeah for your patch below works just fine for my
test cases so:

Tested-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

> ---
>  drivers/base/power/wakeirq.c |    9 ++++-----
>  drivers/base/power/wakeup.c  |    2 +-
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/base/power/wakeirq.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/wakeirq.c
> +++ linux-pm/drivers/base/power/wakeirq.c
> @@ -33,7 +33,6 @@ static int dev_pm_attach_wake_irq(struct
>  				  struct wake_irq *wirq)
>  {
>  	unsigned long flags;
> -	int err;
>  
>  	if (!dev || !wirq)
>  		return -EINVAL;
> @@ -45,12 +44,12 @@ static int dev_pm_attach_wake_irq(struct
>  		return -EEXIST;
>  	}
>  
> -	err = device_wakeup_attach_irq(dev, wirq);
> -	if (!err)
> -		dev->power.wakeirq = wirq;
> +	dev->power.wakeirq = wirq;
> +	if (dev->power.wakeup)
> +		device_wakeup_attach_irq(dev, wirq);
>  
>  	spin_unlock_irqrestore(&dev->power.lock, flags);
> -	return err;
> +	return 0;
>  }
>  
>  /**
> Index: linux-pm/drivers/base/power/wakeup.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/wakeup.c
> +++ linux-pm/drivers/base/power/wakeup.c
> @@ -303,7 +303,7 @@ int device_wakeup_attach_irq(struct devi
>  	}
>  
>  	if (ws->wakeirq)
> -		return -EEXIST;
> +		dev_err(dev, "Leftover wakeup IRQ found, overriding\n");
>  
>  	ws->wakeirq = wakeirq;
>  	return 0;
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Wednesday, January 3, 2018 9:08:37 PM CET Tony Lindgren wrote:
> Hi,
> 
> * Rafael J. Wysocki <rjw@rjwysocki.net> [171228 17:33]:
> > So we are fine except for the race and we need the wakeirq field in wakeup
> > sources to automatically arm the wakeup IRQs during suspend.
> > 
> > If I'm not mistaken, we only need something like the patch below (untested).
> 
> Yeah for your patch below works just fine for my
> test cases so:
> 
> Tested-by: Tony Lindgren <tony@atomide.com>

Cool, thanks!

Let me respin it with a subject/changelog.

Thanks,
Rafael

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Brian Norris]

Hi,

Trying to catch up on this thread...

On Wed, Dec 27, 2017 at 01:57:07AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> > Hi Rafael,
> > 
> > Thanks for your reply :)
> > 
> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
> > >> >+
> > >> >+	dn = pci_device_to_OF_node(ppdev);
> > >> >+	if (!dn)
> > >> >+		return 0;
> > >> >+
> > >> >+	irq = of_irq_get_byname(dn, "wakeup");
> > > Why is this a property of the bridge and not of the device itself?

Wait, isn't 'dn' the port node, not the bridge node?

> > That is suggested by Brian, because in that way, the wakeup pin would 
> > not "tied to what exact device is installed (or no device, if it's a slot)."

I believe my thinking has evolved a bit over time, and I definitely am
not the one true authority on this. I'll explain my main concerns, and
whatever solution resolves these concerns is fine with me.

* I was primarily interested in avoiding handling WAKE# in the endpoint
  drivers (e.g., as mwifiex is today).
* I was also interested in not having to redefine a new DT device
  node (with new "pciABCD,1234" compatible property) for each new device
  attached. That just won't work for removable cards.

I need to reread the rest of this thread a few times to really
understand what Rafael and Tony are discussing. But I feel like some of
this is still moving away from the second point above.

> But I don't think it works when there are two devices using different WAKE#
> interrupt lines under the same bridge.  Or how does it work then?

Brian

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Friday, January 5, 2018 1:41:31 AM CET Brian Norris wrote:
> Hi,
> 
> Trying to catch up on this thread...
> 
> On Wed, Dec 27, 2017 at 01:57:07AM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> > > Hi Rafael,
> > > 
> > > Thanks for your reply :)
> > > 
> > > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
> > > >> >+
> > > >> >+	dn = pci_device_to_OF_node(ppdev);
> > > >> >+	if (!dn)
> > > >> >+		return 0;
> > > >> >+
> > > >> >+	irq = of_irq_get_byname(dn, "wakeup");
> > > > Why is this a property of the bridge and not of the device itself?
> 
> Wait, isn't 'dn' the port node, not the bridge node?

I may be confused about the structure you have in DT.

In the device hierarchy PCIe ports are represented as bridges.

> > > That is suggested by Brian, because in that way, the wakeup pin would 
> > > not "tied to what exact device is installed (or no device, if it's a slot)."
> 
> I believe my thinking has evolved a bit over time, and I definitely am
> not the one true authority on this. I'll explain my main concerns, and
> whatever solution resolves these concerns is fine with me.
> 
> * I was primarily interested in avoiding handling WAKE# in the endpoint
>   drivers (e.g., as mwifiex is today).

OK, everybody on this thread is interested in that. :-)

> * I was also interested in not having to redefine a new DT device
>   node (with new "pciABCD,1234" compatible property) for each new device
>   attached. That just won't work for removable cards.

So if you make it the property of a bridge, it should be fine (as long
as the bridge itself is not removable).

> I need to reread the rest of this thread a few times to really
> understand what Rafael and Tony are discussing. But I feel like some of
> this is still moving away from the second point above.
> 
> > But I don't think it works when there are two devices using different WAKE#
> > interrupt lines under the same bridge.  Or how does it work then?

We seem to have agreed that this case needs to be neglected here.

The "wakeup-interrupt" property at the bridge level basically has to be defined
as the wakeup interrupt for all devices on the bus under the bridge.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Brian Norris]

Hi Rafael,

Sorry once again on the delays...My email hygiene has been getting bad,
so mail gets buried.

On Fri, Jan 05, 2018 at 02:13:33AM +0100, Rafael J. Wysocki wrote:
> On Friday, January 5, 2018 1:41:31 AM CET Brian Norris wrote:
> > On Wed, Dec 27, 2017 at 01:57:07AM +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote:
> > > > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote:
> > > > >> >+
> > > > >> >+	dn = pci_device_to_OF_node(ppdev);
> > > > >> >+	if (!dn)
> > > > >> >+		return 0;
> > > > >> >+
> > > > >> >+	irq = of_irq_get_byname(dn, "wakeup");
> > > > > Why is this a property of the bridge and not of the device itself?
> > 
> > Wait, isn't 'dn' the port node, not the bridge node?
> 
> I may be confused about the structure you have in DT.

Probably :) And my DT structure may not be correct. But it's easily
available to review. See arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi:

http://elixir.free-electrons.com/linux/v4.14.2/source/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi#L705

It has a bridge->port->device nesting hierarchy. I'm not actually sure
where this came from exactly, but I *thought* it was derived from the
standard documentation:

Documentation/devicetree/bindings/pci/pci.txt
PCI Bus Binding to: IEEE Std 1275-1994
http://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf

I'm not so sure now...

> In the device hierarchy PCIe ports are represented as bridges.

But device hierarchy can be slightly different than DT hierarchy. In our
case, pcie@0,0 seems to correspond to the port, but has no companion
"device".

Or maybe I'm really off-base.

> > > > That is suggested by Brian, because in that way, the wakeup pin would 
> > > > not "tied to what exact device is installed (or no device, if it's a slot)."
> > 
> > I believe my thinking has evolved a bit over time, and I definitely am
> > not the one true authority on this. I'll explain my main concerns, and
> > whatever solution resolves these concerns is fine with me.
> > 
> > * I was primarily interested in avoiding handling WAKE# in the endpoint
> >   drivers (e.g., as mwifiex is today).
> 
> OK, everybody on this thread is interested in that. :-)

Sure :)

> > * I was also interested in not having to redefine a new DT device
> >   node (with new "pciABCD,1234" compatible property) for each new device
> >   attached. That just won't work for removable cards.
> 
> So if you make it the property of a bridge, it should be fine (as long
> as the bridge itself is not removable).

OK...in that case you've answered your question at the top: "Why is this
a property of the bridge and not of the device itself?" I think Jeffy
answered similarly, but this walked you through how *I* got to that
conclusion, at least.

> > I need to reread the rest of this thread a few times to really
> > understand what Rafael and Tony are discussing. But I feel like some of
> > this is still moving away from the second point above.
> > 
> > > But I don't think it works when there are two devices using different WAKE#
> > > interrupt lines under the same bridge.  Or how does it work then?
> 
> We seem to have agreed that this case needs to be neglected here.

OK, I guess that's a reasonable conclusion.

> The "wakeup-interrupt" property at the bridge level basically has to be defined
> as the wakeup interrupt for all devices on the bus under the bridge.

The only thing I'm at a loss for is whether this goes in (referring to
rk3399-gru.dtsi) &pcie or &pci_rootport. I think some versions of this
series have been aiming for the former, and some the latter.

Brian

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

* Brian Norris <briannorris@chromium.org> [180125 01:22]:
> On Fri, Jan 05, 2018 at 02:13:33AM +0100, Rafael J. Wysocki wrote:
> > The "wakeup-interrupt" property at the bridge level basically has to be defined
> > as the wakeup interrupt for all devices on the bus under the bridge.
> 
> The only thing I'm at a loss for is whether this goes in (referring to
> rk3399-gru.dtsi) &pcie or &pci_rootport. I think some versions of this
> series have been aiming for the former, and some the latter.

I'd keep the wakeup interrupt property at the rootport level. That way
it can work with whatever pcie card that might be plugged into that
slot. That is in case it's just a slot and not hardwired pcie device :)

Regards,

Tony

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Rafael J. Wysocki]

On Thu, Jan 25, 2018 at 5:40 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Brian Norris <briannorris@chromium.org> [180125 01:22]:
>> On Fri, Jan 05, 2018 at 02:13:33AM +0100, Rafael J. Wysocki wrote:
>> > The "wakeup-interrupt" property at the bridge level basically has to be defined
>> > as the wakeup interrupt for all devices on the bus under the bridge.
>>
>> The only thing I'm at a loss for is whether this goes in (referring to
>> rk3399-gru.dtsi) &pcie or &pci_rootport. I think some versions of this
>> series have been aiming for the former, and some the latter.
>
> I'd keep the wakeup interrupt property at the rootport level. That way
> it can work with whatever pcie card that might be plugged into that
> slot. That is in case it's just a slot and not hardwired pcie device :)

Do I understand correctly that &pcie is the device and the
&pci_rootport is the port that device is connected to?

Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Brian Norris]

Hi,

On Thu, Jan 25, 2018 at 05:54:23PM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 25, 2018 at 5:40 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> [180125 01:22]:
> >> On Fri, Jan 05, 2018 at 02:13:33AM +0100, Rafael J. Wysocki wrote:
> >> > The "wakeup-interrupt" property at the bridge level basically has to be defined
> >> > as the wakeup interrupt for all devices on the bus under the bridge.
> >>
> >> The only thing I'm at a loss for is whether this goes in (referring to
> >> rk3399-gru.dtsi) &pcie or &pci_rootport. I think some versions of this
> >> series have been aiming for the former, and some the latter.
> >
> > I'd keep the wakeup interrupt property at the rootport level. That way
> > it can work with whatever pcie card that might be plugged into that
> > slot. That is in case it's just a slot and not hardwired pcie device :)

^^ Right, and that's what I believe this series was doing. Previous
versions put it in &pcie, which might have had a similar effect. The
existing behavior is the misguided bindings that put it in &mvl_wifi
(the endpoint device).

> Do I understand correctly that &pcie is the device and the
> &pci_rootport is the port that device is connected to?

No. (Assuming "device" means "endpoint PCIe device".)

&pcie: The top-level representation of the host bridge

&pci_rootport: a virtual representation of a root port. I don't think
this corresponds to a Linux device in the end, but I thought it
corresponded most closely to a "slot" [1]

&mvl_wifi: an endpoint device

Brian

[1] As another example, consider
arch/arm/boot/dts/armada-370{.dtsi,-dlink-dns327l.dts}
It has a structure of:

  pciec: pcie@82000000 {
    pcie0: pcie@1,0 /* Port 0, Lane 0 */ {
      // Brian: no subnode, since we don't generally want to describe
      // specific endpoints in DT, when they should be autodetectable
    };
    pcie2: pcie@2,0 /* Port 1, Lane 0 */ {
      // Brian: no subnode, since we don't generally want to describe
      // specific endpoints in DT, when they should be autodetectable
    };
  };

Where Gru's &pcie is equivalent to Armada's &pciec, and Gru's
&pci_rootport is equivalent to &pcie0 and &pcie2.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html