From d99516b57a6b5d02f40423201b0d7f2a9a43fda1 Mon Sep 17 00:00:00 2001 From: Maximilian Luz Date: Thu, 29 Oct 2020 22:04:38 +0100 Subject: [PATCH] PCI: Allow D3cold for hot-plug ports on Surface Books The Microsoft Surface Book series of devices have a tablet part (so called clipboard) that can be detached from the base of the device. While the clipboard contains the CPU, the base can contain a discrete GPU (dGPU). This dGPU is connected via a PCIe hot-plug port. Currently D3cold is disallowed for all hot-plug ports. On the Surface Book 2 and 3, this leads to increased power consumption during suspend and when the dGPU is not used (i.e. runtime suspended). This can be observed not only in battery drain, but also by the dGPU getting notably warm while suspended and not in D3cold. Testing shows that the Surface Books behave well with D3cold enabled for hot-plug ports, alleviating the aforementioned issues. Thus white-list D3cold for hot-plug ports on those devices. Note: PCIe hot-plug signalling while the device is in D3cold is handled via ACPI, out-of-band interrupts, and the surface_hotplug driver (combined). The device will work without the surface_hotplug driver, however, device removal/addition will only be detected on device resume. Signed-off-by: Maximilian Luz Patchset: surface-hotplug --- drivers/pci/pci.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b2fed944903e..e288e5058520 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2814,6 +2814,32 @@ static const struct dmi_system_id bridge_d3_blacklist[] = { { } }; +static const struct dmi_system_id bridge_d3_hotplug_whitelist[] = { +#ifdef CONFIG_X86 + { + /* + * Microsoft Surface Books have a hot-plug root port for the + * discrete GPU (the device containing it can be detached form + * the top-part, containing the cpu). + * + * If this discrete GPU is not transitioned into D3cold for + * suspend, the device will become notably warm and also + * consume a lot more power than desirable. + * + * We assume that since those devices have been confirmed + * working with D3, future Surface devices will too. So let's + * keep this match generic. + */ + .ident = "Microsoft Surface", + .matches = { + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"), + DMI_MATCH(DMI_PRODUCT_NAME, "Surface"), + }, + }, +#endif + { } +}; + /** * pci_bridge_d3_possible - Is it possible to put the bridge into D3 * @bridge: Bridge to check @@ -2854,10 +2880,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge) /* * Hotplug ports handled natively by the OS were not validated * by vendors for runtime D3 at least until 2018 because there - * was no OS support. + * was no OS support. Explicitly whitelist systems that have + * been confirmed working. */ if (bridge->is_hotplug_bridge) - return false; + return dmi_check_system(bridge_d3_hotplug_whitelist); if (dmi_check_system(bridge_d3_blacklist)) return false; -- 2.29.2 From ac21f0d507c8b1f57566b1bbd947281b708f0fc8 Mon Sep 17 00:00:00 2001 From: Maximilian Luz Date: Mon, 9 Nov 2020 14:23:00 +0100 Subject: [PATCH] PCI: Run platform power transition on initial D0 entry On some devices and platforms, the initial platform power state is not in sync with the power state of the PCI device. pci_enable_device_flags() updates the state of a PCI device by reading from the the PCI_PM_CTRL register. This may change the stored power state of the device without running the appropriate platform power transition. Due to the stored power-state being changed, the later call to pci_set_power_state(..., PCI_D0) in do_pci_enable_device() can evaluate to a no-op if the stored state has been changed to D0 via that. This will then prevent the appropriate platform power transition to be run, which can on some devices and platforms lead to platform and PCI power state being entirely different, i.e. out-of-sync. On ACPI platforms, this can lead to power resources not being turned on, even though they are marked as required for D0. Specifically, on the Microsoft Surface Book 2 and 3, some ACPI power regions that should be "on" for the D0 state (and others) are initialized as "off" in ACPI, whereas the PCI device is in D0. As the state is updated in pci_enable_device_flags() without ensuring that the platform state is also updated, the power resource will never be properly turned on. Instead, it lives in a sort of on-but-marked-as-off zombie-state, which confuses things down the line when attempting to transition the device into D3cold: As the resource is already marked as off, it won't be turned off and the device does not fully enter D3cold, causing increased power consumption during (runtime-)suspend. By replacing pci_set_power_state() in do_pci_enable_device() with pci_power_up(), we can force pci_platform_power_transition() to be called, which will then check if the platform power state needs updating and appropriate actions need to be taken. Signed-off-by: Maximilian Luz Patchset: surface-hotplug --- drivers/pci/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e288e5058520..d260c0fcf2d5 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1791,7 +1791,7 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars) u16 cmd; u8 pin; - err = pci_set_power_state(dev, PCI_D0); + err = pci_power_up(dev); if (err < 0 && err != -EIO) return err; -- 2.29.2 From edebb042089e982ae1623df8eee05ceaae289b3c Mon Sep 17 00:00:00 2001 From: Maximilian Luz Date: Sat, 31 Oct 2020 20:46:33 +0100 Subject: [PATCH] PCI: Add sysfs attribute for PCI device power state While most PCI power-states can be queried from user-space via lspci, this has some limits. Specifically, lspci fails to provide an accurate value when the device is in D3cold as it has to resume the device before it can access its power state via the configuration space, leading to it reporting D0 or another on-state. Thus lspci can, for example, not be used to diagnose power-consumption issues for devices that can enter D3cold or to ensure that devices properly enter D3cold at all. To alleviate this issue, introduce a new sysfs device attribute for the PCI power state, showing the current power state as seen by the kernel. Signed-off-by: Maximilian Luz Patchset: surface-hotplug --- Documentation/ABI/testing/sysfs-bus-pci | 9 +++++++++ drivers/pci/pci-sysfs.c | 12 ++++++++++++ 2 files changed, 21 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci index 450296cc7948..881040af2611 100644 --- a/Documentation/ABI/testing/sysfs-bus-pci +++ b/Documentation/ABI/testing/sysfs-bus-pci @@ -360,3 +360,12 @@ Contact: Heiner Kallweit Description: If ASPM is supported for an endpoint, these files can be used to disable or enable the individual power management states. Write y/1/on to enable, n/0/off to disable. + +What: /sys/bus/pci/devices/.../power_state +Date: November 2020 +Contact: Linux PCI developers +Description: + This file contains the current PCI power state of the device. + The value comes from the PCI kernel device state and can be one + of: "unknown", "error", "D0", D1", "D2", "D3hot", "D3cold". + The file is read only. diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 6d78df981d41..17f186ce8e87 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -124,6 +124,17 @@ static ssize_t cpulistaffinity_show(struct device *dev, } static DEVICE_ATTR_RO(cpulistaffinity); +/* PCI power state */ +static ssize_t power_state_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct pci_dev *pci_dev = to_pci_dev(dev); + pci_power_t state = READ_ONCE(pci_dev->current_state); + + return sprintf(buf, "%s\n", pci_power_name(state)); +} +static DEVICE_ATTR_RO(power_state); + /* show resources */ static ssize_t resource_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -581,6 +592,7 @@ static ssize_t driver_override_show(struct device *dev, static DEVICE_ATTR_RW(driver_override); static struct attribute *pci_dev_attrs[] = { + &dev_attr_power_state.attr, &dev_attr_resource.attr, &dev_attr_vendor.attr, &dev_attr_device.attr, -- 2.29.2 From 2f0014c35ac01e1524483b064c0b7e0be21987da Mon Sep 17 00:00:00 2001 From: Maximilian Luz Date: Mon, 14 Dec 2020 20:50:59 +0100 Subject: [PATCH] platform/x86: Add Surface Hotplug driver Add a driver to handle out-of-band hot-plug signaling for the discrete GPU (dGPU) on Microsoft Surface Book 2 and 3 devices. This driver is required to properly detect hot-plugging of the dGPU and relay the appropriate signal to the PCIe hot-plug driver core. Signed-off-by: Maximilian Luz Patchset: surface-hotplug --- drivers/platform/x86/Kconfig | 12 ++ drivers/platform/x86/Makefile | 1 + drivers/platform/x86/surface_hotplug.c | 267 +++++++++++++++++++++++++ 3 files changed, 280 insertions(+) create mode 100644 drivers/platform/x86/surface_hotplug.c diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index a9b12f4dcbd1..3e882b1e1f74 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -910,6 +910,18 @@ config SURFACE_GPE accordingly. It is required on those devices to allow wake-ups from suspend by opening the lid. +config SURFACE_HOTPLUG + tristate "Surface Hot-Plug System Driver" + depends on ACPI + default m + help + Driver for the Surface discrete GPU (dGPU) hot-plug system. + + This driver provides support for out-of-band hot-plug event signaling + on Surface Book 2 and 3 devices. This out-of-band signaling is + required to notify the kernel of any hot-plug events when the dGPU is + powered off, i.e. in D3cold. + config SURFACE_BOOK1_DGPU_SWITCH tristate "Surface Book 1 dGPU Switch Driver" depends on ACPI && SYSFS diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 562d83940e7b..2009224dcaae 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -87,6 +87,7 @@ obj-$(CONFIG_SURFACE_3_BUTTON) += surface3_button.o obj-$(CONFIG_SURFACE_3_POWER_OPREGION) += surface3_power.o obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o +obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o obj-$(CONFIG_SURFACE_BOOK1_DGPU_SWITCH) += sb1_dgpu_sw.o # MSI diff --git a/drivers/platform/x86/surface_hotplug.c b/drivers/platform/x86/surface_hotplug.c new file mode 100644 index 000000000000..572fba30cd77 --- /dev/null +++ b/drivers/platform/x86/surface_hotplug.c @@ -0,0 +1,267 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Surface Book (gen. 2 and later) hot-plug driver. + * + * Surface Book devices (can) have a hot-pluggable discrete GPU (dGPU). This + * driver is responsible for out-of-band hot-plug event signaling on these + * devices. It is specifically required when the hot-plug device is in D3cold + * and can thus not generate PCIe hot-plug events itself. + * + * Event signaling is handled via ACPI, which will generate the appropriate + * device-check notifications to be picked up by the PCIe hot-plug driver. + * + * Copyright (C) 2019-2020 Maximilian Luz + */ + +#include +#include +#include +#include +#include +#include +#include + +static const struct acpi_gpio_params shps_base_presence_int = { 0, 0, false }; +static const struct acpi_gpio_params shps_base_presence = { 1, 0, false }; +static const struct acpi_gpio_params shps_device_power_int = { 2, 0, false }; +static const struct acpi_gpio_params shps_device_power = { 3, 0, false }; +static const struct acpi_gpio_params shps_device_presence_int = { 4, 0, false }; +static const struct acpi_gpio_params shps_device_presence = { 5, 0, false }; + +static const struct acpi_gpio_mapping shps_acpi_gpios[] = { + { "base_presence-int-gpio", &shps_base_presence_int, 1 }, + { "base_presence-gpio", &shps_base_presence, 1 }, + { "device_power-int-gpio", &shps_device_power_int, 1 }, + { "device_power-gpio", &shps_device_power, 1 }, + { "device_presence-int-gpio", &shps_device_presence_int, 1 }, + { "device_presence-gpio", &shps_device_presence, 1 }, + { }, +}; + +/* 5515a847-ed55-4b27-8352-cd320e10360a */ +static const guid_t shps_dsm_guid = + GUID_INIT(0x5515a847, 0xed55, 0x4b27, 0x83, 0x52, 0xcd, + 0x32, 0x0e, 0x10, 0x36, 0x0a); + +#define SHPS_DSM_REVISION 1 + +enum shps_dsm_fn { + SHPS_DSM_FN_PCI_NUM_ENTRIES = 0x01, + SHPS_DSM_FN_PCI_GET_ENTRIES = 0x02, + SHPS_DSM_FN_IRQ_BASE_PRESENCE = 0x03, + SHPS_DSM_FN_IRQ_DEVICE_POWER = 0x04, + SHPS_DSM_FN_IRQ_DEVICE_PRESENCE = 0x05, +}; + +enum shps_irq_type { + /* NOTE: Must be in order of DSM function */ + SHPS_IRQ_TYPE_BASE_PRESENCE = 0, + SHPS_IRQ_TYPE_DEVICE_POWER = 1, + SHPS_IRQ_TYPE_DEVICE_PRESENCE = 2, + + SHPS_NUM_IRQS, +}; + +static const char *const shps_gpio_names[] = { + [SHPS_IRQ_TYPE_BASE_PRESENCE] = "base_presence", + [SHPS_IRQ_TYPE_DEVICE_POWER] = "device_power", + [SHPS_IRQ_TYPE_DEVICE_PRESENCE] = "device_presence", +}; + +struct shps_device { + struct mutex lock[SHPS_NUM_IRQS]; + struct gpio_desc *gpio[SHPS_NUM_IRQS]; + unsigned int irq[SHPS_NUM_IRQS]; +}; + +#define SHPS_IRQ_NOT_PRESENT ((unsigned int)-1) + +static void shps_dsm_notify_irq(struct platform_device *pdev, + enum shps_irq_type type) +{ + struct shps_device *sdev = platform_get_drvdata(pdev); + acpi_handle handle = ACPI_HANDLE(&pdev->dev); + union acpi_object *result; + union acpi_object param; + int value; + + mutex_lock(&sdev->lock[type]); + + value = gpiod_get_value_cansleep(sdev->gpio[type]); + if (value < 0) { + mutex_unlock(&sdev->lock[type]); + dev_err(&pdev->dev, "failed to get gpio: %d (irq=%d)\n", + type, value); + return; + } + + dev_dbg(&pdev->dev, "IRQ notification via DSM (irq=%d, value=%d)\n", + type, value); + + param.type = ACPI_TYPE_INTEGER; + param.integer.value = value; + + result = acpi_evaluate_dsm(handle, &shps_dsm_guid, SHPS_DSM_REVISION, + SHPS_DSM_FN_IRQ_BASE_PRESENCE + type, ¶m); + + if (!result) { + mutex_unlock(&sdev->lock[type]); + dev_err(&pdev->dev, + "IRQ notification via DSM failed (irq=%d, gpio=%d)\n", + type, value); + return; + } + + if (result->type != ACPI_TYPE_BUFFER) { + dev_err(&pdev->dev, + "IRQ notification via DSM failed: unexpected result type (irq=%d, gpio=%d)\n", + type, value); + } + + if (result->buffer.length != 1 || result->buffer.pointer[0] != 0) { + dev_err(&pdev->dev, + "IRQ notification via DSM failed: unexpected result value (irq=%d, gpio=%d)\n", + type, value); + } + + mutex_unlock(&sdev->lock[type]); + ACPI_FREE(result); +} + +static irqreturn_t shps_handle_irq(int irq, void *data) +{ + struct platform_device *pdev = data; + struct shps_device *sdev = platform_get_drvdata(pdev); + int type; + + /* Figure out which IRQ we're handling. */ + for (type = 0; type < SHPS_NUM_IRQS; type++) + if (irq == sdev->irq[type]) + break; + + /* We should have found our interrupt, if not: this is a bug. */ + if (WARN(type >= SHPS_NUM_IRQS, "invalid IRQ number: %d\n", irq)) + return IRQ_HANDLED; + + /* Forward interrupt to ACPI via DSM. */ + shps_dsm_notify_irq(pdev, type); + return IRQ_HANDLED; +} + +static int shps_setup_irq(struct platform_device *pdev, enum shps_irq_type type) +{ + unsigned long flags = IRQF_ONESHOT | IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING; + struct shps_device *sdev = platform_get_drvdata(pdev); + struct gpio_desc *gpiod; + acpi_handle handle = ACPI_HANDLE(&pdev->dev); + const char *irq_name; + const int dsm = SHPS_DSM_FN_IRQ_BASE_PRESENCE + type; + int status, irq; + + /* Initialize as "not present". */ + sdev->gpio[type] = NULL; + sdev->irq[type] = SHPS_IRQ_NOT_PRESENT; + + /* Only set up interrupts that we actually need. */ + if (!acpi_check_dsm(handle, &shps_dsm_guid, SHPS_DSM_REVISION, BIT(dsm))) { + dev_dbg(&pdev->dev, "IRQ notification via DSM not present (irq=%d)\n", + type); + return 0; + } + + gpiod = devm_gpiod_get(&pdev->dev, shps_gpio_names[type], GPIOD_ASIS); + if (IS_ERR(gpiod)) + return PTR_ERR(gpiod); + + irq = gpiod_to_irq(gpiod); + if (irq < 0) + return irq; + + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "shps-irq-%d", type); + if (!irq_name) + return -ENOMEM; + + status = devm_request_threaded_irq(&pdev->dev, irq, NULL, shps_handle_irq, + flags, irq_name, pdev); + if (status) + return status; + + dev_dbg(&pdev->dev, "set up irq %d as type %d\n", irq, type); + + sdev->gpio[type] = gpiod; + sdev->irq[type] = irq; + + return 0; +} + +static int surface_hotplug_probe(struct platform_device *pdev) +{ + struct shps_device *sdev; + int status, i; + + if (gpiod_count(&pdev->dev, NULL) < 0) + return -ENODEV; + + status = devm_acpi_dev_add_driver_gpios(&pdev->dev, shps_acpi_gpios); + if (status) + return status; + + sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL); + if (!sdev) + return -ENOMEM; + + platform_set_drvdata(pdev, sdev); + + /* Set up IRQs. */ + for (i = 0; i < SHPS_NUM_IRQS; i++) { + mutex_init(&sdev->lock[i]); + + status = shps_setup_irq(pdev, i); + if (status) { + dev_err(&pdev->dev, "failed to set up IRQ %d: %d\n", + i, status); + return status; + } + } + + /* Ensure everything is up-to-date. */ + for (i = 0; i < SHPS_NUM_IRQS; i++) + if (sdev->irq[i] != SHPS_IRQ_NOT_PRESENT) + shps_dsm_notify_irq(pdev, i); + + return 0; +} + +static int surface_hotplug_remove(struct platform_device *pdev) +{ + struct shps_device *sdev = platform_get_drvdata(pdev); + int i; + + /* Ensure that IRQs have been fully handled and won't trigger any more. */ + for (i = 0; i < SHPS_NUM_IRQS; i++) + if (sdev->irq[i] != SHPS_IRQ_NOT_PRESENT) + disable_irq(sdev->irq[i]); + + return 0; +} + +static const struct acpi_device_id surface_hotplug_acpi_match[] = { + { "MSHW0153", 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, surface_hotplug_acpi_match); + +static struct platform_driver surface_hotplug_driver = { + .probe = surface_hotplug_probe, + .remove = surface_hotplug_remove, + .driver = { + .name = "surface_hotplug", + .acpi_match_table = surface_hotplug_acpi_match, + .probe_type = PROBE_PREFER_ASYNCHRONOUS, + }, +}; +module_platform_driver(surface_hotplug_driver); + +MODULE_AUTHOR("Maximilian Luz "); +MODULE_DESCRIPTION("Surface Hot-Plug Signaling Driver for Surface Book Devices"); +MODULE_LICENSE("GPL"); -- 2.29.2