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

[RFC PATCH v10 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 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.
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.

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
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  mwifiex: Disable wakeup irq handling for pcie
  arm64: dts: rockchip: Move PCIe WAKE# irq to pcie driver for Gru
  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 |   8 ++
 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                          | 127 ++++++++++++++++++++++++++
 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, 361 insertions(+), 97 deletions(-)
 create mode 100644 drivers/pci/pci-of.c

-- 
2.11.0

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

[Jeffy Chen]

We are going to handle PCIe WAKE# pin for PCI bus bridges and PCI
devices in the pci core, so add definitions of the optional PCIe
WAKE# pin for PCI bus bridges and 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 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index c77981c5dd18..d4406d4e15ad 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,11 @@ 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.
+
+PCI devices have standardized Device Tree bindings:
+
+- 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 v10 1/7] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq

[Brian Norris]

Hi,

On Fri, Oct 27, 2017 at 03:26:06PM +0800, Jeffy Chen wrote:
> We are going to handle PCIe WAKE# pin for PCI bus bridges and PCI
> devices in the pci core, so add definitions of the optional PCIe
> WAKE# pin for PCI bus bridges and 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 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 | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
> index c77981c5dd18..d4406d4e15ad 100644
> --- a/Documentation/devicetree/bindings/pci/pci.txt
> +++ b/Documentation/devicetree/bindings/pci/pci.txt
> @@ -24,3 +24,11 @@ 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.

s/contains/contain/

> +
> +PCI devices have standardized Device Tree bindings:

This line is a little unclear, especially since there *is* an old
documented standard, yet the following text is actually introducing new,
non-standard additions.

> +
> +- interrupts: Interrupt specifier for each name in interrupt-names.
> +- interrupt-names: May contains "wakeup" for PCIe WAKE# interrupt and "pci" for

s/contains/contain/

> +  PCI interrupt.

IMO, since you're trying to augment a standardized binding, you need to
be a lot clearer here. I expect you should mention the existing standard
(that devices may optionally include an 'interrupts' property that
represents the legacy PCI interrupt) and how you're augmenting it (that
additional interrupts can be supported optionally, but they require a
corresponding 'interrupt-names' property).

Also, is this binding only applying either to a host bridge or to
devices? No intermediate bridges or ports? It seems so, but I wanted to
be clear. (And it probably could be extended if needed. Notably, ACPI
has a tree-walk implementation, so if the device itself doesn't have
a wakeup config, it can look into any of its parents.)

Once you fix up the documentation...I suppose this looks like a sane
idea. But I'd like 2nd opinions on this.

Brian

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

[Rob Herring]

On Fri, Oct 27, 2017 at 01:45:17PM -0700, Brian Norris wrote:
> Hi,
> 
> On Fri, Oct 27, 2017 at 03:26:06PM +0800, Jeffy Chen wrote:
> > We are going to handle PCIe WAKE# pin for PCI bus bridges and PCI
> > devices in the pci core, so add definitions of the optional PCIe
> > WAKE# pin for PCI bus bridges and 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 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 | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
> > index c77981c5dd18..d4406d4e15ad 100644
> > --- a/Documentation/devicetree/bindings/pci/pci.txt
> > +++ b/Documentation/devicetree/bindings/pci/pci.txt
> > @@ -24,3 +24,11 @@ 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.
> 
> s/contains/contain/
> 
> > +
> > +PCI devices have standardized Device Tree bindings:
> 
> This line is a little unclear, especially since there *is* an old
> documented standard, yet the following text is actually introducing new,
> non-standard additions.
> 
> > +
> > +- interrupts: Interrupt specifier for each name in interrupt-names.
> > +- interrupt-names: May contains "wakeup" for PCIe WAKE# interrupt and "pci" for
> 
> s/contains/contain/
> 
> > +  PCI interrupt.
> 
> IMO, since you're trying to augment a standardized binding, you need to
> be a lot clearer here. I expect you should mention the existing standard
> (that devices may optionally include an 'interrupts' property that
> represents the legacy PCI interrupt) and how you're augmenting it (that
> additional interrupts can be supported optionally, but they require a
> corresponding 'interrupt-names' property).

There's an additional complication that I'd guess the wakeup is 
typically a GPIO line and hence a different parent. We have 2 options 
there. The first is interrupts-extended which is generally implicitly 
supported (i.e. we only document interrupts). The second is we already 
have interrupt-map if we have legacy interrupts and can map to different 
parents. For this to work, we'd have to use a number >4 for the wakeup 
interrupts.

Rob

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

[Tony Lindgren]

* Rob Herring <robh@kernel.org> [171101 21:07]:
> On Fri, Oct 27, 2017 at 01:45:17PM -0700, Brian Norris wrote:
> > IMO, since you're trying to augment a standardized binding, you need to
> > be a lot clearer here. I expect you should mention the existing standard
> > (that devices may optionally include an 'interrupts' property that
> > represents the legacy PCI interrupt) and how you're augmenting it (that
> > additional interrupts can be supported optionally, but they require a
> > corresponding 'interrupt-names' property).
> 
> There's an additional complication that I'd guess the wakeup is 
> typically a GPIO line and hence a different parent. We have 2 options 
> there. The first is interrupts-extended which is generally implicitly 
> supported (i.e. we only document interrupts). The second is we already 
> have interrupt-map if we have legacy interrupts and can map to different 
> parents. For this to work, we'd have to use a number >4 for the wakeup 
> interrupts.

The wakeup interrupt can also be a separate always on interrupt
controller in addition to GPIOs. Anyways, the interrupts-extended
binding works well for these. And the interrupt-names we seem
to have standardized on are "irq" and "wakeup".

Regards,

Tony

[RFC PATCH v10 2/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 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 | 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 v10 2/7] of/irq: Adjust of_pci_irq parsing for multiple interrupts

[Brian Norris]

Hi Jeffy,

On Fri, Oct 27, 2017 at 03:26:07PM +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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> 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 | 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;
> +		}

You seem to have expanded of_property_for_each_string(). You could also
do this more clearly with:

		of_property_for_each_string(dn, "interrupt-names", prop, name) {
			if (!strcmp(name, "pci"))
				break;
			index++;
		}

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

Suppose you provide a "wakeup" interrupt but no "pci" interrupt: are you
sure you're properly falling back to the tree-walking INTx discovery? At
this point, 'index' will be out of bounds, so I guess you rely on
of_irq_parse_one(dn, OUT_OF_BOUNDS, out_irq) returning an error?

It seems like you could also be sure to skip the IRQ parsing in that
case by writing this:

		/*
		 * 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;
		}

Brian

>  	}
> -- 
> 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 v10 4/7] arm64: dts: rockchip: Move PCIe WAKE# irq to pcie driver for Gru

[Jeffy Chen]

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

Move it to rockchip pcie driver 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

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


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

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

[Rafael J. Wysocki]

On Friday, October 27, 2017 9:26:05 AM CEST Jeffy Chen wrote:
> 
> 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 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.
> 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.
> 
> 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
>   of/irq: Adjust of_pci_irq parsing for multiple interrupts
>   mwifiex: Disable wakeup irq handling for pcie
>   arm64: dts: rockchip: Move PCIe WAKE# irq to pcie driver for Gru
>   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

Overall, I don't quite like the direction this is going into, but I need to
have a deeper look.  Which may take some time, so please bear with me.

Thanks,
Rafael

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

[jeffy]

Hi Rafael,

thanks for your reply.

On 10/28/2017 05:07 PM, Rafael J. Wysocki wrote:
> Overall, I don't quite like the direction this is going into, but I need to
> have a deeper look.  Which may take some time, so please bear with me.
>
ok, i'll wait for your comments, thanks :)

> Thanks,
> Rafael


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