[PATCH v7 0/3] PCI: rockchip: Move PCIE_WAKE handling into pci core

[PATCH v7 0/3] 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 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
Move to pci.txt
Use "wakeup" instead of "wake"

Changes in v3:
Fix error handling

Changes in v2:
Use dev_pm_set_dedicated_wake_irq
        -- Suggested by Brian Norris <briannorris@chromium.com>

Jeffy Chen (3):
  PCI: Add support for wake irq
  dt-bindings: PCI: Add definition of pcie wake irq
  arm64: dts: rockchip: Handle pcie wake in pcie driver for Gru

 Documentation/devicetree/bindings/pci/pci.txt |  2 ++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  | 15 +++++++-----
 drivers/pci/pci.c                             | 34 +++++++++++++++++++++++++--
 drivers/pci/probe.c                           | 32 +++++++++++++++++++++----
 drivers/pci/remove.c                          |  9 +++++++
 5 files changed, 80 insertions(+), 12 deletions(-)

-- 
2.11.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 1/3] PCI: Add support for wake irq

[Jeffy Chen]

Add support for PCIE_WAKE pin.

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

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
        -- Suggested by Brian Norris <briannorris@chromium.com>

 drivers/pci/pci.c    | 34 ++++++++++++++++++++++++++++++++--
 drivers/pci/probe.c  | 32 ++++++++++++++++++++++++++++----
 drivers/pci/remove.c |  9 +++++++++
 3 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f0d68066c726..49080a10bdf0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
 			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
 }
 
+static int pci_dev_check_wakeup(struct pci_dev *dev, void *data)
+{
+	bool *wakeup = data;
+
+	if (device_may_wakeup(&dev->dev))
+		*wakeup = true;
+
+	return *wakeup;
+}
+
 static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
 {
-	return pci_platform_pm ?
-			pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
+	struct pci_dev *parent = dev;
+	struct pci_bus *bus;
+	bool wakeup = false;
+
+	if (pci_platform_pm)
+		return pci_platform_pm->set_wakeup(dev, enable);
+
+	device_set_wakeup_capable(&dev->dev, enable);
+
+	while ((parent = pci_upstream_bridge(parent)))
+		bus = parent->bus;
+
+	if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent)
+		return -ENODEV;
+
+	pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup);
+	device_set_wakeup_capable(bus->bridge->parent, wakeup);
+
+	dev_dbg(bus->bridge->parent,
+		"Wakeup %s\n", wakeup ? "enabled" : "disabled");
+
+	return 0;
 }
 
 static inline bool platform_pci_need_resume(struct pci_dev *dev)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cdc2f83c11c5..fd43ca832665 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -7,6 +7,7 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/of_pci.h>
 #include <linux/pci_hotplug.h>
 #include <linux/slab.h>
@@ -17,6 +18,7 @@
 #include <linux/acpi.h>
 #include <linux/irqdomain.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeirq.h>
 #include "pci.h"
 
 #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
@@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	struct resource *res;
 	char addr[64], *fmt;
 	const char *name;
-	int err;
+	int err, irq;
+
+	if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) {
+		irq = of_irq_get_byname(parent->of_node, "wakeup");
+		if (irq == -EPROBE_DEFER)
+			return irq;
+		if (irq > 0) {
+			device_init_wakeup(parent, true);
+			err = dev_pm_set_dedicated_wake_irq(parent, irq);
+			if (err) {
+				dev_err(parent, "Failed to setup wakeup IRQ\n");
+				goto deinit_wakeup;
+			}
+			dev_info(parent, "Wakeup enabled with IRQ %d\n", irq);
+		}
+	}
 
 	bus = pci_alloc_bus(NULL);
-	if (!bus)
-		return -ENOMEM;
+	if (!bus) {
+		err = -ENOMEM;
+		goto clear_wake_irq;
+	}
 
 	bridge->bus = bus;
 
@@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 unregister:
 	put_device(&bridge->dev);
 	device_unregister(&bridge->dev);
-
 free:
 	kfree(bus);
+clear_wake_irq:
+	if (parent)
+		dev_pm_clear_wake_irq(parent);
+deinit_wakeup:
+	if (parent)
+		device_init_wakeup(parent, false);
 	return err;
 }
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 73a03d382590..cb7a326429e1 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -1,6 +1,7 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/pci-aspm.h>
+#include <linux/pm_wakeirq.h>
 #include "pci.h"
 
 static void pci_free_resources(struct pci_dev *dev)
@@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus)
 {
 	struct pci_dev *child, *tmp;
 	struct pci_host_bridge *host_bridge;
+	struct device *parent;
 
 	if (!pci_is_root_bus(bus))
 		return;
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
+	parent = host_bridge->dev.parent;
+
 	list_for_each_entry_safe_reverse(child, tmp,
 					 &bus->devices, bus_list)
 		pci_stop_bus_device(child);
 
 	/* stop the host bridge */
 	device_release_driver(&host_bridge->dev);
+
+	if (parent) {
+		dev_pm_clear_wake_irq(parent);
+		device_init_wakeup(parent, false);
+	}
 }
 EXPORT_SYMBOL_GPL(pci_stop_root_bus);
 
-- 
2.11.0

Re: [PATCH v7 1/3] PCI: Add support for wake irq

[Brian Norris]

+ PM folks

Hi Jeffy,

It's probably good if you send the whole thing to linux-pm@ in the
future, if you're really trying to implement generic PCI/PM for device
tree systems.

On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin.

This is kind of an important change, so it feels like you should
document it a little more thoroughly than this. Particularly, I have a
few questions below, and it seems like some of these questions should be
acknowledged up front. e.g., why does this look so different than the
ACPI hooks?

> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> 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
>         -- Suggested by Brian Norris <briannorris@chromium.com>
> 
>  drivers/pci/pci.c    | 34 ++++++++++++++++++++++++++++++++--
>  drivers/pci/probe.c  | 32 ++++++++++++++++++++++++++++----
>  drivers/pci/remove.c |  9 +++++++++
>  3 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f0d68066c726..49080a10bdf0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
>  			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
>  }
>  
> +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data)
> +{
> +	bool *wakeup = data;
> +
> +	if (device_may_wakeup(&dev->dev))
> +		*wakeup = true;
> +
> +	return *wakeup;
> +}
> +
>  static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
>  {
> -	return pci_platform_pm ?
> -			pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
> +	struct pci_dev *parent = dev;
> +	struct pci_bus *bus;
> +	bool wakeup = false;

It feels like you're implementing a set of pci_platform_pm_ops, except
you're not actually implementing them. It almost seems like we should
have a drivers/pci/pci-of.c to do this. But that brings up a few
questions....

> +
> +	if (pci_platform_pm)

So, if somebody already registered ops, then you won't follow the "OF"
route? That means this all breaks as soon as a kernel has both
CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64,
which 'select's OF and may also be built/run with CONFIG_ACPI.

And that conflict is the same if we try to register pci_platform_pm_ops
for OF systems -- it'll be a race over who sets them up first (or
rather, last).

Also, what happens on !ACPI && !OF? Or if the device tree did not
contain a "wakeup" definition? You're now implementing a default path
that doesn't make much sense IMO; you may claim wakeup capability
without actually having set it up somewhere.

I think you could use some more comments, and (again) a real commit
message.

> +		return pci_platform_pm->set_wakeup(dev, enable);
> +
> +	device_set_wakeup_capable(&dev->dev, enable);

Why are you setting that here? This function should just be telling the
lower layers to enable the physical WAKE# ability. In our case, it just
means configuring the WAKE# interrupt for wakeup -- or, since you've
used dev_pm_set_dedicated_wake_irq() which handles most of this
automatically...do you need this at all? It seems like you should
*either* implement these callbacks to manually manage the wakeup IRQ or
else use the dedicated wakeirq infrastructure -- not both.

And even if you need this, I don't think you need to do this many times;
you should only need to set up the capabilities once, when you first set
up the device.

And BTW, the description for the set_wakeup() callback says:

 * @set_wakeup: enables/disables wakeup capability for the device

I *don't* think that means "capability" as in the device framework's
view of "wakeup capable"; I think it means capability as in the physical
ability (a la, enable_irq_wake() or similar).

> +
> +	while ((parent = pci_upstream_bridge(parent)))
> +		bus = parent->bus;
> +
> +	if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent)
> +		return -ENODEV;
> +
> +	pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup);
> +	device_set_wakeup_capable(bus->bridge->parent, wakeup);

What happens to any intermediate buses? You haven't marked them as
wakeup-capable. Should you?

And the more fundamental question here is: is this a per-device
configuration or a per-root-port configuration? The APIs here are
modeled after ACPI, where I guess this is a per-device thing. The PCIe
spec doesn't exactly specify how many WAKE# pins you need, though it
seems to say

(a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs
    should be wired up to it)
(b) it *can* be done as a single input to the system controller, since
    it's an open drain signal
(c) ...but I also see now in the PCIe Card Electromechanical
    specification:

    "WAKE# may be bused to multiple PCI Express add-in card connectors,
    forming a single input connection at the PM controller, or
    individual connectors can have individual connections to the PM
    controller."

So I think you're kind of going along the lines of (b) (as I suggested
to you previously), and that matches the current hardware (we only have
a single WAKE#) and proposed DT binding. But should this be set up in a
way that suits (c) too? It's hard to tell exactly what ACPI-based
systems do, since they have this abstracted behind ACPI interfaces that
seem like they *could* support per-device or per-bridge type of hookups.

Bjorn, any thoughts? This seems like a halfway attempt in between two
different designs, and I'm not really sure which one makes more sense.

Brian

> +
> +	dev_dbg(bus->bridge->parent,
> +		"Wakeup %s\n", wakeup ? "enabled" : "disabled");
> +
> +	return 0;
>  }
>  
>  static inline bool platform_pci_need_resume(struct pci_dev *dev)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cdc2f83c11c5..fd43ca832665 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -7,6 +7,7 @@
>  #include <linux/init.h>
>  #include <linux/pci.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_pci.h>
>  #include <linux/pci_hotplug.h>
>  #include <linux/slab.h>
> @@ -17,6 +18,7 @@
>  #include <linux/acpi.h>
>  #include <linux/irqdomain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
>  #include "pci.h"
>  
>  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
> @@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  	struct resource *res;
>  	char addr[64], *fmt;
>  	const char *name;
> -	int err;
> +	int err, irq;
> +
> +	if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) {
> +		irq = of_irq_get_byname(parent->of_node, "wakeup");
> +		if (irq == -EPROBE_DEFER)
> +			return irq;
> +		if (irq > 0) {
> +			device_init_wakeup(parent, true);
> +			err = dev_pm_set_dedicated_wake_irq(parent, irq);
> +			if (err) {
> +				dev_err(parent, "Failed to setup wakeup IRQ\n");
> +				goto deinit_wakeup;
> +			}
> +			dev_info(parent, "Wakeup enabled with IRQ %d\n", irq);
> +		}
> +	}
>  
>  	bus = pci_alloc_bus(NULL);
> -	if (!bus)
> -		return -ENOMEM;
> +	if (!bus) {
> +		err = -ENOMEM;
> +		goto clear_wake_irq;
> +	}
>  
>  	bridge->bus = bus;
>  
> @@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  unregister:
>  	put_device(&bridge->dev);
>  	device_unregister(&bridge->dev);
> -
>  free:
>  	kfree(bus);
> +clear_wake_irq:
> +	if (parent)
> +		dev_pm_clear_wake_irq(parent);
> +deinit_wakeup:
> +	if (parent)
> +		device_init_wakeup(parent, false);
>  	return err;
>  }
>  
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 73a03d382590..cb7a326429e1 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -1,6 +1,7 @@
>  #include <linux/pci.h>
>  #include <linux/module.h>
>  #include <linux/pci-aspm.h>
> +#include <linux/pm_wakeirq.h>
>  #include "pci.h"
>  
>  static void pci_free_resources(struct pci_dev *dev)
> @@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus)
>  {
>  	struct pci_dev *child, *tmp;
>  	struct pci_host_bridge *host_bridge;
> +	struct device *parent;
>  
>  	if (!pci_is_root_bus(bus))
>  		return;
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
> +	parent = host_bridge->dev.parent;
> +
>  	list_for_each_entry_safe_reverse(child, tmp,
>  					 &bus->devices, bus_list)
>  		pci_stop_bus_device(child);
>  
>  	/* stop the host bridge */
>  	device_release_driver(&host_bridge->dev);
> +
> +	if (parent) {
> +		dev_pm_clear_wake_irq(parent);
> +		device_init_wakeup(parent, false);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(pci_stop_root_bus);
>  
> -- 
> 2.11.0
> 
> 

Re: [PATCH v7 1/3] PCI: Add support for wake irq

[jeffy]

Hi Brian,

On 10/24/2017 07:02 AM, Brian Norris wrote:
> + PM folks
>
> Hi Jeffy,
>
> It's probably good if you send the whole thing to linux-pm@ in the
> future, if you're really trying to implement generic PCI/PM for device
> tree systems.
ok
>
> On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote:
>> Add support for PCIE_WAKE pin.
>
> This is kind of an important change, so it feels like you should
> document it a little more thoroughly than this. Particularly, I have a
> few questions below, and it seems like some of these questions should be
> acknowledged up front. e.g., why does this look so different than the
> ACPI hooks?
sure, will do in next version.
>
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> 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
>>          -- Suggested by Brian Norris <briannorris@chromium.com>
>>
>>   drivers/pci/pci.c    | 34 ++++++++++++++++++++++++++++++++--
>>   drivers/pci/probe.c  | 32 ++++++++++++++++++++++++++++----
>>   drivers/pci/remove.c |  9 +++++++++
>>   3 files changed, 69 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index f0d68066c726..49080a10bdf0 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
>>   			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
>>   }
>>
>> +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data)
>> +{
>> +	bool *wakeup = data;
>> +
>> +	if (device_may_wakeup(&dev->dev))
>> +		*wakeup = true;
>> +
>> +	return *wakeup;
>> +}
>> +
>>   static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
>>   {
>> -	return pci_platform_pm ?
>> -			pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
>> +	struct pci_dev *parent = dev;
>> +	struct pci_bus *bus;
>> +	bool wakeup = false;
>
> It feels like you're implementing a set of pci_platform_pm_ops, except
> you're not actually implementing them. It almost seems like we should
> have a drivers/pci/pci-of.c to do this. But that brings up a few
> questions....
i saw the drivers might call set_wakeup() in suspend/resume/shutdown to 
configure the wakeup ability, maybe we can call 
device_set_wakeup_enable() here as a common part of set_wakeup()?

static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool 
enable) {
     device_set_wakeup_enable()
...
     return pci_platform_pm ? pci_platform_pm->set_wakeup(dev, enable) : 
-ENODEV;

>
>> +
>> +	if (pci_platform_pm)
>
> So, if somebody already registered ops, then you won't follow the "OF"
> route? That means this all breaks as soon as a kernel has both
> CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64,
> which 'select's OF and may also be built/run with CONFIG_ACPI.
>
> And that conflict is the same if we try to register pci_platform_pm_ops
> for OF systems -- it'll be a race over who sets them up first (or
> rather, last).
>
> Also, what happens on !ACPI && !OF? Or if the device tree did not
> contain a "wakeup" definition? You're now implementing a default path
> that doesn't make much sense IMO; you may claim wakeup capability
> without actually having set it up somewhere.
maybe we can use device_set_wakeup_enable(), which will check the setup 
before setting?
>
> I think you could use some more comments, and (again) a real commit
> message.
ok, will do.
>
>> +		return pci_platform_pm->set_wakeup(dev, enable);
>> +
>> +	device_set_wakeup_capable(&dev->dev, enable);
>
> Why are you setting that here? This function should just be telling the
> lower layers to enable the physical WAKE# ability. In our case, it just
> means configuring the WAKE# interrupt for wakeup -- or, since you've
> used dev_pm_set_dedicated_wake_irq() which handles most of this
> automatically...do you need this at all? It seems like you should
> *either* implement these callbacks to manually manage the wakeup IRQ or
> else use the dedicated wakeirq infrastructure -- not both.
>
> And even if you need this, I don't think you need to do this many times;
> you should only need to set up the capabilities once, when you first set
> up the device.
>
> And BTW, the description for the set_wakeup() callback says:
>
>   * @set_wakeup: enables/disables wakeup capability for the device
>
> I *don't* think that means "capability" as in the device framework's
> view of "wakeup capable"; I think it means capability as in the physical
> ability (a la, enable_irq_wake() or similar).
i was thinking maybe we should disable the wakeup if all children 
request set_wakeup(false)?

and it seems like the dedicated wakeirq can be disabled by:
1/ dev_pm_clear_wake_irq(), then we may need to store the irq somewhere 
to set it up again in the future?

2/ let device_may_wakeup return false:
void dev_pm_arm_wake_irq(struct wake_irq *wirq)
{
         if (!wirq)
                 return;

         if (device_may_wakeup(wirq->dev)) {
                 if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
                         enable_irq(wirq->irq);

                 enable_irq_wake(wirq->irq);
         }

>
>> +
>> +	while ((parent = pci_upstream_bridge(parent)))
>> +		bus = parent->bus;
>> +
>> +	if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent)
>> +		return -ENODEV;
>> +
>> +	pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup);
>> +	device_set_wakeup_capable(bus->bridge->parent, wakeup);
>
> What happens to any intermediate buses? You haven't marked them as
> wakeup-capable. Should you?
>
> And the more fundamental question here is: is this a per-device
> configuration or a per-root-port configuration? The APIs here are
> modeled after ACPI, where I guess this is a per-device thing. The PCIe
> spec doesn't exactly specify how many WAKE# pins you need, though it
> seems to say
>
> (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs
>      should be wired up to it)
> (b) it *can* be done as a single input to the system controller, since
>      it's an open drain signal
> (c) ...but I also see now in the PCIe Card Electromechanical
>      specification:
>
>      "WAKE# may be bused to multiple PCI Express add-in card connectors,
>      forming a single input connection at the PM controller, or
>      individual connectors can have individual connections to the PM
>      controller."
>
> So I think you're kind of going along the lines of (b) (as I suggested
> to you previously), and that matches the current hardware (we only have
> a single WAKE#) and proposed DT binding. But should this be set up in a
> way that suits (c) too? It's hard to tell exactly what ACPI-based
> systems do, since they have this abstracted behind ACPI interfaces that
> seem like they *could* support per-device or per-bridge type of hookups.
maybe we can try to setup wake irq for each pci devices which have it in 
the dts, then in the set_wakeup(), try to find the parents(or itself) 
who has wake irq, and enable/disable them(maybe also need a refcount)?
>
> Bjorn, any thoughts? This seems like a halfway attempt in between two
> different designs, and I'm not really sure which one makes more sense.
>
> Brian
>
>> +
>> +	dev_dbg(bus->bridge->parent,
>> +		"Wakeup %s\n", wakeup ? "enabled" : "disabled");
>> +
>> +	return 0;
>>   }
>>
>>   static inline bool platform_pci_need_resume(struct pci_dev *dev)
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index cdc2f83c11c5..fd43ca832665 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -7,6 +7,7 @@
>>   #include <linux/init.h>
>>   #include <linux/pci.h>
>>   #include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/pci_hotplug.h>
>>   #include <linux/slab.h>
>> @@ -17,6 +18,7 @@
>>   #include <linux/acpi.h>
>>   #include <linux/irqdomain.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/pm_wakeirq.h>
>>   #include "pci.h"
>>
>>   #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
>> @@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>>   	struct resource *res;
>>   	char addr[64], *fmt;
>>   	const char *name;
>> -	int err;
>> +	int err, irq;
>> +
>> +	if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) {
>> +		irq = of_irq_get_byname(parent->of_node, "wakeup");
>> +		if (irq == -EPROBE_DEFER)
>> +			return irq;
>> +		if (irq > 0) {
>> +			device_init_wakeup(parent, true);
>> +			err = dev_pm_set_dedicated_wake_irq(parent, irq);
>> +			if (err) {
>> +				dev_err(parent, "Failed to setup wakeup IRQ\n");
>> +				goto deinit_wakeup;
>> +			}
>> +			dev_info(parent, "Wakeup enabled with IRQ %d\n", irq);
>> +		}
>> +	}
>>
>>   	bus = pci_alloc_bus(NULL);
>> -	if (!bus)
>> -		return -ENOMEM;
>> +	if (!bus) {
>> +		err = -ENOMEM;
>> +		goto clear_wake_irq;
>> +	}
>>
>>   	bridge->bus = bus;
>>
>> @@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>>   unregister:
>>   	put_device(&bridge->dev);
>>   	device_unregister(&bridge->dev);
>> -
>>   free:
>>   	kfree(bus);
>> +clear_wake_irq:
>> +	if (parent)
>> +		dev_pm_clear_wake_irq(parent);
>> +deinit_wakeup:
>> +	if (parent)
>> +		device_init_wakeup(parent, false);
>>   	return err;
>>   }
>>
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 73a03d382590..cb7a326429e1 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -1,6 +1,7 @@
>>   #include <linux/pci.h>
>>   #include <linux/module.h>
>>   #include <linux/pci-aspm.h>
>> +#include <linux/pm_wakeirq.h>
>>   #include "pci.h"
>>
>>   static void pci_free_resources(struct pci_dev *dev)
>> @@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus)
>>   {
>>   	struct pci_dev *child, *tmp;
>>   	struct pci_host_bridge *host_bridge;
>> +	struct device *parent;
>>
>>   	if (!pci_is_root_bus(bus))
>>   		return;
>>
>>   	host_bridge = to_pci_host_bridge(bus->bridge);
>> +	parent = host_bridge->dev.parent;
>> +
>>   	list_for_each_entry_safe_reverse(child, tmp,
>>   					 &bus->devices, bus_list)
>>   		pci_stop_bus_device(child);
>>
>>   	/* stop the host bridge */
>>   	device_release_driver(&host_bridge->dev);
>> +
>> +	if (parent) {
>> +		dev_pm_clear_wake_irq(parent);
>> +		device_init_wakeup(parent, false);
>> +	}
>>   }
>>   EXPORT_SYMBOL_GPL(pci_stop_root_bus);
>>
>> --
>> 2.11.0
>>
>>
>
>
>

Re: [PATCH v7 1/3] PCI: Add support for wake irq

[jeffy]

Hi Brian,

checking the pci-acpi code, it has:
1/ pci_acpi_setup() and pci_acpi_cleanup() to setup/cleanup the 
wakeup(and other stuff) for pci devices

we may need it too(for per-device wake) to parse wake irq and init 
wakeup(false) and maybe setup dedicated wakeirq.

2/ acpi_pci_wakeup(), which would do:
find a parent or root bus or pci dev itself, which can do wakeup, and 
update it's wakeup ability(with a enable_count).

we may need to do something like that, but the can_wakeup() would be 
check if wake irq avaliable, and set_device/bridge_wakeup() would be 
setup/clear dedicated wakeirq, or just call device_set_wakeup_enable()



so maybe we can:
1/ add a setup_root_bus() platform ops callback to parse/setup root 
bus's wakeirq in pci_register_host_bridge(),and clean it in 
pci_stop_bus_device()

2/ add a device_setup() and device_cleanup() callbacks to setup/clean 
pci device's wake irq, and maybe call it in pci_device_probe() or 
pci_setup_device()?

3/ add a can_wakeup() and set_device_wakeup() and set_bridge_wakeup() 
callbacks, and move acpi_pci_wakeup() and acpi_pci_propagate_wakeup()'s 
code and the enable_count code into common platform_pci_set_wakeup().


does this make sense?


On 10/24/2017 12:06 PM, jeffy wrote:
> Hi Brian,
>
> On 10/24/2017 07:02 AM, Brian Norris wrote:
>> + PM folks
>>
>> Hi Jeffy,
>>
>> It's probably good if you send the whole thing to linux-pm@ in the
>> future, if you're really trying to implement generic PCI/PM for device
>> tree systems.
> ok
>>
>> On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote:
>>> Add support for PCIE_WAKE pin.
>>
>> This is kind of an important change, so it feels like you should
>> document it a little more thoroughly than this. Particularly, I have a
>> few questions below, and it seems like some of these questions should be
>> acknowledged up front. e.g., why does this look so different than the
>> ACPI hooks?
> sure, will do in next version.
>>
>>>
>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>> ---
>>>
>>> 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
>>>          -- Suggested by Brian Norris <briannorris@chromium.com>
>>>
>>>   drivers/pci/pci.c    | 34 ++++++++++++++++++++++++++++++++--
>>>   drivers/pci/probe.c  | 32 ++++++++++++++++++++++++++++----
>>>   drivers/pci/remove.c |  9 +++++++++
>>>   3 files changed, 69 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index f0d68066c726..49080a10bdf0 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -603,10 +603,40 @@ static inline pci_power_t
>>> platform_pci_choose_state(struct pci_dev *dev)
>>>               pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
>>>   }
>>>
>>> +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data)
>>> +{
>>> +    bool *wakeup = data;
>>> +
>>> +    if (device_may_wakeup(&dev->dev))
>>> +        *wakeup = true;
>>> +
>>> +    return *wakeup;
>>> +}
>>> +
>>>   static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool
>>> enable)
>>>   {
>>> -    return pci_platform_pm ?
>>> -            pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
>>> +    struct pci_dev *parent = dev;
>>> +    struct pci_bus *bus;
>>> +    bool wakeup = false;
>>
>> It feels like you're implementing a set of pci_platform_pm_ops, except
>> you're not actually implementing them. It almost seems like we should
>> have a drivers/pci/pci-of.c to do this. But that brings up a few
>> questions....
> i saw the drivers might call set_wakeup() in suspend/resume/shutdown to
> configure the wakeup ability, maybe we can call
> device_set_wakeup_enable() here as a common part of set_wakeup()?
>
> static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool
> enable) {
>      device_set_wakeup_enable()
> ...
>      return pci_platform_pm ? pci_platform_pm->set_wakeup(dev, enable) :
> -ENODEV;
>
>>
>>> +
>>> +    if (pci_platform_pm)
>>
>> So, if somebody already registered ops, then you won't follow the "OF"
>> route? That means this all breaks as soon as a kernel has both
>> CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64,
>> which 'select's OF and may also be built/run with CONFIG_ACPI.
>>
>> And that conflict is the same if we try to register pci_platform_pm_ops
>> for OF systems -- it'll be a race over who sets them up first (or
>> rather, last).
>>
>> Also, what happens on !ACPI && !OF? Or if the device tree did not
>> contain a "wakeup" definition? You're now implementing a default path
>> that doesn't make much sense IMO; you may claim wakeup capability
>> without actually having set it up somewhere.
> maybe we can use device_set_wakeup_enable(), which will check the setup
> before setting?
>>
>> I think you could use some more comments, and (again) a real commit
>> message.
> ok, will do.
>>
>>> +        return pci_platform_pm->set_wakeup(dev, enable);
>>> +
>>> +    device_set_wakeup_capable(&dev->dev, enable);
>>
>> Why are you setting that here? This function should just be telling the
>> lower layers to enable the physical WAKE# ability. In our case, it just
>> means configuring the WAKE# interrupt for wakeup -- or, since you've
>> used dev_pm_set_dedicated_wake_irq() which handles most of this
>> automatically...do you need this at all? It seems like you should
>> *either* implement these callbacks to manually manage the wakeup IRQ or
>> else use the dedicated wakeirq infrastructure -- not both.
>>
>> And even if you need this, I don't think you need to do this many times;
>> you should only need to set up the capabilities once, when you first set
>> up the device.
>>
>> And BTW, the description for the set_wakeup() callback says:
>>
>>   * @set_wakeup: enables/disables wakeup capability for the device
>>
>> I *don't* think that means "capability" as in the device framework's
>> view of "wakeup capable"; I think it means capability as in the physical
>> ability (a la, enable_irq_wake() or similar).
> i was thinking maybe we should disable the wakeup if all children
> request set_wakeup(false)?
>
> and it seems like the dedicated wakeirq can be disabled by:
> 1/ dev_pm_clear_wake_irq(), then we may need to store the irq somewhere
> to set it up again in the future?
>
> 2/ let device_may_wakeup return false:
> void dev_pm_arm_wake_irq(struct wake_irq *wirq)
> {
>          if (!wirq)
>                  return;
>
>          if (device_may_wakeup(wirq->dev)) {
>                  if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
>                          enable_irq(wirq->irq);
>
>                  enable_irq_wake(wirq->irq);
>          }
>
>>
>>> +
>>> +    while ((parent = pci_upstream_bridge(parent)))
>>> +        bus = parent->bus;
>>> +
>>> +    if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent)
>>> +        return -ENODEV;
>>> +
>>> +    pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup);
>>> +    device_set_wakeup_capable(bus->bridge->parent, wakeup);
>>
>> What happens to any intermediate buses? You haven't marked them as
>> wakeup-capable. Should you?
>>
>> And the more fundamental question here is: is this a per-device
>> configuration or a per-root-port configuration? The APIs here are
>> modeled after ACPI, where I guess this is a per-device thing. The PCIe
>> spec doesn't exactly specify how many WAKE# pins you need, though it
>> seems to say
>>
>> (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs
>>      should be wired up to it)
>> (b) it *can* be done as a single input to the system controller, since
>>      it's an open drain signal
>> (c) ...but I also see now in the PCIe Card Electromechanical
>>      specification:
>>
>>      "WAKE# may be bused to multiple PCI Express add-in card connectors,
>>      forming a single input connection at the PM controller, or
>>      individual connectors can have individual connections to the PM
>>      controller."
>>
>> So I think you're kind of going along the lines of (b) (as I suggested
>> to you previously), and that matches the current hardware (we only have
>> a single WAKE#) and proposed DT binding. But should this be set up in a
>> way that suits (c) too? It's hard to tell exactly what ACPI-based
>> systems do, since they have this abstracted behind ACPI interfaces that
>> seem like they *could* support per-device or per-bridge type of hookups.
> maybe we can try to setup wake irq for each pci devices which have it in
> the dts, then in the set_wakeup(), try to find the parents(or itself)
> who has wake irq, and enable/disable them(maybe also need a refcount)?
>>
>> Bjorn, any thoughts? This seems like a halfway attempt in between two
>> different designs, and I'm not really sure which one makes more sense.
>>
>> Brian
>>
>>> +
>>> +    dev_dbg(bus->bridge->parent,
>>> +        "Wakeup %s\n", wakeup ? "enabled" : "disabled");
>>> +
>>> +    return 0;
>>>   }
>>>
>>>   static inline bool platform_pci_need_resume(struct pci_dev *dev)
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index cdc2f83c11c5..fd43ca832665 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -7,6 +7,7 @@
>>>   #include <linux/init.h>
>>>   #include <linux/pci.h>
>>>   #include <linux/of_device.h>
>>> +#include <linux/of_irq.h>
>>>   #include <linux/of_pci.h>
>>>   #include <linux/pci_hotplug.h>
>>>   #include <linux/slab.h>
>>> @@ -17,6 +18,7 @@
>>>   #include <linux/acpi.h>
>>>   #include <linux/irqdomain.h>
>>>   #include <linux/pm_runtime.h>
>>> +#include <linux/pm_wakeirq.h>
>>>   #include "pci.h"
>>>
>>>   #define CARDBUS_LATENCY_TIMER    176    /* secondary latency timer */
>>> @@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct
>>> pci_host_bridge *bridge)
>>>       struct resource *res;
>>>       char addr[64], *fmt;
>>>       const char *name;
>>> -    int err;
>>> +    int err, irq;
>>> +
>>> +    if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) {
>>> +        irq = of_irq_get_byname(parent->of_node, "wakeup");
>>> +        if (irq == -EPROBE_DEFER)
>>> +            return irq;
>>> +        if (irq > 0) {
>>> +            device_init_wakeup(parent, true);
>>> +            err = dev_pm_set_dedicated_wake_irq(parent, irq);
>>> +            if (err) {
>>> +                dev_err(parent, "Failed to setup wakeup IRQ\n");
>>> +                goto deinit_wakeup;
>>> +            }
>>> +            dev_info(parent, "Wakeup enabled with IRQ %d\n", irq);
>>> +        }
>>> +    }
>>>
>>>       bus = pci_alloc_bus(NULL);
>>> -    if (!bus)
>>> -        return -ENOMEM;
>>> +    if (!bus) {
>>> +        err = -ENOMEM;
>>> +        goto clear_wake_irq;
>>> +    }
>>>
>>>       bridge->bus = bus;
>>>
>>> @@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct
>>> pci_host_bridge *bridge)
>>>   unregister:
>>>       put_device(&bridge->dev);
>>>       device_unregister(&bridge->dev);
>>> -
>>>   free:
>>>       kfree(bus);
>>> +clear_wake_irq:
>>> +    if (parent)
>>> +        dev_pm_clear_wake_irq(parent);
>>> +deinit_wakeup:
>>> +    if (parent)
>>> +        device_init_wakeup(parent, false);
>>>       return err;
>>>   }
>>>
>>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>>> index 73a03d382590..cb7a326429e1 100644
>>> --- a/drivers/pci/remove.c
>>> +++ b/drivers/pci/remove.c
>>> @@ -1,6 +1,7 @@
>>>   #include <linux/pci.h>
>>>   #include <linux/module.h>
>>>   #include <linux/pci-aspm.h>
>>> +#include <linux/pm_wakeirq.h>
>>>   #include "pci.h"
>>>
>>>   static void pci_free_resources(struct pci_dev *dev)
>>> @@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus)
>>>   {
>>>       struct pci_dev *child, *tmp;
>>>       struct pci_host_bridge *host_bridge;
>>> +    struct device *parent;
>>>
>>>       if (!pci_is_root_bus(bus))
>>>           return;
>>>
>>>       host_bridge = to_pci_host_bridge(bus->bridge);
>>> +    parent = host_bridge->dev.parent;
>>> +
>>>       list_for_each_entry_safe_reverse(child, tmp,
>>>                        &bus->devices, bus_list)
>>>           pci_stop_bus_device(child);
>>>
>>>       /* stop the host bridge */
>>>       device_release_driver(&host_bridge->dev);
>>> +
>>> +    if (parent) {
>>> +        dev_pm_clear_wake_irq(parent);
>>> +        device_init_wakeup(parent, false);
>>> +    }
>>>   }
>>>   EXPORT_SYMBOL_GPL(pci_stop_root_bus);
>>>
>>> --
>>> 2.11.0
>>>
>>>
>>
>>
>>
>

Re: [PATCH v7 1/3] PCI: Add support for wake irq

[Bjorn Helgaas]

Include "PCIe WAKE#" signal in the subject, since this is specifically
about that wire.

On Mon, Oct 23, 2017 at 04:02:53PM -0700, Brian Norris wrote:
> + PM folks
> 
> Hi Jeffy,
> 
> It's probably good if you send the whole thing to linux-pm@ in the
> future, if you're really trying to implement generic PCI/PM for device
> tree systems.
> 
> On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin.

I think you're referring to what the spec calls "the WAKE# signal".
It will reduce confusion if you use exactly the same notation as the
spec.

> This is kind of an important change, so it feels like you should
> document it a little more thoroughly than this. Particularly, I have a
> few questions below, and it seems like some of these questions should be
> acknowledged up front. e.g., why does this look so different than the
> ACPI hooks?
> 
> > 
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> > 
> > 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
> >         -- Suggested by Brian Norris <briannorris@chromium.com>
> > 
> >  drivers/pci/pci.c    | 34 ++++++++++++++++++++++++++++++++--
> >  drivers/pci/probe.c  | 32 ++++++++++++++++++++++++++++----
> >  drivers/pci/remove.c |  9 +++++++++
> >  3 files changed, 69 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index f0d68066c726..49080a10bdf0 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
> >  			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
> >  }
> >  
> > +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data)
> > +{
> > +	bool *wakeup = data;
> > +
> > +	if (device_may_wakeup(&dev->dev))
> > +		*wakeup = true;
> > +
> > +	return *wakeup;
> > +}
> > +
> >  static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
> >  {
> > -	return pci_platform_pm ?
> > -			pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
> > +	struct pci_dev *parent = dev;
> > +	struct pci_bus *bus;
> > +	bool wakeup = false;
> 
> It feels like you're implementing a set of pci_platform_pm_ops, except
> you're not actually implementing them. It almost seems like we should
> have a drivers/pci/pci-of.c to do this. But that brings up a few
> questions....
> 
> > +
> > +	if (pci_platform_pm)
> 
> So, if somebody already registered ops, then you won't follow the "OF"
> route? That means this all breaks as soon as a kernel has both
> CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64,
> which 'select's OF and may also be built/run with CONFIG_ACPI.
> 
> And that conflict is the same if we try to register pci_platform_pm_ops
> for OF systems -- it'll be a race over who sets them up first (or
> rather, last).
> 
> Also, what happens on !ACPI && !OF? Or if the device tree did not
> contain a "wakeup" definition? You're now implementing a default path
> that doesn't make much sense IMO; you may claim wakeup capability
> without actually having set it up somewhere.
> 
> I think you could use some more comments, and (again) a real commit
> message.
> 
> > +		return pci_platform_pm->set_wakeup(dev, enable);
> > +
> > +	device_set_wakeup_capable(&dev->dev, enable);
> 
> Why are you setting that here? This function should just be telling the
> lower layers to enable the physical WAKE# ability. In our case, it just
> means configuring the WAKE# interrupt for wakeup -- or, since you've
> used dev_pm_set_dedicated_wake_irq() which handles most of this
> automatically...do you need this at all? It seems like you should
> *either* implement these callbacks to manually manage the wakeup IRQ or
> else use the dedicated wakeirq infrastructure -- not both.
> 
> And even if you need this, I don't think you need to do this many times;
> you should only need to set up the capabilities once, when you first set
> up the device.
> 
> And BTW, the description for the set_wakeup() callback says:
> 
>  * @set_wakeup: enables/disables wakeup capability for the device
> 
> I *don't* think that means "capability" as in the device framework's
> view of "wakeup capable"; I think it means capability as in the physical
> ability (a la, enable_irq_wake() or similar).
> 
> > +
> > +	while ((parent = pci_upstream_bridge(parent)))
> > +		bus = parent->bus;
> > +
> > +	if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent)
> > +		return -ENODEV;
> > +
> > +	pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup);
> > +	device_set_wakeup_capable(bus->bridge->parent, wakeup);
> 
> What happens to any intermediate buses? You haven't marked them as
> wakeup-capable. Should you?
> 
> And the more fundamental question here is: is this a per-device
> configuration or a per-root-port configuration? The APIs here are
> modeled after ACPI, where I guess this is a per-device thing. The PCIe
> spec doesn't exactly specify how many WAKE# pins you need, though it
> seems to say
> 
> (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs
>     should be wired up to it)
> (b) it *can* be done as a single input to the system controller, since
>     it's an open drain signal
> (c) ...but I also see now in the PCIe Card Electromechanical
>     specification:
> 
>     "WAKE# may be bused to multiple PCI Express add-in card connectors,
>     forming a single input connection at the PM controller, or
>     individual connectors can have individual connections to the PM
>     controller."
> 
> So I think you're kind of going along the lines of (b) (as I suggested
> to you previously), and that matches the current hardware (we only have
> a single WAKE#) and proposed DT binding. But should this be set up in a
> way that suits (c) too? It's hard to tell exactly what ACPI-based
> systems do, since they have this abstracted behind ACPI interfaces that
> seem like they *could* support per-device or per-bridge type of hookups.
> 
> Bjorn, any thoughts? This seems like a halfway attempt in between two
> different designs, and I'm not really sure which one makes more sense.

No thoughts yet.  Seems like this needs a little more time in the
oven, and I'll take a deeper look after some of the issues you pointed
out have been addressed.

Bjorn

Re: [PATCH v7 1/3] PCI: Add support for wake irq

[Rafael J. Wysocki]

On Tue, Oct 24, 2017 at 10:10 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Include "PCIe WAKE#" signal in the subject, since this is specifically
> about that wire.
>
> On Mon, Oct 23, 2017 at 04:02:53PM -0700, Brian Norris wrote:
>> + PM folks
>>
>> Hi Jeffy,
>>
>> It's probably good if you send the whole thing to linux-pm@ in the
>> future, if you're really trying to implement generic PCI/PM for device
>> tree systems.
>>
>> On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote:
>> > Add support for PCIE_WAKE pin.
>
> I think you're referring to what the spec calls "the WAKE# signal".
> It will reduce confusion if you use exactly the same notation as the
> spec.
>
>> This is kind of an important change, so it feels like you should
>> document it a little more thoroughly than this. Particularly, I have a
>> few questions below, and it seems like some of these questions should be
>> acknowledged up front. e.g., why does this look so different than the
>> ACPI hooks?
>>
>> >
>> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> > ---
>> >
>> > 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
>> >         -- Suggested by Brian Norris <briannorris@chromium.com>
>> >
>> >  drivers/pci/pci.c    | 34 ++++++++++++++++++++++++++++++++--
>> >  drivers/pci/probe.c  | 32 ++++++++++++++++++++++++++++----
>> >  drivers/pci/remove.c |  9 +++++++++
>> >  3 files changed, 69 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> > index f0d68066c726..49080a10bdf0 100644
>> > --- a/drivers/pci/pci.c
>> > +++ b/drivers/pci/pci.c
>> > @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
>> >                     pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
>> >  }
>> >
>> > +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data)
>> > +{
>> > +   bool *wakeup = data;
>> > +
>> > +   if (device_may_wakeup(&dev->dev))
>> > +           *wakeup = true;
>> > +
>> > +   return *wakeup;
>> > +}
>> > +
>> >  static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
>> >  {
>> > -   return pci_platform_pm ?
>> > -                   pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
>> > +   struct pci_dev *parent = dev;
>> > +   struct pci_bus *bus;
>> > +   bool wakeup = false;
>>
>> It feels like you're implementing a set of pci_platform_pm_ops, except
>> you're not actually implementing them. It almost seems like we should
>> have a drivers/pci/pci-of.c to do this. But that brings up a few
>> questions....
>>
>> > +
>> > +   if (pci_platform_pm)
>>
>> So, if somebody already registered ops, then you won't follow the "OF"
>> route? That means this all breaks as soon as a kernel has both
>> CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64,
>> which 'select's OF and may also be built/run with CONFIG_ACPI.
>>
>> And that conflict is the same if we try to register pci_platform_pm_ops
>> for OF systems -- it'll be a race over who sets them up first (or
>> rather, last).
>>
>> Also, what happens on !ACPI && !OF? Or if the device tree did not
>> contain a "wakeup" definition? You're now implementing a default path
>> that doesn't make much sense IMO; you may claim wakeup capability
>> without actually having set it up somewhere.
>>
>> I think you could use some more comments, and (again) a real commit
>> message.
>>
>> > +           return pci_platform_pm->set_wakeup(dev, enable);
>> > +
>> > +   device_set_wakeup_capable(&dev->dev, enable);
>>
>> Why are you setting that here? This function should just be telling the
>> lower layers to enable the physical WAKE# ability. In our case, it just
>> means configuring the WAKE# interrupt for wakeup -- or, since you've
>> used dev_pm_set_dedicated_wake_irq() which handles most of this
>> automatically...do you need this at all? It seems like you should
>> *either* implement these callbacks to manually manage the wakeup IRQ or
>> else use the dedicated wakeirq infrastructure -- not both.
>>
>> And even if you need this, I don't think you need to do this many times;
>> you should only need to set up the capabilities once, when you first set
>> up the device.
>>
>> And BTW, the description for the set_wakeup() callback says:
>>
>>  * @set_wakeup: enables/disables wakeup capability for the device
>>
>> I *don't* think that means "capability" as in the device framework's
>> view of "wakeup capable"; I think it means capability as in the physical
>> ability (a la, enable_irq_wake() or similar).
>>
>> > +
>> > +   while ((parent = pci_upstream_bridge(parent)))
>> > +           bus = parent->bus;
>> > +
>> > +   if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent)
>> > +           return -ENODEV;
>> > +
>> > +   pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup);
>> > +   device_set_wakeup_capable(bus->bridge->parent, wakeup);
>>
>> What happens to any intermediate buses? You haven't marked them as
>> wakeup-capable. Should you?
>>
>> And the more fundamental question here is: is this a per-device
>> configuration or a per-root-port configuration? The APIs here are
>> modeled after ACPI, where I guess this is a per-device thing. The PCIe
>> spec doesn't exactly specify how many WAKE# pins you need, though it
>> seems to say
>>
>> (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs
>>     should be wired up to it)
>> (b) it *can* be done as a single input to the system controller, since
>>     it's an open drain signal
>> (c) ...but I also see now in the PCIe Card Electromechanical
>>     specification:
>>
>>     "WAKE# may be bused to multiple PCI Express add-in card connectors,
>>     forming a single input connection at the PM controller, or
>>     individual connectors can have individual connections to the PM
>>     controller."
>>
>> So I think you're kind of going along the lines of (b) (as I suggested
>> to you previously), and that matches the current hardware (we only have
>> a single WAKE#) and proposed DT binding. But should this be set up in a
>> way that suits (c) too? It's hard to tell exactly what ACPI-based
>> systems do, since they have this abstracted behind ACPI interfaces that
>> seem like they *could* support per-device or per-bridge type of hookups.
>>
>> Bjorn, any thoughts? This seems like a halfway attempt in between two
>> different designs, and I'm not really sure which one makes more sense.
>
> No thoughts yet.  Seems like this needs a little more time in the
> oven,

Agreed.

> and I'll take a deeper look after some of the issues you pointed
> out have been addressed.

This is in my list of things to look at, but I'm working on something
else now, so I'll be looking at it when I'm done with the other
thing(s).

Thanks,
Rafael

[PATCH v7 2/3] dt-bindings: PCI: Add definition of pcie wake irq

[Jeffy Chen]

Add an optional interrupt for PCIE_WAKE pin.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Acked-by: Rob Herring <robh@kernel.org>
---

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

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index c77981c5dd18..5ae3a9e0134d 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,5 @@ 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.
+- interrupts: Interrupt specifier for each name in interrupt-names.
+- interrupt-names: May contains "wakeup" for PCI WAKE# interrupt.
-- 
2.11.0