Page MenuHomeFreeBSD

USB: split dwc3 (Synopsys) up for FDT and ACPI attachment
Needs RevisionPublic

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

Details

Reviewers
manu
mw
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
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 41882
Build 38770: arc lint + arc unit

Event Timeline

bz requested review of this revision.Oct 1 2021, 12:55 PM
bz added subscribers: arm64, hselasky.

(surprised I cannot find a #usb project in Phabricator)

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>

mw requested changes to this revision.Oct 1 2021, 5:05 PM

+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
206

This is a separate functional change. Please move to a separate smaller commit.

sys/dev/usb/controller/dwc3_fdt.c
136

Please move above block to a common subroutine - it can help avoiding some duplication.

154

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

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.

sys/dev/usb/controller/dwc3_fdt.c
154

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

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

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…

sys/dev/usb/controller/dwc3_fdt.c
154

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.