Currently we are handling wake irq in mrvl wifi driver. Move it into
pci core.
Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).
Changes in v13:
Fix compiler error reported by kbuild test robot <[email protected]>
Changes in v12:
Only add irq definitions for PCI devices and rewrite the commit message.
Enable the wake irq in noirq stage to avoid possible irq storm.
Changes in v11:
Address Brian's comments.
Only support 1-per-device PCIe WAKE# pin as suggested.
Move to pcie port as Brian suggested.
Changes in v10:
Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
Changes in v9:
Add section for PCI devices and rewrite the commit message.
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.
Rewrite the commit message.
Changes in v8:
Add optional "pci", and rewrite commit message.
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
Rewrite the commit message.
Changes in v7:
Move PCIE_WAKE handling into pci core.
Changes in v6:
Fix device_init_wake error handling, and add some comments.
Changes in v5:
Move to pci.txt
Rebase.
Use "wakeup" instead of "wake"
Changes in v3:
Fix error handling.
Changes in v2:
Use dev_pm_set_dedicated_wake_irq.
Jeffy Chen (5):
dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
of/irq: Adjust of_pci_irq parsing for multiple interrupts
mwifiex: Disable wakeup irq handling for pcie
PCI / PM: Add support for the PCIe WAKE# signal for OF
arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru
Documentation/devicetree/bindings/pci/pci.txt | 10 ++++
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 11 ++--
drivers/net/wireless/marvell/mwifiex/main.c | 4 ++
drivers/of/of_pci_irq.c | 71 +++++++++++++++++++++++--
drivers/pci/Makefile | 1 +
drivers/pci/pci-driver.c | 10 ++++
drivers/pci/pci-of.c | 75 +++++++++++++++++++++++++++
include/linux/of_pci.h | 9 ++++
8 files changed, 183 insertions(+), 8 deletions(-)
create mode 100644 drivers/pci/pci-of.c
--
2.11.0
We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
so add definitions of the optional PCIe WAKE# pin for PCI devices.
Also add an definition of the optional PCI interrupt pin for PCI
devices to distinguish it from the PCIe WAKE# pin.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v13: None
Changes in v12:
Only add irq definitions for PCI devices and rewrite the commit message.
Changes in v11: None
Changes in v10: None
Changes in v9:
Add section for PCI devices and rewrite the commit message.
Changes in v8:
Add optional "pci", and rewrite commit message.
Changes in v7: None
Changes in v6: None
Changes in v5:
Move to pci.txt
Changes in v3: None
Changes in v2: None
Documentation/devicetree/bindings/pci/pci.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index c77981c5dd18..3045ac452f27 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -24,3 +24,13 @@ driver implementation may support the following properties:
unsupported link speed, for instance, trying to do training for
unsupported link speed, etc. Must be '4' for gen4, '3' for gen3, '2'
for gen2, and '1' for gen1. Any other values are invalid.
+
+PCI devices may support the following properties:
+
+- interrupts: Interrupt specifier for each name in interrupt-names.
+- interrupt-names:
+ May contain "wakeup" for PCIe WAKE# interrupt and "pci" for PCI interrupt.
+ The PCI devices may optionally include an 'interrupts' property that
+ represents the legacy PCI interrupt. And when we try to specify the PCIe
+ WAKE# pin, a corresponding 'interrupt-names' property is required to
+ distinguish them.
--
2.11.0
Currently we are considering the first irq as the PCI interrupt pin,
but a PCI device may have multiple interrupts(e.g. PCIe WAKE# pin).
Only parse the PCI interrupt pin when the irq is unnamed or named as
"pci".
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v13: None
Changes in v12: None
Changes in v11:
Address Brian's comments.
Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None
drivers/of/of_pci_irq.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index 3a05568f65df..d39565d5477b 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -27,9 +27,25 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq
*/
dn = pci_device_to_OF_node(pdev);
if (dn) {
- rc = of_irq_parse_one(dn, 0, out_irq);
- if (!rc)
- return rc;
+ struct property *prop;
+ const char *name;
+ int index = 0;
+
+ of_property_for_each_string(dn, "interrupt-names", prop, name) {
+ if (!strcmp(name, "pci"))
+ break;
+ index++;
+ }
+
+ /*
+ * Only parse from DT if we have no "interrupt-names",
+ * or if we found an interrupt named "pci".
+ */
+ if (index == 0 || name) {
+ rc = of_irq_parse_one(dn, index, out_irq);
+ if (!rc)
+ return rc;
+ }
}
/* Ok, we don't, time to have fun. Let's start by building up an
--
2.11.0
We are going to handle the wakeup irq in the pci core.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v13: None
Changes in v12: None
Changes in v11: None
Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None
drivers/net/wireless/marvell/mwifiex/main.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index a96bd7e653bf..3cc3403b977a 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1567,6 +1567,10 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
goto err_exit;
adapter->dt_node = dev->of_node;
+
+ if (adapter->iface_type != MWIFIEX_PCIE)
+ goto err_exit;
+
adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
if (!adapter->irq_wakeup) {
dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
--
2.11.0
Add of_pci_setup_wake_irq() and of_pci_teardown_wake_irq() to handle
the PCIe WAKE# interrupt.
Also use the dedicated wakeirq infrastructure to simplify it.
And add pci-of.c to enable/disable the wakeup irq in noirq stage to
avoid possible irq storm.
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v13:
Fix compiler error reported by kbuild test robot <[email protected]>
Changes in v12:
Enable the wake irq in noirq stage to avoid possible irq storm.
Changes in v11:
Only support 1-per-device PCIe WAKE# pin as suggested.
Changes in v10:
Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
Changes in v9:
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.
Changes in v8:
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
Changes in v7:
Move PCIE_WAKE handling into pci core.
Changes in v6:
Fix device_init_wake error handling, and add some comments.
Changes in v5:
Rebase.
Changes in v3:
Fix error handling.
Changes in v2:
Use dev_pm_set_dedicated_wake_irq.
drivers/of/of_pci_irq.c | 49 +++++++++++++++++++++++++++++++
drivers/pci/Makefile | 1 +
drivers/pci/pci-driver.c | 10 +++++++
drivers/pci/pci-of.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_pci.h | 9 ++++++
5 files changed, 144 insertions(+)
create mode 100644 drivers/pci/pci-of.c
diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index d39565d5477b..abec3a44853b 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -1,8 +1,57 @@
#include <linux/kernel.h>
#include <linux/of_pci.h>
#include <linux/of_irq.h>
+#include <linux/pm_wakeirq.h>
#include <linux/export.h>
+int of_pci_setup_wake_irq(struct pci_dev *pdev)
+{
+ struct pci_dev *ppdev;
+ struct device_node *dn;
+ int ret, irq;
+
+ /* Get the pci_dev of our parent. Hopefully it's a port. */
+ ppdev = pdev->bus->self;
+ /* Nope, it's a host bridge. */
+ if (!ppdev)
+ return 0;
+
+ dn = pci_device_to_OF_node(ppdev);
+ if (!dn)
+ return 0;
+
+ irq = of_irq_get_byname(dn, "wakeup");
+ if (irq == -EPROBE_DEFER) {
+ return irq;
+ } else if (irq < 0) {
+ /* Ignore other errors, since a missing wakeup is non-fatal. */
+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
+ return 0;
+ }
+
+ device_init_wakeup(&pdev->dev, true);
+
+ ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, irq);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to set wake IRQ: %d\n", ret);
+ device_init_wakeup(&pdev->dev, false);
+ return ret;
+ }
+
+ /* Start out disabled to avoid irq storm */
+ dev_pm_disable_wake_irq(&pdev->dev);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
+
+void of_pci_teardown_wake_irq(struct pci_dev *pdev)
+{
+ dev_pm_clear_wake_irq(&pdev->dev);
+ device_init_wakeup(&pdev->dev, false);
+}
+EXPORT_SYMBOL_GPL(of_pci_teardown_wake_irq);
+
/**
* of_irq_parse_pci - Resolve the interrupt for a PCI device
* @pdev: the device whose interrupt is to be resolved
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index c7819b973df7..d0182c82162a 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
# ACPI _DSM provided firmware instance and string name
#
obj-$(CONFIG_ACPI) += pci-acpi.o
+obj-$(CONFIG_OF) += pci-of.o
# SMBIOS provided firmware instance and labels
obj-$(CONFIG_PCI_LABEL) += pci-label.o
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index d79dbc377b9c..b4475ff35d97 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -17,6 +17,7 @@
#include <linux/slab.h>
#include <linux/sched.h>
#include <linux/cpu.h>
+#include <linux/of_pci.h>
#include <linux/pm_runtime.h>
#include <linux/suspend.h>
#include <linux/kexec.h>
@@ -421,10 +422,17 @@ static int pci_device_probe(struct device *dev)
if (error < 0)
return error;
+ error = of_pci_setup_wake_irq(pci_dev);
+ if (error < 0) {
+ pcibios_free_irq(pci_dev);
+ return error;
+ }
+
pci_dev_get(pci_dev);
if (pci_device_can_probe(pci_dev)) {
error = __pci_device_probe(drv, pci_dev);
if (error) {
+ of_pci_teardown_wake_irq(pci_dev);
pcibios_free_irq(pci_dev);
pci_dev_put(pci_dev);
}
@@ -438,6 +446,8 @@ static int pci_device_remove(struct device *dev)
struct pci_dev *pci_dev = to_pci_dev(dev);
struct pci_driver *drv = pci_dev->driver;
+ of_pci_teardown_wake_irq(pci_dev);
+
if (drv) {
if (drv->remove) {
pm_runtime_get_sync(dev);
diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
new file mode 100644
index 000000000000..ad413b2de508
--- /dev/null
+++ b/drivers/pci/pci-of.c
@@ -0,0 +1,75 @@
+/*
+ * File: pci-of.c
+ * Purpose: Provide PCI PM/wakeup support in OF systems
+ *
+ * Copyright (C) 2017 Google, Inc.
+ * Brian Norris <[email protected]>
+ *
+ * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
+ * Jeffy Chen <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pm_wakeirq.h>
+#include "pci.h"
+
+static bool of_pci_power_manageable(struct pci_dev *dev)
+{
+ return false;
+}
+
+static int of_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
+{
+ return -ENOSYS;
+}
+
+static pci_power_t of_pci_get_power_state(struct pci_dev *dev)
+{
+ return PCI_UNKNOWN;
+}
+
+static pci_power_t of_pci_choose_state(struct pci_dev *pdev)
+{
+ return PCI_POWER_ERROR;
+}
+
+static int of_pci_wakeup(struct pci_dev *dev, bool enable)
+{
+ if (enable)
+ dev_pm_enable_wake_irq(&dev->dev);
+ else
+ dev_pm_disable_wake_irq(&dev->dev);
+ return 0;
+}
+
+static bool of_pci_need_resume(struct pci_dev *dev)
+{
+ return false;
+}
+
+static const struct pci_platform_pm_ops of_pci_platform_pm = {
+ .is_manageable = of_pci_power_manageable,
+ .set_state = of_pci_set_power_state,
+ .get_state = of_pci_get_power_state,
+ .choose_state = of_pci_choose_state,
+ .set_wakeup = of_pci_wakeup,
+ .need_resume = of_pci_need_resume,
+};
+
+static int __init of_pci_init(void)
+{
+ if (!acpi_disabled)
+ return 0;
+
+ pci_set_platform_pm(&of_pci_platform_pm);
+
+ return 0;
+}
+arch_initcall(of_pci_init);
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index bf588a05d0d0..80fe8ae3393e 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -10,6 +10,8 @@ struct of_phandle_args;
struct device_node;
#ifdef CONFIG_OF_PCI
+int of_pci_setup_wake_irq(struct pci_dev *pdev);
+void of_pci_teardown_wake_irq(struct pci_dev *pdev);
int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq);
struct device_node *of_pci_find_child_device(struct device_node *parent,
unsigned int devfn);
@@ -23,6 +25,13 @@ int of_pci_map_rid(struct device_node *np, u32 rid,
const char *map_name, const char *map_mask_name,
struct device_node **target, u32 *id_out);
#else
+static inline int of_pci_setup_wake_irq(struct pci_dev *pdev)
+{
+ return 0;
+}
+
+static void of_pci_teardown_wake_irq(struct pci_dev *pdev) { };
+
static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
{
return 0;
--
2.11.0
Currently we are handling PCIe WAKE# irq in mrvl wifi driver.
Move it to rockchip pcie port since we are going to handle it in the
pci core.
Also avoid this irq been considered as the PCI interrupt pin in the
of_irq_parse_pci().
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v13: None
Changes in v12: None
Changes in v11:
Move to pcie port as Brian suggested.
Changes in v10: None
Changes in v9:
Rewrite the commit message.
Changes in v8:
Rewrite the commit message.
Changes in v7: None
Changes in v6: None
Changes in v5:
Use "wakeup" instead of "wake"
Changes in v3: None
Changes in v2: None
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 03f195025390..be41d363efd8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -719,15 +719,16 @@ ap_i2c_audio: &i2c8 {
#size-cells = <2>;
ranges;
+ interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-names = "pci", "wakeup";
+ pinctrl-names = "default";
+ pinctrl-0 = <&wlan_host_wake_l>;
+ wakeup-source;
+
mvl_wifi: wifi@0,0 {
compatible = "pci1b4b,2b42";
reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
0x83010000 0x0 0x00100000 0x0 0x00100000>;
- interrupt-parent = <&gpio0>;
- interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
- pinctrl-names = "default";
- pinctrl-0 = <&wlan_host_wake_l>;
- wakeup-source;
};
};
};
--
2.11.0
On Mon, Dec 25, 2017 at 8:36 PM, Jeffy Chen <[email protected]> wrote:
> Currently we are considering the first irq as the PCI interrupt pin,
> but a PCI device may have multiple interrupts(e.g. PCIe WAKE# pin).
>
> Only parse the PCI interrupt pin when the irq is unnamed or named as
> "pci".
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v13: None
> Changes in v12: None
> Changes in v11:
> Address Brian's comments.
>
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v3: None
> Changes in v2: None
>
> drivers/of/of_pci_irq.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> index 3a05568f65df..d39565d5477b 100644
> --- a/drivers/of/of_pci_irq.c
> +++ b/drivers/of/of_pci_irq.c
> @@ -27,9 +27,25 @@ int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq
> */
> dn = pci_device_to_OF_node(pdev);
> if (dn) {
> - rc = of_irq_parse_one(dn, 0, out_irq);
> - if (!rc)
> - return rc;
> + struct property *prop;
> + const char *name;
> + int index = 0;
> +
> + of_property_for_each_string(dn, "interrupt-names", prop, name) {
> + if (!strcmp(name, "pci"))
> + break;
> + index++;
> + }
> +
> + /*
> + * Only parse from DT if we have no "interrupt-names",
> + * or if we found an interrupt named "pci".
> + */
> + if (index == 0 || name) {
> + rc = of_irq_parse_one(dn, index, out_irq);
> + if (!rc)
> + return rc;
> + }
As mentioned before, use of_property_match_string. The following should work:
index = of_property_match_string(dn, "interrupt-names", "pci");
if (index == -EINVAL) /* Property doesn't exist */
index = 0;
if (index >= 0) {
rc = of_irq_parse_one(dn, index, out_irq);
if (!rc)
return rc;
}
> }
>
> /* Ok, we don't, time to have fun. Let's start by building up an
> --
> 2.11.0
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 26, 2017 at 10:36:42AM +0800, Jeffy Chen wrote:
> We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
> so add definitions of the optional PCIe WAKE# pin for PCI devices.
>
> Also add an definition of the optional PCI interrupt pin for PCI
> devices to distinguish it from the PCIe WAKE# pin.
By v13 you should drop "RFC". RFC implies not ready for merging.
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v13: None
> Changes in v12:
> Only add irq definitions for PCI devices and rewrite the commit message.
>
> Changes in v11: None
> Changes in v10: None
> Changes in v9:
> Add section for PCI devices and rewrite the commit message.
>
> Changes in v8:
> Add optional "pci", and rewrite commit message.
>
> Changes in v7: None
> Changes in v6: None
> Changes in v5:
> Move to pci.txt
>
> Changes in v3: None
> Changes in v2: None
>
> Documentation/devicetree/bindings/pci/pci.txt | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Reviewed-by: Rob Herring <[email protected]>
On Tue, Dec 26, 2017 at 10:36:45AM +0800, Jeffy Chen wrote:
> Add of_pci_setup_wake_irq() and of_pci_teardown_wake_irq() to handle
> the PCIe WAKE# interrupt.
>
> Also use the dedicated wakeirq infrastructure to simplify it.
>
> And add pci-of.c to enable/disable the wakeup irq in noirq stage to
> avoid possible irq storm.
>
> Signed-off-by: Jeffy Chen <[email protected]>
> ---
>
> Changes in v13:
> Fix compiler error reported by kbuild test robot <[email protected]>
>
> Changes in v12:
> Enable the wake irq in noirq stage to avoid possible irq storm.
>
> Changes in v11:
> Only support 1-per-device PCIe WAKE# pin as suggested.
>
> Changes in v10:
> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
> since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
>
> Changes in v9:
> Fix check error in .cleanup().
> Move dedicated wakeirq setup to setup() callback and use
> device_set_wakeup_enable() to enable/disable.
>
> Changes in v8:
> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
>
> Changes in v7:
> Move PCIE_WAKE handling into pci core.
>
> Changes in v6:
> Fix device_init_wake error handling, and add some comments.
>
> Changes in v5:
> Rebase.
>
> Changes in v3:
> Fix error handling.
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq.
>
> drivers/of/of_pci_irq.c | 49 +++++++++++++++++++++++++++++++
Please move this to drivers/pci/of.c (or perhaps create pci/of_irq.c).
> drivers/pci/Makefile | 1 +
> drivers/pci/pci-driver.c | 10 +++++++
> drivers/pci/pci-of.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
We already have drivers/pci/of.c. It's not clear what the difference is
from the filenames. Either merge with of.c or perhaps of-pm.c.
> include/linux/of_pci.h | 9 ++++++
> 5 files changed, 144 insertions(+)
> create mode 100644 drivers/pci/pci-of.c
>
> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> index d39565d5477b..abec3a44853b 100644
> --- a/drivers/of/of_pci_irq.c
> +++ b/drivers/of/of_pci_irq.c
> @@ -1,8 +1,57 @@
> #include <linux/kernel.h>
> #include <linux/of_pci.h>
> #include <linux/of_irq.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/export.h>
>
> +int of_pci_setup_wake_irq(struct pci_dev *pdev)
> +{
> + struct pci_dev *ppdev;
> + struct device_node *dn;
> + int ret, irq;
> +
> + /* Get the pci_dev of our parent. Hopefully it's a port. */
> + ppdev = pdev->bus->self;
> + /* Nope, it's a host bridge. */
> + if (!ppdev)
> + return 0;
> +
> + dn = pci_device_to_OF_node(ppdev);
> + if (!dn)
> + return 0;
> +
> + irq = of_irq_get_byname(dn, "wakeup");
> + if (irq == -EPROBE_DEFER) {
> + return irq;
> + } else if (irq < 0) {
> + /* Ignore other errors, since a missing wakeup is non-fatal. */
> + dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
> + return 0;
> + }
> +
> + device_init_wakeup(&pdev->dev, true);
> +
> + ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, irq);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to set wake IRQ: %d\n", ret);
> + device_init_wakeup(&pdev->dev, false);
> + return ret;
> + }
> +
> + /* Start out disabled to avoid irq storm */
> + dev_pm_disable_wake_irq(&pdev->dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
> +
> +void of_pci_teardown_wake_irq(struct pci_dev *pdev)
> +{
> + dev_pm_clear_wake_irq(&pdev->dev);
> + device_init_wakeup(&pdev->dev, false);
> +}
> +EXPORT_SYMBOL_GPL(of_pci_teardown_wake_irq);
> +
> /**
> * of_irq_parse_pci - Resolve the interrupt for a PCI device
> * @pdev: the device whose interrupt is to be resolved
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index c7819b973df7..d0182c82162a 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
> # ACPI _DSM provided firmware instance and string name
> #
> obj-$(CONFIG_ACPI) += pci-acpi.o
> +obj-$(CONFIG_OF) += pci-of.o
>
> # SMBIOS provided firmware instance and labels
> obj-$(CONFIG_PCI_LABEL) += pci-label.o
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index d79dbc377b9c..b4475ff35d97 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -17,6 +17,7 @@
> #include <linux/slab.h>
> #include <linux/sched.h>
> #include <linux/cpu.h>
> +#include <linux/of_pci.h>
> #include <linux/pm_runtime.h>
> #include <linux/suspend.h>
> #include <linux/kexec.h>
> @@ -421,10 +422,17 @@ static int pci_device_probe(struct device *dev)
> if (error < 0)
> return error;
>
> + error = of_pci_setup_wake_irq(pci_dev);
> + if (error < 0) {
> + pcibios_free_irq(pci_dev);
> + return error;
> + }
> +
> pci_dev_get(pci_dev);
> if (pci_device_can_probe(pci_dev)) {
> error = __pci_device_probe(drv, pci_dev);
> if (error) {
> + of_pci_teardown_wake_irq(pci_dev);
> pcibios_free_irq(pci_dev);
> pci_dev_put(pci_dev);
> }
> @@ -438,6 +446,8 @@ static int pci_device_remove(struct device *dev)
> struct pci_dev *pci_dev = to_pci_dev(dev);
> struct pci_driver *drv = pci_dev->driver;
>
> + of_pci_teardown_wake_irq(pci_dev);
> +
> if (drv) {
> if (drv->remove) {
> pm_runtime_get_sync(dev);
> diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
> new file mode 100644
> index 000000000000..ad413b2de508
> --- /dev/null
> +++ b/drivers/pci/pci-of.c
> @@ -0,0 +1,75 @@
> +/*
> + * File: pci-of.c
> + * Purpose: Provide PCI PM/wakeup support in OF systems
> + *
> + * Copyright (C) 2017 Google, Inc.
> + * Brian Norris <[email protected]>
> + *
> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> + * Jeffy Chen <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/pm_wakeirq.h>
> +#include "pci.h"
> +
> +static bool of_pci_power_manageable(struct pci_dev *dev)
> +{
> + return false;
> +}
> +
> +static int of_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> +{
> + return -ENOSYS;
> +}
> +
> +static pci_power_t of_pci_get_power_state(struct pci_dev *dev)
> +{
> + return PCI_UNKNOWN;
> +}
> +
> +static pci_power_t of_pci_choose_state(struct pci_dev *pdev)
> +{
> + return PCI_POWER_ERROR;
> +}
> +
> +static int of_pci_wakeup(struct pci_dev *dev, bool enable)
> +{
> + if (enable)
> + dev_pm_enable_wake_irq(&dev->dev);
> + else
> + dev_pm_disable_wake_irq(&dev->dev);
> + return 0;
> +}
> +
> +static bool of_pci_need_resume(struct pci_dev *dev)
> +{
> + return false;
> +}
> +
> +static const struct pci_platform_pm_ops of_pci_platform_pm = {
> + .is_manageable = of_pci_power_manageable,
> + .set_state = of_pci_set_power_state,
> + .get_state = of_pci_get_power_state,
> + .choose_state = of_pci_choose_state,
> + .set_wakeup = of_pci_wakeup,
> + .need_resume = of_pci_need_resume,
> +};
> +
> +static int __init of_pci_init(void)
> +{
> + if (!acpi_disabled)
> + return 0;
> +
> + pci_set_platform_pm(&of_pci_platform_pm);
I guess no DT based system will override this?
> +
> + return 0;
> +}
> +arch_initcall(of_pci_init);
On Wednesday, December 27, 2017 12:35:52 AM CET Rob Herring wrote:
> On Tue, Dec 26, 2017 at 10:36:42AM +0800, Jeffy Chen wrote:
> > We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
> > so add definitions of the optional PCIe WAKE# pin for PCI devices.
> >
> > Also add an definition of the optional PCI interrupt pin for PCI
> > devices to distinguish it from the PCIe WAKE# pin.
>
> By v13 you should drop "RFC". RFC implies not ready for merging.
Which very much still is the case AFAICS.
Thanks,
Rafael
On Tuesday, December 26, 2017 3:36:41 AM CET Jeffy Chen wrote:
>
> Currently we are handling wake irq in mrvl wifi driver. Move it into
> pci core.
>
> Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).
>
>
> Changes in v13:
> Fix compiler error reported by kbuild test robot <[email protected]>
>
> Changes in v12:
> Only add irq definitions for PCI devices and rewrite the commit message.
> Enable the wake irq in noirq stage to avoid possible irq storm.
>
> Changes in v11:
> Address Brian's comments.
> Only support 1-per-device PCIe WAKE# pin as suggested.
> Move to pcie port as Brian suggested.
>
> Changes in v10:
> Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
> since dedicated wakeirq will be lost in device_set_wakeup_enable(false).
>
> Changes in v9:
> Add section for PCI devices and rewrite the commit message.
> Fix check error in .cleanup().
> Move dedicated wakeirq setup to setup() callback and use
> device_set_wakeup_enable() to enable/disable.
> Rewrite the commit message.
>
> Changes in v8:
> Add optional "pci", and rewrite commit message.
> Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
> Rewrite the commit message.
>
> Changes in v7:
> Move PCIE_WAKE handling into pci core.
>
> Changes in v6:
> Fix device_init_wake error handling, and add some comments.
>
> Changes in v5:
> Move to pci.txt
> Rebase.
> Use "wakeup" instead of "wake"
>
> Changes in v3:
> Fix error handling.
>
> Changes in v2:
> Use dev_pm_set_dedicated_wake_irq.
>
> Jeffy Chen (5):
> dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
> of/irq: Adjust of_pci_irq parsing for multiple interrupts
> mwifiex: Disable wakeup irq handling for pcie
> PCI / PM: Add support for the PCIe WAKE# signal for OF
> arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru
>
> Documentation/devicetree/bindings/pci/pci.txt | 10 ++++
> arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 11 ++--
> drivers/net/wireless/marvell/mwifiex/main.c | 4 ++
> drivers/of/of_pci_irq.c | 71 +++++++++++++++++++++++--
> drivers/pci/Makefile | 1 +
> drivers/pci/pci-driver.c | 10 ++++
> drivers/pci/pci-of.c | 75 +++++++++++++++++++++++++++
> include/linux/of_pci.h | 9 ++++
> 8 files changed, 183 insertions(+), 8 deletions(-)
> create mode 100644 drivers/pci/pci-of.c
I'm going to ignore this version till the discussion on the previous one is
over.
Thanks,
Rafael
Hi Rob,
Thanks for your reply.
On 12/27/2017 07:56 AM, Rob Herring wrote:
>> >
>> > drivers/of/of_pci_irq.c | 49 +++++++++++++++++++++++++++++++
> Please move this to drivers/pci/of.c (or perhaps create pci/of_irq.c).
>
>> > drivers/pci/Makefile | 1 +
>> > drivers/pci/pci-driver.c | 10 +++++++
>> > drivers/pci/pci-of.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
> We already have drivers/pci/of.c. It's not clear what the difference is
> from the filenames. Either merge with of.c or perhaps of-pm.c.
this new file does something similar to the pci-acpi.c and pci-mid.c..
and i am agree the naming is not clear, maybe we can rename both of
those files to something like pci-pm-***.c?
Hi Rafael, do you think this would make sense?
>> > include/linux/of_pci.h | 9 ++++++
>> > 5 files changed, 144 insertions(+)
>> > create mode 100644 drivers/pci/pci-of.c
>> >
>> >diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
>> >index d39565d5477b..abec3a44853b 100644
>> >--- a/drivers/of/of_pci_irq.c
>> >+++ b/drivers/of/of_pci_irq.c
>> >@@ -1,8 +1,57 @@
>> > #include <linux/kernel.h>
>> > #include <linux/of_pci.h>
>> > #include <linux/of_irq.h>
>> >+#include <linux/pm_wakeirq.h>
>> > #include <linux/export.h>
>> >
>> >+int of_pci_setup_wake_irq(struct pci_dev *pdev)
>> >+{
>> >+ struct pci_dev *ppdev;
>> >+ struct device_node *dn;
>> >+ int ret, irq;
>> >+
>> >+ /* Get the pci_dev of our parent. Hopefully it's a port. */
>> >+ ppdev = pdev->bus->self;
>> >+ /* Nope, it's a host bridge. */
>> >+ if (!ppdev)
>> >+ return 0;
>> >+
>> >+ dn = pci_device_to_OF_node(ppdev);
>> >+ if (!dn)
>> >+ return 0;
>> >+
>> >+ irq = of_irq_get_byname(dn, "wakeup");
>> >+ if (irq == -EPROBE_DEFER) {
>> >+ return irq;
>> >+ } else if (irq < 0) {
>> >+ /* Ignore other errors, since a missing wakeup is non-fatal. */
>> >+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq);
>> >+ return 0;
>> >+ }
>> >+
>> >+ device_init_wakeup(&pdev->dev, true);
>> >+
>> >+ ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, irq);
>> >+ if (ret < 0) {
>> >+ dev_err(&pdev->dev, "failed to set wake IRQ: %d\n", ret);
>> >+ device_init_wakeup(&pdev->dev, false);
>> >+ return ret;
>> >+ }
>> >+
>> >+ /* Start out disabled to avoid irq storm */
>> >+ dev_pm_disable_wake_irq(&pdev->dev);
>> >+
>> >+ return 0;
>> >+}
>> >+EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq);
>> >+
>> >+void of_pci_teardown_wake_irq(struct pci_dev *pdev)
>> >+{
>> >+ dev_pm_clear_wake_irq(&pdev->dev);
>> >+ device_init_wakeup(&pdev->dev, false);
>> >+}
>> >+EXPORT_SYMBOL_GPL(of_pci_teardown_wake_irq);
>> >+
>> > /**
>> > * of_irq_parse_pci - Resolve the interrupt for a PCI device
>> > * @pdev: the device whose interrupt is to be resolved
>> >diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> >index c7819b973df7..d0182c82162a 100644
>> >--- a/drivers/pci/Makefile
>> >+++ b/drivers/pci/Makefile
>> >@@ -29,6 +29,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
>> > # ACPI _DSM provided firmware instance and string name
>> > #
>> > obj-$(CONFIG_ACPI) += pci-acpi.o
>> >+obj-$(CONFIG_OF) += pci-of.o
>> >
>> > # SMBIOS provided firmware instance and labels
>> > obj-$(CONFIG_PCI_LABEL) += pci-label.o
>> >diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> >index d79dbc377b9c..b4475ff35d97 100644
>> >--- a/drivers/pci/pci-driver.c
>> >+++ b/drivers/pci/pci-driver.c
>> >@@ -17,6 +17,7 @@
>> > #include <linux/slab.h>
>> > #include <linux/sched.h>
>> > #include <linux/cpu.h>
>> >+#include <linux/of_pci.h>
>> > #include <linux/pm_runtime.h>
>> > #include <linux/suspend.h>
>> > #include <linux/kexec.h>
>> >@@ -421,10 +422,17 @@ static int pci_device_probe(struct device *dev)
>> > if (error < 0)
>> > return error;
>> >
>> >+ error = of_pci_setup_wake_irq(pci_dev);
>> >+ if (error < 0) {
>> >+ pcibios_free_irq(pci_dev);
>> >+ return error;
>> >+ }
>> >+
>> > pci_dev_get(pci_dev);
>> > if (pci_device_can_probe(pci_dev)) {
>> > error = __pci_device_probe(drv, pci_dev);
>> > if (error) {
>> >+ of_pci_teardown_wake_irq(pci_dev);
>> > pcibios_free_irq(pci_dev);
>> > pci_dev_put(pci_dev);
>> > }
>> >@@ -438,6 +446,8 @@ static int pci_device_remove(struct device *dev)
>> > struct pci_dev *pci_dev = to_pci_dev(dev);
>> > struct pci_driver *drv = pci_dev->driver;
>> >
>> >+ of_pci_teardown_wake_irq(pci_dev);
>> >+
>> > if (drv) {
>> > if (drv->remove) {
>> > pm_runtime_get_sync(dev);
>> >diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
>> >new file mode 100644
>> >index 000000000000..ad413b2de508
>> >--- /dev/null
>> >+++ b/drivers/pci/pci-of.c
>> >@@ -0,0 +1,75 @@
>> >+/*
>> >+ * File: pci-of.c
>> >+ * Purpose: Provide PCI PM/wakeup support in OF systems
>> >+ *
>> >+ * Copyright (C) 2017 Google, Inc.
>> >+ * Brian Norris<[email protected]>
>> >+ *
>> >+ * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
>> >+ * Jeffy Chen<[email protected]>
>> >+ *
>> >+ * This program is free software; you can redistribute it and/or modify it
>> >+ * under the terms and conditions of the GNU General Public License,
>> >+ * version 2, as published by the Free Software Foundation.
>> >+ */
>> >+
>> >+#include <linux/acpi.h>
>> >+#include <linux/init.h>
>> >+#include <linux/module.h>
>> >+#include <linux/pci.h>
>> >+#include <linux/pm_wakeirq.h>
>> >+#include "pci.h"
>> >+
>> >+static bool of_pci_power_manageable(struct pci_dev *dev)
>> >+{
>> >+ return false;
>> >+}
>> >+
>> >+static int of_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>> >+{
>> >+ return -ENOSYS;
>> >+}
>> >+
>> >+static pci_power_t of_pci_get_power_state(struct pci_dev *dev)
>> >+{
>> >+ return PCI_UNKNOWN;
>> >+}
>> >+
>> >+static pci_power_t of_pci_choose_state(struct pci_dev *pdev)
>> >+{
>> >+ return PCI_POWER_ERROR;
>> >+}
>> >+
>> >+static int of_pci_wakeup(struct pci_dev *dev, bool enable)
>> >+{
>> >+ if (enable)
>> >+ dev_pm_enable_wake_irq(&dev->dev);
>> >+ else
>> >+ dev_pm_disable_wake_irq(&dev->dev);
>> >+ return 0;
>> >+}
>> >+
>> >+static bool of_pci_need_resume(struct pci_dev *dev)
>> >+{
>> >+ return false;
>> >+}
>> >+
>> >+static const struct pci_platform_pm_ops of_pci_platform_pm = {
>> >+ .is_manageable = of_pci_power_manageable,
>> >+ .set_state = of_pci_set_power_state,
>> >+ .get_state = of_pci_get_power_state,
>> >+ .choose_state = of_pci_choose_state,
>> >+ .set_wakeup = of_pci_wakeup,
>> >+ .need_resume = of_pci_need_resume,
>> >+};
>> >+
>> >+static int __init of_pci_init(void)
>> >+{
>> >+ if (!acpi_disabled)
>> >+ return 0;
>> >+
>> >+ pci_set_platform_pm(&of_pci_platform_pm);
> I guess no DT based system will override this?
i think the !acpi_disabled means acpi been disabled or CONFIG_ACPI is
undefined.
and pci-acpi.c would only work when we have CONFIG_ACPI.
but i have no idea about pci-mid.c or would it possible to have more
platform pm ops in the future...maybe we should add some dependency in
the Kconfig?
>
Hi Rafael and Rob,
Thanks for your reply.
On 12/27/2017 08:43 AM, Rafael J. Wysocki wrote:
> On Wednesday, December 27, 2017 12:35:52 AM CET Rob Herring wrote:
>> >On Tue, Dec 26, 2017 at 10:36:42AM +0800, Jeffy Chen wrote:
>>> > >We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
>>> > >so add definitions of the optional PCIe WAKE# pin for PCI devices.
>>> > >
>>> > >Also add an definition of the optional PCI interrupt pin for PCI
>>> > >devices to distinguish it from the PCIe WAKE# pin.
>> >
>> >By v13 you should drop "RFC". RFC implies not ready for merging.
> Which very much still is the case AFAICS.
>
maybe i should split this series, and send dt-binding patch and the pci
irq parsing patch along without RFC ?
> Thanks,
> Rafael
>
>
>
>
On Tue, Dec 26, 2017 at 7:32 PM, JeffyChen <[email protected]> wrote:
> Hi Rob,
>
> Thanks for your reply.
>
> On 12/27/2017 07:56 AM, Rob Herring wrote:
>>>
>>> >
>>> > drivers/of/of_pci_irq.c | 49 +++++++++++++++++++++++++++++++
>>
>> Please move this to drivers/pci/of.c (or perhaps create pci/of_irq.c).
>>
>>> > drivers/pci/Makefile | 1 +
>>> > drivers/pci/pci-driver.c | 10 +++++++
>>> > drivers/pci/pci-of.c | 75
>>> > ++++++++++++++++++++++++++++++++++++++++++++++++
>>
>> We already have drivers/pci/of.c. It's not clear what the difference is
>> from the filenames. Either merge with of.c or perhaps of-pm.c.
>
>
> this new file does something similar to the pci-acpi.c and pci-mid.c..
pci-acpi.c has similar things to pci/of.c. The naming is just not consistent.
Also, I plan to move the rest of drivers/of/of_pci* to drivers/pci.
> and i am agree the naming is not clear, maybe we can rename both of those
> files to something like pci-pm-***.c?
At least pci-acpi.c is more than just PM functions, so that doesn't
make sense. Given that all the ACPI related functions are in 1 file,
we should do the same for DT.
>
> Hi Rafael, do you think this would make sense?
>
>
[...]
>>> >+static int __init of_pci_init(void)
>>> >+{
>>> >+ if (!acpi_disabled)
>>> >+ return 0;
>>> >+
>>> >+ pci_set_platform_pm(&of_pci_platform_pm);
>>
>> I guess no DT based system will override this?
>
>
> i think the !acpi_disabled means acpi been disabled or CONFIG_ACPI is
> undefined.
>
> and pci-acpi.c would only work when we have CONFIG_ACPI.
>
> but i have no idea about pci-mid.c or would it possible to have more
> platform pm ops in the future...maybe we should add some dependency in the
> Kconfig?
It's probably fine given there are only 2 other implementations so far.
Rob
On Wed, Dec 27, 2017 at 4:30 PM, Rob Herring <[email protected]> wrote:
> On Tue, Dec 26, 2017 at 7:32 PM, JeffyChen <[email protected]> wrote:
>> Hi Rob,
>>
>> Thanks for your reply.
>>
>> On 12/27/2017 07:56 AM, Rob Herring wrote:
>>>>
>>>> >
>>>> > drivers/of/of_pci_irq.c | 49 +++++++++++++++++++++++++++++++
>>>
>>> Please move this to drivers/pci/of.c (or perhaps create pci/of_irq.c).
>>>
>>>> > drivers/pci/Makefile | 1 +
>>>> > drivers/pci/pci-driver.c | 10 +++++++
>>>> > drivers/pci/pci-of.c | 75
>>>> > ++++++++++++++++++++++++++++++++++++++++++++++++
>>>
>>> We already have drivers/pci/of.c. It's not clear what the difference is
>>> from the filenames. Either merge with of.c or perhaps of-pm.c.
>>
>>
>> this new file does something similar to the pci-acpi.c and pci-mid.c..
>
> pci-acpi.c has similar things to pci/of.c. The naming is just not consistent.
>
> Also, I plan to move the rest of drivers/of/of_pci* to drivers/pci.
>
>> and i am agree the naming is not clear, maybe we can rename both of those
>> files to something like pci-pm-***.c?
>
> At least pci-acpi.c is more than just PM functions, so that doesn't
> make sense. Given that all the ACPI related functions are in 1 file,
> we should do the same for DT.
Agreed.
Thanks,
Rafael
On Wed, 2017-12-27 at 09:30 -0600, Rob Herring wrote:
> On Tue, Dec 26, 2017 at 7:32 PM, JeffyChen <[email protected]>
> wrote:
> this new file does something similar to the pci-acpi.c and pci-mid.c..
> pci-acpi.c has similar things to pci/of.c. The naming is just not
> consistent.
> > and i am agree the naming is not clear, maybe we can rename both of
> > those
> > files to something like pci-pm-***.c?
>
> At least pci-acpi.c is more than just PM functions, so that doesn't
> make sense. Given that all the ACPI related functions are in 1 file,
> we should do the same for DT.
> > but i have no idea about pci-mid.c or would it possible to have more
> > platform pm ops in the future...maybe we should add some dependency
> > in the
> > Kconfig?
You may consider pci-mid.c as pci-sfi.c to some extend.
So, in that sense it more looks like pci-of.c would be the name for DT
case.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
* Jeffy Chen <[email protected]> [171226 02:41]:
> Currently we are handling PCIe WAKE# irq in mrvl wifi driver.
>
> Move it to rockchip pcie port since we are going to handle it in the
> pci core.
Yes in the PCIe case, the pcie port node is the right place for
the wakeirq instead of the child the mvl_wifi node. So one
question further down below to verify this..
> Also avoid this irq been considered as the PCI interrupt pin in the
> of_irq_parse_pci().
The above paragraph needs a bit more clarification to be
readable :)
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> @@ -719,15 +719,16 @@ ap_i2c_audio: &i2c8 {
> #size-cells = <2>;
> ranges;
>
> + interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-names = "pci", "wakeup";
> + pinctrl-names = "default";
> + pinctrl-0 = <&wlan_host_wake_l>;
> + wakeup-source;
> +
> mvl_wifi: wifi@0,0 {
> compatible = "pci1b4b,2b42";
> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
> - interrupt-parent = <&gpio0>;
> - interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> - pinctrl-names = "default";
> - pinctrl-0 = <&wlan_host_wake_l>;
> - wakeup-source;
> };
> };
> };
So the above modifies pcie@0,0 node. And that node describes
the particular PCIe port that the WLAN is connected to instead
of describing the whole PCIe controller device, right?
If so, then yeah it's totally where the wakeirq should be
defined for a PCIe device in the dts file :)
Regards,
Tony
On Fri, Dec 29, 2017 at 6:55 PM, Tony Lindgren <[email protected]> wrote:
> * Jeffy Chen <[email protected]> [171226 02:41]:
>> Currently we are handling PCIe WAKE# irq in mrvl wifi driver.
>>
>> Move it to rockchip pcie port since we are going to handle it in the
>> pci core.
>
> Yes in the PCIe case, the pcie port node is the right place for
> the wakeirq instead of the child the mvl_wifi node. So one
> question further down below to verify this..
You seem to be using a convention by which the port represents the
whole "slot" or "PCI device" (as an entity consisting of up to 8
functions) connected to it.
That is fair enough as long as the port is not the top of a more
complex branch of the PCIe hierarchy, so maybe that case needs to be
made special somehow?
Also, I would document the convention by mentioning that the wakeup
signaled via that interrupt doesn't apply to the port itself, but to
the functions (endpoints) below it.
>> Also avoid this irq been considered as the PCI interrupt pin in the
>> of_irq_parse_pci().
>
> The above paragraph needs a bit more clarification to be
> readable :)
>
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> @@ -719,15 +719,16 @@ ap_i2c_audio: &i2c8 {
>> #size-cells = <2>;
>> ranges;
>>
>> + interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
>> + interrupt-names = "pci", "wakeup";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&wlan_host_wake_l>;
>> + wakeup-source;
>> +
>> mvl_wifi: wifi@0,0 {
>> compatible = "pci1b4b,2b42";
>> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
>> - interrupt-parent = <&gpio0>;
>> - interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>> - pinctrl-names = "default";
>> - pinctrl-0 = <&wlan_host_wake_l>;
>> - wakeup-source;
>> };
>> };
>> };
>
> So the above modifies pcie@0,0 node. And that node describes
> the particular PCIe port that the WLAN is connected to instead
> of describing the whole PCIe controller device, right?
>
> If so, then yeah it's totally where the wakeirq should be
> defined for a PCIe device in the dts file :)
As long as the convention used here is clear to everybody, that is.
Thanks,
Rafael