* [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices
2021-11-20 1:57 [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function David E. Box
@ 2021-11-20 1:57 ` David E. Box
2021-11-30 8:03 ` [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function Krzysztof Wilczyński
1 sibling, 0 replies; 3+ messages in thread
From: David E. Box @ 2021-11-20 1:57 UTC (permalink / raw)
To: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, robh, kw,
bhelgaas, david.e.box, michael.a.bottini, rafael
Cc: linux-pci, linux-kernel
From: Michael Bottini <michael.a.bottini@linux.intel.com>
On Tiger Lake and Alder Lake platforms, VMD controllers do not have ASPM
enabled nor LTR values set by BIOS. This leads high power consumption on
these platforms when VMD is enabled as reported in bugzilla [1]. Enable
these features in the VMD driver using pcie_aspm_policy_override() to set
the ASPM policy for the root ports.
To do this, add an additional flag in VMD features to specify devices that
must have their respective policies overridden.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=213717
Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
drivers/pci/controller/vmd.c | 40 +++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index a45e8e59d3d4..0d0b8888cd90 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -20,6 +20,8 @@
#include <asm/irqdomain.h>
+#include "../pci.h"
+
#define VMD_CFGBAR 0
#define VMD_MEMBAR1 2
#define VMD_MEMBAR2 4
@@ -67,6 +69,12 @@ enum vmd_features {
* interrupt handling.
*/
VMD_FEAT_CAN_BYPASS_MSI_REMAP = (1 << 4),
+
+ /*
+ * Device must have ASPM policy overridden, as its default policy is
+ * incorrect.
+ */
+ VMD_FEAT_QUIRK_OVERRIDE_ASPM = (1 << 5),
};
static DEFINE_IDA(vmd_instance_ida);
@@ -661,6 +669,27 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
return 0;
}
+/*
+ * Override the BIOS ASPM policy and set the LTR value for PCI storage
+ * devices on the VMD bride.
+ */
+int vmd_enable_aspm(struct pci_dev *pdev, void *userdata)
+{
+ int features = *(int *)userdata;
+
+ if (features & VMD_FEAT_QUIRK_OVERRIDE_ASPM &&
+ pdev->class == PCI_CLASS_STORAGE_EXPRESS) {
+ int pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+
+ if (pos) {
+ pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
+ pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+ pcie_aspm_policy_override(pdev);
+ }
+ }
+ return 0;
+}
+
static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
{
struct pci_sysdata *sd = &vmd->sysdata;
@@ -807,6 +836,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
pci_scan_child_bus(vmd->bus);
pci_assign_unassigned_bus_resources(vmd->bus);
+ pci_walk_bus(vmd->bus, vmd_enable_aspm, &features);
+
/*
* VMD root buses are virtual and don't return true on pci_is_pcie()
* and will fail pcie_bus_configure_settings() early. It can instead be
@@ -948,15 +979,18 @@ static const struct pci_device_id vmd_ids[] = {
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x467f),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ VMD_FEAT_OFFSET_FIRST_VECTOR |
+ VMD_FEAT_QUIRK_OVERRIDE_ASPM,},
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x4c3d),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ VMD_FEAT_OFFSET_FIRST_VECTOR |
+ VMD_FEAT_QUIRK_OVERRIDE_ASPM,},
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ VMD_FEAT_OFFSET_FIRST_VECTOR |
+ VMD_FEAT_QUIRK_OVERRIDE_ASPM,},
{0,}
};
MODULE_DEVICE_TABLE(pci, vmd_ids);
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function
2021-11-20 1:57 [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function David E. Box
2021-11-20 1:57 ` [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices David E. Box
@ 2021-11-30 8:03 ` Krzysztof Wilczyński
1 sibling, 0 replies; 3+ messages in thread
From: Krzysztof Wilczyński @ 2021-11-30 8:03 UTC (permalink / raw)
To: David E. Box
Cc: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, robh, bhelgaas,
michael.a.bottini, rafael, linux-pci, linux-kernel
Hi David,
[...]
> @@ -562,11 +562,13 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
> void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> void pcie_aspm_pm_state_change(struct pci_dev *pdev);
> void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
> +void pcie_aspm_policy_override(struct pci_dev *dev);
> #else
> static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
> static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
> static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
> +static inline void pcie_aspm_policy_override(struct pci_dev *dev) {}
> #endif
A small nitpick, and a slight OCD on my part, so feel free to ignore this,
of course: a missing space between curly brackets, to keep things aligned
with previous definitions.
> @@ -1140,6 +1140,24 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
> }
> EXPORT_SYMBOL(pci_disable_link_state);
>
> +void pcie_aspm_policy_override(struct pci_dev *pdev)
> +{
> + struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> +
> + down_read(&pci_bus_sem);
> + mutex_lock(&aspm_lock);
> +
> + if (link) {
> + link->aspm_default = ASPM_STATE_ALL;
> + pcie_config_aspm_link(link, policy_to_aspm_state(link));
> + pcie_set_clkpm(link, policy_to_clkpm_state(link));
> + }
> +
> + mutex_unlock(&aspm_lock);
> + up_read(&pci_bus_sem);
> +}
> +EXPORT_SYMBOL(pcie_aspm_policy_override);
What about the following version where if we have no link (albeit, I am not
sure how often this is going to be the case?) then we don't even attempt to
get a hold on the lock and such:
void pcie_aspm_policy_override(struct pci_dev *pdev)
{
struct pcie_link_state *link = pcie_aspm_get_link(pdev);
if (!link)
return;
down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
link->aspm_default = ASPM_STATE_ALL;
pcie_config_aspm_link(link, policy_to_aspm_state(link));
pcie_set_clkpm(link, policy_to_clkpm_state(link));
mutex_unlock(&aspm_lock);
up_read(&pci_bus_sem);
}
What do you think? Would this make sense?
Krzysztof
^ permalink raw reply [flat|nested] 3+ messages in thread