Details

Reviewers

manu
mw

Commits

rGbf7ce294145c: USB: dwc3: use device_{has,get}_property()
rGb11f52f4db44: USB: dwc3: use device_{has,get}_property()

Summary

Switch the driver to use device based functions which will work not
only with FDT but also ACPI in the future.

While here make dr_mode a local variable as it is only used during
probe and not needed later in the softc.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 46142
Build 43031: arc lint + arc unit

Event Timeline

bz created this revision.Nov 29 2021, 9:29 PM

Herald added a subscriber: imp. · View Herald TranscriptNov 29 2021, 9:29 PM

bz requested review of this revision.Nov 29 2021, 9:29 PM

Harbormaster completed remote builds in B43031: Diff 99180.Nov 29 2021, 9:29 PM

bz added a parent revision: D32256: USB: split dwc3 (Synopsys) up for FDT and ACPI attachment.Nov 29 2021, 9:29 PM

bz added inline comments.Nov 29 2021, 11:29 PM

sys/dev/usb/controller/dwc3.c
284–285	Can someone please test this with FDT? ofwdump is fine on the running system but the device_get_property here returns garbage and no error on a nanopi-m4v2 that I resurrected using an overlay to set dr_mode to host. I might debug it some more tomorrow if needed (but I don't trust that nanopi much as it has other memory issues).

bz added a subscriber: arm64.Nov 29 2021, 11:29 PM

kd added a subscriber: kd.Nov 30 2021, 12:15 PM

kd added inline comments.

sys/dev/usb/controller/dwc3.c

284–285

With this patch applied XHCI fails to attach with:

xhci0: Only host mode is supported
simplebus0: <usb@3100000> mem 0x3100000-0x310ffff irq 36 compat fsl,ls1028a-dwc3 (no driver attached)
xhci0: Only host mode is supported
simplebus0: <usb@3110000> mem 0x3110000-0x311ffff irq 37 compat fsl,ls1028a-dwc3 (no driver attached)

I tried it on LS1028A board.

bz added inline comments.Nov 30 2021, 2:05 PM

sys/dev/usb/controller/dwc3.c
284–285	Thanks. That was re-assuring that my nanopi is not at fault. I locally adjusted the code and I get: xhci0: Found dr_mode 'tsoh' but only 'host' supported. s=5 xhci0: Found dr_mode 'tsoh' but only 'host' supported. s=5 xhci0: Found dr_mode 'tsoh' but only 'host' supported. s=5 xhci0: Found dr_mode 'tsoh' but only 'host' supported. s=5 Seems something is not doing the right thing there in simplebus/FDT land for a string with device_get_property()?

Be more verbose in case of device_get_property() returning no
error but our dr_mode is not "host" so that people will see
what they are set to.

Harbormaster completed remote builds in B43052: Diff 99236.Nov 30 2021, 2:09 PM

Ok, I think I know what the problem here is.
Simplebus implementation of device_get_property uses OF_getencprop, which does a be32toh conversion on property data.
Previously dr_mode was read using OF_getprop, which doesn't do anything like that.

andrew added a subscriber: andrew.Nov 30 2021, 3:39 PM

andrew added inline comments.

sys/dev/usb/controller/dwc3.c
284–285	`device_get_property` is broken when reading a string on little endian OFW/FDT systems. It calls `OF_getencprop` which swaps endian on from big to host endian on each 4 byte block of data. This is needed for integer types that are stored in 4 byte blocks in big endian, but not for strings. You are reading 16 bytes of data so would not have triggered the `(length % 4) == 0` assertion.

Is someone else (from Semihalf) going to fix this?
I assume this is a missing bit in 3f9a00e3b577dcca57e331842e0baf2dbdf9325f which was only exercised with integers on FDT while the ACPI version does deal with the string specifically?

In D33170#750128, @bz wrote:

Is someone else (from Semihalf) going to fix this?
I assume this is a missing bit in 3f9a00e3b577dcca57e331842e0baf2dbdf9325f which was only exercised with integers on FDT while the ACPI version does deal with the string specifically?

We're taking a look and will try to propose some solution.

In D33170#750136, @mw wrote:

In D33170#750128, @bz wrote:

Is someone else (from Semihalf) going to fix this?
I assume this is a missing bit in 3f9a00e3b577dcca57e331842e0baf2dbdf9325f which was only exercised with integers on FDT while the ACPI version does deal with the string specifically?

We're taking a look and will try to propose some solution.

Thanks! My OFW/FDT knowledge is rather limited when it comes to its internals. So very much appreciated!

dsl added a subscriber: dsl.Nov 30 2021, 5:12 PM

mw added inline comments.Nov 30 2021, 5:23 PM

sys/dev/usb/controller/dwc3.c
284–285	`device_get_property` is broken when reading a string on little endian OFW/FDT systems. It calls `OF_getencprop` which swaps endian on from big to host endian on each 4 byte block of data. This is needed for integer types that are stored in 4 byte blocks in big endian, but not for strings. You are reading 16 bytes of data so would not have triggered the `(length % 4) == 0` assertion. After internal discussion and some research, it seems to be impossible to handle all types in runtime for OF_ (contrary to ACPI_). Therefore the most feasible and simple solution is to add another method (eg. `device_get_string_property`) for handling string properties and let user pick the appropriate one depending on a processed binding. Linux does exactly that and has various available types of routines, see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/property.h?h=v5.16-rc3#n37

bz added inline comments.Nov 30 2021, 9:55 PM

sys/dev/usb/controller/dwc3.c
284–285	I'd suggest opening a review and make this a parent review so we can continue here once that is sorted? PS: I am not sure I'd want an endless amount of BUS functions for all kinds of types. Maybe we can have wrappers in subr_bus and pass a type along to the ... I'll leave that for there.

kd added a parent revision: D33457: Expand device_get_property API.Dec 15 2021, 10:42 AM

dan.kotowski_a9development.com added a subscriber: dan.kotowski_a9development.com.Dec 24 2021, 3:56 PM

kd mentioned this in D33457: Expand device_get_property API.Feb 10 2022, 6:45 PM

Update to use the new KPI after D34031, D33457.

Harbormaster completed remote builds in B44491: Diff 102905.Feb 17 2022, 6:19 PM

Rebase to main.

Harbormaster completed remote builds in B45970: Diff 106904.Jun 14 2022, 3:57 PM

From all I can test this seems to work on a nanopi-m4v2.

...
ehci1: usbpf: Attached
ohci1: <Generic OHCI Controller> mem 0xfe3e0000-0xfe3fffff irq 16 on ofwbus0
usbus3 on ohci1
ohci1: usbpf: Attached
rk_dwc30: <Rockchip RK3399 DWC3> on ofwbus0
xhci0: Found dr_mode 'otg' but only 'host' supported. s=4
rk_dwc30: <usb@fe800000> mem 0xfe800000-0xfe8fffff irq 79 compat snps,dwc3 (no driver attached)
xhci0: Found dr_mode 'otg' but only 'host' supported. s=4
rk_dwc31: <Rockchip RK3399 DWC3> on ofwbus0
xhci0: <Synopsys Designware DWC3> mem 0xfe900000-0xfe9fffff irq 80 on rk_dwc31
xhci0: snps id: 5533300a
xhci0: 64 bytes context size, 32-bit DMA
usbus4: trying to attach
usbus4 on xhci0
xhci0: usbpf: Attached
ofwbus0: <dp@fec00000> mem 0xfec00000-0xfecfffff irq 17 disabled compat rockchip,rk3399-cdn-dp (no driver attached)
...

bz added a child revision: D35484: dwc3: fix and add more quirks.Jun 14 2022, 5:03 PM

mw added inline comments.Jun 27 2022, 4:07 PM

sys/dev/usb/controller/dwc3.c
285–294	Do we need to "zero" the variable? I think it should be rather safe to rely on ACPI/OF_ internals.
294	Do you think the string breaking can be avoided here?

No longer zero dr_mode;
re-wrap a printf so that the string stays on one line.

Harbormaster completed remote builds in B46142: Diff 107455.Jun 27 2022, 10:55 PM

mw accepted this revision.Jun 27 2022, 11:27 PM

This revision is now accepted and ready to land.Jun 27 2022, 11:27 PM

Closed by commit rGb11f52f4db44: USB: dwc3: use device_{has,get}_property() (authored by bz). · Explain WhyJun 28 2022, 1:04 PM

This revision was automatically updated to reflect the committed changes.

bz added a commit: rGb11f52f4db44: USB: dwc3: use device_{has,get}_property().

bz added inline comments.Jul 4 2022, 11:05 PM

sys/dev/usb/controller/dwc3.c
285–294	Getting back to this; yes! or we need to change to comparing the return length using strncmp(dr_mode, "host", s) != 0 snps_dwc3_acpi0: Found dr_mode 'hostÿÿÿÿpèû' but only 'host' supported. s=4 snps_dwc3_acpi0: Found dr_mode 'hostÿÿÿÿpèû' but only 'host' supported. s=4

mw added inline comments.Jul 5 2022, 5:54 PM

sys/dev/usb/controller/dwc3.c
285–294	Thanks, this is what I suspected - we obtain raw buffer from ACPI. I thought about it and in this case, I'd refrain from modifying the acpi_get_property callback. Anyway, I think this change belongs more to https://reviews.freebsd.org/D32256 instead of this ofw->device migration. I think strncmp option is a bit cleaner. Instead of memset, wouldn't declaring a zeroed array suffice? Can you please leave code as is and just declare dr_mode like this: `char dr_mode[16] = {0};` and check what happens with DT and ACPI?

bz added a commit: rGbf7ce294145c: USB: dwc3: use device_{has,get}_property().Jul 18 2022, 9:51 AM

USB: dwc3: use device_{has,get}_property()
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 107455

sys/dev/usb/controller/dwc3.c

USB: dwc3: use device_{has,get}_property()ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 107455

sys/dev/usb/controller/dwc3.c

USB: dwc3: use device_{has,get}_property()
ClosedPublic
Actions

Revision Contents
Changeset List