diff mbox

[RFC,v8,7/7] PCI / PM: Add support for the PCIe WAKE# signal for OF

Message ID 20171026132840.20946-8-jeffy.chen@rock-chips.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Jeffy Chen Oct. 26, 2017, 1:28 p.m. UTC
Add pci-of.c to handle the PCIe WAKE# interrupt.

Also use the dedicated wakeirq infrastructure to simplify it.

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

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/pci/Makefile |   2 +-
 drivers/pci/pci-of.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/pci-of.c

Comments

Tony Lindgren Oct. 26, 2017, 2:42 p.m. UTC | #1
* Jeffy Chen <jeffy.chen@rock-chips.com> [171026 06:31]:
> Add pci-of.c to handle the PCIe WAKE# interrupt.
> 
> Also use the dedicated wakeirq infrastructure to simplify it.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>

Thanks for doing this, looks good to me from wakeirq point of view:

Acked-by: Tony Lindgren <tony@atomide.com>
Sinan Kaya Oct. 26, 2017, 3:16 p.m. UTC | #2
On 10/26/2017 9:28 AM, Jeffy Chen wrote:
>  drivers/pci/Makefile |   2 +-
>  drivers/pci/pci-of.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 137 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pci-of.c
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 66a21acad952..4f76dbdb024c 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
>  
>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
>  
> -obj-$(CONFIG_OF) += of.o
> +obj-$(CONFIG_OF) += of.o pci-of.o

If the intention is to push this to pci directory, this code needs to be made
platform agnostic by splitting into two pieces.

I think you can make this code common by abstracting the IRQ number and
have some generic code like pci-wake.c in pci directory without the of prefix
in this file.

Then, you can have some other OF specific code in the drivers/of directory
that reads the IRQ from OF and calls the common code in PCI directory.
Jeffy Chen Oct. 27, 2017, 2:09 a.m. UTC | #3
Hi Sinan,

Thanks for your reply.

On 10/26/2017 11:16 PM, Sinan Kaya wrote:
> If the intention is to push this to pci directory, this code needs to be made
> platform agnostic by splitting into two pieces.
>
> I think you can make this code common by abstracting the IRQ number and
> have some generic code like pci-wake.c in pci directory without the of prefix
> in this file.
>
> Then, you can have some other OF specific code in the drivers/of directory
> that reads the IRQ from OF and calls the common code in PCI directory.

right, that make sense to me...i'll wait for Brian & Bjorn & Rafael's 
opinions, then start rewriting these, thanks :)
Brian Norris Oct. 27, 2017, 5:55 a.m. UTC | #4
Hi Jeffy,

On Thu, Oct 26, 2017 at 09:28:40PM +0800, Jeffy Chen wrote:
> Add pci-of.c to handle the PCIe WAKE# interrupt.
> 
> Also use the dedicated wakeirq infrastructure to simplify it.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> 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/pci/Makefile |   2 +-
>  drivers/pci/pci-of.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 137 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pci-of.c
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 66a21acad952..4f76dbdb024c 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
>  
>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
>  
> -obj-$(CONFIG_OF) += of.o
> +obj-$(CONFIG_OF) += of.o pci-of.o
>  
>  ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>  
> diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
> new file mode 100644
> index 000000000000..55a33206fc84
> --- /dev/null
> +++ b/drivers/pci/pci-of.c
> @@ -0,0 +1,136 @@
> +/*
> + * OF PCI PM support
> + *
> + * Copyright (c) 2017 Rockchip, Inc.
> + *
> + * Author: Jeffy Chen <jeffy.chen@rock-chips.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/pm_wakeirq.h>
> +#include "pci.h"
> +
> +struct of_pci_pm_data {
> +	struct device	*dev;
> +	unsigned int	wakeup_irq;
> +	atomic_t	wakeup_cnt;
> +};
> +
> +static void *of_pci_setup(struct device *dev)
> +{
> +	struct of_pci_pm_data *data;
> +	int irq;
> +
> +	if (!dev->of_node)
> +		return NULL;
> +
> +	data = devm_kzalloc(dev, sizeof(struct of_pci_pm_data), GFP_KERNEL);
> +	if (!data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	irq = of_irq_get_byname(dev->of_node, "wakeup");
> +	if (irq < 0) {
> +		if (irq == -EPROBE_DEFER)
> +			return ERR_PTR(irq);
> +
> +		return NULL;
> +	}
> +
> +	data->wakeup_irq = irq;
> +	data->dev = dev;
> +
> +	device_init_wakeup(dev, false);
> +
> +	dev_info(dev, "Wakeup IRQ %d\n", irq);
> +	return data;
> +}
> +
> +static void *of_pci_setup_dev(struct pci_dev *pci_dev)
> +{
> +	return of_pci_setup(&pci_dev->dev);
> +}
> +
> +static void *of_pci_setup_host_bridge(struct pci_host_bridge *bridge)
> +{
> +	return of_pci_setup(bridge->dev.parent);
> +}
> +
> +static void of_pci_cleanup(void *pmdata)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (IS_ERR_OR_NULL(data)) {
> +		device_init_wakeup(data->dev, false);
> +		dev_pm_clear_wake_irq(data->dev);
> +	}
> +}
> +
> +static bool of_pci_can_wakeup(void *pmdata)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (IS_ERR_OR_NULL(data))
> +		return false;
> +
> +	return data->wakeup_irq > 0;
> +}
> +
> +static int of_pci_dev_wakeup(void *pmdata, bool enable)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +	struct device *dev = data->dev;
> +	int ret;
> +
> +	if (!enable) {
> +		dev_pm_clear_wake_irq(dev);
> +		return device_set_wakeup_enable(dev, false);
> +	}
> +
> +	ret = device_set_wakeup_enable(dev, true);
> +	if (ret < 0)
> +		return ret;

One reason this series is failing for me: the above is failing with
-EINVAL -- it seems like no one set the 'can_wakeup' flag for the
Marvell Wifi card I'm using. It seems like we probably *should* be
calling device_set_wakeup_capable() from your new setup method, to say
that we're capable of wakeup. The PCI PME code does this already, which
seems to make sense. There are also some network drivers that do it too
(e.g., ath10k), but not all.

Brian

> +
> +	ret = dev_pm_set_dedicated_wake_irq(dev, data->wakeup_irq);
> +	if (ret < 0) {
> +		device_set_wakeup_enable(dev, false);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int of_pci_bridge_wakeup(void *pmdata, bool enable)
> +{
> +	struct of_pci_pm_data *data = pmdata;
> +
> +	if (enable && atomic_inc_return(&data->wakeup_cnt) != 1)
> +		return 0;
> +
> +	if (!enable && atomic_dec_return(&data->wakeup_cnt) != 0)
> +		return 0;
> +
> +	return of_pci_dev_wakeup(pmdata, enable);
> +}
> +
> +static const struct pci_platform_pm_ops of_pci_platform_pm = {
> +	.setup_dev		= of_pci_setup_dev,
> +	.setup_host_bridge	= of_pci_setup_host_bridge,
> +	.cleanup		= of_pci_cleanup,
> +	.can_wakeup		= of_pci_can_wakeup,
> +	.dev_wakeup		= of_pci_dev_wakeup,
> +	.bridge_wakeup		= of_pci_bridge_wakeup,
> +};
> +
> +static int __init of_pci_init(void)
> +{
> +	pci_set_platform_pm(&of_pci_platform_pm);
> +	return 0;
> +}
> +arch_initcall(of_pci_init);
> -- 
> 2.11.0
> 
>
Jeffy Chen Oct. 27, 2017, 7:32 a.m. UTC | #5
Hi Brian,

On 10/27/2017 01:55 PM, Brian Norris wrote:
> One reason this series is failing for me: the above is failing with
> -EINVAL -- it seems like no one set the 'can_wakeup' flag for the
> Marvell Wifi card I'm using. It seems like we probably*should*  be
> calling device_set_wakeup_capable() from your new setup method, to say
> that we're capable of wakeup. The PCI PME code does this already, which
> seems to make sense. There are also some network drivers that do it too
> (e.g., ath10k), but not all.
>
right, thanks for noticing, will fix it in the new version...

and Sinan suggested to separate of code and wakeirq handling, would it 
make sense to rewrite this as pci-wakeirq.c(platform_pm_ops based on 
irq), and move of parse code into drivers/of/(maybe into 
of_irq_parse_pci()'s PCI interrupt parsing code)?

> Brian
>
Jeffy Chen Oct. 27, 2017, 7:37 a.m. UTC | #6
Hi Tony,

thanks for your Acked-by. but i'm moving the dedicated wakeirq setup to 
the setup(), so that the irq would be tracked in the /proc/interrupts.

please help to check the new v10 patches, thanks :)

On 10/26/2017 10:42 PM, Tony Lindgren wrote:
> * Jeffy Chen <jeffy.chen@rock-chips.com> [171026 06:31]:
>> Add pci-of.c to handle the PCIe WAKE# interrupt.
>>
>> Also use the dedicated wakeirq infrastructure to simplify it.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>
> Thanks for doing this, looks good to me from wakeirq point of view:
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>
>
>
Brian Norris Oct. 28, 2017, 12:04 a.m. UTC | #7
Hi Sinan,

I'm not sure I understand all your suggestions below.

On Thu, Oct 26, 2017 at 11:16:55AM -0400, Sinan Kaya wrote:
> On 10/26/2017 9:28 AM, Jeffy Chen wrote:
> >  drivers/pci/Makefile |   2 +-
> >  drivers/pci/pci-of.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 137 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/pci/pci-of.c
> > 
> > diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> > index 66a21acad952..4f76dbdb024c 100644
> > --- a/drivers/pci/Makefile
> > +++ b/drivers/pci/Makefile
> > @@ -49,7 +49,7 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
> >  
> >  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
> >  
> > -obj-$(CONFIG_OF) += of.o
> > +obj-$(CONFIG_OF) += of.o pci-of.o
> 
> If the intention is to push this to pci directory, this code needs to be made
> platform agnostic by splitting into two pieces.
> 
> I think you can make this code common by abstracting the IRQ number and
> have some generic code like pci-wake.c in pci directory without the of prefix
> in this file.

Why would 'pci-wake' be a good name for this? We're doing basically the
same things that pci-acpi.c is doing (it also can configure the WAKE#
signal, via ACPI firmware calls).

> Then, you can have some other OF specific code in the drivers/of directory
> that reads the IRQ from OF and calls the common code in PCI directory.

Why is drivers/of/ special? I thought in general, the DT maintainers
preferred to move domain-specific stuff into the respective subsystems.

Also, the extraction is a very tiny piece of code, and the logic around
walking a PCI tree is the more important part. And in the
meantime...Jeffy has sent two more revisions of this patch set already,
and he did the latter (I like his abstraction of PCI device trees,
shared between ACPI and OF code) but not the former (it's still all
'pci-of.c').

Feel free to comment further on here or on v10, but at the moment I'm
not sure I understand yet how your suggestions would improve things.

Brian
Sinan Kaya Oct. 28, 2017, 6:01 p.m. UTC | #8
On 10/27/2017 8:04 PM, Brian Norris wrote:
>> I think you can make this code common by abstracting the IRQ number and
>> have some generic code like pci-wake.c in pci directory without the of prefix
>> in this file.
> Why would 'pci-wake' be a good name for this? We're doing basically the
> same things that pci-acpi.c is doing (it also can configure the WAKE#
> signal, via ACPI firmware calls).
> 

I was looking for some code that can be used between OF and ACPI rather than
duplicating the code in two different places just to extract some information
from the platform firmware. 

My initial look at your code suggested it to be very generic except the IRQ
read function. 

I was expecting that the kernel's PCI Wake handling to be common whether the IRQ
information is coming from DT/ACPI.

There are different approaches to portability.

* One approach is to keep of specific stuff in OF directory. 
* Another approach is keep some common code can call it from multiple platform firmware
or from the ARCH directory.
* Another approach is have one file but put OF and ACPI specific pieces at the bottom.

It depends on the code. 

Bjorn should chime in here with his preference.
Brian Norris Oct. 28, 2017, 8:39 p.m. UTC | #9
On Sat, Oct 28, 2017 at 02:01:46PM -0400, Sinan Kaya wrote:
> I was looking for some code that can be used between OF and ACPI rather than
> duplicating the code in two different places just to extract some information
> from the platform firmware. 
> 
> My initial look at your code suggested it to be very generic except the IRQ
> read function. 
> 
> I was expecting that the kernel's PCI Wake handling to be common whether the IRQ
> information is coming from DT/ACPI.

I'm not an ACPI expert, but from reading the existing ACPI code handling
this wakeup configuration...they're not really that similar. Whereas for
device tree firmware there's not much magic to it -- the OS is really
just configuring a standard interrupt with wakeup capability -- ACPI
layers this all behind its GPE (General Purpose Event) abstraction,
which doesn't act like a typical interrupt. You manage GPE handlers with
a completely different set of APIs.

Anyway, Rafael seemed to have some of his own opinions (didn't like the
current approach it seems), so maybe we're all completely off track
here.

Regards,
Brian
diff mbox

Patch

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 66a21acad952..4f76dbdb024c 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -49,7 +49,7 @@  obj-$(CONFIG_PCI_ECAM) += ecam.o
 
 obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
 
-obj-$(CONFIG_OF) += of.o
+obj-$(CONFIG_OF) += of.o pci-of.o
 
 ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
 
diff --git a/drivers/pci/pci-of.c b/drivers/pci/pci-of.c
new file mode 100644
index 000000000000..55a33206fc84
--- /dev/null
+++ b/drivers/pci/pci-of.c
@@ -0,0 +1,136 @@ 
+/*
+ * OF PCI PM support
+ *
+ * Copyright (c) 2017 Rockchip, Inc.
+ *
+ * Author: Jeffy Chen <jeffy.chen@rock-chips.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/of_irq.h>
+#include <linux/pci.h>
+#include <linux/pm_wakeirq.h>
+#include "pci.h"
+
+struct of_pci_pm_data {
+	struct device	*dev;
+	unsigned int	wakeup_irq;
+	atomic_t	wakeup_cnt;
+};
+
+static void *of_pci_setup(struct device *dev)
+{
+	struct of_pci_pm_data *data;
+	int irq;
+
+	if (!dev->of_node)
+		return NULL;
+
+	data = devm_kzalloc(dev, sizeof(struct of_pci_pm_data), GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	irq = of_irq_get_byname(dev->of_node, "wakeup");
+	if (irq < 0) {
+		if (irq == -EPROBE_DEFER)
+			return ERR_PTR(irq);
+
+		return NULL;
+	}
+
+	data->wakeup_irq = irq;
+	data->dev = dev;
+
+	device_init_wakeup(dev, false);
+
+	dev_info(dev, "Wakeup IRQ %d\n", irq);
+	return data;
+}
+
+static void *of_pci_setup_dev(struct pci_dev *pci_dev)
+{
+	return of_pci_setup(&pci_dev->dev);
+}
+
+static void *of_pci_setup_host_bridge(struct pci_host_bridge *bridge)
+{
+	return of_pci_setup(bridge->dev.parent);
+}
+
+static void of_pci_cleanup(void *pmdata)
+{
+	struct of_pci_pm_data *data = pmdata;
+
+	if (IS_ERR_OR_NULL(data)) {
+		device_init_wakeup(data->dev, false);
+		dev_pm_clear_wake_irq(data->dev);
+	}
+}
+
+static bool of_pci_can_wakeup(void *pmdata)
+{
+	struct of_pci_pm_data *data = pmdata;
+
+	if (IS_ERR_OR_NULL(data))
+		return false;
+
+	return data->wakeup_irq > 0;
+}
+
+static int of_pci_dev_wakeup(void *pmdata, bool enable)
+{
+	struct of_pci_pm_data *data = pmdata;
+	struct device *dev = data->dev;
+	int ret;
+
+	if (!enable) {
+		dev_pm_clear_wake_irq(dev);
+		return device_set_wakeup_enable(dev, false);
+	}
+
+	ret = device_set_wakeup_enable(dev, true);
+	if (ret < 0)
+		return ret;
+
+	ret = dev_pm_set_dedicated_wake_irq(dev, data->wakeup_irq);
+	if (ret < 0) {
+		device_set_wakeup_enable(dev, false);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int of_pci_bridge_wakeup(void *pmdata, bool enable)
+{
+	struct of_pci_pm_data *data = pmdata;
+
+	if (enable && atomic_inc_return(&data->wakeup_cnt) != 1)
+		return 0;
+
+	if (!enable && atomic_dec_return(&data->wakeup_cnt) != 0)
+		return 0;
+
+	return of_pci_dev_wakeup(pmdata, enable);
+}
+
+static const struct pci_platform_pm_ops of_pci_platform_pm = {
+	.setup_dev		= of_pci_setup_dev,
+	.setup_host_bridge	= of_pci_setup_host_bridge,
+	.cleanup		= of_pci_cleanup,
+	.can_wakeup		= of_pci_can_wakeup,
+	.dev_wakeup		= of_pci_dev_wakeup,
+	.bridge_wakeup		= of_pci_bridge_wakeup,
+};
+
+static int __init of_pci_init(void)
+{
+	pci_set_platform_pm(&of_pci_platform_pm);
+	return 0;
+}
+arch_initcall(of_pci_init);