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

[RFC PATCH v8 0/7] 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 v8:
Add optional "pci", and rewrite commit message.
Rewrite the commit message.
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:
Move to pci.txt
Use "wakeup" instead of "wake"
Rebase.

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (7):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  mwifiex: Disable wakeup irq handling for pcie
  arm64: dts: rockchip: Handle PCIe WAKE# signal in pcie driver for Gru
  of/irq: Adjust of pci irq parsing for multiple interrupts
  PCI: Make pci_platform_pm_ops's callbacks optional
  PCI / PM: Move acpi wakeup code to pci core
  PCI / PM: Add support for the PCIe WAKE# signal for OF

 Documentation/devicetree/bindings/pci/pci.txt |   3 +
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  |  15 +--
 drivers/net/wireless/marvell/mwifiex/main.c   |   4 +
 drivers/of/of_pci_irq.c                       |  13 ++-
 drivers/pci/Makefile                          |   2 +-
 drivers/pci/pci-acpi.c                        | 121 +++++++++++------------
 drivers/pci/pci-driver.c                      |   9 ++
 drivers/pci/pci-of.c                          | 136 ++++++++++++++++++++++++++
 drivers/pci/pci.c                             | 112 +++++++++++++++++----
 drivers/pci/pci.h                             |  31 ++++--
 drivers/pci/probe.c                           |  12 ++-
 drivers/pci/remove.c                          |   2 +
 include/linux/pci.h                           |   2 +
 13 files changed, 365 insertions(+), 97 deletions(-)
 create mode 100644 drivers/pci/pci-of.c

-- 
2.11.0



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

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

[Jeffy Chen]

Add optional interrupts for PCIe WAKE# pin and PCI interrupt pin.

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

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

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index c77981c5dd18..faed405811cd 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,6 @@ 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 PCIe WAKE# interrupt and "pci"
+  for PCI interrupt.
-- 
2.11.0

Re: [RFC PATCH v8 1/7] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq

[Brian Norris]

On Thu, Oct 26, 2017 at 09:28:34PM +0800, Jeffy Chen wrote:
> Add optional interrupts for PCIe WAKE# pin and PCI interrupt pin.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
> index c77981c5dd18..faed405811cd 100644
> --- a/Documentation/devicetree/bindings/pci/pci.txt
> +++ b/Documentation/devicetree/bindings/pci/pci.txt
> @@ -24,3 +24,6 @@ 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 PCIe WAKE# interrupt and "pci"
> +  for PCI interrupt.

Similar criticism to what Rob made on patch 4: this file already says "a
host bridge driver implementation may support the following properties",
so this property is clearly not for child devices. And so having the
"PCI interrupt" here doesn't make much sense.

Similarly, you're documenting "wakeup" here as a host bridge property,
but then patch 7 is adding per-device support it seems? That seems
wrong.

In fact, I'm pretty sure this series fails to actually look in the host
bridge for the "wakeup" interrupt at all! Did you actually test this?

And again, describing your intentions a little better in the commit
message would make this clearer. Then we could tell which way you
intended this to work...

Brian

Re: [RFC PATCH v8 1/7] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq

[jeffy]

Hi Brian,

On 10/27/2017 10:33 AM, Brian Norris wrote:
> On Thu, Oct 26, 2017 at 09:28:34PM +0800, Jeffy Chen wrote:
>> Add optional interrupts for PCIe WAKE# pin and PCI interrupt pin.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> 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 | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
>> index c77981c5dd18..faed405811cd 100644
>> --- a/Documentation/devicetree/bindings/pci/pci.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci.txt
>> @@ -24,3 +24,6 @@ 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 PCIe WAKE# interrupt and "pci"
>> +  for PCI interrupt.
>
> Similar criticism to what Rob made on patch 4: this file already says "a
> host bridge driver implementation may support the following properties",
> so this property is clearly not for child devices. And so having the
> "PCI interrupt" here doesn't make much sense.
>
> Similarly, you're documenting "wakeup" here as a host bridge property,
> but then patch 7 is adding per-device support it seems? That seems
> wrong.
oops...so there's no section for PCI device here, maybe i should add a 
section about "PCI device have standardized Device Tree bindings:" to 
place it? will do in next version.


>
> In fact, I'm pretty sure this series fails to actually look in the host
> bridge for the "wakeup" interrupt at all! Did you actually test this?
actually it could...

static void *of_pci_setup(struct device *dev)
{
...
         device_init_wakeup(dev, false);

         dev_info(dev, "Wakeup IRQ %d\n", irq);
         return data;
}

[    1.546561] OF: PCI:   MEM 0xfa000000..0xfbdfffff -> 0xfa000000
[    1.553154] OF: PCI:    IO 0xfbe00000..0xfbefffff -> 0xfbe00000
[    1.560859] rockchip-pcie f8000000.pcie: Wakeup IRQ 64
[    1.566555] rockchip-pcie f8000000.pcie: PCI host bridge to bus

>
> And again, describing your intentions a little better in the commit
> message would make this clearer. Then we could tell which way you
> intended this to work...
ok, will do in next version...
>
> Brian
>
>
>

Re: [RFC PATCH v8 1/7] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq

[Brian Norris]

Hi Jeffy,

On Fri, Oct 27, 2017 at 11:06:34AM +0800, Jeffy Chen wrote:
> On 10/27/2017 10:33 AM, Brian Norris wrote:
> >In fact, I'm pretty sure this series fails to actually look in the host
> >bridge for the "wakeup" interrupt at all! Did you actually test this?
> actually it could...
> 
> static void *of_pci_setup(struct device *dev)
> {
> ...
>         device_init_wakeup(dev, false);
> 
>         dev_info(dev, "Wakeup IRQ %d\n", irq);
>         return data;
> }
> 
> [    1.546561] OF: PCI:   MEM 0xfa000000..0xfbdfffff -> 0xfa000000
> [    1.553154] OF: PCI:    IO 0xfbe00000..0xfbefffff -> 0xfbe00000
> [    1.560859] rockchip-pcie f8000000.pcie: Wakeup IRQ 64
> [    1.566555] rockchip-pcie f8000000.pcie: PCI host bridge to bus

Hmm, I think I missed the .setup_host_bridge() stuff. So you do handle
both. I'll have to take a little closer look tomorrow. But you
definitely at least need to improve the documentation as mentioned.

Another odd thing about this series is that the interrupt doesn't
actually show up in /proc/interrupts, /sys/kernel/debug/gpio, or
similar, seemingly because the wakeirq is requested/released every time
we suspend/resume. So it's really not that obvious that the interrupt is
being configured properly. That's not really a functional problem,
necessarily, but it doesn't quite seem ideal.

Brian

Re: [RFC PATCH v8 1/7] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq

[jeffy]

Hi Brian,

On 10/27/2017 01:40 PM, Brian Norris wrote:
> Another odd thing about this series is that the interrupt doesn't
> actually show up in /proc/interrupts, /sys/kernel/debug/gpio, or
> similar, seemingly because the wakeirq is requested/released every time
> we suspend/resume. So it's really not that obvious that the interrupt is
> being configured properly. That's not really a functional problem,
> necessarily, but it doesn't quite seem ideal.
>
right, so maybe we can call dev_pm_set_dedicated_wake_irq() in the 
setup(), and use device_set_wakeup_enable() to enable/disable it in the 
set_wakeup()?

> Brian

[RFC PATCH v8 5/7] PCI: Make pci_platform_pm_ops's callbacks optional

[Jeffy Chen]

Allow platforms not to provide some of the pci_platform_pm_ops's
callbacks.

Also change the return value -ENOSYS to -ENODEV for:
warning: drivers/pci/pci.c,594: ENOSYS means 'invalid syscall nr' and nothing else

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

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

 drivers/pci/pci.c | 38 +++++++++++++++++++++++++-------------
 drivers/pci/pci.h |  5 +----
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f0d68066c726..e120b00a9017 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -572,46 +572,58 @@ static void pci_restore_bars(struct pci_dev *dev)
 
 static const struct pci_platform_pm_ops *pci_platform_pm;
 
-int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
+void pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
 {
-	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
-	    !ops->choose_state  || !ops->set_wakeup || !ops->need_resume)
-		return -EINVAL;
 	pci_platform_pm = ops;
-	return 0;
 }
 
 static inline bool platform_pci_power_manageable(struct pci_dev *dev)
 {
-	return pci_platform_pm ? pci_platform_pm->is_manageable(dev) : false;
+	if (pci_platform_pm && pci_platform_pm->is_manageable)
+		return pci_platform_pm->is_manageable(dev);
+
+	return false;
 }
 
 static inline int platform_pci_set_power_state(struct pci_dev *dev,
 					       pci_power_t t)
 {
-	return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS;
+	if (pci_platform_pm && pci_platform_pm->set_state)
+		return pci_platform_pm->set_state(dev, t);
+
+	return -ENODEV;
 }
 
 static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
 {
-	return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN;
+	if (pci_platform_pm && pci_platform_pm->get_state)
+		return pci_platform_pm->get_state(dev);
+
+	return PCI_UNKNOWN;
 }
 
 static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
 {
-	return pci_platform_pm ?
-			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
+	if (pci_platform_pm && pci_platform_pm->choose_state)
+		return pci_platform_pm->choose_state(dev);
+
+	return PCI_POWER_ERROR;
 }
 
 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;
+	if (pci_platform_pm && pci_platform_pm->set_wakeup)
+		return pci_platform_pm->set_wakeup(dev, enable);
+
+	return -ENODEV;
 }
 
 static inline bool platform_pci_need_resume(struct pci_dev *dev)
 {
-	return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
+	if (pci_platform_pm && pci_platform_pm->need_resume)
+		return pci_platform_pm->need_resume(dev);
+
+	return false;
 }
 
 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e816a13e6259..048668271014 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -52,9 +52,6 @@ int pci_probe_reset_function(struct pci_dev *dev);
  * @need_resume: returns 'true' if the given device (which is currently
  *		suspended) needs to be resumed to be configured for system
  *		wakeup.
- *
- * If given platform is generally capable of power managing PCI devices, all of
- * these callbacks are mandatory.
  */
 struct pci_platform_pm_ops {
 	bool (*is_manageable)(struct pci_dev *dev);
@@ -65,7 +62,7 @@ struct pci_platform_pm_ops {
 	bool (*need_resume)(struct pci_dev *dev);
 };
 
-int pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
+void pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_power_up(struct pci_dev *dev);
 void pci_disable_enabled_device(struct pci_dev *dev);
-- 
2.11.0

[RFC PATCH v8 6/7] PCI / PM: Move acpi wakeup code to pci core

[Jeffy Chen]

Move acpi wakeup code to pci core as pci_set_wakeup(), so that other
platform could reuse it.

Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm
ops's callbacks to setup and cleanup pci devices and host bridge for
wakeup.

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

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

 drivers/pci/pci-acpi.c   | 121 +++++++++++++++++++++++------------------------
 drivers/pci/pci-driver.c |   9 ++++
 drivers/pci/pci.c        |  84 ++++++++++++++++++++++++++++----
 drivers/pci/pci.h        |  28 +++++++++--
 drivers/pci/probe.c      |  12 ++++-
 drivers/pci/remove.c     |   2 +
 include/linux/pci.h      |   2 +
 7 files changed, 180 insertions(+), 78 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 4708eb9df71b..ee96e7afe1ac 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -569,31 +569,6 @@ static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
 	return state_conv[state];
 }
 
-static int acpi_pci_propagate_wakeup(struct pci_bus *bus, bool enable)
-{
-	while (bus->parent) {
-		if (acpi_pm_device_can_wakeup(&bus->self->dev))
-			return acpi_pm_set_bridge_wakeup(&bus->self->dev, enable);
-
-		bus = bus->parent;
-	}
-
-	/* We have reached the root bus. */
-	if (bus->bridge) {
-		if (acpi_pm_device_can_wakeup(bus->bridge))
-			return acpi_pm_set_bridge_wakeup(bus->bridge, enable);
-	}
-	return 0;
-}
-
-static int acpi_pci_wakeup(struct pci_dev *dev, bool enable)
-{
-	if (acpi_pm_device_can_wakeup(&dev->dev))
-		return acpi_pm_set_device_wakeup(&dev->dev, enable);
-
-	return acpi_pci_propagate_wakeup(dev->bus, enable);
-}
-
 static bool acpi_pci_need_resume(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
@@ -610,14 +585,29 @@ static bool acpi_pci_need_resume(struct pci_dev *dev)
 	return !!adev->power.flags.dsw_present;
 }
 
-static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
-	.is_manageable = acpi_pci_power_manageable,
-	.set_state = acpi_pci_set_power_state,
-	.get_state = acpi_pci_get_power_state,
-	.choose_state = acpi_pci_choose_state,
-	.set_wakeup = acpi_pci_wakeup,
-	.need_resume = acpi_pci_need_resume,
-};
+static bool acpi_pci_can_wakeup(void *pmdata)
+{
+	struct device *dev = pmdata;
+
+	if (!dev)
+		return false;
+
+	return acpi_pm_device_can_wakeup(dev);
+}
+
+static int acpi_pci_dev_wakeup(void *pmdata, bool enable)
+{
+	struct device *dev = pmdata;
+
+	return acpi_pm_set_device_wakeup(dev, enable);
+}
+
+static int acpi_pci_bridge_wakeup(void *pmdata, bool enable)
+{
+	struct device *dev = pmdata;
+
+	return acpi_pm_set_bridge_wakeup(dev, enable);
+}
 
 void acpi_pci_add_bus(struct pci_bus *bus)
 {
@@ -658,20 +648,6 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
 	acpi_pci_slot_remove(bus);
 }
 
-/* ACPI bus type */
-static struct acpi_device *acpi_pci_find_companion(struct device *dev)
-{
-	struct pci_dev *pci_dev = to_pci_dev(dev);
-	bool check_children;
-	u64 addr;
-
-	check_children = pci_is_bridge(pci_dev);
-	/* Please ref to ACPI spec for the syntax of _ADR */
-	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
-	return acpi_find_child_device(ACPI_COMPANION(dev->parent), addr,
-				      check_children);
-}
-
 /**
  * pci_acpi_optimize_delay - optimize PCI D3 and D3cold delay from ACPI
  * @pdev: the PCI device whose delay is to be updated
@@ -723,34 +699,55 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
 	ACPI_FREE(obj);
 }
 
-static void pci_acpi_setup(struct device *dev)
+static void *acpi_pci_setup_dev(struct pci_dev *pci_dev)
 {
-	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *adev = ACPI_COMPANION(&pci_dev->dev);
 
 	if (!adev)
-		return;
+		return NULL;
 
 	pci_acpi_optimize_delay(pci_dev, adev->handle);
 
 	pci_acpi_add_pm_notifier(adev, pci_dev);
 	if (!adev->wakeup.flags.valid)
-		return;
+		return NULL;
+
+	device_set_wakeup_capable(&pci_dev->dev, true);
+	acpi_pm_set_device_wakeup(&pci_dev->dev, false);
 
-	device_set_wakeup_capable(dev, true);
-	acpi_pci_wakeup(pci_dev, false);
+	return &pci_dev->dev;
 }
 
-static void pci_acpi_cleanup(struct device *dev)
+static void *acpi_pci_setup_host_bridge(struct pci_host_bridge *bridge)
 {
-	struct acpi_device *adev = ACPI_COMPANION(dev);
+	return &bridge->dev;
+}
 
-	if (!adev)
-		return;
+static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
+	.is_manageable = acpi_pci_power_manageable,
+	.set_state = acpi_pci_set_power_state,
+	.get_state = acpi_pci_get_power_state,
+	.choose_state = acpi_pci_choose_state,
+	.need_resume = acpi_pci_need_resume,
+	.setup_dev = acpi_pci_setup_dev,
+	.setup_host_bridge = acpi_pci_setup_host_bridge,
+	.can_wakeup = acpi_pci_can_wakeup,
+	.dev_wakeup = acpi_pci_dev_wakeup,
+	.bridge_wakeup = acpi_pci_bridge_wakeup,
+};
+
+/* ACPI bus type */
+static struct acpi_device *acpi_pci_find_companion(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	bool check_children;
+	u64 addr;
 
-	pci_acpi_remove_pm_notifier(adev);
-	if (adev->wakeup.flags.valid)
-		device_set_wakeup_capable(dev, false);
+	check_children = pci_is_bridge(pci_dev);
+	/* Please ref to ACPI spec for the syntax of _ADR */
+	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
+	return acpi_find_child_device(ACPI_COMPANION(dev->parent), addr,
+				      check_children);
 }
 
 static bool pci_acpi_bus_match(struct device *dev)
@@ -762,8 +759,6 @@ static struct acpi_bus_type acpi_pci_bus = {
 	.name = "PCI",
 	.match = pci_acpi_bus_match,
 	.find_companion = acpi_pci_find_companion,
-	.setup = pci_acpi_setup,
-	.cleanup = pci_acpi_cleanup,
 };
 
 
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 9be563067c0c..abb7caa8a6e5 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -421,10 +421,17 @@ static int pci_device_probe(struct device *dev)
 	if (error < 0)
 		return error;
 
+	pci_dev->pmdata = platform_pci_setup_dev(pci_dev);
+	if (IS_ERR(pci_dev->pmdata)) {
+		pcibios_free_irq(pci_dev);
+		return PTR_ERR(pci_dev->pmdata);
+	}
+
 	pci_dev_get(pci_dev);
 	if (pci_device_can_probe(pci_dev)) {
 		error = __pci_device_probe(drv, pci_dev);
 		if (error) {
+			platform_pci_cleanup(pci_dev->pmdata);
 			pcibios_free_irq(pci_dev);
 			pci_dev_put(pci_dev);
 		}
@@ -438,6 +445,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;
 
+	platform_pci_cleanup(pci_dev->pmdata);
+
 	if (drv) {
 		if (drv->remove) {
 			pm_runtime_get_sync(dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e120b00a9017..6add5d3209bf 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -585,6 +585,52 @@ static inline bool platform_pci_power_manageable(struct pci_dev *dev)
 	return false;
 }
 
+void *platform_pci_setup_dev(struct pci_dev *dev)
+{
+	if (pci_platform_pm && pci_platform_pm->setup_dev)
+		return pci_platform_pm->setup_dev(dev);
+
+	return NULL;
+}
+
+void *platform_pci_setup_host_bridge(struct pci_host_bridge *bridge)
+{
+	if (pci_platform_pm && pci_platform_pm->setup_host_bridge)
+		return pci_platform_pm->setup_host_bridge(bridge);
+
+	return NULL;
+}
+
+void platform_pci_cleanup(void *pmdata)
+{
+	if (pci_platform_pm && pci_platform_pm->cleanup)
+		pci_platform_pm->cleanup(pmdata);
+}
+
+static inline bool platform_pci_can_wakeup(void *pmdata)
+{
+	if (pci_platform_pm && pci_platform_pm->can_wakeup)
+		return pci_platform_pm->can_wakeup(pmdata);
+
+	return false;
+}
+
+static inline int platform_pci_dev_wakeup(void *pmdata, bool enable)
+{
+	if (pci_platform_pm && pci_platform_pm->dev_wakeup)
+		return pci_platform_pm->dev_wakeup(pmdata, enable);
+
+	return -ENODEV;
+}
+
+static inline int platform_pci_bridge_wakeup(void *pmdata, bool enable)
+{
+	if (pci_platform_pm && pci_platform_pm->bridge_wakeup)
+		return pci_platform_pm->bridge_wakeup(pmdata, enable);
+
+	return -ENODEV;
+}
+
 static inline int platform_pci_set_power_state(struct pci_dev *dev,
 					       pci_power_t t)
 {
@@ -610,14 +656,6 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
 	return PCI_POWER_ERROR;
 }
 
-static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
-{
-	if (pci_platform_pm && pci_platform_pm->set_wakeup)
-		return pci_platform_pm->set_wakeup(dev, enable);
-
-	return -ENODEV;
-}
-
 static inline bool platform_pci_need_resume(struct pci_dev *dev)
 {
 	if (pci_platform_pm && pci_platform_pm->need_resume)
@@ -1903,6 +1941,32 @@ void pci_pme_active(struct pci_dev *dev, bool enable)
 }
 EXPORT_SYMBOL(pci_pme_active);
 
+static int pci_set_wakeup(struct pci_dev *dev, bool enable)
+{
+	struct pci_bus *bus = dev->bus;
+	struct pci_host_bridge *bridge;
+
+	/* Handle per device wakeup  */
+	if (platform_pci_can_wakeup(dev->pmdata))
+		return platform_pci_dev_wakeup(dev->pmdata, enable);
+
+	/* Find a parent which can handle wakeup */
+	while (bus->parent) {
+		if (platform_pci_can_wakeup(bus->self->pmdata))
+			return platform_pci_bridge_wakeup(bus->self->pmdata,
+							  enable);
+
+		bus = bus->parent;
+	}
+
+	/* We have reached the root bus. */
+	bridge = to_pci_host_bridge(bus->bridge);
+	if (platform_pci_can_wakeup(bridge->pmdata))
+		return platform_pci_bridge_wakeup(bridge->pmdata, enable);
+
+	return 0;
+}
+
 /**
  * pci_enable_wake - enable PCI device as wakeup event source
  * @dev: PCI device affected
@@ -1950,13 +2014,13 @@ int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable)
 			pci_pme_active(dev, true);
 		else
 			ret = 1;
-		error = platform_pci_set_wakeup(dev, true);
+		error = pci_set_wakeup(dev, true);
 		if (ret)
 			ret = error;
 		if (!ret)
 			dev->wakeup_prepared = true;
 	} else {
-		platform_pci_set_wakeup(dev, false);
+		pci_set_wakeup(dev, false);
 		pci_pme_active(dev, false);
 		dev->wakeup_prepared = false;
 	}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 048668271014..dcefb9e759d7 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -47,21 +47,43 @@ int pci_probe_reset_function(struct pci_dev *dev);
  *                platform; to be used during system-wide transitions from a
  *                sleeping state to the working state and vice versa
  *
- * @set_wakeup: enables/disables wakeup capability for the device
- *
  * @need_resume: returns 'true' if the given device (which is currently
  *		suspended) needs to be resumed to be configured for system
  *		wakeup.
+ *
+ * @setup_dev: setup device's wakeup ability, alloc and return device's private
+ * 		pm data.
+ *
+ * @setup_host_bridge: setup host bridge's wakeup ability, alloc and return host
+ * 		bridge's private pm data.
+ *
+ * @cleanup: cleanup the private pm data and deinit wakeup ability.
+ *
+ * @can_wakeup: returns 'true' if given device is capable of waking up the
+ *		system from a sleeping state.
+ *
+ * @dev_wakeup: enables/disables wakeup capability for the device.
+ *
+ * @bridge_wakeup: enables/disables wakeup capability for the bridge.
  */
 struct pci_platform_pm_ops {
 	bool (*is_manageable)(struct pci_dev *dev);
 	int (*set_state)(struct pci_dev *dev, pci_power_t state);
 	pci_power_t (*get_state)(struct pci_dev *dev);
 	pci_power_t (*choose_state)(struct pci_dev *dev);
-	int (*set_wakeup)(struct pci_dev *dev, bool enable);
 	bool (*need_resume)(struct pci_dev *dev);
+	void *(*setup_dev)(struct pci_dev *dev);
+	void *(*setup_host_bridge)(struct pci_host_bridge *bridge);
+	void (*cleanup)(void *pmdata);
+	bool (*can_wakeup)(void *pmdata);
+	int (*dev_wakeup)(void *pmdata, bool enable);
+	int (*bridge_wakeup)(void *pmdata, bool enable);
 };
 
+void *platform_pci_setup_dev(struct pci_dev *dev);
+void *platform_pci_setup_host_bridge(struct pci_host_bridge *bridge);
+void platform_pci_cleanup(void *pmdata);
+
 void pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_power_up(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cdc2f83c11c5..b12c552a5522 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -809,7 +809,13 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 
 	err = device_register(&bus->dev);
 	if (err)
-		goto unregister;
+		goto unregister_bridge;
+
+	bridge->pmdata = platform_pci_setup_host_bridge(bridge);
+	if (IS_ERR(bridge->pmdata)) {
+		err = PTR_ERR(bridge->pmdata);
+		goto unregister_bus;
+	}
 
 	pcibios_add_bus(bus);
 
@@ -853,7 +859,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 
 	return 0;
 
-unregister:
+unregister_bus:
+	device_unregister(&bus->dev);
+unregister_bridge:
 	put_device(&bridge->dev);
 	device_unregister(&bridge->dev);
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 73a03d382590..d7a8cf1dc69f 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -153,6 +153,8 @@ void pci_remove_root_bus(struct pci_bus *bus)
 	if (!pci_is_root_bus(bus))
 		return;
 
+	platform_pci_cleanup(host_bridge->pmdata);
+
 	host_bridge = to_pci_host_bridge(bus->bridge);
 	list_for_each_entry_safe(child, tmp,
 				 &bus->devices, bus_list)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 80eaa2dbe3e9..628faa58c790 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -293,6 +293,7 @@ struct pci_dev {
 	struct pci_bus	*subordinate;	/* bus this device bridges to */
 
 	void		*sysdata;	/* hook for sys-specific extension */
+	void		*pmdata;	/* data for platform pm */
 	struct proc_dir_entry *procent;	/* device entry in /proc/bus/pci */
 	struct pci_slot	*slot;		/* Physical slot this device is in */
 
@@ -467,6 +468,7 @@ struct pci_host_bridge {
 	struct pci_bus *bus;		/* root bus */
 	struct pci_ops *ops;
 	void *sysdata;
+	void *pmdata;			/* data for platform pm */
 	int busnr;
 	struct list_head windows;	/* resource_entry */
 	u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* platform IRQ swizzler */
-- 
2.11.0

[RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Jeffy Chen]

Add pci-of.c 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 v8:
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.

Changes in v7:
Move PCIE_WAKE handling into pci core.

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

Changes in v5:
Rebase.

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

 drivers/pci/Makefile |   2 +-
 drivers/pci/pci-of.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/pci-of.c

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 66a21acad952..4f76dbdb024c 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
 
 obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
 
-obj-$(CONFIG_OF) += of.o
+obj-$(CONFIG_OF) += of.o pci-of.o
 
 ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
 
diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
new file mode 100644
index 000000000000..55a33206fc84
--- /dev/null
+++ b/drivers/pci/pci-of.c
@@ -0,0 +1,136 @@
+/*
+ * OF PCI PM support
+ *
+ * Copyright (c) 2017 Rockchip, Inc.
+ *
+ * Author: Jeffy Chen <jeffy.chen@rock-chips.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/of_irq.h>
+#include <linux/pci.h>
+#include <linux/pm_wakeirq.h>
+#include "pci.h"
+
+struct of_pci_pm_data {
+	struct device	*dev;
+	unsigned int	wakeup_irq;
+	atomic_t	wakeup_cnt;
+};
+
+static void *of_pci_setup(struct device *dev)
+{
+	struct of_pci_pm_data *data;
+	int irq;
+
+	if (!dev->of_node)
+		return NULL;
+
+	data = devm_kzalloc(dev, sizeof(struct of_pci_pm_data), GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	irq = of_irq_get_byname(dev->of_node, "wakeup");
+	if (irq < 0) {
+		if (irq == -EPROBE_DEFER)
+			return ERR_PTR(irq);
+
+		return NULL;
+	}
+
+	data->wakeup_irq = irq;
+	data->dev = dev;
+
+	device_init_wakeup(dev, false);
+
+	dev_info(dev, "Wakeup IRQ %d\n", irq);
+	return data;
+}
+
+static void *of_pci_setup_dev(struct pci_dev *pci_dev)
+{
+	return of_pci_setup(&pci_dev->dev);
+}
+
+static void *of_pci_setup_host_bridge(struct pci_host_bridge *bridge)
+{
+	return of_pci_setup(bridge->dev.parent);
+}
+
+static void of_pci_cleanup(void *pmdata)
+{
+	struct of_pci_pm_data *data = pmdata;
+
+	if (IS_ERR_OR_NULL(data)) {
+		device_init_wakeup(data->dev, false);
+		dev_pm_clear_wake_irq(data->dev);
+	}
+}
+
+static bool of_pci_can_wakeup(void *pmdata)
+{
+	struct of_pci_pm_data *data = pmdata;
+
+	if (IS_ERR_OR_NULL(data))
+		return false;
+
+	return data->wakeup_irq > 0;
+}
+
+static int of_pci_dev_wakeup(void *pmdata, bool enable)
+{
+	struct of_pci_pm_data *data = pmdata;
+	struct device *dev = data->dev;
+	int ret;
+
+	if (!enable) {
+		dev_pm_clear_wake_irq(dev);
+		return device_set_wakeup_enable(dev, false);
+	}
+
+	ret = device_set_wakeup_enable(dev, true);
+	if (ret < 0)
+		return ret;
+
+	ret = dev_pm_set_dedicated_wake_irq(dev, data->wakeup_irq);
+	if (ret < 0) {
+		device_set_wakeup_enable(dev, false);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int of_pci_bridge_wakeup(void *pmdata, bool enable)
+{
+	struct of_pci_pm_data *data = pmdata;
+
+	if (enable && atomic_inc_return(&data->wakeup_cnt) != 1)
+		return 0;
+
+	if (!enable && atomic_dec_return(&data->wakeup_cnt) != 0)
+		return 0;
+
+	return of_pci_dev_wakeup(pmdata, enable);
+}
+
+static const struct pci_platform_pm_ops of_pci_platform_pm = {
+	.setup_dev		= of_pci_setup_dev,
+	.setup_host_bridge	= of_pci_setup_host_bridge,
+	.cleanup		= of_pci_cleanup,
+	.can_wakeup		= of_pci_can_wakeup,
+	.dev_wakeup		= of_pci_dev_wakeup,
+	.bridge_wakeup		= of_pci_bridge_wakeup,
+};
+
+static int __init of_pci_init(void)
+{
+	pci_set_platform_pm(&of_pci_platform_pm);
+	return 0;
+}
+arch_initcall(of_pci_init);
-- 
2.11.0

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Tony Lindgren]

* Jeffy Chen <jeffy.chen@rock-chips.com> [171026 06:31]:
> Add pci-of.c 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>

Thanks for doing this, looks good to me from wakeirq point of view:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[jeffy]

Hi Tony,

thanks for your Acked-by. but i'm moving the dedicated wakeirq setup to 
the setup(), so that the irq would be tracked in the /proc/interrupts.

please help to check the new v10 patches, thanks :)

On 10/26/2017 10:42 PM, Tony Lindgren wrote:
> * Jeffy Chen <jeffy.chen@rock-chips.com> [171026 06:31]:
>> Add pci-of.c 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>
>
> Thanks for doing this, looks good to me from wakeirq point of view:
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>
>
>

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Sinan Kaya]

On 10/26/2017 9:28 AM, Jeffy Chen wrote:
>  drivers/pci/Makefile |   2 +-
>  drivers/pci/pci-of.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 137 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pci-of.c
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 66a21acad952..4f76dbdb024c 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
>  
>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
>  
> -obj-$(CONFIG_OF) += of.o
> +obj-$(CONFIG_OF) += of.o pci-of.o

If the intention is to push this to pci directory, this code needs to be made
platform agnostic by splitting into two pieces.

I think you can make this code common by abstracting the IRQ number and
have some generic code like pci-wake.c in pci directory without the of prefix
in this file.

Then, you can have some other OF specific code in the drivers/of directory
that reads the IRQ from OF and calls the common code in PCI directory.


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[jeffy]

Hi Sinan,

Thanks for your reply.

On 10/26/2017 11:16 PM, Sinan Kaya wrote:
> If the intention is to push this to pci directory, this code needs to be made
> platform agnostic by splitting into two pieces.
>
> I think you can make this code common by abstracting the IRQ number and
> have some generic code like pci-wake.c in pci directory without the of prefix
> in this file.
>
> Then, you can have some other OF specific code in the drivers/of directory
> that reads the IRQ from OF and calls the common code in PCI directory.

right, that make sense to me...i'll wait for Brian & Bjorn & Rafael's 
opinions, then start rewriting these, thanks :)

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Brian Norris]

Hi Sinan,

I'm not sure I understand all your suggestions below.

On Thu, Oct 26, 2017 at 11:16:55AM -0400, Sinan Kaya wrote:
> On 10/26/2017 9:28 AM, Jeffy Chen wrote:
> >  drivers/pci/Makefile |   2 +-
> >  drivers/pci/pci-of.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 137 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/pci/pci-of.c
> > 
> > diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> > index 66a21acad952..4f76dbdb024c 100644
> > --- a/drivers/pci/Makefile
> > +++ b/drivers/pci/Makefile
> > @@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
> >  
> >  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
> >  
> > -obj-$(CONFIG_OF) += of.o
> > +obj-$(CONFIG_OF) += of.o pci-of.o
> 
> If the intention is to push this to pci directory, this code needs to be made
> platform agnostic by splitting into two pieces.
> 
> I think you can make this code common by abstracting the IRQ number and
> have some generic code like pci-wake.c in pci directory without the of prefix
> in this file.

Why would 'pci-wake' be a good name for this? We're doing basically the
same things that pci-acpi.c is doing (it also can configure the WAKE#
signal, via ACPI firmware calls).

> Then, you can have some other OF specific code in the drivers/of directory
> that reads the IRQ from OF and calls the common code in PCI directory.

Why is drivers/of/ special? I thought in general, the DT maintainers
preferred to move domain-specific stuff into the respective subsystems.

Also, the extraction is a very tiny piece of code, and the logic around
walking a PCI tree is the more important part. And in the
meantime...Jeffy has sent two more revisions of this patch set already,
and he did the latter (I like his abstraction of PCI device trees,
shared between ACPI and OF code) but not the former (it's still all
'pci-of.c').

Feel free to comment further on here or on v10, but at the moment I'm
not sure I understand yet how your suggestions would improve things.

Brian

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Sinan Kaya]

On 10/27/2017 8:04 PM, Brian Norris wrote:
>> I think you can make this code common by abstracting the IRQ number and
>> have some generic code like pci-wake.c in pci directory without the of prefix
>> in this file.
> Why would 'pci-wake' be a good name for this? We're doing basically the
> same things that pci-acpi.c is doing (it also can configure the WAKE#
> signal, via ACPI firmware calls).
> 

I was looking for some code that can be used between OF and ACPI rather than
duplicating the code in two different places just to extract some information
from the platform firmware. 

My initial look at your code suggested it to be very generic except the IRQ
read function. 

I was expecting that the kernel's PCI Wake handling to be common whether the IRQ
information is coming from DT/ACPI.

There are different approaches to portability.

* One approach is to keep of specific stuff in OF directory. 
* Another approach is keep some common code can call it from multiple platform firmware
or from the ARCH directory.
* Another approach is have one file but put OF and ACPI specific pieces at the bottom.

It depends on the code. 

Bjorn should chime in here with his preference.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Brian Norris]

On Sat, Oct 28, 2017 at 02:01:46PM -0400, Sinan Kaya wrote:
> I was looking for some code that can be used between OF and ACPI rather than
> duplicating the code in two different places just to extract some information
> from the platform firmware. 
> 
> My initial look at your code suggested it to be very generic except the IRQ
> read function. 
> 
> I was expecting that the kernel's PCI Wake handling to be common whether the IRQ
> information is coming from DT/ACPI.

I'm not an ACPI expert, but from reading the existing ACPI code handling
this wakeup configuration...they're not really that similar. Whereas for
device tree firmware there's not much magic to it -- the OS is really
just configuring a standard interrupt with wakeup capability -- ACPI
layers this all behind its GPE (General Purpose Event) abstraction,
which doesn't act like a typical interrupt. You manage GPE handlers with
a completely different set of APIs.

Anyway, Rafael seemed to have some of his own opinions (didn't like the
current approach it seems), so maybe we're all completely off track
here.

Regards,
Brian

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Brian Norris]

Hi Jeffy,

On Thu, Oct 26, 2017 at 09:28:40PM +0800, Jeffy Chen wrote:
> Add pci-of.c 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 v8:
> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
> 
> Changes in v7:
> Move PCIE_WAKE handling into pci core.
> 
> Changes in v6:
> Fix device_init_wake error handling, and add some comments.
> 
> Changes in v5:
> Rebase.
> 
> Changes in v3:
> Fix error handling.
> 
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq.
> 
>  drivers/pci/Makefile |   2 +-
>  drivers/pci/pci-of.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 137 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pci-of.c
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 66a21acad952..4f76dbdb024c 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
>  
>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
>  
> -obj-$(CONFIG_OF) += of.o
> +obj-$(CONFIG_OF) += of.o pci-of.o
>  
>  ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>  
> diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
> new file mode 100644
> index 000000000000..55a33206fc84
> --- /dev/null
> +++ b/drivers/pci/pci-of.c
> @@ -0,0 +1,136 @@
> +/*
> + * OF PCI PM support
> + *
> + * Copyright (c) 2017 Rockchip, Inc.
> + *
> + * Author: Jeffy Chen <jeffy.chen@rock-chips.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/pm_wakeirq.h>
> +#include "pci.h"
> +
> +struct of_pci_pm_data {
> +	struct device	*dev;
> +	unsigned int	wakeup_irq;
> +	atomic_t	wakeup_cnt;
> +};
> +
> +static void *of_pci_setup(struct device *dev)
> +{
> +	struct of_pci_pm_data *data;
> +	int irq;
> +
> +	if (!dev->of_node)
> +		return NULL;
> +
> +	data = devm_kzalloc(dev, sizeof(struct of_pci_pm_data), GFP_KERNEL);
> +	if (!data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	irq = of_irq_get_byname(dev->of_node, "wakeup");
> +	if (irq < 0) {
> +		if (irq == -EPROBE_DEFER)
> +			return ERR_PTR(irq);
> +
> +		return NULL;
> +	}
> +
> +	data->wakeup_irq = irq;
> +	data->dev = dev;
> +
> +	device_init_wakeup(dev, false);
> +
> +	dev_info(dev, "Wakeup IRQ %d\n", irq);
> +	return data;
> +}
> +
> +static void *of_pci_setup_dev(struct pci_dev *pci_dev)
> +{
> +	return of_pci_setup(&pci_dev->dev);
> +}
> +
> +static void *of_pci_setup_host_bridge(struct pci_host_bridge *bridge)
> +{
> +	return of_pci_setup(bridge->dev.parent);
> +}
> +
> +static void of_pci_cleanup(void *pmdata)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (IS_ERR_OR_NULL(data)) {
> +		device_init_wakeup(data->dev, false);
> +		dev_pm_clear_wake_irq(data->dev);
> +	}
> +}
> +
> +static bool of_pci_can_wakeup(void *pmdata)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (IS_ERR_OR_NULL(data))
> +		return false;
> +
> +	return data->wakeup_irq > 0;
> +}
> +
> +static int of_pci_dev_wakeup(void *pmdata, bool enable)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +	struct device *dev = data->dev;
> +	int ret;
> +
> +	if (!enable) {
> +		dev_pm_clear_wake_irq(dev);
> +		return device_set_wakeup_enable(dev, false);
> +	}
> +
> +	ret = device_set_wakeup_enable(dev, true);
> +	if (ret < 0)
> +		return ret;

One reason this series is failing for me: the above is failing with
-EINVAL -- it seems like no one set the 'can_wakeup' flag for the
Marvell Wifi card I'm using. It seems like we probably *should* be
calling device_set_wakeup_capable() from your new setup method, to say
that we're capable of wakeup. The PCI PME code does this already, which
seems to make sense. There are also some network drivers that do it too
(e.g., ath10k), but not all.

Brian

> +
> +	ret = dev_pm_set_dedicated_wake_irq(dev, data->wakeup_irq);
> +	if (ret < 0) {
> +		device_set_wakeup_enable(dev, false);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int of_pci_bridge_wakeup(void *pmdata, bool enable)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (enable && atomic_inc_return(&data->wakeup_cnt) != 1)
> +		return 0;
> +
> +	if (!enable && atomic_dec_return(&data->wakeup_cnt) != 0)
> +		return 0;
> +
> +	return of_pci_dev_wakeup(pmdata, enable);
> +}
> +
> +static const struct pci_platform_pm_ops of_pci_platform_pm = {
> +	.setup_dev		= of_pci_setup_dev,
> +	.setup_host_bridge	= of_pci_setup_host_bridge,
> +	.cleanup		= of_pci_cleanup,
> +	.can_wakeup		= of_pci_can_wakeup,
> +	.dev_wakeup		= of_pci_dev_wakeup,
> +	.bridge_wakeup		= of_pci_bridge_wakeup,
> +};
> +
> +static int __init of_pci_init(void)
> +{
> +	pci_set_platform_pm(&of_pci_platform_pm);
> +	return 0;
> +}
> +arch_initcall(of_pci_init);
> -- 
> 2.11.0
> 
> 

Re: [RFC PATCH v8 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[jeffy]

Hi Brian,

On 10/27/2017 01:55 PM, Brian Norris wrote:
> One reason this series is failing for me: the above is failing with
> -EINVAL -- it seems like no one set the 'can_wakeup' flag for the
> Marvell Wifi card I'm using. It seems like we probably*should*  be
> calling device_set_wakeup_capable() from your new setup method, to say
> that we're capable of wakeup. The PCI PME code does this already, which
> seems to make sense. There are also some network drivers that do it too
> (e.g., ath10k), but not all.
>
right, thanks for noticing, will fix it in the new version...

and Sinan suggested to separate of code and wakeirq handling, would it 
make sense to rewrite this as pci-wakeirq.c(platform_pm_ops based on 
irq), and move of parse code into drivers/of/(maybe into 
of_irq_parse_pci()'s PCI interrupt parsing code)?

> Brian
>