[PATCH V6 0/3] PCI: vmd: Enable PCIe ASPM and LTR

[PATCH V6 0/3] PCI: vmd: Enable PCIe ASPM and LTR

[David E. Box]

This series adds support for enabling PCIe ASPM and for setting PCIe LTR
values on devices on root ports reserved by VMD. Configuration of these
capabilities is usually done by BIOS. But for VMD ports these capabilities
will not be configured because those ports are not visible to BIOS. For
future products, post Alder Lake, the hardware team has agreed to do this
enabling in BIOS.  But this will not apply to current products, so this
work around is provided for them. Without this, laptops running in VMD mode
will not be able to power gate roots ports, resulting in higher power
consumption.

Since V4 we have more information from the BIOS team as to why BIOS
needs to program device LTRs. This is something that should be done by
devices, but there are many that don't provide LTR values causing them
to block SoC level power management. BIOS sets an initial default LTR to
account for such devices. This SoC specific value is the maximum latency
required to allow the SoC to enter the deepest power state.

David E. Box (2):
  PCI: vmd: Add vmd_device_data
  PCI: vmd: Configure PCIe ASPM and LTR

Michael Bottini (1):
  PCI/ASPM: Add ASPM BIOS override function

 drivers/pci/controller/vmd.c | 134 ++++++++++++++++++++++++++++-------
 drivers/pci/pcie/aspm.c      |  54 ++++++++++++++
 include/linux/pci.h          |   7 ++
 3 files changed, 169 insertions(+), 26 deletions(-)


base-commit: 754e0b0e35608ed5206d6a67a791563c631cec07
-- 
2.25.1

[PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state()

[David E. Box]

From: Michael Bottini <michael.a.bottini@linux.intel.com>

Add pci_enable_default_link_state() to allow devices to change the default
BIOS configured states. Clears the BIOS default settings then sets the new
states and reconfigures the link under the semaphore. Also add
PCIE_LINK_STATE_ALL macro for convenience for callers that want to enable
all link states.

Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 V6
  - No change
 V5
  - Rename to pci_enable_default_link_state and model after
    pci_disable_link_state() as suggested by Bjorn.
  - Add helper PCIE_LINK_STATE_ALL which sets bits for all links states and
    clock pm.
  - Clarify commit language to indicate the function changes the default
    link states (not ASPM policy).
 V4
  - Refactor vmd_enable_apsm() to exit early, making the lines shorter
    and more readable. Suggested by Christoph.
 V3
  - No changes
 V2
  - Use return status to print pci_info message if ASPM cannot be enabled.
  - Add missing static declaration, caught by lkp@intel.com
 

 drivers/pci/pcie/aspm.c | 54 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |  7 ++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a96b7424c9bc..b2752851b1ba 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1132,6 +1132,60 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 
+/**
+ * pci_enable_default_link_state - Clears and sets the default device link state
+ * so that the link may be allowed to enter the specified states. Note that
+ * if the BIOS didn't grant ASPM control to the OS, this does nothing because
+ * we can't touch the LNKCTL register. Also note that this does not enable
+ * states disabled by pci_disable_link_state(). Returns 0 or a negative errno.
+ *
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ */
+int pci_enable_default_link_state(struct pci_dev *pdev, int state)
+{
+	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
+
+	if (!link)
+		return -EINVAL;
+	/*
+	 * A driver requested that ASPM be enabled on this device, but
+	 * if we don't have permission to manage ASPM (e.g., on ACPI
+	 * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
+	 * the _OSC method), we can't honor that request.
+	 */
+	if (aspm_disabled) {
+		pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
+		return -EPERM;
+	}
+
+	down_read(&pci_bus_sem);
+	mutex_lock(&aspm_lock);
+	link->aspm_default = 0;
+	if (state & PCIE_LINK_STATE_L0S)
+		link->aspm_default |= ASPM_STATE_L0S;
+	if (state & PCIE_LINK_STATE_L1)
+		/* L1 PM substates require L1 */
+		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
+	if (state & PCIE_LINK_STATE_L1_1)
+		link->aspm_default |= ASPM_STATE_L1_1;
+	if (state & PCIE_LINK_STATE_L1_2)
+		link->aspm_default |= ASPM_STATE_L1_2;
+	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
+		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
+	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
+		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
+	pcie_config_aspm_link(link, policy_to_aspm_state(link));
+
+	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
+	pcie_set_clkpm(link, policy_to_clkpm_state(link));
+	mutex_unlock(&aspm_lock);
+	up_read(&pci_bus_sem);
+
+	return 0;
+}
+EXPORT_SYMBOL(pci_enable_default_link_state);
+
 static int pcie_aspm_set_policy(const char *val,
 				const struct kernel_param *kp)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8253a5413d7c..fd710afe0209 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1641,10 +1641,15 @@ extern bool pcie_ports_native;
 #define PCIE_LINK_STATE_L1_2		BIT(4)
 #define PCIE_LINK_STATE_L1_1_PCIPM	BIT(5)
 #define PCIE_LINK_STATE_L1_2_PCIPM	BIT(6)
+#define PCIE_LINK_STATE_ALL		(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
+					 PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
+					 PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
+					 PCIE_LINK_STATE_L1_2_PCIPM)
 
 #ifdef CONFIG_PCIEASPM
 int pci_disable_link_state(struct pci_dev *pdev, int state);
 int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
+int pci_enable_default_link_state(struct pci_dev *pdev, int state);
 void pcie_no_aspm(void);
 bool pcie_aspm_support_enabled(void);
 bool pcie_aspm_enabled(struct pci_dev *pdev);
@@ -1653,6 +1658,8 @@ static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
 { return 0; }
 static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
 { return 0; }
+static inline int pci_enable_default_link_state(struct pci_dev *pdev, int state)
+{ return 0; }
 static inline void pcie_no_aspm(void) { }
 static inline bool pcie_aspm_support_enabled(void) { return false; }
 static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
-- 
2.25.1

[PATCH V6 2/3] PCI: vmd: Add vmd_device_data

[David E. Box]

Add vmd_device_data to allow adding additional info for driver data. Also
refactor the PCI ID list to use PCI_VDEVICE.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 V6
   - Inline the declarations for driver data in the vmd_ids list.
     Suggested by Jonathan
 V5
   - New patch

 drivers/pci/controller/vmd.c | 76 ++++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index cc166c683638..cde6e2cba210 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -69,6 +69,10 @@ enum vmd_features {
 	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
 };
 
+struct vmd_device_data {
+	enum vmd_features features;
+};
+
 static DEFINE_IDA(vmd_instance_ida);
 
 /*
@@ -710,11 +714,12 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 	vmd_bridge->native_dpc = root_bridge->native_dpc;
 }
 
-static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
+static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
 	struct resource *res;
 	u32 upper_bits;
+	unsigned long features = info->features;
 	unsigned long flags;
 	LIST_HEAD(resources);
 	resource_size_t offset[2] = {0};
@@ -881,7 +886,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	unsigned long features = (unsigned long) id->driver_data;
+	struct vmd_device_data *info = (struct vmd_device_data *)id->driver_data;
+	unsigned long features = info->features;
 	struct vmd_dev *vmd;
 	int err;
 
@@ -925,7 +931,7 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	spin_lock_init(&vmd->cfg_lock);
 	pci_set_drvdata(dev, vmd);
-	err = vmd_enable_domain(vmd, features);
+	err = vmd_enable_domain(vmd, info);
 	if (err)
 		goto out_release_instance;
 
@@ -994,29 +1000,47 @@ static int vmd_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
 
 static const struct pci_device_id vmd_ids[] = {
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_CAN_BYPASS_MSI_REMAP,},
-	{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,},
-	{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,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa77f),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
-	{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,},
-	{0,}
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
+		},
+	},
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_CAN_BYPASS_MSI_REMAP,
+		},
+	},
+	{ PCI_VDEVICE(INTEL, 0x467f),
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
+	{ PCI_VDEVICE(INTEL, 0x4c3d),
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
+	{ PCI_VDEVICE(INTEL, 0xa77f),
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
+	{ }
 };
 MODULE_DEVICE_TABLE(pci, vmd_ids);
 
-- 
2.25.1

[PATCH V6 3/3] PCI: vmd: Configure PCIe ASPM and LTR

[David E. Box]

PCIe ports reserved for VMD use are not visible to BIOS and therefore not
configured to enable PCIE ASPM. Additionally, PCIE LTR values may be left
unset since BIOS will set a default maximum LTR value on endpoints to
ensure that they don't block SoC power management. Lack of this programming
results in high power consumption on laptops as reported in bugzilla [1].
For currently affected products, use pci_enable_default_link_state to set
the allowed link states for devices on the root ports. Also set the LTR
value to the maximum value needed for the SoC. Per the VMD hardware team
future products using VMD will enable BIOS configuration of these
capabilities. This solution is a workaround for current products that
mainly targets laptops. Support is not provided if a switch used nor for
hotplug.

[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>
---
 V6
  - Set ASPM first before setting LTR. This is needed because some
    devices may only have LTR set by BIOS and not ASPM
  - Skip setting the LTR if the current LTR in non-zero.
 V5
  - Provide the LTR value as driver data.
  - Use DWORD for the config space write to avoid PCI WORD access bug.
  - Set ASPM links firsts, enabling all link states, before setting a
    default LTR if the capability is present
  - Add kernel message that VMD is setting the device LTR.
 V4
  - Refactor vmd_enable_apsm() to exit early, making the lines shorter
    and more readable. Suggested by Christoph.
 V3
  - No changes
 V2
  - Use return status to print pci_info message if ASPM cannot be enabled.
  - Add missing static declaration, caught by lkp@intel.com

 drivers/pci/controller/vmd.c | 66 +++++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index cde6e2cba210..8525bb8312f2 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -67,10 +67,19 @@ enum vmd_features {
 	 * interrupt handling.
 	 */
 	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
+
+	/*
+	 * Enable ASPM on the PCIE root ports and set the default LTR of the
+	 * storage devices on platforms where these values are not configured by
+	 * BIOS. This is needed for laptops, which require these settings for
+	 * proper power management of the SoC.
+	 */
+	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
 };
 
 struct vmd_device_data {
 	enum vmd_features features;
+	u16 ltr;
 };
 
 static DEFINE_IDA(vmd_instance_ida);
@@ -714,6 +723,45 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 	vmd_bridge->native_dpc = root_bridge->native_dpc;
 }
 
+/*
+ * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ */
+static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
+{
+	struct vmd_device_data *info = userdata;
+	u32 ltr_reg;
+	int pos;
+
+	if (!(info->features & VMD_FEAT_BIOS_PM_QUIRK))
+		return 0;
+
+	pci_enable_default_link_state(pdev, PCIE_LINK_STATE_ALL);
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!pos)
+		return 0;
+
+	/*
+	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it
+	 * so the LTR quirk is not needed.
+	 */
+	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
+	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
+		return 0;
+
+	/*
+	 * Set the default values to the maximum required by the platform to
+	 * allow the deepest power management savings. Write as a DWORD where
+	 * the lower word is the max snoop latency and the upper word is the
+	 * max non-snoop latency.
+	 */
+	ltr_reg = (info->ltr << 16) | info->ltr;
+	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
+	pci_info(pdev, "VMD: Default LTR set\n");
+
+	return 0;
+}
+
 static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
@@ -867,6 +915,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
 		pci_reset_bus(child->self);
 	pci_assign_unassigned_bus_resources(vmd->bus);
 
+	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, info);
+
 	/*
 	 * 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
@@ -1016,28 +1066,36 @@ static const struct pci_device_id vmd_ids[] = {
 		(kernel_ulong_t)&(struct vmd_device_data) {
 			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+				    VMD_FEAT_OFFSET_FIRST_VECTOR |
+				    VMD_FEAT_BIOS_PM_QUIRK,
+			.ltr = 0x1003, /* 3145728 ns */
 		},
 	},
 	{ PCI_VDEVICE(INTEL, 0x4c3d),
 		(kernel_ulong_t)&(struct vmd_device_data) {
 			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+				    VMD_FEAT_OFFSET_FIRST_VECTOR |
+				    VMD_FEAT_BIOS_PM_QUIRK,
+			.ltr = 0x1003, /* 3145728 ns */
 		},
 	},
 	{ PCI_VDEVICE(INTEL, 0xa77f),
 		(kernel_ulong_t)&(struct vmd_device_data) {
 			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+				    VMD_FEAT_OFFSET_FIRST_VECTOR |
+				    VMD_FEAT_BIOS_PM_QUIRK,
+			.ltr = 0x1003, /* 3145728 ns */
 		},
 	},
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
 		(kernel_ulong_t)&(struct vmd_device_data) {
 			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+				    VMD_FEAT_OFFSET_FIRST_VECTOR |
+				    VMD_FEAT_BIOS_PM_QUIRK,
+			.ltr = 0x1003, /* 3145728 ns */
 		},
 	},
 	{ }
-- 
2.25.1

Re: [PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state()

[Christoph Hellwig]

On Mon, Feb 28, 2022 at 08:19:41PM -0800, David E. Box wrote:
> +	down_read(&pci_bus_sem);
> +	mutex_lock(&aspm_lock);
> +	link->aspm_default = 0;
> +	if (state & PCIE_LINK_STATE_L0S)
> +		link->aspm_default |= ASPM_STATE_L0S;
> +	if (state & PCIE_LINK_STATE_L1)
> +		/* L1 PM substates require L1 */
> +		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> +	if (state & PCIE_LINK_STATE_L1_1)
> +		link->aspm_default |= ASPM_STATE_L1_1;
> +	if (state & PCIE_LINK_STATE_L1_2)
> +		link->aspm_default |= ASPM_STATE_L1_2;
> +	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
> +		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
> +	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> +		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
> +	pcie_config_aspm_link(link, policy_to_aspm_state(link));

Is there any reason the ASPM_* values aren't passed directly to this
function?

Re: [PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state()

[David E. Box]

On Tue, 2022-03-01 at 00:13 -0800, Christoph Hellwig wrote:
> On Mon, Feb 28, 2022 at 08:19:41PM -0800, David E. Box wrote:
> > +	down_read(&pci_bus_sem);
> > +	mutex_lock(&aspm_lock);
> > +	link->aspm_default = 0;
> > +	if (state & PCIE_LINK_STATE_L0S)
> > +		link->aspm_default |= ASPM_STATE_L0S;
> > +	if (state & PCIE_LINK_STATE_L1)
> > +		/* L1 PM substates require L1 */
> > +		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> > +	if (state & PCIE_LINK_STATE_L1_1)
> > +		link->aspm_default |= ASPM_STATE_L1_1;
> > +	if (state & PCIE_LINK_STATE_L1_2)
> > +		link->aspm_default |= ASPM_STATE_L1_2;
> > +	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
> > +		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
> > +	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> > +		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
> > +	pcie_config_aspm_link(link, policy_to_aspm_state(link));
> 
> Is there any reason the ASPM_* values aren't passed directly to this
> function?

The ASPM_* macors aren't visible outside of aspm.c whereas the
PCIE_LINK_STATE_* macros are defined in pci.h. This is similar to what
is done for pci_disable_link_state().

David

Re: [PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state()

[Christoph Hellwig]

On Tue, Mar 01, 2022 at 05:31:51AM -0800, David E. Box wrote:
> > Is there any reason the ASPM_* values aren't passed directly to this
> > function?
> 
> The ASPM_* macors aren't visible outside of aspm.c whereas the
> PCIE_LINK_STATE_* macros are defined in pci.h. This is similar to what
> is done for pci_disable_link_state().

Ok.  This looks a little weird but I guess we should stick to the
existing pattern here.

Re: [PATCH V6 0/3] PCI: vmd: Enable PCIe ASPM and LTR

[Jonathan Derrick]

Set looks good to me
Thanks!

Reviewed-by: Jon Derrick <jonathan.derrick@linux.dev>

On 2/28/2022 9:19 PM, David E. Box wrote:
> This series adds support for enabling PCIe ASPM and for setting PCIe LTR
> values on devices on root ports reserved by VMD. Configuration of these
> capabilities is usually done by BIOS. But for VMD ports these capabilities
> will not be configured because those ports are not visible to BIOS. For
> future products, post Alder Lake, the hardware team has agreed to do this
> enabling in BIOS.  But this will not apply to current products, so this
> work around is provided for them. Without this, laptops running in VMD mode
> will not be able to power gate roots ports, resulting in higher power
> consumption.
> 
> Since V4 we have more information from the BIOS team as to why BIOS
> needs to program device LTRs. This is something that should be done by
> devices, but there are many that don't provide LTR values causing them
> to block SoC level power management. BIOS sets an initial default LTR to
> account for such devices. This SoC specific value is the maximum latency
> required to allow the SoC to enter the deepest power state.
> 
> David E. Box (2):
>    PCI: vmd: Add vmd_device_data
>    PCI: vmd: Configure PCIe ASPM and LTR
> 
> Michael Bottini (1):
>    PCI/ASPM: Add ASPM BIOS override function
> 
>   drivers/pci/controller/vmd.c | 134 ++++++++++++++++++++++++++++-------
>   drivers/pci/pcie/aspm.c      |  54 ++++++++++++++
>   include/linux/pci.h          |   7 ++
>   3 files changed, 169 insertions(+), 26 deletions(-)
> 
> 
> base-commit: 754e0b0e35608ed5206d6a67a791563c631cec07

Re: [PATCH V6 2/3] PCI: vmd: Add vmd_device_data

[Lorenzo Pieralisi]

On Mon, Feb 28, 2022 at 08:19:42PM -0800, David E. Box wrote:
> Add vmd_device_data to allow adding additional info for driver data. Also
> refactor the PCI ID list to use PCI_VDEVICE.

I think this must be two patches, (1) conversion to PCI_VDEVICE
and (2) vmd_device_data.

Thanks,
Lorenzo

> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  V6
>    - Inline the declarations for driver data in the vmd_ids list.
>      Suggested by Jonathan
>  V5
>    - New patch
> 
>  drivers/pci/controller/vmd.c | 76 ++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index cc166c683638..cde6e2cba210 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -69,6 +69,10 @@ enum vmd_features {
>  	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
>  };
>  
> +struct vmd_device_data {
> +	enum vmd_features features;
> +};
> +
>  static DEFINE_IDA(vmd_instance_ida);
>  
>  /*
> @@ -710,11 +714,12 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
>  	vmd_bridge->native_dpc = root_bridge->native_dpc;
>  }
>  
> -static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> +static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
>  {
>  	struct pci_sysdata *sd = &vmd->sysdata;
>  	struct resource *res;
>  	u32 upper_bits;
> +	unsigned long features = info->features;
>  	unsigned long flags;
>  	LIST_HEAD(resources);
>  	resource_size_t offset[2] = {0};
> @@ -881,7 +886,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  
>  static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
> -	unsigned long features = (unsigned long) id->driver_data;
> +	struct vmd_device_data *info = (struct vmd_device_data *)id->driver_data;
> +	unsigned long features = info->features;
>  	struct vmd_dev *vmd;
>  	int err;
>  
> @@ -925,7 +931,7 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  
>  	spin_lock_init(&vmd->cfg_lock);
>  	pci_set_drvdata(dev, vmd);
> -	err = vmd_enable_domain(vmd, features);
> +	err = vmd_enable_domain(vmd, info);
>  	if (err)
>  		goto out_release_instance;
>  
> @@ -994,29 +1000,47 @@ static int vmd_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
>  
>  static const struct pci_device_id vmd_ids[] = {
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
> -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
> -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW |
> -				VMD_FEAT_HAS_BUS_RESTRICTIONS |
> -				VMD_FEAT_CAN_BYPASS_MSI_REMAP,},
> -	{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,},
> -	{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,},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa77f),
> -		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> -				VMD_FEAT_HAS_BUS_RESTRICTIONS |
> -				VMD_FEAT_OFFSET_FIRST_VECTOR,},
> -	{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,},
> -	{0,}
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
> +		(kernel_ulong_t)&(struct vmd_device_data) {
> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
> +		},
> +	},
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
> +		(kernel_ulong_t)&(struct vmd_device_data) {
> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW |
> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> +				    VMD_FEAT_CAN_BYPASS_MSI_REMAP,
> +		},
> +	},
> +	{ PCI_VDEVICE(INTEL, 0x467f),
> +		(kernel_ulong_t)&(struct vmd_device_data) {
> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +		},
> +	},
> +	{ PCI_VDEVICE(INTEL, 0x4c3d),
> +		(kernel_ulong_t)&(struct vmd_device_data) {
> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +		},
> +	},
> +	{ PCI_VDEVICE(INTEL, 0xa77f),
> +		(kernel_ulong_t)&(struct vmd_device_data) {
> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +		},
> +	},
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> +		(kernel_ulong_t)&(struct vmd_device_data) {
> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +		},
> +	},
> +	{ }
>  };
>  MODULE_DEVICE_TABLE(pci, vmd_ids);
>  
> -- 
> 2.25.1
> 
> 

Re: [PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state()

[Lorenzo Pieralisi]

On Mon, Feb 28, 2022 at 08:19:41PM -0800, David E. Box wrote:
> From: Michael Bottini <michael.a.bottini@linux.intel.com>
> 
> Add pci_enable_default_link_state() to allow devices to change the default
> BIOS configured states. Clears the BIOS default settings then sets the new
> states and reconfigures the link under the semaphore. Also add
> PCIE_LINK_STATE_ALL macro for convenience for callers that want to enable
> all link states.
> 
> Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  V6
>   - No change
>  V5
>   - Rename to pci_enable_default_link_state and model after
>     pci_disable_link_state() as suggested by Bjorn.
>   - Add helper PCIE_LINK_STATE_ALL which sets bits for all links states and
>     clock pm.
>   - Clarify commit language to indicate the function changes the default
>     link states (not ASPM policy).
>  V4
>   - Refactor vmd_enable_apsm() to exit early, making the lines shorter
>     and more readable. Suggested by Christoph.
>  V3
>   - No changes
>  V2
>   - Use return status to print pci_info message if ASPM cannot be enabled.
>   - Add missing static declaration, caught by lkp@intel.com
>  
> 
>  drivers/pci/pcie/aspm.c | 54 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h     |  7 ++++++
>  2 files changed, 61 insertions(+)

This requires Bjorn's ACK, delegated to him in patchwork.

Lorenzo

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b7424c9bc..b2752851b1ba 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1132,6 +1132,60 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
>  }
>  EXPORT_SYMBOL(pci_disable_link_state);
>  
> +/**
> + * pci_enable_default_link_state - Clears and sets the default device link state
> + * so that the link may be allowed to enter the specified states. Note that
> + * if the BIOS didn't grant ASPM control to the OS, this does nothing because
> + * we can't touch the LNKCTL register. Also note that this does not enable
> + * states disabled by pci_disable_link_state(). Returns 0 or a negative errno.
> + *
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + */
> +int pci_enable_default_link_state(struct pci_dev *pdev, int state)
> +{
> +	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> +
> +	if (!link)
> +		return -EINVAL;
> +	/*
> +	 * A driver requested that ASPM be enabled on this device, but
> +	 * if we don't have permission to manage ASPM (e.g., on ACPI
> +	 * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
> +	 * the _OSC method), we can't honor that request.
> +	 */
> +	if (aspm_disabled) {
> +		pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
> +		return -EPERM;
> +	}
> +
> +	down_read(&pci_bus_sem);
> +	mutex_lock(&aspm_lock);
> +	link->aspm_default = 0;
> +	if (state & PCIE_LINK_STATE_L0S)
> +		link->aspm_default |= ASPM_STATE_L0S;
> +	if (state & PCIE_LINK_STATE_L1)
> +		/* L1 PM substates require L1 */
> +		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> +	if (state & PCIE_LINK_STATE_L1_1)
> +		link->aspm_default |= ASPM_STATE_L1_1;
> +	if (state & PCIE_LINK_STATE_L1_2)
> +		link->aspm_default |= ASPM_STATE_L1_2;
> +	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
> +		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
> +	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> +		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
> +	pcie_config_aspm_link(link, policy_to_aspm_state(link));
> +
> +	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> +	pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +	mutex_unlock(&aspm_lock);
> +	up_read(&pci_bus_sem);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(pci_enable_default_link_state);
> +
>  static int pcie_aspm_set_policy(const char *val,
>  				const struct kernel_param *kp)
>  {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8253a5413d7c..fd710afe0209 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1641,10 +1641,15 @@ extern bool pcie_ports_native;
>  #define PCIE_LINK_STATE_L1_2		BIT(4)
>  #define PCIE_LINK_STATE_L1_1_PCIPM	BIT(5)
>  #define PCIE_LINK_STATE_L1_2_PCIPM	BIT(6)
> +#define PCIE_LINK_STATE_ALL		(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
> +					 PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
> +					 PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
> +					 PCIE_LINK_STATE_L1_2_PCIPM)
>  
>  #ifdef CONFIG_PCIEASPM
>  int pci_disable_link_state(struct pci_dev *pdev, int state);
>  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> +int pci_enable_default_link_state(struct pci_dev *pdev, int state);
>  void pcie_no_aspm(void);
>  bool pcie_aspm_support_enabled(void);
>  bool pcie_aspm_enabled(struct pci_dev *pdev);
> @@ -1653,6 +1658,8 @@ static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
>  { return 0; }
>  static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
>  { return 0; }
> +static inline int pci_enable_default_link_state(struct pci_dev *pdev, int state)
> +{ return 0; }
>  static inline void pcie_no_aspm(void) { }
>  static inline bool pcie_aspm_support_enabled(void) { return false; }
>  static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
> -- 
> 2.25.1
> 
> 

Re: [PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state()

[Bjorn Helgaas]

On Mon, Feb 28, 2022 at 08:19:41PM -0800, David E. Box wrote:
> From: Michael Bottini <michael.a.bottini@linux.intel.com>
> 
> Add pci_enable_default_link_state() to allow devices to change the default
> BIOS configured states. Clears the BIOS default settings then sets the new
> states and reconfigures the link under the semaphore. Also add
> PCIE_LINK_STATE_ALL macro for convenience for callers that want to enable
> all link states.
> 
> Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

With minor changes below,

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> + * pci_enable_default_link_state - Clears and sets the default device link state
> + * so that the link may be allowed to enter the specified states. Note that
> + * if the BIOS didn't grant ASPM control to the OS, this does nothing because
> + * we can't touch the LNKCTL register. Also note that this does not enable
> + * states disabled by pci_disable_link_state(). Returns 0 or a negative errno.

"Clear and set" to match imperative mood of similar comments.

Similarly "Return 0 or ...".

> + *
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + */
> +int pci_enable_default_link_state(struct pci_dev *pdev, int state)

I think "pci_enable_link_state()" would be a better name since
"default" isn't relevant to the caller and it would be more parallel
with pci_disable_link_state().

> +{
> +	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> +
> +	if (!link)
> +		return -EINVAL;
> +	/*
> +	 * A driver requested that ASPM be enabled on this device, but
> +	 * if we don't have permission to manage ASPM (e.g., on ACPI
> +	 * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
> +	 * the _OSC method), we can't honor that request.
> +	 */
> +	if (aspm_disabled) {
> +		pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
> +		return -EPERM;
> +	}
> +
> +	down_read(&pci_bus_sem);
> +	mutex_lock(&aspm_lock);
> +	link->aspm_default = 0;
> +	if (state & PCIE_LINK_STATE_L0S)
> +		link->aspm_default |= ASPM_STATE_L0S;
> +	if (state & PCIE_LINK_STATE_L1)
> +		/* L1 PM substates require L1 */
> +		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> +	if (state & PCIE_LINK_STATE_L1_1)
> +		link->aspm_default |= ASPM_STATE_L1_1;
> +	if (state & PCIE_LINK_STATE_L1_2)
> +		link->aspm_default |= ASPM_STATE_L1_2;
> +	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
> +		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
> +	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> +		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
> +	pcie_config_aspm_link(link, policy_to_aspm_state(link));
> +
> +	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> +	pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +	mutex_unlock(&aspm_lock);
> +	up_read(&pci_bus_sem);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(pci_enable_default_link_state);
> +
>  static int pcie_aspm_set_policy(const char *val,
>  				const struct kernel_param *kp)
>  {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8253a5413d7c..fd710afe0209 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1641,10 +1641,15 @@ extern bool pcie_ports_native;
>  #define PCIE_LINK_STATE_L1_2		BIT(4)
>  #define PCIE_LINK_STATE_L1_1_PCIPM	BIT(5)
>  #define PCIE_LINK_STATE_L1_2_PCIPM	BIT(6)
> +#define PCIE_LINK_STATE_ALL		(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
> +					 PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
> +					 PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
> +					 PCIE_LINK_STATE_L1_2_PCIPM)
>  
>  #ifdef CONFIG_PCIEASPM
>  int pci_disable_link_state(struct pci_dev *pdev, int state);
>  int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> +int pci_enable_default_link_state(struct pci_dev *pdev, int state);
>  void pcie_no_aspm(void);
>  bool pcie_aspm_support_enabled(void);
>  bool pcie_aspm_enabled(struct pci_dev *pdev);
> @@ -1653,6 +1658,8 @@ static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
>  { return 0; }
>  static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
>  { return 0; }
> +static inline int pci_enable_default_link_state(struct pci_dev *pdev, int state)
> +{ return 0; }
>  static inline void pcie_no_aspm(void) { }
>  static inline bool pcie_aspm_support_enabled(void) { return false; }
>  static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
> -- 
> 2.25.1
> 

Re: [PATCH V6 3/3] PCI: vmd: Configure PCIe ASPM and LTR

[Bjorn Helgaas]

On Mon, Feb 28, 2022 at 08:19:43PM -0800, David E. Box wrote:
> PCIe ports reserved for VMD use are not visible to BIOS and therefore not
> configured to enable PCIE ASPM.

> Additionally, PCIE LTR values may be left unset since BIOS will set
> a default maximum LTR value on endpoints to ensure that they don't
> block SoC power management.

If the ports aren't visible to BIOS, I assume BIOS doesn't configure
*anything*, including LTR.  This sentence seems like it has a little
too much information; if BIOS doesn't see the ports, LTR, SoC power
management, etc., is not relevant.

> Lack of this programming results in high power consumption on
> laptops as reported in bugzilla [1].

> For currently affected products, use pci_enable_default_link_state to set
> the allowed link states for devices on the root ports.

"Currently affected products" makes me wonder about the *other*
products?  Seems like we should handle *all* VMD devices the same way.

> Also set the LTR value to the maximum value needed for the SoC. Per
> the VMD hardware team future products using VMD will enable BIOS
> configuration of these capabilities. This solution is a workaround
> for current products that mainly targets laptops.

I guess the cover letter has a little more background on this,
although I don't understand how talking to the Intel BIOS team can
solve this for *all* vendors using these parts.

> Support is not provided if a switch used nor for hotplug.

What switch are you referring to?  What is the hotplug scenario?  Are
VMD ports hot-pluggable?  I assumed they were built into the Root
Complex and not hot-pluggable.

s/PCIE/PCIe/ several times above so they're all consistent.

s/pci_enable_default_link_state/pci_enable_default_link_state()/ so it
looks like a function.

That's a big block of text; maybe could be 2-3 paragraphs.

> [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>
> ---
>  V6
>   - Set ASPM first before setting LTR. This is needed because some
>     devices may only have LTR set by BIOS and not ASPM
>   - Skip setting the LTR if the current LTR in non-zero.
>  V5
>   - Provide the LTR value as driver data.
>   - Use DWORD for the config space write to avoid PCI WORD access bug.
>   - Set ASPM links firsts, enabling all link states, before setting a
>     default LTR if the capability is present
>   - Add kernel message that VMD is setting the device LTR.
>  V4
>   - Refactor vmd_enable_apsm() to exit early, making the lines shorter
>     and more readable. Suggested by Christoph.
>  V3
>   - No changes
>  V2
>   - Use return status to print pci_info message if ASPM cannot be enabled.
>   - Add missing static declaration, caught by lkp@intel.com
> 
>  drivers/pci/controller/vmd.c | 66 +++++++++++++++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index cde6e2cba210..8525bb8312f2 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -67,10 +67,19 @@ enum vmd_features {
>  	 * interrupt handling.
>  	 */
>  	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
> +
> +	/*
> +	 * Enable ASPM on the PCIE root ports and set the default LTR of the
> +	 * storage devices on platforms where these values are not configured by
> +	 * BIOS. This is needed for laptops, which require these settings for
> +	 * proper power management of the SoC.
> +	 */
> +	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
>  };
>  
>  struct vmd_device_data {
>  	enum vmd_features features;
> +	u16 ltr;
>  };
>  
>  static DEFINE_IDA(vmd_instance_ida);
> @@ -714,6 +723,45 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
>  	vmd_bridge->native_dpc = root_bridge->native_dpc;
>  }
>  
> +/*
> + * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> + */
> +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> +{
> +	struct vmd_device_data *info = userdata;
> +	u32 ltr_reg;
> +	int pos;
> +
> +	if (!(info->features & VMD_FEAT_BIOS_PM_QUIRK))
> +		return 0;
> +
> +	pci_enable_default_link_state(pdev, PCIE_LINK_STATE_ALL);
> +
> +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> +	if (!pos)
> +		return 0;
> +
> +	/*
> +	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it
> +	 * so the LTR quirk is not needed.
> +	 */
> +	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
> +	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> +		return 0;
> +
> +	/*
> +	 * Set the default values to the maximum required by the platform to
> +	 * allow the deepest power management savings. Write as a DWORD where
> +	 * the lower word is the max snoop latency and the upper word is the
> +	 * max non-snoop latency.
> +	 */
> +	ltr_reg = (info->ltr << 16) | info->ltr;

The fact that you have to hard-code the LTR values in the driver seems
problematic because it requires updates for every new device.  I guess
you have to update the driver anyway to add Device IDs.

But surely there should be a firmware interface to discover this
platform-specific information?  Does the _DSM for Latency Tolerance
Reporting (PCI Firmware spec r3.3, sec 4.6.6) supply this? 

We badly need generic support for that _DSM, but the documentation is
somewhat lacking.

> +	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
> +	pci_info(pdev, "VMD: Default LTR set\n");
> +
> +	return 0;
> +}
> +
>  static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
>  {
>  	struct pci_sysdata *sd = &vmd->sysdata;
> @@ -867,6 +915,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
>  		pci_reset_bus(child->self);
>  	pci_assign_unassigned_bus_resources(vmd->bus);
>  
> +	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, info);
> +
>  	/*
>  	 * 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
> @@ -1016,28 +1066,36 @@ static const struct pci_device_id vmd_ids[] = {
>  		(kernel_ulong_t)&(struct vmd_device_data) {
>  			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>  				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> -				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR |
> +				    VMD_FEAT_BIOS_PM_QUIRK,
> +			.ltr = 0x1003, /* 3145728 ns */
>  		},
>  	},
>  	{ PCI_VDEVICE(INTEL, 0x4c3d),
>  		(kernel_ulong_t)&(struct vmd_device_data) {
>  			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>  				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> -				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR |
> +				    VMD_FEAT_BIOS_PM_QUIRK,
> +			.ltr = 0x1003, /* 3145728 ns */
>  		},
>  	},
>  	{ PCI_VDEVICE(INTEL, 0xa77f),
>  		(kernel_ulong_t)&(struct vmd_device_data) {
>  			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>  				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> -				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR |
> +				    VMD_FEAT_BIOS_PM_QUIRK,
> +			.ltr = 0x1003, /* 3145728 ns */
>  		},
>  	},
>  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>  		(kernel_ulong_t)&(struct vmd_device_data) {
>  			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>  				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> -				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR |
> +				    VMD_FEAT_BIOS_PM_QUIRK,
> +			.ltr = 0x1003, /* 3145728 ns */
>  		},
>  	},
>  	{ }
> -- 
> 2.25.1
> 

Re: [PATCH V6 3/3] PCI: vmd: Configure PCIe ASPM and LTR

[David E. Box]

Hi Bjorn,

Sorry for the delay. I'm just back from an extended leave.

On Fri, 2022-08-26 at 12:01 -0500, Bjorn Helgaas wrote:
> On Mon, Feb 28, 2022 at 08:19:43PM -0800, David E. Box wrote:
> > PCIe ports reserved for VMD use are not visible to BIOS and therefore not
> > configured to enable PCIE ASPM.
> > Additionally, PCIE LTR values may be left unset since BIOS will set
> > a default maximum LTR value on endpoints to ensure that they don't
> > block SoC power management.
> 
> If the ports aren't visible to BIOS, I assume BIOS doesn't configure
> *anything*, including LTR.  This sentence seems like it has a little
> too much information; if BIOS doesn't see the ports, LTR, SoC power
> management, etc., is not relevant.

It's mentioned as an explanation for why the patch also programs LTR values.

> 
> > Lack of this programming results in high power consumption on
> > laptops as reported in bugzilla [1].
> > For currently affected products, use pci_enable_default_link_state to set
> > the allowed link states for devices on the root ports.
> 
> "Currently affected products" makes me wonder about the *other*
> products?  Seems like we should handle *all* VMD devices the same way.

Well it affects Rocket Lake, Tiger Lake, and Alder Lake. It partially affects
Raptor Lake because we were able to have BIOS implement a solution to program
the LTR values. The plan is to use the same method to also program ASPM by
Meteor Lake and future products. So we do intend this to be a quirk for current
platforms, not a permanent solution.

> > Also set the LTR value to the maximum value needed for the SoC. Per
> > the VMD hardware team future products using VMD will enable BIOS
> > configuration of these capabilities. This solution is a workaround
> > for current products that mainly targets laptops.
> 
> I guess the cover letter has a little more background on this,
> although I don't understand how talking to the Intel BIOS team can
> solve this for *all* vendors using these parts.
> 
> > Support is not provided if a switch used nor for hotplug.
> 
> What switch are you referring to?

An expansion switch added between the root port and the downstream device.

>   What is the hotplug scenario?  Are
> VMD ports hot-pluggable? I assumed they were built into the Root
> Complex and not hot-pluggable.

I understand that they can be hot-pluggable. But the focus of the patch is to
address high power consumption on laptop systems since this is where we have
bugs reported. I don't know that it's worth the effort to make this work with
hot-plug.

> 
> s/PCIE/PCIe/ several times above so they're all consistent.
> 
> s/pci_enable_default_link_state/pci_enable_default_link_state()/ so it
> looks like a function.
> 
> That's a big block of text; maybe could be 2-3 paragraphs.

Ack

> 
> > [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>
> > ---
> >  V6
> >   - Set ASPM first before setting LTR. This is needed because some
> >     devices may only have LTR set by BIOS and not ASPM
> >   - Skip setting the LTR if the current LTR in non-zero.
> >  V5
> >   - Provide the LTR value as driver data.
> >   - Use DWORD for the config space write to avoid PCI WORD access bug.
> >   - Set ASPM links firsts, enabling all link states, before setting a
> >     default LTR if the capability is present
> >   - Add kernel message that VMD is setting the device LTR.
> >  V4
> >   - Refactor vmd_enable_apsm() to exit early, making the lines shorter
> >     and more readable. Suggested by Christoph.
> >  V3
> >   - No changes
> >  V2
> >   - Use return status to print pci_info message if ASPM cannot be enabled.
> >   - Add missing static declaration, caught by lkp@intel.com
> > 
> >  drivers/pci/controller/vmd.c | 66 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 62 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index cde6e2cba210..8525bb8312f2 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -67,10 +67,19 @@ enum vmd_features {
> >  	 * interrupt handling.
> >  	 */
> >  	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
> > +
> > +	/*
> > +	 * Enable ASPM on the PCIE root ports and set the default LTR of the
> > +	 * storage devices on platforms where these values are not configured by
> > +	 * BIOS. This is needed for laptops, which require these settings for
> > +	 * proper power management of the SoC.
> > +	 */
> > +	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
> >  };
> >  
> >  struct vmd_device_data {
> >  	enum vmd_features features;
> > +	u16 ltr;
> >  };
> >  
> >  static DEFINE_IDA(vmd_instance_ida);
> > @@ -714,6 +723,45 @@ static void vmd_copy_host_bridge_flags(struct
> > pci_host_bridge *root_bridge,
> >  	vmd_bridge->native_dpc = root_bridge->native_dpc;
> >  }
> >  
> > +/*
> > + * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> > + */
> > +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > +{
> > +	struct vmd_device_data *info = userdata;
> > +	u32 ltr_reg;
> > +	int pos;
> > +
> > +	if (!(info->features & VMD_FEAT_BIOS_PM_QUIRK))
> > +		return 0;
> > +
> > +	pci_enable_default_link_state(pdev, PCIE_LINK_STATE_ALL);
> > +
> > +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > +	if (!pos)
> > +		return 0;
> > +
> > +	/*
> > +	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it
> > +	 * so the LTR quirk is not needed.
> > +	 */
> > +	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
> > +	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> > +		return 0;
> > +
> > +	/*
> > +	 * Set the default values to the maximum required by the platform to
> > +	 * allow the deepest power management savings. Write as a DWORD where
> > +	 * the lower word is the max snoop latency and the upper word is the
> > +	 * max non-snoop latency.
> > +	 */
> > +	ltr_reg = (info->ltr << 16) | info->ltr;
> 
> The fact that you have to hard-code the LTR values in the driver seems
> problematic because it requires updates for every new device.  I guess
> you have to update the driver anyway to add Device IDs.
> 
> But surely there should be a firmware interface to discover this
> platform-specific information?  Does the _DSM for Latency Tolerance
> Reporting (PCI Firmware spec r3.3, sec 4.6.6) supply this?
> 
> We badly need generic support for that _DSM, but the documentation is
> somewhat lacking.

Looking at the spec (and at current DSDTs) it looks like that _DSM can be used
for exactly this purpose. However, proper VMD ACPI support is yet another
headache for Linux. The AML exists, but uses a different addressing scheme in
order to hide the devices from normal enumeration. Because of this, there are no
ACPI companion devices associated with the PCI devices.

David

> 
> > +	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
> > +	pci_info(pdev, "VMD: Default LTR set\n");
> > +
> > +	return 0;
> > +}
> > +
> >  static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data
> > *info)
> >  {
> >  	struct pci_sysdata *sd = &vmd->sysdata;
> > @@ -867,6 +915,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, struct
> > vmd_device_data *info)
> >  		pci_reset_bus(child->self);
> >  	pci_assign_unassigned_bus_resources(vmd->bus);
> >  
> > +	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, info);
> > +
> >  	/*
> >  	 * 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
> > @@ -1016,28 +1066,36 @@ static const struct pci_device_id vmd_ids[] = {
> >  		(kernel_ulong_t)&(struct vmd_device_data) {
> >  			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >  				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > -				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> > +				    VMD_FEAT_OFFSET_FIRST_VECTOR |
> > +				    VMD_FEAT_BIOS_PM_QUIRK,
> > +			.ltr = 0x1003, /* 3145728 ns */
> >  		},
> >  	},
> >  	{ PCI_VDEVICE(INTEL, 0x4c3d),
> >  		(kernel_ulong_t)&(struct vmd_device_data) {
> >  			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >  				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > -				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> > +				    VMD_FEAT_OFFSET_FIRST_VECTOR |
> > +				    VMD_FEAT_BIOS_PM_QUIRK,
> > +			.ltr = 0x1003, /* 3145728 ns */
> >  		},
> >  	},
> >  	{ PCI_VDEVICE(INTEL, 0xa77f),
> >  		(kernel_ulong_t)&(struct vmd_device_data) {
> >  			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >  				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > -				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> > +				    VMD_FEAT_OFFSET_FIRST_VECTOR |
> > +				    VMD_FEAT_BIOS_PM_QUIRK,
> > +			.ltr = 0x1003, /* 3145728 ns */
> >  		},
> >  	},
> >  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> >  		(kernel_ulong_t)&(struct vmd_device_data) {
> >  			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >  				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > -				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> > +				    VMD_FEAT_OFFSET_FIRST_VECTOR |
> > +				    VMD_FEAT_BIOS_PM_QUIRK,
> > +			.ltr = 0x1003, /* 3145728 ns */
> >  		},
> >  	},
> >  	{ }
> > -- 
> > 2.25.1
> >