| Message ID | 20171019111007.25234-2-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
+ PM folks Hi Jeffy, It's probably good if you send the whole thing to linux-pm@ in the future, if you're really trying to implement generic PCI/PM for device tree systems. On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote: > Add support for PCIE_WAKE pin. This is kind of an important change, so it feels like you should document it a little more thoroughly than this. Particularly, I have a few questions below, and it seems like some of these questions should be acknowledged up front. e.g., why does this look so different than the ACPI hooks? > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > 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 > -- Suggested by Brian Norris <briannorris@chromium.com> > > drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++-- > drivers/pci/probe.c | 32 ++++++++++++++++++++++++++++---- > drivers/pci/remove.c | 9 +++++++++ > 3 files changed, 69 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index f0d68066c726..49080a10bdf0 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) > pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR; > } > > +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data) > +{ > + bool *wakeup = data; > + > + if (device_may_wakeup(&dev->dev)) > + *wakeup = true; > + > + return *wakeup; > +} > + > static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) > { > - return pci_platform_pm ? > - pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; > + struct pci_dev *parent = dev; > + struct pci_bus *bus; > + bool wakeup = false; It feels like you're implementing a set of pci_platform_pm_ops, except you're not actually implementing them. It almost seems like we should have a drivers/pci/pci-of.c to do this. But that brings up a few questions.... > + > + if (pci_platform_pm) So, if somebody already registered ops, then you won't follow the "OF" route? That means this all breaks as soon as a kernel has both CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64, which 'select's OF and may also be built/run with CONFIG_ACPI. And that conflict is the same if we try to register pci_platform_pm_ops for OF systems -- it'll be a race over who sets them up first (or rather, last). Also, what happens on !ACPI && !OF? Or if the device tree did not contain a "wakeup" definition? You're now implementing a default path that doesn't make much sense IMO; you may claim wakeup capability without actually having set it up somewhere. I think you could use some more comments, and (again) a real commit message. > + return pci_platform_pm->set_wakeup(dev, enable); > + > + device_set_wakeup_capable(&dev->dev, enable); Why are you setting that here? This function should just be telling the lower layers to enable the physical WAKE# ability. In our case, it just means configuring the WAKE# interrupt for wakeup -- or, since you've used dev_pm_set_dedicated_wake_irq() which handles most of this automatically...do you need this at all? It seems like you should *either* implement these callbacks to manually manage the wakeup IRQ or else use the dedicated wakeirq infrastructure -- not both. And even if you need this, I don't think you need to do this many times; you should only need to set up the capabilities once, when you first set up the device. And BTW, the description for the set_wakeup() callback says: * @set_wakeup: enables/disables wakeup capability for the device I *don't* think that means "capability" as in the device framework's view of "wakeup capable"; I think it means capability as in the physical ability (a la, enable_irq_wake() or similar). > + > + while ((parent = pci_upstream_bridge(parent))) > + bus = parent->bus; > + > + if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent) > + return -ENODEV; > + > + pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup); > + device_set_wakeup_capable(bus->bridge->parent, wakeup); What happens to any intermediate buses? You haven't marked them as wakeup-capable. Should you? And the more fundamental question here is: is this a per-device configuration or a per-root-port configuration? The APIs here are modeled after ACPI, where I guess this is a per-device thing. The PCIe spec doesn't exactly specify how many WAKE# pins you need, though it seems to say (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs should be wired up to it) (b) it *can* be done as a single input to the system controller, since it's an open drain signal (c) ...but I also see now in the PCIe Card Electromechanical specification: "WAKE# may be bused to multiple PCI Express add-in card connectors, forming a single input connection at the PM controller, or individual connectors can have individual connections to the PM controller." So I think you're kind of going along the lines of (b) (as I suggested to you previously), and that matches the current hardware (we only have a single WAKE#) and proposed DT binding. But should this be set up in a way that suits (c) too? It's hard to tell exactly what ACPI-based systems do, since they have this abstracted behind ACPI interfaces that seem like they *could* support per-device or per-bridge type of hookups. Bjorn, any thoughts? This seems like a halfway attempt in between two different designs, and I'm not really sure which one makes more sense. Brian > + > + dev_dbg(bus->bridge->parent, > + "Wakeup %s\n", wakeup ? "enabled" : "disabled"); > + > + return 0; > } > > static inline bool platform_pci_need_resume(struct pci_dev *dev) > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index cdc2f83c11c5..fd43ca832665 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -7,6 +7,7 @@ > #include <linux/init.h> > #include <linux/pci.h> > #include <linux/of_device.h> > +#include <linux/of_irq.h> > #include <linux/of_pci.h> > #include <linux/pci_hotplug.h> > #include <linux/slab.h> > @@ -17,6 +18,7 @@ > #include <linux/acpi.h> > #include <linux/irqdomain.h> > #include <linux/pm_runtime.h> > +#include <linux/pm_wakeirq.h> > #include "pci.h" > > #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ > @@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > struct resource *res; > char addr[64], *fmt; > const char *name; > - int err; > + int err, irq; > + > + if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) { > + irq = of_irq_get_byname(parent->of_node, "wakeup"); > + if (irq == -EPROBE_DEFER) > + return irq; > + if (irq > 0) { > + device_init_wakeup(parent, true); > + err = dev_pm_set_dedicated_wake_irq(parent, irq); > + if (err) { > + dev_err(parent, "Failed to setup wakeup IRQ\n"); > + goto deinit_wakeup; > + } > + dev_info(parent, "Wakeup enabled with IRQ %d\n", irq); > + } > + } > > bus = pci_alloc_bus(NULL); > - if (!bus) > - return -ENOMEM; > + if (!bus) { > + err = -ENOMEM; > + goto clear_wake_irq; > + } > > bridge->bus = bus; > > @@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > unregister: > put_device(&bridge->dev); > device_unregister(&bridge->dev); > - > free: > kfree(bus); > +clear_wake_irq: > + if (parent) > + dev_pm_clear_wake_irq(parent); > +deinit_wakeup: > + if (parent) > + device_init_wakeup(parent, false); > return err; > } > > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c > index 73a03d382590..cb7a326429e1 100644 > --- a/drivers/pci/remove.c > +++ b/drivers/pci/remove.c > @@ -1,6 +1,7 @@ > #include <linux/pci.h> > #include <linux/module.h> > #include <linux/pci-aspm.h> > +#include <linux/pm_wakeirq.h> > #include "pci.h" > > static void pci_free_resources(struct pci_dev *dev) > @@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus) > { > struct pci_dev *child, *tmp; > struct pci_host_bridge *host_bridge; > + struct device *parent; > > if (!pci_is_root_bus(bus)) > return; > > host_bridge = to_pci_host_bridge(bus->bridge); > + parent = host_bridge->dev.parent; > + > list_for_each_entry_safe_reverse(child, tmp, > &bus->devices, bus_list) > pci_stop_bus_device(child); > > /* stop the host bridge */ > device_release_driver(&host_bridge->dev); > + > + if (parent) { > + dev_pm_clear_wake_irq(parent); > + device_init_wakeup(parent, false); > + } > } > EXPORT_SYMBOL_GPL(pci_stop_root_bus); > > -- > 2.11.0 > >
Hi Brian, On 10/24/2017 07:02 AM, Brian Norris wrote: > + PM folks > > Hi Jeffy, > > It's probably good if you send the whole thing to linux-pm@ in the > future, if you're really trying to implement generic PCI/PM for device > tree systems. ok > > On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote: >> Add support for PCIE_WAKE pin. > > This is kind of an important change, so it feels like you should > document it a little more thoroughly than this. Particularly, I have a > few questions below, and it seems like some of these questions should be > acknowledged up front. e.g., why does this look so different than the > ACPI hooks? sure, will do in next version. > >> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >> --- >> >> 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 >> -- Suggested by Brian Norris <briannorris@chromium.com> >> >> drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++-- >> drivers/pci/probe.c | 32 ++++++++++++++++++++++++++++---- >> drivers/pci/remove.c | 9 +++++++++ >> 3 files changed, 69 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index f0d68066c726..49080a10bdf0 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) >> pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR; >> } >> >> +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data) >> +{ >> + bool *wakeup = data; >> + >> + if (device_may_wakeup(&dev->dev)) >> + *wakeup = true; >> + >> + return *wakeup; >> +} >> + >> static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) >> { >> - return pci_platform_pm ? >> - pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; >> + struct pci_dev *parent = dev; >> + struct pci_bus *bus; >> + bool wakeup = false; > > It feels like you're implementing a set of pci_platform_pm_ops, except > you're not actually implementing them. It almost seems like we should > have a drivers/pci/pci-of.c to do this. But that brings up a few > questions.... i saw the drivers might call set_wakeup() in suspend/resume/shutdown to configure the wakeup ability, maybe we can call device_set_wakeup_enable() here as a common part of set_wakeup()? static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) { device_set_wakeup_enable() ... return pci_platform_pm ? pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; > >> + >> + if (pci_platform_pm) > > So, if somebody already registered ops, then you won't follow the "OF" > route? That means this all breaks as soon as a kernel has both > CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64, > which 'select's OF and may also be built/run with CONFIG_ACPI. > > And that conflict is the same if we try to register pci_platform_pm_ops > for OF systems -- it'll be a race over who sets them up first (or > rather, last). > > Also, what happens on !ACPI && !OF? Or if the device tree did not > contain a "wakeup" definition? You're now implementing a default path > that doesn't make much sense IMO; you may claim wakeup capability > without actually having set it up somewhere. maybe we can use device_set_wakeup_enable(), which will check the setup before setting? > > I think you could use some more comments, and (again) a real commit > message. ok, will do. > >> + return pci_platform_pm->set_wakeup(dev, enable); >> + >> + device_set_wakeup_capable(&dev->dev, enable); > > Why are you setting that here? This function should just be telling the > lower layers to enable the physical WAKE# ability. In our case, it just > means configuring the WAKE# interrupt for wakeup -- or, since you've > used dev_pm_set_dedicated_wake_irq() which handles most of this > automatically...do you need this at all? It seems like you should > *either* implement these callbacks to manually manage the wakeup IRQ or > else use the dedicated wakeirq infrastructure -- not both. > > And even if you need this, I don't think you need to do this many times; > you should only need to set up the capabilities once, when you first set > up the device. > > And BTW, the description for the set_wakeup() callback says: > > * @set_wakeup: enables/disables wakeup capability for the device > > I *don't* think that means "capability" as in the device framework's > view of "wakeup capable"; I think it means capability as in the physical > ability (a la, enable_irq_wake() or similar). i was thinking maybe we should disable the wakeup if all children request set_wakeup(false)? and it seems like the dedicated wakeirq can be disabled by: 1/ dev_pm_clear_wake_irq(), then we may need to store the irq somewhere to set it up again in the future? 2/ let device_may_wakeup return false: void dev_pm_arm_wake_irq(struct wake_irq *wirq) { if (!wirq) return; if (device_may_wakeup(wirq->dev)) { if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED) enable_irq(wirq->irq); enable_irq_wake(wirq->irq); } > >> + >> + while ((parent = pci_upstream_bridge(parent))) >> + bus = parent->bus; >> + >> + if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent) >> + return -ENODEV; >> + >> + pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup); >> + device_set_wakeup_capable(bus->bridge->parent, wakeup); > > What happens to any intermediate buses? You haven't marked them as > wakeup-capable. Should you? > > And the more fundamental question here is: is this a per-device > configuration or a per-root-port configuration? The APIs here are > modeled after ACPI, where I guess this is a per-device thing. The PCIe > spec doesn't exactly specify how many WAKE# pins you need, though it > seems to say > > (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs > should be wired up to it) > (b) it *can* be done as a single input to the system controller, since > it's an open drain signal > (c) ...but I also see now in the PCIe Card Electromechanical > specification: > > "WAKE# may be bused to multiple PCI Express add-in card connectors, > forming a single input connection at the PM controller, or > individual connectors can have individual connections to the PM > controller." > > So I think you're kind of going along the lines of (b) (as I suggested > to you previously), and that matches the current hardware (we only have > a single WAKE#) and proposed DT binding. But should this be set up in a > way that suits (c) too? It's hard to tell exactly what ACPI-based > systems do, since they have this abstracted behind ACPI interfaces that > seem like they *could* support per-device or per-bridge type of hookups. maybe we can try to setup wake irq for each pci devices which have it in the dts, then in the set_wakeup(), try to find the parents(or itself) who has wake irq, and enable/disable them(maybe also need a refcount)? > > Bjorn, any thoughts? This seems like a halfway attempt in between two > different designs, and I'm not really sure which one makes more sense. > > Brian > >> + >> + dev_dbg(bus->bridge->parent, >> + "Wakeup %s\n", wakeup ? "enabled" : "disabled"); >> + >> + return 0; >> } >> >> static inline bool platform_pci_need_resume(struct pci_dev *dev) >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index cdc2f83c11c5..fd43ca832665 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -7,6 +7,7 @@ >> #include <linux/init.h> >> #include <linux/pci.h> >> #include <linux/of_device.h> >> +#include <linux/of_irq.h> >> #include <linux/of_pci.h> >> #include <linux/pci_hotplug.h> >> #include <linux/slab.h> >> @@ -17,6 +18,7 @@ >> #include <linux/acpi.h> >> #include <linux/irqdomain.h> >> #include <linux/pm_runtime.h> >> +#include <linux/pm_wakeirq.h> >> #include "pci.h" >> >> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ >> @@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) >> struct resource *res; >> char addr[64], *fmt; >> const char *name; >> - int err; >> + int err, irq; >> + >> + if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) { >> + irq = of_irq_get_byname(parent->of_node, "wakeup"); >> + if (irq == -EPROBE_DEFER) >> + return irq; >> + if (irq > 0) { >> + device_init_wakeup(parent, true); >> + err = dev_pm_set_dedicated_wake_irq(parent, irq); >> + if (err) { >> + dev_err(parent, "Failed to setup wakeup IRQ\n"); >> + goto deinit_wakeup; >> + } >> + dev_info(parent, "Wakeup enabled with IRQ %d\n", irq); >> + } >> + } >> >> bus = pci_alloc_bus(NULL); >> - if (!bus) >> - return -ENOMEM; >> + if (!bus) { >> + err = -ENOMEM; >> + goto clear_wake_irq; >> + } >> >> bridge->bus = bus; >> >> @@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) >> unregister: >> put_device(&bridge->dev); >> device_unregister(&bridge->dev); >> - >> free: >> kfree(bus); >> +clear_wake_irq: >> + if (parent) >> + dev_pm_clear_wake_irq(parent); >> +deinit_wakeup: >> + if (parent) >> + device_init_wakeup(parent, false); >> return err; >> } >> >> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c >> index 73a03d382590..cb7a326429e1 100644 >> --- a/drivers/pci/remove.c >> +++ b/drivers/pci/remove.c >> @@ -1,6 +1,7 @@ >> #include <linux/pci.h> >> #include <linux/module.h> >> #include <linux/pci-aspm.h> >> +#include <linux/pm_wakeirq.h> >> #include "pci.h" >> >> static void pci_free_resources(struct pci_dev *dev) >> @@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus) >> { >> struct pci_dev *child, *tmp; >> struct pci_host_bridge *host_bridge; >> + struct device *parent; >> >> if (!pci_is_root_bus(bus)) >> return; >> >> host_bridge = to_pci_host_bridge(bus->bridge); >> + parent = host_bridge->dev.parent; >> + >> list_for_each_entry_safe_reverse(child, tmp, >> &bus->devices, bus_list) >> pci_stop_bus_device(child); >> >> /* stop the host bridge */ >> device_release_driver(&host_bridge->dev); >> + >> + if (parent) { >> + dev_pm_clear_wake_irq(parent); >> + device_init_wakeup(parent, false); >> + } >> } >> EXPORT_SYMBOL_GPL(pci_stop_root_bus); >> >> -- >> 2.11.0 >> >> > > >
Hi Brian, checking the pci-acpi code, it has: 1/ pci_acpi_setup() and pci_acpi_cleanup() to setup/cleanup the wakeup(and other stuff) for pci devices we may need it too(for per-device wake) to parse wake irq and init wakeup(false) and maybe setup dedicated wakeirq. 2/ acpi_pci_wakeup(), which would do: find a parent or root bus or pci dev itself, which can do wakeup, and update it's wakeup ability(with a enable_count). we may need to do something like that, but the can_wakeup() would be check if wake irq avaliable, and set_device/bridge_wakeup() would be setup/clear dedicated wakeirq, or just call device_set_wakeup_enable() so maybe we can: 1/ add a setup_root_bus() platform ops callback to parse/setup root bus's wakeirq in pci_register_host_bridge(),and clean it in pci_stop_bus_device() 2/ add a device_setup() and device_cleanup() callbacks to setup/clean pci device's wake irq, and maybe call it in pci_device_probe() or pci_setup_device()? 3/ add a can_wakeup() and set_device_wakeup() and set_bridge_wakeup() callbacks, and move acpi_pci_wakeup() and acpi_pci_propagate_wakeup()'s code and the enable_count code into common platform_pci_set_wakeup(). does this make sense? On 10/24/2017 12:06 PM, jeffy wrote: > Hi Brian, > > On 10/24/2017 07:02 AM, Brian Norris wrote: >> + PM folks >> >> Hi Jeffy, >> >> It's probably good if you send the whole thing to linux-pm@ in the >> future, if you're really trying to implement generic PCI/PM for device >> tree systems. > ok >> >> On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote: >>> Add support for PCIE_WAKE pin. >> >> This is kind of an important change, so it feels like you should >> document it a little more thoroughly than this. Particularly, I have a >> few questions below, and it seems like some of these questions should be >> acknowledged up front. e.g., why does this look so different than the >> ACPI hooks? > sure, will do in next version. >> >>> >>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>> --- >>> >>> 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 >>> -- Suggested by Brian Norris <briannorris@chromium.com> >>> >>> drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++-- >>> drivers/pci/probe.c | 32 ++++++++++++++++++++++++++++---- >>> drivers/pci/remove.c | 9 +++++++++ >>> 3 files changed, 69 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>> index f0d68066c726..49080a10bdf0 100644 >>> --- a/drivers/pci/pci.c >>> +++ b/drivers/pci/pci.c >>> @@ -603,10 +603,40 @@ static inline pci_power_t >>> platform_pci_choose_state(struct pci_dev *dev) >>> pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR; >>> } >>> >>> +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data) >>> +{ >>> + bool *wakeup = data; >>> + >>> + if (device_may_wakeup(&dev->dev)) >>> + *wakeup = true; >>> + >>> + return *wakeup; >>> +} >>> + >>> static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool >>> enable) >>> { >>> - return pci_platform_pm ? >>> - pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; >>> + struct pci_dev *parent = dev; >>> + struct pci_bus *bus; >>> + bool wakeup = false; >> >> It feels like you're implementing a set of pci_platform_pm_ops, except >> you're not actually implementing them. It almost seems like we should >> have a drivers/pci/pci-of.c to do this. But that brings up a few >> questions.... > i saw the drivers might call set_wakeup() in suspend/resume/shutdown to > configure the wakeup ability, maybe we can call > device_set_wakeup_enable() here as a common part of set_wakeup()? > > static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool > enable) { > device_set_wakeup_enable() > ... > return pci_platform_pm ? pci_platform_pm->set_wakeup(dev, enable) : > -ENODEV; > >> >>> + >>> + if (pci_platform_pm) >> >> So, if somebody already registered ops, then you won't follow the "OF" >> route? That means this all breaks as soon as a kernel has both >> CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64, >> which 'select's OF and may also be built/run with CONFIG_ACPI. >> >> And that conflict is the same if we try to register pci_platform_pm_ops >> for OF systems -- it'll be a race over who sets them up first (or >> rather, last). >> >> Also, what happens on !ACPI && !OF? Or if the device tree did not >> contain a "wakeup" definition? You're now implementing a default path >> that doesn't make much sense IMO; you may claim wakeup capability >> without actually having set it up somewhere. > maybe we can use device_set_wakeup_enable(), which will check the setup > before setting? >> >> I think you could use some more comments, and (again) a real commit >> message. > ok, will do. >> >>> + return pci_platform_pm->set_wakeup(dev, enable); >>> + >>> + device_set_wakeup_capable(&dev->dev, enable); >> >> Why are you setting that here? This function should just be telling the >> lower layers to enable the physical WAKE# ability. In our case, it just >> means configuring the WAKE# interrupt for wakeup -- or, since you've >> used dev_pm_set_dedicated_wake_irq() which handles most of this >> automatically...do you need this at all? It seems like you should >> *either* implement these callbacks to manually manage the wakeup IRQ or >> else use the dedicated wakeirq infrastructure -- not both. >> >> And even if you need this, I don't think you need to do this many times; >> you should only need to set up the capabilities once, when you first set >> up the device. >> >> And BTW, the description for the set_wakeup() callback says: >> >> * @set_wakeup: enables/disables wakeup capability for the device >> >> I *don't* think that means "capability" as in the device framework's >> view of "wakeup capable"; I think it means capability as in the physical >> ability (a la, enable_irq_wake() or similar). > i was thinking maybe we should disable the wakeup if all children > request set_wakeup(false)? > > and it seems like the dedicated wakeirq can be disabled by: > 1/ dev_pm_clear_wake_irq(), then we may need to store the irq somewhere > to set it up again in the future? > > 2/ let device_may_wakeup return false: > void dev_pm_arm_wake_irq(struct wake_irq *wirq) > { > if (!wirq) > return; > > if (device_may_wakeup(wirq->dev)) { > if (wirq->status & WAKE_IRQ_DEDICATED_ALLOCATED) > enable_irq(wirq->irq); > > enable_irq_wake(wirq->irq); > } > >> >>> + >>> + while ((parent = pci_upstream_bridge(parent))) >>> + bus = parent->bus; >>> + >>> + if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent) >>> + return -ENODEV; >>> + >>> + pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup); >>> + device_set_wakeup_capable(bus->bridge->parent, wakeup); >> >> What happens to any intermediate buses? You haven't marked them as >> wakeup-capable. Should you? >> >> And the more fundamental question here is: is this a per-device >> configuration or a per-root-port configuration? The APIs here are >> modeled after ACPI, where I guess this is a per-device thing. The PCIe >> spec doesn't exactly specify how many WAKE# pins you need, though it >> seems to say >> >> (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs >> should be wired up to it) >> (b) it *can* be done as a single input to the system controller, since >> it's an open drain signal >> (c) ...but I also see now in the PCIe Card Electromechanical >> specification: >> >> "WAKE# may be bused to multiple PCI Express add-in card connectors, >> forming a single input connection at the PM controller, or >> individual connectors can have individual connections to the PM >> controller." >> >> So I think you're kind of going along the lines of (b) (as I suggested >> to you previously), and that matches the current hardware (we only have >> a single WAKE#) and proposed DT binding. But should this be set up in a >> way that suits (c) too? It's hard to tell exactly what ACPI-based >> systems do, since they have this abstracted behind ACPI interfaces that >> seem like they *could* support per-device or per-bridge type of hookups. > maybe we can try to setup wake irq for each pci devices which have it in > the dts, then in the set_wakeup(), try to find the parents(or itself) > who has wake irq, and enable/disable them(maybe also need a refcount)? >> >> Bjorn, any thoughts? This seems like a halfway attempt in between two >> different designs, and I'm not really sure which one makes more sense. >> >> Brian >> >>> + >>> + dev_dbg(bus->bridge->parent, >>> + "Wakeup %s\n", wakeup ? "enabled" : "disabled"); >>> + >>> + return 0; >>> } >>> >>> static inline bool platform_pci_need_resume(struct pci_dev *dev) >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index cdc2f83c11c5..fd43ca832665 100644 >>> --- a/drivers/pci/probe.c >>> +++ b/drivers/pci/probe.c >>> @@ -7,6 +7,7 @@ >>> #include <linux/init.h> >>> #include <linux/pci.h> >>> #include <linux/of_device.h> >>> +#include <linux/of_irq.h> >>> #include <linux/of_pci.h> >>> #include <linux/pci_hotplug.h> >>> #include <linux/slab.h> >>> @@ -17,6 +18,7 @@ >>> #include <linux/acpi.h> >>> #include <linux/irqdomain.h> >>> #include <linux/pm_runtime.h> >>> +#include <linux/pm_wakeirq.h> >>> #include "pci.h" >>> >>> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ >>> @@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct >>> pci_host_bridge *bridge) >>> struct resource *res; >>> char addr[64], *fmt; >>> const char *name; >>> - int err; >>> + int err, irq; >>> + >>> + if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) { >>> + irq = of_irq_get_byname(parent->of_node, "wakeup"); >>> + if (irq == -EPROBE_DEFER) >>> + return irq; >>> + if (irq > 0) { >>> + device_init_wakeup(parent, true); >>> + err = dev_pm_set_dedicated_wake_irq(parent, irq); >>> + if (err) { >>> + dev_err(parent, "Failed to setup wakeup IRQ\n"); >>> + goto deinit_wakeup; >>> + } >>> + dev_info(parent, "Wakeup enabled with IRQ %d\n", irq); >>> + } >>> + } >>> >>> bus = pci_alloc_bus(NULL); >>> - if (!bus) >>> - return -ENOMEM; >>> + if (!bus) { >>> + err = -ENOMEM; >>> + goto clear_wake_irq; >>> + } >>> >>> bridge->bus = bus; >>> >>> @@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct >>> pci_host_bridge *bridge) >>> unregister: >>> put_device(&bridge->dev); >>> device_unregister(&bridge->dev); >>> - >>> free: >>> kfree(bus); >>> +clear_wake_irq: >>> + if (parent) >>> + dev_pm_clear_wake_irq(parent); >>> +deinit_wakeup: >>> + if (parent) >>> + device_init_wakeup(parent, false); >>> return err; >>> } >>> >>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c >>> index 73a03d382590..cb7a326429e1 100644 >>> --- a/drivers/pci/remove.c >>> +++ b/drivers/pci/remove.c >>> @@ -1,6 +1,7 @@ >>> #include <linux/pci.h> >>> #include <linux/module.h> >>> #include <linux/pci-aspm.h> >>> +#include <linux/pm_wakeirq.h> >>> #include "pci.h" >>> >>> static void pci_free_resources(struct pci_dev *dev) >>> @@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus) >>> { >>> struct pci_dev *child, *tmp; >>> struct pci_host_bridge *host_bridge; >>> + struct device *parent; >>> >>> if (!pci_is_root_bus(bus)) >>> return; >>> >>> host_bridge = to_pci_host_bridge(bus->bridge); >>> + parent = host_bridge->dev.parent; >>> + >>> list_for_each_entry_safe_reverse(child, tmp, >>> &bus->devices, bus_list) >>> pci_stop_bus_device(child); >>> >>> /* stop the host bridge */ >>> device_release_driver(&host_bridge->dev); >>> + >>> + if (parent) { >>> + dev_pm_clear_wake_irq(parent); >>> + device_init_wakeup(parent, false); >>> + } >>> } >>> EXPORT_SYMBOL_GPL(pci_stop_root_bus); >>> >>> -- >>> 2.11.0 >>> >>> >> >> >> >
Include "PCIe WAKE#" signal in the subject, since this is specifically about that wire. On Mon, Oct 23, 2017 at 04:02:53PM -0700, Brian Norris wrote: > + PM folks > > Hi Jeffy, > > It's probably good if you send the whole thing to linux-pm@ in the > future, if you're really trying to implement generic PCI/PM for device > tree systems. > > On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote: > > Add support for PCIE_WAKE pin. I think you're referring to what the spec calls "the WAKE# signal". It will reduce confusion if you use exactly the same notation as the spec. > This is kind of an important change, so it feels like you should > document it a little more thoroughly than this. Particularly, I have a > few questions below, and it seems like some of these questions should be > acknowledged up front. e.g., why does this look so different than the > ACPI hooks? > > > > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > > --- > > > > 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 > > -- Suggested by Brian Norris <briannorris@chromium.com> > > > > drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++-- > > drivers/pci/probe.c | 32 ++++++++++++++++++++++++++++---- > > drivers/pci/remove.c | 9 +++++++++ > > 3 files changed, 69 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index f0d68066c726..49080a10bdf0 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) > > pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR; > > } > > > > +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data) > > +{ > > + bool *wakeup = data; > > + > > + if (device_may_wakeup(&dev->dev)) > > + *wakeup = true; > > + > > + return *wakeup; > > +} > > + > > static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) > > { > > - return pci_platform_pm ? > > - pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; > > + struct pci_dev *parent = dev; > > + struct pci_bus *bus; > > + bool wakeup = false; > > It feels like you're implementing a set of pci_platform_pm_ops, except > you're not actually implementing them. It almost seems like we should > have a drivers/pci/pci-of.c to do this. But that brings up a few > questions.... > > > + > > + if (pci_platform_pm) > > So, if somebody already registered ops, then you won't follow the "OF" > route? That means this all breaks as soon as a kernel has both > CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64, > which 'select's OF and may also be built/run with CONFIG_ACPI. > > And that conflict is the same if we try to register pci_platform_pm_ops > for OF systems -- it'll be a race over who sets them up first (or > rather, last). > > Also, what happens on !ACPI && !OF? Or if the device tree did not > contain a "wakeup" definition? You're now implementing a default path > that doesn't make much sense IMO; you may claim wakeup capability > without actually having set it up somewhere. > > I think you could use some more comments, and (again) a real commit > message. > > > + return pci_platform_pm->set_wakeup(dev, enable); > > + > > + device_set_wakeup_capable(&dev->dev, enable); > > Why are you setting that here? This function should just be telling the > lower layers to enable the physical WAKE# ability. In our case, it just > means configuring the WAKE# interrupt for wakeup -- or, since you've > used dev_pm_set_dedicated_wake_irq() which handles most of this > automatically...do you need this at all? It seems like you should > *either* implement these callbacks to manually manage the wakeup IRQ or > else use the dedicated wakeirq infrastructure -- not both. > > And even if you need this, I don't think you need to do this many times; > you should only need to set up the capabilities once, when you first set > up the device. > > And BTW, the description for the set_wakeup() callback says: > > * @set_wakeup: enables/disables wakeup capability for the device > > I *don't* think that means "capability" as in the device framework's > view of "wakeup capable"; I think it means capability as in the physical > ability (a la, enable_irq_wake() or similar). > > > + > > + while ((parent = pci_upstream_bridge(parent))) > > + bus = parent->bus; > > + > > + if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent) > > + return -ENODEV; > > + > > + pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup); > > + device_set_wakeup_capable(bus->bridge->parent, wakeup); > > What happens to any intermediate buses? You haven't marked them as > wakeup-capable. Should you? > > And the more fundamental question here is: is this a per-device > configuration or a per-root-port configuration? The APIs here are > modeled after ACPI, where I guess this is a per-device thing. The PCIe > spec doesn't exactly specify how many WAKE# pins you need, though it > seems to say > > (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs > should be wired up to it) > (b) it *can* be done as a single input to the system controller, since > it's an open drain signal > (c) ...but I also see now in the PCIe Card Electromechanical > specification: > > "WAKE# may be bused to multiple PCI Express add-in card connectors, > forming a single input connection at the PM controller, or > individual connectors can have individual connections to the PM > controller." > > So I think you're kind of going along the lines of (b) (as I suggested > to you previously), and that matches the current hardware (we only have > a single WAKE#) and proposed DT binding. But should this be set up in a > way that suits (c) too? It's hard to tell exactly what ACPI-based > systems do, since they have this abstracted behind ACPI interfaces that > seem like they *could* support per-device or per-bridge type of hookups. > > Bjorn, any thoughts? This seems like a halfway attempt in between two > different designs, and I'm not really sure which one makes more sense. No thoughts yet. Seems like this needs a little more time in the oven, and I'll take a deeper look after some of the issues you pointed out have been addressed. Bjorn
On Tue, Oct 24, 2017 at 10:10 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > Include "PCIe WAKE#" signal in the subject, since this is specifically > about that wire. > > On Mon, Oct 23, 2017 at 04:02:53PM -0700, Brian Norris wrote: >> + PM folks >> >> Hi Jeffy, >> >> It's probably good if you send the whole thing to linux-pm@ in the >> future, if you're really trying to implement generic PCI/PM for device >> tree systems. >> >> On Thu, Oct 19, 2017 at 07:10:05PM +0800, Jeffy Chen wrote: >> > Add support for PCIE_WAKE pin. > > I think you're referring to what the spec calls "the WAKE# signal". > It will reduce confusion if you use exactly the same notation as the > spec. > >> This is kind of an important change, so it feels like you should >> document it a little more thoroughly than this. Particularly, I have a >> few questions below, and it seems like some of these questions should be >> acknowledged up front. e.g., why does this look so different than the >> ACPI hooks? >> >> > >> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >> > --- >> > >> > 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 >> > -- Suggested by Brian Norris <briannorris@chromium.com> >> > >> > drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++-- >> > drivers/pci/probe.c | 32 ++++++++++++++++++++++++++++---- >> > drivers/pci/remove.c | 9 +++++++++ >> > 3 files changed, 69 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> > index f0d68066c726..49080a10bdf0 100644 >> > --- a/drivers/pci/pci.c >> > +++ b/drivers/pci/pci.c >> > @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) >> > pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR; >> > } >> > >> > +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data) >> > +{ >> > + bool *wakeup = data; >> > + >> > + if (device_may_wakeup(&dev->dev)) >> > + *wakeup = true; >> > + >> > + return *wakeup; >> > +} >> > + >> > static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) >> > { >> > - return pci_platform_pm ? >> > - pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; >> > + struct pci_dev *parent = dev; >> > + struct pci_bus *bus; >> > + bool wakeup = false; >> >> It feels like you're implementing a set of pci_platform_pm_ops, except >> you're not actually implementing them. It almost seems like we should >> have a drivers/pci/pci-of.c to do this. But that brings up a few >> questions.... >> >> > + >> > + if (pci_platform_pm) >> >> So, if somebody already registered ops, then you won't follow the "OF" >> route? That means this all breaks as soon as a kernel has both >> CONFIG_ACPI and CONFIG_OF enabled. This is possible on at least ARM64, >> which 'select's OF and may also be built/run with CONFIG_ACPI. >> >> And that conflict is the same if we try to register pci_platform_pm_ops >> for OF systems -- it'll be a race over who sets them up first (or >> rather, last). >> >> Also, what happens on !ACPI && !OF? Or if the device tree did not >> contain a "wakeup" definition? You're now implementing a default path >> that doesn't make much sense IMO; you may claim wakeup capability >> without actually having set it up somewhere. >> >> I think you could use some more comments, and (again) a real commit >> message. >> >> > + return pci_platform_pm->set_wakeup(dev, enable); >> > + >> > + device_set_wakeup_capable(&dev->dev, enable); >> >> Why are you setting that here? This function should just be telling the >> lower layers to enable the physical WAKE# ability. In our case, it just >> means configuring the WAKE# interrupt for wakeup -- or, since you've >> used dev_pm_set_dedicated_wake_irq() which handles most of this >> automatically...do you need this at all? It seems like you should >> *either* implement these callbacks to manually manage the wakeup IRQ or >> else use the dedicated wakeirq infrastructure -- not both. >> >> And even if you need this, I don't think you need to do this many times; >> you should only need to set up the capabilities once, when you first set >> up the device. >> >> And BTW, the description for the set_wakeup() callback says: >> >> * @set_wakeup: enables/disables wakeup capability for the device >> >> I *don't* think that means "capability" as in the device framework's >> view of "wakeup capable"; I think it means capability as in the physical >> ability (a la, enable_irq_wake() or similar). >> >> > + >> > + while ((parent = pci_upstream_bridge(parent))) >> > + bus = parent->bus; >> > + >> > + if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent) >> > + return -ENODEV; >> > + >> > + pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup); >> > + device_set_wakeup_capable(bus->bridge->parent, wakeup); >> >> What happens to any intermediate buses? You haven't marked them as >> wakeup-capable. Should you? >> >> And the more fundamental question here is: is this a per-device >> configuration or a per-root-port configuration? The APIs here are >> modeled after ACPI, where I guess this is a per-device thing. The PCIe >> spec doesn't exactly specify how many WAKE# pins you need, though it >> seems to say >> >> (a) it's all-or-nothing (if one device uses it, all wakeup-capable EPs >> should be wired up to it) >> (b) it *can* be done as a single input to the system controller, since >> it's an open drain signal >> (c) ...but I also see now in the PCIe Card Electromechanical >> specification: >> >> "WAKE# may be bused to multiple PCI Express add-in card connectors, >> forming a single input connection at the PM controller, or >> individual connectors can have individual connections to the PM >> controller." >> >> So I think you're kind of going along the lines of (b) (as I suggested >> to you previously), and that matches the current hardware (we only have >> a single WAKE#) and proposed DT binding. But should this be set up in a >> way that suits (c) too? It's hard to tell exactly what ACPI-based >> systems do, since they have this abstracted behind ACPI interfaces that >> seem like they *could* support per-device or per-bridge type of hookups. >> >> Bjorn, any thoughts? This seems like a halfway attempt in between two >> different designs, and I'm not really sure which one makes more sense. > > No thoughts yet. Seems like this needs a little more time in the > oven, Agreed. > and I'll take a deeper look after some of the issues you pointed > out have been addressed. This is in my list of things to look at, but I'm working on something else now, so I'll be looking at it when I'm done with the other thing(s). Thanks, Rafael
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index f0d68066c726..49080a10bdf0 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -603,10 +603,40 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR; } +static int pci_dev_check_wakeup(struct pci_dev *dev, void *data) +{ + bool *wakeup = data; + + if (device_may_wakeup(&dev->dev)) + *wakeup = true; + + return *wakeup; +} + static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) { - return pci_platform_pm ? - pci_platform_pm->set_wakeup(dev, enable) : -ENODEV; + struct pci_dev *parent = dev; + struct pci_bus *bus; + bool wakeup = false; + + if (pci_platform_pm) + return pci_platform_pm->set_wakeup(dev, enable); + + device_set_wakeup_capable(&dev->dev, enable); + + while ((parent = pci_upstream_bridge(parent))) + bus = parent->bus; + + if (!bus || !pci_is_root_bus(bus) || !bus->bridge->parent) + return -ENODEV; + + pci_walk_bus(bus, pci_dev_check_wakeup, &wakeup); + device_set_wakeup_capable(bus->bridge->parent, wakeup); + + dev_dbg(bus->bridge->parent, + "Wakeup %s\n", wakeup ? "enabled" : "disabled"); + + return 0; } static inline bool platform_pci_need_resume(struct pci_dev *dev) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index cdc2f83c11c5..fd43ca832665 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -7,6 +7,7 @@ #include <linux/init.h> #include <linux/pci.h> #include <linux/of_device.h> +#include <linux/of_irq.h> #include <linux/of_pci.h> #include <linux/pci_hotplug.h> #include <linux/slab.h> @@ -17,6 +18,7 @@ #include <linux/acpi.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> +#include <linux/pm_wakeirq.h> #include "pci.h" #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ @@ -756,11 +758,28 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) struct resource *res; char addr[64], *fmt; const char *name; - int err; + int err, irq; + + if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) { + irq = of_irq_get_byname(parent->of_node, "wakeup"); + if (irq == -EPROBE_DEFER) + return irq; + if (irq > 0) { + device_init_wakeup(parent, true); + err = dev_pm_set_dedicated_wake_irq(parent, irq); + if (err) { + dev_err(parent, "Failed to setup wakeup IRQ\n"); + goto deinit_wakeup; + } + dev_info(parent, "Wakeup enabled with IRQ %d\n", irq); + } + } bus = pci_alloc_bus(NULL); - if (!bus) - return -ENOMEM; + if (!bus) { + err = -ENOMEM; + goto clear_wake_irq; + } bridge->bus = bus; @@ -856,9 +875,14 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) unregister: put_device(&bridge->dev); device_unregister(&bridge->dev); - free: kfree(bus); +clear_wake_irq: + if (parent) + dev_pm_clear_wake_irq(parent); +deinit_wakeup: + if (parent) + device_init_wakeup(parent, false); return err; } diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 73a03d382590..cb7a326429e1 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -1,6 +1,7 @@ #include <linux/pci.h> #include <linux/module.h> #include <linux/pci-aspm.h> +#include <linux/pm_wakeirq.h> #include "pci.h" static void pci_free_resources(struct pci_dev *dev) @@ -131,17 +132,25 @@ void pci_stop_root_bus(struct pci_bus *bus) { struct pci_dev *child, *tmp; struct pci_host_bridge *host_bridge; + struct device *parent; if (!pci_is_root_bus(bus)) return; host_bridge = to_pci_host_bridge(bus->bridge); + parent = host_bridge->dev.parent; + list_for_each_entry_safe_reverse(child, tmp, &bus->devices, bus_list) pci_stop_bus_device(child); /* stop the host bridge */ device_release_driver(&host_bridge->dev); + + if (parent) { + dev_pm_clear_wake_irq(parent); + device_init_wakeup(parent, false); + } } EXPORT_SYMBOL_GPL(pci_stop_root_bus);
Add support for PCIE_WAKE pin. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- 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 -- Suggested by Brian Norris <briannorris@chromium.com> drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++-- drivers/pci/probe.c | 32 ++++++++++++++++++++++++++++---- drivers/pci/remove.c | 9 +++++++++ 3 files changed, 69 insertions(+), 6 deletions(-)