| Message ID | 20220204182820.130339-3-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Mark USB4 controllers as is_thunderbolt | expand |
Follow subject line capitalization conventions: $ git log --oneline drivers/pci/probe.c 661c4c4f2693 PCI: Let pcibios_root_bridge_prepare() access bridge->windows d2c64f98c387 PCI: Use pci_find_vsec_capability() when looking for TBT devices fd1ae23b495b PCI: Prefer 'unsigned int' over bare 'unsigned' e1b0d0bb2032 PCI: Re-enable Downstream Port LTR after reset or hotplug 7c3855c423b1 PCI: Coalesce host bridge contiguous apertures 06dc660e6eb8 PCI: Rename pcibios_add_device() to pcibios_device_add() 41dd40fd7179 PCI: Support populating MSI domains of root buses via bridges On Fri, Feb 04, 2022 at 12:28:20PM -0600, Mario Limonciello wrote: Add an intro sentence to tell us what this patch does (mark USB4 devices as 'is_thunderbolt'. I know it's in the subject, but people read the commit log separately and it should be complete in itself. A title is not the first sentence of a book. > Downstream drivers use this information to declare functional > differences in how the drivers performed by knowing that they are > connected to an upstream TBT/USB4 port. s/Downstream drivers/Drivers of downstream devices/ s/performed/perform/ (I guess?) I'm guessing this really refers to differences in how *devices* (not drivers) perform when they are below a Thunderbolt or USB port. I've never liked "is_thunderbolt" because it tells us nothing about what functionality is of interest, so it's an unmaintainable mess. Right now: - We assume Root Ports and Switch Ports marked "is_thunderbolt" support D3 (pci_bridge_d3_possible()). - Downstream Ports marked "is_thunderbolt" don't support native hotplug Command Completed events, even if they claim they do (pcie_init()). - Apparently, if *any* device in the system is marked "is_thunderbolt", a GPU external DP port is not fully switchable because ? (gmux_probe()). - Whether an AMD GPU is attached via Thunderbolt tells us something about what sort of power control and runtime power management we can do (amdgpu_driver_load_kms(), radeon_driver_load_kms()). - We don't register Thunderbolt eGPU devices with VGA switcheroo because ? (nouveau_vga_init(), radeon_device_init()). - If an AMD GPU is attached via Thunderbolt, we program different ASPM time values because ? (nbio_v2_3_enable_aspm()). This is totally bonkers. Broken things like hotplug Command Completed should be quirks. That one is my fault. The ASPM thing should be integrated with the PCI core somehow. It's just asking for trouble to have the PCI core assuming *it* is managing ASPM and then have drivers doing non-standard ASPM stuff behind its back. I have no idea about the VGA switching and power management stuff. But it's not clear to me that these all fit under the "is_thunderbolt" umbrella. If they actually do correspond to Thunderbolt- or USB4- specific features, we should include references to the pertinent sections of the spec. > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/pci/probe.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 17a969942d37..b59f6c05e606 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1581,9 +1581,9 @@ static void set_pcie_thunderbolt(struct pci_dev *dev) > { > u16 vsec; > > - /* Is the device part of a Thunderbolt controller? */ > + /* Is the device part of a Thunderbolt or USB4 controller? */ > vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT); > - if (vsec) > + if (vsec || dev->class == PCI_CLASS_SERIAL_USB_USB4) > dev->is_thunderbolt = 1; This could be rewritten as: if (dev->class == PCI_CLASS_SERIAL_USB_USB4 || pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT)) dev->is_thunderbolt = 1; to avoid searching USB4 devices for a TBT capability unnecessarily. > } > > -- > 2.34.1 >
On Fri, Feb 04, 2022 at 04:29:56PM -0600, Bjorn Helgaas wrote: > I've never liked "is_thunderbolt" because it tells us nothing about > what functionality is of interest, so it's an unmaintainable mess. > > Right now: > > - We assume Root Ports and Switch Ports marked "is_thunderbolt" > support D3 (pci_bridge_d3_possible()). We don't allow D3 on hotplug bridges because: /* * Hotplug ports handled natively by the OS were not validated * by vendors for runtime D3 at least until 2018 because there * was no OS support. */ if (bridge->is_hotplug_bridge) return false; And we don't allow D3 on older non-hotplug bridges because: /* * It should be safe to put PCIe ports from 2015 or newer * to D3. */ if (dmi_get_bios_year() >= 2015) return true; However we must allow D3 on *Thunderbolt* bridges to take advantage of power savings. So the following check is an exception of the above-stated rules: /* Even the oldest 2010 Thunderbolt controller supports D3. */ if (bridge->is_thunderbolt) return true; This is most likely necessary for AMD Thunderbolt as well, but could be achieved by adding another check to pci_bridge_d3_possible() which returns true for the USB4 class. > - Downstream Ports marked "is_thunderbolt" don't support native > hotplug Command Completed events, even if they claim they do > (pcie_init()). That's a quirk needed for older Thunderbolt controllers. It could be replaced by a check for the device IDs listed in 493fb50e958c. It most likely does not affect AMD Thunderbolt. > - Apparently, if *any* device in the system is marked > "is_thunderbolt", a GPU external DP port is not fully switchable > because ? (gmux_probe()). This could be replaced by a DMI check for the affected MacBook Pro models. Those happen not to possess a Thunderbolt controller, so checking for Thunderbolt presence seemed simpler and more clever at the time... I can produce a list of affected models if you want. This does not affect AMD Thunderbolt. > - Whether an AMD GPU is attached via Thunderbolt tells us something > about what sort of power control and runtime power management we > can do (amdgpu_driver_load_kms(), radeon_driver_load_kms()). External eGPUs are not supposed to be managed by vga_switcheroo (which is only responsible for switching between a chipset-integrated iGPU and an on-board discrete dGPU), that's what these checks are for. This does affect AMD Thunderbolt. > - We don't register Thunderbolt eGPU devices with VGA switcheroo > because ? (nouveau_vga_init(), radeon_device_init()). Same as above. > - If an AMD GPU is attached via Thunderbolt, we program different > ASPM time values because ? (nbio_v2_3_enable_aspm()). That wasn't introduced by me, so not sure what the rationale is. Let me know if I can help clarify things further so that we can find a solution that you feel more comfortable with. Thanks, Lukas
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 17a969942d37..b59f6c05e606 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1581,9 +1581,9 @@ static void set_pcie_thunderbolt(struct pci_dev *dev) { u16 vsec; - /* Is the device part of a Thunderbolt controller? */ + /* Is the device part of a Thunderbolt or USB4 controller? */ vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT); - if (vsec) + if (vsec || dev->class == PCI_CLASS_SERIAL_USB_USB4) dev->is_thunderbolt = 1; }
Downstream drivers use this information to declare functional differences in how the drivers performed by knowing that they are connected to an upstream TBT/USB4 port. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/pci/probe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)