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 | 104 ++++++++++++++++++++++++++---------
drivers/pci/pcie/aspm.c | 54 ++++++++++++++++++
include/linux/pci.h | 7 +++
3 files changed, 139 insertions(+), 26 deletions(-)
--
2.25.1
From: Michael Bottini <[email protected]>
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 <[email protected]>
Signed-off-by: David E. Box <[email protected]>
---
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 [email protected]
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
Currently, 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 misconfigured devices 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 is used nor for hotplug.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=213717
Signed-off-by: Michael Bottini <[email protected]>
Signed-off-by: David E. Box <[email protected]>
---
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 [email protected]
drivers/pci/controller/vmd.c | 48 +++++++++++++++++++++++++++++++++++-
1 file changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index a582c351b461..eac379c80cd7 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,38 @@ 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;
+
+ /*
+ * If the LTR capability is present, set the default values to the
+ * maximum required by the platform to allow the deepest power
+ * management savings. Write this as a single DWORD where the lower word
+ * is the max snoop latency and the upper word is the max non-snoop
+ * latency.
+ */
+ pci_info(pdev, "VMD: Setting a default LTR\n");
+ ltr_reg = (info->ltr << 16) | info->ltr;
+ pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
+
+ return 0;
+}
+
static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
{
struct pci_sysdata *sd = &vmd->sysdata;
@@ -867,6 +908,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
@@ -1012,7 +1055,10 @@ static const struct vmd_device_data vmd_28c0_data = {
static const struct vmd_device_data vmd_467f_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,
+ /* 3145728 ns (LatencyScale of 1048576 ns with a LatencyValue of 3) */
+ .ltr = 0x1003,
};
static const struct pci_device_id vmd_ids[] = {
--
2.25.1
Add vmd_device_data to allow adding additional info for driver data. Also
refactor the PCI ID list to use PCI_VDEVICE and simplify assignments for
devices that use the same driver_data.
Signed-off-by: David E. Box <[email protected]>
---
V5
- New patch
drivers/pci/controller/vmd.c | 58 ++++++++++++++++++++----------------
1 file changed, 32 insertions(+), 26 deletions(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index cc166c683638..a582c351b461 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;
@@ -993,30 +999,30 @@ static int vmd_resume(struct device *dev)
#endif
static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
+static const struct vmd_device_data vmd_201d_data = {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
+};
+
+static const struct vmd_device_data vmd_28c0_data = {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_CAN_BYPASS_MSI_REMAP,
+};
+
+static const struct vmd_device_data vmd_467f_data = {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_OFFSET_FIRST_VECTOR,
+};
+
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)&vmd_201d_data },
+ { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0), (kernel_ulong_t)&vmd_28c0_data },
+ { 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 },
+ { }
};
MODULE_DEVICE_TABLE(pci, vmd_ids);
--
2.25.1
On 2/17/2022 9:50 PM, David E. Box wrote:
> Currently, 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 misconfigured devices 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.
Will the refreshes of the affected CPU be supported in BIOS or this workaround?
> This solution is a workaround for current products that mainly targets
> laptops. Support is not provided if a switch is used nor for hotplug.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=213717
>
> Signed-off-by: Michael Bottini <[email protected]>
> Signed-off-by: David E. Box <[email protected]>
> ---
> 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 [email protected]
>
> drivers/pci/controller/vmd.c | 48 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index a582c351b461..eac379c80cd7 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,38 @@ 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;
> +
> + /*
> + * If the LTR capability is present, set the default values to the
> + * maximum required by the platform to allow the deepest power
> + * management savings. Write this as a single DWORD where the lower word
> + * is the max snoop latency and the upper word is the max non-snoop
> + * latency.
> + */
> + pci_info(pdev, "VMD: Setting a default LTR\n");
> + ltr_reg = (info->ltr << 16) | info->ltr;
> + pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
> +
> + return 0;
> +}
> +
> static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
> {
> struct pci_sysdata *sd = &vmd->sysdata;
> @@ -867,6 +908,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
> @@ -1012,7 +1055,10 @@ static const struct vmd_device_data vmd_28c0_data = {
> static const struct vmd_device_data vmd_467f_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,
> + /* 3145728 ns (LatencyScale of 1048576 ns with a LatencyValue of 3) */
> + .ltr = 0x1003,
> };
>
> static const struct pci_device_id vmd_ids[] = {
On Fri, 2022-02-18 at 14:19 -0700, Jonathan Derrick wrote:
> Hi David,
>
> On 2/17/2022 9:50 PM, 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 and simplify
> > assignments for
> > devices that use the same driver_data.
> >
> > Signed-off-by: David E. Box <[email protected]>
> > ---
> >
> > V5
> > - New patch
> >
> > drivers/pci/controller/vmd.c | 58 ++++++++++++++++++++-----------
> > -----
> > 1 file changed, 32 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/pci/controller/vmd.c
> > b/drivers/pci/controller/vmd.c
> > index cc166c683638..a582c351b461 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;
> >
> > @@ -993,30 +999,30 @@ static int vmd_resume(struct device *dev)
> > #endif
> > static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend,
> > vmd_resume);
> >
> > +static const struct vmd_device_data vmd_201d_data = {
> > + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
> > +};
> > +
> > +static const struct vmd_device_data vmd_28c0_data = {
> > + .features = VMD_FEAT_HAS_MEMBAR_SHADOW |
> > + VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > + VMD_FEAT_CAN_BYPASS_MSI_REMAP,
> > +};
> > +
> > +static const struct vmd_device_data vmd_467f_data = {
> > + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > + VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > + VMD_FEAT_OFFSET_FIRST_VECTOR,
> > +};
> > +
>
> I'm not the biggest fan of this structure
> Can you inline the declarations into the vmd_ids table below?
>
>
>
> > 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)&vmd_201d_data },
> > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
> > (kernel_ulong_t)&vmd_28c0_data },
> > + { 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 },
> > + { }
>
> For instance:
> { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
> (kernel_ulong_t)&(struct vmd_device_data) {
> .features =
> VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
Sure.
David
>
>
> > };
> > MODULE_DEVICE_TABLE(pci, vmd_ids);
> >
Hi,
On Fri, 2022-02-18 at 14:30 -0700, Jonathan Derrick wrote:
>
> On 2/17/2022 9:50 PM, David E. Box wrote:
> > Currently, 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 misconfigured devices 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.
> Will the refreshes of the affected CPU be supported in BIOS or this
> workaround?
I'll have to check with the hardware/BIOS team. Still because it is
BIOS this should check for signs that it hasn't already configured ASPM
before modifying anything. I'll make this change.
David
>
> > This solution is a workaround for current products that mainly
> > targets
> > laptops. Support is not provided if a switch is used nor for
> > hotplug.
> >
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=213717
> >
> > Signed-off-by: Michael Bottini <[email protected]>
> > Signed-off-by: David E. Box <[email protected]>
> > ---
> > 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 [email protected]
> >
> > drivers/pci/controller/vmd.c | 48
> > +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/vmd.c
> > b/drivers/pci/controller/vmd.c
> > index a582c351b461..eac379c80cd7 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,38 @@ 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;
> > +
> > + /*
> > + * If the LTR capability is present, set the default values to
> > the
> > + * maximum required by the platform to allow the deepest power
> > + * management savings. Write this as a single DWORD where the
> > lower word
> > + * is the max snoop latency and the upper word is the max non-
> > snoop
> > + * latency.
> > + */
> > + pci_info(pdev, "VMD: Setting a default LTR\n");
> > + ltr_reg = (info->ltr << 16) | info->ltr;
> > + pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT,
> > ltr_reg);
> > +
> > + return 0;
> > +}
> > +
> > static int vmd_enable_domain(struct vmd_dev *vmd, struct
> > vmd_device_data *info)
> > {
> > struct pci_sysdata *sd = &vmd->sysdata;
> > @@ -867,6 +908,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
> > @@ -1012,7 +1055,10 @@ static const struct vmd_device_data
> > vmd_28c0_data = {
> > static const struct vmd_device_data vmd_467f_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,
> > + /* 3145728 ns (LatencyScale of 1048576 ns with a LatencyValue
> > of 3) */
> > + .ltr = 0x1003,
> > };
> >
> > static const struct pci_device_id vmd_ids[] = {
Hi David,
On 2/17/2022 9:50 PM, 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 and simplify assignments for
> devices that use the same driver_data.
>
> Signed-off-by: David E. Box <[email protected]>
> ---
>
> V5
> - New patch
>
> drivers/pci/controller/vmd.c | 58 ++++++++++++++++++++----------------
> 1 file changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index cc166c683638..a582c351b461 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;
>
> @@ -993,30 +999,30 @@ static int vmd_resume(struct device *dev)
> #endif
> static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
>
> +static const struct vmd_device_data vmd_201d_data = {
> + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
> +};
> +
> +static const struct vmd_device_data vmd_28c0_data = {
> + .features = VMD_FEAT_HAS_MEMBAR_SHADOW |
> + VMD_FEAT_HAS_BUS_RESTRICTIONS |
> + VMD_FEAT_CAN_BYPASS_MSI_REMAP,
> +};
> +
> +static const struct vmd_device_data vmd_467f_data = {
> + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> + VMD_FEAT_HAS_BUS_RESTRICTIONS |
> + VMD_FEAT_OFFSET_FIRST_VECTOR,
> +};
> +
I'm not the biggest fan of this structure
Can you inline the declarations into the vmd_ids table below?
> 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)&vmd_201d_data },
> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0), (kernel_ulong_t)&vmd_28c0_data },
> + { 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 },
> + { }
For instance:
{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D), (kernel_ulong_t)&(struct vmd_device_data) {
.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
> };
> MODULE_DEVICE_TABLE(pci, vmd_ids);
>