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

Groups > linux.kernel > #1712745 > unrolled thread

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

Started byJeffy Chen <jeffy.chen@rock-chips.com>
First post2017-08-16 10:00 +0200
Last post2017-08-17 14:10 +0200
Articles 6 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/3] PCI: rockchip: Move PCIE_WAKE handling into rockchip pcie driver Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-08-16 10:00 +0200
    [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-08-16 10:00 +0200
      Re: [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq Shawn Lin <shawn.lin@rock-chips.com> - 2017-08-16 11:10 +0200
        Re: [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq jeffy <jeffy.chen@rock-chips.com> - 2017-08-17 02:30 +0200
      Re: [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq Brian Norris <briannorris@chromium.org> - 2017-08-16 19:50 +0200
        Re: [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq jeffy <jeffy.chen@rock-chips.com> - 2017-08-17 14:10 +0200

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

FromJeffy Chen <jeffy.chen@rock-chips.com>
Date2017-08-16 10:00 +0200
Subject[RFC PATCH 0/3] PCI: rockchip: Move PCIE_WAKE handling into rockchip pcie driver
Message-ID<uf1p7-4gv-7@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromJeffy Chen <jeffy.chen@rock-chips.com>
Date2017-08-16 10:00 +0200
Subject[RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq
Message-ID<uf1p8-4gv-27@gated-at.bofh.it>
In reply to#1712745
Add support for PCIE_WAKE pin in rockchip pcie driver.

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

 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

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

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

FromShawn Lin <shawn.lin@rock-chips.com>
Date2017-08-16 11:10 +0200
SubjectRe: [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq
Message-ID<uf2uR-5aP-9@gated-at.bofh.it>
In reply to#1712746
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 <jeffy.chen@rock-chips.com>
> ---
> 
>   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)
>   };
>

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

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

Fromjeffy <jeffy.chen@rock-chips.com>
Date2017-08-17 02:30 +0200
SubjectRe: [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq
Message-ID<ufgRb-5Ak-1@gated-at.bofh.it>
In reply to#1712783
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 <jeffy.chen@rock-chips.com>
>> ---
>>
>>   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)
>>   };
>>
>
>
>

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

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

FromBrian Norris <briannorris@chromium.org>
Date2017-08-16 19:50 +0200
SubjectRe: [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq
Message-ID<ufaC6-1CF-11@gated-at.bofh.it>
In reply to#1712746
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 <jeffy.chen@rock-chips.com>
> ---
> 
>  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

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

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

Fromjeffy <jeffy.chen@rock-chips.com>
Date2017-08-17 14:10 +0200
SubjectRe: [RFC PATCH 1/3] PCI: rockchip: Add support for pcie wake irq
Message-ID<ufrMC-4Ff-39@gated-at.bofh.it>
In reply to#1713193
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 <jeffy.chen@rock-chips.com>
>> ---
>>
>>   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
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web