Page MenuHomeFreeBSD

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

Authored by bz on Nov 29 2021, 9:29 PM.
Tags
None
Referenced Files
F77920873: D33170.diff
Sun, Feb 25, 8:24 AM
Unknown Object (File)
Sat, Feb 10, 8:15 PM
Unknown Object (File)
Fri, Feb 2, 3:14 AM
Unknown Object (File)
Sun, Jan 28, 1:06 AM
Unknown Object (File)
Jan 24 2024, 2:39 AM
Unknown Object (File)
Dec 29 2023, 4:16 AM
Unknown Object (File)
Dec 29 2023, 4:16 AM
Unknown Object (File)
Dec 29 2023, 4:16 AM

Details

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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bz requested review of this revision.Nov 29 2021, 9: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).

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.

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.

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 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!

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

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.

Update to use the new KPI after D34031, D33457.

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)
...
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?

bz marked 2 inline comments as done.

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

This revision is now accepted and ready to land.Jun 27 2022, 11:27 PM
This revision was automatically updated to reflect the committed changes.
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
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?