From patchwork Wed Jun 5 14:58:18 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mika Westerberg X-Patchwork-Id: 10977157 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 1BEF013AD for ; Wed, 5 Jun 2019 14:58:29 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0B97C2860C for ; Wed, 5 Jun 2019 14:58:29 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F3B73285FB; Wed, 5 Jun 2019 14:58:28 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B4D0428786 for ; Wed, 5 Jun 2019 14:58:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728520AbfFEO60 (ORCPT ); Wed, 5 Jun 2019 10:58:26 -0400 Received: from mga07.intel.com ([134.134.136.100]:27458 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728448AbfFEO60 (ORCPT ); Wed, 5 Jun 2019 10:58:26 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jun 2019 07:58:25 -0700 X-ExtLoop1: 1 Received: from black.fi.intel.com ([10.237.72.28]) by orsmga003.jf.intel.com with ESMTP; 05 Jun 2019 07:58:22 -0700 Received: by black.fi.intel.com (Postfix, from userid 1001) id 43C6A15A; Wed, 5 Jun 2019 17:58:21 +0300 (EEST) From: Mika Westerberg To: Bjorn Helgaas , "Rafael J. Wysocki" Cc: Len Brown , Lukas Wunner , Keith Busch , Alex Williamson , Alexandru Gagniuc , Mika Westerberg , linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org Subject: [PATCH 1/3] PCI: Add missing link delays required by the PCIe spec Date: Wed, 5 Jun 2019 17:58:18 +0300 Message-Id: <20190605145820.37169-2-mika.westerberg@linux.intel.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20190605145820.37169-1-mika.westerberg@linux.intel.com> References: <20190605145820.37169-1-mika.westerberg@linux.intel.com> MIME-Version: 1.0 Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Currently Linux does not follow PCIe spec regarding the required delays after reset. A concrete example is a Thunderbolt add-in-card that consists of a PCIe switch and two PCIe endpoints: +-1b.0-[01-6b]----00.0-[02-6b]--+-00.0-[03]----00.0 TBT controller +-01.0-[04-36]-- DS hotplug port +-02.0-[37]----00.0 xHCI controller \-04.0-[38-6b]-- DS hotplug port The root port (1b.0) and the PCIe switch downstream ports are all PCIe gen3 so they support 8GT/s link speeds. We wait for the PCIe hierarchy to enter D3cold (runtime): pcieport 0000:00:1b.0: power state changed by ACPI to D3cold When it wakes up from D3cold, according to the PCIe 4.0 section 5.8 the PCIe switch is put to reset and its power is re-applied. This means that we must follow the rules in PCIe 4.0 section 6.6.1. For the PCIe gen3 ports we are dealing with here, the following applies: With a Downstream Port that supports Link speeds greater than 5.0 GT/s, software must wait a minimum of 100 ms after Link training completes before sending a Configuration Request to the device immediately below that Port. Software can determine when Link training completes by polling the Data Link Layer Link Active bit or by setting up an associated interrupt (see Section 6.7.3.3). Translating this into the above topology we would need to do this (DLLLA stands for Data Link Layer Link Active): pcieport 0000:00:1b.0: wait for 100ms after DLLLA is set before access to 0000:01:00.0 pcieport 0000:02:00.0: wait for 100ms after DLLLA is set before access to 0000:03:00.0 pcieport 0000:02:02.0: wait for 100ms after DLLLA is set before access to 0000:37:00.0 I've instrumented the kernel with additional logging so we can see the actual delays the kernel performs: pcieport 0000:00:1b.0: power state changed by ACPI to D0 pcieport 0000:00:1b.0: waiting for D3cold delay of 100 ms pcieport 0000:00:1b.0: waking up bus pcieport 0000:00:1b.0: waiting for D3hot delay of 10 ms pcieport 0000:00:1b.0: restoring config space at offset 0x2c (was 0x60, writing 0x60) ... pcieport 0000:00:1b.0: PME# disabled pcieport 0000:01:00.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) ... pcieport 0000:01:00.0: PME# disabled pcieport 0000:02:00.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) ... pcieport 0000:02:00.0: PME# disabled pcieport 0000:02:01.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) ... pcieport 0000:02:01.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) pcieport 0000:02:01.0: PME# disabled pcieport 0000:02:02.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) ... pcieport 0000:02:02.0: PME# disabled pcieport 0000:02:04.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) ... pcieport 0000:02:04.0: PME# disabled pcieport 0000:02:01.0: PME# enabled pcieport 0000:02:01.0: waiting for D3hot delay of 10 ms pcieport 0000:02:04.0: PME# enabled pcieport 0000:02:04.0: waiting for D3hot delay of 10 ms thunderbolt 0000:03:00.0: restoring config space at offset 0x14 (was 0x0, writing 0x8a040000) ... thunderbolt 0000:03:00.0: PME# disabled xhci_hcd 0000:37:00.0: restoring config space at offset 0x10 (was 0x0, writing 0x73f00000) ... xhci_hcd 0000:37:00.0: PME# disabled For the switch upstream port (01:00.0) we wait for 100ms but not taking into account the DLLLA requirement. We then wait 10ms for D3hot -> D0 transition of the root port and the two downstream hotplug ports. This means that we deviate from what the spec requires. Performing the same check for system sleep (s2idle) transitions we can see following when resuming from s2idle: pcieport 0000:00:1b.0: power state changed by ACPI to D0 pcieport 0000:00:1b.0: restoring config space at offset 0x2c (was 0x60, writing 0x60) ... pcieport 0000:01:00.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) ... pcieport 0000:02:02.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) pcieport 0000:02:02.0: restoring config space at offset 0x2c (was 0x0, writing 0x0) pcieport 0000:02:01.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) pcieport 0000:02:04.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) pcieport 0000:02:02.0: restoring config space at offset 0x28 (was 0x0, writing 0x0) pcieport 0000:02:00.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) pcieport 0000:02:02.0: restoring config space at offset 0x24 (was 0x10001, writing 0x1fff1) pcieport 0000:02:01.0: restoring config space at offset 0x2c (was 0x0, writing 0x60) pcieport 0000:02:02.0: restoring config space at offset 0x20 (was 0x0, writing 0x73f073f0) pcieport 0000:02:04.0: restoring config space at offset 0x2c (was 0x0, writing 0x60) pcieport 0000:02:01.0: restoring config space at offset 0x28 (was 0x0, writing 0x60) pcieport 0000:02:00.0: restoring config space at offset 0x2c (was 0x0, writing 0x0) pcieport 0000:02:02.0: restoring config space at offset 0x1c (was 0x101, writing 0x1f1) pcieport 0000:02:04.0: restoring config space at offset 0x28 (was 0x0, writing 0x60) pcieport 0000:02:01.0: restoring config space at offset 0x24 (was 0x10001, writing 0x1ff10001) pcieport 0000:02:00.0: restoring config space at offset 0x28 (was 0x0, writing 0x0) pcieport 0000:02:02.0: restoring config space at offset 0x18 (was 0x0, writing 0x373702) pcieport 0000:02:04.0: restoring config space at offset 0x24 (was 0x10001, writing 0x49f12001) pcieport 0000:02:01.0: restoring config space at offset 0x20 (was 0x0, writing 0x73e05c00) pcieport 0000:02:00.0: restoring config space at offset 0x24 (was 0x10001, writing 0x1fff1) pcieport 0000:02:04.0: restoring config space at offset 0x20 (was 0x0, writing 0x89f07400) pcieport 0000:02:01.0: restoring config space at offset 0x1c (was 0x101, writing 0x5151) pcieport 0000:02:00.0: restoring config space at offset 0x20 (was 0x0, writing 0x8a008a00) pcieport 0000:02:02.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020) pcieport 0000:02:04.0: restoring config space at offset 0x1c (was 0x101, writing 0x6161) pcieport 0000:02:01.0: restoring config space at offset 0x18 (was 0x0, writing 0x360402) pcieport 0000:02:00.0: restoring config space at offset 0x1c (was 0x101, writing 0x1f1) pcieport 0000:02:04.0: restoring config space at offset 0x18 (was 0x0, writing 0x6b3802) pcieport 0000:02:02.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) pcieport 0000:02:00.0: restoring config space at offset 0x18 (was 0x0, writing 0x30302) pcieport 0000:02:01.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020) pcieport 0000:02:04.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020) pcieport 0000:02:00.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020) pcieport 0000:02:01.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) pcieport 0000:02:04.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) pcieport 0000:02:00.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) xhci_hcd 0000:37:00.0: restoring config space at offset 0x10 (was 0x0, writing 0x73f00000) ... thunderbolt 0000:03:00.0: restoring config space at offset 0x14 (was 0x0, writing 0x8a040000) This is even worse. None of the mandatory delays are performed. If this would be S3 instead of s2idle then according to PCI FW spec 3.2 section 4.6.8. there is a specific _DSM that allows the OS to skip the delays but this platform does not provide the _DSM and does not go to S3 anyway so no firmware is involved that could already handle these delays. In this particular Intel Coffee Lake platform these delays are not actually needed because there is an additional delay as part of the ACPI power resource that is used to turn on power to the hierarchy but since that additional delay is not required by any of standards (PCIe, ACPI) it is not present in the Intel Ice Lake, for example where missing the mandatory delays causes pciehp to start tearing down the stack too early (links are not yet trained). For this reason, change the PCIe portdrv PM resume hooks so that they perform the mandatory delays before the downstream component gets resumed. We perform the delays before port services are resumed because otherwise pciehp might find that the link is not up (even if it is just training) and tears-down the hierarchy. Signed-off-by: Mika Westerberg Reviewed-by: Rafael J. Wysocki --- drivers/pci/pci.c | 29 +++++++++------ drivers/pci/pci.h | 1 + drivers/pci/pcie/portdrv_core.c | 62 +++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 10 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 8abc843b1615..87a1f902fa8e 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1004,15 +1004,10 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state) if (state == PCI_D0) { pci_platform_power_transition(dev, PCI_D0); /* - * Mandatory power management transition delays, see - * PCI Express Base Specification Revision 2.0 Section - * 6.6.1: Conventional Reset. Do not delay for - * devices powered on/off by corresponding bridge, - * because have already delayed for the bridge. + * Mandatory power management transition delays are + * handled in the PCIe portdrv resume hooks. */ if (dev->runtime_d3cold) { - if (dev->d3cold_delay && !dev->imm_ready) - msleep(dev->d3cold_delay); /* * When powering on a bridge from D3cold, the * whole hierarchy may be powered on into @@ -4568,14 +4563,16 @@ static int pci_pm_reset(struct pci_dev *dev, int probe) return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS); } + /** - * pcie_wait_for_link - Wait until link is active or inactive + * pcie_wait_for_link_delay - Wait until link is active or inactive * @pdev: Bridge device * @active: waiting for active or inactive? + * @delay: Delay to wait after link has become active (in ms) * * Use this to wait till link becomes active or inactive. */ -bool pcie_wait_for_link(struct pci_dev *pdev, bool active) +bool pcie_wait_for_link_delay(struct pci_dev *pdev, bool active, int delay) { int timeout = 1000; bool ret; @@ -4612,13 +4609,25 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active) timeout -= 10; } if (active && ret) - msleep(100); + msleep(delay); else if (ret != active) pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n", active ? "set" : "cleared"); return ret == active; } +/** + * pcie_wait_for_link - Wait until link is active or inactive + * @pdev: Bridge device + * @active: waiting for active or inactive? + * + * Use this to wait till link becomes active or inactive. + */ +bool pcie_wait_for_link(struct pci_dev *pdev, bool active) +{ + return pcie_wait_for_link_delay(pdev, active, 100); +} + void pci_reset_secondary_bus(struct pci_dev *dev) { u16 ctrl; diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 9cb99380c61e..59802b3def4b 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -493,6 +493,7 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev) void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, u32 service); +bool pcie_wait_for_link_delay(struct pci_dev *pdev, bool active, int delay); bool pcie_wait_for_link(struct pci_dev *pdev, bool active); #ifdef CONFIG_PCIEASPM void pcie_aspm_init_link_state(struct pci_dev *pdev); diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 1b330129089f..88d151a54be6 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -378,6 +379,58 @@ static int pm_iter(struct device *dev, void *data) return 0; } +static int get_downstream_delay(struct pci_bus *bus) +{ + struct pci_dev *pdev; + int min_delay = 100; + int max_delay = 0; + + list_for_each_entry(pdev, &bus->devices, bus_list) { + if (!pdev->imm_ready) + min_delay = 0; + else if (pdev->d3cold_delay < min_delay) + min_delay = pdev->d3cold_delay; + if (pdev->d3cold_delay > max_delay) + max_delay = pdev->d3cold_delay; + } + + return max(min_delay, max_delay); +} + +static void wait_for_downstream_link(struct pci_dev *pdev) +{ + /* + * Handle delays according to PCIe 4.0 section 6.6.1 before + * configuration access to the downstream component is permitted. + * + * This blocks PCI core resume of the hierarchy below this port + * until the link is trained. + */ + if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || + pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) && + pdev->subordinate && !list_empty(&pdev->subordinate->devices) && + pdev->bridge_d3 && !pci_dev_is_disconnected(pdev)) { + int delay; + + delay = get_downstream_delay(pdev->subordinate); + if (!delay) + return; + + dev_dbg(&pdev->dev, "waiting downstream link for %d ms\n", delay); + + /* + * If downstream port does not support speeds greater than + * 5 GT/s need to wait 100ms. For higher speeds (gen3) we + * need to wait first for the data link layer to become + * active. + */ + if (pcie_get_speed_cap(pdev) <= PCIE_SPEED_5_0GT) + msleep(delay); + else + pcie_wait_for_link_delay(pdev, true, delay); + } +} + /** * pcie_port_device_suspend - suspend port services associated with a PCIe port * @dev: PCI Express port to handle @@ -391,6 +444,13 @@ int pcie_port_device_suspend(struct device *dev) int pcie_port_device_resume_noirq(struct device *dev) { size_t off = offsetof(struct pcie_port_service_driver, resume_noirq); + + /* + * Wait for the link to be fully up before resuming port services. + * This prevents pciehp from starting to tear-down the hierarchy + * too soon. + */ + wait_for_downstream_link(to_pci_dev(dev)); return device_for_each_child(dev, &off, pm_iter); } @@ -421,6 +481,8 @@ int pcie_port_device_runtime_suspend(struct device *dev) int pcie_port_device_runtime_resume(struct device *dev) { size_t off = offsetof(struct pcie_port_service_driver, runtime_resume); + + wait_for_downstream_link(to_pci_dev(dev)); return device_for_each_child(dev, &off, pm_iter); } #endif /* PM */ From patchwork Wed Jun 5 14:58:19 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mika Westerberg X-Patchwork-Id: 10977149 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 0356F13AD for ; Wed, 5 Jun 2019 14:58:27 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E95DF205A8 for ; Wed, 5 Jun 2019 14:58:26 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DD65628970; Wed, 5 Jun 2019 14:58:26 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7EFA4205A8 for ; Wed, 5 Jun 2019 14:58:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728493AbfFEO6Z (ORCPT ); Wed, 5 Jun 2019 10:58:25 -0400 Received: from mga02.intel.com ([134.134.136.20]:16397 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728448AbfFEO6Z (ORCPT ); Wed, 5 Jun 2019 10:58:25 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jun 2019 07:58:24 -0700 X-ExtLoop1: 1 Received: from black.fi.intel.com ([10.237.72.28]) by fmsmga006.fm.intel.com with ESMTP; 05 Jun 2019 07:58:22 -0700 Received: by black.fi.intel.com (Postfix, from userid 1001) id 5193C462; Wed, 5 Jun 2019 17:58:21 +0300 (EEST) From: Mika Westerberg To: Bjorn Helgaas , "Rafael J. Wysocki" Cc: Len Brown , Lukas Wunner , Keith Busch , Alex Williamson , Alexandru Gagniuc , Mika Westerberg , linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org Subject: [PATCH 2/3] PCI: Do not poll for PME if the device is in D3cold Date: Wed, 5 Jun 2019 17:58:19 +0300 Message-Id: <20190605145820.37169-3-mika.westerberg@linux.intel.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20190605145820.37169-1-mika.westerberg@linux.intel.com> References: <20190605145820.37169-1-mika.westerberg@linux.intel.com> MIME-Version: 1.0 Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP PME polling does not take into account that a device that is directly connected to the host bridge may go into D3cold as well. This leads to a situation where the PME poll thread reads from a config space of a device that is in D3cold and gets incorrect information because the config space is not accessible. Here is an example from Intel Ice Lake system where two PCIe root ports are in D3cold (I've instrumented the kernel to log the PMCSR register contents): [ 62.971442] pcieport 0000:00:07.1: Check PME status, PMCSR=0xffff [ 62.971504] pcieport 0000:00:07.0: Check PME status, PMCSR=0xffff Since 0xffff is interpreted so that PME is pending, the root ports will be runtime resumed. This repeats over and over again essentially blocking all runtime power management. Prevent this from happening by checking whether the device is in D3cold before its PME status is read. Signed-off-by: Mika Westerberg Signed-off-by: Lukas Wunner Reviewed-by: Lukas Wunner Reviewed-by: Rafael J. Wysocki --- drivers/pci/pci.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 87a1f902fa8e..720da09d4d73 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2060,6 +2060,13 @@ static void pci_pme_list_scan(struct work_struct *work) */ if (bridge && bridge->current_state != PCI_D0) continue; + /* + * If the device is in D3cold it should not be + * polled either. + */ + if (pme_dev->dev->current_state == PCI_D3cold) + continue; + pci_pme_wakeup(pme_dev->dev, NULL); } else { list_del(&pme_dev->list); From patchwork Wed Jun 5 14:58:20 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mika Westerberg X-Patchwork-Id: 10977155 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 4CB263A3F for ; Wed, 5 Jun 2019 14:58:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3DB5C205A8 for ; Wed, 5 Jun 2019 14:58:28 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 30AC628970; Wed, 5 Jun 2019 14:58:28 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 60D7B285FB for ; Wed, 5 Jun 2019 14:58:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728500AbfFEO60 (ORCPT ); Wed, 5 Jun 2019 10:58:26 -0400 Received: from mga07.intel.com ([134.134.136.100]:27458 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728469AbfFEO60 (ORCPT ); Wed, 5 Jun 2019 10:58:26 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jun 2019 07:58:25 -0700 X-ExtLoop1: 1 Received: from black.fi.intel.com ([10.237.72.28]) by orsmga008.jf.intel.com with ESMTP; 05 Jun 2019 07:58:22 -0700 Received: by black.fi.intel.com (Postfix, from userid 1001) id 61E854E1; Wed, 5 Jun 2019 17:58:21 +0300 (EEST) From: Mika Westerberg To: Bjorn Helgaas , "Rafael J. Wysocki" Cc: Len Brown , Lukas Wunner , Keith Busch , Alex Williamson , Alexandru Gagniuc , Mika Westerberg , linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org Subject: [PATCH 3/3] PCI / ACPI: Handle sibling devices sharing power resources Date: Wed, 5 Jun 2019 17:58:20 +0300 Message-Id: <20190605145820.37169-4-mika.westerberg@linux.intel.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20190605145820.37169-1-mika.westerberg@linux.intel.com> References: <20190605145820.37169-1-mika.westerberg@linux.intel.com> MIME-Version: 1.0 Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Intel Ice Lake has an interated Thunderbolt controller which means that the PCIe topology is extended directly from the two root ports (RP0 and RP1). Power management is handled by ACPI power resources that are shared between the root ports, Thunderbolt controller (NHI) and xHCI controller. The topology with the power resources (marked with []) looks like: Host bridge | +- RP0 ---\ +- RP1 ---|--+--> [TBT] +- NHI --/ | | | | v +- xHCI --> [D3C] Here TBT and D3C are the shared ACPI power resources. ACPI _PR3() method returns either TBT or D3C or both. Say we runtime suspend first the root ports RP0 and RP1, then NHI. Now since the TBT power resource is still on when the root ports are runtime suspended their dev->current_state is set to D3hot. When NHI is runtime suspended TBT is finally turned off but state of the root ports remain to be D3hot. If the user now runs lspci for instance, the result is all 1's like in the below output (07.0 is the first root port, RP0): # lspci -vv -s 07.0 00:07.0 PCI bridge: Intel Corporation Device 8a1d (rev ff) (prog-if ff) !!! Unknown header type 7f Kernel driver in use: pcieport I short the hardware state is not in sync with the software state anymore. The exact same thing happens with the PME polling thread which ends up bringing the root ports back into D0 after they are runtime suspended. There is another issue that happens when the power resource(s) are turned on. Assume the Thunderbolt controller is runtime resumed and the power resources are turned on which means that the other devices sharing them (RP0, RP1 and xHCI) are transitioned into D0uninitialized state. If they were configured to trigger wake (PME) on certain event that configuration gets lost after reset so we would need to re-initialize them to get the wakeup working as expected again. This means that we would need to runtime resume all of them to make sure their registers get restored properly before we can runtime suspend them again. Prevent this from happening by asking from the platform (ACPI) if there are other devices sharing the same power source when device power is turned on/off through platform. If we find such devices we update their dev->current_state (in case of D3hot -> D3cold transition) or runtime resume them (in case of D3cold -> D0uninitialized). Signed-off-by: Mika Westerberg --- drivers/acpi/power.c | 32 ++++++++++++ drivers/pci/pci-acpi.c | 32 ++++++++++-- drivers/pci/pci.c | 106 ++++++++++++++++++++++++++++++---------- drivers/pci/pci.h | 5 ++ include/acpi/acpi_bus.h | 3 ++ 5 files changed, 146 insertions(+), 32 deletions(-) diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c index a916417b9e70..9d244fde05a4 100644 --- a/drivers/acpi/power.c +++ b/drivers/acpi/power.c @@ -739,6 +739,38 @@ int acpi_power_transition(struct acpi_device *device, int state) return result; } +/** + * acpi_shared_power_resource - Do given devices share power resource + * @adev1: First device to check + * @adev2: Second device to check + * @state: State which power resources are looked + * + * Checks if given devices share one or more power resources and in that + * case returns true. + */ +bool acpi_shared_power_resource(struct acpi_device *adev1, struct acpi_device *adev2, + int state) +{ + struct acpi_power_resource_entry *e1, *e2; + struct list_head *l1, *l2; + + /* Both need to have power resources */ + if (!adev1->power.flags.power_resources || + !adev2->power.flags.power_resources) + return false; + + l1 = &adev1->power.states[state].resources; + l2 = &adev2->power.states[state].resources; + + list_for_each_entry(e1, l1, node) + list_for_each_entry(e2, l2, node) { + if (e1->resource == e2->resource) + return true; + } + + return false; +} + static void acpi_release_power_resource(struct device *dev) { struct acpi_device *device = to_acpi_device(dev); diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 1897847ceb0c..39112e684403 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -632,16 +632,23 @@ static bool acpi_pci_power_manageable(struct pci_dev *dev) return adev ? acpi_device_power_manageable(adev) : false; } -static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) +static const u8 pci_power_to_acpi(pci_power_t state) { - struct acpi_device *adev = ACPI_COMPANION(&dev->dev); - static const u8 state_conv[] = { + static const int state_conv[] = { [PCI_D0] = ACPI_STATE_D0, [PCI_D1] = ACPI_STATE_D1, [PCI_D2] = ACPI_STATE_D2, [PCI_D3hot] = ACPI_STATE_D3_HOT, [PCI_D3cold] = ACPI_STATE_D3_COLD, }; + + return state_conv[state]; +} + +static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) +{ + struct acpi_device *adev = ACPI_COMPANION(&dev->dev); + int acpi_state = pci_power_to_acpi(state); int error = -EINVAL; /* If the ACPI device has _EJ0, ignore the device */ @@ -660,12 +667,12 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state) case PCI_D1: case PCI_D2: case PCI_D3hot: - error = acpi_device_set_power(adev, state_conv[state]); + error = acpi_device_set_power(adev, acpi_state); } if (!error) pci_dbg(dev, "power state changed by ACPI to %s\n", - acpi_power_state_string(state_conv[state])); + acpi_power_state_string(acpi_state)); return error; } @@ -743,6 +750,20 @@ static bool acpi_pci_need_resume(struct pci_dev *dev) return !!adev->power.flags.dsw_present; } +static bool acpi_pci_shared_power(struct pci_dev *dev1, struct pci_dev *dev2, + pci_power_t state) +{ + struct acpi_device *adev1 = ACPI_COMPANION(&dev1->dev); + struct acpi_device *adev2 = ACPI_COMPANION(&dev2->dev); + + if (!adev1 || !acpi_device_power_manageable(adev1)) + return false; + if (!adev2 || !acpi_device_power_manageable(adev2)) + return false; + + return acpi_shared_power_resource(adev1, adev2, pci_power_to_acpi(state)); +} + static const struct pci_platform_pm_ops acpi_pci_platform_pm = { .bridge_d3 = acpi_pci_bridge_d3, .is_manageable = acpi_pci_power_manageable, @@ -751,6 +772,7 @@ static const struct pci_platform_pm_ops acpi_pci_platform_pm = { .choose_state = acpi_pci_choose_state, .set_wakeup = acpi_pci_wakeup, .need_resume = acpi_pci_need_resume, + .shared_power = acpi_pci_shared_power, }; void acpi_pci_add_bus(struct pci_bus *bus) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 720da09d4d73..224689116df3 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -755,7 +755,8 @@ static const struct pci_platform_pm_ops *pci_platform_pm; int pci_set_platform_pm(const struct pci_platform_pm_ops *ops) { if (!ops->is_manageable || !ops->set_state || !ops->get_state || - !ops->choose_state || !ops->set_wakeup || !ops->need_resume) + !ops->choose_state || !ops->set_wakeup || !ops->need_resume || + !ops->bridge_d3 || !ops->shared_power) return -EINVAL; pci_platform_pm = ops; return 0; @@ -799,6 +800,12 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev) return pci_platform_pm ? pci_platform_pm->bridge_d3(dev) : false; } +static inline bool platform_pci_shared_power(struct pci_dev *dev1, struct pci_dev *dev2, + pci_power_t t) +{ + return pci_platform_pm ? pci_platform_pm->shared_power(dev1, dev2, t) : false; +} + /** * pci_raw_set_power_state - Use PCI PM registers to set the power state of * given PCI device @@ -994,31 +1001,6 @@ void pci_wakeup_bus(struct pci_bus *bus) pci_walk_bus(bus, pci_wakeup, NULL); } -/** - * __pci_start_power_transition - Start power transition of a PCI device - * @dev: PCI device to handle. - * @state: State to put the device into. - */ -static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state) -{ - if (state == PCI_D0) { - pci_platform_power_transition(dev, PCI_D0); - /* - * Mandatory power management transition delays are - * handled in the PCIe portdrv resume hooks. - */ - if (dev->runtime_d3cold) { - /* - * When powering on a bridge from D3cold, the - * whole hierarchy may be powered on into - * D0uninitialized state, resume them to give - * them a chance to suspend again - */ - pci_wakeup_bus(dev->subordinate); - } - } -} - /** * __pci_dev_set_current_state - Set current state of a PCI device * @dev: Device to handle @@ -1043,6 +1025,76 @@ void pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state) pci_walk_bus(bus, __pci_dev_set_current_state, &state); } + +static void pci_update_topology_power_state(struct pci_dev *dev, pci_power_t state) +{ + struct pci_dev *p = NULL; + + if (!platform_pci_power_manageable(dev)) + return; + + if (state == PCI_D3cold) { + /* The topology below is in D3cold as well */ + pci_bus_set_current_state(dev->subordinate, PCI_D3cold); + /* + * Find other devices that were in PCI_D3hot and now are in + * D3cold that share the power resource. + */ + for_each_pci_dev(p) { + if (p == dev || !platform_pci_power_manageable(p)) + continue; + + if (platform_pci_shared_power(dev, p, PCI_D3hot) && + p->current_state == PCI_D3hot && + platform_pci_get_power_state(p) == PCI_D3cold) { + dev_dbg(&p->dev, + "transition into D3cold because power turned off\n"); + p->current_state = PCI_D3cold; + pci_bus_set_current_state(p->subordinate, PCI_D3cold); + } + } + } else if (state == PCI_D0) { + /* + * When powering on a bridge from D3cold, the whole + * hierarchy may be powered on into D0uninitialized state, + * resume them to give them a chance to suspend again. + */ + pci_wakeup_bus(dev->subordinate); + /* + * Find other devices that were in PCI_D3cold and now are + * in D0uninitialized because of the shared power resource + * and resume them now so that they get properly + * re-initialized. + */ + for_each_pci_dev(p) { + if (p == dev || !platform_pci_power_manageable(p)) + continue; + + if (platform_pci_shared_power(dev, p, PCI_D0) && + p->current_state == PCI_D3cold && + platform_pci_get_power_state(p) == PCI_D0) { + dev_dbg(&p->dev, "waking up because power turned on\n"); + pci_wakeup(p, NULL); + pci_wakeup_bus(p->subordinate); + } + } + } +} + +/** + * __pci_start_power_transition - Start power transition of a PCI device + * @dev: PCI device to handle. + * @state: State to put the device into. + */ +static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state) +{ + if (state == PCI_D0) { + pci_platform_power_transition(dev, PCI_D0); + if (dev->runtime_d3cold) + pci_update_topology_power_state(dev, PCI_D0); + } +} + /** * __pci_complete_power_transition - Complete power transition of a PCI device * @dev: PCI device to handle. @@ -1059,7 +1111,7 @@ int __pci_complete_power_transition(struct pci_dev *dev, pci_power_t state) ret = pci_platform_power_transition(dev, state); /* Power off the bridge may power off the whole hierarchy */ if (!ret && state == PCI_D3cold) - pci_bus_set_current_state(dev->subordinate, PCI_D3cold); + pci_update_topology_power_state(dev, PCI_D3cold); return ret; } EXPORT_SYMBOL_GPL(__pci_complete_power_transition); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 59802b3def4b..855c9a2f3079 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -61,6 +61,9 @@ int pci_bus_error_reset(struct pci_dev *dev); * suspended) needs to be resumed to be configured for system * wakeup. * + * @shared_power: Returns 'true' if the two PCI devices share power source + * such as ACPI power resource. + * * If given platform is generally capable of power managing PCI devices, all of * these callbacks are mandatory. */ @@ -72,6 +75,8 @@ struct pci_platform_pm_ops { 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); + bool (*shared_power)(struct pci_dev *dev1, struct pci_dev *dev2, + pci_power_t state); }; int pci_set_platform_pm(const struct pci_platform_pm_ops *ops); diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 31b6c87d6240..d70dbbdb5164 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -597,6 +597,9 @@ struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle); int acpi_enable_wakeup_device_power(struct acpi_device *dev, int state); int acpi_disable_wakeup_device_power(struct acpi_device *dev); +bool acpi_shared_power_resource(struct acpi_device *adev1, + struct acpi_device *adev2, int state); + #ifdef CONFIG_X86 bool acpi_device_always_present(struct acpi_device *adev); #else