| Message ID | 20171225114742.18920-5-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Monday, December 25, 2017 12:47:41 PM CET 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. > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > 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/pci-driver.c | 10 ++++++++++ > include/linux/of_pci.h | 9 +++++++++ > 3 files changed, 68 insertions(+) > > diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c > index d39565d5477b..def884c1a37a 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"); Why is this a property of the bridge and not of the device itself? > + if (irq == -EPROBE_DEFER) Braces here, please. > + return irq; > + /* Ignore other errors, since a missing wakeup is non-fatal. */ > + else if (irq < 0) { > + dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq); > + return 0; > + } > + > + device_init_wakeup(&pdev->dev, true); Why do you call this before dev_pm_set_dedicated_wake_irq()? > + > + 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; > + } > + It would be more straightforward to call device_init_wakeup() here. > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq); > + > +void of_pci_teardown_wake_irq(struct pci_dev *pdev) > +{ > + if (!pdev->dev.power.wakeirq) > + return; > + > + 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 Thanks, Rafael
Hi Rafael, Thanks for your reply :) On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: >> >+ >> >+ dn = pci_device_to_OF_node(ppdev); >> >+ if (!dn) >> >+ return 0; >> >+ >> >+ irq = of_irq_get_byname(dn, "wakeup"); > Why is this a property of the bridge and not of the device itself? That is suggested by Brian, because in that way, the wakeup pin would not "tied to what exact device is installed (or no device, if it's a slot)." > >> >+ if (irq == -EPROBE_DEFER) > Braces here, please. ok, will fix in the next version. > >> >+ return irq; >> >+ /* Ignore other errors, since a missing wakeup is non-fatal. */ >> >+ else if (irq < 0) { >> >+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq); >> >+ return 0; >> >+ } >> >+ >> >+ device_init_wakeup(&pdev->dev, true); > Why do you call this before dev_pm_set_dedicated_wake_irq()? hmmm, i thought so too, but it turns out the dedicated wake irq framework requires device_init_wakeup(dev, true) before attach the wake irq: int device_wakeup_attach_irq(struct device *dev, struct wake_irq *wakeirq) { struct wakeup_source *ws; ws = dev->power.wakeup; if (!ws) { dev_err(dev, "forgot to call device_init_wakeup?\n"); return -EINVAL; > >> >+ >> >+ 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; >> >+ } >> >+ > It would be more straightforward to call device_init_wakeup() here. > >> >+ return 0; >> >+} >> >+EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq); >> >+ >> >+void of_pci_teardown_wake_irq(struct pci_dev *pdev) >> >+{ >> >+ if (!pdev->dev.power.wakeirq) >> >+ return; >> >+ >> >+ 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 > Thanks, > Rafael > > > >
On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: > Hi Rafael, > > Thanks for your reply :) > > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: > >> >+ > >> >+ dn = pci_device_to_OF_node(ppdev); > >> >+ if (!dn) > >> >+ return 0; > >> >+ > >> >+ irq = of_irq_get_byname(dn, "wakeup"); > > Why is this a property of the bridge and not of the device itself? > > That is suggested by Brian, because in that way, the wakeup pin would > not "tied to what exact device is installed (or no device, if it's a slot)." But I don't think it works when there are two devices using different WAKE# interrupt lines under the same bridge. Or how does it work then? > >> >+ if (irq == -EPROBE_DEFER) > > Braces here, please. > ok, will fix in the next version. > > > > >> >+ return irq; > >> >+ /* Ignore other errors, since a missing wakeup is non-fatal. */ > >> >+ else if (irq < 0) { > >> >+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq); > >> >+ return 0; > >> >+ } > >> >+ > >> >+ device_init_wakeup(&pdev->dev, true); > > Why do you call this before dev_pm_set_dedicated_wake_irq()? > > hmmm, i thought so too, but it turns out the dedicated wake irq > framework requires device_init_wakeup(dev, true) before attach the wake irq: > > int device_wakeup_attach_irq(struct device *dev, > struct wake_irq *wakeirq) > { > struct wakeup_source *ws; > > ws = dev->power.wakeup; > if (!ws) { > dev_err(dev, "forgot to call device_init_wakeup?\n"); > return -EINVAL; > Well, that's a framework issue, fair enough. That said, what if user space removes the wakeup source from under you concurrently via sysfs? Tony? Thanks, Rafael
* Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]: > On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: > > Hi Rafael, > > > > Thanks for your reply :) > > > > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: > > >> >+ > > >> >+ dn = pci_device_to_OF_node(ppdev); > > >> >+ if (!dn) > > >> >+ return 0; > > >> >+ > > >> >+ irq = of_irq_get_byname(dn, "wakeup"); > > > Why is this a property of the bridge and not of the device itself? > > > > That is suggested by Brian, because in that way, the wakeup pin would > > not "tied to what exact device is installed (or no device, if it's a slot)." > > But I don't think it works when there are two devices using different WAKE# > interrupt lines under the same bridge. Or how does it work then? It won't work currently for multiple devices but adding more than one wakeriq per device is doable. And I think we will have other cases where multiple wakeirqs are connected to a single device, so that issue should be sorted out sooner or later. And if requesting wakeirq for the PCI WAKE# lines at the PCI controller does the job, then maybe that's all we need to start with. Then in addition to that, we could do the following to allow PCI devices to request the wakeirq from the PCI controller: 1. PCI controller or framework implements a chained irq for the WAKE# lines assuming it can mask/unmask the WAKE# lines 2. PCI devices then can just request the wakeirq from the PCI controller And that's about it. Optionally we could leave out the dependency to having PCI devices implement PM runtime and just resume the parent (PCI controller) if PCI devices has not implemented PM runtime. > > >> >+ if (irq == -EPROBE_DEFER) > > > Braces here, please. > > ok, will fix in the next version. > > > > > > > >> >+ return irq; > > >> >+ /* Ignore other errors, since a missing wakeup is non-fatal. */ > > >> >+ else if (irq < 0) { > > >> >+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq); > > >> >+ return 0; > > >> >+ } > > >> >+ > > >> >+ device_init_wakeup(&pdev->dev, true); > > > Why do you call this before dev_pm_set_dedicated_wake_irq()? > > > > hmmm, i thought so too, but it turns out the dedicated wake irq > > framework requires device_init_wakeup(dev, true) before attach the wake irq: > > > > int device_wakeup_attach_irq(struct device *dev, > > struct wake_irq *wakeirq) > > { > > struct wakeup_source *ws; > > > > ws = dev->power.wakeup; > > if (!ws) { > > dev_err(dev, "forgot to call device_init_wakeup?\n"); > > return -EINVAL; > > > > Well, that's a framework issue, fair enough. > > That said, what if user space removes the wakeup source from under you > concurrently via sysfs? Tony? Hmm sounds racy, need to take a look. I think the only reason to have the wakeirq pointer there was to save memory. It might make sense to remove the wakeirq dependency here. Regards, Tony
On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony@atomide.com> wrote: > * Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]: >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: >> > Hi Rafael, >> > >> > Thanks for your reply :) >> > >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: >> > >> >+ >> > >> >+ dn = pci_device_to_OF_node(ppdev); >> > >> >+ if (!dn) >> > >> >+ return 0; >> > >> >+ >> > >> >+ irq = of_irq_get_byname(dn, "wakeup"); >> > > Why is this a property of the bridge and not of the device itself? >> > >> > That is suggested by Brian, because in that way, the wakeup pin would >> > not "tied to what exact device is installed (or no device, if it's a slot)." >> >> But I don't think it works when there are two devices using different WAKE# >> interrupt lines under the same bridge. Or how does it work then? > > It won't work currently for multiple devices but adding more than > one wakeriq per device is doable. And I think we will have other > cases where multiple wakeirqs are connected to a single device, so > that issue should be sorted out sooner or later. > > And if requesting wakeirq for the PCI WAKE# lines at the PCI > controller does the job, then maybe that's all we need to start with. These are expected to be out-of-band, so not having anything to do with the Root Complex. In-band PME Messages go through the PCIe hierarchy, but that is a standard mechanism and it is supported already. WAKE# are platform-specific, pretty much by definition and I guess that on most ARM boards they are just going to be some kind of GPIO pins. > Then in addition to that, we could do the following to allow > PCI devices to request the wakeirq from the PCI controller: > > 1. PCI controller or framework implements a chained irq for > the WAKE# lines assuming it can mask/unmask the WAKE# lines > > 2. PCI devices then can just request the wakeirq from the PCI > controller > > And that's about it. Optionally we could leave out the dependency > to having PCI devices implement PM runtime and just resume the > parent (PCI controller) if PCI devices has not implemented > PM runtime. So if my understanding is correct, DT should give you the WAKE# IRQ for the given endpoint PCI device and you only are expected to request it. The rest should just follow from the other pieces of information in the DT. With the quite obvious caveat that the same IRQ may be used as WAKE# for multiple endpoint devices (which BTW need not be under the same bridge even). >> > >> >+ if (irq == -EPROBE_DEFER) >> > > Braces here, please. >> > ok, will fix in the next version. >> > >> > > >> > >> >+ return irq; >> > >> >+ /* Ignore other errors, since a missing wakeup is non-fatal. */ >> > >> >+ else if (irq < 0) { >> > >> >+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq); >> > >> >+ return 0; >> > >> >+ } >> > >> >+ >> > >> >+ device_init_wakeup(&pdev->dev, true); >> > > Why do you call this before dev_pm_set_dedicated_wake_irq()? >> > >> > hmmm, i thought so too, but it turns out the dedicated wake irq >> > framework requires device_init_wakeup(dev, true) before attach the wake irq: >> > >> > int device_wakeup_attach_irq(struct device *dev, >> > struct wake_irq *wakeirq) >> > { >> > struct wakeup_source *ws; >> > >> > ws = dev->power.wakeup; >> > if (!ws) { >> > dev_err(dev, "forgot to call device_init_wakeup?\n"); >> > return -EINVAL; >> > >> >> Well, that's a framework issue, fair enough. >> >> That said, what if user space removes the wakeup source from under you >> concurrently via sysfs? Tony? > > Hmm sounds racy, need to take a look. Not only racy, as I don't see anything to prevent user space from making the dev->power.wakeup wakeup source go away via sysfs at any time *after* the IRQ has been requested. Pretty much right after dev_pm_set_dedicated_wake_irq() has returned, device_wakeup_disable() may be called on the device via wakeup_store() and it doesn't even check if the device has a wakeup irq. > I think the only reason > to have the wakeirq pointer there was to save memory. It might > make sense to remove the wakeirq dependency here. Well, that looks necessary to be honest. Thanks, Rafael
* Rafael J. Wysocki <rafael@kernel.org> [171228 00:51]: > On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony@atomide.com> wrote: > > * Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]: > >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: > >> > Hi Rafael, > >> > > >> > Thanks for your reply :) > >> > > >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: > >> > >> >+ > >> > >> >+ dn = pci_device_to_OF_node(ppdev); > >> > >> >+ if (!dn) > >> > >> >+ return 0; > >> > >> >+ > >> > >> >+ irq = of_irq_get_byname(dn, "wakeup"); > >> > > Why is this a property of the bridge and not of the device itself? > >> > > >> > That is suggested by Brian, because in that way, the wakeup pin would > >> > not "tied to what exact device is installed (or no device, if it's a slot)." > >> > >> But I don't think it works when there are two devices using different WAKE# > >> interrupt lines under the same bridge. Or how does it work then? > > > > It won't work currently for multiple devices but adding more than > > one wakeriq per device is doable. And I think we will have other > > cases where multiple wakeirqs are connected to a single device, so > > that issue should be sorted out sooner or later. > > > > And if requesting wakeirq for the PCI WAKE# lines at the PCI > > controller does the job, then maybe that's all we need to start with. > > These are expected to be out-of-band, so not having anything to do > with the Root Complex. > > In-band PME Messages go through the PCIe hierarchy, but that is a > standard mechanism and it is supported already. > > WAKE# are platform-specific, pretty much by definition and I guess > that on most ARM boards they are just going to be some kind of GPIO > pins. OK. So probably supporting the following two configurations should be enough then: 1. One or more WAKE# lines configured as a wakeirq for the PCI controller When the wakeirq calls pm_wakeup_event() for the PCI controller device driver, the PCI controller wakes up and can deal with it's child devices 2. Optionally a WAKE# line from a PCI device configured as wakeirq for the PCI device driver In this case calling the PM runtime resume in the child PCI device will also wake up the parent PCI controller, and then the PCI controller can deal with it's children Seems like this series is pretty close to 1 above except we need to have a list of wakeirqs per device instead of just one. And option 2 should already work as long as the PCI device driver parses and configures the wakeirq. > > Then in addition to that, we could do the following to allow > > PCI devices to request the wakeirq from the PCI controller: > > > > 1. PCI controller or framework implements a chained irq for > > the WAKE# lines assuming it can mask/unmask the WAKE# lines > > > > 2. PCI devices then can just request the wakeirq from the PCI > > controller > > > > And that's about it. Optionally we could leave out the dependency > > to having PCI devices implement PM runtime and just resume the > > parent (PCI controller) if PCI devices has not implemented > > PM runtime. > > So if my understanding is correct, DT should give you the WAKE# IRQ > for the given endpoint PCI device and you only are expected to request > it. The rest should just follow from the other pieces of information > in the DT. Yeah and it seems that we should allow configuring both cases 1 and 2 above. > With the quite obvious caveat that the same IRQ may be used as WAKE# > for multiple endpoint devices (which BTW need not be under the same > bridge even). And with the shared interrupts we can't do the masking/unmasking automatically.. > >> > >> >+ if (irq == -EPROBE_DEFER) > >> > > Braces here, please. > >> > ok, will fix in the next version. > >> > > >> > > > >> > >> >+ return irq; > >> > >> >+ /* Ignore other errors, since a missing wakeup is non-fatal. */ > >> > >> >+ else if (irq < 0) { > >> > >> >+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq); > >> > >> >+ return 0; > >> > >> >+ } > >> > >> >+ > >> > >> >+ device_init_wakeup(&pdev->dev, true); > >> > > Why do you call this before dev_pm_set_dedicated_wake_irq()? > >> > > >> > hmmm, i thought so too, but it turns out the dedicated wake irq > >> > framework requires device_init_wakeup(dev, true) before attach the wake irq: > >> > > >> > int device_wakeup_attach_irq(struct device *dev, > >> > struct wake_irq *wakeirq) > >> > { > >> > struct wakeup_source *ws; > >> > > >> > ws = dev->power.wakeup; > >> > if (!ws) { > >> > dev_err(dev, "forgot to call device_init_wakeup?\n"); > >> > return -EINVAL; > >> > > >> > >> Well, that's a framework issue, fair enough. > >> > >> That said, what if user space removes the wakeup source from under you > >> concurrently via sysfs? Tony? > > > > Hmm sounds racy, need to take a look. > > Not only racy, as I don't see anything to prevent user space from > making the dev->power.wakeup wakeup source go away via sysfs at any > time *after* the IRQ has been requested. Currently nothing happens with wakeirqs if there's no struct wakeup_source. On device_wakeup_enable() we call device_wakeup_attach() that just copies dev->power.wakeirq to ws->wakeirq. And when struct wake_source is freed the device should be active and wakeirq disabled. Or are you seeing other issues here? > Pretty much right after dev_pm_set_dedicated_wake_irq() has returned, > device_wakeup_disable() may be called on the device via wakeup_store() > and it doesn't even check if the device has a wakeup irq. > > > I think the only reason > > to have the wakeirq pointer there was to save memory. It might > > make sense to remove the wakeirq dependency here. > > Well, that looks necessary to be honest. Seems like we're OK there except for the race. But I still wonder if could just get rid of wakeirq in struct wakeup_source. Maybe all we need is to see if dev->power.wakeup is allocated for the wakeirqs. Regards, Tony
On Thu, Dec 28, 2017 at 5:22 AM, Tony Lindgren <tony@atomide.com> wrote: > * Rafael J. Wysocki <rafael@kernel.org> [171228 00:51]: >> On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony@atomide.com> wrote: >> > * Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]: >> >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: >> >> > Hi Rafael, >> >> > >> >> > Thanks for your reply :) >> >> > >> >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: >> >> > >> >+ >> >> > >> >+ dn = pci_device_to_OF_node(ppdev); >> >> > >> >+ if (!dn) >> >> > >> >+ return 0; >> >> > >> >+ >> >> > >> >+ irq = of_irq_get_byname(dn, "wakeup"); >> >> > > Why is this a property of the bridge and not of the device itself? >> >> > >> >> > That is suggested by Brian, because in that way, the wakeup pin would >> >> > not "tied to what exact device is installed (or no device, if it's a slot)." >> >> >> >> But I don't think it works when there are two devices using different WAKE# >> >> interrupt lines under the same bridge. Or how does it work then? >> > >> > It won't work currently for multiple devices but adding more than >> > one wakeriq per device is doable. And I think we will have other >> > cases where multiple wakeirqs are connected to a single device, so >> > that issue should be sorted out sooner or later. >> > >> > And if requesting wakeirq for the PCI WAKE# lines at the PCI >> > controller does the job, then maybe that's all we need to start with. >> >> These are expected to be out-of-band, so not having anything to do >> with the Root Complex. >> >> In-band PME Messages go through the PCIe hierarchy, but that is a >> standard mechanism and it is supported already. >> >> WAKE# are platform-specific, pretty much by definition and I guess >> that on most ARM boards they are just going to be some kind of GPIO >> pins. > > OK. So probably supporting the following two configurations > should be enough then: > > 1. One or more WAKE# lines configured as a wakeirq for the PCI > controller > > When the wakeirq calls pm_wakeup_event() for the PCI controller > device driver, the PCI controller wakes up and can deal with > it's child devices But this shouldn't be necessary at all. Or if it is, I wonder why that's the case. I'm assuming that we're talking about PCI Express here, which has two wakeup mechanisms defined, one of which is based on using PME Messages (Beacon) and the second one is WAKE#: "The WAKE# mechanism uses sideband signaling to implement wakeup functionality. WAKE# is an “open drain” signal asserted by components requesting wakeup and observed by the associated power controller." (from PCIe Base Spec 3.0). [And there's a diagram showing the routing of WAKE# in two cases in Figure 5-4: Conceptual Diagrams Showing Two Example Cases of WAKE# Routing.] Note that WAKE# is defined to be "observed by the associated power controller", so I'm not sure what the PCI controller's role in the handing of it is at all. > 2. Optionally a WAKE# line from a PCI device configured as wakeirq > for the PCI device driver > > In this case calling the PM runtime resume in the child > PCI device will also wake up the parent PCI controller, > and then the PCI controller can deal with it's children > > Seems like this series is pretty close to 1 above except > we need to have a list of wakeirqs per device instead of > just one. And option 2 should already work as long as the > PCI device driver parses and configures the wakeirq. Well, this is confusing, because as I said above, option 1 doesn't look relevant even. >> > Then in addition to that, we could do the following to allow >> > PCI devices to request the wakeirq from the PCI controller: >> > >> > 1. PCI controller or framework implements a chained irq for >> > the WAKE# lines assuming it can mask/unmask the WAKE# lines >> > >> > 2. PCI devices then can just request the wakeirq from the PCI >> > controller >> > >> > And that's about it. Optionally we could leave out the dependency >> > to having PCI devices implement PM runtime and just resume the >> > parent (PCI controller) if PCI devices has not implemented >> > PM runtime. >> >> So if my understanding is correct, DT should give you the WAKE# IRQ >> for the given endpoint PCI device and you only are expected to request >> it. The rest should just follow from the other pieces of information >> in the DT. > > Yeah and it seems that we should allow configuring both cases > 1 and 2 above. > >> With the quite obvious caveat that the same IRQ may be used as WAKE# >> for multiple endpoint devices (which BTW need not be under the same >> bridge even). > > And with the shared interrupts we can't do the masking/unmasking > automatically.. Or we need to use reference counting (so actually the wakeup IRQs are not dedicated). >> >> > >> >+ if (irq == -EPROBE_DEFER) >> >> > > Braces here, please. >> >> > ok, will fix in the next version. >> >> > >> >> > > >> >> > >> >+ return irq; >> >> > >> >+ /* Ignore other errors, since a missing wakeup is non-fatal. */ >> >> > >> >+ else if (irq < 0) { >> >> > >> >+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq); >> >> > >> >+ return 0; >> >> > >> >+ } >> >> > >> >+ >> >> > >> >+ device_init_wakeup(&pdev->dev, true); >> >> > > Why do you call this before dev_pm_set_dedicated_wake_irq()? >> >> > >> >> > hmmm, i thought so too, but it turns out the dedicated wake irq >> >> > framework requires device_init_wakeup(dev, true) before attach the wake irq: >> >> > >> >> > int device_wakeup_attach_irq(struct device *dev, >> >> > struct wake_irq *wakeirq) >> >> > { >> >> > struct wakeup_source *ws; >> >> > >> >> > ws = dev->power.wakeup; >> >> > if (!ws) { >> >> > dev_err(dev, "forgot to call device_init_wakeup?\n"); >> >> > return -EINVAL; >> >> > >> >> >> >> Well, that's a framework issue, fair enough. >> >> >> >> That said, what if user space removes the wakeup source from under you >> >> concurrently via sysfs? Tony? >> > >> > Hmm sounds racy, need to take a look. >> >> Not only racy, as I don't see anything to prevent user space from >> making the dev->power.wakeup wakeup source go away via sysfs at any >> time *after* the IRQ has been requested. > > Currently nothing happens with wakeirqs if there's no struct > wakeup_source. On device_wakeup_enable() we call device_wakeup_attach() > that just copies dev->power.wakeirq to ws->wakeirq. And when struct > wake_source is freed the device should be active and wakeirq > disabled. Or are you seeing other issues here? I'm suspicious about one thing, but I need to look deeper into the code. :-) >> Pretty much right after dev_pm_set_dedicated_wake_irq() has returned, >> device_wakeup_disable() may be called on the device via wakeup_store() >> and it doesn't even check if the device has a wakeup irq. >> >> > I think the only reason >> > to have the wakeirq pointer there was to save memory. It might >> > make sense to remove the wakeirq dependency here. >> >> Well, that looks necessary to be honest. > > Seems like we're OK there except for the race. But I still wonder > if could just get rid of wakeirq in struct wakeup_source. Maybe > all we need is to see if dev->power.wakeup is allocated for the > wakeirqs. I guess "no", but let me get back to you when I have looked at things in more detail. Thanks, Rafael
* Rafael J. Wysocki <rafael@kernel.org> [171228 12:21]: > On Thu, Dec 28, 2017 at 5:22 AM, Tony Lindgren <tony@atomide.com> wrote: > > * Rafael J. Wysocki <rafael@kernel.org> [171228 00:51]: > >> On Wed, Dec 27, 2017 at 4:08 PM, Tony Lindgren <tony@atomide.com> wrote: > >> > * Rafael J. Wysocki <rjw@rjwysocki.net> [171227 01:00]: > >> >> On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: > >> >> > Hi Rafael, > >> >> > > >> >> > Thanks for your reply :) > >> >> > > >> >> > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: > >> >> > >> >+ > >> >> > >> >+ dn = pci_device_to_OF_node(ppdev); > >> >> > >> >+ if (!dn) > >> >> > >> >+ return 0; > >> >> > >> >+ > >> >> > >> >+ irq = of_irq_get_byname(dn, "wakeup"); > >> >> > > Why is this a property of the bridge and not of the device itself? > >> >> > > >> >> > That is suggested by Brian, because in that way, the wakeup pin would > >> >> > not "tied to what exact device is installed (or no device, if it's a slot)." > >> >> > >> >> But I don't think it works when there are two devices using different WAKE# > >> >> interrupt lines under the same bridge. Or how does it work then? > >> > > >> > It won't work currently for multiple devices but adding more than > >> > one wakeriq per device is doable. And I think we will have other > >> > cases where multiple wakeirqs are connected to a single device, so > >> > that issue should be sorted out sooner or later. > >> > > >> > And if requesting wakeirq for the PCI WAKE# lines at the PCI > >> > controller does the job, then maybe that's all we need to start with. > >> > >> These are expected to be out-of-band, so not having anything to do > >> with the Root Complex. > >> > >> In-band PME Messages go through the PCIe hierarchy, but that is a > >> standard mechanism and it is supported already. > >> > >> WAKE# are platform-specific, pretty much by definition and I guess > >> that on most ARM boards they are just going to be some kind of GPIO > >> pins. > > > > OK. So probably supporting the following two configurations > > should be enough then: > > > > 1. One or more WAKE# lines configured as a wakeirq for the PCI > > controller > > > > When the wakeirq calls pm_wakeup_event() for the PCI controller > > device driver, the PCI controller wakes up and can deal with > > it's child devices > > But this shouldn't be necessary at all. Or if it is, I wonder why > that's the case. Well Brian had a concern where we would have to implement PM runtime for all device drivers for PCI devices. > I'm assuming that we're talking about PCI Express here, which has two > wakeup mechanisms defined, one of which is based on using PME Messages > (Beacon) and the second one is WAKE#: > > "The WAKE# mechanism uses sideband signaling to implement wakeup > functionality. WAKE# is > an “open drain” signal asserted by components requesting wakeup and > observed by the associated > power controller." > > (from PCIe Base Spec 3.0). [And there's a diagram showing the routing > of WAKE# in two cases in Figure 5-4: Conceptual Diagrams Showing Two > Example Cases of WAKE# Routing.] Thanks for the pointer, I had not seen that :) So the use cases I was trying to describe above are similar to the wiring in the PCIe Base Spec 3.0 "Figure 5-4" , but numbered the other way around. > Note that WAKE# is defined to be "observed by the associated power > controller", so I'm not sure what the PCI controller's role in the > handing of it is at all. To me it seems the "switch" part stays at least partially powered and then in-band PME messages are used after host is woken up to figure out which WAKE# triggered? > > 2. Optionally a WAKE# line from a PCI device configured as wakeirq > > for the PCI device driver > > > > In this case calling the PM runtime resume in the child > > PCI device will also wake up the parent PCI controller, > > and then the PCI controller can deal with it's children > > > > Seems like this series is pretty close to 1 above except > > we need to have a list of wakeirqs per device instead of > > just one. And option 2 should already work as long as the > > PCI device driver parses and configures the wakeirq. > > Well, this is confusing, because as I said above, option 1 doesn't > look relevant even. So isn't my option 1 above similar to the PCIe spec "Figure 5-4" case 2? Anyways, let's standardize on the "Figure 5-4" naming from now to avoid confusion :) > >> > Then in addition to that, we could do the following to allow > >> > PCI devices to request the wakeirq from the PCI controller: > >> > > >> > 1. PCI controller or framework implements a chained irq for > >> > the WAKE# lines assuming it can mask/unmask the WAKE# lines > >> > > >> > 2. PCI devices then can just request the wakeirq from the PCI > >> > controller > >> > > >> > And that's about it. Optionally we could leave out the dependency > >> > to having PCI devices implement PM runtime and just resume the > >> > parent (PCI controller) if PCI devices has not implemented > >> > PM runtime. > >> > >> So if my understanding is correct, DT should give you the WAKE# IRQ > >> for the given endpoint PCI device and you only are expected to request > >> it. The rest should just follow from the other pieces of information > >> in the DT. > > > > Yeah and it seems that we should allow configuring both cases > > 1 and 2 above. > > > >> With the quite obvious caveat that the same IRQ may be used as WAKE# > >> for multiple endpoint devices (which BTW need not be under the same > >> bridge even). > > > > And with the shared interrupts we can't do the masking/unmasking > > automatically.. > > Or we need to use reference counting (so actually the wakeup IRQs are > not dedicated). Yeah. FYI, for the dedicated wakeirq cases I have, we need to keep them masked during runtime to avoid tons of interrupts as they are often wired to the RX pins. > >> >> > >> >+ if (irq == -EPROBE_DEFER) > >> >> > > Braces here, please. > >> >> > ok, will fix in the next version. > >> >> > > >> >> > > > >> >> > >> >+ return irq; > >> >> > >> >+ /* Ignore other errors, since a missing wakeup is non-fatal. */ > >> >> > >> >+ else if (irq < 0) { > >> >> > >> >+ dev_info(&pdev->dev, "cannot get wakeup interrupt: %d\n", irq); > >> >> > >> >+ return 0; > >> >> > >> >+ } > >> >> > >> >+ > >> >> > >> >+ device_init_wakeup(&pdev->dev, true); > >> >> > > Why do you call this before dev_pm_set_dedicated_wake_irq()? > >> >> > > >> >> > hmmm, i thought so too, but it turns out the dedicated wake irq > >> >> > framework requires device_init_wakeup(dev, true) before attach the wake irq: > >> >> > > >> >> > int device_wakeup_attach_irq(struct device *dev, > >> >> > struct wake_irq *wakeirq) > >> >> > { > >> >> > struct wakeup_source *ws; > >> >> > > >> >> > ws = dev->power.wakeup; > >> >> > if (!ws) { > >> >> > dev_err(dev, "forgot to call device_init_wakeup?\n"); > >> >> > return -EINVAL; > >> >> > > >> >> > >> >> Well, that's a framework issue, fair enough. > >> >> > >> >> That said, what if user space removes the wakeup source from under you > >> >> concurrently via sysfs? Tony? > >> > > >> > Hmm sounds racy, need to take a look. > >> > >> Not only racy, as I don't see anything to prevent user space from > >> making the dev->power.wakeup wakeup source go away via sysfs at any > >> time *after* the IRQ has been requested. > > > > Currently nothing happens with wakeirqs if there's no struct > > wakeup_source. On device_wakeup_enable() we call device_wakeup_attach() > > that just copies dev->power.wakeirq to ws->wakeirq. And when struct > > wake_source is freed the device should be active and wakeirq > > disabled. Or are you seeing other issues here? > > I'm suspicious about one thing, but I need to look deeper into the code. :-) OK. My response time will be laggy this week in case you find something that needs urgent fixing :) Regards, Tony
Hi, Trying to catch up on this thread... On Wed, Dec 27, 2017 at 01:57:07AM +0100, Rafael J. Wysocki wrote: > On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: > > Hi Rafael, > > > > Thanks for your reply :) > > > > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: > > >> >+ > > >> >+ dn = pci_device_to_OF_node(ppdev); > > >> >+ if (!dn) > > >> >+ return 0; > > >> >+ > > >> >+ irq = of_irq_get_byname(dn, "wakeup"); > > > Why is this a property of the bridge and not of the device itself? Wait, isn't 'dn' the port node, not the bridge node? > > That is suggested by Brian, because in that way, the wakeup pin would > > not "tied to what exact device is installed (or no device, if it's a slot)." I believe my thinking has evolved a bit over time, and I definitely am not the one true authority on this. I'll explain my main concerns, and whatever solution resolves these concerns is fine with me. * I was primarily interested in avoiding handling WAKE# in the endpoint drivers (e.g., as mwifiex is today). * I was also interested in not having to redefine a new DT device node (with new "pciABCD,1234" compatible property) for each new device attached. That just won't work for removable cards. I need to reread the rest of this thread a few times to really understand what Rafael and Tony are discussing. But I feel like some of this is still moving away from the second point above. > But I don't think it works when there are two devices using different WAKE# > interrupt lines under the same bridge. Or how does it work then? Brian
On Friday, January 5, 2018 1:41:31 AM CET Brian Norris wrote: > Hi, > > Trying to catch up on this thread... > > On Wed, Dec 27, 2017 at 01:57:07AM +0100, Rafael J. Wysocki wrote: > > On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: > > > Hi Rafael, > > > > > > Thanks for your reply :) > > > > > > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: > > > >> >+ > > > >> >+ dn = pci_device_to_OF_node(ppdev); > > > >> >+ if (!dn) > > > >> >+ return 0; > > > >> >+ > > > >> >+ irq = of_irq_get_byname(dn, "wakeup"); > > > > Why is this a property of the bridge and not of the device itself? > > Wait, isn't 'dn' the port node, not the bridge node? I may be confused about the structure you have in DT. In the device hierarchy PCIe ports are represented as bridges. > > > That is suggested by Brian, because in that way, the wakeup pin would > > > not "tied to what exact device is installed (or no device, if it's a slot)." > > I believe my thinking has evolved a bit over time, and I definitely am > not the one true authority on this. I'll explain my main concerns, and > whatever solution resolves these concerns is fine with me. > > * I was primarily interested in avoiding handling WAKE# in the endpoint > drivers (e.g., as mwifiex is today). OK, everybody on this thread is interested in that. :-) > * I was also interested in not having to redefine a new DT device > node (with new "pciABCD,1234" compatible property) for each new device > attached. That just won't work for removable cards. So if you make it the property of a bridge, it should be fine (as long as the bridge itself is not removable). > I need to reread the rest of this thread a few times to really > understand what Rafael and Tony are discussing. But I feel like some of > this is still moving away from the second point above. > > > But I don't think it works when there are two devices using different WAKE# > > interrupt lines under the same bridge. Or how does it work then? We seem to have agreed that this case needs to be neglected here. The "wakeup-interrupt" property at the bridge level basically has to be defined as the wakeup interrupt for all devices on the bus under the bridge. Thanks, Rafael
Hi Rafael, Sorry once again on the delays...My email hygiene has been getting bad, so mail gets buried. On Fri, Jan 05, 2018 at 02:13:33AM +0100, Rafael J. Wysocki wrote: > On Friday, January 5, 2018 1:41:31 AM CET Brian Norris wrote: > > On Wed, Dec 27, 2017 at 01:57:07AM +0100, Rafael J. Wysocki wrote: > > > On Tuesday, December 26, 2017 2:06:47 AM CET JeffyChen wrote: > > > > On 12/26/2017 08:11 AM, Rafael J. Wysocki wrote: > > > > >> >+ > > > > >> >+ dn = pci_device_to_OF_node(ppdev); > > > > >> >+ if (!dn) > > > > >> >+ return 0; > > > > >> >+ > > > > >> >+ irq = of_irq_get_byname(dn, "wakeup"); > > > > > Why is this a property of the bridge and not of the device itself? > > > > Wait, isn't 'dn' the port node, not the bridge node? > > I may be confused about the structure you have in DT. Probably :) And my DT structure may not be correct. But it's easily available to review. See arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi: http://elixir.free-electrons.com/linux/v4.14.2/source/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi#L705 It has a bridge->port->device nesting hierarchy. I'm not actually sure where this came from exactly, but I *thought* it was derived from the standard documentation: Documentation/devicetree/bindings/pci/pci.txt PCI Bus Binding to: IEEE Std 1275-1994 http://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf I'm not so sure now... > In the device hierarchy PCIe ports are represented as bridges. But device hierarchy can be slightly different than DT hierarchy. In our case, pcie@0,0 seems to correspond to the port, but has no companion "device". Or maybe I'm really off-base. > > > > That is suggested by Brian, because in that way, the wakeup pin would > > > > not "tied to what exact device is installed (or no device, if it's a slot)." > > > > I believe my thinking has evolved a bit over time, and I definitely am > > not the one true authority on this. I'll explain my main concerns, and > > whatever solution resolves these concerns is fine with me. > > > > * I was primarily interested in avoiding handling WAKE# in the endpoint > > drivers (e.g., as mwifiex is today). > > OK, everybody on this thread is interested in that. :-) Sure :) > > * I was also interested in not having to redefine a new DT device > > node (with new "pciABCD,1234" compatible property) for each new device > > attached. That just won't work for removable cards. > > So if you make it the property of a bridge, it should be fine (as long > as the bridge itself is not removable). OK...in that case you've answered your question at the top: "Why is this a property of the bridge and not of the device itself?" I think Jeffy answered similarly, but this walked you through how *I* got to that conclusion, at least. > > I need to reread the rest of this thread a few times to really > > understand what Rafael and Tony are discussing. But I feel like some of > > this is still moving away from the second point above. > > > > > But I don't think it works when there are two devices using different WAKE# > > > interrupt lines under the same bridge. Or how does it work then? > > We seem to have agreed that this case needs to be neglected here. OK, I guess that's a reasonable conclusion. > The "wakeup-interrupt" property at the bridge level basically has to be defined > as the wakeup interrupt for all devices on the bus under the bridge. The only thing I'm at a loss for is whether this goes in (referring to rk3399-gru.dtsi) &pcie or &pci_rootport. I think some versions of this series have been aiming for the former, and some the latter. Brian
* Brian Norris <briannorris@chromium.org> [180125 01:22]: > On Fri, Jan 05, 2018 at 02:13:33AM +0100, Rafael J. Wysocki wrote: > > The "wakeup-interrupt" property at the bridge level basically has to be defined > > as the wakeup interrupt for all devices on the bus under the bridge. > > The only thing I'm at a loss for is whether this goes in (referring to > rk3399-gru.dtsi) &pcie or &pci_rootport. I think some versions of this > series have been aiming for the former, and some the latter. I'd keep the wakeup interrupt property at the rootport level. That way it can work with whatever pcie card that might be plugged into that slot. That is in case it's just a slot and not hardwired pcie device :) Regards, Tony
On Thu, Jan 25, 2018 at 5:40 PM, Tony Lindgren <tony@atomide.com> wrote: > * Brian Norris <briannorris@chromium.org> [180125 01:22]: >> On Fri, Jan 05, 2018 at 02:13:33AM +0100, Rafael J. Wysocki wrote: >> > The "wakeup-interrupt" property at the bridge level basically has to be defined >> > as the wakeup interrupt for all devices on the bus under the bridge. >> >> The only thing I'm at a loss for is whether this goes in (referring to >> rk3399-gru.dtsi) &pcie or &pci_rootport. I think some versions of this >> series have been aiming for the former, and some the latter. > > I'd keep the wakeup interrupt property at the rootport level. That way > it can work with whatever pcie card that might be plugged into that > slot. That is in case it's just a slot and not hardwired pcie device :) Do I understand correctly that &pcie is the device and the &pci_rootport is the port that device is connected to?
Hi, On Thu, Jan 25, 2018 at 05:54:23PM +0100, Rafael J. Wysocki wrote: > On Thu, Jan 25, 2018 at 5:40 PM, Tony Lindgren <tony@atomide.com> wrote: > > * Brian Norris <briannorris@chromium.org> [180125 01:22]: > >> On Fri, Jan 05, 2018 at 02:13:33AM +0100, Rafael J. Wysocki wrote: > >> > The "wakeup-interrupt" property at the bridge level basically has to be defined > >> > as the wakeup interrupt for all devices on the bus under the bridge. > >> > >> The only thing I'm at a loss for is whether this goes in (referring to > >> rk3399-gru.dtsi) &pcie or &pci_rootport. I think some versions of this > >> series have been aiming for the former, and some the latter. > > > > I'd keep the wakeup interrupt property at the rootport level. That way > > it can work with whatever pcie card that might be plugged into that > > slot. That is in case it's just a slot and not hardwired pcie device :) ^^ Right, and that's what I believe this series was doing. Previous versions put it in &pcie, which might have had a similar effect. The existing behavior is the misguided bindings that put it in &mvl_wifi (the endpoint device). > Do I understand correctly that &pcie is the device and the > &pci_rootport is the port that device is connected to? No. (Assuming "device" means "endpoint PCIe device".) &pcie: The top-level representation of the host bridge &pci_rootport: a virtual representation of a root port. I don't think this corresponds to a Linux device in the end, but I thought it corresponded most closely to a "slot" [1] &mvl_wifi: an endpoint device Brian [1] As another example, consider arch/arm/boot/dts/armada-370{.dtsi,-dlink-dns327l.dts} It has a structure of: pciec: pcie@82000000 { pcie0: pcie@1,0 /* Port 0, Lane 0 */ { // Brian: no subnode, since we don't generally want to describe // specific endpoints in DT, when they should be autodetectable }; pcie2: pcie@2,0 /* Port 1, Lane 0 */ { // Brian: no subnode, since we don't generally want to describe // specific endpoints in DT, when they should be autodetectable }; }; Where Gru's &pcie is equivalent to Armada's &pciec, and Gru's &pci_rootport is equivalent to &pcie0 and &pcie2.
diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c index d39565d5477b..def884c1a37a 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; + /* Ignore other errors, since a missing wakeup is non-fatal. */ + else if (irq < 0) { + 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; + } + + return 0; +} +EXPORT_SYMBOL_GPL(of_pci_setup_wake_irq); + +void of_pci_teardown_wake_irq(struct pci_dev *pdev) +{ + if (!pdev->dev.power.wakeirq) + return; + + 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/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/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;
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. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- 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/pci-driver.c | 10 ++++++++++ include/linux/of_pci.h | 9 +++++++++ 3 files changed, 68 insertions(+)