[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 3/7] mwifiex: Disable wakeup irq handling for pcie

[Jeffy Chen]

We are going to handle the wakeup irq in the pci core.

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/net/wireless/marvell/mwifiex/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index ee40b739b289..ba081c16f85c 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1568,6 +1568,10 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
 		goto err_exit;
 
 	adapter->dt_node = dev->of_node;
+
+	if (adapter->iface_type != MWIFIEX_PCIE)
+		goto err_exit;
+
 	adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
 	if (!adapter->irq_wakeup) {
 		dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
-- 
2.11.0

[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

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

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

[Rafael J. Wysocki]

On Friday, October 27, 2017 9:26:10 AM CET Jeffy Chen wrote:
> Allow platforms not to provide some of the pci_platform_pm_ops's
> callbacks.

So?

What exactly is wrong with having empty ops in there?

Is it really better to have everyone do extra checks every time an op is
invoked even when all of the ops are present?

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

Moving stuff around and changing it at the same time is a bad idea.

Change it in one patch and move it around in another one and you'll be less
likely to make a mistake.  Moreover, reviewing it will be easier too, IMO.

Thanks,
Rafael

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

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

[Brian Norris]

Hi Jeffy,

On Fri, Oct 27, 2017 at 03:26:11PM +0800, Jeffy Chen wrote:
> Move acpi wakeup code to pci core as pci_set_wakeup(), so that other
> platforms could reuse it.

I think you need to double check your refactoring. I believe you may
have changed some behavior here.

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

So this function used to return -ENODEV if the platform didn't implement
PM. Now you're transitioning to pci_set_wakeup() below:

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

And it looks like the fallback behavior is just 'return 0', if none of
the devices supported wakeup. It's weird that this already used to
happen for ACPI -- if no device/bridge implemented wakeup, we still
returned success. But now you're making this happen for platforms
without PM operations too.

This affects the behavior of pci_enable_wake below, which tries to
return an error if both PME and "platform wake" failed.

I wonder if this should just return an error code if we reached the end
(i.e., no device or bridge supported wakeup). This would technically
change the ACPI behavior, but then I think it would also be more
correct.

If you do this, that should probably be a separate patch, to be clear
we're changing the error logic before we refactor.

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

I believe you're breaking pci-mid.c, which still assigns a '.set_wakeup'
callback.

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

You're splitting the "set_wakeup" callback into a "dev_wakeup" and a
"bridge_wakeup" function, but you're losing the "set" terminology, which
I think makes things clearer here. So, maybe "set_dev_wakeup" and
"set_bridge_wakeup"? And same for the corresponding platform_pci_*()
helpers.

Brian

>  };
>  
> +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
> 
> 

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

[Rafael J. Wysocki]

On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote:
> Move acpi wakeup code to pci core as pci_set_wakeup(), so that other
> platforms could reuse it.

What exactly do you want to reuse?

It looks like that's just several lines of code in acpi_pci_wakeup()
and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level
functions, so IMO not worth it at all.

The structure for other platform code may be the same or similar, but
the details will almost certainly be different and I don't think that
having more callback pointers in pci_platform_pm_ops is necessarily better.

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

Why are they needed?

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

Thanks,
Rafael

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

[Brian Norris]

Hi Rafael,

I'll answer some of it from my perspective, though Jeffy might have had
different ideas (and answers) when he implemented this.

On Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote:
> On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote:
> > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other
> > platforms could reuse it.
> 
> What exactly do you want to reuse?
> 
> It looks like that's just several lines of code in acpi_pci_wakeup()
> and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level
> functions, so IMO not worth it at all.

The important part he's sharing here is the walking of the tree
structure, in which it's possible for some parent along the way to
handle wakeup for its children. I'm not sure how valuable nor how
reusable that is.

In this case (the Rockchip platforms Jeffy and I are working on), I
think we really want to just support a single WAKE# pin for the whole
system, so maybe the complexity isn't needed. The spec does describe
that there are good reasons for supporting more than 1 WAKE# pin though
(e.g., 1 per device), so it doesn't seem really wise to shoehorn
oursleves into a single setup.

But that can be implemented either via copying the "few" lines of
tree-walking logic, or by trying to share them.

> The structure for other platform code may be the same or similar, but
> the details will almost certainly be different and I don't think that
> having more callback pointers in pci_platform_pm_ops is necessarily better.

I suppose that's reasonable.

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

The implementation is in patch 7, if you really needed more info about
why, or provide alternatives.

The current set of hooks assumes that there is no state information or
initialization needed for tracking actions of these platform PM hooks on
a device. For ACPI this works, because devices have "companion"
acpi_dev's to handle everything, and the ACPI framework generally
prepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be
trivial (and arguably wrong -- several are no-ops, where we might expect
the platform to tell us whether the operation was actually supported or
not).

For device tree, there isn't really a canonical place to store this
information, nor to initialize something like wakeup interrupts.

Technically, we could shoehorn this into the .set_wakeup() call, but
we'd probably rather not do things like request_irq() on every attempt
to suspend/resume the system (among other reasons, we'd lose information
that we might otherwise track in /proc/ or /sys/).

Or the inverse of the above: where would you suggest initializing or
tearing down the wakeirq?

An alternative could be to include any necessary state into the
pci_host_bridge or pci_dev and just inline the setup code into
pci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c
(pci_device_{probe,remove}()). But I'm not sure that's much more
beautiful.

Brian

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

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

[Rafael J. Wysocki]

On Tuesday, November 14, 2017 3:51:11 AM CET Brian Norris wrote:
> Hi Rafael,
> 
> I'll answer some of it from my perspective, though Jeffy might have had
> different ideas (and answers) when he implemented this.
> 
> On Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote:
> > On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote:
> > > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other
> > > platforms could reuse it.
> > 
> > What exactly do you want to reuse?
> > 
> > It looks like that's just several lines of code in acpi_pci_wakeup()
> > and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level
> > functions, so IMO not worth it at all.
> 
> The important part he's sharing here is the walking of the tree
> structure, in which it's possible for some parent along the way to
> handle wakeup for its children. I'm not sure how valuable nor how
> reusable that is.

ACPI very well may be the only case needing to walk the hierarchy for that.

> In this case (the Rockchip platforms Jeffy and I are working on), I
> think we really want to just support a single WAKE# pin for the whole
> system, so maybe the complexity isn't needed.

So unless I know you'll need it, please don't add it.

> The spec does describe that there are good reasons for supporting more than
> 1 WAKE# pin though (e.g., 1 per device), so it doesn't seem really wise to
> shoehorn oursleves into a single setup.

One WAKE# pin per device is reasonable enough, but WAKE# is specifically
defined as out-of-band and "orthogonal" to the PCIe hierarchy.  What you
need is a way to program WAKE# and (possibly) wakeup power on the given
platform for each device having a WAKE# pin individually.
 
The main reason why ACPI needs to walk the hierarchy is that on some systems
the firmware takes over the handling of the native PME mechanism which then is
taken care of by the AML (and the kernel is not granted control of the
relevant PCIe registers).  I don't think you'll ever see anything like that
on non-ACPI systems.

> But that can be implemented either via copying the "few" lines of
> tree-walking logic, or by trying to share them.
> 
> > The structure for other platform code may be the same or similar, but
> > the details will almost certainly be different and I don't think that
> > having more callback pointers in pci_platform_pm_ops is necessarily better.
> 
> I suppose that's reasonable.
> 
> > > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm
> > > ops's callbacks to setup and cleanup pci devices and host bridge for
> > > wakeup.
> > 
> > Why are they needed?
> 
> The implementation is in patch 7, if you really needed more info about
> why, or provide alternatives.

Well, that's quite questionable.

At least defining ->dev_wakeup to do device_set_wakeup_capable() doesn't
really make sense to me.

> The current set of hooks assumes that there is no state information or
> initialization needed for tracking actions of these platform PM hooks on
> a device. For ACPI this works, because devices have "companion"
> acpi_dev's to handle everything, and the ACPI framework generally
> prepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be
> trivial (and arguably wrong -- several are no-ops, where we might expect
> the platform to tell us whether the operation was actually supported or
> not).
> 
> For device tree, there isn't really a canonical place to store this
> information, nor to initialize something like wakeup interrupts.

The only thing you need IMO is ->set_wakeup which also is what ACPI needs
and that enables or disables wakeup for the device.  It doesn't actually
have to track anything (other than, possibly, whether or not wakeup has
been enabled for it already).

And since the core takes care of native PMEs for you, the only thing
this needs to cover is the WAKE# pin programming AFAICS.

The setup part, in turn, in the ACPI case is done via acpi_platform_notify()
and, analogously, acpi_platform_notify_remove() does the cleanup.  You seem
to be trying to add something like it via pci_platform_pm_ops, but is it
really the most suitable place for that?

> Technically, we could shoehorn this into the .set_wakeup() call, but
> we'd probably rather not do things like request_irq() on every attempt
> to suspend/resume the system (among other reasons, we'd lose information
> that we might otherwise track in /proc/ or /sys/).
> 
> Or the inverse of the above: where would you suggest initializing or
> tearing down the wakeirq?

Again, ACPI does that via acpi_platform_notify()/acpi_platform_notify_remove(),
so maybe something similar?

In any case, you need a way to associate DT-provided data with PCI devices and,
ideally, that should be done at the enumeration time.

> An alternative could be to include any necessary state into the
> pci_host_bridge or pci_dev and just inline the setup code into
> pci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c
> (pci_device_{probe,remove}()). But I'm not sure that's much more
> beautiful.

Well, that's not the way it is done in the ACPI case anyway.

Thanks,
Rafael

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

[Brian Norris]

Hi Rafael,

Thanks for the responses, and sorry for some delay. My thoughts are
still a bit scattered on this (and I'm also busy with other things), and
I'd like some feedback on the device tree definitions from someone, if
possible. (I expect you're more familiar with ACPI than with device
tree, but perhaps you or someone else on copy can humor me?)

I'm also not sure I agree with all of your suggestions, though maybe
something "similar" could be OK.

On Wed, Nov 22, 2017 at 01:26:02AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 14, 2017 3:51:11 AM CET Brian Norris wrote:
> > On Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote:
> > > On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote:
> > > > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other
> > > > platforms could reuse it.
> > > 
> > > What exactly do you want to reuse?
> > > 
> > > It looks like that's just several lines of code in acpi_pci_wakeup()
> > > and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level
> > > functions, so IMO not worth it at all.
> > 
> > The important part he's sharing here is the walking of the tree
> > structure, in which it's possible for some parent along the way to
> > handle wakeup for its children. I'm not sure how valuable nor how
> > reusable that is.
> 
> ACPI very well may be the only case needing to walk the hierarchy for that.

Sure. It does seem like a bit of overkill, on second thought.

> > In this case (the Rockchip platforms Jeffy and I are working on), I
> > think we really want to just support a single WAKE# pin for the whole
> > system, so maybe the complexity isn't needed.
> 
> So unless I know you'll need it, please don't add it.

Sure, arbitrary hierarchy is probably over-engineering. But 1-per-device
is quite reasonable -- even if we don't support it immediately, it would
be nice if it can be added without too much trouble.

> > The spec does describe that there are good reasons for supporting more than
> > 1 WAKE# pin though (e.g., 1 per device), so it doesn't seem really wise to
> > shoehorn oursleves into a single setup.
> 
> One WAKE# pin per device is reasonable enough, but WAKE# is specifically
> defined as out-of-band and "orthogonal" to the PCIe hierarchy.  What you
> need is a way to program WAKE# and (possibly) wakeup power on the given
> platform for each device having a WAKE# pin individually.

So, *don't* define the wakeup in the host bridge?

Or put another way: how would you define a DT description for this?

By the way, it seems pretty ambiguous how we want to handle things like
(a) multiple devices sharing the same WAKE#
(b) systems where a slot is swappable

For (a), the main problem is that if we have to repeat the interrupt
definition in multiple devices, then we have to deal with something like
IRQF_SHARED. That can be done, but it makes it much harder to use the
dedicated wakeirq helpers.

For (b), it's already weird to write device trees for probable PCI
devices; you technically need to redefine the node for the PCI ID of
each device that gets plugged in. So instead, it seems more like maybe
a property of the port?

Or more concretely:

pcie@XXXXXXXX {
	compatible = "rockchip,rk3399-pcie";
	interrupts-extended = ..., <&gpioX Y>;  // the existing
						// proposal, for
						// 'wakeup' in the host
						// bridge
	interrupt-names = ..., "wakeup";

	pcie_port: pcie@0,0 {
		...
		/*
		// This is a possible proposal, to account for (b)?
		interrupts = <Y>;
		interrupt-parent = <&gpioX>;
		interrupt-names = "wakeup";
		*/

		wifi_device: wifi@0,0 {
			compatible = "pci11ab,2b42";
			/*
			// This is the "1-per-device" proposal?
			// But it doesn't work well if the PCI ID
			// changes (e.g., card swap). Probably want to
			// avoid this if possible.
			// Also, it has an awkward conflict with INTx
			// definitions.
			interrupts-extended = <&intX 0>, <&gpioX Y>;
			interrupt-names = "pci", "wakeup";
			*/
			...
		};
	};
};

Does the description at the "port" level seem reasonable? We'd still
have to figure out how to parse this properly of course...

> The main reason why ACPI needs to walk the hierarchy is that on some systems
> the firmware takes over the handling of the native PME mechanism which then is
> taken care of by the AML (and the kernel is not granted control of the
> relevant PCIe registers).  I don't think you'll ever see anything like that
> on non-ACPI systems.

OK.

> > But that can be implemented either via copying the "few" lines of
> > tree-walking logic, or by trying to share them.
> > 
> > > The structure for other platform code may be the same or similar, but
> > > the details will almost certainly be different and I don't think that
> > > having more callback pointers in pci_platform_pm_ops is necessarily better.
> > 
> > I suppose that's reasonable.
> > 
> > > > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm
> > > > ops's callbacks to setup and cleanup pci devices and host bridge for
> > > > wakeup.
> > > 
> > > Why are they needed?
> > 
> > The implementation is in patch 7, if you really needed more info about
> > why, or provide alternatives.
> 
> Well, that's quite questionable.
> 
> At least defining ->dev_wakeup to do device_set_wakeup_capable() doesn't
> really make sense to me.

Hmm, that does seem a little weird, but I believe maybe Jeffy is using
it as a hack for fitting in with the "dedicated IRQ" stuff? It seems
more like you should be able to use something like
dev_pm_enable_wake_irq() / dev_pm_disable_wake_irq() instead though.
That's a question for patch 7 though, not for the general architecture
of these hooks -- we want to do *something* the arm/disarm the wake-IRQ
there.

> > The current set of hooks assumes that there is no state information
> > or
> > initialization needed for tracking actions of these platform PM hooks on
> > a device. For ACPI this works, because devices have "companion"
> > acpi_dev's to handle everything, and the ACPI framework generally
> > prepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be
> > trivial (and arguably wrong -- several are no-ops, where we might expect
> > the platform to tell us whether the operation was actually supported or
> > not).
> > 
> > For device tree, there isn't really a canonical place to store this
> > information, nor to initialize something like wakeup interrupts.
> 
> The only thing you need IMO is ->set_wakeup which also is what ACPI needs
> and that enables or disables wakeup for the device.  It doesn't actually
> have to track anything (other than, possibly, whether or not wakeup has
> been enabled for it already).

Yes, that's technically the only essential thing needed, though it does
make things a little tougher. We still may need to (depending on whether
we decide to define WAKE# in the bridge, the port, or the device) do
some hierarchy walking. I'm also not yet convinced about "where to
initialize" (see comments below).

> And since the core takes care of native PMEs for you, the only thing
> this needs to cover is the WAKE# pin programming AFAICS.

Right.

> The setup part, in turn, in the ACPI case is done via acpi_platform_notify()
> and, analogously, acpi_platform_notify_remove() does the cleanup.  You seem
> to be trying to add something like it via pci_platform_pm_ops, but is it
> really the most suitable place for that?

Well, the global 'platform_notify' and 'platform_notify_remove'
singletons seem even more fragile. Do we really want to take over the
whole system's device registration hooks just to handle something for a
particular bus type? We're not going to be able to write a generic "add"
and "remove" hook for all devices on all Device Tree systems for this
type of feature. It would likely break a lot of sub-devices to start
parsing "wakeup" properties there.

We *could* do something like what it8152 does:

static int it8152_pci_platform_notify(struct device *dev)
{
	if (dev_is_pci(dev)) {
	...
	}
	return 0;
}

but that doesn't look very nice to me. Among other problems, it won't
stack nicely. What if an 'it8152' system wants to use this 'wakeup'
interrupt definition too?

Instead, I think it makes perfect sense to put the DT-centric wakeup
handling all in the same module -- the "Firmware PM callbacks" (struct
pci_platform_pm_ops) -- instead of scattering it around the codebase.

> > Technically, we could shoehorn this into the .set_wakeup() call, but
> > we'd probably rather not do things like request_irq() on every attempt
> > to suspend/resume the system (among other reasons, we'd lose information
> > that we might otherwise track in /proc/ or /sys/).
> > 
> > Or the inverse of the above: where would you suggest initializing or
> > tearing down the wakeirq?
> 
> Again, ACPI does that via acpi_platform_notify()/acpi_platform_notify_remove(),
> so maybe something similar?

I don't like that solution, per the above. (Depending on what "or maybe
something similar" means.)

> In any case, you need a way to associate DT-provided data with PCI devices and,
> ideally, that should be done at the enumeration time.

Yes, but that's what these hooks were allowing; PCI PM firmware hooks
get a chance to do enumeration-time initialization for both bridges and
devices.

> > An alternative could be to include any necessary state into the
> > pci_host_bridge or pci_dev and just inline the setup code into
> > pci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c
> > (pci_device_{probe,remove}()). But I'm not sure that's much more
> > beautiful.
> 
> Well, that's not the way it is done in the ACPI case anyway.

Brian

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

[Tony Lindgren]

* Brian Norris <briannorris@chromium.org> [171206 19:36]:
> By the way, it seems pretty ambiguous how we want to handle things like
> (a) multiple devices sharing the same WAKE#
> (b) systems where a slot is swappable
> 
> For (a), the main problem is that if we have to repeat the interrupt
> definition in multiple devices, then we have to deal with something like
> IRQF_SHARED. That can be done, but it makes it much harder to use the
> dedicated wakeirq helpers.

This will get messy, let's not go there :) That is unless the hardware
really has a single interrupt wired to multiple devices. And in that
case almost certainly a custom interrupt handler is needed.

Regards,

Tony

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

[Brian Norris]

On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote:
> * Brian Norris <briannorris@chromium.org> [171206 19:36]:
> > By the way, it seems pretty ambiguous how we want to handle things like
> > (a) multiple devices sharing the same WAKE#
> > (b) systems where a slot is swappable
> > 
> > For (a), the main problem is that if we have to repeat the interrupt
> > definition in multiple devices, then we have to deal with something like
> > IRQF_SHARED. That can be done, but it makes it much harder to use the
> > dedicated wakeirq helpers.
> 
> This will get messy, let's not go there :) That is unless the hardware
> really has a single interrupt wired to multiple devices. And in that
> case almost certainly a custom interrupt handler is needed.

As Rafael mentioned, the spec doesn't clearly delineate a required
hierarchy to the WAKE# pin, and it's certainly possible to share it. I'm
fine dodging that question for now, and only writing said custom
interrupt handler if/when needed.

But device tree bindings are "forever", so it seems reasonable to at
least agree how it should be defined.

Brian

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

[Tony Lindgren]

* Brian Norris <briannorris@chromium.org> [171207 00:32]:
> On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote:
> > * Brian Norris <briannorris@chromium.org> [171206 19:36]:
> > > By the way, it seems pretty ambiguous how we want to handle things like
> > > (a) multiple devices sharing the same WAKE#
> > > (b) systems where a slot is swappable
> > > 
> > > For (a), the main problem is that if we have to repeat the interrupt
> > > definition in multiple devices, then we have to deal with something like
> > > IRQF_SHARED. That can be done, but it makes it much harder to use the
> > > dedicated wakeirq helpers.
> > 
> > This will get messy, let's not go there :) That is unless the hardware
> > really has a single interrupt wired to multiple devices. And in that
> > case almost certainly a custom interrupt handler is needed.
> 
> As Rafael mentioned, the spec doesn't clearly delineate a required
> hierarchy to the WAKE# pin, and it's certainly possible to share it. I'm
> fine dodging that question for now, and only writing said custom
> interrupt handler if/when needed.

OK if the WAKE# pin is shared then PCI (or hardware specific?) code needs
to figure out from where it came from.

> But device tree bindings are "forever", so it seems reasonable to at
> least agree how it should be defined.

Well that's why we're just using the existing interrupts-extended
binding there :) It does not leave out the option for shared interrupts,
it's just that drivers/base/power/wakeirq.c can't deal with them in a
sane way or at least we'd have to add a flag to not enable/disable the
wakeirq automatically.

Regards,

Tony

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

[Rafael J. Wysocki]

On Fri, Dec 8, 2017 at 5:37 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Brian Norris <briannorris@chromium.org> [171207 00:32]:
>> On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote:
>> > * Brian Norris <briannorris@chromium.org> [171206 19:36]:
>> > > By the way, it seems pretty ambiguous how we want to handle things like
>> > > (a) multiple devices sharing the same WAKE#
>> > > (b) systems where a slot is swappable
>> > >
>> > > For (a), the main problem is that if we have to repeat the interrupt
>> > > definition in multiple devices, then we have to deal with something like
>> > > IRQF_SHARED. That can be done, but it makes it much harder to use the
>> > > dedicated wakeirq helpers.
>> >
>> > This will get messy, let's not go there :) That is unless the hardware
>> > really has a single interrupt wired to multiple devices. And in that
>> > case almost certainly a custom interrupt handler is needed.
>>
>> As Rafael mentioned, the spec doesn't clearly delineate a required
>> hierarchy to the WAKE# pin, and it's certainly possible to share it. I'm
>> fine dodging that question for now, and only writing said custom
>> interrupt handler if/when needed.
>
> OK if the WAKE# pin is shared then PCI (or hardware specific?) code needs
> to figure out from where it came from.

Right.

Basically, that would need a list (or equivalent) of devices sharing
the wakeup IRQ and if that triggers, it needs to walk the list and
call pci_pme_wakeup() for all devices in it.

>> But device tree bindings are "forever", so it seems reasonable to at
>> least agree how it should be defined.
>
> Well that's why we're just using the existing interrupts-extended
> binding there :) It does not leave out the option for shared interrupts,
> it's just that drivers/base/power/wakeirq.c can't deal with them in a
> sane way or at least we'd have to add a flag to not enable/disable the
> wakeirq automatically.

I think that the binding needs to be sort-of PCI-specific to cover the
shared case.

I'm not sure if there are any other buses needing that (they would
need to define standard PM registers as PCI does).

[RFC PATCH v10 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 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/pci/Makefile |   2 +-
 drivers/pci/pci-of.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 128 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..28f3c4a0eec8
--- /dev/null
+++ b/drivers/pci/pci-of.c
@@ -0,0 +1,127 @@
+/*
+ * 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, ret;
+
+	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, true);
+	ret = dev_pm_set_dedicated_wake_irq(dev, irq);
+	if (ret < 0) {
+		device_init_wakeup(dev, false);
+		return NULL;
+	}
+	device_set_wakeup_capable(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;
+
+	device_set_wakeup_capable(dev, enable);
+	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 v10 7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

[Brian Norris]

Hi Jeffy,

On Fri, Oct 27, 2017 at 03:26:12PM +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 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/pci/Makefile |   2 +-
>  drivers/pci/pci-of.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 128 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..28f3c4a0eec8
> --- /dev/null
> +++ b/drivers/pci/pci-of.c
> @@ -0,0 +1,127 @@
> +/*
> + * 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, ret;
> +
> +	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, true);
> +	ret = dev_pm_set_dedicated_wake_irq(dev, irq);

I'm seeing this WARNING during system resume when I enable wake-on-Wifi
with this series:

[  135.259920] Unbalanced IRQ 64 wake disable
[  135.259929] ------------[ cut here ]------------
[  135.259942] WARNING: CPU: 0 PID: 3233 at kernel/irq/manage.c:606 irq_set_irq_wake+0xac/0x12c
[  135.259944] Modules linked in: btusb btrtl btbcm btintel bluetooth ecdh_generic cdc_ether usbnet uvcvideo mwifiex_pcie videobuf2_vmalloc videobuf2_memops mwifiex videobuf2_v4l2 videobuf2_core cfg80211 ip6table_filter r8152 mii joydev
[  135.259986] CPU: 0 PID: 3233 Comm: bash Not tainted 4.14.0-rc3+ #40
[  135.259988] Hardware name: Google Kevin (DT)
[  135.259991] task: ffffffc0f133c880 task.stack: ffffff8010520000
[  135.259994] PC is at irq_set_irq_wake+0xac/0x12c
[  135.259998] LR is at irq_set_irq_wake+0xac/0x12c
...
[  135.260121] [<ffffff80080ff1a4>] irq_set_irq_wake+0xac/0x12c
[  135.260127] [<ffffff8008494a7c>] dev_pm_disarm_wake_irq+0x3c/0x58
[  135.260133] [<ffffff800849989c>] device_wakeup_disarm_wake_irqs+0x50/0x78
[  135.260137] [<ffffff800849667c>] dpm_noirq_end+0x18/0x24
[  135.260140] [<ffffff80084966ac>] dpm_resume_noirq+0x24/0x30
[  135.260146] [<ffffff80080f85cc>] suspend_devices_and_enter+0x474/0x970
[  135.260150] [<ffffff80080f9150>] pm_suspend+0x688/0x6cc
[  135.260154] [<ffffff80080f7388>] state_store+0xd4/0xf8
[  135.260160] [<ffffff80087c3f3c>] kobj_attr_store+0x18/0x28
[  135.260165] [<ffffff80082818f8>] sysfs_kf_write+0x5c/0x68
[  135.260169] [<ffffff80082808c0>] kernfs_fop_write+0x15c/0x198
[  135.260174] [<ffffff80082081a8>] __vfs_write+0x58/0x160
[  135.260178] [<ffffff8008208484>] vfs_write+0xc4/0x15c
[  135.260181] [<ffffff80082086cc>] SyS_write+0x64/0xb4

I'm not yet sure if this is your series' fault, or if the dedicated wake IRQ
infrastructure did something wrong.

> +	if (ret < 0) {
> +		device_init_wakeup(dev, false);
> +		return NULL;
> +	}
> +	device_set_wakeup_capable(dev, false);
> +
> +	dev_info(dev, "Wakeup IRQ %d\n", irq);

Do you actually need to print this out? It'll be available in things
like /proc/interrupts now, so this seems overkill.

> +	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;
> +
> +	device_set_wakeup_capable(dev, enable);
> +	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)
> +{

I still don't think you've worked out the potential conflict between
ACPI and OF on (e.g.) ARM64 systems with both enabled in the kernel. On
such kernels, both acpi_pci_init() and of_pci_init() are built in as
arch_initcalls, and which one wins will be based on implementation
details like link order.

Seems like acpi_pci_init() could still use something like this:

	if (acpi_disabled)
		return;

And do the opposite here in of_pci_init().

It also feels like we could use something like this in pci.c:

 void pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
 {
+	WARN(pci_platform_pm, "PCI platform PM ops already registered\n");
 	pci_platform_pm = ops;
 }

And I wonder what happens with pci-mid.c -- does this currently win the
collision because pci-mid.o is linked after pci-acpi.o?

Brian

> +	pci_set_platform_pm(&of_pci_platform_pm);
> +	return 0;
> +}
> +arch_initcall(of_pci_init);
> -- 
> 2.11.0
> 
> 

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