[PATCH v4 0/4] PCI: rockchip: Move PCIE_WAKE handling into rockchip pcie driver

Currently we are handling pcie wake in mrvl wifi driver. But Brian
suggests to move it into rockchip pcie driver.

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


Changes in v4:
Rebase on newest for-next branch, also fix error handling by:
1e7f570a1b86 PCI: rockchip: Idle inactive PHY(s)

Changes in v3:
Fix error handling

Changes in v2:
Use dev_pm_set_dedicated_wake_irq
        -- Suggested by Brian Norris [off-list ref]

Jeffy Chen (4):
  PCI: rockchip: Fix error handlings
  PCI: rockchip: Add support for pcie wake irq
  dt-bindings: PCI: rockchip: Add support for pcie wake irq
  arm64: dts: rockchip: Handle pcie wake in pcie driver for Gru

 .../devicetree/bindings/pci/rockchip-pcie.txt      |  20 ++-
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi       |  15 +-
 drivers/pci/host/pcie-rockchip.c                   | 179 ++++++++++++---------
 3 files changed, 126 insertions(+), 88 deletions(-)

-- 
2.11.0

Add an optional interrupt for PCIE_WAKE pin.

Signed-off-by: Jeffy Chen <redacted>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 .../devicetree/bindings/pci/rockchip-pcie.txt        | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 5678be82530d..9f6504129e80 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt

@@ -20,10 +20,13 @@ Required properties:
 - msi-map: Maps a Requester ID to an MSI controller and associated
 	msi-specifier data. See ./pci-msi.txt
 - interrupts: Three interrupt entries must be specified.
-- interrupt-names: Must include the following names
-	- "sys"
-	- "legacy"
-	- "client"
+- interrupt-names: Include the following names
+	Required:
+		- "sys"
+		- "legacy"
+		- "client"
+	Optional:
+		- "wake"
 - resets: Must contain seven entries for each entry in reset-names.
 	   See ../reset/reset.txt for details.
 - reset-names: Must include the following names

@@ -87,10 +90,11 @@ pcie0: pcie@f8000000 {
 	clock-names = "aclk", "aclk-perf",
 		      "hclk", "pm";
 	bus-range = <0x0 0x1>;
-	interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
-		     <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
-		     <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>;
-	interrupt-names = "sys", "legacy", "client";
+	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", "wake";
 	assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
 	assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
 	assigned-clock-rates = <100000000>;

-- 
2.11.0

On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:

Add an optional interrupt for PCIE_WAKE pin.

Rob?

Why is there no other PCI binding that includes "wake" as an
interrupt-name?  This feels like something that should be fairly
common across host controllers.  I don't want a Rockport-specific
DT description if it could be made more general.

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

On Thu, Aug 24, 2017 at 11:53:54AM -0500, Bjorn Helgaas wrote:

On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:

quoted

Signed-off-by: Jeffy Chen <redacted>

quoted

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 5678be82530d..9f6504129e80 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt

@@ -20,10 +20,13 @@ Required properties:
 - msi-map: Maps a Requester ID to an MSI controller and associated
 	msi-specifier data. See ./pci-msi.txt
 - interrupts: Three interrupt entries must be specified.
-- interrupt-names: Must include the following names
-	- "sys"
-	- "legacy"
-	- "client"
+- interrupt-names: Include the following names
+	Required:
+		- "sys"
+		- "legacy"
+		- "client"
+	Optional:
+		- "wake"

Why is there no other PCI binding that includes "wake" as an
interrupt-name?  This feels like something that should be fairly
common across host controllers.  I don't want a Rockport-specific

s/port/chip/ :)

DT description if it could be made more general.

I'm not sure we can really answer that question ("why do no other PCI
bindings have this?"). But one guess would be that every other
controller uses only beacon wake.

It would be OK with me if we made a blanket statement that a controller
with a "wake" interrupt means PCI WAKE# (per the specification). It's
possible this could even be stuck into some generic PCI/DT code
eventually. (I don't think we have a really good place for this today.)

Brian

quoted

 - resets: Must contain seven entries for each entry in reset-names.
 	   See ../reset/reset.txt for details.
 - reset-names: Must include the following names

@@ -87,10 +90,11 @@ pcie0: pcie@f8000000 {
 	clock-names = "aclk", "aclk-perf",
 		      "hclk", "pm";
 	bus-range = <0x0 0x1>;
-	interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
-		     <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
-		     <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>;
-	interrupt-names = "sys", "legacy", "client";
+	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", "wake";
 	assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
 	assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
 	assigned-clock-rates = <100000000>;

-- 
2.11.0

On 2017/8/25 10:11, Brian Norris wrote:

On Thu, Aug 24, 2017 at 11:53:54AM -0500, Bjorn Helgaas wrote:

quoted

On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:

quoted

Signed-off-by: Jeffy Chen <redacted>

quoted

quoted

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 5678be82530d..9f6504129e80 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt

@@ -20,10 +20,13 @@ Required properties:
  - msi-map: Maps a Requester ID to an MSI controller and associated
  	msi-specifier data. See ./pci-msi.txt
  - interrupts: Three interrupt entries must be specified.
-- interrupt-names: Must include the following names
-	- "sys"
-	- "legacy"
-	- "client"
+- interrupt-names: Include the following names
+	Required:
+		- "sys"
+		- "legacy"
+		- "client"
+	Optional:
+		- "wake"

Why is there no other PCI binding that includes "wake" as an
interrupt-name?  This feels like something that should be fairly
common across host controllers.  I don't want a Rockport-specific

s/port/chip/ :)

quoted

DT description if it could be made more general.

I'm not sure we can really answer that question ("why do no other PCI
bindings have this?"). But one guess would be that every other
controller uses only beacon wake.

It would be OK with me if we made a blanket statement that a controller
with a "wake" interrupt means PCI WAKE# (per the specification). It's
possible this could even be stuck into some generic PCI/DT code
eventually. (I don't think we have a really good place for this today.)

I guess we could register a pcie port service for dedicated WAKE# as it 
seems fairly parallel to pme code there, if we need a common place for
that?

Brian

quoted

quoted

  - resets: Must contain seven entries for each entry in reset-names.
  	   See ../reset/reset.txt for details.
  - reset-names: Must include the following names

@@ -87,10 +90,11 @@ pcie0: pcie@f8000000 {
  	clock-names = "aclk", "aclk-perf",
  		      "hclk", "pm";
  	bus-range = <0x0 0x1>;
-	interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH 0>,
-		     <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH 0>,
-		     <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH 0>;
-	interrupt-names = "sys", "legacy", "client";
+	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", "wake";
  	assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
  	assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
  	assigned-clock-rates = <100000000>;

-- 
2.11.0

On Thu, Aug 24, 2017 at 07:11:32PM -0700, Brian Norris wrote:

On Thu, Aug 24, 2017 at 11:53:54AM -0500, Bjorn Helgaas wrote:

quoted

On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:

quoted

Signed-off-by: Jeffy Chen <redacted>

quoted

quoted

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 5678be82530d..9f6504129e80 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt

@@ -20,10 +20,13 @@ Required properties:
 - msi-map: Maps a Requester ID to an MSI controller and associated
 	msi-specifier data. See ./pci-msi.txt
 - interrupts: Three interrupt entries must be specified.
-- interrupt-names: Must include the following names
-	- "sys"
-	- "legacy"
-	- "client"
+- interrupt-names: Include the following names
+	Required:
+		- "sys"
+		- "legacy"
+		- "client"
+	Optional:
+		- "wake"

Why is there no other PCI binding that includes "wake" as an
interrupt-name?  This feels like something that should be fairly
common across host controllers.  I don't want a Rockport-specific

s/port/chip/ :)

I visited Rockport this summer, guess I had it on the brain :)

quoted

DT description if it could be made more general.

I'm not sure we can really answer that question ("why do no other PCI
bindings have this?"). But one guess would be that every other
controller uses only beacon wake.

It would be OK with me if we made a blanket statement that a controller
with a "wake" interrupt means PCI WAKE# (per the specification). It's
possible this could even be stuck into some generic PCI/DT code
eventually. (I don't think we have a really good place for this today.)

I'd just like every controller that uses WAKE# to use the same name in
DT.  Maybe all that means for now is mentioning it in
Documentation/devicetree/bindings/pci/pci.txt instead of (or in
addition to) Documentation/devicetree/bindings/pci/rockchip-pcie.txt

On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:

Use the wakeup source binding:
Documentation/devicetree/bindings/power/wakeup-source.txt

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

On Fri, Aug 25, 2017 at 01:14:39PM -0500, Rob Herring wrote:

On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:

quoted

Add an optional interrupt for PCIE_WAKE pin.

Signed-off-by: Jeffy Chen <redacted>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 .../devicetree/bindings/pci/rockchip-pcie.txt        | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 5678be82530d..9f6504129e80 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt

@@ -20,10 +20,13 @@ Required properties:
 - msi-map: Maps a Requester ID to an MSI controller and associated
 	msi-specifier data. See ./pci-msi.txt
 - interrupts: Three interrupt entries must be specified.
-- interrupt-names: Must include the following names
-	- "sys"
-	- "legacy"
-	- "client"
+- interrupt-names: Include the following names
+	Required:
+		- "sys"
+		- "legacy"
+		- "client"
+	Optional:
+		- "wake"

Use the wakeup source binding:
Documentation/devicetree/bindings/power/wakeup-source.txt

And I suppose this means we'd fall under this paragraph?

    "However if the devices have dedicated interrupt as the wakeup source
    then they need to specify/identify the same using device specific
    interrupt name. In such cases only that interrupt can be used as wakeup
    interrupt."

We don't expect *any* interrupt to qualify as PCI WAKE#; so we should
still also document the interrupt name ("wake"?) in
Documentation/devicetree/bindings/pci/pci.txt as Bjorn suggested, in
addition to using the 'wakeup-source' property documented there.

Brian

On Fri, Aug 25, 2017 at 1:20 PM, Brian Norris [off-list ref] wrote:

On Fri, Aug 25, 2017 at 01:14:39PM -0500, Rob Herring wrote:

quoted

On Tue, Aug 22, 2017 at 11:19:33AM +0800, Jeffy Chen wrote:

quoted

Add an optional interrupt for PCIE_WAKE pin.

Signed-off-by: Jeffy Chen <redacted>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 .../devicetree/bindings/pci/rockchip-pcie.txt        | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 5678be82530d..9f6504129e80 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt

@@ -20,10 +20,13 @@ Required properties:
 - msi-map: Maps a Requester ID to an MSI controller and associated
    msi-specifier data. See ./pci-msi.txt
 - interrupts: Three interrupt entries must be specified.
-- interrupt-names: Must include the following names
-   - "sys"
-   - "legacy"
-   - "client"
+- interrupt-names: Include the following names
+   Required:
+           - "sys"
+           - "legacy"
+           - "client"
+   Optional:
+           - "wake"

Use the wakeup source binding:
Documentation/devicetree/bindings/power/wakeup-source.txt

And I suppose this means we'd fall under this paragraph?

    "However if the devices have dedicated interrupt as the wakeup source
    then they need to specify/identify the same using device specific
    interrupt name. In such cases only that interrupt can be used as wakeup
    interrupt."

We don't expect *any* interrupt to qualify as PCI WAKE#; so we should
still also document the interrupt name ("wake"?) in
Documentation/devicetree/bindings/pci/pci.txt as Bjorn suggested, in
addition to using the 'wakeup-source' property documented there.

I believe the defined interrupt name is "wakeup" as example 1 shows.

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

On Mon, Aug 28, 2017 at 04:32:55PM -0500, Rob Herring wrote:

On Fri, Aug 25, 2017 at 1:20 PM, Brian Norris [off-list ref] wrote:

quoted

On Fri, Aug 25, 2017 at 01:14:39PM -0500, Rob Herring wrote:

quoted

Use the wakeup source binding:
Documentation/devicetree/bindings/power/wakeup-source.txt

And I suppose this means we'd fall under this paragraph?

    "However if the devices have dedicated interrupt as the wakeup source
    then they need to specify/identify the same using device specific
    interrupt name. In such cases only that interrupt can be used as wakeup
    interrupt."

We don't expect *any* interrupt to qualify as PCI WAKE#; so we should
still also document the interrupt name ("wake"?) in
Documentation/devicetree/bindings/pci/pci.txt as Bjorn suggested, in
addition to using the 'wakeup-source' property documented there.

I believe the defined interrupt name is "wakeup" as example 1 shows.

That's an example, not a definition. And the definition I quoted
literally says "device specific interrupt name". The PCIe specification
calls it "WAKE#" all over the place, so I figured that's a good name to
use.

"wakeup" is also fine I suppose, as long as we document that it must be
PCIe WAKE# signal, as per the PCIe specfication.

Brian

Currently we are handling pcie wake irq in mrvl wifi driver.
Move it to rockchip pcie driver for Gru boards.

Signed-off-by: Jeffy Chen <redacted>
---

Changes in v4: None
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 d48e98b62d09..42158512e979 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi

@@ -712,7 +712,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", "wake";
+	/delete-property/ interrupts;
+
 	vpcie3v3-supply = <&pp3300_wifi_bt>;
 	vpcie1v8-supply = <&wlan_pd_n>; /* HACK: see &wlan_pd_n */
 	vpcie0v9-supply = <&pp900_pcie>;

@@ -727,11 +735,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

On Tue, Aug 22, 2017 at 11:19:30AM +0800, Jeffy Chen wrote:

Currently we are handling pcie wake in mrvl wifi driver. But Brian
suggests to move it into rockchip pcie driver.

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


Changes in v4:
Rebase on newest for-next branch, also fix error handling by:
1e7f570a1b86 PCI: rockchip: Idle inactive PHY(s)

Changes in v3:
Fix error handling

Changes in v2:
Use dev_pm_set_dedicated_wake_irq
        -- Suggested by Brian Norris [off-list ref]

Jeffy Chen (4):
  PCI: rockchip: Fix error handlings
  PCI: rockchip: Add support for pcie wake irq
  dt-bindings: PCI: rockchip: Add support for pcie wake irq
  arm64: dts: rockchip: Handle pcie wake in pcie driver for Gru

 .../devicetree/bindings/pci/rockchip-pcie.txt      |  20 ++-
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi       |  15 +-
 drivers/pci/host/pcie-rockchip.c                   | 179 ++++++++++++---------
 3 files changed, 126 insertions(+), 88 deletions(-)

Looking for acks from Shawn and Rob...

And I'm not sure about the DT wake IRQ description.  That seems like it
could potentially be generic than this Rockchip-specific proposal.
--
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

Hi Bjorn,

On 08/25/2017 12:55 AM, Bjorn Helgaas wrote:

Looking for acks from Shawn and Rob...

And I'm not sure about the DT wake IRQ description.  That seems like it
could potentially be generic than this Rockchip-specific proposal.

it looks like shawn already take the error handling patch into his 
series, so i'll remove that one in my next version.

i'll wait for those error handling patches to land, then rebase and 
resend this wake irq patches, thanks:)

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