Re: [PATCH 2/2] pci: mark USB4 devices as "is_thunderbolt"

[Lukas Wunner <lukas@wunner.de>]

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