| Message ID | 20220218045056.333799-4-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: vmd: Enable PCIE ASPM and LTR | expand |
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 <michael.a.bottini@linux.intel.com> > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > 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 | 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, 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 <michael.a.bottini@linux.intel.com> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > --- > > 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 | 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[] = {
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[] = {