Page MenuHomeFreeBSD
Differential D32256

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

Authored by bz on Oct 1 2021, 12:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 30, 5:23 AM
Unknown Object (File)
Thu, Nov 21, 8:46 AM
Unknown Object (File)
Wed, Nov 20, 2:51 AM
Unknown Object (File)
Tue, Nov 19, 6:08 AM
Unknown Object (File)
Fri, Nov 15, 6:35 AM
Unknown Object (File)
Mon, Nov 11, 11:57 PM
Unknown Object (File)
Mon, Nov 11, 11:15 PM
Unknown Object (File)
Sat, Nov 9, 3:40 PM
View All Files
Subscribers
andrew
dan.kotowski_a9development.com
emaste
• hselasky
imp
val_packett.cool
arm64

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
bz added reviewers: manu, mw.Oct 1 2021, 12:56 PM
bz added subscribers: arm64, • hselasky.

(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
manu added a comment.Oct 1 2021, 3:23 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.

dan.kotowski_a9development.com added a comment.Oct 1 2021, 4:06 PM

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
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
bz added a comment.Oct 1 2021, 5:47 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.

dan.kotowski_a9development.com added a comment.Oct 1 2021, 7:03 PM

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 added a subscriber: val_packett.cool.Oct 1 2021, 10:46 PM
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
dan.kotowski_a9development.com added a comment.Oct 5 2021, 2:06 PM

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
bz planned changes to this revision.Jul 3 2022, 10:23 PM

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

bz updated this revision to Diff 107774.Jul 5 2022, 1:13 AM
bz edited the test plan for this revision. (Show Details)

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.

bz marked an inline comment as done.Jul 5 2022, 1:18 AM
mw mentioned this in D33170: USB: dwc3: use device_{has,get}_property().Jul 5 2022, 5:54 PM
mw added inline comments.Jul 5 2022, 6:13 PM
sys/dev/usb/controller/dwc3.c
344–345

Would:

char dr_mode[16] = {0};

declaration work instead of memset?

bz updated this revision to Diff 107802.Jul 5 2022, 10:10 PM
bz marked an inline comment as done.

Use static array initializer instead of memset.

Harbormaster completed remote builds in B46274: Diff 107802.Jul 5 2022, 10:10 PM
mw accepted this revision.Jul 5 2022, 11:02 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 5 2022, 11:02 PM
manu accepted this revision.Jul 6 2022, 6:38 AM

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 added inline comments.Jul 6 2022, 8:22 AM
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 added inline comments.Jul 6 2022, 9:47 AM
sys/dev/usb/controller/dwc3.c
180

Oops yes :)
Nevermind, nothing to see here.

Closed by commit rGfbb5cb66f74c: dwc3: add ACPI attachment (authored by bz). · Explain WhyJul 6 2022, 11:45 AM
This revision was automatically updated to reflect the committed changes.
bz added a commit: rGfbb5cb66f74c: dwc3: add ACPI attachment.
bz added a commit: rG30f6e3752287: dwc3: add ACPI attachment.Jul 18 2022, 9:51 AM

Revision Contents
Changeset List

PathSize
sys/
conf/
2 lines
dev/
usb/
controller/
199 lines

Diff 107828

View Options

sys/conf/files.arm64

Loading...
View Options

sys/dev/usb/controller/dwc3.c

Loading...