* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-09-11 15:10 ` Jeffy Chen
0 siblings, 0 replies; 40+ messages in thread
From: Jeffy Chen @ 2017-09-11 15:10 UTC (permalink / raw)
To: linux-arm-kernel
Add support for PCIE_WAKE pin in rockchip pcie driver.
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
Changes in v5:
Rebase
Changes in v3:
Fix error handling
Changes in v2:
Use dev_pm_set_dedicated_wake_irq
-- Suggested by Brian Norris <briannorris@chromium.com>
drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 9051c6c8fea4..a8b7272597a7 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -37,6 +37,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>
@@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
return err;
}
+ /* Must init wakeup before setting dedicated wakeup irq. */
+ device_init_wakeup(dev, true);
+ irq = platform_get_irq_byname(pdev, "wakeup");
+ if (irq >= 0) {
+ err = dev_pm_set_dedicated_wake_irq(dev, irq);
+ if (err)
+ dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
+ }
+
return 0;
}
@@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
err = rockchip_pcie_parse_dt(rockchip);
if (err)
- return err;
+ goto err_disable_wake;
err = rockchip_pcie_enable_clocks(rockchip);
if (err)
- return err;
+ goto err_disable_wake;
err = rockchip_pcie_set_vpcie(rockchip);
if (err) {
@@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
regulator_disable(rockchip->vpcie0v9);
err_set_vpcie:
rockchip_pcie_disable_clocks(rockchip);
+err_disable_wake:
+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
return err;
}
@@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
return 0;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-09-11 15:10 ` Jeffy Chen
0 siblings, 0 replies; 40+ messages in thread
From: Jeffy Chen @ 2017-09-11 15:10 UTC (permalink / raw)
To: linux-kernel, bhelgaas
Cc: Jeffy Chen, Heiko Stuebner, linux-pci, shawn.lin, briannorris,
dianders, linux-rockchip, linux-arm-kernel
Add support for PCIE_WAKE pin in rockchip pcie driver.
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
Changes in v5:
Rebase
Changes in v3:
Fix error handling
Changes in v2:
Use dev_pm_set_dedicated_wake_irq
-- Suggested by Brian Norris <briannorris@chromium.com>
drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 9051c6c8fea4..a8b7272597a7 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -37,6 +37,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>
@@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
return err;
}
+ /* Must init wakeup before setting dedicated wakeup irq. */
+ device_init_wakeup(dev, true);
+ irq = platform_get_irq_byname(pdev, "wakeup");
+ if (irq >= 0) {
+ err = dev_pm_set_dedicated_wake_irq(dev, irq);
+ if (err)
+ dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
+ }
+
return 0;
}
@@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
err = rockchip_pcie_parse_dt(rockchip);
if (err)
- return err;
+ goto err_disable_wake;
err = rockchip_pcie_enable_clocks(rockchip);
if (err)
- return err;
+ goto err_disable_wake;
err = rockchip_pcie_set_vpcie(rockchip);
if (err) {
@@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
regulator_disable(rockchip->vpcie0v9);
err_set_vpcie:
rockchip_pcie_disable_clocks(rockchip);
+err_disable_wake:
+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
return err;
}
@@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
if (!IS_ERR(rockchip->vpcie0v9))
regulator_disable(rockchip->vpcie0v9);
+ dev_pm_clear_wake_irq(dev);
+ device_init_wakeup(dev, false);
return 0;
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-09-11 15:10 ` Jeffy Chen
(?)
@ 2017-09-12 0:39 ` Shawn Lin
-1 siblings, 0 replies; 40+ messages in thread
From: Shawn Lin @ 2017-09-12 0:39 UTC (permalink / raw)
To: Jeffy Chen
Cc: Heiko Stuebner, linux-pci, shawn.lin, briannorris, dianders,
linux-rockchip, bhelgaas, linux-arm-kernel
On 2017/9/11 23:10, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
LGTM,
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> Changes in v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-09-12 0:39 ` Shawn Lin
0 siblings, 0 replies; 40+ messages in thread
From: Shawn Lin @ 2017-09-12 0:39 UTC (permalink / raw)
To: linux-arm-kernel
On 2017/9/11 23:10, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
LGTM,
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> Changes in v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-09-12 0:39 ` Shawn Lin
0 siblings, 0 replies; 40+ messages in thread
From: Shawn Lin @ 2017-09-12 0:39 UTC (permalink / raw)
To: Jeffy Chen
Cc: bhelgaas, shawn.lin, briannorris, dianders, Heiko Stuebner,
linux-pci, linux-rockchip, linux-arm-kernel
On 2017/9/11 23:10, Jeffy Chen wrote:
> Add support for PCIE_WAKE pin in rockchip pcie driver.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
LGTM,
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> Changes in v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-09-11 15:10 ` Jeffy Chen
(?)
@ 2017-10-13 1:56 ` Brian Norris
-1 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2017-10-13 1:56 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-kernel, bhelgaas, shawn.lin, dianders, Heiko Stuebner,
linux-pci, linux-rockchip, linux-arm-kernel
Hi,
On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
Am I crazy, or should this go inside the 'irq >= 0' conditional?
Otherwise, for the whole series:
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Bjorn, were you planning to pick this up?
Also, the DT binding change conflicts (just simple context) with the
PERST# series I just sent out. Would be good if we could land one or
both :)
Thanks,
Brian
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
> --
> 2.11.0
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 1:56 ` Brian Norris
0 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2017-10-13 1:56 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
Am I crazy, or should this go inside the 'irq >= 0' conditional?
Otherwise, for the whole series:
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Bjorn, were you planning to pick this up?
Also, the DT binding change conflicts (just simple context) with the
PERST# series I just sent out. Would be good if we could land one or
both :)
Thanks,
Brian
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
> --
> 2.11.0
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 1:56 ` Brian Norris
0 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2017-10-13 1:56 UTC (permalink / raw)
To: Jeffy Chen
Cc: Heiko Stuebner, linux-pci, shawn.lin, linux-kernel, dianders,
linux-rockchip, bhelgaas, linux-arm-kernel
Hi,
On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
Am I crazy, or should this go inside the 'irq >= 0' conditional?
Otherwise, for the whole series:
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Bjorn, were you planning to pick this up?
Also, the DT binding change conflicts (just simple context) with the
PERST# series I just sent out. Would be good if we could land one or
both :)
Thanks,
Brian
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
> --
> 2.11.0
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-10-13 1:56 ` Brian Norris
(?)
@ 2017-10-13 2:32 ` Bjorn Helgaas
-1 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 2:32 UTC (permalink / raw)
To: Brian Norris
Cc: Jeffy Chen, Heiko Stuebner, linux-pci, shawn.lin, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
On Thu, Oct 12, 2017 at 06:56:22PM -0700, Brian Norris wrote:
> Hi,
>
> On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> > Rebase
> >
> > Changes in v3:
> > Fix error handling
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
> >
> > drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 9051c6c8fea4..a8b7272597a7 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -37,6 +37,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>
> >
> > @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + /* Must init wakeup before setting dedicated wakeup irq. */
> > + device_init_wakeup(dev, true);
>
> Am I crazy, or should this go inside the 'irq >= 0' conditional?
> Otherwise, for the whole series:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
>
> Bjorn, were you planning to pick this up?
I had already applied this to pci/host-rockchip, but I must have gotten
interrupted before sending the email.
But now that you mention it, I looked at this again and have some
questions. I'll respond to the original patch.
> Also, the DT binding change conflicts (just simple context) with the
> PERST# series I just sent out. Would be good if we could land one or
> both :)
>
> Thanks,
> Brian
>
> > + irq = platform_get_irq_byname(pdev, "wakeup");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> > + if (err)
> > + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >
> > err = rockchip_pcie_parse_dt(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_enable_clocks(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_set_vpcie(rockchip);
> > if (err) {
> > @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > regulator_disable(rockchip->vpcie0v9);
> > err_set_vpcie:
> > rockchip_pcie_disable_clocks(rockchip);
> > +err_disable_wake:
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
> > return err;
> > }
> >
> > @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> > if (!IS_ERR(rockchip->vpcie0v9))
> > regulator_disable(rockchip->vpcie0v9);
> >
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
> > return 0;
> > }
> >
> > --
> > 2.11.0
> >
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 2:32 ` Bjorn Helgaas
0 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 2:32 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Oct 12, 2017 at 06:56:22PM -0700, Brian Norris wrote:
> Hi,
>
> On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> > Rebase
> >
> > Changes in v3:
> > Fix error handling
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
> >
> > drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 9051c6c8fea4..a8b7272597a7 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -37,6 +37,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>
> >
> > @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + /* Must init wakeup before setting dedicated wakeup irq. */
> > + device_init_wakeup(dev, true);
>
> Am I crazy, or should this go inside the 'irq >= 0' conditional?
> Otherwise, for the whole series:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
>
> Bjorn, were you planning to pick this up?
I had already applied this to pci/host-rockchip, but I must have gotten
interrupted before sending the email.
But now that you mention it, I looked at this again and have some
questions. I'll respond to the original patch.
> Also, the DT binding change conflicts (just simple context) with the
> PERST# series I just sent out. Would be good if we could land one or
> both :)
>
> Thanks,
> Brian
>
> > + irq = platform_get_irq_byname(pdev, "wakeup");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> > + if (err)
> > + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >
> > err = rockchip_pcie_parse_dt(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_enable_clocks(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_set_vpcie(rockchip);
> > if (err) {
> > @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > regulator_disable(rockchip->vpcie0v9);
> > err_set_vpcie:
> > rockchip_pcie_disable_clocks(rockchip);
> > +err_disable_wake:
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
> > return err;
> > }
> >
> > @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> > if (!IS_ERR(rockchip->vpcie0v9))
> > regulator_disable(rockchip->vpcie0v9);
> >
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
> > return 0;
> > }
> >
> > --
> > 2.11.0
> >
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 2:32 ` Bjorn Helgaas
0 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 2:32 UTC (permalink / raw)
To: Brian Norris
Cc: Heiko Stuebner, linux-pci, shawn.lin, Jeffy Chen, dianders,
linux-kernel, linux-rockchip, bhelgaas, linux-arm-kernel
On Thu, Oct 12, 2017 at 06:56:22PM -0700, Brian Norris wrote:
> Hi,
>
> On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> > Rebase
> >
> > Changes in v3:
> > Fix error handling
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
> >
> > drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 9051c6c8fea4..a8b7272597a7 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -37,6 +37,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>
> >
> > @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + /* Must init wakeup before setting dedicated wakeup irq. */
> > + device_init_wakeup(dev, true);
>
> Am I crazy, or should this go inside the 'irq >= 0' conditional?
> Otherwise, for the whole series:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
>
> Bjorn, were you planning to pick this up?
I had already applied this to pci/host-rockchip, but I must have gotten
interrupted before sending the email.
But now that you mention it, I looked at this again and have some
questions. I'll respond to the original patch.
> Also, the DT binding change conflicts (just simple context) with the
> PERST# series I just sent out. Would be good if we could land one or
> both :)
>
> Thanks,
> Brian
>
> > + irq = platform_get_irq_byname(pdev, "wakeup");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> > + if (err)
> > + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >
> > err = rockchip_pcie_parse_dt(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_enable_clocks(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_set_vpcie(rockchip);
> > if (err) {
> > @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > regulator_disable(rockchip->vpcie0v9);
> > err_set_vpcie:
> > rockchip_pcie_disable_clocks(rockchip);
> > +err_disable_wake:
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
> > return err;
> > }
> >
> > @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> > if (!IS_ERR(rockchip->vpcie0v9))
> > regulator_disable(rockchip->vpcie0v9);
> >
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
> > return 0;
> > }
> >
> > --
> > 2.11.0
> >
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-10-13 2:32 ` Bjorn Helgaas
@ 2017-10-13 6:31 ` Brian Norris
-1 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2017-10-13 6:31 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jeffy Chen, Heiko Stuebner, linux-pci, shawn.lin, linux-kernel,
dianders, linux-rockchip, bhelgaas, linux-arm-kernel
On Thu, Oct 12, 2017 at 09:32:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 12, 2017 at 06:56:22PM -0700, Brian Norris wrote:
> > Bjorn, were you planning to pick this up?
>
> I had already applied this to pci/host-rockchip, but I must have gotten
> interrupted before sending the email.
You also hadn't merged it into your /next branch either, which is where
I was looking (and basing my other patch set).
> But now that you mention it, I looked at this again and have some
> questions. I'll respond to the original patch.
Brian
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 6:31 ` Brian Norris
0 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2017-10-13 6:31 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Oct 12, 2017 at 09:32:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 12, 2017 at 06:56:22PM -0700, Brian Norris wrote:
> > Bjorn, were you planning to pick this up?
>
> I had already applied this to pci/host-rockchip, but I must have gotten
> interrupted before sending the email.
You also hadn't merged it into your /next branch either, which is where
I was looking (and basing my other patch set).
> But now that you mention it, I looked at this again and have some
> questions. I'll respond to the original patch.
Brian
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-09-11 15:10 ` Jeffy Chen
(?)
@ 2017-10-13 3:04 ` Bjorn Helgaas
-1 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 3:04 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-kernel, bhelgaas, Heiko Stuebner, linux-pci, shawn.lin,
briannorris, dianders, linux-rockchip, linux-arm-kernel,
Rafael J. Wysocki, linux-pm
[+cc Rafael, linux-pm]
On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
because I know anything at all about it, but because there are only
five callers in the whole tree, three of which are in UART code, and
none in anything resembling PCI code.
Is Rockchip really that special, or are we going about this the wrong
way?
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
The above could be structured as:
irq = platform_get_irq_byname(pdev, "wakeup");
if (irq < 0)
return 0;
device_init_wakeup(dev, true);
err = dev_pm_set_dedicated_wake_irq(dev, irq);
if (err) {
dev_pm_clear_wake_irq(dev);
device_init_wakeup(dev, false);
}
return 0;
to unindent the mainline non-error code.
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
I think this error cleanup should be done in rockchip_pcie_setup_irq()
as shown above. There's no real connection between
rockchip_pcie_probe() and the wake setup.
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
> --
> 2.11.0
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 3:04 ` Bjorn Helgaas
0 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 3:04 UTC (permalink / raw)
To: linux-arm-kernel
[+cc Rafael, linux-pm]
On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
because I know anything at all about it, but because there are only
five callers in the whole tree, three of which are in UART code, and
none in anything resembling PCI code.
Is Rockchip really that special, or are we going about this the wrong
way?
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
The above could be structured as:
irq = platform_get_irq_byname(pdev, "wakeup");
if (irq < 0)
return 0;
device_init_wakeup(dev, true);
err = dev_pm_set_dedicated_wake_irq(dev, irq);
if (err) {
dev_pm_clear_wake_irq(dev);
device_init_wakeup(dev, false);
}
return 0;
to unindent the mainline non-error code.
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
I think this error cleanup should be done in rockchip_pcie_setup_irq()
as shown above. There's no real connection between
rockchip_pcie_probe() and the wake setup.
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
> --
> 2.11.0
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 3:04 ` Bjorn Helgaas
0 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 3:04 UTC (permalink / raw)
To: Jeffy Chen
Cc: Heiko Stuebner, linux-pm, linux-pci, shawn.lin, briannorris,
Rafael J. Wysocki, linux-kernel, dianders, linux-rockchip,
bhelgaas, linux-arm-kernel
[+cc Rafael, linux-pm]
On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> Rebase
>
> Changes in v3:
> Fix error handling
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq
> -- Suggested by Brian Norris <briannorris@chromium.com>
>
> drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 9051c6c8fea4..a8b7272597a7 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,6 +37,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>
>
> @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> return err;
> }
>
> + /* Must init wakeup before setting dedicated wakeup irq. */
> + device_init_wakeup(dev, true);
> + irq = platform_get_irq_byname(pdev, "wakeup");
> + if (irq >= 0) {
> + err = dev_pm_set_dedicated_wake_irq(dev, irq);
I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
because I know anything at all about it, but because there are only
five callers in the whole tree, three of which are in UART code, and
none in anything resembling PCI code.
Is Rockchip really that special, or are we going about this the wrong
way?
> + if (err)
> + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> + }
> +
> return 0;
The above could be structured as:
irq = platform_get_irq_byname(pdev, "wakeup");
if (irq < 0)
return 0;
device_init_wakeup(dev, true);
err = dev_pm_set_dedicated_wake_irq(dev, irq);
if (err) {
dev_pm_clear_wake_irq(dev);
device_init_wakeup(dev, false);
}
return 0;
to unindent the mainline non-error code.
> }
>
> @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>
> err = rockchip_pcie_parse_dt(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_disable_wake;
>
> err = rockchip_pcie_set_vpcie(rockchip);
> if (err) {
> @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> regulator_disable(rockchip->vpcie0v9);
> err_set_vpcie:
> rockchip_pcie_disable_clocks(rockchip);
> +err_disable_wake:
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
I think this error cleanup should be done in rockchip_pcie_setup_irq()
as shown above. There's no real connection between
rockchip_pcie_probe() and the wake setup.
> return err;
> }
>
> @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> if (!IS_ERR(rockchip->vpcie0v9))
> regulator_disable(rockchip->vpcie0v9);
>
> + dev_pm_clear_wake_irq(dev);
> + device_init_wakeup(dev, false);
> return 0;
> }
>
> --
> 2.11.0
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-10-13 3:04 ` Bjorn Helgaas
@ 2017-10-13 13:21 ` Rafael J. Wysocki
-1 siblings, 0 replies; 40+ messages in thread
From: Rafael J. Wysocki @ 2017-10-13 13:21 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jeffy Chen, linux-kernel, bhelgaas, Heiko Stuebner, linux-pci,
shawn.lin, briannorris, dianders, linux-rockchip,
linux-arm-kernel, linux-pm, Tony Lindgren
[+cc Tony]
On Friday, October 13, 2017 5:04:41 AM CEST Bjorn Helgaas wrote:
> [+cc Rafael, linux-pm]
>
> On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> > Rebase
> >
> > Changes in v3:
> > Fix error handling
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
> >
> > drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 9051c6c8fea4..a8b7272597a7 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -37,6 +37,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>
> >
> > @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + /* Must init wakeup before setting dedicated wakeup irq. */
> > + device_init_wakeup(dev, true);
> > + irq = platform_get_irq_byname(pdev, "wakeup");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
>
> I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> because I know anything at all about it, but because there are only
> five callers in the whole tree, three of which are in UART code, and
> none in anything resembling PCI code.
>
> Is Rockchip really that special, or are we going about this the wrong
> way?
>
> > + if (err)
> > + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> > + }
> > +
> > return 0;
>
> The above could be structured as:
>
> irq = platform_get_irq_byname(pdev, "wakeup");
> if (irq < 0)
> return 0;
>
> device_init_wakeup(dev, true);
> err = dev_pm_set_dedicated_wake_irq(dev, irq);
> if (err) {
> dev_pm_clear_wake_irq(dev);
> device_init_wakeup(dev, false);
> }
>
> return 0;
>
> to unindent the mainline non-error code.
>
> > }
> >
> > @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >
> > err = rockchip_pcie_parse_dt(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_enable_clocks(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_set_vpcie(rockchip);
> > if (err) {
> > @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > regulator_disable(rockchip->vpcie0v9);
> > err_set_vpcie:
> > rockchip_pcie_disable_clocks(rockchip);
> > +err_disable_wake:
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
>
> I think this error cleanup should be done in rockchip_pcie_setup_irq()
> as shown above. There's no real connection between
> rockchip_pcie_probe() and the wake setup.
>
> > return err;
> > }
> >
> > @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> > if (!IS_ERR(rockchip->vpcie0v9))
> > regulator_disable(rockchip->vpcie0v9);
> >
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
> > return 0;
> > }
> >
>
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 13:21 ` Rafael J. Wysocki
0 siblings, 0 replies; 40+ messages in thread
From: Rafael J. Wysocki @ 2017-10-13 13:21 UTC (permalink / raw)
To: linux-arm-kernel
[+cc Tony]
On Friday, October 13, 2017 5:04:41 AM CEST Bjorn Helgaas wrote:
> [+cc Rafael, linux-pm]
>
> On Mon, Sep 11, 2017 at 11:10:27PM +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 v5:
> > Rebase
> >
> > Changes in v3:
> > Fix error handling
> >
> > Changes in v2:
> > Use dev_pm_set_dedicated_wake_irq
> > -- Suggested by Brian Norris <briannorris@chromium.com>
> >
> > drivers/pci/host/pcie-rockchip.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> > index 9051c6c8fea4..a8b7272597a7 100644
> > --- a/drivers/pci/host/pcie-rockchip.c
> > +++ b/drivers/pci/host/pcie-rockchip.c
> > @@ -37,6 +37,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>
> >
> > @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> > return err;
> > }
> >
> > + /* Must init wakeup before setting dedicated wakeup irq. */
> > + device_init_wakeup(dev, true);
> > + irq = platform_get_irq_byname(pdev, "wakeup");
> > + if (irq >= 0) {
> > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
>
> I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> because I know anything at all about it, but because there are only
> five callers in the whole tree, three of which are in UART code, and
> none in anything resembling PCI code.
>
> Is Rockchip really that special, or are we going about this the wrong
> way?
>
> > + if (err)
> > + dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> > + }
> > +
> > return 0;
>
> The above could be structured as:
>
> irq = platform_get_irq_byname(pdev, "wakeup");
> if (irq < 0)
> return 0;
>
> device_init_wakeup(dev, true);
> err = dev_pm_set_dedicated_wake_irq(dev, irq);
> if (err) {
> dev_pm_clear_wake_irq(dev);
> device_init_wakeup(dev, false);
> }
>
> return 0;
>
> to unindent the mainline non-error code.
>
> > }
> >
> > @@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >
> > err = rockchip_pcie_parse_dt(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_enable_clocks(rockchip);
> > if (err)
> > - return err;
> > + goto err_disable_wake;
> >
> > err = rockchip_pcie_set_vpcie(rockchip);
> > if (err) {
> > @@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > regulator_disable(rockchip->vpcie0v9);
> > err_set_vpcie:
> > rockchip_pcie_disable_clocks(rockchip);
> > +err_disable_wake:
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
>
> I think this error cleanup should be done in rockchip_pcie_setup_irq()
> as shown above. There's no real connection between
> rockchip_pcie_probe() and the wake setup.
>
> > return err;
> > }
> >
> > @@ -1682,6 +1695,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> > if (!IS_ERR(rockchip->vpcie0v9))
> > regulator_disable(rockchip->vpcie0v9);
> >
> > + dev_pm_clear_wake_irq(dev);
> > + device_init_wakeup(dev, false);
> > return 0;
> > }
> >
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-10-13 13:21 ` Rafael J. Wysocki
@ 2017-10-13 17:58 ` Tony Lindgren
-1 siblings, 0 replies; 40+ messages in thread
From: Tony Lindgren @ 2017-10-13 17:58 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Bjorn Helgaas, Jeffy Chen, linux-kernel, bhelgaas,
Heiko Stuebner, linux-pci, shawn.lin, briannorris, dianders,
linux-rockchip, linux-arm-kernel, linux-pm
* Rafael J. Wysocki <rjw@rjwysocki.net> [171013 06:32]:
> [+cc Tony]
>
> On Friday, October 13, 2017 5:04:41 AM CEST Bjorn Helgaas wrote:
> > [+cc Rafael, linux-pm]
> > On Mon, Sep 11, 2017 at 11:10:27PM +0800, Jeffy Chen wrote:
> > > Add support for PCIE_WAKE pin in rockchip pcie driver.
...
> > > @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> > > return err;
> > > }
> > >
> > > + /* Must init wakeup before setting dedicated wakeup irq. */
> > > + device_init_wakeup(dev, true);
> > > + irq = platform_get_irq_byname(pdev, "wakeup");
> > > + if (irq >= 0) {
> > > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> >
> > I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> > because I know anything at all about it, but because there are only
> > five callers in the whole tree, three of which are in UART code, and
> > none in anything resembling PCI code.
Well it should work for any device that can provide an out of band wakeup
interrupt such as a dedicated GPIO line.
> > Is Rockchip really that special, or are we going about this the wrong
> > way?
Maybe this can be set up in a generic way somewhere similar to what
the I2C bus is doing?
See i2c_device_probe() for dev_pm_set_dedicated_wake_irq() in
drivers/i2c/i2c-core-base.c.
Regards,
Tony
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 17:58 ` Tony Lindgren
0 siblings, 0 replies; 40+ messages in thread
From: Tony Lindgren @ 2017-10-13 17:58 UTC (permalink / raw)
To: linux-arm-kernel
* Rafael J. Wysocki <rjw@rjwysocki.net> [171013 06:32]:
> [+cc Tony]
>
> On Friday, October 13, 2017 5:04:41 AM CEST Bjorn Helgaas wrote:
> > [+cc Rafael, linux-pm]
> > On Mon, Sep 11, 2017 at 11:10:27PM +0800, Jeffy Chen wrote:
> > > Add support for PCIE_WAKE pin in rockchip pcie driver.
...
> > > @@ -995,6 +996,15 @@ static int rockchip_pcie_setup_irq(struct rockchip_pcie *rockchip)
> > > return err;
> > > }
> > >
> > > + /* Must init wakeup before setting dedicated wakeup irq. */
> > > + device_init_wakeup(dev, true);
> > > + irq = platform_get_irq_byname(pdev, "wakeup");
> > > + if (irq >= 0) {
> > > + err = dev_pm_set_dedicated_wake_irq(dev, irq);
> >
> > I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> > because I know anything at all about it, but because there are only
> > five callers in the whole tree, three of which are in UART code, and
> > none in anything resembling PCI code.
Well it should work for any device that can provide an out of band wakeup
interrupt such as a dedicated GPIO line.
> > Is Rockchip really that special, or are we going about this the wrong
> > way?
Maybe this can be set up in a generic way somewhere similar to what
the I2C bus is doing?
See i2c_device_probe() for dev_pm_set_dedicated_wake_irq() in
drivers/i2c/i2c-core-base.c.
Regards,
Tony
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-10-13 13:21 ` Rafael J. Wysocki
@ 2017-10-13 18:33 ` jeffy
-1 siblings, 0 replies; 40+ messages in thread
From: jeffy @ 2017-10-13 18:33 UTC (permalink / raw)
To: Rafael J. Wysocki, Bjorn Helgaas
Cc: linux-kernel, bhelgaas, Heiko Stuebner, linux-pci, shawn.lin,
briannorris, dianders, linux-rockchip, linux-arm-kernel,
linux-pm, Tony Lindgren
Hi Rafael,
On 10/13/2017 09:21 PM, Rafael J. Wysocki wrote:
>>
>>I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
>>because I know anything at all about it, but because there are only
>>five callers in the whole tree, three of which are in UART code, and
>>none in anything resembling PCI code.
>>
>>Is Rockchip really that special, or are we going about this the wrong
>>way?
we used to put these codes in the wifi driver, but another wifi vendor
suggests these should go into the pcie driver.
and as tony said, it could go into pcie common code :)
>>
>> > >+ if (err)
>> > >+ dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
>> > >+ }
>> > >+
>> > > return 0;
>>
>>The above could be structured as:
>>
>> irq = platform_get_irq_byname(pdev, "wakeup");
>> if (irq < 0)
>> return 0;
>>
>> device_init_wakeup(dev, true);
>> err = dev_pm_set_dedicated_wake_irq(dev, irq);
>> if (err) {
>> dev_pm_clear_wake_irq(dev);
>> device_init_wakeup(dev, false);
>> }
>>
there's no need to call dev_pm_clear_wake_irq when
dev_pm_set_dedicated_wake_irq failed...and i agree the
device_init_wakeup part, i'll add that in the next version(with brian's
comment too)
>> return 0;
>>
>>to unindent the mainline non-error code.
>>
>> > > }
>> > >
>> > >@@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>> > >
>> > > err = rockchip_pcie_parse_dt(rockchip);
>> > > if (err)
>> > >- return err;
>> > >+ goto err_disable_wake;
>> > >
>> > > err = rockchip_pcie_enable_clocks(rockchip);
>> > > if (err)
>> > >- return err;
>> > >+ goto err_disable_wake;
>> > >
>> > > err = rockchip_pcie_set_vpcie(rockchip);
>> > > if (err) {
>> > >@@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>> > > regulator_disable(rockchip->vpcie0v9);
>> > > err_set_vpcie:
>> > > rockchip_pcie_disable_clocks(rockchip);
>> > >+err_disable_wake:
>> > >+ dev_pm_clear_wake_irq(dev);
>> > >+ device_init_wakeup(dev, false);
>>
>>I think this error cleanup should be done in rockchip_pcie_setup_irq()
>>as shown above. There's no real connection between
>>rockchip_pcie_probe() and the wake setup.
this error handling is like inline "rockchip_pcie_cleanup_irq()", and
they are harmless to be called even if we don't have the wakeup irq :)
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 18:33 ` jeffy
0 siblings, 0 replies; 40+ messages in thread
From: jeffy @ 2017-10-13 18:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi Rafael,
On 10/13/2017 09:21 PM, Rafael J. Wysocki wrote:
>>
>>I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
>>because I know anything at all about it, but because there are only
>>five callers in the whole tree, three of which are in UART code, and
>>none in anything resembling PCI code.
>>
>>Is Rockchip really that special, or are we going about this the wrong
>>way?
we used to put these codes in the wifi driver, but another wifi vendor
suggests these should go into the pcie driver.
and as tony said, it could go into pcie common code :)
>>
>> > >+ if (err)
>> > >+ dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
>> > >+ }
>> > >+
>> > > return 0;
>>
>>The above could be structured as:
>>
>> irq = platform_get_irq_byname(pdev, "wakeup");
>> if (irq < 0)
>> return 0;
>>
>> device_init_wakeup(dev, true);
>> err = dev_pm_set_dedicated_wake_irq(dev, irq);
>> if (err) {
>> dev_pm_clear_wake_irq(dev);
>> device_init_wakeup(dev, false);
>> }
>>
there's no need to call dev_pm_clear_wake_irq when
dev_pm_set_dedicated_wake_irq failed...and i agree the
device_init_wakeup part, i'll add that in the next version(with brian's
comment too)
>> return 0;
>>
>>to unindent the mainline non-error code.
>>
>> > > }
>> > >
>> > >@@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>> > >
>> > > err = rockchip_pcie_parse_dt(rockchip);
>> > > if (err)
>> > >- return err;
>> > >+ goto err_disable_wake;
>> > >
>> > > err = rockchip_pcie_enable_clocks(rockchip);
>> > > if (err)
>> > >- return err;
>> > >+ goto err_disable_wake;
>> > >
>> > > err = rockchip_pcie_set_vpcie(rockchip);
>> > > if (err) {
>> > >@@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>> > > regulator_disable(rockchip->vpcie0v9);
>> > > err_set_vpcie:
>> > > rockchip_pcie_disable_clocks(rockchip);
>> > >+err_disable_wake:
>> > >+ dev_pm_clear_wake_irq(dev);
>> > >+ device_init_wakeup(dev, false);
>>
>>I think this error cleanup should be done in rockchip_pcie_setup_irq()
>>as shown above. There's no real connection between
>>rockchip_pcie_probe() and the wake setup.
this error handling is like inline "rockchip_pcie_cleanup_irq()", and
they are harmless to be called even if we don't have the wakeup irq :)
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-10-13 18:33 ` jeffy
(?)
@ 2017-10-13 19:19 ` Bjorn Helgaas
-1 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 19:19 UTC (permalink / raw)
To: jeffy
Cc: Rafael J. Wysocki, linux-kernel, bhelgaas, Heiko Stuebner,
linux-pci, shawn.lin, briannorris, dianders, linux-rockchip,
linux-arm-kernel, linux-pm, Tony Lindgren
On Sat, Oct 14, 2017 at 02:33:45AM +0800, jeffy wrote:
> Hi Rafael,
>
> On 10/13/2017 09:21 PM, Rafael J. Wysocki wrote:
> >>
> >>I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> >>because I know anything at all about it, but because there are only
> >>five callers in the whole tree, three of which are in UART code, and
> >>none in anything resembling PCI code.
> >>
> >>Is Rockchip really that special, or are we going about this the wrong
> >>way?
>
> we used to put these codes in the wifi driver, but another wifi
> vendor suggests these should go into the pcie driver.
>
> and as tony said, it could go into pcie common code :)
I guess the implication (I'm speculating here) is that in most
existing cases, the WAKE# signal is fielded by an ACPI BIOS, which
knows how it's connected. I suppose that would end up being turned
into an SCI that Linux already knows how to handle generically.
And further, that the non-ACPI drivers are relatively new and you're
the first attempt to use WAKE# with a non-ACPI PCI host driver?
If this setup could be done somewhere in PCIe common code, that would
be great. We have so much copy and pasted code already, it'd be nice
to avoid adding more. I don't know if this would fit in
pci_scan_root_bus_bridge(), doing something like dma_configure() does
to get hold of a struct platform_device * or a struct device * so you
could lookup the IRQ?
> >>> >+ if (err)
> >>> >+ dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> >>> >+ }
> >>> >+
> >>> > return 0;
> >>
> >>The above could be structured as:
> >>
> >> irq = platform_get_irq_byname(pdev, "wakeup");
> >> if (irq < 0)
> >> return 0;
> >>
> >> device_init_wakeup(dev, true);
> >> err = dev_pm_set_dedicated_wake_irq(dev, irq);
> >> if (err) {
> >> dev_pm_clear_wake_irq(dev);
> >> device_init_wakeup(dev, false);
> >> }
> >>
> there's no need to call dev_pm_clear_wake_irq when
> dev_pm_set_dedicated_wake_irq failed...and i agree the
> device_init_wakeup part, i'll add that in the next version(with
> brian's comment too)
> >> return 0;
> >>
> >>to unindent the mainline non-error code.
> >>
> >>> > }
> >>> >
> >>> >@@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >>> >
> >>> > err = rockchip_pcie_parse_dt(rockchip);
> >>> > if (err)
> >>> >- return err;
> >>> >+ goto err_disable_wake;
> >>> >
> >>> > err = rockchip_pcie_enable_clocks(rockchip);
> >>> > if (err)
> >>> >- return err;
> >>> >+ goto err_disable_wake;
> >>> >
> >>> > err = rockchip_pcie_set_vpcie(rockchip);
> >>> > if (err) {
> >>> >@@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >>> > regulator_disable(rockchip->vpcie0v9);
> >>> > err_set_vpcie:
> >>> > rockchip_pcie_disable_clocks(rockchip);
> >>> >+err_disable_wake:
> >>> >+ dev_pm_clear_wake_irq(dev);
> >>> >+ device_init_wakeup(dev, false);
> >>
> >>I think this error cleanup should be done in rockchip_pcie_setup_irq()
> >>as shown above. There's no real connection between
> >>rockchip_pcie_probe() and the wake setup.
>
> this error handling is like inline "rockchip_pcie_cleanup_irq()",
> and they are harmless to be called even if we don't have the wakeup
> irq :)
I'm sure they're harmless. The point is that the cleanup should be
done near the failure, not in the caller of the caller of the function
where the failure was detected. You have:
rockchip_pcie_probe
rockchip_pcie_parse_dt
rockchip_pcie_setup_irq
err = dev_pm_set_dedicated_wake_irq
if (err)
dev_err(...)
So you detect the error in rockchip_pcie_setup_irq(), but you clean up
from it in rockchip_pcie_probe(), which doesn't make sense because
rockchip_pcie_probe() doesn't do anything related to wakeup interupts.
Bjorn
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 19:19 ` Bjorn Helgaas
0 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 19:19 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Oct 14, 2017 at 02:33:45AM +0800, jeffy wrote:
> Hi Rafael,
>
> On 10/13/2017 09:21 PM, Rafael J. Wysocki wrote:
> >>
> >>I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> >>because I know anything at all about it, but because there are only
> >>five callers in the whole tree, three of which are in UART code, and
> >>none in anything resembling PCI code.
> >>
> >>Is Rockchip really that special, or are we going about this the wrong
> >>way?
>
> we used to put these codes in the wifi driver, but another wifi
> vendor suggests these should go into the pcie driver.
>
> and as tony said, it could go into pcie common code :)
I guess the implication (I'm speculating here) is that in most
existing cases, the WAKE# signal is fielded by an ACPI BIOS, which
knows how it's connected. I suppose that would end up being turned
into an SCI that Linux already knows how to handle generically.
And further, that the non-ACPI drivers are relatively new and you're
the first attempt to use WAKE# with a non-ACPI PCI host driver?
If this setup could be done somewhere in PCIe common code, that would
be great. We have so much copy and pasted code already, it'd be nice
to avoid adding more. I don't know if this would fit in
pci_scan_root_bus_bridge(), doing something like dma_configure() does
to get hold of a struct platform_device * or a struct device * so you
could lookup the IRQ?
> >>> >+ if (err)
> >>> >+ dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> >>> >+ }
> >>> >+
> >>> > return 0;
> >>
> >>The above could be structured as:
> >>
> >> irq = platform_get_irq_byname(pdev, "wakeup");
> >> if (irq < 0)
> >> return 0;
> >>
> >> device_init_wakeup(dev, true);
> >> err = dev_pm_set_dedicated_wake_irq(dev, irq);
> >> if (err) {
> >> dev_pm_clear_wake_irq(dev);
> >> device_init_wakeup(dev, false);
> >> }
> >>
> there's no need to call dev_pm_clear_wake_irq when
> dev_pm_set_dedicated_wake_irq failed...and i agree the
> device_init_wakeup part, i'll add that in the next version(with
> brian's comment too)
> >> return 0;
> >>
> >>to unindent the mainline non-error code.
> >>
> >>> > }
> >>> >
> >>> >@@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >>> >
> >>> > err = rockchip_pcie_parse_dt(rockchip);
> >>> > if (err)
> >>> >- return err;
> >>> >+ goto err_disable_wake;
> >>> >
> >>> > err = rockchip_pcie_enable_clocks(rockchip);
> >>> > if (err)
> >>> >- return err;
> >>> >+ goto err_disable_wake;
> >>> >
> >>> > err = rockchip_pcie_set_vpcie(rockchip);
> >>> > if (err) {
> >>> >@@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >>> > regulator_disable(rockchip->vpcie0v9);
> >>> > err_set_vpcie:
> >>> > rockchip_pcie_disable_clocks(rockchip);
> >>> >+err_disable_wake:
> >>> >+ dev_pm_clear_wake_irq(dev);
> >>> >+ device_init_wakeup(dev, false);
> >>
> >>I think this error cleanup should be done in rockchip_pcie_setup_irq()
> >>as shown above. There's no real connection between
> >>rockchip_pcie_probe() and the wake setup.
>
> this error handling is like inline "rockchip_pcie_cleanup_irq()",
> and they are harmless to be called even if we don't have the wakeup
> irq :)
I'm sure they're harmless. The point is that the cleanup should be
done near the failure, not in the caller of the caller of the function
where the failure was detected. You have:
rockchip_pcie_probe
rockchip_pcie_parse_dt
rockchip_pcie_setup_irq
err = dev_pm_set_dedicated_wake_irq
if (err)
dev_err(...)
So you detect the error in rockchip_pcie_setup_irq(), but you clean up
from it in rockchip_pcie_probe(), which doesn't make sense because
rockchip_pcie_probe() doesn't do anything related to wakeup interupts.
Bjorn
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 19:19 ` Bjorn Helgaas
0 siblings, 0 replies; 40+ messages in thread
From: Bjorn Helgaas @ 2017-10-13 19:19 UTC (permalink / raw)
To: jeffy
Cc: Heiko Stuebner, linux-pm, Tony Lindgren, linux-pci, shawn.lin,
briannorris, Rafael J. Wysocki, linux-kernel, dianders,
linux-rockchip, bhelgaas, linux-arm-kernel
On Sat, Oct 14, 2017 at 02:33:45AM +0800, jeffy wrote:
> Hi Rafael,
>
> On 10/13/2017 09:21 PM, Rafael J. Wysocki wrote:
> >>
> >>I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> >>because I know anything at all about it, but because there are only
> >>five callers in the whole tree, three of which are in UART code, and
> >>none in anything resembling PCI code.
> >>
> >>Is Rockchip really that special, or are we going about this the wrong
> >>way?
>
> we used to put these codes in the wifi driver, but another wifi
> vendor suggests these should go into the pcie driver.
>
> and as tony said, it could go into pcie common code :)
I guess the implication (I'm speculating here) is that in most
existing cases, the WAKE# signal is fielded by an ACPI BIOS, which
knows how it's connected. I suppose that would end up being turned
into an SCI that Linux already knows how to handle generically.
And further, that the non-ACPI drivers are relatively new and you're
the first attempt to use WAKE# with a non-ACPI PCI host driver?
If this setup could be done somewhere in PCIe common code, that would
be great. We have so much copy and pasted code already, it'd be nice
to avoid adding more. I don't know if this would fit in
pci_scan_root_bus_bridge(), doing something like dma_configure() does
to get hold of a struct platform_device * or a struct device * so you
could lookup the IRQ?
> >>> >+ if (err)
> >>> >+ dev_err(dev, "failed to setup PCIe wakeup IRQ\n");
> >>> >+ }
> >>> >+
> >>> > return 0;
> >>
> >>The above could be structured as:
> >>
> >> irq = platform_get_irq_byname(pdev, "wakeup");
> >> if (irq < 0)
> >> return 0;
> >>
> >> device_init_wakeup(dev, true);
> >> err = dev_pm_set_dedicated_wake_irq(dev, irq);
> >> if (err) {
> >> dev_pm_clear_wake_irq(dev);
> >> device_init_wakeup(dev, false);
> >> }
> >>
> there's no need to call dev_pm_clear_wake_irq when
> dev_pm_set_dedicated_wake_irq failed...and i agree the
> device_init_wakeup part, i'll add that in the next version(with
> brian's comment too)
> >> return 0;
> >>
> >>to unindent the mainline non-error code.
> >>
> >>> > }
> >>> >
> >>> >@@ -1542,11 +1552,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >>> >
> >>> > err = rockchip_pcie_parse_dt(rockchip);
> >>> > if (err)
> >>> >- return err;
> >>> >+ goto err_disable_wake;
> >>> >
> >>> > err = rockchip_pcie_enable_clocks(rockchip);
> >>> > if (err)
> >>> >- return err;
> >>> >+ goto err_disable_wake;
> >>> >
> >>> > err = rockchip_pcie_set_vpcie(rockchip);
> >>> > if (err) {
> >>> >@@ -1656,6 +1666,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >>> > regulator_disable(rockchip->vpcie0v9);
> >>> > err_set_vpcie:
> >>> > rockchip_pcie_disable_clocks(rockchip);
> >>> >+err_disable_wake:
> >>> >+ dev_pm_clear_wake_irq(dev);
> >>> >+ device_init_wakeup(dev, false);
> >>
> >>I think this error cleanup should be done in rockchip_pcie_setup_irq()
> >>as shown above. There's no real connection between
> >>rockchip_pcie_probe() and the wake setup.
>
> this error handling is like inline "rockchip_pcie_cleanup_irq()",
> and they are harmless to be called even if we don't have the wakeup
> irq :)
I'm sure they're harmless. The point is that the cleanup should be
done near the failure, not in the caller of the caller of the function
where the failure was detected. You have:
rockchip_pcie_probe
rockchip_pcie_parse_dt
rockchip_pcie_setup_irq
err = dev_pm_set_dedicated_wake_irq
if (err)
dev_err(...)
So you detect the error in rockchip_pcie_setup_irq(), but you clean up
from it in rockchip_pcie_probe(), which doesn't make sense because
rockchip_pcie_probe() doesn't do anything related to wakeup interupts.
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-10-13 19:19 ` Bjorn Helgaas
@ 2017-10-13 19:26 ` jeffy
-1 siblings, 0 replies; 40+ messages in thread
From: jeffy @ 2017-10-13 19:26 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Rafael J. Wysocki, linux-kernel, bhelgaas, Heiko Stuebner,
linux-pci, shawn.lin, briannorris, dianders, linux-rockchip,
linux-arm-kernel, linux-pm, Tony Lindgren
Hi Bjorn,
On 10/14/2017 03:19 AM, Bjorn Helgaas wrote:
> I'm sure they're harmless. The point is that the cleanup should be
> done near the failure, not in the caller of the caller of the function
> where the failure was detected. You have:
>
> rockchip_pcie_probe
> rockchip_pcie_parse_dt
> rockchip_pcie_setup_irq
> err = dev_pm_set_dedicated_wake_irq
> if (err)
> dev_err(...)
>
> So you detect the error in rockchip_pcie_setup_irq(), but you clean up
> from it in rockchip_pcie_probe(), which doesn't make sense because
> rockchip_pcie_probe() doesn't do anything related to wakeup interupts.
>
right, but if something wrong happens in rockchip_pcie_probe() later
than rockchip_pcie_setup_irq(), we may still need to clean it up ;)
i think the error handling is a little like what we do in the remove
callback
> Bjorn
>
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 19:26 ` jeffy
0 siblings, 0 replies; 40+ messages in thread
From: jeffy @ 2017-10-13 19:26 UTC (permalink / raw)
To: linux-arm-kernel
Hi Bjorn,
On 10/14/2017 03:19 AM, Bjorn Helgaas wrote:
> I'm sure they're harmless. The point is that the cleanup should be
> done near the failure, not in the caller of the caller of the function
> where the failure was detected. You have:
>
> rockchip_pcie_probe
> rockchip_pcie_parse_dt
> rockchip_pcie_setup_irq
> err = dev_pm_set_dedicated_wake_irq
> if (err)
> dev_err(...)
>
> So you detect the error in rockchip_pcie_setup_irq(), but you clean up
> from it in rockchip_pcie_probe(), which doesn't make sense because
> rockchip_pcie_probe() doesn't do anything related to wakeup interupts.
>
right, but if something wrong happens in rockchip_pcie_probe() later
than rockchip_pcie_setup_irq(), we may still need to clean it up ;)
i think the error handling is a little like what we do in the remove
callback
> Bjorn
>
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
2017-10-13 19:19 ` Bjorn Helgaas
@ 2017-10-13 20:44 ` Brian Norris
-1 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2017-10-13 20:44 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: jeffy, Rafael J. Wysocki, linux-kernel, bhelgaas, Heiko Stuebner,
linux-pci, shawn.lin, dianders, linux-rockchip, linux-arm-kernel,
linux-pm, Tony Lindgren
Hi,
On Fri, Oct 13, 2017 at 02:19:06PM -0500, Bjorn Helgaas wrote:
> On Sat, Oct 14, 2017 at 02:33:45AM +0800, jeffy wrote:
> > Hi Rafael,
> >
> > On 10/13/2017 09:21 PM, Rafael J. Wysocki wrote:
> > >>
> > >>I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> > >>because I know anything at all about it, but because there are only
> > >>five callers in the whole tree, three of which are in UART code, and
> > >>none in anything resembling PCI code.
> > >>
> > >>Is Rockchip really that special, or are we going about this the wrong
> > >>way?
> >
> > we used to put these codes in the wifi driver, but another wifi
> > vendor suggests these should go into the pcie driver.
> >
> > and as tony said, it could go into pcie common code :)
>
> I guess the implication (I'm speculating here) is that in most
> existing cases, the WAKE# signal is fielded by an ACPI BIOS, which
> knows how it's connected. I suppose that would end up being turned
> into an SCI that Linux already knows how to handle generically.
I wasn't sure how ACPI did this when I first suggested Rockchip take
this approach, but since then I believe have figured it out. We have:
pci_prepare_to_sleep() -> pci_enable_wake()
where pci_enable_wake() will configure PME wakeup and/or "platform" wake
(which presumably is the WAKE# signal). pci-acpi.c has registered hooks
for the latter via pci_set_platform_pm().
This doesn't really make it any more generic for discovering this
platform-specific detail. We'd have to set up some kind of platform ops
that could be shared for any DT-based platforms.
But that *does* answer the question I had about conditionality: should
we always enable WAKE# for platforms that have the pin hooked up to the
host? Or is this configured on a per-device basis? IIUC, the intention
is that there's only a single open-drain WAKE# pin for the whole system,
and it's just pulled high for EPs that don't implement it.
> And further, that the non-ACPI drivers are relatively new and you're
> the first attempt to use WAKE# with a non-ACPI PCI host driver?
Quite possibly. Or everyone just sidestepped this an configured the pin
elsewhere (e.g., you could stick a GPIO like this into a gpio-keys
device and it would mostly work).
> If this setup could be done somewhere in PCIe common code, that would
> be great. We have so much copy and pasted code already, it'd be nice
> to avoid adding more. I don't know if this would fit in
> pci_scan_root_bus_bridge(), doing something like dma_configure() does
> to get hold of a struct platform_device * or a struct device * so you
> could lookup the IRQ?
It looks like the infrastructure is in pci_set_platform_pm(), sort of.
But that still doesn't help you for the repetition; you're just lucky
you only have 2 controller drivers that call this right now :)
Side note: there's some dissonance between this statement, in
Documentation/driver-api/pm/devices.rst:
"Device drivers, however, are not expected to call
:c:func:`device_set_wakeup_enable()` directly in any case."
Yet:
$ git grep -l device_set_wakeup_enable drivers/ | wc -l
69
And particularly, I believe that was necessary for Wifi drivers like
drivers/net/wireless/ath/ath10k/wow.c.
Brian
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v5 1/3] PCI: rockchip: Add support for pcie wake irq
@ 2017-10-13 20:44 ` Brian Norris
0 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2017-10-13 20:44 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Fri, Oct 13, 2017 at 02:19:06PM -0500, Bjorn Helgaas wrote:
> On Sat, Oct 14, 2017 at 02:33:45AM +0800, jeffy wrote:
> > Hi Rafael,
> >
> > On 10/13/2017 09:21 PM, Rafael J. Wysocki wrote:
> > >>
> > >>I'm a little skeptical about dev_pm_set_dedicated_wake_irq(), not
> > >>because I know anything at all about it, but because there are only
> > >>five callers in the whole tree, three of which are in UART code, and
> > >>none in anything resembling PCI code.
> > >>
> > >>Is Rockchip really that special, or are we going about this the wrong
> > >>way?
> >
> > we used to put these codes in the wifi driver, but another wifi
> > vendor suggests these should go into the pcie driver.
> >
> > and as tony said, it could go into pcie common code :)
>
> I guess the implication (I'm speculating here) is that in most
> existing cases, the WAKE# signal is fielded by an ACPI BIOS, which
> knows how it's connected. I suppose that would end up being turned
> into an SCI that Linux already knows how to handle generically.
I wasn't sure how ACPI did this when I first suggested Rockchip take
this approach, but since then I believe have figured it out. We have:
pci_prepare_to_sleep() -> pci_enable_wake()
where pci_enable_wake() will configure PME wakeup and/or "platform" wake
(which presumably is the WAKE# signal). pci-acpi.c has registered hooks
for the latter via pci_set_platform_pm().
This doesn't really make it any more generic for discovering this
platform-specific detail. We'd have to set up some kind of platform ops
that could be shared for any DT-based platforms.
But that *does* answer the question I had about conditionality: should
we always enable WAKE# for platforms that have the pin hooked up to the
host? Or is this configured on a per-device basis? IIUC, the intention
is that there's only a single open-drain WAKE# pin for the whole system,
and it's just pulled high for EPs that don't implement it.
> And further, that the non-ACPI drivers are relatively new and you're
> the first attempt to use WAKE# with a non-ACPI PCI host driver?
Quite possibly. Or everyone just sidestepped this an configured the pin
elsewhere (e.g., you could stick a GPIO like this into a gpio-keys
device and it would mostly work).
> If this setup could be done somewhere in PCIe common code, that would
> be great. We have so much copy and pasted code already, it'd be nice
> to avoid adding more. I don't know if this would fit in
> pci_scan_root_bus_bridge(), doing something like dma_configure() does
> to get hold of a struct platform_device * or a struct device * so you
> could lookup the IRQ?
It looks like the infrastructure is in pci_set_platform_pm(), sort of.
But that still doesn't help you for the repetition; you're just lucky
you only have 2 controller drivers that call this right now :)
Side note: there's some dissonance between this statement, in
Documentation/driver-api/pm/devices.rst:
"Device drivers, however, are not expected to call
:c:func:`device_set_wakeup_enable()` directly in any case."
Yet:
$ git grep -l device_set_wakeup_enable drivers/ | wc -l
69
And particularly, I believe that was necessary for Wifi drivers like
drivers/net/wireless/ath/ath10k/wow.c.
Brian
^ permalink raw reply [flat|nested] 40+ messages in thread