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

[bz]

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.

[bz]

(surprised I cannot find a USB project in Phabricator)

[manu]

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.

[dan.kotowski_a9development.com]

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]

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

[bz]

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

[bz]

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.

[dan.kotowski_a9development.com]

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

[val_packett.cool]

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]

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

[dan.kotowski_a9development.com]

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]

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

[bz]

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

[bz]

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.

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.

400

We could move the entire PHY bits into snps_dwc3_configure_phy() if we wanted now.

[mw]

sys/dev/usb/controller/dwc3.c

345

Would:

char dr_mode[16] = {0};

declaration work instead of memset?

[bz]

Use static array initializer instead of memset.

[mw]

LGTM, thanks!

[manu]

Small nits on one ifdef but otherwise looks ok to me.

sys/dev/usb/controller/dwc3.c

180

This was dropped, is it intentional ?

[mw]

sys/dev/usb/controller/dwc3.c

180

snsp_dwc3_dump_regs() and snps_dwc3_dump_ctrlparams() were in 2 adjacent #ifdef DWC3_DEBUG sections. bz@ merged them.

[manu]

sys/dev/usb/controller/dwc3.c

180

Oops yes :)
Nevermind, nothing to see here.