[{"id":1795157,"web_url":"http://patchwork.ozlabs.org/comment/1795157/","msgid":"<20171027234833.GD105121@google.com>","list_archive_url":null,"date":"2017-10-27T23:48:33","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":67074,"url":"http://patchwork.ozlabs.org/api/people/67074/","name":"Brian Norris","email":"briannorris@chromium.org"},"content":"Hi Jeffy,\n\nOn Fri, Oct 27, 2017 at 03:26:11PM +0800, Jeffy Chen wrote:\n> Move acpi wakeup code to pci core as pci_set_wakeup(), so that other\n> platforms could reuse it.\n\nI think you need to double check your refactoring. I believe you may\nhave changed some behavior here.\n\n> Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm\n> ops's callbacks to setup and cleanup pci devices and host bridge for\n> wakeup.\n> \n> Signed-off-by: Jeffy Chen \n> ---\n> \n> Changes in v10: None\n> Changes in v9: None\n> Changes in v8: None\n> Changes in v7: None\n> Changes in v6: None\n> Changes in v5: None\n> Changes in v3: None\n> Changes in v2: None\n> \n> drivers/pci/pci-acpi.c | 121 +++++++++++++++++++++++------------------------\n> drivers/pci/pci-driver.c | 9 ++++\n> drivers/pci/pci.c | 84 ++++++++++++++++++++++++++++----\n> drivers/pci/pci.h | 28 +++++++++--\n> drivers/pci/probe.c | 12 ++++-\n> drivers/pci/remove.c | 2 +\n> include/linux/pci.h | 2 +\n> 7 files changed, 180 insertions(+), 78 deletions(-)\n> \n> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c\n> index 4708eb9df71b..ee96e7afe1ac 100644\n> --- a/drivers/pci/pci-acpi.c\n> +++ b/drivers/pci/pci-acpi.c\n> @@ -569,31 +569,6 @@ static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)\n> \treturn state_conv[state];\n> }\n> \n> -static int acpi_pci_propagate_wakeup(struct pci_bus *bus, bool enable)\n> -{\n> -\twhile (bus->parent) {\n> -\t\tif (acpi_pm_device_can_wakeup(&bus->self->dev))\n> -\t\t\treturn acpi_pm_set_bridge_wakeup(&bus->self->dev, enable);\n> -\n> -\t\tbus = bus->parent;\n> -\t}\n> -\n> -\t/* We have reached the root bus. */\n> -\tif (bus->bridge) {\n> -\t\tif (acpi_pm_device_can_wakeup(bus->bridge))\n> -\t\t\treturn acpi_pm_set_bridge_wakeup(bus->bridge, enable);\n> -\t}\n> -\treturn 0;\n> -}\n> -\n> -static int acpi_pci_wakeup(struct pci_dev *dev, bool enable)\n> -{\n> -\tif (acpi_pm_device_can_wakeup(&dev->dev))\n> -\t\treturn acpi_pm_set_device_wakeup(&dev->dev, enable);\n> -\n> -\treturn acpi_pci_propagate_wakeup(dev->bus, enable);\n> -}\n> -\n> static bool acpi_pci_need_resume(struct pci_dev *dev)\n> {\n> \tstruct acpi_device *adev = ACPI_COMPANION(&dev->dev);\n> @@ -610,14 +585,29 @@ static bool acpi_pci_need_resume(struct pci_dev *dev)\n> \treturn !!adev->power.flags.dsw_present;\n> }\n> \n> -static const struct pci_platform_pm_ops acpi_pci_platform_pm = {\n> -\t.is_manageable = acpi_pci_power_manageable,\n> -\t.set_state = acpi_pci_set_power_state,\n> -\t.get_state = acpi_pci_get_power_state,\n> -\t.choose_state = acpi_pci_choose_state,\n> -\t.set_wakeup = acpi_pci_wakeup,\n> -\t.need_resume = acpi_pci_need_resume,\n> -};\n> +static bool acpi_pci_can_wakeup(void *pmdata)\n> +{\n> +\tstruct device *dev = pmdata;\n> +\n> +\tif (!dev)\n> +\t\treturn false;\n> +\n> +\treturn acpi_pm_device_can_wakeup(dev);\n> +}\n> +\n> +static int acpi_pci_dev_wakeup(void *pmdata, bool enable)\n> +{\n> +\tstruct device *dev = pmdata;\n> +\n> +\treturn acpi_pm_set_device_wakeup(dev, enable);\n> +}\n> +\n> +static int acpi_pci_bridge_wakeup(void *pmdata, bool enable)\n> +{\n> +\tstruct device *dev = pmdata;\n> +\n> +\treturn acpi_pm_set_bridge_wakeup(dev, enable);\n> +}\n> \n> void acpi_pci_add_bus(struct pci_bus *bus)\n> {\n> @@ -658,20 +648,6 @@ void acpi_pci_remove_bus(struct pci_bus *bus)\n> \tacpi_pci_slot_remove(bus);\n> }\n> \n> -/* ACPI bus type */\n> -static struct acpi_device *acpi_pci_find_companion(struct device *dev)\n> -{\n> -\tstruct pci_dev *pci_dev = to_pci_dev(dev);\n> -\tbool check_children;\n> -\tu64 addr;\n> -\n> -\tcheck_children = pci_is_bridge(pci_dev);\n> -\t/* Please ref to ACPI spec for the syntax of _ADR */\n> -\taddr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);\n> -\treturn acpi_find_child_device(ACPI_COMPANION(dev->parent), addr,\n> -\t\t\t\t check_children);\n> -}\n> -\n> /**\n> * pci_acpi_optimize_delay - optimize PCI D3 and D3cold delay from ACPI\n> * @pdev: the PCI device whose delay is to be updated\n> @@ -723,34 +699,55 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,\n> \tACPI_FREE(obj);\n> }\n> \n> -static void pci_acpi_setup(struct device *dev)\n> +static void *acpi_pci_setup_dev(struct pci_dev *pci_dev)\n> {\n> -\tstruct pci_dev *pci_dev = to_pci_dev(dev);\n> -\tstruct acpi_device *adev = ACPI_COMPANION(dev);\n> +\tstruct acpi_device *adev = ACPI_COMPANION(&pci_dev->dev);\n> \n> \tif (!adev)\n> -\t\treturn;\n> +\t\treturn NULL;\n> \n> \tpci_acpi_optimize_delay(pci_dev, adev->handle);\n> \n> \tpci_acpi_add_pm_notifier(adev, pci_dev);\n> \tif (!adev->wakeup.flags.valid)\n> -\t\treturn;\n> +\t\treturn NULL;\n> +\n> +\tdevice_set_wakeup_capable(&pci_dev->dev, true);\n> +\tacpi_pm_set_device_wakeup(&pci_dev->dev, false);\n> \n> -\tdevice_set_wakeup_capable(dev, true);\n> -\tacpi_pci_wakeup(pci_dev, false);\n> +\treturn &pci_dev->dev;\n> }\n> \n> -static void pci_acpi_cleanup(struct device *dev)\n> +static void *acpi_pci_setup_host_bridge(struct pci_host_bridge *bridge)\n> {\n> -\tstruct acpi_device *adev = ACPI_COMPANION(dev);\n> +\treturn &bridge->dev;\n> +}\n> \n> -\tif (!adev)\n> -\t\treturn;\n> +static const struct pci_platform_pm_ops acpi_pci_platform_pm = {\n> +\t.is_manageable = acpi_pci_power_manageable,\n> +\t.set_state = acpi_pci_set_power_state,\n> +\t.get_state = acpi_pci_get_power_state,\n> +\t.choose_state = acpi_pci_choose_state,\n> +\t.need_resume = acpi_pci_need_resume,\n> +\t.setup_dev = acpi_pci_setup_dev,\n> +\t.setup_host_bridge = acpi_pci_setup_host_bridge,\n> +\t.can_wakeup = acpi_pci_can_wakeup,\n> +\t.dev_wakeup = acpi_pci_dev_wakeup,\n> +\t.bridge_wakeup = acpi_pci_bridge_wakeup,\n> +};\n> +\n> +/* ACPI bus type */\n> +static struct acpi_device *acpi_pci_find_companion(struct device *dev)\n> +{\n> +\tstruct pci_dev *pci_dev = to_pci_dev(dev);\n> +\tbool check_children;\n> +\tu64 addr;\n> \n> -\tpci_acpi_remove_pm_notifier(adev);\n> -\tif (adev->wakeup.flags.valid)\n> -\t\tdevice_set_wakeup_capable(dev, false);\n> +\tcheck_children = pci_is_bridge(pci_dev);\n> +\t/* Please ref to ACPI spec for the syntax of _ADR */\n> +\taddr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);\n> +\treturn acpi_find_child_device(ACPI_COMPANION(dev->parent), addr,\n> +\t\t\t\t check_children);\n> }\n> \n> static bool pci_acpi_bus_match(struct device *dev)\n> @@ -762,8 +759,6 @@ static struct acpi_bus_type acpi_pci_bus = {\n> \t.name = \"PCI\",\n> \t.match = pci_acpi_bus_match,\n> \t.find_companion = acpi_pci_find_companion,\n> -\t.setup = pci_acpi_setup,\n> -\t.cleanup = pci_acpi_cleanup,\n> };\n> \n> \n> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c\n> index 9be563067c0c..abb7caa8a6e5 100644\n> --- a/drivers/pci/pci-driver.c\n> +++ b/drivers/pci/pci-driver.c\n> @@ -421,10 +421,17 @@ static int pci_device_probe(struct device *dev)\n> \tif (error < 0)\n> \t\treturn error;\n> \n> +\tpci_dev->pmdata = platform_pci_setup_dev(pci_dev);\n> +\tif (IS_ERR(pci_dev->pmdata)) {\n> +\t\tpcibios_free_irq(pci_dev);\n> +\t\treturn PTR_ERR(pci_dev->pmdata);\n> +\t}\n> +\n> \tpci_dev_get(pci_dev);\n> \tif (pci_device_can_probe(pci_dev)) {\n> \t\terror = __pci_device_probe(drv, pci_dev);\n> \t\tif (error) {\n> +\t\t\tplatform_pci_cleanup(pci_dev->pmdata);\n> \t\t\tpcibios_free_irq(pci_dev);\n> \t\t\tpci_dev_put(pci_dev);\n> \t\t}\n> @@ -438,6 +445,8 @@ static int pci_device_remove(struct device *dev)\n> \tstruct pci_dev *pci_dev = to_pci_dev(dev);\n> \tstruct pci_driver *drv = pci_dev->driver;\n> \n> +\tplatform_pci_cleanup(pci_dev->pmdata);\n> +\n> \tif (drv) {\n> \t\tif (drv->remove) {\n> \t\t\tpm_runtime_get_sync(dev);\n> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c\n> index e120b00a9017..6add5d3209bf 100644\n> --- a/drivers/pci/pci.c\n> +++ b/drivers/pci/pci.c\n> @@ -585,6 +585,52 @@ static inline bool platform_pci_power_manageable(struct pci_dev *dev)\n> \treturn false;\n> }\n> \n> +void *platform_pci_setup_dev(struct pci_dev *dev)\n> +{\n> +\tif (pci_platform_pm && pci_platform_pm->setup_dev)\n> +\t\treturn pci_platform_pm->setup_dev(dev);\n> +\n> +\treturn NULL;\n> +}\n> +\n> +void *platform_pci_setup_host_bridge(struct pci_host_bridge *bridge)\n> +{\n> +\tif (pci_platform_pm && pci_platform_pm->setup_host_bridge)\n> +\t\treturn pci_platform_pm->setup_host_bridge(bridge);\n> +\n> +\treturn NULL;\n> +}\n> +\n> +void platform_pci_cleanup(void *pmdata)\n> +{\n> +\tif (pci_platform_pm && pci_platform_pm->cleanup)\n> +\t\tpci_platform_pm->cleanup(pmdata);\n> +}\n> +\n> +static inline bool platform_pci_can_wakeup(void *pmdata)\n> +{\n> +\tif (pci_platform_pm && pci_platform_pm->can_wakeup)\n> +\t\treturn pci_platform_pm->can_wakeup(pmdata);\n> +\n> +\treturn false;\n> +}\n> +\n> +static inline int platform_pci_dev_wakeup(void *pmdata, bool enable)\n> +{\n> +\tif (pci_platform_pm && pci_platform_pm->dev_wakeup)\n> +\t\treturn pci_platform_pm->dev_wakeup(pmdata, enable);\n> +\n> +\treturn -ENODEV;\n> +}\n> +\n> +static inline int platform_pci_bridge_wakeup(void *pmdata, bool enable)\n> +{\n> +\tif (pci_platform_pm && pci_platform_pm->bridge_wakeup)\n> +\t\treturn pci_platform_pm->bridge_wakeup(pmdata, enable);\n> +\n> +\treturn -ENODEV;\n> +}\n> +\n> static inline int platform_pci_set_power_state(struct pci_dev *dev,\n> \t\t\t\t\t pci_power_t t)\n> {\n> @@ -610,14 +656,6 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)\n> \treturn PCI_POWER_ERROR;\n> }\n> \n> -static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)\n> -{\n> -\tif (pci_platform_pm && pci_platform_pm->set_wakeup)\n> -\t\treturn pci_platform_pm->set_wakeup(dev, enable);\n> -\n> -\treturn -ENODEV;\n\nSo this function used to return -ENODEV if the platform didn't implement\nPM. Now you're transitioning to pci_set_wakeup() below:\n\n> -}\n> -\n> static inline bool platform_pci_need_resume(struct pci_dev *dev)\n> {\n> \tif (pci_platform_pm && pci_platform_pm->need_resume)\n> @@ -1903,6 +1941,32 @@ void pci_pme_active(struct pci_dev *dev, bool enable)\n> }\n> EXPORT_SYMBOL(pci_pme_active);\n> \n> +static int pci_set_wakeup(struct pci_dev *dev, bool enable)\n> +{\n> +\tstruct pci_bus *bus = dev->bus;\n> +\tstruct pci_host_bridge *bridge;\n> +\n> +\t/* Handle per device wakeup */\n> +\tif (platform_pci_can_wakeup(dev->pmdata))\n> +\t\treturn platform_pci_dev_wakeup(dev->pmdata, enable);\n> +\n> +\t/* Find a parent which can handle wakeup */\n> +\twhile (bus->parent) {\n> +\t\tif (platform_pci_can_wakeup(bus->self->pmdata))\n> +\t\t\treturn platform_pci_bridge_wakeup(bus->self->pmdata,\n> +\t\t\t\t\t\t\t enable);\n> +\n> +\t\tbus = bus->parent;\n> +\t}\n> +\n> +\t/* We have reached the root bus. */\n> +\tbridge = to_pci_host_bridge(bus->bridge);\n> +\tif (platform_pci_can_wakeup(bridge->pmdata))\n> +\t\treturn platform_pci_bridge_wakeup(bridge->pmdata, enable);\n> +\n> +\treturn 0;\n\nAnd it looks like the fallback behavior is just 'return 0', if none of\nthe devices supported wakeup. It's weird that this already used to\nhappen for ACPI -- if no device/bridge implemented wakeup, we still\nreturned success. But now you're making this happen for platforms\nwithout PM operations too.\n\nThis affects the behavior of pci_enable_wake below, which tries to\nreturn an error if both PME and \"platform wake\" failed.\n\nI wonder if this should just return an error code if we reached the end\n(i.e., no device or bridge supported wakeup). This would technically\nchange the ACPI behavior, but then I think it would also be more\ncorrect.\n\nIf you do this, that should probably be a separate patch, to be clear\nwe're changing the error logic before we refactor.\n\n> +}\n> +\n> /**\n> * pci_enable_wake - enable PCI device as wakeup event source\n> * @dev: PCI device affected\n> @@ -1950,13 +2014,13 @@ int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable)\n> \t\t\tpci_pme_active(dev, true);\n> \t\telse\n> \t\t\tret = 1;\n> -\t\terror = platform_pci_set_wakeup(dev, true);\n> +\t\terror = pci_set_wakeup(dev, true);\n> \t\tif (ret)\n> \t\t\tret = error;\n> \t\tif (!ret)\n> \t\t\tdev->wakeup_prepared = true;\n> \t} else {\n> -\t\tplatform_pci_set_wakeup(dev, false);\n> +\t\tpci_set_wakeup(dev, false);\n> \t\tpci_pme_active(dev, false);\n> \t\tdev->wakeup_prepared = false;\n> \t}\n> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h\n> index 048668271014..dcefb9e759d7 100644\n> --- a/drivers/pci/pci.h\n> +++ b/drivers/pci/pci.h\n> @@ -47,21 +47,43 @@ int pci_probe_reset_function(struct pci_dev *dev);\n> * platform; to be used during system-wide transitions from a\n> * sleeping state to the working state and vice versa\n> *\n> - * @set_wakeup: enables/disables wakeup capability for the device\n> - *\n> * @need_resume: returns 'true' if the given device (which is currently\n> *\t\tsuspended) needs to be resumed to be configured for system\n> *\t\twakeup.\n> + *\n> + * @setup_dev: setup device's wakeup ability, alloc and return device's private\n> + * \t\tpm data.\n> + *\n> + * @setup_host_bridge: setup host bridge's wakeup ability, alloc and return host\n> + * \t\tbridge's private pm data.\n> + *\n> + * @cleanup: cleanup the private pm data and deinit wakeup ability.\n> + *\n> + * @can_wakeup: returns 'true' if given device is capable of waking up the\n> + *\t\tsystem from a sleeping state.\n> + *\n> + * @dev_wakeup: enables/disables wakeup capability for the device.\n> + *\n> + * @bridge_wakeup: enables/disables wakeup capability for the bridge.\n> */\n> struct pci_platform_pm_ops {\n> \tbool (*is_manageable)(struct pci_dev *dev);\n> \tint (*set_state)(struct pci_dev *dev, pci_power_t state);\n> \tpci_power_t (*get_state)(struct pci_dev *dev);\n> \tpci_power_t (*choose_state)(struct pci_dev *dev);\n> -\tint (*set_wakeup)(struct pci_dev *dev, bool enable);\n\nI believe you're breaking pci-mid.c, which still assigns a '.set_wakeup'\ncallback.\n\n> \tbool (*need_resume)(struct pci_dev *dev);\n> +\tvoid *(*setup_dev)(struct pci_dev *dev);\n> +\tvoid *(*setup_host_bridge)(struct pci_host_bridge *bridge);\n> +\tvoid (*cleanup)(void *pmdata);\n> +\tbool (*can_wakeup)(void *pmdata);\n> +\tint (*dev_wakeup)(void *pmdata, bool enable);\n> +\tint (*bridge_wakeup)(void *pmdata, bool enable);\n\nYou're splitting the \"set_wakeup\" callback into a \"dev_wakeup\" and a\n\"bridge_wakeup\" function, but you're losing the \"set\" terminology, which\nI think makes things clearer here. So, maybe \"set_dev_wakeup\" and\n\"set_bridge_wakeup\"? And same for the corresponding platform_pci_*()\nhelpers.\n\nBrian\n\n> };\n> \n> +void *platform_pci_setup_dev(struct pci_dev *dev);\n> +void *platform_pci_setup_host_bridge(struct pci_host_bridge *bridge);\n> +void platform_pci_cleanup(void *pmdata);\n> +\n> void pci_set_platform_pm(const struct pci_platform_pm_ops *ops);\n> void pci_update_current_state(struct pci_dev *dev, pci_power_t state);\n> void pci_power_up(struct pci_dev *dev);\n> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c\n> index cdc2f83c11c5..b12c552a5522 100644\n> --- a/drivers/pci/probe.c\n> +++ b/drivers/pci/probe.c\n> @@ -809,7 +809,13 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)\n> \n> \terr = device_register(&bus->dev);\n> \tif (err)\n> -\t\tgoto unregister;\n> +\t\tgoto unregister_bridge;\n> +\n> +\tbridge->pmdata = platform_pci_setup_host_bridge(bridge);\n> +\tif (IS_ERR(bridge->pmdata)) {\n> +\t\terr = PTR_ERR(bridge->pmdata);\n> +\t\tgoto unregister_bus;\n> +\t}\n> \n> \tpcibios_add_bus(bus);\n> \n> @@ -853,7 +859,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)\n> \n> \treturn 0;\n> \n> -unregister:\n> +unregister_bus:\n> +\tdevice_unregister(&bus->dev);\n> +unregister_bridge:\n> \tput_device(&bridge->dev);\n> \tdevice_unregister(&bridge->dev);\n> \n> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c\n> index 73a03d382590..d7a8cf1dc69f 100644\n> --- a/drivers/pci/remove.c\n> +++ b/drivers/pci/remove.c\n> @@ -153,6 +153,8 @@ void pci_remove_root_bus(struct pci_bus *bus)\n> \tif (!pci_is_root_bus(bus))\n> \t\treturn;\n> \n> +\tplatform_pci_cleanup(host_bridge->pmdata);\n> +\n> \thost_bridge = to_pci_host_bridge(bus->bridge);\n> \tlist_for_each_entry_safe(child, tmp,\n> \t\t\t\t &bus->devices, bus_list)\n> diff --git a/include/linux/pci.h b/include/linux/pci.h\n> index 80eaa2dbe3e9..628faa58c790 100644\n> --- a/include/linux/pci.h\n> +++ b/include/linux/pci.h\n> @@ -293,6 +293,7 @@ struct pci_dev {\n> \tstruct pci_bus\t*subordinate;\t/* bus this device bridges to */\n> \n> \tvoid\t\t*sysdata;\t/* hook for sys-specific extension */\n> +\tvoid\t\t*pmdata;\t/* data for platform pm */\n> \tstruct proc_dir_entry *procent;\t/* device entry in /proc/bus/pci */\n> \tstruct pci_slot\t*slot;\t\t/* Physical slot this device is in */\n> \n> @@ -467,6 +468,7 @@ struct pci_host_bridge {\n> \tstruct pci_bus *bus;\t\t/* root bus */\n> \tstruct pci_ops *ops;\n> \tvoid *sysdata;\n> +\tvoid *pmdata;\t\t\t/* data for platform pm */\n> \tint busnr;\n> \tstruct list_head windows;\t/* resource_entry */\n> \tu8 (*swizzle_irq)(struct pci_dev *, u8 *); /* platform IRQ swizzler */\n> -- \n> 2.11.0\n> \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"FnIAMZPR\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yP0vF5ctGz9t4X\n\tfor ;\n\tSat, 28 Oct 2017 10:48:41 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932387AbdJ0Xsj (ORCPT );\n\tFri, 27 Oct 2017 19:48:39 -0400","from mail-io0-f193.google.com ([209.85.223.193]:52940 \"EHLO\n\tmail-io0-f193.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S932301AbdJ0Xsi (ORCPT\n\t); Fri, 27 Oct 2017 19:48:38 -0400","by mail-io0-f193.google.com with SMTP id f20so15995515ioj.9\n\tfor ; Fri, 27 Oct 2017 16:48:37 -0700 (PDT)","from google.com ([2620:0:1000:1600:822:58a4:e85c:5b35])\n\tby smtp.gmail.com with ESMTPSA id\n\ti130sm1411761itf.42.2017.10.27.16.48.35\n\t(version=TLS1_2 cipher=AES128-SHA bits=128/128);\n\tFri, 27 Oct 2017 16:48:36 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=hhfN+xfeB27Mbvn05KaOKqDJ40wgCX407FAaZI0DW7E=;\n\tb=FnIAMZPRoMy7mRDsi9Ig9o6/u+Rxbc/J4ApGr9BjBD5pW6GOIzQHCd/vN4IsgzsH4o\n\t6U4P79Gu45Owe6pG4VayyioNlLDdBZW675z+Fj3k8mu+LnhR6LbrUhrwwzrXymQSL1au\n\tWnnGIBZmJmIGreKtF1NKiut56QdQpiDWBQlls=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=hhfN+xfeB27Mbvn05KaOKqDJ40wgCX407FAaZI0DW7E=;\n\tb=j+wnxL4w4vcJ+ilsM/BNExxBpt3pYHI/U4DC24m5eVP1V90H3Vy6Uy/fs2BUmpyAxr\n\toA28IoJoYyUm1rw9E49Jgr4ltgesnxJaG7/sd/kg/6jjcuM6Y/ExMMcg1d7pUvaHjchA\n\t5LbCbJCQf7ZySK7GRMutZvMbtGhx8HW2ykDwJe4kKVUyDbTGiHqKzn5GX7bBScRg2rHX\n\tA0yh/GsGhTLix7vZdu4AXlAPcXFGYjlzofDPcb+x8g/08t9BiUqvm7CkRzunAdY2yqVw\n\t/AmQjChKVoe+XrXSOiqGyZaLZKa20gM6fxzlJ9xSqrDqNmrOD00lh/utBGcAlAMsX5LX\n\toADA==","X-Gm-Message-State":"AMCzsaWLTSCJ4fl8QiMMEY3rqEUJ4UbRvGho5jNhe+CWoI5NnR3YRaiJ\n\toLgwBWjKK1pOVn53By2hQ/g2bw==","X-Google-Smtp-Source":"ABhQp+RNsBNrs7pP0R1uAJsQmCbMSZ25T5OKtiSTFxaqe8TYW3lwHfFpJiyNS1K7t/EaBr9uoKJ5pw==","X-Received":"by 10.36.88.137 with SMTP id f131mr2684216itb.4.1509148117141;\n\tFri, 27 Oct 2017 16:48:37 -0700 (PDT)","Date":"Fri, 27 Oct 2017 16:48:33 -0700","From":"Brian Norris ","To":"Jeffy Chen ","Cc":"linux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, tony@atomide.com,\n\tshawn.lin@rock-chips.com, rjw@rjwysocki.net, dianders@chromium.org,\n\tlinux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,\n\tLen Brown ","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","Message-ID":"<20171027234833.GD105121@google.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<20171027072612.26565-7-jeffy.chen@rock-chips.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171027072612.26565-7-jeffy.chen@rock-chips.com>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1801754,"web_url":"http://patchwork.ozlabs.org/comment/1801754/","msgid":"<1894178.xtK0vD2N4H@aspire.rjw.lan>","list_archive_url":null,"date":"2017-11-08T22:32:20","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote:\n> Move acpi wakeup code to pci core as pci_set_wakeup(), so that other\n> platforms could reuse it.\n\nWhat exactly do you want to reuse?\n\nIt looks like that's just several lines of code in acpi_pci_wakeup()\nand acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level\nfunctions, so IMO not worth it at all.\n\nThe structure for other platform code may be the same or similar, but\nthe details will almost certainly be different and I don't think that\nhaving more callback pointers in pci_platform_pm_ops is necessarily better.\n\n> Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm\n> ops's callbacks to setup and cleanup pci devices and host bridge for\n> wakeup.\n\nWhy are they needed?\n\n> Signed-off-by: Jeffy Chen \n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yXLdn5zzwz9s0Z\n\tfor ;\n\tThu, 9 Nov 2017 09:32:29 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752906AbdKHWc1 (ORCPT );\n\tWed, 8 Nov 2017 17:32:27 -0500","from cloudserver094114.home.net.pl ([79.96.170.134]:58600 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1752900AbdKHWc0 (ORCPT\n\t); Wed, 8 Nov 2017 17:32:26 -0500","from 79.184.254.73.ipv4.supernova.orange.pl (79.184.254.73) (HELO\n\taspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 5f9b6d103d68e131; Wed, 8 Nov 2017 23:32:24 +0100"],"From":"\"Rafael J. Wysocki\" ","To":"Jeffy Chen ","Cc":"linux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, tony@atomide.com,\n\tshawn.lin@rock-chips.com, briannorris@chromium.org,\n\tdianders@chromium.org, linux-pci@vger.kernel.org,\n\tlinux-acpi@vger.kernel.org, Len Brown ","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","Date":"Wed, 08 Nov 2017 23:32:20 +0100","Message-ID":"<1894178.xtK0vD2N4H@aspire.rjw.lan>","In-Reply-To":"<20171027072612.26565-7-jeffy.chen@rock-chips.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<20171027072612.26565-7-jeffy.chen@rock-chips.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1804060,"web_url":"http://patchwork.ozlabs.org/comment/1804060/","msgid":"<20171114025109.GA43048@google.com>","list_archive_url":null,"date":"2017-11-14T02:51:11","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":67074,"url":"http://patchwork.ozlabs.org/api/people/67074/","name":"Brian Norris","email":"briannorris@chromium.org"},"content":"Hi Rafael,\n\nI'll answer some of it from my perspective, though Jeffy might have had\ndifferent ideas (and answers) when he implemented this.\n\nOn Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote:\n> On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote:\n> > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other\n> > platforms could reuse it.\n> \n> What exactly do you want to reuse?\n> \n> It looks like that's just several lines of code in acpi_pci_wakeup()\n> and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level\n> functions, so IMO not worth it at all.\n\nThe important part he's sharing here is the walking of the tree\nstructure, in which it's possible for some parent along the way to\nhandle wakeup for its children. I'm not sure how valuable nor how\nreusable that is.\n\nIn this case (the Rockchip platforms Jeffy and I are working on), I\nthink we really want to just support a single WAKE# pin for the whole\nsystem, so maybe the complexity isn't needed. The spec does describe\nthat there are good reasons for supporting more than 1 WAKE# pin though\n(e.g., 1 per device), so it doesn't seem really wise to shoehorn\noursleves into a single setup.\n\nBut that can be implemented either via copying the \"few\" lines of\ntree-walking logic, or by trying to share them.\n\n> The structure for other platform code may be the same or similar, but\n> the details will almost certainly be different and I don't think that\n> having more callback pointers in pci_platform_pm_ops is necessarily better.\n\nI suppose that's reasonable.\n\n> > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm\n> > ops's callbacks to setup and cleanup pci devices and host bridge for\n> > wakeup.\n> \n> Why are they needed?\n\nThe implementation is in patch 7, if you really needed more info about\nwhy, or provide alternatives.\n\nThe current set of hooks assumes that there is no state information or\ninitialization needed for tracking actions of these platform PM hooks on\na device. For ACPI this works, because devices have \"companion\"\nacpi_dev's to handle everything, and the ACPI framework generally\nprepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be\ntrivial (and arguably wrong -- several are no-ops, where we might expect\nthe platform to tell us whether the operation was actually supported or\nnot).\n\nFor device tree, there isn't really a canonical place to store this\ninformation, nor to initialize something like wakeup interrupts.\n\nTechnically, we could shoehorn this into the .set_wakeup() call, but\nwe'd probably rather not do things like request_irq() on every attempt\nto suspend/resume the system (among other reasons, we'd lose information\nthat we might otherwise track in /proc/ or /sys/).\n\nOr the inverse of the above: where would you suggest initializing or\ntearing down the wakeirq?\n\nAn alternative could be to include any necessary state into the\npci_host_bridge or pci_dev and just inline the setup code into\npci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c\n(pci_device_{probe,remove}()). But I'm not sure that's much more\nbeautiful.\n\nBrian\n\n> > Signed-off-by: Jeffy Chen \n> \n> Thanks,\n> Rafael\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"d+yrAa+C\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3ybX871y9kz9s7C\n\tfor ;\n\tTue, 14 Nov 2017 13:51:19 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752371AbdKNCvR (ORCPT );\n\tMon, 13 Nov 2017 21:51:17 -0500","from mail-io0-f196.google.com ([209.85.223.196]:52718 \"EHLO\n\tmail-io0-f196.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751182AbdKNCvP (ORCPT\n\t); Mon, 13 Nov 2017 21:51:15 -0500","by mail-io0-f196.google.com with SMTP id u42so4865524ioi.9\n\tfor ; Mon, 13 Nov 2017 18:51:14 -0800 (PST)","from google.com ([2620:0:1000:1600:c580:5aaf:aae5:5a6e])\n\tby smtp.gmail.com with ESMTPSA id\n\td14sm2500661itj.11.2017.11.13.18.51.13\n\t(version=TLS1_2 cipher=AES128-SHA bits=128/128);\n\tMon, 13 Nov 2017 18:51:13 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=w/9RN3vWqCIPbSNwW9t30W9CKWG4zwnYIQZO4O9V/Io=;\n\tb=d+yrAa+CClWKdbqEZLlLrIrswwdBvZ3MPGddxIM2Qk/eZPeTvvwXuYc9zJKij8qL+j\n\tzoVfY/15svWTu8/At8iZTI+aib0e+uKBN9uvsYiLY2ma9uWgjG9H8iaAty71SEzSxbDE\n\tEd4tgjA4ms9ZTN7AKdsgGyCkb9Mi4zrGsRRXk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=w/9RN3vWqCIPbSNwW9t30W9CKWG4zwnYIQZO4O9V/Io=;\n\tb=E10U3auok3/SJa7QcaXSpWVGX5Gzk/EucdBnQukT6i+mdLsMBpMH7ynWyLgJr7ihWp\n\tZ3XxJcbg/NJxD2ljvbo6+vi2NYI8KqS0Ysn8s6AbLen4/u3uslAQK1PM9u7v89llq38t\n\txPJq6QZl6v0oQlRpPdmPkDMRs/guyoR+ca8/flB1Xe/r5tEh8ajZgfw2HiLQI/c9riPN\n\tBkEFmqlPJJj7+fyesqn1TkI5znIw+ARh158nkNywdBK1w7dzi54iuj/cO+yUikkU0rcA\n\tmFhxm2ip2Q2fEleKvKmN9paUo3oE5fBHFQmqiG+KlGXvkxH1Glgqi7MfxGiDQKazuUfF\n\tSLfw==","X-Gm-Message-State":"AJaThX4lLeeDk5ab/8TbshuDPNLiFdBHKItu+X1yD30oMDhrK9N3CpCe\n\txMNPl67KcJXTfDX6p6Osb+4PWw==","X-Google-Smtp-Source":"AGs4zMZgq8TT7AK+obV/zGLzpzCYlVuc+IzBbYFjfljMatC2VHbS+gXd1t7yjyNzX5WuuBc56dt+iw==","X-Received":"by 10.107.43.75 with SMTP id r72mr13091637ior.31.1510627874522; \n\tMon, 13 Nov 2017 18:51:14 -0800 (PST)","Date":"Mon, 13 Nov 2017 18:51:11 -0800","From":"Brian Norris ","To":"\"Rafael J. Wysocki\" ","Cc":"Jeffy Chen ,\n\tlinux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, tony@atomide.com,\n\tshawn.lin@rock-chips.com, dianders@chromium.org,\n\tlinux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,\n\tLen Brown ","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","Message-ID":"<20171114025109.GA43048@google.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<20171027072612.26565-7-jeffy.chen@rock-chips.com>\n\t<1894178.xtK0vD2N4H@aspire.rjw.lan>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1894178.xtK0vD2N4H@aspire.rjw.lan>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1808445,"web_url":"http://patchwork.ozlabs.org/comment/1808445/","msgid":"<1882670.s3LR2t44nD@aspire.rjw.lan>","list_archive_url":null,"date":"2017-11-22T00:26:02","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Tuesday, November 14, 2017 3:51:11 AM CET Brian Norris wrote:\n> Hi Rafael,\n> \n> I'll answer some of it from my perspective, though Jeffy might have had\n> different ideas (and answers) when he implemented this.\n> \n> On Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote:\n> > On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote:\n> > > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other\n> > > platforms could reuse it.\n> > \n> > What exactly do you want to reuse?\n> > \n> > It looks like that's just several lines of code in acpi_pci_wakeup()\n> > and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level\n> > functions, so IMO not worth it at all.\n> \n> The important part he's sharing here is the walking of the tree\n> structure, in which it's possible for some parent along the way to\n> handle wakeup for its children. I'm not sure how valuable nor how\n> reusable that is.\n\nACPI very well may be the only case needing to walk the hierarchy for that.\n\n> In this case (the Rockchip platforms Jeffy and I are working on), I\n> think we really want to just support a single WAKE# pin for the whole\n> system, so maybe the complexity isn't needed.\n\nSo unless I know you'll need it, please don't add it.\n\n> The spec does describe that there are good reasons for supporting more than\n> 1 WAKE# pin though (e.g., 1 per device), so it doesn't seem really wise to\n> shoehorn oursleves into a single setup.\n\nOne WAKE# pin per device is reasonable enough, but WAKE# is specifically\ndefined as out-of-band and \"orthogonal\" to the PCIe hierarchy. What you\nneed is a way to program WAKE# and (possibly) wakeup power on the given\nplatform for each device having a WAKE# pin individually.\n \nThe main reason why ACPI needs to walk the hierarchy is that on some systems\nthe firmware takes over the handling of the native PME mechanism which then is\ntaken care of by the AML (and the kernel is not granted control of the\nrelevant PCIe registers). I don't think you'll ever see anything like that\non non-ACPI systems.\n\n> But that can be implemented either via copying the \"few\" lines of\n> tree-walking logic, or by trying to share them.\n> \n> > The structure for other platform code may be the same or similar, but\n> > the details will almost certainly be different and I don't think that\n> > having more callback pointers in pci_platform_pm_ops is necessarily better.\n> \n> I suppose that's reasonable.\n> \n> > > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm\n> > > ops's callbacks to setup and cleanup pci devices and host bridge for\n> > > wakeup.\n> > \n> > Why are they needed?\n> \n> The implementation is in patch 7, if you really needed more info about\n> why, or provide alternatives.\n\nWell, that's quite questionable.\n\nAt least defining ->dev_wakeup to do device_set_wakeup_capable() doesn't\nreally make sense to me.\n\n> The current set of hooks assumes that there is no state information or\n> initialization needed for tracking actions of these platform PM hooks on\n> a device. For ACPI this works, because devices have \"companion\"\n> acpi_dev's to handle everything, and the ACPI framework generally\n> prepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be\n> trivial (and arguably wrong -- several are no-ops, where we might expect\n> the platform to tell us whether the operation was actually supported or\n> not).\n> \n> For device tree, there isn't really a canonical place to store this\n> information, nor to initialize something like wakeup interrupts.\n\nThe only thing you need IMO is ->set_wakeup which also is what ACPI needs\nand that enables or disables wakeup for the device. It doesn't actually\nhave to track anything (other than, possibly, whether or not wakeup has\nbeen enabled for it already).\n\nAnd since the core takes care of native PMEs for you, the only thing\nthis needs to cover is the WAKE# pin programming AFAICS.\n\nThe setup part, in turn, in the ACPI case is done via acpi_platform_notify()\nand, analogously, acpi_platform_notify_remove() does the cleanup. You seem\nto be trying to add something like it via pci_platform_pm_ops, but is it\nreally the most suitable place for that?\n\n> Technically, we could shoehorn this into the .set_wakeup() call, but\n> we'd probably rather not do things like request_irq() on every attempt\n> to suspend/resume the system (among other reasons, we'd lose information\n> that we might otherwise track in /proc/ or /sys/).\n> \n> Or the inverse of the above: where would you suggest initializing or\n> tearing down the wakeirq?\n\nAgain, ACPI does that via acpi_platform_notify()/acpi_platform_notify_remove(),\nso maybe something similar?\n\nIn any case, you need a way to associate DT-provided data with PCI devices and,\nideally, that should be done at the enumeration time.\n\n> An alternative could be to include any necessary state into the\n> pci_host_bridge or pci_dev and just inline the setup code into\n> pci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c\n> (pci_device_{probe,remove}()). But I'm not sure that's much more\n> beautiful.\n\nWell, that's not the way it is done in the ACPI case anyway.\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yhNYJ3jyfz9s7g\n\tfor ;\n\tWed, 22 Nov 2017 11:26:28 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751409AbdKVA0Z (ORCPT );\n\tTue, 21 Nov 2017 19:26:25 -0500","from cloudserver094114.home.net.pl ([79.96.170.134]:51828 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751372AbdKVA0Y (ORCPT\n\t); Tue, 21 Nov 2017 19:26:24 -0500","from 79.184.254.73.ipv4.supernova.orange.pl (79.184.254.73) (HELO\n\taspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id f0db364af7a7a93f; Wed, 22 Nov 2017 01:26:21 +0100"],"From":"\"Rafael J. Wysocki\" ","To":"Brian Norris ","Cc":"Jeffy Chen ,\n\tlinux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, tony@atomide.com,\n\tshawn.lin@rock-chips.com, dianders@chromium.org,\n\tlinux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,\n\tLen Brown ","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","Date":"Wed, 22 Nov 2017 01:26:02 +0100","Message-ID":"<1882670.s3LR2t44nD@aspire.rjw.lan>","In-Reply-To":"<20171114025109.GA43048@google.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<1894178.xtK0vD2N4H@aspire.rjw.lan>\n\t<20171114025109.GA43048@google.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1817161,"web_url":"http://patchwork.ozlabs.org/comment/1817161/","msgid":"<20171206193421.GA143886@google.com>","list_archive_url":null,"date":"2017-12-06T19:34:24","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":67074,"url":"http://patchwork.ozlabs.org/api/people/67074/","name":"Brian Norris","email":"briannorris@chromium.org"},"content":"Hi Rafael,\n\nThanks for the responses, and sorry for some delay. My thoughts are\nstill a bit scattered on this (and I'm also busy with other things), and\nI'd like some feedback on the device tree definitions from someone, if\npossible. (I expect you're more familiar with ACPI than with device\ntree, but perhaps you or someone else on copy can humor me?)\n\nI'm also not sure I agree with all of your suggestions, though maybe\nsomething \"similar\" could be OK.\n\nOn Wed, Nov 22, 2017 at 01:26:02AM +0100, Rafael J. Wysocki wrote:\n> On Tuesday, November 14, 2017 3:51:11 AM CET Brian Norris wrote:\n> > On Wed, Nov 08, 2017 at 11:32:20PM +0100, Rafael J. Wysocki wrote:\n> > > On Friday, October 27, 2017 9:26:11 AM CET Jeffy Chen wrote:\n> > > > Move acpi wakeup code to pci core as pci_set_wakeup(), so that other\n> > > > platforms could reuse it.\n> > > \n> > > What exactly do you want to reuse?\n> > > \n> > > It looks like that's just several lines of code in acpi_pci_wakeup()\n> > > and acpi_pci_propagate_wakeup() which invoke ACPI-specific lower-level\n> > > functions, so IMO not worth it at all.\n> > \n> > The important part he's sharing here is the walking of the tree\n> > structure, in which it's possible for some parent along the way to\n> > handle wakeup for its children. I'm not sure how valuable nor how\n> > reusable that is.\n> \n> ACPI very well may be the only case needing to walk the hierarchy for that.\n\nSure. It does seem like a bit of overkill, on second thought.\n\n> > In this case (the Rockchip platforms Jeffy and I are working on), I\n> > think we really want to just support a single WAKE# pin for the whole\n> > system, so maybe the complexity isn't needed.\n> \n> So unless I know you'll need it, please don't add it.\n\nSure, arbitrary hierarchy is probably over-engineering. But 1-per-device\nis quite reasonable -- even if we don't support it immediately, it would\nbe nice if it can be added without too much trouble.\n\n> > The spec does describe that there are good reasons for supporting more than\n> > 1 WAKE# pin though (e.g., 1 per device), so it doesn't seem really wise to\n> > shoehorn oursleves into a single setup.\n> \n> One WAKE# pin per device is reasonable enough, but WAKE# is specifically\n> defined as out-of-band and \"orthogonal\" to the PCIe hierarchy. What you\n> need is a way to program WAKE# and (possibly) wakeup power on the given\n> platform for each device having a WAKE# pin individually.\n\nSo, *don't* define the wakeup in the host bridge?\n\nOr put another way: how would you define a DT description for this?\n\nBy the way, it seems pretty ambiguous how we want to handle things like\n(a) multiple devices sharing the same WAKE#\n(b) systems where a slot is swappable\n\nFor (a), the main problem is that if we have to repeat the interrupt\ndefinition in multiple devices, then we have to deal with something like\nIRQF_SHARED. That can be done, but it makes it much harder to use the\ndedicated wakeirq helpers.\n\nFor (b), it's already weird to write device trees for probable PCI\ndevices; you technically need to redefine the node for the PCI ID of\neach device that gets plugged in. So instead, it seems more like maybe\na property of the port?\n\nOr more concretely:\n\npcie@XXXXXXXX {\n\tcompatible = \"rockchip,rk3399-pcie\";\n\tinterrupts-extended = ..., <&gpioX Y>; // the existing\n\t\t\t\t\t\t// proposal, for\n\t\t\t\t\t\t// 'wakeup' in the host\n\t\t\t\t\t\t// bridge\n\tinterrupt-names = ..., \"wakeup\";\n\n\tpcie_port: pcie@0,0 {\n\t\t...\n\t\t/*\n\t\t// This is a possible proposal, to account for (b)?\n\t\tinterrupts = ;\n\t\tinterrupt-parent = <&gpioX>;\n\t\tinterrupt-names = \"wakeup\";\n\t\t*/\n\n\t\twifi_device: wifi@0,0 {\n\t\t\tcompatible = \"pci11ab,2b42\";\n\t\t\t/*\n\t\t\t// This is the \"1-per-device\" proposal?\n\t\t\t// But it doesn't work well if the PCI ID\n\t\t\t// changes (e.g., card swap). Probably want to\n\t\t\t// avoid this if possible.\n\t\t\t// Also, it has an awkward conflict with INTx\n\t\t\t// definitions.\n\t\t\tinterrupts-extended = <&intX 0>, <&gpioX Y>;\n\t\t\tinterrupt-names = \"pci\", \"wakeup\";\n\t\t\t*/\n\t\t\t...\n\t\t};\n\t};\n};\n\nDoes the description at the \"port\" level seem reasonable? We'd still\nhave to figure out how to parse this properly of course...\n\n> The main reason why ACPI needs to walk the hierarchy is that on some systems\n> the firmware takes over the handling of the native PME mechanism which then is\n> taken care of by the AML (and the kernel is not granted control of the\n> relevant PCIe registers). I don't think you'll ever see anything like that\n> on non-ACPI systems.\n\nOK.\n\n> > But that can be implemented either via copying the \"few\" lines of\n> > tree-walking logic, or by trying to share them.\n> > \n> > > The structure for other platform code may be the same or similar, but\n> > > the details will almost certainly be different and I don't think that\n> > > having more callback pointers in pci_platform_pm_ops is necessarily better.\n> > \n> > I suppose that's reasonable.\n> > \n> > > > Also add .setup_dev() / .setup_host_bridge() / .cleanup() platform pm\n> > > > ops's callbacks to setup and cleanup pci devices and host bridge for\n> > > > wakeup.\n> > > \n> > > Why are they needed?\n> > \n> > The implementation is in patch 7, if you really needed more info about\n> > why, or provide alternatives.\n> \n> Well, that's quite questionable.\n> \n> At least defining ->dev_wakeup to do device_set_wakeup_capable() doesn't\n> really make sense to me.\n\nHmm, that does seem a little weird, but I believe maybe Jeffy is using\nit as a hack for fitting in with the \"dedicated IRQ\" stuff? It seems\nmore like you should be able to use something like\ndev_pm_enable_wake_irq() / dev_pm_disable_wake_irq() instead though.\nThat's a question for patch 7 though, not for the general architecture\nof these hooks -- we want to do *something* the arm/disarm the wake-IRQ\nthere.\n\n> > The current set of hooks assumes that there is no state information\n> > or\n> > initialization needed for tracking actions of these platform PM hooks on\n> > a device. For ACPI this works, because devices have \"companion\"\n> > acpi_dev's to handle everything, and the ACPI framework generally\n> > prepares GPE's for you, IIUC. For 'pci-mid', the operations happen to be\n> > trivial (and arguably wrong -- several are no-ops, where we might expect\n> > the platform to tell us whether the operation was actually supported or\n> > not).\n> > \n> > For device tree, there isn't really a canonical place to store this\n> > information, nor to initialize something like wakeup interrupts.\n> \n> The only thing you need IMO is ->set_wakeup which also is what ACPI needs\n> and that enables or disables wakeup for the device. It doesn't actually\n> have to track anything (other than, possibly, whether or not wakeup has\n> been enabled for it already).\n\nYes, that's technically the only essential thing needed, though it does\nmake things a little tougher. We still may need to (depending on whether\nwe decide to define WAKE# in the bridge, the port, or the device) do\nsome hierarchy walking. I'm also not yet convinced about \"where to\ninitialize\" (see comments below).\n\n> And since the core takes care of native PMEs for you, the only thing\n> this needs to cover is the WAKE# pin programming AFAICS.\n\nRight.\n\n> The setup part, in turn, in the ACPI case is done via acpi_platform_notify()\n> and, analogously, acpi_platform_notify_remove() does the cleanup. You seem\n> to be trying to add something like it via pci_platform_pm_ops, but is it\n> really the most suitable place for that?\n\nWell, the global 'platform_notify' and 'platform_notify_remove'\nsingletons seem even more fragile. Do we really want to take over the\nwhole system's device registration hooks just to handle something for a\nparticular bus type? We're not going to be able to write a generic \"add\"\nand \"remove\" hook for all devices on all Device Tree systems for this\ntype of feature. It would likely break a lot of sub-devices to start\nparsing \"wakeup\" properties there.\n\nWe *could* do something like what it8152 does:\n\nstatic int it8152_pci_platform_notify(struct device *dev)\n{\n\tif (dev_is_pci(dev)) {\n\t...\n\t}\n\treturn 0;\n}\n\nbut that doesn't look very nice to me. Among other problems, it won't\nstack nicely. What if an 'it8152' system wants to use this 'wakeup'\ninterrupt definition too?\n\nInstead, I think it makes perfect sense to put the DT-centric wakeup\nhandling all in the same module -- the \"Firmware PM callbacks\" (struct\npci_platform_pm_ops) -- instead of scattering it around the codebase.\n\n> > Technically, we could shoehorn this into the .set_wakeup() call, but\n> > we'd probably rather not do things like request_irq() on every attempt\n> > to suspend/resume the system (among other reasons, we'd lose information\n> > that we might otherwise track in /proc/ or /sys/).\n> > \n> > Or the inverse of the above: where would you suggest initializing or\n> > tearing down the wakeirq?\n> \n> Again, ACPI does that via acpi_platform_notify()/acpi_platform_notify_remove(),\n> so maybe something similar?\n\nI don't like that solution, per the above. (Depending on what \"or maybe\nsomething similar\" means.)\n\n> In any case, you need a way to associate DT-provided data with PCI devices and,\n> ideally, that should be done at the enumeration time.\n\nYes, but that's what these hooks were allowing; PCI PM firmware hooks\nget a chance to do enumeration-time initialization for both bridges and\ndevices.\n\n> > An alternative could be to include any necessary state into the\n> > pci_host_bridge or pci_dev and just inline the setup code into\n> > pci.c/remove.c (e.g., pci_register_host_bridge()) and pci-driver.c\n> > (pci_device_{probe,remove}()). But I'm not sure that's much more\n> > beautiful.\n> \n> Well, that's not the way it is done in the ACPI case anyway.\n\nBrian","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"iCL6iNFK\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3ysTMp61fgz9s82\n\tfor ;\n\tThu, 7 Dec 2017 06:34:46 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752034AbdLFTeb (ORCPT );\n\tWed, 6 Dec 2017 14:34:31 -0500","from mail-it0-f67.google.com ([209.85.214.67]:39760 \"EHLO\n\tmail-it0-f67.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751544AbdLFTe3 (ORCPT\n\t); Wed, 6 Dec 2017 14:34:29 -0500","by mail-it0-f67.google.com with SMTP id 68so8878624ite.4\n\tfor ; Wed, 06 Dec 2017 11:34:29 -0800 (PST)","from google.com ([2620:0:1000:1600:3db8:49fb:8289:5c16])\n\tby smtp.gmail.com with ESMTPSA id\n\t3sm1908351itk.19.2017.12.06.11.34.26\n\t(version=TLS1_2 cipher=AES128-SHA bits=128/128);\n\tWed, 06 Dec 2017 11:34:27 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=yb5x9Vkzo9rMJqnN82vfMqd1Le/3H65P2rgprZ+cSSE=;\n\tb=iCL6iNFKG+ijT+hLQgKwQRUZj2Opj3+2O42gUPA7184Ys2dx87jYk+ut40bTHxiqp+\n\tFECESfNs8ZZhUYV/4oyyWEyWeZHv8DPkFOqHndI8b6Z5d+OP0omk29vMcU6kmVl1c/bI\n\tBYpYy3C/XbtHd65kbrzOZXARKvD0xlR25NYUI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=yb5x9Vkzo9rMJqnN82vfMqd1Le/3H65P2rgprZ+cSSE=;\n\tb=oL67zCsJkxxiTCaq0cym+SY9Kpj9+OiPZHJoXkQFyScviFT31/h4R87S+kYKXAtyrn\n\tdDpyjiL/LyUK4UEkx+/bRXsWwMjDb9KX19Lw9FKBB7LXDM+Tn/JNYwPZRTuGAwdlabxi\n\tILNYT4PDWG5q2g71O2jFeW9nbExGB4feewgf9I3kQXtk+qjvm9L4SzVnS81XOapZh1I/\n\t6mABNIwddE4HP9QpFoOQwH1xEeAaYPoReH7uSpqVeLO9FJly/xsPpCxacmfS9OM/bizF\n\tQ9g+uf63zEpxytwxTA/1hKbsMfsRbBSG7xPyv4EsMqGpY3lnGXvyCNy6emqN16+tEhoI\n\t8TMQ==","X-Gm-Message-State":"AJaThX7MBDgM3emSGbwrqMf1s+JJ3Yx9lsDd3DV6o9tVzvbyZeOXWE9P\n\tDGRVp5Wlkpa0HW2GA44W0lpBiw==","X-Google-Smtp-Source":"AGs4zMYKGbT3bXae4WzNLTRidZQJCZSqV4IodseP0oJ1v2tOfkux2nmYHsuFAn9+vUFxuJAmP1uGCg==","X-Received":"by 10.107.102.18 with SMTP id a18mr36733911ioc.105.1512588868449;\n\tWed, 06 Dec 2017 11:34:28 -0800 (PST)","Date":"Wed, 6 Dec 2017 11:34:24 -0800","From":"Brian Norris ","To":"\"Rafael J. Wysocki\" ","Cc":"Jeffy Chen ,\n\tlinux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, tony@atomide.com,\n\tshawn.lin@rock-chips.com, dianders@chromium.org,\n\tlinux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,\n\tLen Brown ","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","Message-ID":"<20171206193421.GA143886@google.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<1894178.xtK0vD2N4H@aspire.rjw.lan>\n\t<20171114025109.GA43048@google.com>\n\t<1882670.s3LR2t44nD@aspire.rjw.lan>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1882670.s3LR2t44nD@aspire.rjw.lan>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1817331,"web_url":"http://patchwork.ozlabs.org/comment/1817331/","msgid":"<20171207001754.GB28152@atomide.com>","list_archive_url":null,"date":"2017-12-07T00:17:54","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":365,"url":"http://patchwork.ozlabs.org/api/people/365/","name":"Tony Lindgren","email":"tony@atomide.com"},"content":"* Brian Norris [171206 19:36]:\n> By the way, it seems pretty ambiguous how we want to handle things like\n> (a) multiple devices sharing the same WAKE#\n> (b) systems where a slot is swappable\n> \n> For (a), the main problem is that if we have to repeat the interrupt\n> definition in multiple devices, then we have to deal with something like\n> IRQF_SHARED. That can be done, but it makes it much harder to use the\n> dedicated wakeirq helpers.\n\nThis will get messy, let's not go there :) That is unless the hardware\nreally has a single interrupt wired to multiple devices. And in that\ncase almost certainly a custom interrupt handler is needed.\n\nRegards,\n\nTony","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3ysbfd2kT8z9s74\n\tfor ;\n\tThu, 7 Dec 2017 11:18:01 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752071AbdLGAR7 (ORCPT );\n\tWed, 6 Dec 2017 19:17:59 -0500","from muru.com ([72.249.23.125]:53918 \"EHLO muru.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751748AbdLGAR6 (ORCPT );\n\tWed, 6 Dec 2017 19:17:58 -0500","from atomide.com (localhost [127.0.0.1])\n\tby muru.com (Postfix) with ESMTPS id F3C058061;\n\tThu, 7 Dec 2017 00:20:11 +0000 (UTC)"],"Date":"Wed, 6 Dec 2017 16:17:54 -0800","From":"Tony Lindgren ","To":"Brian Norris ","Cc":"\"Rafael J. Wysocki\" ,\n\tJeffy Chen ,\n\tlinux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, shawn.lin@rock-chips.com,\n\tdianders@chromium.org, linux-pci@vger.kernel.org,\n\tlinux-acpi@vger.kernel.org, Len Brown ","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","Message-ID":"<20171207001754.GB28152@atomide.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<1894178.xtK0vD2N4H@aspire.rjw.lan>\n\t<20171114025109.GA43048@google.com>\n\t<1882670.s3LR2t44nD@aspire.rjw.lan>\n\t<20171206193421.GA143886@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171206193421.GA143886@google.com>","User-Agent":"Mutt/1.9.1 (2017-09-22)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1817336,"web_url":"http://patchwork.ozlabs.org/comment/1817336/","msgid":"<20171207002955.GA40447@google.com>","list_archive_url":null,"date":"2017-12-07T00:29:56","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":67074,"url":"http://patchwork.ozlabs.org/api/people/67074/","name":"Brian Norris","email":"briannorris@chromium.org"},"content":"On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote:\n> * Brian Norris [171206 19:36]:\n> > By the way, it seems pretty ambiguous how we want to handle things like\n> > (a) multiple devices sharing the same WAKE#\n> > (b) systems where a slot is swappable\n> > \n> > For (a), the main problem is that if we have to repeat the interrupt\n> > definition in multiple devices, then we have to deal with something like\n> > IRQF_SHARED. That can be done, but it makes it much harder to use the\n> > dedicated wakeirq helpers.\n> \n> This will get messy, let's not go there :) That is unless the hardware\n> really has a single interrupt wired to multiple devices. And in that\n> case almost certainly a custom interrupt handler is needed.\n\nAs Rafael mentioned, the spec doesn't clearly delineate a required\nhierarchy to the WAKE# pin, and it's certainly possible to share it. I'm\nfine dodging that question for now, and only writing said custom\ninterrupt handler if/when needed.\n\nBut device tree bindings are \"forever\", so it seems reasonable to at\nleast agree how it should be defined.\n\nBrian","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"ZMM7mP0z\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3ysbwn3TxCz9s0g\n\tfor ;\n\tThu, 7 Dec 2017 11:30:17 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752150AbdLGAaC (ORCPT );\n\tWed, 6 Dec 2017 19:30:02 -0500","from mail-it0-f65.google.com ([209.85.214.65]:42434 \"EHLO\n\tmail-it0-f65.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751950AbdLGAaA (ORCPT\n\t); Wed, 6 Dec 2017 19:30:00 -0500","by mail-it0-f65.google.com with SMTP id p139so10795990itb.1\n\tfor ; Wed, 06 Dec 2017 16:30:00 -0800 (PST)","from google.com ([2620:0:1000:1600:3d85:4e84:cce8:a0cb])\n\tby smtp.gmail.com with ESMTPSA id\n\tc141sm1875493ioe.13.2017.12.06.16.29.58\n\t(version=TLS1_2 cipher=AES128-SHA bits=128/128);\n\tWed, 06 Dec 2017 16:29:59 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=INs6DTgxORCKPUxPBsNDvlO2uM11BEWNl46N0ddEuQs=;\n\tb=ZMM7mP0zC6R73aIPpNT2IttkyYxzKnelmxJ6bGVLkpAek1q+sB4bfw7hCwOpqiMzYG\n\tQBqeYkl8zDAY+AFr/28xrJoIty6lx2TYDJ+eA1hS6NuTMCMobAKBjWq27Z9VkY21s34I\n\tEDHbtYAkxuhqIUBhexIbZ32MNromGxDZTDoMM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=INs6DTgxORCKPUxPBsNDvlO2uM11BEWNl46N0ddEuQs=;\n\tb=NKkh6eHqEMRiZHodANzyOBVssD8LDYk6kgHsnXCArfRDCGv4WWMO3BM0I25A+a8wjl\n\tT2ljyEs5KgaltN7JfGoW9U8BwSVE1eFfccjydQgMqCQ/5kreNk4J9i56wFBm71JxItVg\n\tteIIf3qESxQHInk66eLD7bMVnnYIX/GnojyIP/o5DvL7upa32fysbG60Gs0phkR2XP7c\n\tIXJMdvk9NOCagX1bVHvqfWJJRm66o+T085WJFUkKnWmTsgC2fr6TMle//Ew3aV43IR7p\n\tjcF0+QxmhrH2r1Qyvt70t7UBcFxzZS3Dblpjs2xESOlvpvD9k5YtQzDl/rb1kKxqO1HZ\n\tbt3A==","X-Gm-Message-State":"AJaThX5TY7pVsUSC5b/xT6Xhs+L30AXHEH+3/3FZjYM10qPKWL+c/SZS\n\tUmrXldxwIQTiSprjVHwZDQwmjQ==","X-Google-Smtp-Source":"AGs4zMYjd3qt66PyPtkWCMmsUa0xq++nK0xD09tO6Nrjmk40i8mRLzhcwlYYOeNsL70oVsiG5tBHnA==","X-Received":"by 10.36.164.13 with SMTP id z13mr26301605ite.115.1512606599779; \n\tWed, 06 Dec 2017 16:29:59 -0800 (PST)","Date":"Wed, 6 Dec 2017 16:29:56 -0800","From":"Brian Norris ","To":"Tony Lindgren ","Cc":"\"Rafael J. Wysocki\" ,\n\tJeffy Chen ,\n\tlinux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, shawn.lin@rock-chips.com,\n\tdianders@chromium.org, linux-pci@vger.kernel.org,\n\tlinux-acpi@vger.kernel.org, Len Brown ","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","Message-ID":"<20171207002955.GA40447@google.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<1894178.xtK0vD2N4H@aspire.rjw.lan>\n\t<20171114025109.GA43048@google.com>\n\t<1882670.s3LR2t44nD@aspire.rjw.lan>\n\t<20171206193421.GA143886@google.com>\n\t<20171207001754.GB28152@atomide.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171207001754.GB28152@atomide.com>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1818910,"web_url":"http://patchwork.ozlabs.org/comment/1818910/","msgid":"<20171208163710.GB24344@atomide.com>","list_archive_url":null,"date":"2017-12-08T16:37:10","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":365,"url":"http://patchwork.ozlabs.org/api/people/365/","name":"Tony Lindgren","email":"tony@atomide.com"},"content":"* Brian Norris [171207 00:32]:\n> On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote:\n> > * Brian Norris [171206 19:36]:\n> > > By the way, it seems pretty ambiguous how we want to handle things like\n> > > (a) multiple devices sharing the same WAKE#\n> > > (b) systems where a slot is swappable\n> > > \n> > > For (a), the main problem is that if we have to repeat the interrupt\n> > > definition in multiple devices, then we have to deal with something like\n> > > IRQF_SHARED. That can be done, but it makes it much harder to use the\n> > > dedicated wakeirq helpers.\n> > \n> > This will get messy, let's not go there :) That is unless the hardware\n> > really has a single interrupt wired to multiple devices. And in that\n> > case almost certainly a custom interrupt handler is needed.\n> \n> As Rafael mentioned, the spec doesn't clearly delineate a required\n> hierarchy to the WAKE# pin, and it's certainly possible to share it. I'm\n> fine dodging that question for now, and only writing said custom\n> interrupt handler if/when needed.\n\nOK if the WAKE# pin is shared then PCI (or hardware specific?) code needs\nto figure out from where it came from.\n\n> But device tree bindings are \"forever\", so it seems reasonable to at\n> least agree how it should be defined.\n\nWell that's why we're just using the existing interrupts-extended\nbinding there :) It does not leave out the option for shared interrupts,\nit's just that drivers/base/power/wakeirq.c can't deal with them in a\nsane way or at least we'd have to add a flag to not enable/disable the\nwakeirq automatically.\n\nRegards,\n\nTony","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3ytdL62bKJz9t9j\n\tfor ;\n\tSat, 9 Dec 2017 03:37:18 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753141AbdLHQhQ (ORCPT );\n\tFri, 8 Dec 2017 11:37:16 -0500","from muru.com ([72.249.23.125]:58832 \"EHLO muru.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751800AbdLHQhP (ORCPT );\n\tFri, 8 Dec 2017 11:37:15 -0500","from atomide.com (localhost [127.0.0.1])\n\tby muru.com (Postfix) with ESMTPS id 9A28A8068;\n\tFri, 8 Dec 2017 16:39:30 +0000 (UTC)"],"Date":"Fri, 8 Dec 2017 08:37:10 -0800","From":"Tony Lindgren ","To":"Brian Norris ","Cc":"\"Rafael J. Wysocki\" ,\n\tJeffy Chen ,\n\tlinux-kernel@vger.kernel.org, bhelgaas@google.com,\n\tlinux-pm@vger.kernel.org, shawn.lin@rock-chips.com,\n\tdianders@chromium.org, linux-pci@vger.kernel.org,\n\tlinux-acpi@vger.kernel.org, Len Brown ","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","Message-ID":"<20171208163710.GB24344@atomide.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<1894178.xtK0vD2N4H@aspire.rjw.lan>\n\t<20171114025109.GA43048@google.com>\n\t<1882670.s3LR2t44nD@aspire.rjw.lan>\n\t<20171206193421.GA143886@google.com>\n\t<20171207001754.GB28152@atomide.com>\n\t<20171207002955.GA40447@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171207002955.GA40447@google.com>","User-Agent":"Mutt/1.9.1 (2017-09-22)","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}},{"id":1818947,"web_url":"http://patchwork.ozlabs.org/comment/1818947/","msgid":"","list_archive_url":null,"date":"2017-12-08T17:12:42","subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","submitter":{"id":64267,"url":"http://patchwork.ozlabs.org/api/people/64267/","name":"Rafael J. Wysocki","email":"rafael@kernel.org"},"content":"On Fri, Dec 8, 2017 at 5:37 PM, Tony Lindgren wrote:\n> * Brian Norris [171207 00:32]:\n>> On Wed, Dec 06, 2017 at 04:17:54PM -0800, Tony Lindgren wrote:\n>> > * Brian Norris [171206 19:36]:\n>> > > By the way, it seems pretty ambiguous how we want to handle things like\n>> > > (a) multiple devices sharing the same WAKE#\n>> > > (b) systems where a slot is swappable\n>> > >\n>> > > For (a), the main problem is that if we have to repeat the interrupt\n>> > > definition in multiple devices, then we have to deal with something like\n>> > > IRQF_SHARED. That can be done, but it makes it much harder to use the\n>> > > dedicated wakeirq helpers.\n>> >\n>> > This will get messy, let's not go there :) That is unless the hardware\n>> > really has a single interrupt wired to multiple devices. And in that\n>> > case almost certainly a custom interrupt handler is needed.\n>>\n>> As Rafael mentioned, the spec doesn't clearly delineate a required\n>> hierarchy to the WAKE# pin, and it's certainly possible to share it. I'm\n>> fine dodging that question for now, and only writing said custom\n>> interrupt handler if/when needed.\n>\n> OK if the WAKE# pin is shared then PCI (or hardware specific?) code needs\n> to figure out from where it came from.\n\nRight.\n\nBasically, that would need a list (or equivalent) of devices sharing\nthe wakeup IRQ and if that triggers, it needs to walk the list and\ncall pci_pme_wakeup() for all devices in it.\n\n>> But device tree bindings are \"forever\", so it seems reasonable to at\n>> least agree how it should be defined.\n>\n> Well that's why we're just using the existing interrupts-extended\n> binding there :) It does not leave out the option for shared interrupts,\n> it's just that drivers/base/power/wakeirq.c can't deal with them in a\n> sane way or at least we'd have to add a flag to not enable/disable the\n> wakeirq automatically.\n\nI think that the binding needs to be sort-of PCI-specific to cover the\nshared case.\n\nI'm not sure if there are any other buses needing that (they would\nneed to define standard PM registers as PCI does).","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-pci-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"rGv7WQeg\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3ytf743Xdyz9tB8\n\tfor ;\n\tSat, 9 Dec 2017 04:12:48 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751113AbdLHRMp (ORCPT );\n\tFri, 8 Dec 2017 12:12:45 -0500","from mail-ot0-f169.google.com ([74.125.82.169]:42523 \"EHLO\n\tmail-ot0-f169.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751492AbdLHRMo (ORCPT\n\t); Fri, 8 Dec 2017 12:12:44 -0500","by mail-ot0-f169.google.com with SMTP id i1so9704054oth.9;\n\tFri, 08 Dec 2017 09:12:43 -0800 (PST)","by 10.157.46.145 with HTTP; Fri, 8 Dec 2017 09:12:42 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=S9UF+dWvbAdy74sx0H3da3Yrfzxo2nCB7koZzY1riAQ=;\n\tb=rGv7WQegcez9jSUzByTdsHAGDXZVDPu+mxAaM3VpdoFdsoFHhqeM9LEugwxwWK9mFh\n\teKy3KyXT7OgIzSgwMgzoPIEpxyxfuEGSHzifUJWqPn/jq7LLTO3pfsHikU2zp8CRgFrU\n\tJK9bOu2eIm/5a2iiybWdmH0H+i3LrAALekuSYZCyicxfoF5q0uHBm6R8ChMKAAaWIdsa\n\ttLlSRWsm4C7Z5sW9AKmKfwVSWE2HRdwmfMrWae0o1/fkTJ+jB12eQzuty5Xo4CeiluiZ\n\tcWYboxZRX471x3+GMbeHCW3pOuwS0h/1T+0akfIsmULlOnSdqMI3ZXH2trSux2bgOF9a\n\tHyXw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=S9UF+dWvbAdy74sx0H3da3Yrfzxo2nCB7koZzY1riAQ=;\n\tb=fh1zYN/WAN/ORPeu1jkcgWDd+ErXCYfJjNMSYVrO3Uum7ybDJBFyr3bCsry5xxhuyb\n\t24vkxyPJrOswcsLKNaB3qAQhbSM5d4UUOWvtin+53X0dESUGcblRyW3gLWbUAFA/+S6f\n\t4KzoEuvkGBQWab6f/RJdsRhsCUbuP48XxJjRJLiy8IMAePBi0YLFk1CI11OlHAqrbet6\n\td//tF91Fp+ykVyiGNBnd1Q173vy4Dns6a8jZ99NUbBPrPy971KFx7GBdLG6Sb8z6bA4G\n\tL8SDLikkwaqL4HB8CKQNFK2STUG0K12RZwW3FHQSWVZRoIvugbhwhH+yFiQAP1twFEZX\n\twxtQ==","X-Gm-Message-State":"AJaThX4jXx1StoPhzGMwjsrhI7DO15/OzknyJ3sC/0ad/K1nfU0n/BsQ\n\tXhA3swACSXCI8Gj5SxmN85Updm1uPhZjMmOf/fk=","X-Google-Smtp-Source":"AGs4zManlkSsrnBgm0mZ0ZUaXFbem1d2/l0IQLGoYhU2j/2iR1hF4vcO3FBXn14MQ+lKoP1l0v1DUXZMfVJ6/8227Hg=","X-Received":"by 10.157.55.137 with SMTP id x9mr28082697otb.367.1512753163220; \n\tFri, 08 Dec 2017 09:12:43 -0800 (PST)","MIME-Version":"1.0","In-Reply-To":"<20171208163710.GB24344@atomide.com>","References":"<20171027072612.26565-1-jeffy.chen@rock-chips.com>\n\t<1894178.xtK0vD2N4H@aspire.rjw.lan>\n\t<20171114025109.GA43048@google.com>\n\t<1882670.s3LR2t44nD@aspire.rjw.lan>\n\t<20171206193421.GA143886@google.com>\n\t<20171207001754.GB28152@atomide.com>\n\t<20171207002955.GA40447@google.com>\n\t<20171208163710.GB24344@atomide.com>","From":"\"Rafael J. Wysocki\" ","Date":"Fri, 8 Dec 2017 18:12:42 +0100","X-Google-Sender-Auth":"dI_U_lPz2kcddEleNn1-Cqem3FE","Message-ID":"","Subject":"Re: [RFC PATCH v10 6/7] PCI / PM: Move acpi wakeup code to pci core","To":"Tony Lindgren ","Cc":"Brian Norris ,\n\t\"Rafael J. Wysocki\" ,\n\tJeffy Chen ,\n\tLinux Kernel Mailing List ,\n\tBjorn Helgaas ,\n\tLinux PM , shawn.lin@rock-chips.com,\n\tDoug Anderson ,\n\tLinux PCI ,\n\tACPI Devel Maling List ,\n\tLen Brown ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-pci-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-pci@vger.kernel.org"}}]