[PATCH 1/1][U/OEM-5.6] PCI: vmd: Enable ASPM if BIOS requests it
Alex Hung
alex.hung at canonical.com
Tue Aug 4 20:42:52 UTC 2020
On 2020-07-31 2:00 a.m., Colin Ian King wrote:
> On 30/07/2020 11:34, You-Sheng Yang wrote:
>> From: Jon Derrick <jonathan.derrick at intel.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1889384
>>
>> VMD domains are not ACPI-managed devices and do not have the necessary
>> ACPI hooks to enable ASPM. However if the BIOS has requested ASPM
>> enablement, we should try to honor that request regardless. This patch
>> adds the ASPM support to VMD child devices if requested by the FADT
>> table.
>>
>> Signed-off-by: Jon Derrick <jonathan.derrick at intel.com>
>> (cherry picked from
>> https://lore.kernel.org/linux-pci/20200728161321.38229-1-jonathan.derrick@intel.com/)
>> Signed-off-by: You-Sheng Yang <vicamo.yang at canonical.com>
>> ---
>> drivers/pci/controller/vmd.c | 9 ++++++++-
>> drivers/pci/pcie/aspm.c | 19 ++-----------------
>> include/linux/pci.h | 17 +++++++++++++++++
>> 3 files changed, 27 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index 9a64cf90c291..60a9373d3360 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -14,6 +14,7 @@
>> #include <linux/srcu.h>
>> #include <linux/rculist.h>
>> #include <linux/rcupdate.h>
>> +#include <linux/acpi.h>
>>
>> #include <asm/irqdomain.h>
>> #include <asm/device.h>
>> @@ -574,8 +575,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>> * and will fail pcie_bus_configure_settings() early. It can instead be
>> * run on each of the real root ports.
>> */
>> - list_for_each_entry(child, &vmd->bus->children, node)
>> + list_for_each_entry(child, &vmd->bus->children, node) {
>> +#if IS_ENABLED(CONFIG_PCIEASPM)
>> + if (!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM))
>> + pcie_config_aspm_link(child->self->link_state,
>> + ASPM_STATE_ALL);
>> +#endif
>> pcie_bus_configure_settings(child);
>> + }
Technical speaking, ACPI FADT would never ask OS to enable ASPM. FADT's
flag is defined as
==================================
PCIe ASPM Controls - If set, indicates to OSPM that it must not enable
OSPM ASPM control on this platform.
==================================
This means
1) [NORMAL] when clear, no known ASPM compatibility issues and OS can
enable or disable ASPM as needed.
2) [WORKAROUND] when set, known devices (usually bundled hardware
devices) has compatibility issues with ASPM. For example, a device can
fail with L1 even if the devices claims it supports L1. As a results,
ACPI BIOS tells OS not to touch ASPM settings after BIOS configured it.
>>
>> pci_bus_add_devices(vmd->bus);
>>
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index b17e5ffd31b1..23a3fc82364f 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -25,22 +25,6 @@
>> #endif
>> #define MODULE_PARAM_PREFIX "pcie_aspm."
>>
>> -/* Note: those are not register definitions */
>> -#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */
>> -#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */
>> -#define ASPM_STATE_L1 (4) /* L1 state */
>> -#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */
>> -#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */
>> -#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */
>> -#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */
>> -#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM)
>> -#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM)
>> -#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\
>> - ASPM_STATE_L1_2_MASK)
>> -#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW)
>> -#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \
>> - ASPM_STATE_L1SS)
>> -
>> struct aspm_latency {
>> u32 l0s; /* L0s latency (nsec) */
>> u32 l1; /* L1 latency (nsec) */
>> @@ -748,7 +732,7 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
>> PCI_EXP_LNKCTL_ASPMC, val);
>> }
>>
>> -static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>> +void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>> {
>> u32 upstream = 0, dwstream = 0;
>> struct pci_dev *child = link->downstream, *parent = link->pdev;
>> @@ -798,6 +782,7 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>>
>> link->aspm_enabled = state;
>> }
>> +EXPORT_SYMBOL_GPL(pcie_config_aspm_link);
>>
>> static void pcie_config_aspm_path(struct pcie_link_state *link)
>> {
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 34c1c4f45288..ec0a8b7a55f8 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -377,6 +377,22 @@ struct pci_dev {
>> unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */
>>
>> #ifdef CONFIG_PCIEASPM
>> +/* Note: those are not register definitions */
>> +#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */
>> +#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */
>> +#define ASPM_STATE_L1 (4) /* L1 state */
>> +#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */
>> +#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */
>> +#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */
>> +#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */
>> +#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM)
>> +#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM)
>> +#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\
>> + ASPM_STATE_L1_2_MASK)
>> +#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW)
>> +#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \
>> + ASPM_STATE_L1SS)
>> +
>> struct pcie_link_state *link_state; /* ASPM link state */
>> unsigned int ltr_path:1; /* Latency Tolerance Reporting
>> supported from root to here */
>> @@ -1570,6 +1586,7 @@ extern bool pcie_ports_native;
>> #define PCIE_LINK_STATE_L1_2_PCIPM BIT(6)
>>
>> #ifdef CONFIG_PCIEASPM
>> +void pcie_config_aspm_link(struct pcie_link_state *link, u32 state);
>> int pci_disable_link_state(struct pci_dev *pdev, int state);
>> int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
>> void pcie_no_aspm(void);
>>
>
> The patch originated from:
> https://lore.kernel.org/linux-pci/20200728161321.38229-1-jonathan.derrick@intel.com/
> - it is labeled as a RFC (request for comment). Also it contains the
> text after the SoBs:
>
>
> "Hi,
>
> My knowledge on these kinds of power modes is limited, and we are having
> trouble bringing the Root Port child device out of L1 with this patch.
>
> Can you help me understand the correct flow for bringing the Root Port
> device out of L1 with kernel flow, and what I might be missing here?"
>
> ..this makes me concerned that this need some time to be properly
> reviewed. As it stands, my knowledge of ASPM is that the BIOS can lie
> about ASPM being allowed, so I'm concerned about power state regressions
> with this.
and a good example is b361663c5a40c in kernel 5.8 where a piece of
hardware says it supports ASPM but its document says otherwise.
>
> Colin
>
More information about the kernel-team
mailing list