USB4 host reset (& writing to router config space)
Needs ReviewPublic
Actions

Authored by obiwac on Mar 22 2025, 1:45 AM.

Details

Reviewers

mohammad_fbsdphab_thelightbird.com
emaste
jhb
imp

Summary

Get host interface version (either 1.0 or 2.0) and reset accordingly.

This required support for writing to router config space.

Test Plan

Tested on AMD Pink Sardine controller (1022:1668/9), which is host interface version 1.0. It would be nice to try this out on version 2.0, but unfortunately I don't have the hardware to do this as it is fairly new.

This does require the D49451 patch to work on Pink Sardine.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 72335
Build 69218: arc lint + arc unit

Event Timeline

obiwac created this revision.Mar 22 2025, 1:45 AM

Herald added a subscriber: imp. · View Herald TranscriptMar 22 2025, 1:45 AM

obiwac requested review of this revision.Mar 22 2025, 1:45 AM

Harbormaster completed remote builds in B63047: Diff 152526.Mar 22 2025, 1:45 AM

obiwac added a parent revision: D49450: USB4 base.Mar 22 2025, 1:45 AM

obiwac added a child revision: D49453: USB4 initial work on suspend routine.Mar 22 2025, 1:50 AM

obiwac mentioned this in D49450: USB4 base.Mar 22 2025, 3:21 AM

obiwac added reviewers: emaste, jhb, imp.Mar 22 2025, 3:24 AM

imp added inline comments.Mar 25 2025, 3:19 AM

sys/dev/thunderbolt/nhi.c
272	Why not NHI_SLPR_WAIT_US? C_HARDCLOCK? Why?

obiwac added inline comments.Mar 25 2025, 3:25 PM

sys/dev/thunderbolt/nhi.c
272	`NHI_SLPR_WAIT_US` is for the time between us setting SLP and the router setting SLPR. This is the same number but a different value defined elsewhere in the spec. Including these constants in this patch is a mistake, they're not used. Will remove. As for `C_HARDCLOCK`, I don't know, I had copied this line from another driver assuming it was the same. Will learn about this.

Add missing router defines.

Harbormaster completed remote builds in B63791: Diff 154524.Apr 29 2025, 9:56 PM

vishwin added a subscriber: vishwin.Jun 14 2025, 8:10 PM

This needs a rebase after D52861

rebase

Harbormaster completed remote builds in B72335: Diff 175859.Sun, Apr 19, 1:50 PM

adrian added a subscriber: adrian.Sun, Apr 19, 3:23 PM

adrian added inline comments.

sys/dev/thunderbolt/tbcfg_reg.h
68	Looking at this change, and yeah i downloaded the 2025 spec from here: https://usb.org/document-library/usb4r-specification-v20 config read request is 6.4.2.3, read response is 6.4.2.4 (the section above this one in the source) 6.4.2.5 is clear about the crc being after "write size", not a fixed location like this. having 0 entry field and then a fixed size field afterwards feels wrong? ;-) So I think this change isn't correct?

obiwac added inline comments.Tue, Apr 21, 9:59 AM

sys/dev/thunderbolt/tbcfg_reg.h
68	config read request is 6.4.2.3, read response is 6.4.2.4 (the section above this one in the source) Yeah there's a typo in the `struct tb_cfg_read_resp` comment actually, it should be read response not request. So I think this change isn't correct? I can't remember why I made it an actual member, I think probably because it doesn't make a functional difference and maybe thought it could be useful to sizeof the crc. But i agree it is a bit bizarre so I'll revert it back to being a comment.

what do i need to check in dmesg / pciconf to see if i have something that can test this?

In D49452#1295289, @adrian wrote:

what do i need to check in dmesg / pciconf to see if i have something that can test this?

you should have a generic USB4 NHI controller (listed in pciconf), that is class 0x0c0340. If it is AMD, you probably need to apply D52862 first though