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

Lukas Wunner
2022-02-05T09:39:36Z

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