| Message ID | 20221119021411.1383248-5-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: vmd: Enable PCIe ASPM and LTR on select | expand |
HI, On 11/18/22 6:14 PM, David E. Box wrote: > 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 bugzilla. For affected products 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 is a workaround for products from Rocket Lake through Alder Lake. > Raptor Lake, the latest product at this time, has already implemented LTR > configuring in BIOS. Future products will move ASPM configuration back to > BIOS as well. As this solution is intended for laptops, support is not > added for hotplug or for devices downstream of a switch on the root port. > > 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> > --- > V8 > - Removed struct vmd_device_data patch. Instead use #define for the LTR > value which is the same across all products needing the quirk. > V7 > - No change > 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 | 64 ++++++++++++++++++++++++++++++++---- > 1 file changed, 58 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > index 86f3085db014..cba57e3091f6 100644 > --- a/drivers/pci/controller/vmd.c > +++ b/drivers/pci/controller/vmd.c > @@ -66,12 +66,22 @@ 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), > }; > > #define VMD_FEATS_CLIENT (VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP | \ > VMD_FEAT_HAS_BUS_RESTRICTIONS | \ > VMD_FEAT_OFFSET_FIRST_VECTOR) > > +#define VMD_BIOS_PM_QUIRK_LTR 0x1003 /* 3145728 ns */ > + > static DEFINE_IDA(vmd_instance_ida); > > /* > @@ -713,6 +723,46 @@ 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) > +{ > + unsigned long features = *(unsigned long *)userdata; > + u16 ltr = VMD_BIOS_PM_QUIRK_LTR; > + u32 ltr_reg; > + int pos; > + > + if (!(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, <r_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 = (ltr << 16) | 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, unsigned long features) > { > struct pci_sysdata *sd = &vmd->sysdata; > @@ -867,6 +917,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) > pci_reset_bus(child->self); > pci_assign_unassigned_bus_resources(vmd->bus); > > + pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features); > + > /* > * VMD root buses are virtual and don't return true on pci_is_pcie() > * and will fail pcie_bus_configure_settings() early. It can instead be > @@ -1005,17 +1057,17 @@ static const struct pci_device_id vmd_ids[] = { > VMD_FEAT_HAS_BUS_RESTRICTIONS | > VMD_FEAT_CAN_BYPASS_MSI_REMAP,}, > {PCI_VDEVICE(INTEL, 0x467f), > - .driver_data = VMD_FEATS_CLIENT,}, > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > {PCI_VDEVICE(INTEL, 0x4c3d), > - .driver_data = VMD_FEATS_CLIENT,}, > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > {PCI_VDEVICE(INTEL, 0xa77f), > - .driver_data = VMD_FEATS_CLIENT,}, > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > {PCI_VDEVICE(INTEL, 0x7d0b), > - .driver_data = VMD_FEATS_CLIENT,}, > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > {PCI_VDEVICE(INTEL, 0xad0b), > - .driver_data = VMD_FEATS_CLIENT,}, > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B), > - .driver_data = VMD_FEATS_CLIENT,}, > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, Why not add VMD_FEAT_BIOS_PM_QUIRK part of VMD_FEATS_CLIENT? > {0,} > }; > MODULE_DEVICE_TABLE(pci, vmd_ids);
On Fri, 2022-11-18 at 21:50 -0800, Sathyanarayanan Kuppuswamy wrote: > HI, > > On 11/18/22 6:14 PM, David E. Box wrote: > > 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 bugzilla. For affected products 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 is a workaround for products from Rocket Lake through Alder Lake. > > Raptor Lake, the latest product at this time, has already implemented LTR > > configuring in BIOS. Future products will move ASPM configuration back to > > BIOS as well. As this solution is intended for laptops, support is not > > added for hotplug or for devices downstream of a switch on the root port. > > > > 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> > > --- > > V8 > > - Removed struct vmd_device_data patch. Instead use #define for the LTR > > value which is the same across all products needing the quirk. > > V7 > > - No change > > 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 | 64 ++++++++++++++++++++++++++++++++---- > > 1 file changed, 58 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > > index 86f3085db014..cba57e3091f6 100644 > > --- a/drivers/pci/controller/vmd.c > > +++ b/drivers/pci/controller/vmd.c > > @@ -66,12 +66,22 @@ 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), > > }; > > > > #define VMD_FEATS_CLIENT (VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP | \ > > VMD_FEAT_HAS_BUS_RESTRICTIONS | \ > > VMD_FEAT_OFFSET_FIRST_VECTOR) > > > > +#define VMD_BIOS_PM_QUIRK_LTR 0x1003 /* 3145728 ns */ > > + > > static DEFINE_IDA(vmd_instance_ida); > > > > /* > > @@ -713,6 +723,46 @@ 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) > > +{ > > + unsigned long features = *(unsigned long *)userdata; > > + u16 ltr = VMD_BIOS_PM_QUIRK_LTR; > > + u32 ltr_reg; > > + int pos; > > + > > + if (!(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, <r_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 = (ltr << 16) | 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, unsigned long features) > > { > > struct pci_sysdata *sd = &vmd->sysdata; > > @@ -867,6 +917,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, > > unsigned long features) > > pci_reset_bus(child->self); > > pci_assign_unassigned_bus_resources(vmd->bus); > > > > + pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features); > > + > > /* > > * VMD root buses are virtual and don't return true on pci_is_pcie() > > * and will fail pcie_bus_configure_settings() early. It can instead > > be > > @@ -1005,17 +1057,17 @@ static const struct pci_device_id vmd_ids[] = { > > VMD_FEAT_HAS_BUS_RESTRICTIONS | > > VMD_FEAT_CAN_BYPASS_MSI_REMAP,}, > > {PCI_VDEVICE(INTEL, 0x467f), > > - .driver_data = VMD_FEATS_CLIENT,}, > > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > > {PCI_VDEVICE(INTEL, 0x4c3d), > > - .driver_data = VMD_FEATS_CLIENT,}, > > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > > {PCI_VDEVICE(INTEL, 0xa77f), > > - .driver_data = VMD_FEATS_CLIENT,}, > > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > > {PCI_VDEVICE(INTEL, 0x7d0b), > > - .driver_data = VMD_FEATS_CLIENT,}, > > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > > {PCI_VDEVICE(INTEL, 0xad0b), > > - .driver_data = VMD_FEATS_CLIENT,}, > > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > > {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B), > > - .driver_data = VMD_FEATS_CLIENT,}, > > + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, > > Why not add VMD_FEAT_BIOS_PM_QUIRK part of VMD_FEATS_CLIENT? Because our VMD team is in the middle of removing the need for the current on next gen. David > > > {0,} > > }; > > MODULE_DEVICE_TABLE(pci, vmd_ids); >
On 11/20/22 7:30 PM, David E. Box wrote: >>> + >>> static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) >>> { >>> struct pci_sysdata *sd = &vmd->sysdata; >>> @@ -867,6 +917,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, >>> unsigned long features) >>> pci_reset_bus(child->self); >>> pci_assign_unassigned_bus_resources(vmd->bus); >>> >>> + pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features); >>> + >>> /* >>> * VMD root buses are virtual and don't return true on pci_is_pcie() >>> * and will fail pcie_bus_configure_settings() early. It can instead >>> be >>> @@ -1005,17 +1057,17 @@ static const struct pci_device_id vmd_ids[] = { >>> VMD_FEAT_HAS_BUS_RESTRICTIONS | >>> VMD_FEAT_CAN_BYPASS_MSI_REMAP,}, >>> {PCI_VDEVICE(INTEL, 0x467f), >>> - .driver_data = VMD_FEATS_CLIENT,}, >>> + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, >>> {PCI_VDEVICE(INTEL, 0x4c3d), >>> - .driver_data = VMD_FEATS_CLIENT,}, >>> + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, >>> {PCI_VDEVICE(INTEL, 0xa77f), >>> - .driver_data = VMD_FEATS_CLIENT,}, >>> + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, >>> {PCI_VDEVICE(INTEL, 0x7d0b), >>> - .driver_data = VMD_FEATS_CLIENT,}, >>> + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, >>> {PCI_VDEVICE(INTEL, 0xad0b), >>> - .driver_data = VMD_FEATS_CLIENT,}, >>> + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, >>> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B), >>> - .driver_data = VMD_FEATS_CLIENT,}, >>> + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, >> Why not add VMD_FEAT_BIOS_PM_QUIRK part of VMD_FEATS_CLIENT? > Because our VMD team is in the middle of removing the need for the current on > next gen. You mean you may not need this quirk support from next gen? It looks like you are adding this quirk to all occurances of VMD_FEATS_CLIENT. So I am still not clear why we can't add it directly to that macro?
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c index 86f3085db014..cba57e3091f6 100644 --- a/drivers/pci/controller/vmd.c +++ b/drivers/pci/controller/vmd.c @@ -66,12 +66,22 @@ 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), }; #define VMD_FEATS_CLIENT (VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP | \ VMD_FEAT_HAS_BUS_RESTRICTIONS | \ VMD_FEAT_OFFSET_FIRST_VECTOR) +#define VMD_BIOS_PM_QUIRK_LTR 0x1003 /* 3145728 ns */ + static DEFINE_IDA(vmd_instance_ida); /* @@ -713,6 +723,46 @@ 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) +{ + unsigned long features = *(unsigned long *)userdata; + u16 ltr = VMD_BIOS_PM_QUIRK_LTR; + u32 ltr_reg; + int pos; + + if (!(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, <r_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 = (ltr << 16) | 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, unsigned long features) { struct pci_sysdata *sd = &vmd->sysdata; @@ -867,6 +917,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) pci_reset_bus(child->self); pci_assign_unassigned_bus_resources(vmd->bus); + pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features); + /* * VMD root buses are virtual and don't return true on pci_is_pcie() * and will fail pcie_bus_configure_settings() early. It can instead be @@ -1005,17 +1057,17 @@ static const struct pci_device_id vmd_ids[] = { VMD_FEAT_HAS_BUS_RESTRICTIONS | VMD_FEAT_CAN_BYPASS_MSI_REMAP,}, {PCI_VDEVICE(INTEL, 0x467f), - .driver_data = VMD_FEATS_CLIENT,}, + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, {PCI_VDEVICE(INTEL, 0x4c3d), - .driver_data = VMD_FEATS_CLIENT,}, + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, {PCI_VDEVICE(INTEL, 0xa77f), - .driver_data = VMD_FEATS_CLIENT,}, + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, {PCI_VDEVICE(INTEL, 0x7d0b), - .driver_data = VMD_FEATS_CLIENT,}, + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, {PCI_VDEVICE(INTEL, 0xad0b), - .driver_data = VMD_FEATS_CLIENT,}, + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B), - .driver_data = VMD_FEATS_CLIENT,}, + .driver_data = VMD_FEATS_CLIENT | VMD_FEAT_BIOS_PM_QUIRK,}, {0,} }; MODULE_DEVICE_TABLE(pci, vmd_ids);