Paths

Table of Contentst

USB: split dwc3 (Synopsys) up for FDT and ACPI attachment
ClosedPublic
Actions

Authored by bz on Oct 1 2021, 12:55 PM.

Details

Reviewers

manu
mw

Commits

rG30f6e3752287: dwc3: add ACPI attachment
rGfbb5cb66f74c: dwc3: add ACPI attachment

Summary

Split the current FDT-only implementation up into an FDT and an
ACPI part. Split the softcs up into a "common" part and per-bus
version and use the common one for common functions needed by both.
Given we are dealing with 2.5 softcs don't used "sc" for xhci (now xsc)
or the common part (now csc). Switch to device_{get,has}_property
for common configuration and quirks handling.

This makes the Synopsis XHCI root hubs attach correctly on
SolidRun's HoenyComb instead of just the generic XHCI root and
this means we are also doing proper chip setup and applying the
quirk needed there [1].

[1] https://github.com/SolidRun/edk2-platforms/blob/24698f90b79facfbbfc4067b39a4ddf8c7fdfa88/Silicon/NXP/LX2160A/AcpiTables/Dsdt/Usb.asl

Test Plan

Tested with UEFI attach. I have no FDT device to quickly test this.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bz created this revision.Oct 1 2021, 12:55 PM

Herald added subscribers: emaste, andrew, imp. · View Herald TranscriptOct 1 2021, 12:55 PM

bz requested review of this revision.Oct 1 2021, 12:55 PM

Harbormaster completed remote builds in B41882: Diff 96068.Oct 1 2021, 12:55 PM

(surprised I cannot find a USB project in Phabricator)

bz added a parent revision: D32257: USB: adjust the Generic XHCI ACPI probe return value.Oct 1 2021, 1:04 PM

dan.kotowski_a9development.com added a subscriber: dan.kotowski_a9development.com.Oct 1 2021, 2:28 PM

Could you split this in 3 commits ?
1 that deals with snps,dis_rxdet_inp3_quirk
1 that deals with OF_hasprop -> device_has_property
1 that splits

Things are hard to review right now.
Also I think that you have some rk3399 board, that will be good to test fdt part on it.

I have not tested stability or actual bandwidth yet, but on my Honeycomb I am finally seeing USB3.0 devices negotiate to superspeed. This seems a bit inconsistent however as sometimes superspeed devices still only negotiate to highspeed, but my initial testing leads me to believe that my cabling is at least partially at fault.

[root@honeycomb ~]# usbconfig show_ifdrv | grep -A1 -F 'spd=SUPER'
ugen1.1: <Generic XHCI root HUB> at usbus1, cfg=0 md=HOST spd=SUPER (5.0Gbps) pwr=SAVE (0mA)
ugen1.1.0: uhub0: <Generic XHCI root HUB, class 9/0, rev 3.00/1.00, addr 1>
ugen0.1: <Generic XHCI root HUB> at usbus0, cfg=0 md=HOST spd=SUPER (5.0Gbps) pwr=SAVE (0mA)

ugen0.1.0: uhub1: <Generic XHCI root HUB, class 9/0, rev 3.00/1.00, addr 1>

ugen1.8: <vendor 0x04b4 product 0x6500> at usbus1, cfg=0 md=HOST spd=SUPER (5.0Gbps) pwr=SAVE (0mA)

ugen1.8.0: uhub5: <vendor 0x04b4 product 0x6500, class 9/0, rev 3.00/50.10, addr 7>

ugen1.4: <GenesysLogic USB3.0 Hub> at usbus1, cfg=0 md=HOST spd=SUPER (5.0Gbps) pwr=SAVE (0mA)
ugen1.4.0: uhub6: <GenesysLogic>
ugen1.5: <Microchip LAN7800> at usbus1, cfg=0 md=HOST spd=SUPER (5.0Gbps) pwr=ON (224mA)

ugen1.5.0: muge0: <Microchip LAN7800, rev 3.10/3.00, addr 26>

ugen1.7: <Corsair Voyager GTX> at usbus1, cfg=0 md=HOST spd=SUPER (5.0Gbps) pwr=ON (224mA)
ugen1.7.0: umass0: <Corsair Voyager GTX, class 0/0, rev 3.00/1.00, addr 30>

+1 to the 3 patches split. I also think there may be a single sc - things specific for DT (namely phy_t references) can be hidden under #ifdef EXT_RESOURCES.

sys/dev/usb/controller/dwc3.c
337	This is a separate functional change. Please move to a separate smaller commit.
sys/dev/usb/controller/dwc3_fdt.c
136 ↗	(On Diff #96068)	Please move above block to a common subroutine - it can help avoiding some duplication.
154 ↗	(On Diff #96068)	I think it should be safe to execute this routine before snps_dwc3_reset (in ACPI world the phy is configured by firmware) - if confirmed, we can move: snps_dwc3_reset(csc); snps_dwc3_configure_host(csc); snps_dwc3_do_quirks(csc); snsp_dwc3_dump_regs(csc); to a common snps_dwc3_attach_xhci.
156 ↗	(On Diff #96068)	Please replace if 0 with a DEBUG flag.

This revision now requires changes to proceed.Oct 1 2021, 5:05 PM

I'll happily do the split up. I might still have the nanopi-m4v2 lying around somewhere; I'll let you know.

bz added inline comments.Oct 1 2021, 5:50 PM

sys/dev/usb/controller/dwc3_fdt.c
154 ↗	(On Diff #96068)	How do you then want to "hide" the phy_get_by_ofw_name() stuff in the ACPI case? That's the thing I couldn't figure out last night at 2AM.
156 ↗	(On Diff #96068)	Yeah I can; or even a tunable with is 0 by default. This is just a copy from the current code.

SR changed the HID and CID which is why the new version didn't actually attach properly: https://github.com/SolidRun/edk2-platforms/commit/7de27183f473debe05e77cf0f0aa4b14479e4152

In D32256#728076, @dan.kotowski_a9development.com wrote:

SR changed the HID and CID which is why the new version didn't actually attach properly: https://github.com/SolidRun/edk2-platforms/commit/7de27183f473debe05e77cf0f0aa4b14479e4152

Well, they basically deleted 808622B7 and only left the generic PNP0D10.

Unless they are going to completely handle all DesignWare quirks in firmware this doesn't seem correct…

mw added inline comments.Oct 3 2021, 7:49 AM

sys/dev/usb/controller/dwc3_fdt.c
154 ↗	(On Diff #96068)	I wrote 2 comments at the same time - in the other I was suggesting to leave obtaining phys from DT in snps_dwc3_fdt_attach and move as much as possible to the common attach routine. In case everything worked as expected, that would allow to shrink snps_dwc3_fdt_attach to: allocate resources phy_get_by_ofw_name snps_dwc3_configure_phy snps_dwc3_attach_xhci And snps_dwc3_fdt_attach to: allocate resources snps_dwc3_attach_xhci

Unless they are going to completely handle all DesignWare quirks in firmware this doesn't seem correct…

From Jon at SR:

Almost all the quirks are setup in the firmware, but for the dwc3 controller some of them need to be setup during times like suspend/resume or new devices plugged in. The quirks that are included in the _DSD are piggy backing off the device-tree quirks.

bz added a child revision: D32921: USB dwc3 controller: add quirk snps,dis_rxdet_inp3_quirk.Nov 10 2021, 12:27 AM

bz added a child revision: D33170: USB: dwc3: use device_{has,get}_property().Nov 29 2021, 9:29 PM

I'll rework this after a rebase now that all the other bits went in :-)

Update after all the latest changes went in already.

This should take most of the previous comments into consideration.
It also keeps everything in a single file.
One side effect of this and DEFINE_CLASS_1 is that it's no longer called xhci<n>
but snps_dwc3_acpi<n> and snps_dwc3_fdt<n> as otherwise we get two

static kobj_class_t xhci_baseclasses[] =``` in the same file.  These macros
should use a "unique" bit for this so that ``` #name,``` can actually be the same
but that is a problem for aother day unless we want to insist on keeping it xhci<n>.

Tested on: nanopi-m4v2, ten64, HoneyComb

Harbormaster completed remote builds in B46266: Diff 107774.Jul 5 2022, 1:14 AM

bz added inline comments.Jul 5 2022, 1:18 AM

sys/dev/usb/controller/dwc3.c
209	We do configure the PHYs right before the only call to reset so no need to do it here again.
344–345	This is needed now (again) as ACPI only copies 4 bytes for "host" and does not terminate the string. A strncmp(,,s) below could work until someone adds a "hostfoo" mode.
399	We could move the entire PHY bits into snps_dwc3_configure_phy() if we wanted now.