On Mon, Feb 28, 2022 at 08:19:43PM -0800, David E. Box wrote: > 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 they don't > block SoC power management. If the ports aren't visible to BIOS, I assume BIOS doesn't configure *anything*, including LTR. This sentence seems like it has a little too much information; if BIOS doesn't see the ports, LTR, SoC power management, etc., is not relevant. > 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. "Currently affected products" makes me wonder about the *other* products? Seems like we should handle *all* VMD devices the same way. > 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. I guess the cover letter has a little more background on this, although I don't understand how talking to the Intel BIOS team can solve this for *all* vendors using these parts. > Support is not provided if a switch used nor for hotplug. What switch are you referring to? What is the hotplug scenario? Are VMD ports hot-pluggable? I assumed they were built into the Root Complex and not hot-pluggable. s/PCIE/PCIe/ several times above so they're all consistent. s/pci_enable_default_link_state/pci_enable_default_link_state()/ so it looks like a function. That's a big block of text; maybe could be 2-3 paragraphs. > [1] 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> > --- > 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 | 66 +++++++++++++++++++++++++++++++++--- > 1 file changed, 62 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > index cde6e2cba210..8525bb8312f2 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,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_default_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 = (info->ltr << 16) | info->ltr; The fact that you have to hard-code the LTR values in the driver seems problematic because it requires updates for every new device. I guess you have to update the driver anyway to add Device IDs. But surely there should be a firmware interface to discover this platform-specific information? Does the _DSM for Latency Tolerance Reporting (PCI Firmware spec r3.3, sec 4.6.6) supply this? We badly need generic support for that _DSM, but the documentation is somewhat lacking. > + pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg); > + pci_info(pdev, "VMD: Default LTR set\n"); > + > + return 0; > +} > + > static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info) > { > struct pci_sysdata *sd = &vmd->sysdata; > @@ -867,6 +915,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,28 +1066,36 @@ 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, 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 */ > }, > }, > { } > -- > 2.25.1 >
