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@xxxxxxxxxxxxxxx> > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx> > > Reviewed-by: Jon Derrick <jonathan.derrick@xxxxxxxxx> > > Reviewed-by: Nirmal Patel <nirmal.patel@xxxxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxx > > > > 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); >
