Page MenuHomeFreeBSD

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

Authored by bz on Nov 29 2021, 9:29 PM.

Details

Reviewers
manu
mw
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 44491
Build 41379: arc lint + arc unit

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

mindal_semihalf.com 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.