[PATCH V7 0/4] PCI: vmd: Enable PCIe ASPM and LTR on select hardware

[PATCH V7 0/4] PCI: vmd: Enable PCIe ASPM and LTR on select hardware

[David E. Box]

This series adds a work around for enabling PCIe ASPM and for setting PCIe
LTR values on VMD reserved root ports on select platforms. While
configuration of these capabilities is usually done by BIOS, on these
platforms these capabilities will not be configured because the ports are
not visible to BIOS. This was part of an initial design that expected the
driver to completely handle the ports, including power management. However
on Linux those ports are still managed by the PCIe core, which has the
expectation that they adhere to device standards including BIOS
configuration, leading to this problem.

The target platforms are Tiger Lake, Alder Lake, and Raptor Lake though the
latter has already implemented support for configuring the LTR values.
Meteor Lake is expected add BIOS ASPM support, eliminating the future need
for this work around.

Note, the driver programs the LTRs because BIOS would also normally do this
for devices that do not set them by default. Without this, SoC power
management would be blocked on those platform. This SoC specific value is
the maximum latency required to allow the SoC to enter the deepest power
state.

This patch addresses the following open bugzillas on VMD enabled laptops
that cannot enter low power states.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=212355
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215063
Link: https://bugzilla.kernel.org/show_bug.cgi?id=213717

David E. Box (3):
  PCI: vmd: Use PCI_VDEVICE in device list
  PCI: vmd: Add vmd_device_data
  PCI: vmd: Add quirk to configure PCIe ASPM and LTR

Michael Bottini (1):
  PCI/ASPM: Add pci_enable_link_state()

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


base-commit: 247f34f7b80357943234f93f247a1ae6b6c3a740
-- 
2.25.1

[PATCH V7 1/4] PCI/ASPM: Add pci_enable_link_state()

[David E. Box]

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

Add pci_enable_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>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
 V7
  - Fix description as suggested by Bjorn
  - Rename function to pci_enable_link_state

 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 53a1fa306e1e..339c686a5094 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1181,6 +1181,60 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 
+/**
+ * pci_enable_link_state - Clear and set 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(). Return 0 or a negative errno.
+ *
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ */
+int pci_enable_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_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 2bda4a4e47e8..8c35f15e6012 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1651,10 +1651,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_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);
@@ -1663,6 +1668,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_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 V7 2/4] PCI: vmd: Use PCI_VDEVICE in device list

[David E. Box]

Refactor the PCI ID list to use PCI_VDEVICE.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Jon Derrick <jonathan.derrick@linux.dev>
Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
V7 - New Patch. Separate patch suggested by Lorenzo

 drivers/pci/controller/vmd.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e06e9f4fc50f..9dedca714c18 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -994,33 +994,33 @@ 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),
+	{PCI_VDEVICE(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),
+	{PCI_VDEVICE(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),
+	{PCI_VDEVICE(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),
+	{PCI_VDEVICE(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),
+	{PCI_VDEVICE(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, 0x7d0b),
+	{PCI_VDEVICE(INTEL, 0x7d0b),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_OFFSET_FIRST_VECTOR,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xad0b),
+	{PCI_VDEVICE(INTEL, 0xad0b),
 		.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),
+	{PCI_VDEVICE(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,},
-- 
2.25.1

[PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[David E. Box]

Add vmd_device_data to allow adding additional info for driver data.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Jon Derrick <jonathan.derrick@linux.dev>
Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
 V7
   - Moved PCI_VDEVICE into separate earlier patch.
 V6
   - Inline the declarations for driver data in the vmd_ids list.
     Suggested by Jonathan
 V5
   - New patch

 drivers/pci/controller/vmd.c | 80 +++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9dedca714c18..a53d88fd820c 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -68,6 +68,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);
 
 /*
@@ -709,11 +713,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};
@@ -882,7 +887,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;
 
@@ -927,7 +933,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;
 
@@ -995,35 +1001,59 @@ static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
 
 static const struct pci_device_id vmd_ids[] = {
 	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
+		},
+	},
 	{PCI_VDEVICE(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,},
+		(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),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(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),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(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),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(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, 0x7d0b),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(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, 0xad0b),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(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),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		(kernel_ulong_t)&(struct vmd_device_data) {
+			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
+				    VMD_FEAT_OFFSET_FIRST_VECTOR,
+		},
+	},
 	{0,}
 };
 MODULE_DEVICE_TABLE(pci, vmd_ids);
-- 
2.25.1

[PATCH V7 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR

[David E. Box]

On some platforms, PCIe ports reserved for VMD use are not visible to BIOS
and therefore not configured to enable PCIe ASPM or LTR values (which BIOS
will configure if they are not set). Lack of this programming results in
high power consumption on laptops as reported in several open bugzillas.
For the affected platforms use pci_enable_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.

This workaround applies to Rocket Lake, Tiger Lake, Alder Lake, and Raptor
Lake, though the latter has already implemented LTR configuring in BIOS.
Future products will move ASPM configuration back to BIOS.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=212355
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215063
Link: 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>
Reviewed-by: Jon Derrick <jonathan.derrick@linux.dev>
Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 74 +++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index a53d88fd820c..bb7e9195bc18 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -66,10 +66,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);
@@ -713,6 +722,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_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 value set by driver\n");
+
+	return 0;
+}
+
 static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
@@ -868,6 +916,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,42 +1066,54 @@ 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, 0x7d0b),
 		(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, 0xad0b),
 		(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 */
 		},
 	},
 	{0,}
-- 
2.25.1

Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[Bjorn Helgaas]

On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> Add vmd_device_data to allow adding additional info for driver data.

>  	{PCI_VDEVICE(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,},
> +		(kernel_ulong_t)&(struct vmd_device_data) {
> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +		},
> +	},

It looks like these devices come in families where several device IDs
share the same features.  I think this would be more readable if you
defined each family outside this table and simply referenced the
family here.  E.g., you could do something like:

  static struct vmd_device_data vmd_v1 = {
    .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),
    .driver_data = (kernel_ulong_t) &vmd_v1,

Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
instead of repeating it a half dozen times.

>  	{0,}
>  };
>  MODULE_DEVICE_TABLE(pci, vmd_ids);
> -- 
> 2.25.1
> 

Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[Jonathan Derrick]

On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>> Add vmd_device_data to allow adding additional info for driver data.
> 
>>  	{PCI_VDEVICE(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,},
>> +		(kernel_ulong_t)&(struct vmd_device_data) {
>> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
>> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
>> +		},
>> +	},
> 
> It looks like these devices come in families where several device IDs
> share the same features.  I think this would be more readable if you
> defined each family outside this table and simply referenced the
> family here.  E.g., you could do something like:
> 
>   static struct vmd_device_data vmd_v1 = {
>     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
> 		VMD_FEAT_OFFSET_FIRST_VECTOR,
>   };
I seem to recall it being similar to this in one of the previous revisions
It's fine with me either way

> 
>   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>     .driver_data = (kernel_ulong_t) &vmd_v1,
> 
> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> instead of repeating it a half dozen times.
> 
>>  	{0,}
>>  };
>>  MODULE_DEVICE_TABLE(pci, vmd_ids);
>> -- 
>> 2.25.1
>>

Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[Bjorn Helgaas]

On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> > On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> >> Add vmd_device_data to allow adding additional info for driver data.
> > 
> >>  	{PCI_VDEVICE(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,},
> >> +		(kernel_ulong_t)&(struct vmd_device_data) {
> >> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> >> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> >> +		},
> >> +	},
> > 
> > It looks like these devices come in families where several device IDs
> > share the same features.  I think this would be more readable if you
> > defined each family outside this table and simply referenced the
> > family here.  E.g., you could do something like:
> > 
> >   static struct vmd_device_data vmd_v1 = {
> >     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > 		VMD_FEAT_OFFSET_FIRST_VECTOR,
> >   };
>
> I seem to recall it being similar to this in one of the previous revisions
> It's fine with me either way

Indeed it was:
https://lore.kernel.org/r/366a9602-555f-7a1b-a8db-bbcbf84b7b08@linux.dev
I'd forgotten that.

At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
that used the 467f data.  The current series adds two more (0x7d0b
0x0ad0b).  Maybe the "vmd_467f_data" name could have been more
descriptive, but the code was definitely shorter:

  +     { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
  +     { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
  +     { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
  +     { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B), (kernel_ulong_t)&vmd_467f_data },

I do wish pci_device_id.driver_data were a void pointer, as it is for
of_device_id, which makes it much more natural to express [1], but
that ship has long sailed.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768

> >   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> >     .driver_data = (kernel_ulong_t) &vmd_v1,
> > 
> > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > instead of repeating it a half dozen times.
> > 
> >>  	{0,}
> >>  };
> >>  MODULE_DEVICE_TABLE(pci, vmd_ids);
> >> -- 
> >> 2.25.1
> >>

Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[David E. Box]

On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
> > On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> > > On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> > > > Add vmd_device_data to allow adding additional info for driver data.
> > > >  	{PCI_VDEVICE(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,},
> > > > +		(kernel_ulong_t)&(struct vmd_device_data) {
> > > > +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > > +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > > +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
> > > > +		},
> > > > +	},
> > > 
> > > It looks like these devices come in families where several device IDs
> > > share the same features.  I think this would be more readable if you
> > > defined each family outside this table and simply referenced the
> > > family here.  E.g., you could do something like:
> > > 
> > >   static struct vmd_device_data vmd_v1 = {
> > >     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > 		VMD_FEAT_OFFSET_FIRST_VECTOR,
> > >   };
> > 
> > I seem to recall it being similar to this in one of the previous revisions
> > It's fine with me either way
> 
> Indeed it was:
> https://lore.kernel.org/r/366a9602-555f-7a1b-a8db-bbcbf84b7b08@linux.dev
> I'd forgotten that.
> 
> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
> that used the 467f data.  The current series adds two more (0x7d0b
> 0x0ad0b).  Maybe the "vmd_467f_data" name could have been more
> descriptive, but the code was definitely shorter:
> 
>   +     { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
>   +     { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
>   +     { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
>   +     { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> (kernel_ulong_t)&vmd_467f_data },

I prefer this too but don't know what's the best name. Could just be by the
platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
you think Jonathan?

David

> 
> I do wish pci_device_id.driver_data were a void pointer, as it is for
> of_device_id, which makes it much more natural to express [1], but
> that ship has long sailed.
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
> 
> > >   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> > >     .driver_data = (kernel_ulong_t) &vmd_v1,
> > > 
> > > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > > instead of repeating it a half dozen times.
> > > 
> > > >  	{0,}
> > > >  };
> > > >  MODULE_DEVICE_TABLE(pci, vmd_ids);
> > > > -- 
> > > > 2.25.1
> > > > 

Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[Jonathan Derrick]

On 10/28/2022 3:22 PM, David E. Box wrote:
> On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
>> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
>>> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
>>>> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>>>>> Add vmd_device_data to allow adding additional info for driver data.
>>>>>  	{PCI_VDEVICE(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,},
>>>>> +		(kernel_ulong_t)&(struct vmd_device_data) {
>>>>> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>> +		},
>>>>> +	},
>>>>
>>>> It looks like these devices come in families where several device IDs
>>>> share the same features.  I think this would be more readable if you
>>>> defined each family outside this table and simply referenced the
>>>> family here.  E.g., you could do something like:
>>>>
>>>>   static struct vmd_device_data vmd_v1 = {
>>>>     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>> 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>> 		VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>   };
>>>
>>> I seem to recall it being similar to this in one of the previous revisions
>>> It's fine with me either way
>>
>> Indeed it was:
>> https://lore.kernel.org/r/366a9602-555f-7a1b-a8db-bbcbf84b7b08@linux.dev
>> I'd forgotten that.
>>
>> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
>> that used the 467f data.  The current series adds two more (0x7d0b
>> 0x0ad0b).  Maybe the "vmd_467f_data" name could have been more
>> descriptive, but the code was definitely shorter:
>>
>>   +     { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
>>   +     { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
>>   +     { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
>>   +     { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>> (kernel_ulong_t)&vmd_467f_data },
> 
> I prefer this too but don't know what's the best name. Could just be by the
> platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
> you think Jonathan?
> 
> David
vmd_client_data ? (meaning product class; client vs enterprise)


> 
>>
>> I do wish pci_device_id.driver_data were a void pointer, as it is for
>> of_device_id, which makes it much more natural to express [1], but
>> that ship has long sailed.
>>
>> [1] 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
>>
>>>>   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>     .driver_data = (kernel_ulong_t) &vmd_v1,
>>>>
>>>> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
>>>> instead of repeating it a half dozen times.
>>>>
>>>>>  	{0,}
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>>> -- 
>>>>> 2.25.1
>>>>>
> 

Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[Patel, Nirmal]

On 10/28/2022 2:14 PM, Jonathan Derrick wrote:
>
> On 10/28/2022 3:22 PM, David E. Box wrote:
>> On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
>>> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
>>>> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
>>>>> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>>>>>> Add vmd_device_data to allow adding additional info for driver data.
>>>>>>  	{PCI_VDEVICE(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,},
>>>>>> +		(kernel_ulong_t)&(struct vmd_device_data) {
>>>>>> +			.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>>> +				    VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>>> +				    VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>>> +		},
>>>>>> +	},
>>>>> It looks like these devices come in families where several device IDs
>>>>> share the same features.  I think this would be more readable if you
>>>>> defined each family outside this table and simply referenced the
>>>>> family here.  E.g., you could do something like:
>>>>>
>>>>>   static struct vmd_device_data vmd_v1 = {
>>>>>     .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>> 		VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>> 		VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>>   };
>>>> I seem to recall it being similar to this in one of the previous revisions
>>>> It's fine with me either way
>>> Indeed it was:
>>> https://lore.kernel.org/r/366a9602-555f-7a1b-a8db-bbcbf84b7b08@linux.dev
>>> I'd forgotten that.
>>>
>>> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
>>> that used the 467f data.  The current series adds two more (0x7d0b
>>> 0x0ad0b).  Maybe the "vmd_467f_data" name could have been more
>>> descriptive, but the code was definitely shorter:
>>>
>>>   +     { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
>>>   +     { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
>>>   +     { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
>>>   +     { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>> (kernel_ulong_t)&vmd_467f_data },
>> I prefer this too but don't know what's the best name. Could just be by the
>> platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
>> you think Jonathan?
>>
>> David
> vmd_client_data ? (meaning product class; client vs enterprise)
Product class would make sense. We can keep adding new IDs.
>
>>> I do wish pci_device_id.driver_data were a void pointer, as it is for
>>> of_device_id, which makes it much more natural to express [1], but
>>> that ship has long sailed.
>>>
>>> [1] 
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
>>>
>>>>>   {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>>     .driver_data = (kernel_ulong_t) &vmd_v1,
>>>>>
>>>>> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
>>>>> instead of repeating it a half dozen times.
>>>>>
>>>>>>  	{0,}
>>>>>>  };
>>>>>>  MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>>>> -- 
>>>>>> 2.25.1
>>>>>>

Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[Christoph Hellwig]

On Fri, Oct 28, 2022 at 02:13:08PM -0500, Bjorn Helgaas wrote:
> It looks like these devices come in families where several device IDs
> share the same features.  I think this would be more readable if you
> defined each family outside this table and simply referenced the
> family here.  E.g., you could do something like:
> 
>   static struct vmd_device_data vmd_v1 = {
>     .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),
>     .driver_data = (kernel_ulong_t) &vmd_v1,
> 
> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> instead of repeating it a half dozen times.

I wonder why we need the ltr field at all.  For those that set it
is always the same value, so it could just be a quirk flag to set it.

Tat being said I think thegrouping makes a lot of sense, but I'd just
do it with a #define for the set of common quirk flags.

Re: [PATCH V7 3/4] PCI: vmd: Add vmd_device_data

[David E. Box]

On Mon, 2022-10-31 at 00:04 -0700, Christoph Hellwig wrote:
> On Fri, Oct 28, 2022 at 02:13:08PM -0500, Bjorn Helgaas wrote:
> > It looks like these devices come in families where several device IDs
> > share the same features.  I think this would be more readable if you
> > defined each family outside this table and simply referenced the
> > family here.  E.g., you could do something like:
> > 
> >   static struct vmd_device_data vmd_v1 = {
> >     .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),
> >     .driver_data = (kernel_ulong_t) &vmd_v1,
> > 
> > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > instead of repeating it a half dozen times.
> 
> I wonder why we need the ltr field at all.  For those that set it
> is always the same value, so it could just be a quirk flag to set it.

Yeah, this makes sense particularly since this isn't intended as a permanent
fix. I'll get rid of it.

> 
> Tat being said I think thegrouping makes a lot of sense, but I'd just
> do it with a #define for the set of common quirk flags.

Works for me. I'll create a VMD_FEATS_CLIENT group but I'll keep the ltr quirk
separate since future client systems won't be using it.

David

[PATCH V7 0/4] PCI: vmd: Enable PCIe ASPM and LTR on select hardware

[David E. Box]

This series adds a work around for enabling PCIe ASPM and for setting PCIe
LTR values on VMD reserved root ports on select platforms. While
configuration of these capabilities is usually done by BIOS, on these
platforms these capabilities will not be configured because the ports are
not visible to BIOS. This was part of an initial design that expected the
driver to completely handle the ports, including power management. However
on Linux those ports are still managed by the PCIe core, which has the
expectation that they adhere to device standards including BIOS
configuration, leading to this problem.

The target platforms are Tiger Lake, Alder Lake, and Raptor Lake though the
latter has already implemented support for configuring the LTR values.
Meteor Lake is expected add BIOS ASPM support, eliminating the future need
for this work around.

Note, the driver programs the LTRs because BIOS would also normally do this
for devices that do not set them by default. Without this, SoC power
management would be blocked on those platform. This SoC specific value is
the maximum latency required to allow the SoC to enter the deepest power
state.

This patch addresses the following open bugzillas on VMD enabled laptops
that cannot enter low power states.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=212355
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215063
Link: https://bugzilla.kernel.org/show_bug.cgi?id=213717

David E. Box (3):
  PCI: vmd: Use PCI_VDEVICE in device list
  PCI: vmd: Add vmd_device_data
  PCI: vmd: Add quirk to configure PCIe ASPM and LTR

Michael Bottini (1):
  PCI/ASPM: Add pci_enable_link_state()

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


base-commit: 247f34f7b80357943234f93f247a1ae6b6c3a740
-- 
2.25.1