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).
Jeffy Chen (3):
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 | 11 ++--
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 17 ++++---
drivers/pci/host/pcie-rockchip.c | 58 ++++++++++++++++++++++
3 files changed, 76 insertions(+), 10 deletions(-)
--
2.11.0
Add support for PCIE_WAKE pin in rockchip pcie driver.
Signed-off-by: Jeffy Chen <[email protected]>
---
drivers/pci/host/pcie-rockchip.c | 58 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 7bb9870f6d8c..f969a6d3cd85 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -38,6 +38,7 @@
#include <linux/platform_device.h>
#include <linux/reset.h>
#include <linux/regmap.h>
+#include <linux/suspend.h>
/*
* The upper 16 bits of PCIE_CLIENT_CONFIG are a write mask for the lower 16
@@ -226,6 +227,8 @@ struct rockchip_pcie {
struct regulator *vpcie1v8; /* 1.8V power supply */
struct regulator *vpcie0v9; /* 0.9V power supply */
struct gpio_desc *ep_gpio;
+ int wake_irq;
+ bool wake_by_pcie;
u32 lanes;
u8 root_bus_nr;
int link_gen;
@@ -853,6 +856,20 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
chained_irq_exit(chip, desc);
}
+static irqreturn_t rockchip_pcie_wake_irq_handler(int irq, void *arg)
+{
+ struct rockchip_pcie *rockchip = arg;
+
+ rockchip->wake_by_pcie = true;
+
+ disable_irq_nosync(rockchip->wake_irq);
+ disable_irq_wake(rockchip->wake_irq);
+
+ pm_wakeup_event(rockchip->dev, 0);
+ pm_system_wakeup();
+
+ return IRQ_HANDLED;
+}
/**
* rockchip_pcie_parse_dt - Parse Device Tree
@@ -868,6 +885,7 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
struct resource *regs;
int irq;
int err;
+ bool wakeup = 0;
regs = platform_get_resource_byname(pdev,
IORESOURCE_MEM,
@@ -1018,6 +1036,21 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
return err;
}
+ rockchip->wake_irq = platform_get_irq_byname(pdev, "wake");
+ if (rockchip->wake_irq >= 0) {
+ err = devm_request_irq(dev, rockchip->wake_irq,
+ rockchip_pcie_wake_irq_handler,
+ 0, "pcie-wake", rockchip);
+ if (err) {
+ dev_err(dev, "failed to request PCIe wake IRQ\n");
+ return err;
+ }
+
+ disable_irq(rockchip->wake_irq);
+ wakeup = device_property_read_bool(dev, "wakeup-source");
+ }
+ device_init_wakeup(dev, wakeup);
+
rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
if (IS_ERR(rockchip->vpcie3v3)) {
if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
@@ -1270,6 +1303,30 @@ static int rockchip_pcie_wait_l2(struct rockchip_pcie *rockchip)
return 0;
}
+static int __maybe_unused rockchip_pcie_suspend(struct device *dev)
+{
+ struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
+
+ rockchip->wake_by_pcie = false;
+
+ if (device_may_wakeup(dev)) {
+ enable_irq_wake(rockchip->wake_irq);
+ enable_irq(rockchip->wake_irq);
+ }
+ return 0;
+}
+
+static int __maybe_unused rockchip_pcie_resume(struct device *dev)
+{
+ struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
+
+ if (device_may_wakeup(dev) && !rockchip->wake_by_pcie) {
+ disable_irq(rockchip->wake_irq);
+ disable_irq_wake(rockchip->wake_irq);
+ }
+ return 0;
+}
+
static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
{
struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
@@ -1548,6 +1605,7 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
}
static const struct dev_pm_ops rockchip_pcie_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend, rockchip_pcie_resume)
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend_noirq,
rockchip_pcie_resume_noirq)
};
--
2.11.0
Add an optional interrupt for PCIE_WAKE pin.
Signed-off-by: Jeffy Chen <[email protected]>
---
Documentation/devicetree/bindings/pci/rockchip-pcie.txt | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 1453a734c2f5..6ef9903567db 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -22,10 +22,13 @@ Required properties:
- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
- phy-names: MUST be "pcie-phy".
- 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
--
2.11.0
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 <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 17 +++++++++++------
1 file changed, 11 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..7a5e1517a496 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -712,11 +712,21 @@ 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>;
+ wakeup-source;
+
pci_rootport: pcie@0,0 {
reg = <0x83000000 0x0 0x00000000 0x0 0x00000000>;
#address-cells = <3>;
@@ -727,11 +737,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
Hi Jeffy,
On 2017/8/16 15:52, Jeffy Chen wrote:
> 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 <[email protected]>
> ---
>
> arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 17 +++++++++++------
> 1 file changed, 11 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..7a5e1517a496 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> @@ -712,11 +712,21 @@ 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;
> +
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
says "Nodes that describe devices which generate interrupts must contain
an "interrupts" property, an "interrupts-extended" property, or both. If
both are present, the latter should take precedence;"
But it doesn't seem real as drivers/of/irq.c actually says "Try the
new-style interrupts-extended first"
Anyway, it seems you could safely add interrupts-extended and don't need
to delete "interrupts".
> vpcie3v3-supply = <&pp3300_wifi_bt>;
> vpcie1v8-supply = <&wlan_pd_n>; /* HACK: see &wlan_pd_n */
> vpcie0v9-supply = <&pp900_pcie>;
>
> + wakeup-source;
> +
I also have a question for this one but will inline that in the patch 1
> pci_rootport: pcie@0,0 {
> reg = <0x83000000 0x0 0x00000000 0x0 0x00000000>;
> #address-cells = <3>;
> @@ -727,11 +737,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;
> };
> };
> };
>
Hi Jeffy
On 2017/8/16 15:52, Jeffy Chen wrote:
> Add an optional interrupt for PCIE_WAKE pin.
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Documentation/devicetree/bindings/pci/rockchip-pcie.txt | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> index 1453a734c2f5..6ef9903567db 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -22,10 +22,13 @@ Required properties:
> - phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
> - phy-names: MUST be "pcie-phy".
> - 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"
It would be better to introduce interrupts-extended and show a example
here. :)
> - resets: Must contain seven entries for each entry in reset-names.
> See ../reset/reset.txt for details.
> - reset-names: Must include the following names
>
Hi Jeffy,
On 2017/8/16 15:52, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> drivers/pci/host/pcie-rockchip.c | 58 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 7bb9870f6d8c..f969a6d3cd85 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -38,6 +38,7 @@
> #include <linux/platform_device.h>
> #include <linux/reset.h>
> #include <linux/regmap.h>
> +#include <linux/suspend.h>
>
> /*
> * The upper 16 bits of PCIE_CLIENT_CONFIG are a write mask for the lower 16
> @@ -226,6 +227,8 @@ struct rockchip_pcie {
> struct regulator *vpcie1v8; /* 1.8V power supply */
> struct regulator *vpcie0v9; /* 0.9V power supply */
> struct gpio_desc *ep_gpio;
> + int wake_irq;
> + bool wake_by_pcie;
> u32 lanes;
> u8 root_bus_nr;
> int link_gen;
> @@ -853,6 +856,20 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> +static irqreturn_t rockchip_pcie_wake_irq_handler(int irq, void *arg)
> +{
> + struct rockchip_pcie *rockchip = arg;
> +
> + rockchip->wake_by_pcie = true;
> +
> + disable_irq_nosync(rockchip->wake_irq);
> + disable_irq_wake(rockchip->wake_irq);
> +
> + pm_wakeup_event(rockchip->dev, 0);
> + pm_system_wakeup();
> +
> + return IRQ_HANDLED;
> +}
>
> /**
> * rockchip_pcie_parse_dt - Parse Device Tree
> @@ -868,6 +885,7 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> struct resource *regs;
> int irq;
> int err;
> + bool wakeup = 0;
>
> regs = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> @@ -1018,6 +1036,21 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + rockchip->wake_irq = platform_get_irq_byname(pdev, "wake");
> + if (rockchip->wake_irq >= 0) {
> + err = devm_request_irq(dev, rockchip->wake_irq,
> + rockchip_pcie_wake_irq_handler,
> + 0, "pcie-wake", rockchip);
> + if (err) {
> + dev_err(dev, "failed to request PCIe wake IRQ\n");
> + return err;
This is optional, so I'm not sure if we should prevent the driver to
probe?
> + }
> +
> + disable_irq(rockchip->wake_irq);
> + wakeup = device_property_read_bool(dev, "wakeup-source");
The purpose we add this, is for ep to wakeup the system, so why not
always treate it as a wakeup source.
> + }
> + device_init_wakeup(dev, wakeup);
> +
> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> if (IS_ERR(rockchip->vpcie3v3)) {
> if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> @@ -1270,6 +1303,30 @@ static int rockchip_pcie_wait_l2(struct rockchip_pcie *rockchip)
> return 0;
> }
>
> +static int __maybe_unused rockchip_pcie_suspend(struct device *dev)
> +{
> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> +
> + rockchip->wake_by_pcie = false;
> +
> + if (device_may_wakeup(dev)) {
> + enable_irq_wake(rockchip->wake_irq);
> + enable_irq(rockchip->wake_irq);
> + }
> + return 0;
> +}
> +
> +static int __maybe_unused rockchip_pcie_resume(struct device *dev)
> +{
> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> +
> + if (device_may_wakeup(dev) && !rockchip->wake_by_pcie) {
I don't get this that why we need to check wake_by_pcie here?
> + disable_irq(rockchip->wake_irq);
> + disable_irq_wake(rockchip->wake_irq);
> + }
> + return 0;
> +}
> +
> static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
> {
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> @@ -1548,6 +1605,7 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> }
>
> static const struct dev_pm_ops rockchip_pcie_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend, rockchip_pcie_resume)
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend_noirq,
> rockchip_pcie_resume_noirq)
> };
>
Hi Shawn,
On Wed, Aug 16, 2017 at 04:33:38PM +0800, Shawn Lin wrote:
> On 2017/8/16 15:52, Jeffy Chen wrote:
> >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 <[email protected]>
> >---
> >
> > arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 17 +++++++++++------
> > 1 file changed, 11 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..7a5e1517a496 100644
> >--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> >+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> >@@ -712,11 +712,21 @@ 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;
> >+
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> says "Nodes that describe devices which generate interrupts must contain
> an "interrupts" property, an "interrupts-extended" property, or both. If
> both are present, the latter should take precedence;"
I added that language, and as the text notes, it's just for
compatibility in case some software doesn't understand the 'extended'
property. It's occasionally useful, but not extremely often. (In my case
at that time: we actually had the DTB stuck in the bootloader, and so
would support various combinations of old/new DTBs/kernels which may or
may not understand 'interrupts-extended'.)
> But it doesn't seem real as drivers/of/irq.c actually says "Try the
> new-style interrupts-extended first"
How is that not real? That follows exactly what the binding says; "the
latter" (i.e., the new-style interrupts-extended) is taking precedence.
> Anyway, it seems you could safely add interrupts-extended and don't need
> to delete "interrupts".
You could do this, yes. Unless you really need the compatibility (and
are prepared for such a system to ignore the "wake" interrupt in that
case), I'm not sure the value in that though, as it would just make
things more confusing.
Brian
Hi,
On Wed, Aug 16, 2017 at 03:52:22PM +0800, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> drivers/pci/host/pcie-rockchip.c | 58 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 7bb9870f6d8c..f969a6d3cd85 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -38,6 +38,7 @@
> #include <linux/platform_device.h>
> #include <linux/reset.h>
> #include <linux/regmap.h>
> +#include <linux/suspend.h>
>
> /*
> * The upper 16 bits of PCIE_CLIENT_CONFIG are a write mask for the lower 16
> @@ -226,6 +227,8 @@ struct rockchip_pcie {
> struct regulator *vpcie1v8; /* 1.8V power supply */
> struct regulator *vpcie0v9; /* 0.9V power supply */
> struct gpio_desc *ep_gpio;
> + int wake_irq;
> + bool wake_by_pcie;
> u32 lanes;
> u8 root_bus_nr;
> int link_gen;
> @@ -853,6 +856,20 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> +static irqreturn_t rockchip_pcie_wake_irq_handler(int irq, void *arg)
> +{
> + struct rockchip_pcie *rockchip = arg;
> +
> + rockchip->wake_by_pcie = true;
> +
> + disable_irq_nosync(rockchip->wake_irq);
> + disable_irq_wake(rockchip->wake_irq);
> +
> + pm_wakeup_event(rockchip->dev, 0);
> + pm_system_wakeup();
> +
> + return IRQ_HANDLED;
> +}
>
> /**
> * rockchip_pcie_parse_dt - Parse Device Tree
> @@ -868,6 +885,7 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> struct resource *regs;
> int irq;
> int err;
> + bool wakeup = 0;
'0' should be 'false'.
>
> regs = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> @@ -1018,6 +1036,21 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + rockchip->wake_irq = platform_get_irq_byname(pdev, "wake");
> + if (rockchip->wake_irq >= 0) {
> + err = devm_request_irq(dev, rockchip->wake_irq,
> + rockchip_pcie_wake_irq_handler,
> + 0, "pcie-wake", rockchip);
> + if (err) {
> + dev_err(dev, "failed to request PCIe wake IRQ\n");
> + return err;
> + }
> +
> + disable_irq(rockchip->wake_irq);
If you're worried about keeping this disabled at first, you can just use
this nifty trick (since this isn't a shared interrupt) -- call this
before requesting the IRQ:
irq_set_status_flags(rockchip->wake_irq, IRQ_NOAUTOEN);
You could also consider using dev_pm_set_dedicated_wake_irq() to handle
this -- but beware, it still might not quite handle level-triggered
interrupt properly. I'm pretty sure Tony Lindgren would be happy to get
testing or patches for that though :) He already sent me something a
while back but I didn't have time to test it out.
> + wakeup = device_property_read_bool(dev, "wakeup-source");
> + }
> + device_init_wakeup(dev, wakeup);
Shouldn't you call 'device_init_wakeup(dev, false)' on remove()?
> +
> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> if (IS_ERR(rockchip->vpcie3v3)) {
> if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> @@ -1270,6 +1303,30 @@ static int rockchip_pcie_wait_l2(struct rockchip_pcie *rockchip)
> return 0;
> }
>
> +static int __maybe_unused rockchip_pcie_suspend(struct device *dev)
Why do this in suspend() instead of suspend_noirq()? You shouldn't
really need a separate method here.
Note that this should be a level-triggered interrupt which remains
asserted, so there should be no chance of "missing" it if you don't
enable it in time.
And on a related note: if you try the dedicated wake irq approach, this
will only occur just before the noirq phase anyway, since
device_wakeup_arm_wake_irqs() is called in dpm_suspend_noirq().
> +{
> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> +
> + rockchip->wake_by_pcie = false;
> +
> + if (device_may_wakeup(dev)) {
> + enable_irq_wake(rockchip->wake_irq);
> + enable_irq(rockchip->wake_irq);
> + }
> + return 0;
> +}
> +
> +static int __maybe_unused rockchip_pcie_resume(struct device *dev)
> +{
> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> +
> + if (device_may_wakeup(dev) && !rockchip->wake_by_pcie) {
The use of 'wake_by_pcie' is racy; an interrupt could be in flight (but
not completed), and so it could set 'wake_by_pcie' just after you're
reading this. Then, you'll get a double-disable.
I believe the safe way to handle this would be to use an atomic
test-and-set / test-and-clear approach (either atomic_cmpxchg(), or use
a spinlock).
> + disable_irq(rockchip->wake_irq);
> + disable_irq_wake(rockchip->wake_irq);
> + }
> + return 0;
> +}
> +
> static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
> {
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> @@ -1548,6 +1605,7 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> }
>
> static const struct dev_pm_ops rockchip_pcie_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend, rockchip_pcie_resume)
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend_noirq,
> rockchip_pcie_resume_noirq)
> };
Brian
hi shawn,
On 08/16/2017 05:00 PM, Shawn Lin wrote:
> Hi Jeffy,
>
> On 2017/8/16 15:52, Jeffy Chen wrote:
>> Add support for PCIE_WAKE pin in rockchip pcie driver.
>>
>> Signed-off-by: Jeffy Chen <[email protected]>
>> ---
>>
>> drivers/pci/host/pcie-rockchip.c | 58
>> ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 58 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-rockchip.c
>> b/drivers/pci/host/pcie-rockchip.c
>> index 7bb9870f6d8c..f969a6d3cd85 100644
>> --- a/drivers/pci/host/pcie-rockchip.c
>> +++ b/drivers/pci/host/pcie-rockchip.c
>> @@ -38,6 +38,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/reset.h>
>> #include <linux/regmap.h>
>> +#include <linux/suspend.h>
>> /*
>> * The upper 16 bits of PCIE_CLIENT_CONFIG are a write mask for the
>> lower 16
>> @@ -226,6 +227,8 @@ struct rockchip_pcie {
>> struct regulator *vpcie1v8; /* 1.8V power supply */
>> struct regulator *vpcie0v9; /* 0.9V power supply */
>> struct gpio_desc *ep_gpio;
>> + int wake_irq;
>> + bool wake_by_pcie;
>> u32 lanes;
>> u8 root_bus_nr;
>> int link_gen;
>> @@ -853,6 +856,20 @@ static void
>> rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
>> chained_irq_exit(chip, desc);
>> }
>> +static irqreturn_t rockchip_pcie_wake_irq_handler(int irq, void *arg)
>> +{
>> + struct rockchip_pcie *rockchip = arg;
>> +
>> + rockchip->wake_by_pcie = true;
>> +
>> + disable_irq_nosync(rockchip->wake_irq);
>> + disable_irq_wake(rockchip->wake_irq);
>> +
>> + pm_wakeup_event(rockchip->dev, 0);
>> + pm_system_wakeup();
>> +
>> + return IRQ_HANDLED;
>> +}
>> /**
>> * rockchip_pcie_parse_dt - Parse Device Tree
>> @@ -868,6 +885,7 @@ static int rockchip_pcie_parse_dt(struct
>> rockchip_pcie *rockchip)
>> struct resource *regs;
>> int irq;
>> int err;
>> + bool wakeup = 0;
>> regs = platform_get_resource_byname(pdev,
>> IORESOURCE_MEM,
>> @@ -1018,6 +1036,21 @@ static int rockchip_pcie_parse_dt(struct
>> rockchip_pcie *rockchip)
>> return err;
>> }
>> + rockchip->wake_irq = platform_get_irq_byname(pdev, "wake");
>> + if (rockchip->wake_irq >= 0) {
>> + err = devm_request_irq(dev, rockchip->wake_irq,
>> + rockchip_pcie_wake_irq_handler,
>> + 0, "pcie-wake", rockchip);
>> + if (err) {
>> + dev_err(dev, "failed to request PCIe wake IRQ\n");
>> + return err;
>
> This is optional, so I'm not sure if we should prevent the driver to
> probe?
it would only break probe when there's a wake irq, but failed to enable
it. but the wake irq itself is optional
>
>> + }
>> +
>> + disable_irq(rockchip->wake_irq);
>> + wakeup = device_property_read_bool(dev, "wakeup-source");
>
> The purpose we add this, is for ep to wakeup the system, so why not
> always treate it as a wakeup source.
it's an extra option to disable wake by wakeup-source property
>
>> + }
>> + device_init_wakeup(dev, wakeup);
>> +
>> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>> if (IS_ERR(rockchip->vpcie3v3)) {
>> if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
>> @@ -1270,6 +1303,30 @@ static int rockchip_pcie_wait_l2(struct
>> rockchip_pcie *rockchip)
>> return 0;
>> }
>> +static int __maybe_unused rockchip_pcie_suspend(struct device *dev)
>> +{
>> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>> +
>> + rockchip->wake_by_pcie = false;
>> +
>> + if (device_may_wakeup(dev)) {
>> + enable_irq_wake(rockchip->wake_irq);
>> + enable_irq(rockchip->wake_irq);
>> + }
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused rockchip_pcie_resume(struct device *dev)
>> +{
>> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>> +
>> + if (device_may_wakeup(dev) && !rockchip->wake_by_pcie) {
>
> I don't get this that why we need to check wake_by_pcie here?
>
it's to avoid double disable when it got a irq before resume
>> + disable_irq(rockchip->wake_irq);
>> + disable_irq_wake(rockchip->wake_irq);
>> + }
>> + return 0;
>> +}
>> +
>> static int __maybe_unused rockchip_pcie_suspend_noirq(struct device
>> *dev)
>> {
>> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>> @@ -1548,6 +1605,7 @@ static int rockchip_pcie_remove(struct
>> platform_device *pdev)
>> }
>> static const struct dev_pm_ops rockchip_pcie_pm_ops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend, rockchip_pcie_resume)
>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend_noirq,
>> rockchip_pcie_resume_noirq)
>> };
>>
>
>
>
Hi Shawn,
On 08/16/2017 04:35 PM, Shawn Lin wrote:
> Hi Jeffy
>
> On 2017/8/16 15:52, Jeffy Chen wrote:
>> Add an optional interrupt for PCIE_WAKE pin.
>>
>> Signed-off-by: Jeffy Chen <[email protected]>
>> ---
>>
>> Documentation/devicetree/bindings/pci/rockchip-pcie.txt | 11
>> +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> index 1453a734c2f5..6ef9903567db 100644
>> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> @@ -22,10 +22,13 @@ Required properties:
>> - phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
>> - phy-names: MUST be "pcie-phy".
>> - 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"
>
> It would be better to introduce interrupts-extended and show a example
> here. :)
ok, will add it in next version
>
>> - resets: Must contain seven entries for each entry in reset-names.
>> See ../reset/reset.txt for details.
>> - reset-names: Must include the following names
>>
>
>
>
Hi Brian,
i've tried dev_pm_set_dedicated_wake_irq, it doesn't work well on
upstream kernel(for level irq).
it looks like we would delay real set trigger type to request irq after
this commit:
1e2a7d78499e irqdomain: Don't set type when mapping an IRQ
so calling irq_set_status_flags before request irq would lose trigger
type setting.
things work well after reverted that commit, so i will send my patch
soon, and ask irq people about it :)
On 08/17/2017 01:49 AM, Brian Norris wrote:
> Hi,
>
> On Wed, Aug 16, 2017 at 03:52:22PM +0800, Jeffy Chen wrote:
>> Add support for PCIE_WAKE pin in rockchip pcie driver.
>>
>> Signed-off-by: Jeffy Chen <[email protected]>
>> ---
>>
>> drivers/pci/host/pcie-rockchip.c | 58 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 58 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
>> index 7bb9870f6d8c..f969a6d3cd85 100644
>> --- a/drivers/pci/host/pcie-rockchip.c
>> +++ b/drivers/pci/host/pcie-rockchip.c
>> @@ -38,6 +38,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/reset.h>
>> #include <linux/regmap.h>
>> +#include <linux/suspend.h>
>>
>> /*
>> * The upper 16 bits of PCIE_CLIENT_CONFIG are a write mask for the lower 16
>> @@ -226,6 +227,8 @@ struct rockchip_pcie {
>> struct regulator *vpcie1v8; /* 1.8V power supply */
>> struct regulator *vpcie0v9; /* 0.9V power supply */
>> struct gpio_desc *ep_gpio;
>> + int wake_irq;
>> + bool wake_by_pcie;
>> u32 lanes;
>> u8 root_bus_nr;
>> int link_gen;
>> @@ -853,6 +856,20 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
>> chained_irq_exit(chip, desc);
>> }
>>
>> +static irqreturn_t rockchip_pcie_wake_irq_handler(int irq, void *arg)
>> +{
>> + struct rockchip_pcie *rockchip = arg;
>> +
>> + rockchip->wake_by_pcie = true;
>> +
>> + disable_irq_nosync(rockchip->wake_irq);
>> + disable_irq_wake(rockchip->wake_irq);
>> +
>> + pm_wakeup_event(rockchip->dev, 0);
>> + pm_system_wakeup();
>> +
>> + return IRQ_HANDLED;
>> +}
>>
>> /**
>> * rockchip_pcie_parse_dt - Parse Device Tree
>> @@ -868,6 +885,7 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
>> struct resource *regs;
>> int irq;
>> int err;
>> + bool wakeup = 0;
>
> '0' should be 'false'.
right
>
>>
>> regs = platform_get_resource_byname(pdev,
>> IORESOURCE_MEM,
>> @@ -1018,6 +1036,21 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
>> return err;
>> }
>>
>> + rockchip->wake_irq = platform_get_irq_byname(pdev, "wake");
>> + if (rockchip->wake_irq >= 0) {
>> + err = devm_request_irq(dev, rockchip->wake_irq,
>> + rockchip_pcie_wake_irq_handler,
>> + 0, "pcie-wake", rockchip);
>> + if (err) {
>> + dev_err(dev, "failed to request PCIe wake IRQ\n");
>> + return err;
>> + }
>> +
>> + disable_irq(rockchip->wake_irq);
>
> If you're worried about keeping this disabled at first, you can just use
> this nifty trick (since this isn't a shared interrupt) -- call this
> before requesting the IRQ:
>
> irq_set_status_flags(rockchip->wake_irq, IRQ_NOAUTOEN);
>
> You could also consider using dev_pm_set_dedicated_wake_irq() to handle
> this -- but beware, it still might not quite handle level-triggered
> interrupt properly. I'm pretty sure Tony Lindgren would be happy to get
> testing or patches for that though :) He already sent me something a
> while back but I didn't have time to test it out.
ok, that seems very handy
>
>> + wakeup = device_property_read_bool(dev, "wakeup-source");
>> + }
>> + device_init_wakeup(dev, wakeup);
>
> Shouldn't you call 'device_init_wakeup(dev, false)' on remove()?
ok
>
>> +
>> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>> if (IS_ERR(rockchip->vpcie3v3)) {
>> if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
>> @@ -1270,6 +1303,30 @@ static int rockchip_pcie_wait_l2(struct rockchip_pcie *rockchip)
>> return 0;
>> }
>>
>> +static int __maybe_unused rockchip_pcie_suspend(struct device *dev)
>
> Why do this in suspend() instead of suspend_noirq()? You shouldn't
> really need a separate method here.
>
> Note that this should be a level-triggered interrupt which remains
> asserted, so there should be no chance of "missing" it if you don't
> enable it in time.
>
> And on a related note: if you try the dedicated wake irq approach, this
> will only occur just before the noirq phase anyway, since
> device_wakeup_arm_wake_irqs() is called in dpm_suspend_noirq().
right
>
>> +{
>> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>> +
>> + rockchip->wake_by_pcie = false;
>> +
>> + if (device_may_wakeup(dev)) {
>> + enable_irq_wake(rockchip->wake_irq);
>> + enable_irq(rockchip->wake_irq);
>> + }
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused rockchip_pcie_resume(struct device *dev)
>> +{
>> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>> +
>> + if (device_may_wakeup(dev) && !rockchip->wake_by_pcie) {
>
> The use of 'wake_by_pcie' is racy; an interrupt could be in flight (but
> not completed), and so it could set 'wake_by_pcie' just after you're
> reading this. Then, you'll get a double-disable.
>
> I believe the safe way to handle this would be to use an atomic
> test-and-set / test-and-clear approach (either atomic_cmpxchg(), or use
> a spinlock).
right, and we don't need these if using dev_pm_set_dedicated_wake_irq
>
>> + disable_irq(rockchip->wake_irq);
>> + disable_irq_wake(rockchip->wake_irq);
>> + }
>> + return 0;
>> +}
>> +
>> static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>> {
>> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>> @@ -1548,6 +1605,7 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>> }
>>
>> static const struct dev_pm_ops rockchip_pcie_pm_ops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend, rockchip_pcie_resume)
>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend_noirq,
>> rockchip_pcie_resume_noirq)
>> };
>
> Brian
>
>
>