| Message ID | 20171027072612.26565-7-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Hi Jeffy, On Fri, Oct 27, 2017 at 03:26:11PM +0800, Jeffy Chen wrote: > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other > platforms could reuse it. I think you need to double check your refactoring. I believe you may have changed some behavior here. > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm > ops's callbacks to setup and cleanup pci devices and host bridge for > wakeup. > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > Changes in v10: None > Changes in v9: None > Changes in v8: None > Changes in v7: None > Changes in v6: None > Changes in v5: None > Changes in v3: None > Changes in v2: None > > drivers/pci/pci-acpi.c | 121 +++++++++++++++++++++++------------------------ > drivers/pci/pci-driver.c | 9 ++++ > drivers/pci/pci.c | 84 ++++++++++++++++++++++++++++---- > drivers/pci/pci.h | 28 +++++++++-- > drivers/pci/probe.c | 12 ++++- > drivers/pci/remove.c | 2 + > include/linux/pci.h | 2 + > 7 files changed, 180 insertions(+), 78 deletions(-) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 4708eb9df71b..ee96e7afe1ac 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -569,31 +569,6 @@ static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) > return state_conv[state]; > } > > -static int acpi_pci_propagate_wakeup(struct pci_bus *bus, bool enable) > -{ > - while (bus->parent) { > - if (acpi_pm_device_can_wakeup(&bus->self->dev)) > - return acpi_pm_set_bridge_wakeup(&bus->self->dev, enable); > - > - bus = bus->parent; > - } > - > - /* We have reached the root bus. */ > - if (bus->bridge) { > - if (acpi_pm_device_can_wakeup(bus->bridge)) > - return acpi_pm_set_bridge_wakeup(bus->bridge, enable); > - } > - return 0; > -} > - > -static int acpi_pci_wakeup(struct pci_dev *dev, bool enable) > -{ > - if (acpi_pm_device_can_wakeup(&dev->dev)) > - return acpi_pm_set_device_wakeup(&dev->dev, enable); > - > - return acpi_pci_propagate_wakeup(dev->bus, enable); > -} > - > static bool acpi_pci_need_resume(struct pci_dev *dev) > { > struct acpi_device *adev = ACPI_COMPANION(&dev->dev); > @@ -610,14 +585,29 @@ static bool acpi_pci_need_resume(struct pci_dev *dev) > return !!adev->power.flags.dsw_present; > } > > -static const struct pci_platform_pm_ops acpi_pci_platform_pm = { > - .is_manageable = acpi_pci_power_manageable, > - .set_state = acpi_pci_set_power_state, > - .get_state = acpi_pci_get_power_state, > - .choose_state = acpi_pci_choose_state, > - .set_wakeup = acpi_pci_wakeup, > - .need_resume = acpi_pci_need_resume, > -}; > +static bool acpi_pci_can_wakeup(void *pmdata) > +{ > + struct device *dev = pmdata; > + > + if (!dev) > + return false; > + > + return acpi_pm_device_can_wakeup(dev); > +} > + > +static int acpi_pci_dev_wakeup(void *pmdata, bool enable) > +{ > + struct device *dev = pmdata; > + > + return acpi_pm_set_device_wakeup(dev, enable); > +} > + > +static int acpi_pci_bridge_wakeup(void *pmdata, bool enable) > +{ > + struct device *dev = pmdata; > + > + return acpi_pm_set_bridge_wakeup(dev, enable); > +} > > void acpi_pci_add_bus(struct pci_bus *bus) > { > @@ -658,20 +648,6 @@ void acpi_pci_remove_bus(struct pci_bus *bus) > acpi_pci_slot_remove(bus); > } > > -/* ACPI bus type */ > -static struct acpi_device *acpi_pci_find_companion(struct device *dev) > -{ > - struct pci_dev *pci_dev = to_pci_dev(dev); > - bool check_children; > - u64 addr; > - > - check_children = pci_is_bridge(pci_dev); > - /* Please ref to ACPI spec for the syntax of _ADR */ > - addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); > - return acpi_find_child_device(ACPI_COMPANION(dev->parent), addr, > - check_children); > -} > - > /** > * pci_acpi_optimize_delay - optimize PCI D3 and D3cold delay from ACPI > * @pdev: the PCI device whose delay is to be updated > @@ -723,34 +699,55 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev, > ACPI_FREE(obj); > } > > -static void pci_acpi_setup(struct device *dev) > +static void *acpi_pci_setup_dev(struct pci_dev *pci_dev) > { > - struct pci_dev *pci_dev = to_pci_dev(dev); > - struct acpi_device *adev = ACPI_COMPANION(dev); > + struct acpi_device *adev = ACPI_COMPANION(&pci_dev->dev); > > if (!adev) > - return; > + return NULL; > > pci_acpi_optimize_delay(pci_dev, adev->handle); > > pci_acpi_add_pm_notifier(adev, pci_dev); > if (!adev->wakeup.flags.valid) > - return; > + return NULL; > + > + device_set_wakeup_capable(&pci_dev->dev, true); > + acpi_pm_set_device_wakeup(&pci_dev->dev, false); > > - device_set_wakeup_capable(dev, true); > - acpi_pci_wakeup(pci_dev, false); > + return &pci_dev->dev; > } > > -static void pci_acpi_cleanup(struct device *dev) > +static void *acpi_pci_setup_host_bridge(struct pci_host_bridge *bridge) > { > - struct acpi_device *adev = ACPI_COMPANION(dev); > + return &bridge->dev; > +} > > - if (!adev) > - return; > +static const struct pci_platform_pm_ops acpi_pci_platform_pm = { > + .is_manageable = acpi_pci_power_manageable, > + .set_state = acpi_pci_set_power_state, > + .get_state = acpi_pci_get_power_state, > + .choose_state = acpi_pci_choose_state, > + .need_resume = acpi_pci_need_resume, > + .setup_dev = acpi_pci_setup_dev, > + .setup_host_bridge = acpi_pci_setup_host_bridge, > + .can_wakeup = acpi_pci_can_wakeup, > + .dev_wakeup = acpi_pci_dev_wakeup, > + .bridge_wakeup = acpi_pci_bridge_wakeup, > +}; > + > +/* ACPI bus type */ > +static struct acpi_device *acpi_pci_find_companion(struct device *dev) > +{ > + struct pci_dev *pci_dev = to_pci_dev(dev); > + bool check_children; > + u64 addr; > > - pci_acpi_remove_pm_notifier(adev); > - if (adev->wakeup.flags.valid) > - device_set_wakeup_capable(dev, false); > + check_children = pci_is_bridge(pci_dev); > + /* Please ref to ACPI spec for the syntax of _ADR */ > + addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); > + return acpi_find_child_device(ACPI_COMPANION(dev->parent), addr, > + check_children); > } > > static bool pci_acpi_bus_match(struct device *dev) > @@ -762,8 +759,6 @@ static struct acpi_bus_type acpi_pci_bus = { > .name = "PCI", > .match = pci_acpi_bus_match, > .find_companion = acpi_pci_find_companion, > - .setup = pci_acpi_setup, > - .cleanup = pci_acpi_cleanup, > }; > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 9be563067c0c..abb7caa8a6e5 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -421,10 +421,17 @@ static int pci_device_probe(struct device *dev) > if (error < 0) > return error; > > + pci_dev->pmdata = platform_pci_setup_dev(pci_dev); > + if (IS_ERR(pci_dev->pmdata)) { > + pcibios_free_irq(pci_dev); > + return PTR_ERR(pci_dev->pmdata); > + } > + > pci_dev_get(pci_dev); > if (pci_device_can_probe(pci_dev)) { > error = __pci_device_probe(drv, pci_dev); > if (error) { > + platform_pci_cleanup(pci_dev->pmdata); > pcibios_free_irq(pci_dev); > pci_dev_put(pci_dev); > } > @@ -438,6 +445,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; > > + platform_pci_cleanup(pci_dev->pmdata); > + > if (drv) { > if (drv->remove) { > pm_runtime_get_sync(dev); > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e120b00a9017..6add5d3209bf 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -585,6 +585,52 @@ static inline bool platform_pci_power_manageable(struct pci_dev *dev) > return false; > } > > +void *platform_pci_setup_dev(struct pci_dev *dev) > +{ > + if (pci_platform_pm && pci_platform_pm->setup_dev) > + return pci_platform_pm->setup_dev(dev); > + > + return NULL; > +} > + > +void *platform_pci_setup_host_bridge(struct pci_host_bridge *bridge) > +{ > + if (pci_platform_pm && pci_platform_pm->setup_host_bridge) > + return pci_platform_pm->setup_host_bridge(bridge); > + > + return NULL; > +} > + > +void platform_pci_cleanup(void *pmdata) > +{ > + if (pci_platform_pm && pci_platform_pm->cleanup) > + pci_platform_pm->cleanup(pmdata); > +} > + > +static inline bool platform_pci_can_wakeup(void *pmdata) > +{ > + if (pci_platform_pm && pci_platform_pm->can_wakeup) > + return pci_platform_pm->can_wakeup(pmdata); > + > + return false; > +} > + > +static inline int platform_pci_dev_wakeup(void *pmdata, bool enable) > +{ > + if (pci_platform_pm && pci_platform_pm->dev_wakeup) > + return pci_platform_pm->dev_wakeup(pmdata, enable); > + > + return -ENODEV; > +} > + > +static inline int platform_pci_bridge_wakeup(void *pmdata, bool enable) > +{ > + if (pci_platform_pm && pci_platform_pm->bridge_wakeup) > + return pci_platform_pm->bridge_wakeup(pmdata, enable); > + > + return -ENODEV; > +} > + > static inline int platform_pci_set_power_state(struct pci_dev *dev, > pci_power_t t) > { > @@ -610,14 +656,6 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) > return PCI_POWER_ERROR; > } > > -static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) > -{ > - if (pci_platform_pm && pci_platform_pm->set_wakeup) > - return pci_platform_pm->set_wakeup(dev, enable); > - > - return -ENODEV; So this function used to return -ENODEV if the platform didn't implement PM. Now you're transitioning to pci_set_wakeup() below: > -} > - > static inline bool platform_pci_need_resume(struct pci_dev *dev) > { > if (pci_platform_pm && pci_platform_pm->need_resume) > @@ -1903,6 +1941,32 @@ void pci_pme_active(struct pci_dev *dev, bool enable) > } > EXPORT_SYMBOL(pci_pme_active); > > +static int pci_set_wakeup(struct pci_dev *dev, bool enable) > +{ > + struct pci_bus *bus = dev->bus; > + struct pci_host_bridge *bridge; > + > + /* Handle per device wakeup */ > + if (platform_pci_can_wakeup(dev->pmdata)) > + return platform_pci_dev_wakeup(dev->pmdata, enable); > + > + /* Find a parent which can handle wakeup */ > + while (bus->parent) { > + if (platform_pci_can_wakeup(bus->self->pmdata)) > + return platform_pci_bridge_wakeup(bus->self->pmdata, > + enable); > + > + bus = bus->parent; > + } > + > + /* We have reached the root bus. */ > + bridge = to_pci_host_bridge(bus->bridge); > + if (platform_pci_can_wakeup(bridge->pmdata)) > + return platform_pci_bridge_wakeup(bridge->pmdata, enable); > + > + return 0; And it looks like the fallback behavior is just 'return 0', if none of the devices supported wakeup. It's weird that this already used to happen for ACPI -- if no device/bridge implemented wakeup, we still returned success. But now you're making this happen for platforms without PM operations too. This affects the behavior of pci_enable_wake below, which tries to return an error if both PME and "platform wake" failed. I wonder if this should just return an error code if we reached the end (i.e., no device or bridge supported wakeup). This would technically change the ACPI behavior, but then I think it would also be more correct. If you do this, that should probably be a separate patch, to be clear we're changing the error logic before we refactor. > +} > + > /** > * pci_enable_wake - enable PCI device as wakeup event source > * @dev: PCI device affected > @@ -1950,13 +2014,13 @@ int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable) > pci_pme_active(dev, true); > else > ret = 1; > - error = platform_pci_set_wakeup(dev, true); > + error = pci_set_wakeup(dev, true); > if (ret) > ret = error; > if (!ret) > dev->wakeup_prepared = true; > } else { > - platform_pci_set_wakeup(dev, false); > + pci_set_wakeup(dev, false); > pci_pme_active(dev, false); > dev->wakeup_prepared = false; > } > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 048668271014..dcefb9e759d7 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -47,21 +47,43 @@ int pci_probe_reset_function(struct pci_dev *dev); > * platform; to be used during system-wide transitions from a > * sleeping state to the working state and vice versa > * > - * @set_wakeup: enables/disables wakeup capability for the device > - * > * @need_resume: returns 'true' if the given device (which is currently > * suspended) needs to be resumed to be configured for system > * wakeup. > + * > + * @setup_dev: setup device's wakeup ability, alloc and return device's private > + * pm data. > + * > + * @setup_host_bridge: setup host bridge's wakeup ability, alloc and return host > + * bridge's private pm data. > + * > + * @cleanup: cleanup the private pm data and deinit wakeup ability. > + * > + * @can_wakeup: returns 'true' if given device is capable of waking up the > + * system from a sleeping state. > + * > + * @dev_wakeup: enables/disables wakeup capability for the device. > + * > + * @bridge_wakeup: enables/disables wakeup capability for the bridge. > */ > struct pci_platform_pm_ops { > bool (*is_manageable)(struct pci_dev *dev); > int (*set_state)(struct pci_dev *dev, pci_power_t state); > pci_power_t (*get_state)(struct pci_dev *dev); > pci_power_t (*choose_state)(struct pci_dev *dev); > - int (*set_wakeup)(struct pci_dev *dev, bool enable); I believe you're breaking pci-mid.c, which still assigns a '.set_wakeup' callback. > bool (*need_resume)(struct pci_dev *dev); > + void *(*setup_dev)(struct pci_dev *dev); > + void *(*setup_host_bridge)(struct pci_host_bridge *bridge); > + void (*cleanup)(void *pmdata); > + bool (*can_wakeup)(void *pmdata); > + int (*dev_wakeup)(void *pmdata, bool enable); > + int (*bridge_wakeup)(void *pmdata, bool enable); You're splitting the "set_wakeup" callback into a "dev_wakeup" and a "bridge_wakeup" function, but you're losing the "set" terminology, which I think makes things clearer here. So, maybe "set_dev_wakeup" and "set_bridge_wakeup"? And same for the corresponding platform_pci_*() helpers. Brian > }; > > +void *platform_pci_setup_dev(struct pci_dev *dev); > +void *platform_pci_setup_host_bridge(struct pci_host_bridge *bridge); > +void platform_pci_cleanup(void *pmdata); > + > void pci_set_platform_pm(const struct pci_platform_pm_ops *ops); > void pci_update_current_state(struct pci_dev *dev, pci_power_t state); > void pci_power_up(struct pci_dev *dev); > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index cdc2f83c11c5..b12c552a5522 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -809,7 +809,13 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > > err = device_register(&bus->dev); > if (err) > - goto unregister; > + goto unregister_bridge; > + > + bridge->pmdata = platform_pci_setup_host_bridge(bridge); > + if (IS_ERR(bridge->pmdata)) { > + err = PTR_ERR(bridge->pmdata); > + goto unregister_bus; > + } > > pcibios_add_bus(bus); > > @@ -853,7 +859,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > > return 0; > > -unregister: > +unregister_bus: > + device_unregister(&bus->dev); > +unregister_bridge: > put_device(&bridge->dev); > device_unregister(&bridge->dev); > > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c > index 73a03d382590..d7a8cf1dc69f 100644 > --- a/drivers/pci/remove.c > +++ b/drivers/pci/remove.c > @@ -153,6 +153,8 @@ void pci_remove_root_bus(struct pci_bus *bus) > if (!pci_is_root_bus(bus)) > return; > > + platform_pci_cleanup(host_bridge->pmdata); > + > host_bridge = to_pci_host_bridge(bus->bridge); > list_for_each_entry_safe(child, tmp, > &bus->devices, bus_list) > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 80eaa2dbe3e9..628faa58c790 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -293,6 +293,7 @@ struct pci_dev { > struct pci_bus *subordinate; /* bus this device bridges to */ > > void *sysdata; /* hook for sys-specific extension */ > + void *pmdata; /* data for platform pm */ > struct proc_dir_entry *procent; /* device entry in /proc/bus/pci */ > struct pci_slot *slot; /* Physical slot this device is in */ > > @@ -467,6 +468,7 @@ struct pci_host_bridge { > struct pci_bus *bus; /* root bus */ > struct pci_ops *ops; > void *sysdata; > + void *pmdata; /* data for platform pm */ > int busnr; > struct list_head windows; /* resource_entry */ > u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* platform IRQ swizzler */ > -- > 2.11.0 > >
On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote: > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other > platforms could reuse it. What exactly do you want to reuse? It looks like that's just several lines of code in acpi_pci_wakeup() and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level functions, so IMO not worth it at all. The structure for other platform code may be the same or similar, but the details will almost certainly be different and I don't think that having more callback pointers in pci_platform_pm_ops is necessarily better. > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm > ops's callbacks to setup and cleanup pci devices and host bridge for > wakeup. Why are they needed? > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> Thanks, Rafael
Hi Rafael, I'll answer some of it from my perspective, though Jeffy might have had different ideas (and answers) when he implemented this. On Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote: > On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote: > > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other > > platforms could reuse it. > > What exactly do you want to reuse? > > It looks like that's just several lines of code in acpi_pci_wakeup() > and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level > functions, so IMO not worth it at all. The important part he's sharing here is the walking of the tree structure, in which it's possible for some parent along the way to handle wakeup for its children. I'm not sure how valuable nor how reusable that is. In this case (the Rockchip platforms Jeffy and I are working on), I think we really want to just support a single WAKE# pin for the whole system, so maybe the complexity isn't needed. The spec does describe that there are good reasons for supporting more than 1 WAKE# pin though (e.g., 1 per device), so it doesn't seem really wise to shoehorn oursleves into a single setup. But that can be implemented either via copying the "few" lines of tree-walking logic, or by trying to share them. > The structure for other platform code may be the same or similar, but > the details will almost certainly be different and I don't think that > having more callback pointers in pci_platform_pm_ops is necessarily better. I suppose that's reasonable. > > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm > > ops's callbacks to setup and cleanup pci devices and host bridge for > > wakeup. > > Why are they needed? The implementation is in patch 7, if you really needed more info about why, or provide alternatives. The current set of hooks assumes that there is no state information or initialization needed for tracking actions of these platform PM hooks on a device. For ACPI this works, because devices have "companion" acpi_dev's to handle everything, and the ACPI framework generally prepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be trivial (and arguably wrong -- several are no-ops, where we might expect the platform to tell us whether the operation was actually supported or not). For device tree, there isn't really a canonical place to store this information, nor to initialize something like wakeup interrupts. Technically, we could shoehorn this into the .set_wakeup() call, but we'd probably rather not do things like request_irq() on every attempt to suspend/resume the system (among other reasons, we'd lose information that we might otherwise track in /proc/ or /sys/). Or the inverse of the above: where would you suggest initializing or tearing down the wakeirq? An alternative could be to include any necessary state into the pci_host_bridge or pci_dev and just inline the setup code into pci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c (pci_device_{probe,remove}()). But I'm not sure that's much more beautiful. Brian > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > > Thanks, > Rafael >
On Tuesday, November 14, 2017 3:51:11 AM CET Brian Norris wrote: > Hi Rafael, > > I'll answer some of it from my perspective, though Jeffy might have had > different ideas (and answers) when he implemented this. > > On Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote: > > On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote: > > > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other > > > platforms could reuse it. > > > > What exactly do you want to reuse? > > > > It looks like that's just several lines of code in acpi_pci_wakeup() > > and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level > > functions, so IMO not worth it at all. > > The important part he's sharing here is the walking of the tree > structure, in which it's possible for some parent along the way to > handle wakeup for its children. I'm not sure how valuable nor how > reusable that is. ACPI very well may be the only case needing to walk the hierarchy for that. > In this case (the Rockchip platforms Jeffy and I are working on), I > think we really want to just support a single WAKE# pin for the whole > system, so maybe the complexity isn't needed. So unless I know you'll need it, please don't add it. > The spec does describe that there are good reasons for supporting more than > 1 WAKE# pin though (e.g., 1 per device), so it doesn't seem really wise to > shoehorn oursleves into a single setup. One WAKE# pin per device is reasonable enough, but WAKE# is specifically defined as out-of-band and "orthogonal" to the PCIe hierarchy. What you need is a way to program WAKE# and (possibly) wakeup power on the given platform for each device having a WAKE# pin individually. The main reason why ACPI needs to walk the hierarchy is that on some systems the firmware takes over the handling of the native PME mechanism which then is taken care of by the AML (and the kernel is not granted control of the relevant PCIe registers). I don't think you'll ever see anything like that on non-ACPI systems. > But that can be implemented either via copying the "few" lines of > tree-walking logic, or by trying to share them. > > > The structure for other platform code may be the same or similar, but > > the details will almost certainly be different and I don't think that > > having more callback pointers in pci_platform_pm_ops is necessarily better. > > I suppose that's reasonable. > > > > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm > > > ops's callbacks to setup and cleanup pci devices and host bridge for > > > wakeup. > > > > Why are they needed? > > The implementation is in patch 7, if you really needed more info about > why, or provide alternatives. Well, that's quite questionable. At least defining ->dev_wakeup to do device_set_wakeup_capable() doesn't really make sense to me. > The current set of hooks assumes that there is no state information or > initialization needed for tracking actions of these platform PM hooks on > a device. For ACPI this works, because devices have "companion" > acpi_dev's to handle everything, and the ACPI framework generally > prepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be > trivial (and arguably wrong -- several are no-ops, where we might expect > the platform to tell us whether the operation was actually supported or > not). > > For device tree, there isn't really a canonical place to store this > information, nor to initialize something like wakeup interrupts. The only thing you need IMO is ->set_wakeup which also is what ACPI needs and that enables or disables wakeup for the device. It doesn't actually have to track anything (other than, possibly, whether or not wakeup has been enabled for it already). And since the core takes care of native PMEs for you, the only thing this needs to cover is the WAKE# pin programming AFAICS. The setup part, in turn, in the ACPI case is done via acpi_platform_notify() and, analogously, acpi_platform_notify_remove() does the cleanup. You seem to be trying to add something like it via pci_platform_pm_ops, but is it really the most suitable place for that? > Technically, we could shoehorn this into the .set_wakeup() call, but > we'd probably rather not do things like request_irq() on every attempt > to suspend/resume the system (among other reasons, we'd lose information > that we might otherwise track in /proc/ or /sys/). > > Or the inverse of the above: where would you suggest initializing or > tearing down the wakeirq? Again, ACPI does that via acpi_platform_notify()/acpi_platform_notify_remove(), so maybe something similar? In any case, you need a way to associate DT-provided data with PCI devices and, ideally, that should be done at the enumeration time. > An alternative could be to include any necessary state into the > pci_host_bridge or pci_dev and just inline the setup code into > pci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c > (pci_device_{probe,remove}()). But I'm not sure that's much more > beautiful. Well, that's not the way it is done in the ACPI case anyway. Thanks, Rafael
Hi Rafael, Thanks for the responses, and sorry for some delay. My thoughts are still a bit scattered on this (and I'm also busy with other things), and I'd like some feedback on the device tree definitions from someone, if possible. (I expect you're more familiar with ACPI than with device tree, but perhaps you or someone else on copy can humor me?) I'm also not sure I agree with all of your suggestions, though maybe something "similar" could be OK. On Wed, Nov 22, 2017 at 01:26:02AM +0100, Rafael J. Wysocki wrote: > On Tuesday, November 14, 2017 3:51:11 AM CET Brian Norris wrote: > > On Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote: > > > On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote: > > > > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other > > > > platforms could reuse it. > > > > > > What exactly do you want to reuse? > > > > > > It looks like that's just several lines of code in acpi_pci_wakeup() > > > and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level > > > functions, so IMO not worth it at all. > > > > The important part he's sharing here is the walking of the tree > > structure, in which it's possible for some parent along the way to > > handle wakeup for its children. I'm not sure how valuable nor how > > reusable that is. > > ACPI very well may be the only case needing to walk the hierarchy for that. Sure. It does seem like a bit of overkill, on second thought. > > In this case (the Rockchip platforms Jeffy and I are working on), I > > think we really want to just support a single WAKE# pin for the whole > > system, so maybe the complexity isn't needed. > > So unless I know you'll need it, please don't add it. Sure, arbitrary hierarchy is probably over-engineering. But 1-per-device is quite reasonable -- even if we don't support it immediately, it would be nice if it can be added without too much trouble. > > The spec does describe that there are good reasons for supporting more than > > 1 WAKE# pin though (e.g., 1 per device), so it doesn't seem really wise to > > shoehorn oursleves into a single setup. > > One WAKE# pin per device is reasonable enough, but WAKE# is specifically > defined as out-of-band and "orthogonal" to the PCIe hierarchy. What you > need is a way to program WAKE# and (possibly) wakeup power on the given > platform for each device having a WAKE# pin individually. So, *don't* define the wakeup in the host bridge? Or put another way: how would you define a DT description for this? By the way, it seems pretty ambiguous how we want to handle things like (a) multiple devices sharing the same WAKE# (b) systems where a slot is swappable For (a), the main problem is that if we have to repeat the interrupt definition in multiple devices, then we have to deal with something like IRQF_SHARED. That can be done, but it makes it much harder to use the dedicated wakeirq helpers. For (b), it's already weird to write device trees for probable PCI devices; you technically need to redefine the node for the PCI ID of each device that gets plugged in. So instead, it seems more like maybe a property of the port? Or more concretely: pcie@XXXXXXXX { compatible = "rockchip,rk3399-pcie"; interrupts-extended = ..., <&gpioX Y>; // the existing // proposal, for // 'wakeup' in the host // bridge interrupt-names = ..., "wakeup"; pcie_port: pcie@0,0 { ... /* // This is a possible proposal, to account for (b)? interrupts = <Y>; interrupt-parent = <&gpioX>; interrupt-names = "wakeup"; */ wifi_device: wifi@0,0 { compatible = "pci11ab,2b42"; /* // This is the "1-per-device" proposal? // But it doesn't work well if the PCI ID // changes (e.g., card swap). Probably want to // avoid this if possible. // Also, it has an awkward conflict with INTx // definitions. interrupts-extended = <&intX 0>, <&gpioX Y>; interrupt-names = "pci", "wakeup"; */ ... }; }; }; Does the description at the "port" level seem reasonable? We'd still have to figure out how to parse this properly of course... > The main reason why ACPI needs to walk the hierarchy is that on some systems > the firmware takes over the handling of the native PME mechanism which then is > taken care of by the AML (and the kernel is not granted control of the > relevant PCIe registers). I don't think you'll ever see anything like that > on non-ACPI systems. OK. > > But that can be implemented either via copying the "few" lines of > > tree-walking logic, or by trying to share them. > > > > > The structure for other platform code may be the same or similar, but > > > the details will almost certainly be different and I don't think that > > > having more callback pointers in pci_platform_pm_ops is necessarily better. > > > > I suppose that's reasonable. > > > > > > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm > > > > ops's callbacks to setup and cleanup pci devices and host bridge for > > > > wakeup. > > > > > > Why are they needed? > > > > The implementation is in patch 7, if you really needed more info about > > why, or provide alternatives. > > Well, that's quite questionable. > > At least defining ->dev_wakeup to do device_set_wakeup_capable() doesn't > really make sense to me. Hmm, that does seem a little weird, but I believe maybe Jeffy is using it as a hack for fitting in with the "dedicated IRQ" stuff? It seems more like you should be able to use something like dev_pm_enable_wake_irq() / dev_pm_disable_wake_irq() instead though. That's a question for patch 7 though, not for the general architecture of these hooks -- we want to do *something* the arm/disarm the wake-IRQ there. > > The current set of hooks assumes that there is no state information > > or > > initialization needed for tracking actions of these platform PM hooks on > > a device. For ACPI this works, because devices have "companion" > > acpi_dev's to handle everything, and the ACPI framework generally > > prepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be > > trivial (and arguably wrong -- several are no-ops, where we might expect > > the platform to tell us whether the operation was actually supported or > > not). > > > > For device tree, there isn't really a canonical place to store this > > information, nor to initialize something like wakeup interrupts. > > The only thing you need IMO is ->set_wakeup which also is what ACPI needs > and that enables or disables wakeup for the device. It doesn't actually > have to track anything (other than, possibly, whether or not wakeup has > been enabled for it already). Yes, that's technically the only essential thing needed, though it does make things a little tougher. We still may need to (depending on whether we decide to define WAKE# in the bridge, the port, or the device) do some hierarchy walking. I'm also not yet convinced about "where to initialize" (see comments below). > And since the core takes care of native PMEs for you, the only thing > this needs to cover is the WAKE# pin programming AFAICS. Right. > The setup part, in turn, in the ACPI case is done via acpi_platform_notify() > and, analogously, acpi_platform_notify_remove() does the cleanup. You seem > to be trying to add something like it via pci_platform_pm_ops, but is it > really the most suitable place for that? Well, the global 'platform_notify' and 'platform_notify_remove' singletons seem even more fragile. Do we really want to take over the whole system's device registration hooks just to handle something for a particular bus type? We're not going to be able to write a generic "add" and "remove" hook for all devices on all Device Tree systems for this type of feature. It would likely break a lot of sub-devices to start parsing "wakeup" properties there. We *could* do something like what it8152 does: static int it8152_pci_platform_notify(struct device *dev) { if (dev_is_pci(dev)) { ... } return 0; } but that doesn't look very nice to me. Among other problems, it won't stack nicely. What if an 'it8152' system wants to use this 'wakeup' interrupt definition too? Instead, I think it makes perfect sense to put the DT-centric wakeup handling all in the same module -- the "Firmware PM callbacks" (struct pci_platform_pm_ops) -- instead of scattering it around the codebase. > > Technically, we could shoehorn this into the .set_wakeup() call, but > > we'd probably rather not do things like request_irq() on every attempt > > to suspend/resume the system (among other reasons, we'd lose information > > that we might otherwise track in /proc/ or /sys/). > > > > Or the inverse of the above: where would you suggest initializing or > > tearing down the wakeirq? > > Again, ACPI does that via acpi_platform_notify()/acpi_platform_notify_remove(), > so maybe something similar? I don't like that solution, per the above. (Depending on what "or maybe something similar" means.) > In any case, you need a way to associate DT-provided data with PCI devices and, > ideally, that should be done at the enumeration time. Yes, but that's what these hooks were allowing; PCI PM firmware hooks get a chance to do enumeration-time initialization for both bridges and devices. > > An alternative could be to include any necessary state into the > > pci_host_bridge or pci_dev and just inline the setup code into > > pci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c > > (pci_device_{probe,remove}()). But I'm not sure that's much more > > beautiful. > > Well, that's not the way it is done in the ACPI case anyway. Brian
* Brian Norris <briannorris@chromium.org> [171206 19:36]: > By the way, it seems pretty ambiguous how we want to handle things like > (a) multiple devices sharing the same WAKE# > (b) systems where a slot is swappable > > For (a), the main problem is that if we have to repeat the interrupt > definition in multiple devices, then we have to deal with something like > IRQF_SHARED. That can be done, but it makes it much harder to use the > dedicated wakeirq helpers. This will get messy, let's not go there :) That is unless the hardware really has a single interrupt wired to multiple devices. And in that case almost certainly a custom interrupt handler is needed. Regards, Tony
On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote: > * Brian Norris <briannorris@chromium.org> [171206 19:36]: > > By the way, it seems pretty ambiguous how we want to handle things like > > (a) multiple devices sharing the same WAKE# > > (b) systems where a slot is swappable > > > > For (a), the main problem is that if we have to repeat the interrupt > > definition in multiple devices, then we have to deal with something like > > IRQF_SHARED. That can be done, but it makes it much harder to use the > > dedicated wakeirq helpers. > > This will get messy, let's not go there :) That is unless the hardware > really has a single interrupt wired to multiple devices. And in that > case almost certainly a custom interrupt handler is needed. As Rafael mentioned, the spec doesn't clearly delineate a required hierarchy to the WAKE# pin, and it's certainly possible to share it. I'm fine dodging that question for now, and only writing said custom interrupt handler if/when needed. But device tree bindings are "forever", so it seems reasonable to at least agree how it should be defined. Brian
* Brian Norris <briannorris@chromium.org> [171207 00:32]: > On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote: > > * Brian Norris <briannorris@chromium.org> [171206 19:36]: > > > By the way, it seems pretty ambiguous how we want to handle things like > > > (a) multiple devices sharing the same WAKE# > > > (b) systems where a slot is swappable > > > > > > For (a), the main problem is that if we have to repeat the interrupt > > > definition in multiple devices, then we have to deal with something like > > > IRQF_SHARED. That can be done, but it makes it much harder to use the > > > dedicated wakeirq helpers. > > > > This will get messy, let's not go there :) That is unless the hardware > > really has a single interrupt wired to multiple devices. And in that > > case almost certainly a custom interrupt handler is needed. > > As Rafael mentioned, the spec doesn't clearly delineate a required > hierarchy to the WAKE# pin, and it's certainly possible to share it. I'm > fine dodging that question for now, and only writing said custom > interrupt handler if/when needed. OK if the WAKE# pin is shared then PCI (or hardware specific?) code needs to figure out from where it came from. > But device tree bindings are "forever", so it seems reasonable to at > least agree how it should be defined. Well that's why we're just using the existing interrupts-extended binding there :) It does not leave out the option for shared interrupts, it's just that drivers/base/power/wakeirq.c can't deal with them in a sane way or at least we'd have to add a flag to not enable/disable the wakeirq automatically. Regards, Tony
On Fri, Dec 8, 2017 at 5:37 PM, Tony Lindgren <tony@atomide.com> wrote: > * Brian Norris <briannorris@chromium.org> [171207 00:32]: >> On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote: >> > * Brian Norris <briannorris@chromium.org> [171206 19:36]: >> > > By the way, it seems pretty ambiguous how we want to handle things like >> > > (a) multiple devices sharing the same WAKE# >> > > (b) systems where a slot is swappable >> > > >> > > For (a), the main problem is that if we have to repeat the interrupt >> > > definition in multiple devices, then we have to deal with something like >> > > IRQF_SHARED. That can be done, but it makes it much harder to use the >> > > dedicated wakeirq helpers. >> > >> > This will get messy, let's not go there :) That is unless the hardware >> > really has a single interrupt wired to multiple devices. And in that >> > case almost certainly a custom interrupt handler is needed. >> >> As Rafael mentioned, the spec doesn't clearly delineate a required >> hierarchy to the WAKE# pin, and it's certainly possible to share it. I'm >> fine dodging that question for now, and only writing said custom >> interrupt handler if/when needed. > > OK if the WAKE# pin is shared then PCI (or hardware specific?) code needs > to figure out from where it came from. Right. Basically, that would need a list (or equivalent) of devices sharing the wakeup IRQ and if that triggers, it needs to walk the list and call pci_pme_wakeup() for all devices in it. >> But device tree bindings are "forever", so it seems reasonable to at >> least agree how it should be defined. > > Well that's why we're just using the existing interrupts-extended > binding there :) It does not leave out the option for shared interrupts, > it's just that drivers/base/power/wakeirq.c can't deal with them in a > sane way or at least we'd have to add a flag to not enable/disable the > wakeirq automatically. I think that the binding needs to be sort-of PCI-specific to cover the shared case. I'm not sure if there are any other buses needing that (they would need to define standard PM registers as PCI does).
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 4708eb9df71b..ee96e7afe1ac 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -569,31 +569,6 @@ static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev) return state_conv[state]; } -static int acpi_pci_propagate_wakeup(struct pci_bus *bus, bool enable) -{ - while (bus->parent) { - if (acpi_pm_device_can_wakeup(&bus->self->dev)) - return acpi_pm_set_bridge_wakeup(&bus->self->dev, enable); - - bus = bus->parent; - } - - /* We have reached the root bus. */ - if (bus->bridge) { - if (acpi_pm_device_can_wakeup(bus->bridge)) - return acpi_pm_set_bridge_wakeup(bus->bridge, enable); - } - return 0; -} - -static int acpi_pci_wakeup(struct pci_dev *dev, bool enable) -{ - if (acpi_pm_device_can_wakeup(&dev->dev)) - return acpi_pm_set_device_wakeup(&dev->dev, enable); - - return acpi_pci_propagate_wakeup(dev->bus, enable); -} - static bool acpi_pci_need_resume(struct pci_dev *dev) { struct acpi_device *adev = ACPI_COMPANION(&dev->dev); @@ -610,14 +585,29 @@ static bool acpi_pci_need_resume(struct pci_dev *dev) return !!adev->power.flags.dsw_present; } -static const struct pci_platform_pm_ops acpi_pci_platform_pm = { - .is_manageable = acpi_pci_power_manageable, - .set_state = acpi_pci_set_power_state, - .get_state = acpi_pci_get_power_state, - .choose_state = acpi_pci_choose_state, - .set_wakeup = acpi_pci_wakeup, - .need_resume = acpi_pci_need_resume, -}; +static bool acpi_pci_can_wakeup(void *pmdata) +{ + struct device *dev = pmdata; + + if (!dev) + return false; + + return acpi_pm_device_can_wakeup(dev); +} + +static int acpi_pci_dev_wakeup(void *pmdata, bool enable) +{ + struct device *dev = pmdata; + + return acpi_pm_set_device_wakeup(dev, enable); +} + +static int acpi_pci_bridge_wakeup(void *pmdata, bool enable) +{ + struct device *dev = pmdata; + + return acpi_pm_set_bridge_wakeup(dev, enable); +} void acpi_pci_add_bus(struct pci_bus *bus) { @@ -658,20 +648,6 @@ void acpi_pci_remove_bus(struct pci_bus *bus) acpi_pci_slot_remove(bus); } -/* ACPI bus type */ -static struct acpi_device *acpi_pci_find_companion(struct device *dev) -{ - struct pci_dev *pci_dev = to_pci_dev(dev); - bool check_children; - u64 addr; - - check_children = pci_is_bridge(pci_dev); - /* Please ref to ACPI spec for the syntax of _ADR */ - addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); - return acpi_find_child_device(ACPI_COMPANION(dev->parent), addr, - check_children); -} - /** * pci_acpi_optimize_delay - optimize PCI D3 and D3cold delay from ACPI * @pdev: the PCI device whose delay is to be updated @@ -723,34 +699,55 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev, ACPI_FREE(obj); } -static void pci_acpi_setup(struct device *dev) +static void *acpi_pci_setup_dev(struct pci_dev *pci_dev) { - struct pci_dev *pci_dev = to_pci_dev(dev); - struct acpi_device *adev = ACPI_COMPANION(dev); + struct acpi_device *adev = ACPI_COMPANION(&pci_dev->dev); if (!adev) - return; + return NULL; pci_acpi_optimize_delay(pci_dev, adev->handle); pci_acpi_add_pm_notifier(adev, pci_dev); if (!adev->wakeup.flags.valid) - return; + return NULL; + + device_set_wakeup_capable(&pci_dev->dev, true); + acpi_pm_set_device_wakeup(&pci_dev->dev, false); - device_set_wakeup_capable(dev, true); - acpi_pci_wakeup(pci_dev, false); + return &pci_dev->dev; } -static void pci_acpi_cleanup(struct device *dev) +static void *acpi_pci_setup_host_bridge(struct pci_host_bridge *bridge) { - struct acpi_device *adev = ACPI_COMPANION(dev); + return &bridge->dev; +} - if (!adev) - return; +static const struct pci_platform_pm_ops acpi_pci_platform_pm = { + .is_manageable = acpi_pci_power_manageable, + .set_state = acpi_pci_set_power_state, + .get_state = acpi_pci_get_power_state, + .choose_state = acpi_pci_choose_state, + .need_resume = acpi_pci_need_resume, + .setup_dev = acpi_pci_setup_dev, + .setup_host_bridge = acpi_pci_setup_host_bridge, + .can_wakeup = acpi_pci_can_wakeup, + .dev_wakeup = acpi_pci_dev_wakeup, + .bridge_wakeup = acpi_pci_bridge_wakeup, +}; + +/* ACPI bus type */ +static struct acpi_device *acpi_pci_find_companion(struct device *dev) +{ + struct pci_dev *pci_dev = to_pci_dev(dev); + bool check_children; + u64 addr; - pci_acpi_remove_pm_notifier(adev); - if (adev->wakeup.flags.valid) - device_set_wakeup_capable(dev, false); + check_children = pci_is_bridge(pci_dev); + /* Please ref to ACPI spec for the syntax of _ADR */ + addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); + return acpi_find_child_device(ACPI_COMPANION(dev->parent), addr, + check_children); } static bool pci_acpi_bus_match(struct device *dev) @@ -762,8 +759,6 @@ static struct acpi_bus_type acpi_pci_bus = { .name = "PCI", .match = pci_acpi_bus_match, .find_companion = acpi_pci_find_companion, - .setup = pci_acpi_setup, - .cleanup = pci_acpi_cleanup, }; diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 9be563067c0c..abb7caa8a6e5 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -421,10 +421,17 @@ static int pci_device_probe(struct device *dev) if (error < 0) return error; + pci_dev->pmdata = platform_pci_setup_dev(pci_dev); + if (IS_ERR(pci_dev->pmdata)) { + pcibios_free_irq(pci_dev); + return PTR_ERR(pci_dev->pmdata); + } + pci_dev_get(pci_dev); if (pci_device_can_probe(pci_dev)) { error = __pci_device_probe(drv, pci_dev); if (error) { + platform_pci_cleanup(pci_dev->pmdata); pcibios_free_irq(pci_dev); pci_dev_put(pci_dev); } @@ -438,6 +445,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; + platform_pci_cleanup(pci_dev->pmdata); + if (drv) { if (drv->remove) { pm_runtime_get_sync(dev); diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e120b00a9017..6add5d3209bf 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -585,6 +585,52 @@ static inline bool platform_pci_power_manageable(struct pci_dev *dev) return false; } +void *platform_pci_setup_dev(struct pci_dev *dev) +{ + if (pci_platform_pm && pci_platform_pm->setup_dev) + return pci_platform_pm->setup_dev(dev); + + return NULL; +} + +void *platform_pci_setup_host_bridge(struct pci_host_bridge *bridge) +{ + if (pci_platform_pm && pci_platform_pm->setup_host_bridge) + return pci_platform_pm->setup_host_bridge(bridge); + + return NULL; +} + +void platform_pci_cleanup(void *pmdata) +{ + if (pci_platform_pm && pci_platform_pm->cleanup) + pci_platform_pm->cleanup(pmdata); +} + +static inline bool platform_pci_can_wakeup(void *pmdata) +{ + if (pci_platform_pm && pci_platform_pm->can_wakeup) + return pci_platform_pm->can_wakeup(pmdata); + + return false; +} + +static inline int platform_pci_dev_wakeup(void *pmdata, bool enable) +{ + if (pci_platform_pm && pci_platform_pm->dev_wakeup) + return pci_platform_pm->dev_wakeup(pmdata, enable); + + return -ENODEV; +} + +static inline int platform_pci_bridge_wakeup(void *pmdata, bool enable) +{ + if (pci_platform_pm && pci_platform_pm->bridge_wakeup) + return pci_platform_pm->bridge_wakeup(pmdata, enable); + + return -ENODEV; +} + static inline int platform_pci_set_power_state(struct pci_dev *dev, pci_power_t t) { @@ -610,14 +656,6 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev) return PCI_POWER_ERROR; } -static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable) -{ - if (pci_platform_pm && pci_platform_pm->set_wakeup) - return pci_platform_pm->set_wakeup(dev, enable); - - return -ENODEV; -} - static inline bool platform_pci_need_resume(struct pci_dev *dev) { if (pci_platform_pm && pci_platform_pm->need_resume) @@ -1903,6 +1941,32 @@ void pci_pme_active(struct pci_dev *dev, bool enable) } EXPORT_SYMBOL(pci_pme_active); +static int pci_set_wakeup(struct pci_dev *dev, bool enable) +{ + struct pci_bus *bus = dev->bus; + struct pci_host_bridge *bridge; + + /* Handle per device wakeup */ + if (platform_pci_can_wakeup(dev->pmdata)) + return platform_pci_dev_wakeup(dev->pmdata, enable); + + /* Find a parent which can handle wakeup */ + while (bus->parent) { + if (platform_pci_can_wakeup(bus->self->pmdata)) + return platform_pci_bridge_wakeup(bus->self->pmdata, + enable); + + bus = bus->parent; + } + + /* We have reached the root bus. */ + bridge = to_pci_host_bridge(bus->bridge); + if (platform_pci_can_wakeup(bridge->pmdata)) + return platform_pci_bridge_wakeup(bridge->pmdata, enable); + + return 0; +} + /** * pci_enable_wake - enable PCI device as wakeup event source * @dev: PCI device affected @@ -1950,13 +2014,13 @@ int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable) pci_pme_active(dev, true); else ret = 1; - error = platform_pci_set_wakeup(dev, true); + error = pci_set_wakeup(dev, true); if (ret) ret = error; if (!ret) dev->wakeup_prepared = true; } else { - platform_pci_set_wakeup(dev, false); + pci_set_wakeup(dev, false); pci_pme_active(dev, false); dev->wakeup_prepared = false; } diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 048668271014..dcefb9e759d7 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -47,21 +47,43 @@ int pci_probe_reset_function(struct pci_dev *dev); * platform; to be used during system-wide transitions from a * sleeping state to the working state and vice versa * - * @set_wakeup: enables/disables wakeup capability for the device - * * @need_resume: returns 'true' if the given device (which is currently * suspended) needs to be resumed to be configured for system * wakeup. + * + * @setup_dev: setup device's wakeup ability, alloc and return device's private + * pm data. + * + * @setup_host_bridge: setup host bridge's wakeup ability, alloc and return host + * bridge's private pm data. + * + * @cleanup: cleanup the private pm data and deinit wakeup ability. + * + * @can_wakeup: returns 'true' if given device is capable of waking up the + * system from a sleeping state. + * + * @dev_wakeup: enables/disables wakeup capability for the device. + * + * @bridge_wakeup: enables/disables wakeup capability for the bridge. */ struct pci_platform_pm_ops { bool (*is_manageable)(struct pci_dev *dev); int (*set_state)(struct pci_dev *dev, pci_power_t state); pci_power_t (*get_state)(struct pci_dev *dev); pci_power_t (*choose_state)(struct pci_dev *dev); - int (*set_wakeup)(struct pci_dev *dev, bool enable); bool (*need_resume)(struct pci_dev *dev); + void *(*setup_dev)(struct pci_dev *dev); + void *(*setup_host_bridge)(struct pci_host_bridge *bridge); + void (*cleanup)(void *pmdata); + bool (*can_wakeup)(void *pmdata); + int (*dev_wakeup)(void *pmdata, bool enable); + int (*bridge_wakeup)(void *pmdata, bool enable); }; +void *platform_pci_setup_dev(struct pci_dev *dev); +void *platform_pci_setup_host_bridge(struct pci_host_bridge *bridge); +void platform_pci_cleanup(void *pmdata); + void pci_set_platform_pm(const struct pci_platform_pm_ops *ops); void pci_update_current_state(struct pci_dev *dev, pci_power_t state); void pci_power_up(struct pci_dev *dev); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index cdc2f83c11c5..b12c552a5522 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -809,7 +809,13 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) err = device_register(&bus->dev); if (err) - goto unregister; + goto unregister_bridge; + + bridge->pmdata = platform_pci_setup_host_bridge(bridge); + if (IS_ERR(bridge->pmdata)) { + err = PTR_ERR(bridge->pmdata); + goto unregister_bus; + } pcibios_add_bus(bus); @@ -853,7 +859,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) return 0; -unregister: +unregister_bus: + device_unregister(&bus->dev); +unregister_bridge: put_device(&bridge->dev); device_unregister(&bridge->dev); diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 73a03d382590..d7a8cf1dc69f 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -153,6 +153,8 @@ void pci_remove_root_bus(struct pci_bus *bus) if (!pci_is_root_bus(bus)) return; + platform_pci_cleanup(host_bridge->pmdata); + host_bridge = to_pci_host_bridge(bus->bridge); list_for_each_entry_safe(child, tmp, &bus->devices, bus_list) diff --git a/include/linux/pci.h b/include/linux/pci.h index 80eaa2dbe3e9..628faa58c790 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -293,6 +293,7 @@ struct pci_dev { struct pci_bus *subordinate; /* bus this device bridges to */ void *sysdata; /* hook for sys-specific extension */ + void *pmdata; /* data for platform pm */ struct proc_dir_entry *procent; /* device entry in /proc/bus/pci */ struct pci_slot *slot; /* Physical slot this device is in */ @@ -467,6 +468,7 @@ struct pci_host_bridge { struct pci_bus *bus; /* root bus */ struct pci_ops *ops; void *sysdata; + void *pmdata; /* data for platform pm */ int busnr; struct list_head windows; /* resource_entry */ u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* platform IRQ swizzler */
Move acpi wakeup code to pci core as pci_set_wakeup(), so that other platforms could reuse it. Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm ops's callbacks to setup and cleanup pci devices and host bridge for wakeup. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- Changes in v10: None Changes in v9: None Changes in v8: None Changes in v7: None Changes in v6: None Changes in v5: None Changes in v3: None Changes in v2: None drivers/pci/pci-acpi.c | 121 +++++++++++++++++++++++------------------------ drivers/pci/pci-driver.c | 9 ++++ drivers/pci/pci.c | 84 ++++++++++++++++++++++++++++---- drivers/pci/pci.h | 28 +++++++++-- drivers/pci/probe.c | 12 ++++- drivers/pci/remove.c | 2 + include/linux/pci.h | 2 + 7 files changed, 180 insertions(+), 78 deletions(-)