Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1713919 > unrolled thread

[RFC PATCH v2 0/3] PCI: rockchip: Move PCIE_WAKE handling into rockchip pcie driver

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v2 0/3] PCI: rockchip: Move PCIE_WAKE handling into rockchip pcie driver Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-08-17 14:10 +0200
    [RFC PATCH v2 3/3] arm64: dts: rockchip: Handle pcie wake in pcie driver for Gru Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-08-17 14:10 +0200
    [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-08-17 14:10 +0200
      Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq Shawn Lin <shawn.lin@rock-chips.com> - 2017-08-18 09:30 +0200
        Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq jeffy <jeffy.chen@rock-chips.com> - 2017-08-18 10:40 +0200
      Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq Brian Norris <briannorris@chromium.org> - 2017-08-18 19:10 +0200
        Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq Brian Norris <briannorris@chromium.org> - 2017-08-18 19:10 +0200
        Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq jeffy <jeffy.chen@rock-chips.com> - 2017-08-18 19:50 +0200
          Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq Tony Lindgren <tony@atomide.com> - 2017-08-18 20:30 +0200
        Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq Tony Lindgren <tony@atomide.com> - 2017-08-18 20:20 +0200
          Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq jeffy <jeffy.chen@rock-chips.com> - 2017-08-18 22:10 +0200
            Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq Tony Lindgren <tony@atomide.com> - 2017-08-22 19:30 +0200
              Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq jeffy <jeffy.chen@rock-chips.com> - 2017-08-23 03:40 +0200
                Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq Brian Norris <briannorris@chromium.org> - 2017-08-23 04:00 +0200
                  Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq jeffy <jeffy.chen@rock-chips.com> - 2017-08-23 04:20 +0200

#1713919 — [RFC PATCH v2 0/3] 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 v2:
Use dev_pm_set_dedicated_wake_irq
        -- Suggested by Brian Norris <briannorris@chromium.com>

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

[toc] | [next] | [standalone]

#1713920 — [RFC PATCH v2 3/3] arm64: dts: rockchip: Handle pcie wake in pcie driver for Gru

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 <jeffy.chen@rock-chips.com>
---

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

[toc] | [prev] | [next] | [standalone]

#1713928 — [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

Add support for PCIE_WAKE pin in rockchip pcie driver.

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

Changes in v2:
Use dev_pm_set_dedicated_wake_irq
        -- Suggested by Brian Norris <briannorris@chromium.com>

 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

[toc] | [prev] | [next] | [standalone]

#1714629 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

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 <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
>          -- Suggested by Brian Norris <briannorris@chromium.com>
> 
>   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);
> 

[toc] | [prev] | [next] | [standalone]

#1714696 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

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

[toc] | [prev] | [next] | [standalone]

#1715323 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

+ 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 <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
>         -- Suggested by Brian Norris <briannorris@chromium.com>
> 
>  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
> 
> 

[toc] | [prev] | [next] | [standalone]

#1715325 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

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 <jeffy.chen@rock-chips.com>
> > ---
> > 
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> >         -- Suggested by Brian Norris <briannorris@chromium.com>

Oops, I forgot you sent a v3. Same questions/comments apply there
though.

[toc] | [prev] | [next] | [standalone]

#1715347 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

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

[toc] | [prev] | [next] | [standalone]

#1715365 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

* jeffy <jeffy.chen@rock-chips.com> [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

[toc] | [prev] | [next] | [standalone]

#1715362 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

* Brian Norris <briannorris@chromium.org> [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 <jeffy.chen@rock-chips.com>
> > ---
> > 
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> >         -- Suggested by Brian Norris <briannorris@chromium.com>
> > 
> >  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

[toc] | [prev] | [next] | [standalone]

#1715427 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

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

[toc] | [prev] | [next] | [standalone]

#1717607 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

* jeffy <jeffy.chen@rock-chips.com> [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

[toc] | [prev] | [next] | [standalone]

#1717951 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

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

[toc] | [prev] | [next] | [standalone]

#1717974 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

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

[toc] | [prev] | [next] | [standalone]

#1717980 — Re: [RFC PATCH v2 1/3] PCI: rockchip: Add support for pcie wake irq

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web