[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

[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 3/7] arm64: dts: rockchip: Handle PCIe WAKE# signal in pcie driver for Gru

[Jeffy Chen]

Currently we are handling PCIe WAKE# signal in mrvl wifi driver.
Move it to rockchip pcie driver for Gru boards.

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 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 | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 5772c52fbfd3..8e37da69f693 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -708,7 +708,15 @@ ap_i2c_audio: &i2c8 {
 
 	ep-gpios = <&gpio2 27 GPIO_ACTIVE_HIGH>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie_clkreqn_cpm>, <&wifi_perst_l>;
+	pinctrl-0 = <&pcie_clkreqn_cpm>, <&wlan_host_wake_l>, <&wifi_perst_l>;
+
+	interrupts-extended = <&gic GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
+			      <&gic GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
+			      <&gic GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>,
+			      <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
+	interrupt-names = "sys", "legacy", "client", "wakeup";
+	/delete-property/ interrupts;
+
 	vpcie3v3-supply = <&pp3300_wifi_bt>;
 	vpcie1v8-supply = <&wlan_pd_n>; /* HACK: see &wlan_pd_n */
 	vpcie0v9-supply = <&pp900_pcie>;
@@ -723,11 +731,6 @@ ap_i2c_audio: &i2c8 {
 			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

[RFC PATCH v8 4/7] of/irq: Adjust of pci irq parsing for multiple interrupts

[Jeffy Chen]

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

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

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

Changes in 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index 3a05568f65df..8b69211f0b88 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -27,7 +27,18 @@ 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);
+		struct property *prop;
+		const char *name;
+		int index = 0;
+
+		prop = of_find_property(dn, "interrupt-names", NULL);
+		for (name = of_prop_next_string(prop, NULL); name;
+		     name = of_prop_next_string(prop, name), index++) {
+			if (!strcmp(name, "pci"))
+				break;
+		}
+
+		rc = of_irq_parse_one(dn, index, out_irq);
 		if (!rc)
 			return rc;
 	}
-- 
2.11.0

Re: [RFC PATCH v8 4/7] of/irq: Adjust of pci irq parsing for multiple interrupts

[Rob Herring]

On Thu, Oct 26, 2017 at 8:28 AM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> 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".

Why do you need this patch? You're moving the wakeup handling from the
PCI device to the bridge. The bridge device is not PCI interrupts, but
a platform device so this function doesn't matter.

Rob

Re: [RFC PATCH v8 4/7] of/irq: Adjust of pci irq parsing for multiple interrupts

[jeffy]

Hi Rob,

On 10/27/2017 04:02 AM, Rob Herring wrote:
> Why do you need this patch? You're moving the wakeup handling from the
> PCI device to the bridge. The bridge device is not PCI interrupts, but
> a platform device so this function doesn't matter.
>

because it's possible we have multiple PCI devices with individual WAKE# 
interrupt.

so Brian suggested we may need to support both of PCI device wakeup 
handling and the bridge wakeup handling in the pci core(as the ACPI) :)

> Rob
>
>
>

Re: [RFC PATCH v8 4/7] of/irq: Adjust of pci irq parsing for multiple interrupts

[Rob Herring]

On Thu, Oct 26, 2017 at 09:28:37PM +0800, Jeffy Chen wrote:
> Currently we are considering the first irq as the PCI interrupt pin,
> but a pci device may have multiple interrupts(e.g. PCIe WAKE# pin).
> 
> Only parse the PCI interrupt pin when the irq is unnamed or named as
> "pci".
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in 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 | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> index 3a05568f65df..8b69211f0b88 100644
> --- a/drivers/of/of_pci_irq.c
> +++ b/drivers/of/of_pci_irq.c
> @@ -27,7 +27,18 @@ 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);
> +		struct property *prop;
> +		const char *name;
> +		int index = 0;
> +
> +		prop = of_find_property(dn, "interrupt-names", NULL);
> +		for (name = of_prop_next_string(prop, NULL); name;
> +		     name = of_prop_next_string(prop, name), index++) {
> +			if (!strcmp(name, "pci"))
> +				break;

Use of_property_match_string

> +		}
> +
> +		rc = of_irq_parse_one(dn, index, out_irq);
>  		if (!rc)
>  			return rc;
>  	}
> -- 
> 2.11.0
> 
> 

Re: [RFC PATCH v8 4/7] of/irq: Adjust of pci irq parsing for multiple interrupts

[jeffy]

Hi Rob,

On 10/27/2017 10:38 PM, Rob Herring wrote:
>> +		prop = of_find_property(dn, "interrupt-names", NULL);
>> >+		for (name = of_prop_next_string(prop, NULL); name;
>> >+		     name = of_prop_next_string(prop, name), index++) {
>> >+			if (!strcmp(name, "pci"))
>> >+				break;
> Use of_property_match_string
>
i'm trying to find the first unnamed or "pci" named irq in the string 
array, so cannot use that API:)

but will change to of_property_for_each_string as Brian suggested.

Re: [RFC PATCH v8 4/7] of/irq: Adjust of pci irq parsing for multiple interrupts

[Brian Norris]

On Mon, Oct 30, 2017 at 10:05:51AM +0800, Jeffy Chen wrote:
> On 10/27/2017 10:38 PM, Rob Herring wrote:
> >>+		prop = of_find_property(dn, "interrupt-names", NULL);
> >>>+		for (name = of_prop_next_string(prop, NULL); name;
> >>>+		     name = of_prop_next_string(prop, name), index++) {
> >>>+			if (!strcmp(name, "pci"))
> >>>+				break;
> >Use of_property_match_string
> >
> i'm trying to find the first unnamed or "pci" named irq in the
> string array, so cannot use that API:)

Actually, why can't you? It does exactly what your loop does right now.
You'd just need to handle the -ENODATA case better. e.g., check if we
have an interrupt-names property -- if not, just take index 0; if so,
then use of_property_match_string().

Brian

> but will change to of_property_for_each_string as Brian suggested.
>