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

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

[Jeffy Chen]

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

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


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

Changes in v11:
Only add irq definitions for PCI devices and rewrite the commit message.
Address Brian's comments.
Only support 1-per-device PCIe WAKE# pin as suggested.
Move to pcie port as Brian suggested.

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

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

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

Changes in v7:
Move PCIE_WAKE handling into pci core.

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

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

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (5):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  mwifiex: Disable wakeup irq handling for pcie
  PCI / PM: Add support for the PCIe WAKE# signal for OF
  arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru

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

-- 
2.11.0

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

[Jeffy Chen]

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

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

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

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

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

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

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

Changes in v3: None
Changes in v2: None

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

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index c77981c5dd18..3045ac452f27 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,13 @@ driver implementation may support the following properties:
    unsupported link speed, for instance, trying to do training for
    unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
    for gen2, and '1' for gen1. Any other values are invalid.
+
+PCI devices may support the following properties:
+
+- interrupts: Interrupt specifier for each name in interrupt-names.
+- interrupt-names:
+    May contain "wakeup" for PCIe WAKE# interrupt and "pci" for PCI interrupt.
+    The PCI devices may optionally include an 'interrupts' property that
+    represents the legacy PCI interrupt. And when we try to specify the PCIe
+    WAKE# pin, a corresponding 'interrupt-names' property is required to
+    distinguish them.
-- 
2.11.0

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

[Jeffy Chen]

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

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

Signed-off-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

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

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

 drivers/of/of_pci_irq.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

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


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

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

[Jeffy Chen]

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

Also use the dedicated wakeirq infrastructure to simplify it.

And add pci-of.c to enable/disable the wakeup irq in noirq stage to
avoid possible irq storm.

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

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

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

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

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

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

Changes in v7:
Move PCIE_WAKE handling into pci core.

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

Changes in v5:
Rebase.

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

 drivers/of/of_pci_irq.c  | 52 +++++++++++++++++++++++++++++++++
 drivers/pci/Makefile     |  1 +
 drivers/pci/pci-driver.c | 10 +++++++
 drivers/pci/pci-of.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pci.h   |  9 ++++++
 5 files changed, 147 insertions(+)
 create mode 100644 drivers/pci/pci-of.c

diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index d39565d5477b..af7afe6cbce2 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -1,8 +1,60 @@
 #include <linux/kernel.h>
 #include <linux/of_pci.h>
 #include <linux/of_irq.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/export.h>
 
+int of_pci_setup_wake_irq(struct pci_dev *pdev)
+{
+	struct pci_dev *ppdev;
+	struct device_node *dn;
+	int ret, irq;
+
+	/* Get the pci_dev of our parent. Hopefully it's a port. */
+	ppdev = pdev->bus->self;
+	/* Nope, it's a host bridge. */
+	if (!ppdev)
+		return 0;
+
+	dn = pci_device_to_OF_node(ppdev);
+	if (!dn)
+		return 0;
+
+	irq = of_irq_get_byname(dn, "wakeup");
+	if (irq == -EPROBE_DEFER) {
+		return irq;
+	} else if (irq < 0) {
+		/* Ignore other errors, since a missing wakeup is non-fatal. */
+		dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
+		return 0;
+	}
+
+	device_init_wakeup(&pdev->dev, true);
+
+	ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, irq);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to set wake IRQ: %d\n", ret);
+		device_init_wakeup(&pdev->dev, false);
+		return ret;
+	}
+
+	/* Start out disabled to avoid irq storm */
+	dev_pm_disable_wake_irq(&pdev->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
+
+void of_pci_teardown_wake_irq(struct pci_dev *pdev)
+{
+	if (!pdev->dev.power.wakeirq)
+		return;
+
+	dev_pm_clear_wake_irq(&pdev->dev);
+	device_init_wakeup(&pdev->dev, false);
+}
+EXPORT_SYMBOL_GPL(of_pci_teardown_wake_irq);
+
 /**
  * of_irq_parse_pci - Resolve the interrupt for a PCI device
  * @pdev:       the device whose interrupt is to be resolved
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index c7819b973df7..d0182c82162a 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
 # ACPI _DSM provided firmware instance and string name
 #
 obj-$(CONFIG_ACPI)    += pci-acpi.o
+obj-$(CONFIG_OF)      += pci-of.o
 
 # SMBIOS provided firmware instance and labels
 obj-$(CONFIG_PCI_LABEL) += pci-label.o
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index d79dbc377b9c..b4475ff35d97 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/cpu.h>
+#include <linux/of_pci.h>
 #include <linux/pm_runtime.h>
 #include <linux/suspend.h>
 #include <linux/kexec.h>
@@ -421,10 +422,17 @@ static int pci_device_probe(struct device *dev)
 	if (error < 0)
 		return error;
 
+	error = of_pci_setup_wake_irq(pci_dev);
+	if (error < 0) {
+		pcibios_free_irq(pci_dev);
+		return error;
+	}
+
 	pci_dev_get(pci_dev);
 	if (pci_device_can_probe(pci_dev)) {
 		error = __pci_device_probe(drv, pci_dev);
 		if (error) {
+			of_pci_teardown_wake_irq(pci_dev);
 			pcibios_free_irq(pci_dev);
 			pci_dev_put(pci_dev);
 		}
@@ -438,6 +446,8 @@ static int pci_device_remove(struct device *dev)
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct pci_driver *drv = pci_dev->driver;
 
+	of_pci_teardown_wake_irq(pci_dev);
+
 	if (drv) {
 		if (drv->remove) {
 			pm_runtime_get_sync(dev);
diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
new file mode 100644
index 000000000000..ad413b2de508
--- /dev/null
+++ b/drivers/pci/pci-of.c
@@ -0,0 +1,75 @@
+/*
+ * File:	pci-of.c
+ * Purpose:	Provide PCI PM/wakeup support in OF systems
+ *
+ * Copyright (C) 2017 Google, Inc.
+ *	Brian Norris <briannorris@chromium.org>
+ *
+ * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
+ *	Jeffy Chen <jeffy.chen@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pm_wakeirq.h>
+#include "pci.h"
+
+static bool of_pci_power_manageable(struct pci_dev *dev)
+{
+	return false;
+}
+
+static int of_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
+{
+	return -ENOSYS;
+}
+
+static pci_power_t of_pci_get_power_state(struct pci_dev *dev)
+{
+	return PCI_UNKNOWN;
+}
+
+static pci_power_t of_pci_choose_state(struct pci_dev *pdev)
+{
+	return PCI_POWER_ERROR;
+}
+
+static int of_pci_wakeup(struct pci_dev *dev, bool enable)
+{
+	if (enable)
+		dev_pm_enable_wake_irq(&dev->dev);
+	else
+		dev_pm_disable_wake_irq(&dev->dev);
+	return 0;
+}
+
+static bool of_pci_need_resume(struct pci_dev *dev)
+{
+	return false;
+}
+
+static const struct pci_platform_pm_ops of_pci_platform_pm = {
+	.is_manageable = of_pci_power_manageable,
+	.set_state = of_pci_set_power_state,
+	.get_state = of_pci_get_power_state,
+	.choose_state = of_pci_choose_state,
+	.set_wakeup = of_pci_wakeup,
+	.need_resume = of_pci_need_resume,
+};
+
+static int __init of_pci_init(void)
+{
+	if (!acpi_disabled)
+		return 0;
+
+	pci_set_platform_pm(&of_pci_platform_pm);
+
+	return 0;
+}
+arch_initcall(of_pci_init);
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index bf588a05d0d0..80fe8ae3393e 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -10,6 +10,8 @@ struct of_phandle_args;
 struct device_node;
 
 #ifdef CONFIG_OF_PCI
+int of_pci_setup_wake_irq(struct pci_dev *pdev);
+void of_pci_teardown_wake_irq(struct pci_dev *pdev);
 int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq);
 struct device_node *of_pci_find_child_device(struct device_node *parent,
 					     unsigned int devfn);
@@ -23,6 +25,13 @@ int of_pci_map_rid(struct device_node *np, u32 rid,
 		   const char *map_name, const char *map_mask_name,
 		   struct device_node **target, u32 *id_out);
 #else
+static inline int of_pci_setup_wake_irq(struct pci_dev *pdev)
+{
+	return 0;
+}
+
+static void of_pci_teardown_wake_irq(struct pci_dev *pdev) { };
+
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {
 	return 0;
-- 
2.11.0

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

[Jeffy Chen]

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

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

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

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

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

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

Changes in v8:
Rewrite the commit message.

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

Changes in v3: None
Changes in v2: None

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

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

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

[Tony Lindgren]

* Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:11]:
> We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
> so add definitions of the optional PCIe WAKE# pin for PCI devices.
> 
> Also add an definition of the optional PCI interrupt pin for PCI
> devices to distinguish it from the PCIe WAKE# pin.

> --- a/Documentation/devicetree/bindings/pci/pci.txt
> +++ b/Documentation/devicetree/bindings/pci/pci.txt
> @@ -24,3 +24,13 @@ driver implementation may support the following properties:
>     unsupported link speed, for instance, trying to do training for
>     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
>     for gen2, and '1' for gen1. Any other values are invalid.
> +
> +PCI devices may support the following properties:

This should say PCI ports instead of PCI devices.

> +- interrupts: Interrupt specifier for each name in interrupt-names.
> +- interrupt-names:
> +    May contain "wakeup" for PCIe WAKE# interrupt and "pci" for PCI interrupt.
> +    The PCI devices may optionally include an 'interrupts' property that
> +    represents the legacy PCI interrupt. And when we try to specify the PCIe
> +    WAKE# pin, a corresponding 'interrupt-names' property is required to
> +    distinguish them.
> -- 
> 2.11.0
> 
> 

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

[Rafael J. Wysocki]

On Fri, Dec 29, 2017 at 6:57 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:11]:
>> We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
>> so add definitions of the optional PCIe WAKE# pin for PCI devices.
>>
>> Also add an definition of the optional PCI interrupt pin for PCI
>> devices to distinguish it from the PCIe WAKE# pin.
>
>> --- a/Documentation/devicetree/bindings/pci/pci.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci.txt
>> @@ -24,3 +24,13 @@ driver implementation may support the following properties:
>>     unsupported link speed, for instance, trying to do training for
>>     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
>>     for gen2, and '1' for gen1. Any other values are invalid.
>> +
>> +PCI devices may support the following properties:
>
> This should say PCI ports instead of PCI devices.

No, it is more accurate to say "PCI devices".

Well, it actually gets somewhat confusing, because in the PCI
terminology a "PCI device" means a physical piece of hardware that can
be put into a single "slot" (think socket on a board) and may consist
up to 8 functional units called "functions" which are each represented
by a struct pci_dev.  So there may be up to 8 struct pci_dev objects
per "PCI device" (as per the standard language) and, BTW, drivers bind
to functions (via the struct pci_dev objects).

Now, WAKE# is shared by all functions within the same "PCI device"
(I'm not sure if the standard specifies that directly, but at least it
appears to be treated as an obvious physical limitation), so it may be
useful to represent the "slot" or "device" level in the DT even though
it has no struct device based representation in the kernel.

>> +- interrupts: Interrupt specifier for each name in interrupt-names.
>> +- interrupt-names:
>> +    May contain "wakeup" for PCIe WAKE# interrupt and "pci" for PCI interrupt.
>> +    The PCI devices may optionally include an 'interrupts' property that
>> +    represents the legacy PCI interrupt. And when we try to specify the PCIe
>> +    WAKE# pin, a corresponding 'interrupt-names' property is required to
>> +    distinguish them.
>> --
>> 2.11.0
>>
>>

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

[Rafael J. Wysocki]

On Sat, Dec 30, 2017 at 12:50 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Dec 29, 2017 at 6:57 PM, Tony Lindgren <tony@atomide.com> wrote:
>> * Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:11]:
>>> We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
>>> so add definitions of the optional PCIe WAKE# pin for PCI devices.
>>>
>>> Also add an definition of the optional PCI interrupt pin for PCI
>>> devices to distinguish it from the PCIe WAKE# pin.
>>
>>> --- a/Documentation/devicetree/bindings/pci/pci.txt
>>> +++ b/Documentation/devicetree/bindings/pci/pci.txt
>>> @@ -24,3 +24,13 @@ driver implementation may support the following properties:
>>>     unsupported link speed, for instance, trying to do training for
>>>     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
>>>     for gen2, and '1' for gen1. Any other values are invalid.
>>> +
>>> +PCI devices may support the following properties:
>>
>> This should say PCI ports instead of PCI devices.
>
> No, it is more accurate to say "PCI devices".
>
> Well, it actually gets somewhat confusing, because in the PCI
> terminology a "PCI device" means a physical piece of hardware that can
> be put into a single "slot" (think socket on a board) and may consist
> up to 8 functional units called "functions" which are each represented
> by a struct pci_dev.  So there may be up to 8 struct pci_dev objects
> per "PCI device" (as per the standard language) and, BTW, drivers bind
> to functions (via the struct pci_dev objects).
>
> Now, WAKE# is shared by all functions within the same "PCI device"
> (I'm not sure if the standard specifies that directly, but at least it
> appears to be treated as an obvious physical limitation), so it may be
> useful to represent the "slot" or "device" level in the DT even though
> it has no struct device based representation in the kernel.

Within the convention that bridges represent "everything below them"
as far as WAKE# is concerned, it can say "The following properties may
be provided for PCI bridges:" and the description below should explain
the convention.

Thanks,
Rafael

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

[Tony Lindgren]

* Rafael J. Wysocki <rafael@kernel.org> [171229 23:52]:
> On Fri, Dec 29, 2017 at 6:57 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:11]:
> >> We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
> >> so add definitions of the optional PCIe WAKE# pin for PCI devices.
> >>
> >> Also add an definition of the optional PCI interrupt pin for PCI
> >> devices to distinguish it from the PCIe WAKE# pin.
> >
> >> --- a/Documentation/devicetree/bindings/pci/pci.txt
> >> +++ b/Documentation/devicetree/bindings/pci/pci.txt
> >> @@ -24,3 +24,13 @@ driver implementation may support the following properties:
> >>     unsupported link speed, for instance, trying to do training for
> >>     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
> >>     for gen2, and '1' for gen1. Any other values are invalid.
> >> +
> >> +PCI devices may support the following properties:
> >
> > This should say PCI ports instead of PCI devices.
> 
> No, it is more accurate to say "PCI devices".
> 
> Well, it actually gets somewhat confusing, because in the PCI
> terminology a "PCI device" means a physical piece of hardware that can
> be put into a single "slot" (think socket on a board) and may consist
> up to 8 functional units called "functions" which are each represented
> by a struct pci_dev.  So there may be up to 8 struct pci_dev objects
> per "PCI device" (as per the standard language) and, BTW, drivers bind
> to functions (via the struct pci_dev objects).

OK then let's stick to "PCI devices".

> Now, WAKE# is shared by all functions within the same "PCI device"
> (I'm not sure if the standard specifies that directly, but at least it
> appears to be treated as an obvious physical limitation), so it may be
> useful to represent the "slot" or "device" level in the DT even though
> it has no struct device based representation in the kernel.

Yeah that makes sense as there is only one WAKE# line in the slot :)

Regards,

Tony

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

[Tony Lindgren]

* Rafael J. Wysocki <rafael@kernel.org> [171230 00:34]:
> On Sat, Dec 30, 2017 at 12:50 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Fri, Dec 29, 2017 at 6:57 PM, Tony Lindgren <tony@atomide.com> wrote:
> >> * Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:11]:
> >>> We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
> >>> so add definitions of the optional PCIe WAKE# pin for PCI devices.
> >>>
> >>> Also add an definition of the optional PCI interrupt pin for PCI
> >>> devices to distinguish it from the PCIe WAKE# pin.
> >>
> >>> --- a/Documentation/devicetree/bindings/pci/pci.txt
> >>> +++ b/Documentation/devicetree/bindings/pci/pci.txt
> >>> @@ -24,3 +24,13 @@ driver implementation may support the following properties:
> >>>     unsupported link speed, for instance, trying to do training for
> >>>     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
> >>>     for gen2, and '1' for gen1. Any other values are invalid.
> >>> +
> >>> +PCI devices may support the following properties:
> >>
> >> This should say PCI ports instead of PCI devices.
> >
> > No, it is more accurate to say "PCI devices".
> >
> > Well, it actually gets somewhat confusing, because in the PCI
> > terminology a "PCI device" means a physical piece of hardware that can
> > be put into a single "slot" (think socket on a board) and may consist
> > up to 8 functional units called "functions" which are each represented
> > by a struct pci_dev.  So there may be up to 8 struct pci_dev objects
> > per "PCI device" (as per the standard language) and, BTW, drivers bind
> > to functions (via the struct pci_dev objects).
> >
> > Now, WAKE# is shared by all functions within the same "PCI device"
> > (I'm not sure if the standard specifies that directly, but at least it
> > appears to be treated as an obvious physical limitation), so it may be
> > useful to represent the "slot" or "device" level in the DT even though
> > it has no struct device based representation in the kernel.
> 
> Within the convention that bridges represent "everything below them"
> as far as WAKE# is concerned, it can say "The following properties may
> be provided for PCI bridges:" and the description below should explain
> the convention.

Sounds good to me.

Regards,

Tony

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

[Rafael J. Wysocki]

On Fri, Dec 29, 2017 at 6:55 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:41]:
>> Currently we are handling PCIe WAKE# irq in mrvl wifi driver.
>>
>> Move it to rockchip pcie port since we are going to handle it in the
>> pci core.
>
> Yes in the PCIe case, the pcie port node is the right place for
> the wakeirq instead of the child the mvl_wifi node. So one
> question further down below to verify this..

You seem to be using a convention by which the port represents the
whole "slot" or "PCI device" (as an entity consisting of up to 8
functions) connected to it.

That is fair enough as long as the port is not the top of a more
complex branch of the PCIe hierarchy, so maybe that case needs to be
made special somehow?

Also, I would document the convention by mentioning that the wakeup
signaled via that interrupt doesn't apply to the port itself, but to
the functions (endpoints) below it.

>> Also avoid this irq been considered as the PCI interrupt pin in the
>> of_irq_parse_pci().
>
> The above paragraph needs a bit more clarification to be
> readable :)
>
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> @@ -719,15 +719,16 @@ ap_i2c_audio: &i2c8 {
>>               #size-cells = <2>;
>>               ranges;
>>
>> +             interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
>> +             interrupt-names = "pci", "wakeup";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&wlan_host_wake_l>;
>> +             wakeup-source;
>> +
>>               mvl_wifi: wifi@0,0 {
>>                       compatible = "pci1b4b,2b42";
>>                       reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>>                              0x83010000 0x0 0x00100000 0x0 0x00100000>;
>> -                     interrupt-parent = <&gpio0>;
>> -                     interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>> -                     pinctrl-names = "default";
>> -                     pinctrl-0 = <&wlan_host_wake_l>;
>> -                     wakeup-source;
>>               };
>>       };
>>  };
>
> So the above modifies pcie@0,0 node. And that node describes
> the particular PCIe port that the WLAN is connected to instead
> of describing the whole PCIe controller device, right?
>
> If so, then yeah it's totally where the wakeirq should be
> defined for a PCIe device in the dts file :)

As long as the convention used here is clear to everybody, that is.

Thanks,
Rafael

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

[Tony Lindgren]

* Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> [171226 02:41]:
> Currently we are handling PCIe WAKE# irq in mrvl wifi driver.
> 
> Move it to rockchip pcie port since we are going to handle it in the
> pci core.

Yes in the PCIe case, the pcie port node is the right place for
the wakeirq instead of the child the mvl_wifi node. So one
question further down below to verify this..

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

The above paragraph needs a bit more clarification to be
readable :)

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

So the above modifies pcie@0,0 node. And that node describes
the particular PCIe port that the WLAN is connected to instead
of describing the whole PCIe controller device, right?

If so, then yeah it's totally where the wakeirq should be
defined for a PCIe device in the dts file :)

Regards,

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

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

[Jeffy Chen]

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

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

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

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

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

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

Changes in v8:
Rewrite the commit message.

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

Changes in v3: None
Changes in v2: None

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

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