Messages in this thread Patch in this message | | | Date | Tue, 24 Oct 2017 12:06:22 +0800 | From | jeffy <> | Subject | Re: [PATCH v7 1/3] PCI: Add support for wake irq |
| |
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 >> >> > > >
| |