Does this mean all TB3 support will be removed? Is there more to cleanup in that case?

Does this mean all TB3 support will be removed?

as I understood it, USB4 pretty much just standardized existing TB3 controllers, so I guess it would be possible to have a TB3 controller for which a USB4 driver/HCM would work. Not that I know of any such controllers. This is the existing check to set HCM usage, which would maybe imply there are some devices which normally use an ICM but for which a HCM would work:

	if (NHI_IS_USB4(sc) || (sc->force_hcm == NHI_FORCE_HCM_ON))
		sc->caps |= NHI_CAP_HCM;

maybe @scottl could elucidate this

Is there more to cleanup in that case?

yeah, we should anyway also remove all this force_hcm stuff seeing as that's all we're going to support.

The relationship between TB3 and USB4 isn't as simple as you suggest. There are optional vs mandatory feature considerations, there are differences in the bus properties and device config registers, there are differences in the necessary behavior of the Connection Manager, not to mention that an HCM is mandatory for USB4 but not for TB3, and there are even differences in the NHI programming interface. A better comparison is TB4 and USB4, their programming interfaces and functional attributes are more closely related, with TB4 being a superset of USB4.

If you've removed the ICM code, then don't even think about supporting TB3 hosts in the future. However, the tb_pci and picb devices are kind of special. They exist in device peripherals, not just hosts, and there are a lot of devices out there that use AR as their internal controller. Supporting them doesn't mean that you have to support an AR NHI controller. If you want to remove the code from freebsd, that's fine I guess, but it was nice to have the tb pci devices clearly identified in the device tree and dmesg. Also, keep in mind that if you support TB3 devices, your USB4 NHI and HCM code will need to explicitly support them.

Honestly, I'm kinda confused about the state of this code in freebsd. The ICM code was removed, which removes any control that the OS has over TB3 controllers and topology. An HCM doesn't exist yet, correct(?), and neither does USB4-specific enablement code in NHI? If so, then what functionality does this code have in the freebsd repo at the moment? Don't get me wrong, I'm glad that people are interested in this code and in USB4, but there was also a reason that I didn't commit this code back in 2022, it wasn't anywhere close to being ready yet =-)

In D52861#1273135, @scottl wrote:

...

Honestly, I'm kinda confused about the state of this code in freebsd. The ICM code was removed, which removes any control that the OS has over TB3 controllers and topology. An HCM doesn't exist yet, correct(?), and neither does USB4-specific enablement code in NHI? If so, then what functionality does this code have in the freebsd repo at the moment? Don't get me wrong, I'm glad that people are interested in this code and in USB4, but there was also a reason that I didn't commit this code back in 2022, it wasn't anywhere close to being ready yet =-)

Wait... the TB3/ICM code was removed from FreeBSD...? If so, that completely broke any hope of being able to use TB on my Intel MacBook hardware :(. My MacBookPro13,1, MacBookPro16,1, and Macmini8,1 all rely on TB3 to function :(.

The relationship between TB3 and USB4 isn't as simple as you suggest. There are optional vs mandatory feature considerations, there are differences in the bus properties and device config registers, there are differences in the necessary behavior of the Connection Manager, not to mention that an HCM is mandatory for USB4 but not for TB3, and there are even differences in the NHI programming interface. A better comparison is TB4 and USB4, their programming interfaces and functional attributes are more closely related, with TB4 being a superset of USB4.

Got it, thanks for clearing that up. My previous impression was that USB4 was a superset of TB3. Do you have a specific source for this or some comprehensive overview? It's been a little while since I've worked on this but all I really went off was the USB4 spec and didn't find too much info on TB3, and the only hardware I was really concerned about was USB4-only anyways.

If you've removed the ICM code, then don't even think about supporting TB3 hosts in the future. However, the tb_pci and picb devices are kind of special. They exist in device peripherals, not just hosts, and there are a lot of devices out there that use AR as their internal controller. Supporting them doesn't mean that you have to support an AR NHI controller. If you want to remove the code from freebsd, that's fine I guess, but it was nice to have the tb pci devices clearly identified in the device tree and dmesg. Also, keep in mind that if you support TB3 devices, your USB4 NHI and HCM code will need to explicitly support them.

The ICM code can be added back later down the road if we want to support this.

Honestly, I'm kinda confused about the state of this code in freebsd. The ICM code was removed, which removes any control that the OS has over TB3 controllers and topology. An HCM doesn't exist yet, correct(?), and neither does USB4-specific enablement code in NHI? If so, then what functionality does this code have in the freebsd repo at the moment? Don't get me wrong, I'm glad that people are interested in this code and in USB4, but there was also a reason that I didn't commit this code back in 2022, it wasn't anywhere close to being ready yet =-)

My main purpose for bringing this code in was to be able to suspend the USB4 controller, which is necessary for S0i3 entry on certain platforms (due to a "pre-OS" connection manager leaving the controller on when the OS boots by default on these platforms - see D49453). At some point I would like to work on getting a full HCM written though.

Wait... the ICM code was removed from FreeBSD...? If so, that completely broke any hope of being able to use TB on my Intel MacBook hardware :(. My MacBookPro13,1, MacBookPro16,1, and Macmini8,1 all rely on TB3 to function :(.

It was never in FreeBSD; I left it out in my original import of the USB4 (D49450). The bits this revision remove are bits that I probably should've removed during the initial import but were left over. But if someone is willing to work on support for TB3, then it can totally be brought in!

In D52861#1273156, @obiwac wrote:

...

If you've removed the ICM code, then don't even think about supporting TB3 hosts in the future. However, the tb_pci and picb devices are kind of special. They exist in device peripherals, not just hosts, and there are a lot of devices out there that use AR as their internal controller. Supporting them doesn't mean that you have to support an AR NHI controller. If you want to remove the code from freebsd, that's fine I guess, but it was nice to have the tb pci devices clearly identified in the device tree and dmesg. Also, keep in mind that if you support TB3 devices, your USB4 NHI and HCM code will need to explicitly support them.

The ICM code can be added back later down the road if we want to support this.

Honestly, I'm kinda confused about the state of this code in freebsd. The ICM code was removed, which removes any control that the OS has over TB3 controllers and topology. An HCM doesn't exist yet, correct(?), and neither does USB4-specific enablement code in NHI? If so, then what functionality does this code have in the freebsd repo at the moment? Don't get me wrong, I'm glad that people are interested in this code and in USB4, but there was also a reason that I didn't commit this code back in 2022, it wasn't anywhere close to being ready yet =-)

My main purpose for bringing this code in was to be able to suspend the USB4 controller, which is necessary for S0i3 entry on certain platforms (due to a "pre-OS" connection manager leaving the controller on when the OS boots by default on these platforms - see D49453). At some point I would like to work on getting a full HCM written though.

Wait... the ICM code was removed from FreeBSD...? If so, that completely broke any hope of being able to use TB on my Intel MacBook hardware :(. My MacBookPro13,1, MacBookPro16,1, and Macmini8,1 all rely on TB3 to function :(.

It was never in FreeBSD; I left it out in my original import of the USB4 (D49450). The bits this revision remove are bits that I probably should've removed during the initial import but were left over. But if someone is willing to work on support for TB3, then it can totally be brought in!

I see.

If I had access to the specs, I would like to get ICM/HCM working (which includes investing time in making that functionality in TB3 work). It's mildly annoying how I need to have separate RJ-45 dongles for communication between TB nodes when (in reality) I could just get 2 hosts to talk directly with each other over another IP-like protocol using a TB3/4 capable cable.

@guest-seuros and I are working on getting Intel Mac hardware working on FreeBSD. Getting something like ICM in would be helpful and help with interop between FreeBSD and macOS. Once later gen Intel Mac hardware works (iBridge, SMC support, etc), moving to Apple Silicon based platforms should be easier to do.

If I had access to the specs, I would like to get ICM/HCM working (which includes investing time in making that functionality in TB3 work). It's mildly annoying how I need to have separate RJ-45 dongles for communication between TB nodes when (in reality) I could just get 2 hosts to talk directly with each other over another IP-like protocol using a TB3/4 capable cable.

I guess we should first make sure the hardware you have doesn't work with a HCM. I'm assuming Linux has an equivalent of the hw.nhi.force_hcm tunable this revision removes that you could use to test. If not, then I can add the ICM code in (and work on cleaning up/landing my subsequent patches :)). Speaking of which, @scottl can I add you as a reviewer on those?

You can get the USB4 spec from here: https://www.usb.org/document-library/usb4r-specification-v20

The two most relevant files are:

• USB4 Connection Manager Guide Ver. 2.0 - rev. 1.1.pdf
• USB4 V2 with Errata and ECN through September 2024- CLEAN.pdf

Things were relatively clear and easy to read as far as specs go.

I must decline being a reviewer or otherwise being involved in the freebsd code. Happy to discuss publicly available information regarding the tech in general.

I think that if you dig deep into the linux nhi and thunderbolt code you'll see some TB3 vs USB4 differences. I know what I know based on my time at Intel and my access to the TB3 specs at the time, obviously I can't share those with you.

In D52861#1273246, @scottl wrote:

I must decline being a reviewer or otherwise being involved in the freebsd code. Happy to discuss publicly available information regarding the tech in general.

I think that if you dig deep into the linux nhi and thunderbolt code you'll see some TB3 vs USB4 differences. I know what I know based on my time at Intel and my access to the TB3 specs at the time, obviously I can't share those with you.

got it :) Thanks for your input so far!

Either return the PCI IDs in the tb_pcib driver, or remove the entire driver. With this review, the entire driver is dead code.

In D52861#1273277, @scottl wrote:

Either return the PCI IDs in the tb_pcib driver, or remove the entire driver. With this review, the entire driver is dead code.

Well no, it still has all the ring and interrupt setup that allows me to read/write to the router config space, which I need for resetting the controller and suspending it (again, my primary reason for bringing this code in now).

And I'll add the Pink Sardine PCI bridge IDs in D52862, which is the only hardware I have to test this on.

Maybe I’m confused, I was referring to tb_pcib.c.

In D52861#1273704, @scottl wrote:

Maybe I’m confused, I was referring to tb_pcib.c.

ah right, yes indeed. Do any of these work without the equivalent NHI support though? I don't have the hardware to test these but my impression was that a connection manager of some sort was necessary to actually configure the bridge to work.

In D52861#1273741, @obiwac wrote:

In D52861#1273704, @scottl wrote:

Maybe I’m confused, I was referring to tb_pcib.c.

ah right, yes indeed. Do any of these work without the equivalent NHI support though? I don't have the hardware to test these but my impression was that a connection manager of some sort was necessary to actually configure the bridge to work.

That was my concern too.

If you remove all of the TB3 PCI IDs from the NHI and PCIB drivers, the driver is basically useless for TB3 since it won't probe and attach to anything but generic USB4-capable controllers like the "Pink Sardine" ones (as-is). If you could generalize the PCIP_SERIALBUS_USB_USB4 targets vs the Intel TB3 controllers and remove the "Pink Sardine" controllers from the PCI IDs list, I would be totally game for this particular change since it would simplify adding and supporting new drivers (kind of like I did with asmc(4)).

Linux supports per-device quirks for many drivers. We will inevitably likely have to employ similar because of how inconsistent device implementations tend to be with specs. It's best to not nerf this code until it really needs to be nerfed, and it would be nice if the ICM code was actually in the tree.

If you remove all of the TB3 PCI IDs from the NHI and PCIB drivers, the driver is basically useless for TB3 since it won't probe and attach to anything but generic USB4-capable controllers like the "Pink Sardine" ones (as-is).

Yes, but for the time being it isn't supposed to be useful for other controllers and I do not intend to work on these. I don't want to have code in the tree that I'm not working on, and it really wouldn't be that difficult to add the ICM code later on if someone wants to work on that.

If you could generalize the PCIP_SERIALBUS_USB_USB4 targets vs the Intel TB3 controllers and remove the "Pink Sardine" controllers from the PCI IDs list, I would be totally game for this particular change since it would simplify adding and supporting new drivers (kind of like I did with asmc(4)).
Linux supports per-device quirks for many drivers. We will inevitably likely have to employ similar because of how inconsistent device implementations tend to be with specs.

Yeah, we can do something like in asmc eventually.

It's best to not nerf this code until it really needs to be nerfed, and it would be nice if the ICM code was actually in the tree.

I really disagree with this. I don't want people using code that I have committed, that I have no way of testing myself, and have no intentions of supporting at the moment. I really don't see the issue with someone adding this at a later date if they want to work on it, and I'm glad to help you out if you need to add back the removed logic for this.

I will add a notice in the manpage that this driver explicitly only exists to allow us to suspend USB4 controllers to clear up confusion.

In D52861#1281083, @obiwac wrote:

If you remove all of the TB3 PCI IDs from the NHI and PCIB drivers, the driver is basically useless for TB3 since it won't probe and attach to anything but generic USB4-capable controllers like the "Pink Sardine" ones (as-is).

Yes, but for the time being it isn't supposed to be useful for other controllers and I do not intend to work on these. I don't want to have code in the tree that I'm not working on, and it really wouldn't be that difficult to add the ICM code later on if someone wants to work on that.

If you could generalize the PCIP_SERIALBUS_USB_USB4 targets vs the Intel TB3 controllers and remove the "Pink Sardine" controllers from the PCI IDs list, I would be totally game for this particular change since it would simplify adding and supporting new drivers (kind of like I did with asmc(4)).
Linux supports per-device quirks for many drivers. We will inevitably likely have to employ similar because of how inconsistent device implementations tend to be with specs.

Yeah, we can do something like in asmc eventually.

It's best to not nerf this code until it really needs to be nerfed, and it would be nice if the ICM code was actually in the tree.

I really disagree with this. I don't want people using code that I have committed, that I have no way of testing myself, and have no intentions of supporting at the moment. I really don't see the issue with someone adding this at a later date if they want to work on it, and I'm glad to help you out if you need to add back the removed logic for this.

I will add a notice in the manpage that this driver explicitly only exists to allow us to suspend USB4 controllers to clear up confusion.

*sigh* could you please leave what's in there? If you commit the rest of the ICM code you got from hselasky, I'll work on making it function on my TB3 devices. I have a vested interest in making this stuff work on my Macs since the alternatives are all Linux based solutions due to the combination of Mac hardware support and ZFS (bleh..). Or, just pass along the rest of the code and I'll work on adding the rest of it back at a later date if it's such an impedance to what you're working on ACPI wise.

If you commit the rest of the ICM code you got from hselasky, I'll work on making it function on my TB3 devices.

I don't understand why you can't just do this when you get around to it. I'm not going to commit something that I have no way of testing and that I'm not supporting.

Or, just pass along the rest of the code and I'll work on adding the rest of it back at a later date if it's such an impedance to what you're working on ACPI wise.

The code is public: https://github.com/hselasky/usb4

It's not a question of it being an impedance to have this code, it's a question of me committing something I'm not able to test and not in a position to support.

@ngie Having the ICM code might not be enough to make the driver work on certain hardware. If you compare this to the linux driver, there is a lot of functionality that I didn't implement in my driver. Be prepared for a lot of work.

I said I would help Aymeric with this review, from a quick read it seems fine. I haven't thrown it at hardware and I'm not really that familiar with the TB/USB4 side of things.

@ngie would you be able to suggest a review that adds the minimum functionality you need?

I'm not really one to push on reviews, but in this case Aymeric has been pretty clear with why this code isn't really something to have in tree.
As this is holding back s0iX support I think we should land this change and revisit TB3, or minimal TB3 support when we have active patches.

In D52861#1288825, @thj wrote:

I said I would help Aymeric with this review, from a quick read it seems fine. I haven't thrown it at hardware and I'm not really that familiar with the TB/USB4 side of things.

@ngie would you be able to suggest a review that adds the minimum functionality you need?

I'm not really one to push on reviews, but in this case Aymeric has been pretty clear with why this code isn't really something to have in tree.
As this is holding back s0iX support I think we should land this change and revisit TB3, or minimal TB3 support when we have active patches.

I need to do the legwork of figuring what is required in order to get a working TB3 stack... I'll revisit some of my Titan Ridge patches once D55573 is in.