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 v2:
Use dev_pm_set_dedicated_wake_irq
-- Suggested by Brian Norris <[email protected]>
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 | 20 ++++++++++++--------
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 15 +++++++++------
drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
3 files changed, 33 insertions(+), 15 deletions(-)
--
2.11.0
Add an optional interrupt for PCIE_WAKE pin.
Signed-off-by: Jeffy Chen <[email protected]>
---
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 1453a734c2f5..edd779f842fa 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
@@ -76,10 +79,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
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]>
---
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
Add support for PCIE_WAKE pin in rockchip pcie driver.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v2:
Use dev_pm_set_dedicated_wake_irq
-- Suggested by Brian Norris <[email protected]>
drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 7bb9870f6d8c..c2b973c738fe 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -36,6 +36,7 @@
#include <linux/pci_ids.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
#include <linux/reset.h>
#include <linux/regmap.h>
@@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
chained_irq_exit(chip, desc);
}
-
/**
* rockchip_pcie_parse_dt - Parse Device Tree
* @rockchip: PCIe port information
@@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
return err;
}
+ device_init_wakeup(dev, true);
+ irq = platform_get_irq_byname(pdev, "wake");
+ if (irq >= 0) {
+ err = dev_pm_set_dedicated_wake_irq(dev, irq);
+ if (err)
+ dev_err(dev, "failed to setup PCIe wake IRQ\n");
+ }
+
rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
if (IS_ERR(rockchip->vpcie3v3)) {
if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
@@ -1524,6 +1532,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
+
pci_stop_root_bus(rockchip->root_bus);
pci_remove_root_bus(rockchip->root_bus);
pci_unmap_iospace(rockchip->io);
--
2.11.0
Hi Jeffy
On 2017/8/17 20:04, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <[email protected]>
>
> drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 7bb9870f6d8c..c2b973c738fe 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -36,6 +36,7 @@
> #include <linux/pci_ids.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/reset.h>
> #include <linux/regmap.h>
>
> @@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> -
> /**
> * rockchip_pcie_parse_dt - Parse Device Tree
> * @rockchip: PCIe port information
> @@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wake");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> + if (err)
> + dev_err(dev, "failed to setup PCIe wake IRQ\n");
> + }
> +
> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> if (IS_ERR(rockchip->vpcie3v3)) {
> if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> @@ -1524,6 +1532,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> +
Looks good overall but I think we need this on the
error handling path of rockchip_pcie_probe as well?
> pci_stop_root_bus(rockchip->root_bus);
> pci_remove_root_bus(rockchip->root_bus);
> pci_unmap_iospace(rockchip->io);
>
Hi Shawn,
On 08/18/2017 03:23 PM, Shawn Lin wrote:
>>
>> @@ -1524,6 +1532,9 @@ static int rockchip_pcie_remove(struct
>> platform_device *pdev)
>> struct device *dev = &pdev->dev;
>> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>> + dev_pm_clear_wake_irq(dev);
>> + device_init_wakeup(dev, false);
>> +
>
> Looks good overall but I think we need this on the
> error handling path of rockchip_pcie_probe as well?
hmm, right, will fix it, thanks.
and i also notice some other missing error handling, will fix them too:)
+ Tony
On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <[email protected]>
>
> drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 7bb9870f6d8c..c2b973c738fe 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -36,6 +36,7 @@
> #include <linux/pci_ids.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/reset.h>
> #include <linux/regmap.h>
>
> @@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> -
> /**
> * rockchip_pcie_parse_dt - Parse Device Tree
> * @rockchip: PCIe port information
> @@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wake");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
Did you test that this works out correctly as a level-triggered
interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
keep firing. See:
static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
{
struct wake_irq *wirq = _wirq;
int res;
/* Maybe abort suspend? */
if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
pm_wakeup_event(wirq->dev, 0);
return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
}
...
This could cause some kind of an IRQ storm, including a lockup or
significant slowdown, I think.
BTW, in another context, Tony suggested we might need to fix up the IRQ flags
like this:
int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
{
...
err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
- IRQF_ONESHOT, dev_name(dev), wirq);
+ IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
But IIUC, that's not actually necessary, because __setup_irq()
automatically configures the trigger type if the driver didn't request
one explicitly.
Brian
> + if (err)
> + dev_err(dev, "failed to setup PCIe wake IRQ\n");
> + }
> +
> rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> if (IS_ERR(rockchip->vpcie3v3)) {
> if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> @@ -1524,6 +1532,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> +
> pci_stop_root_bus(rockchip->root_bus);
> pci_remove_root_bus(rockchip->root_bus);
> pci_unmap_iospace(rockchip->io);
> --
> 2.11.0
>
>
On Fri, Aug 18, 2017 at 10:01:07AM -0700, Brian Norris wrote:
> + Tony
>
> On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin in rockchip pcie driver.
> >
> > Signed-off-by: Jeffy Chen <[email protected]>
> > ---
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <[email protected]>
Oops, I forgot you sent a v3. Same questions/comments apply there
though.
Hi Brian,
On 08/19/2017 01:01 AM, Brian Norris wrote:
> Did you test that this works out correctly as a level-triggered
> interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
> keep firing. See:
>
> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> {
> struct wake_irq *wirq = _wirq;
> int res;
>
> /* Maybe abort suspend? */
> if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> pm_wakeup_event(wirq->dev, 0);
>
> return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> }
> ...
>
> This could cause some kind of an IRQ storm, including a lockup or
> significant slowdown, I think.
hmmm, right, but as i replied at cros partner issue, this irq handle
might not be called actually...
in my test on cros 4.4 kernel, it would break by irq_may_run(returning
false):
static bool irq_may_run(struct irq_desc *desc)
{
...
/*
* If the interrupt is an armed wakeup source, mark it pending
* and suspended, disable it and notify the pm core about the
* event.
*/
if (irq_pm_check_wakeup(desc))
return false;
bool irq_pm_check_wakeup(struct irq_desc *desc)
{
if (irqd_is_wakeup_armed(&desc->irq_data)) {
irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
desc->istate |= IRQS_SUSPENDED | IRQS_PENDING;
desc->depth++;
irq_disable(desc); <--- disabled here
pm_system_irq_wakeup(irq_desc_get_irq(desc));
return true;
and for irqd_is_wakeup_armed:
static bool suspend_device_irq(struct irq_desc *desc)
{
...
if (irqd_is_wakeup_set(&desc->irq_data)) {
irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); <-- set
irqd_is_wakeup_armed here
void dpm_noirq_begin(void)
{
cpuidle_pause();
device_wakeup_arm_wake_irqs();
suspend_device_irqs();
so unless we get an irq between device_wakeup_arm_wake_irqs and
suspend_device_irq, the irq_pm_check_wakeup would not let us get to
handle_threaded_wake_irq...
>
> BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> like this:
>
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> ...
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
>
> But IIUC, that's not actually necessary, because __setup_irq()
> automatically configures the trigger type if the driver didn't request
> one explicitly.
actually this would not work...irq_get_trigger_type would return zero
due to a bug which we have a patch for it already:
9908207 New [tip:irq/urgent] genirq: Restore trigger settings
in irq_modify_status()
BTW, using dev_name for the name of this wake irq seems not very
convenient...maybe add a ":wake" suffix?
>
> Brian
* Brian Norris <[email protected]> [170818 10:01]:
> + Tony
>
> On Thu, Aug 17, 2017 at 08:04:29PM +0800, Jeffy Chen wrote:
> > Add support for PCIE_WAKE pin in rockchip pcie driver.
> >
> > Signed-off-by: Jeffy Chen <[email protected]>
> > ---
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <[email protected]>
> >
> > drivers/pci/host/pcie-rockchip.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 7bb9870f6d8c..c2b973c738fe 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -36,6 +36,7 @@
> > #include <linux/pci_ids.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_wakeirq.h>
> > #include <linux/reset.h>
> > #include <linux/regmap.h>
> >
> > @@ -853,7 +854,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
> > chained_irq_exit(chip, desc);
> > }
> >
> > -
> > /**
> > * rockchip_pcie_parse_dt - Parse Device Tree
> > * @rockchip: PCIe port information
> > @@ -1018,6 +1018,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + device_init_wakeup(dev, true);
> > + irq = platform_get_irq_byname(pdev, "wake");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
>
> Did you test that this works out correctly as a level-triggered
> interrupt? IIUC, the dummy handler won't mask the interrupt, so it might
> keep firing. See:
>
> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> {
> struct wake_irq *wirq = _wirq;
> int res;
>
> /* Maybe abort suspend? */
> if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> pm_wakeup_event(wirq->dev, 0);
>
> return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> }
> ...
>
> This could cause some kind of an IRQ storm, including a lockup or
> significant slowdown, I think.
Hmm yeah that should be checked. The test cases I have are all
edge interrupts where there is no status whatsoever after the
wake-up event except which irq fired.
To me it seems that the wakeirq consumer driver should be able
to clear the level wakeirq in it's runtime_resume, or even better,
maybe all it takes is just let the consumer driver's irq handler
clear the level wakeirq when it runs after runtime_resume.
> BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> like this:
>
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> ...
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
>
> But IIUC, that's not actually necessary, because __setup_irq()
> automatically configures the trigger type if the driver didn't request
> one explicitly.
OK so we should make sure that the triggering is parsed
properly when passed in from device tree.
Regards,
Tony
* jeffy <[email protected]> [170818 11:05]:
> On 08/19/2017 01:01 AM, Brian Norris wrote:
> > BTW, in another context, Tony suggested we might need to fix up the IRQ flags
> > like this:
> >
> > int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> > {
> > ...
> > err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> > - IRQF_ONESHOT, dev_name(dev), wirq);
> > + IRQF_ONESHOT | irq_get_trigger_type(irq), dev_name(dev), wirq);
> >
> > But IIUC, that's not actually necessary, because __setup_irq()
> > automatically configures the trigger type if the driver didn't request
> > one explicitly.
>
> actually this would not work...irq_get_trigger_type would return zero due to
> a bug which we have a patch for it already:
>
> 9908207 New [tip:irq/urgent] genirq: Restore trigger settings in
> irq_modify_status()
Thanks for that information. So it seems we can leave out the
irq_get_trigger_type() here then? Might be worth checking
that it really does get populated though :)
> BTW, using dev_name for the name of this wake irq seems not very
> convenient...maybe add a ":wake" suffix?
Good idea, will take a look.
Regards,
Tony
Hi guys,
On 08/19/2017 02:14 AM, Tony Lindgren wrote:
>> static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
>> >{
>> > struct wake_irq *wirq = _wirq;
>> > int res;
>> >
>> > /* Maybe abort suspend? */
>> > if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
>> > pm_wakeup_event(wirq->dev, 0);
>> >
>> > return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
>> > }
>> >...
>> >
>> >This could cause some kind of an IRQ storm, including a lockup or
>> >significant slowdown, I think.
> Hmm yeah that should be checked. The test cases I have are all
> edge interrupts where there is no status whatsoever after the
> wake-up event except which irq fired.
>
> To me it seems that the wakeirq consumer driver should be able
> to clear the level wakeirq in it's runtime_resume, or even better,
> maybe all it takes is just let the consumer driver's irq handler
> clear the level wakeirq when it runs after runtime_resume.
>
i did some tests about it:
[ 70.335883] device_wakeup_arm_wake_irqs <-- enable wake irq
[ 70.335932] handle_threaded_wake_irq
...<--- a lot of wake irq handler log
[ 70.335965] suspend_device_irq
[ 70.335987] irq_pm_check_wakeup <--- wake and disable wake irq
...<--- no wake irq handler log
[ 70.336173] resume_irqs <-- enable wake irq
[ 70.336480] handle_threaded_wake_irq
...<--- a lot of wake irq handler log
[ 70.336600] device_wakeup_disarm_wake_irqs < disable wake irq
...<--- no wake irq handler log
so it would indeed possible to get irq storm in
device_wakeup_arm_wake_irqs to suspend_device_irq
and resume_irqs to device_wakeup_disarm_wake_irqs.
a simple workaround would be:
enable_irq_wake
suspend_device_irq
enable_irq
...irq fired, irq_pm_check_wakeup disabled irq
disable_irq
resume_irqs
disable_irq_wake
and i have a hacky patch for that, which works well:
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -1308,6 +1308,8 @@ static int __maybe_unused
rockchip_pcie_suspend_noirq(struct
device *dev)
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
+ dev_pm_enable_wake_irq(dev);
+
return ret;
}
@@ -1316,6 +1318,8 @@ static int __maybe_unused
rockchip_pcie_resume_noirq(struct d
evice *dev)
struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
int err;
+ dev_pm_disable_wake_irq(dev);
+
@ -323,7 +324,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
return;
if (device_may_wakeup(wirq->dev)) {
- if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
+ if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
enable_irq(wirq->irq);
enable_irq_wake(wirq->irq);
@@ -345,7 +346,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
if (device_may_wakeup(wirq->dev)) {
disable_irq_wake(wirq->irq);
- if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
+ if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
disable_irq_nosync(wirq->irq);
}
which is basically move enable_irq and disable_irq to driver noirq
stages, to avoid:
1/ not disabled by irq_pm_check_wakeup when it first fired
2/ re-enabled by resume_irq when it disabled by irq_pm_check_wakeup
with that hack, i no longer saw the irq storm, and the irq handler would
never be called:
[ 9.693385] device_wakeup_arm_wake_irqs
[ 9.697130] suspend_device_irq
<--- suspend noirq, enable wake irq
[ 9.716569] irq_pm_check_wakeup disable the wake irq
<--- resume noirq, disable wake irq
[ 9.968115] resume_irqs <-- enable wake irq(not actually enable,
since disabled twice)
[ 10.193072] device_wakeup_disarm_wake_irqs
* jeffy <[email protected]> [170818 13:05]:
> Hi guys,
>
> On 08/19/2017 02:14 AM, Tony Lindgren wrote:
> > > static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> > > >{
> > > > struct wake_irq *wirq = _wirq;
> > > > int res;
> > > >
> > > > /* Maybe abort suspend? */
> > > > if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
> > > > pm_wakeup_event(wirq->dev, 0);
> > > >
> > > > return IRQ_HANDLED; <--- We can return here, with the trigger still asserted
> > > > }
> > > >...
> > > >
> > > >This could cause some kind of an IRQ storm, including a lockup or
> > > >significant slowdown, I think.
> > Hmm yeah that should be checked. The test cases I have are all
> > edge interrupts where there is no status whatsoever after the
> > wake-up event except which irq fired.
> >
> > To me it seems that the wakeirq consumer driver should be able
> > to clear the level wakeirq in it's runtime_resume, or even better,
> > maybe all it takes is just let the consumer driver's irq handler
> > clear the level wakeirq when it runs after runtime_resume.
> >
>
> i did some tests about it:
> [ 70.335883] device_wakeup_arm_wake_irqs <-- enable wake irq
> [ 70.335932] handle_threaded_wake_irq
> ...<--- a lot of wake irq handler log
> [ 70.335965] suspend_device_irq
> [ 70.335987] irq_pm_check_wakeup <--- wake and disable wake irq
> ...<--- no wake irq handler log
> [ 70.336173] resume_irqs <-- enable wake irq
> [ 70.336480] handle_threaded_wake_irq
> ...<--- a lot of wake irq handler log
> [ 70.336600] device_wakeup_disarm_wake_irqs < disable wake irq
> ...<--- no wake irq handler log
>
>
> so it would indeed possible to get irq storm in device_wakeup_arm_wake_irqs
> to suspend_device_irq
> and resume_irqs to device_wakeup_disarm_wake_irqs.
>
>
> a simple workaround would be:
> enable_irq_wake
> suspend_device_irq
> enable_irq
> ...irq fired, irq_pm_check_wakeup disabled irq
> disable_irq
> resume_irqs
> disable_irq_wake
>
>
>
>
> and i have a hacky patch for that, which works well:
>
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -1308,6 +1308,8 @@ static int __maybe_unused
> rockchip_pcie_suspend_noirq(struct
> device *dev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_enable_wake_irq(dev);
> +
> return ret;
> }
>
> @@ -1316,6 +1318,8 @@ static int __maybe_unused
> rockchip_pcie_resume_noirq(struct d
> evice *dev)
> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> int err;
>
> + dev_pm_disable_wake_irq(dev);
> +
>
>
> @ -323,7 +324,7 @@ void dev_pm_arm_wake_irq(struct wake_irq *wirq)
> return;
>
> if (device_may_wakeup(wirq->dev)) {
> - if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> + if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> enable_irq(wirq->irq);
>
> enable_irq_wake(wirq->irq);
> @@ -345,7 +346,7 @@ void dev_pm_disarm_wake_irq(struct wake_irq *wirq)
> if (device_may_wakeup(wirq->dev)) {
> disable_irq_wake(wirq->irq);
>
> - if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> + if (0 && wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED)
> disable_irq_nosync(wirq->irq);
> }
>
>
>
> which is basically move enable_irq and disable_irq to driver noirq stages,
> to avoid:
> 1/ not disabled by irq_pm_check_wakeup when it first fired
> 2/ re-enabled by resume_irq when it disabled by irq_pm_check_wakeup
>
>
> with that hack, i no longer saw the irq storm, and the irq handler would
> never be called:
>
> [ 9.693385] device_wakeup_arm_wake_irqs
> [ 9.697130] suspend_device_irq
> <--- suspend noirq, enable wake irq
> [ 9.716569] irq_pm_check_wakeup disable the wake irq
> <--- resume noirq, disable wake irq
> [ 9.968115] resume_irqs <-- enable wake irq(not actually enable, since
> disabled twice)
> [ 10.193072] device_wakeup_disarm_wake_irqs
OK, let's fix any wakeriq ordering issues to make it more
usable. Sounds like in your case the wakeirq needs to be enabled
late and disabled early, while in my test cases I can keep it
enabled basically any time.
If this is for suspend/resume, You could just register the
wakeirq on suspend and then remove it on resume. We do have at
least network drivers doing device_init_wakeup(dev, true) and
device_init_wakeup(dev, false) as needed for WOL, see for example
bfin_mac_ethtool_setwol().
Regards,
Tony
Hi Tony,
On 08/23/2017 01:26 AM, Tony Lindgren wrote:
> OK, let's fix any wakeriq ordering issues to make it more
> usable. Sounds like in your case the wakeirq needs to be enabled
> late and disabled early, while in my test cases I can keep it
> enabled basically any time.
yes, in my case it's a level triggered irq, which needs to be disabled
when receive it(by irq_pm_check_wakeup(my hack) or inside of the custom
irq handler)
and for eage irq, maybe we should enable it right after(or before) the
driver activate wake function(for example activate WOWLAN or WOLAN),
otherwise would it be possible to miss some irqs(triggered before we
actually enable the wake irq)?
>
> If this is for suspend/resume, You could just register the
> wakeirq on suspend and then remove it on resume. We do have at
> least network drivers doing device_init_wakeup(dev, true) and
> device_init_wakeup(dev, false) as needed for WOL, see for example
> bfin_mac_ethtool_setwol().
>
> Regards,
>
> Tony
>
>
>
>
Hi Jeffy,
On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> and for eage irq, maybe we should enable it right after(or before)
> the driver activate wake function(for example activate WOWLAN or
> WOLAN), otherwise would it be possible to miss some irqs(triggered
> before we actually enable the wake irq)?
I already mentioned this: for the PCI case, the specification explicitly
says that the WAKE# pin must remain asserted until the system wakes and
resets the link. So we don't have this problem.
But it is probably still useful to make sure there's a well-defined
point at which these interrupts are armed, so that if a device driver
does care, it can account for that. Just before suspend_noirq (as it is
today) is probably fine, so if there's some device-level handling that
needs to happen before we get to suspend (but after the wakeirq is
armed), it can go in the device or bus {suspend,resume}_noirq callbacks.
Brian
Hi Brian,
On 08/23/2017 09:57 AM, Brian Norris wrote:
> Hi Jeffy,
>
> On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
>> and for eage irq, maybe we should enable it right after(or before)
>> the driver activate wake function(for example activate WOWLAN or
>> WOLAN), otherwise would it be possible to miss some irqs(triggered
>> before we actually enable the wake irq)?
>
> I already mentioned this: for the PCI case, the specification explicitly
> says that the WAKE# pin must remain asserted until the system wakes and
> resets the link. So we don't have this problem.
Sorry, i means for other use cases of wakeirq, for example sdio wifi
>
> But it is probably still useful to make sure there's a well-defined
> point at which these interrupts are armed, so that if a device driver
> does care, it can account for that. Just before suspend_noirq (as it is
> today) is probably fine, so if there's some device-level handling that
> needs to happen before we get to suspend (but after the wakeirq is
> armed), it can go in the device or bus {suspend,resume}_noirq callbacks.
Yes, then we may need to handle "disable level irq" job in the irq
handler(or runtime resume callback as current wakeirq API suggested) for
irqs received before suspend devices irqs.
>
> Brian
>
>
>
Hi Jeffy, Tony,
On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> Hi Tony,
>
> On 08/23/2017 01:26 AM, Tony Lindgren wrote:
> >OK, let's fix any wakeriq ordering issues to make it more
> >usable. Sounds like in your case the wakeirq needs to be enabled
> >late and disabled early, while in my test cases I can keep it
> >enabled basically any time.
>
> yes, in my case it's a level triggered irq, which needs to be
> disabled when receive it(by irq_pm_check_wakeup(my hack) or inside
> of the custom irq handler)
>
>
> and for eage irq, maybe we should enable it right after(or before)
> the driver activate wake function(for example activate WOWLAN or
> WOLAN), otherwise would it be possible to miss some irqs(triggered
> before we actually enable the wake irq)?
Did this problem ever get resolved? To be clear, I believe the problem
at hand is:
(a) in suspend/resume (not runtime PM; we may not even have runtime PM
support for most PCI devices)
(b) using a level-triggered signal (PCI WAKE# is active low, and it's
nice to avoid certain races by treating it as level-triggered)
And with the current wakeirq code (and the latest version of Jeffy's
patch series, IIUC), I believe the above case can still trigger an
interrupt storm of sorts (it's not usually unrecoverably-stormy, since
it's a threaded IRQ, and we make "enough" progress).
I don't see how "ordering" can really resolve this problem, unless the
ordering is configured such that the interrupt handler never runs (e.g.,
we disable the IRQ before we get out of any "noirq" phase).
Options I can think of:
(1) implement runtime PM callbacks for all PCI devices, where we clear
any PME status and ensure WAKE# stops asserting [1]
(2) synchronize a device's resume() with the dedicated wake IRQ
(3) skip using the dedicated wake IRQ infrastructure and write our own
interrupt handler for this PCI/PM function
Option (1) seems pretty strange; we don't actually want to manage these
devices with runtime PM.
Option (2) could work, but it would probably require sharing more of the
core suspend/resume internals between
drivers/base/power/{wakeirq,main}.c, which may not be desirable. Among
other problems, that seems more fragile.
Option (3) is easy enough, and we already did that once for the first
pass at poorly implementing this WAKE# logic within the mwifiex driver
:)
> >If this is for suspend/resume, You could just register the
> >wakeirq on suspend and then remove it on resume. We do have at
> >least network drivers doing device_init_wakeup(dev, true) and
> >device_init_wakeup(dev, false) as needed for WOL, see for example
> >bfin_mac_ethtool_setwol().
I don't see how that would be good enough. You still have a window of
time while the driver hasn't finished resuming, in which the interrupt
handler might trigger many times.
Brian
[1] Then we also need to fixup handle_threaded_wake_irq(). Currently it
will not even try to resume the device:
/* Maybe abort suspend? */
if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
pm_wakeup_event(wirq->dev, 0);
return IRQ_HANDLED; <--- we exit here
}
/* We don't want RPM_ASYNC or RPM_NOWAIT here */
res = pm_runtime_resume(wirq->dev);
...
Hi,
* Brian Norris <[email protected]> [171219 00:50]:
> On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
>
> Did this problem ever get resolved? To be clear, I believe the problem
> at hand is:
>
> (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> support for most PCI devices)
It seems it should be enough to implement runtime PM in the PCI
controller. Isn't each PCI WAKE# line is wired from each PCI device
to the PCI controller?
Then the PCI controller can figure out from which PCI device the
WAKE# came from.
> Options I can think of:
> (1) implement runtime PM callbacks for all PCI devices, where we clear
> any PME status and ensure WAKE# stops asserting [1]
I don't think this is needed, it should be enough to have just
the PCI controller implement runtime PM :)
Regards,
Tony
+ Rafael to this thread
On Wed, Dec 20, 2017 at 11:19:12AM -0800, Tony Lindgren wrote:
> * Brian Norris <[email protected]> [171219 00:50]:
> > On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> >
> > Did this problem ever get resolved? To be clear, I believe the problem
> > at hand is:
> >
> > (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> > support for most PCI devices)
>
> It seems it should be enough to implement runtime PM in the PCI
> controller. Isn't each PCI WAKE# line is wired from each PCI device
> to the PCI controller?
No, not really. As discussed in later versions of this thread already,
the WAKE# hierarchy is orthogonal to the PCI hierarchy, and I think we
settled that it's reasonable to just consider this as a 1-per-device
thing, with each device "directly" attached to the PM controller. While
sharing could happen, that's something we decided to punt on...didn't
we?
> Then the PCI controller can figure out from which PCI device the
> WAKE# came from.
I'm not completely sure of the details, but I believe this *can* be
determined by PME. But I'm not sure it's guaranteed to be supported,
especially in cases where we already have 1:1 WAKE#. So we should be
*trying* to report this wakeirq info from the device, if possible.
> > Options I can think of:
> > (1) implement runtime PM callbacks for all PCI devices, where we clear
> > any PME status and ensure WAKE# stops asserting [1]
>
> I don't think this is needed, it should be enough to have just
> the PCI controller implement runtime PM :)
Brian
* Brian Norris <[email protected]> [171222 23:23]:
> + Rafael to this thread
>
> On Wed, Dec 20, 2017 at 11:19:12AM -0800, Tony Lindgren wrote:
> > * Brian Norris <[email protected]> [171219 00:50]:
> > > On Wed, Aug 23, 2017 at 09:32:39AM +0800, Jeffy Chen wrote:
> > >
> > > Did this problem ever get resolved? To be clear, I believe the problem
> > > at hand is:
> > >
> > > (a) in suspend/resume (not runtime PM; we may not even have runtime PM
> > > support for most PCI devices)
> >
> > It seems it should be enough to implement runtime PM in the PCI
> > controller. Isn't each PCI WAKE# line is wired from each PCI device
> > to the PCI controller?
>
> No, not really. As discussed in later versions of this thread already,
> the WAKE# hierarchy is orthogonal to the PCI hierarchy, and I think we
> settled that it's reasonable to just consider this as a 1-per-device
> thing, with each device "directly" attached to the PM controller. While
> sharing could happen, that's something we decided to punt on...didn't
> we?
Sounds like we need a confirmation from some hardware people on
this if the PCI device WAKE# can be wired to PCI controller or
to a separate PM controller directly :)
> > Then the PCI controller can figure out from which PCI device the
> > WAKE# came from.
>
> I'm not completely sure of the details, but I believe this *can* be
> determined by PME. But I'm not sure it's guaranteed to be supported,
> especially in cases where we already have 1:1 WAKE#. So we should be
> *trying* to report this wakeirq info from the device, if possible.
If a PCI device has it's WAKE# wired directly to a PM controller
instead of the PCI controller, then it seems that the PCI controller
should be woken up and then presumably the regular PCI device
interrupts will take care of the rest.
Or else I'd say if the driver for the PCI device in some custom
case needs to do something specific, then having that driver
implement PM runtime makes sense. Naturally we want to avoid a
dependency where all the possible drivers would need to implement
PM runtime, I doubt that's needed though.
Regards,
Tony