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

[bz]

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.

[bz]

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]

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]

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

[bz]

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.

[kd]

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]

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.

[bz]

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?

[mw]

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.

[bz]

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!

[mw]

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]

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.

[bz]

Update to use the new KPI after D34031, D33457.

[bz]

Rebase to main.

[bz]

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

[mw]

sys/dev/usb/controller/dwc3.c

285–293

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]

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

[bz]

sys/dev/usb/controller/dwc3.c

285–293

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]

sys/dev/usb/controller/dwc3.c

285–293

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?