drivers: uhc_dwc2: Initial support [WIP]

[roma-jam]

Description

Adding USB host driver initial support for DWC2 controller.

Features and Limitations

• One peripheral port controller
• One pipe/channel
• Control transfer only
• Buffer DMA only

Commits by their order

• Added DeviceTree description for USB OTG
• Added ESP32 binding for Synopsys dwc2 controller
• Added host_prj.conf overlay for sample/subsys/usb/shell
• Added host register sets to usb_dwc2_hw.h
• Added uhc_dwc2.* files with vendor quirks and Kconfig for UHC_DWC2

Testing

After running the shell sample and initial driver enabling with commands usbh init and usbh enable:

[github-actions]

Hello @roma-jam, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[josuah]

For anyone trying this, here is what I did to get it built:

cd ~/zephyrproject/modules/hal/espressif

Then modify the build script to include one extra directory (espressif-specific):

diff --git a/zephyr/esp32s3/CMakeLists.txt b/zephyr/esp32s3/CMakeLists.txt
index aedf412601..6a5546da9e 100644
--- a/zephyr/esp32s3/CMakeLists.txt
+++ b/zephyr/esp32s3/CMakeLists.txt
@@ -221,7 +221,7 @@ if(CONFIG_SOC_SERIES_ESP32S3)
     )
   endif()

-  if (CONFIG_UDC_DWC2)
+  if (CONFIG_UDC_DWC2 OR CONFIG_UHC_DWC2)
     zephyr_include_directories(
     ../../components/usb/include
     )

Then to build it, disable the device stack (or build error occurs insofar):

west build -b esp32s3_devkitm/esp32s3/procpu samples/subsys/usb/shell/ \
    -DEXTRA_CONF_FILE=host_prj.conf -DCONFIG_USB_DEVICE_STACK_NEXT=n

[josuah]

@roma-jam thank you very much for this effort!

Maybe you saw this incomplete branch from Espressif? raffarost@54dcf56

I could run the firmware but not yet test it I am still waiting an ESP32-S3 devkit (with 2 discrete USB interface to make sure there is no bug related to switching between the "jtag/serial and "usb-otg" interface on the same port) in the mail.

In the other hand, I will also try to get it working on a Nordic devkit that also has DWC2, and it seems like some STM32 part also has it. Glad to see you preserved all the vendor-quirks system to keep all vendor-specific on separate files.

I will post here as I am able to get more progress, and start doing early review as well.

[roma-jam]

Hi @josuah,

yes, I saw that and many thanks to @raffarost, I took couple of things from his work.

Besides that, I decided to proceed with simple implementation for all controllers with dwc2 (I hope, at least I tried to predict as much as I could) with Buffer DMA config (as currently in TinyUSB host) and with similar logic of event handling (ports, pipes and channels) , as in esp-idf USB Host stack.

Buffer DMA (not Scatter-Gahter) because it supports split-transactions, and I believe we want to have fully-working external hubs with HS root ports.

Thanks for you support and just in case - this is still a draft and I pushed it because the first step positive scenario (like device enumeration is done) is copmleted. But there are still a lot of parts are missing.

[josuah]

@roma-jam I received my ESP32-S3 DevkitC-1 to test this. So far I used an OTG adapter on the "USB" interface and connected it to various devices but after usbh init and usbh enable, the usbh device list command did not return any.

No problem, I can investigate this, but just in case there was something obvious I missed, what is your physical setup?
This uses MicroUSB so no USB-PD involved, only the OTG pin, MicroUSB cables only on that picture.

Build command used with your latest commits from 40 minutes ago:

$ west build -b esp32s3_devkitm/esp32s3/procpu samples/subsys/usb/shell/ \
    -DEXTRA_CONF_FILE=host_prj.conf -DCONFIG_USB_DEVICE_STACK_NEXT=n
$ west flash
$ picocom -b 115200 /dev/ttyUSB0
uart:~$ usbh init
host: USB host initialized
[00:00:07.296,000] <wrn> uhc_dwc2: FS PHY config not implemented yet
uart:~$ usbh enable
host: USB host enabled
uart:~$ rem Now connecting various USB devices
uart:~$ usbh device list
uart:~$ usbh device list
uart:~$ usbh device list
uart:~$

I suppose the warning message means it needs an USB HighSpeed device (USB MSC dongle) instead of FullSpeed? Or maybe a LowSpeed only (keyboard/mouse)?

I will try to support for DWC2 OTG on an nRF54LM20 DK, so different hardware anyway, but I was interested in testing the same way as you, to check that things still work on the ESP32 after introducing more code for the nRF54LM20.

I have the access to DWC2 docs and can join the effort with writing the driver as soon as I am done with the low levels (enable the core, PHY setup...).

Thanks!

[roma-jam]

Hi @josuah,

Sorry for this. Yes, there is no 5V on the USB OTG port connector, so this dev kit is for the usb device, not the host.

But! You can use the usb wire, attached to the pins or bypass the D7 to provide 5V to the port.

Please, refer to the schematic: https://dl.espressif.com/dl/schematics/SCH_ESP32-S3-DevKitC-1_V1.1_20221130.pdf

UPD:

I suppose the warning message means it needs an USB HighSpeed device (USB MSC dongle) instead of FullSpeed? Or maybe a LowSpeed only (keyboard/mouse)?

No, the warning message just shows that the supported PHY is FS and there is no config for this so far. On esp this config is by default after the power on, so it will work without it. But I will add it soon.

Thanks.

[josuah]

That makes sense thank you for the hint I thought I missed something obvious.
No problem ofc, that is a good occasion to document the hardware setup for anyone else trying.
I will try soon and give news.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[josuah]

Hello again @roma-jam,

I could adapt this PR to be working on nRF54LM20 by only modifying the vendor quirks.=
This should be making it easy to integrate in this current PR.

There seem to be a bit of coding style work needed, and maybe a few TODOs to cover.
The ideal would be to test on both nRF54LM20 and ESP32, but the nRF54LM20-DK is not out yet.

Should I keep going with the refactoring and coding style change to merge this then?
I could also test on an ESP32-S3 DevkitC with some wire jumper between the VBUS of the two connectors for powering the USB device attached.

I will start in pr_dwc2_uhc_nrf54l_phy in the meantime...

Thanks!

[roma-jam]

Hi @josuah,

thanks for the feedback!

I could adapt this PR to be working on nRF54LM20 by only modifying the vendor quirks.

this seems like a good news, isn't it?

I wasn't able to get back to this task in the last few months, but my plan is still the same:

• refactor quirks to make all of them reasonable (at start, I added maybe a bit more, then necessary)
• change the channel to fully dynamic (right now it is just static allocation for EP0)

But these tasks is nothing to do with the coding style. I saw that I am a bit far from the requirements, but I can catch up later.
I can adapt the coding style myself. I tried to do that but apparently, it was not enough. May I ask you to leave couple of comments as a review for example?
Then, I can follow the style in the future.

maybe a few TODOs to cover.

Could you please specify, what do you want to cover in the priority order? So I can solve them one by one in the meantime.

[josuah]

this seems like a good news, isn't it?

I think it is :)

I wasn't able to get back to this task in the last few months, but my plan is still the same:

This turned out convenient to have some PR stability during this time.

refactor quirks to make all of them reasonable (at start, I added maybe a bit more, then necessary)

Maybe these could eventually be added over time, as new devices need to insert something at a new location

change the channel to fully dynamic (right now it is just static allocation for EP0)

I was thinking static arrays could be used to avoid k_malloc(). thoughts?
I thought about merging a basic class that only supports endpoint 0, and just enumerate for now.
There would not be anything to do with other endpoints yet anyway as no class API yet.
Then, as the class API comes closer to completion, more work can happen on DWC2.

May I ask you to leave couple of comments as a review for example?

What I needed to do:

• CI, get twister happy, including the check_compliance.py checks, which we can see a the bottom of this PR.
• Extra guidelines as seen in https://docs.zephyrproject.org/latest/contribute/index.html
• Follow the code organization of the USB subsystem/drivers to help maintainers (I'm just contributor)
• Follow the Synopsys programmer guide step by step exactly, as that is the only way to communicate with their support is to show that the code follows it to the letter (as I was narrated).

What I wished to do is to:

• convert from sys_read32((uintptr_t)&dwc2->REGISTER); to sys_read32(base + USB_DWC2_REGISTER); to make it easier to extract the registers semi-automatically (mutool convert -o dwc2.txt dwc2_programmer_guide.pdf then $EDITOR dwc2.txt).

I thought it'd be safer to do "big changes" with both hardware at hand to test it after every step (or what if I answer you "it does not work anymore", several days to find 1 typo >_<).

Could you please specify, what do you want to cover in the priority order? So I can solve them one by one in the meantime.

I think enabling, testing HighSpeed support, disabling HNP (should we rely on Type-C only for role swapping?)...
Not much for a first version at least.

What to do next? :)

[josuah]

If rebasing on main, you might face this issue:

• shell: semaphore causes crashes, replacing by mutex works #99081

Which has this workaround:

• 9b72076

[josuah]

If asking to make one change, waiting the answer, then test things locally on the devkit, it will take ages.

Let's try to make modifications, and announce them, so that it's possible to give feedback, and potentially I revert some changes if I am mistaken or better options exist etc.

Once things are in shape, it becomes easier to pursue with actual feature development on top on both our side, this becomes normal PRs...

[josuah]

First few modifications I am doing on my fork...

[josuah]

I am removing every typedef as they are not allowed/recommended by Linux Kernel coding style:
https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs

[josuah]

I am removing the section decorations for replacing them by a plain comment.
A bit less easy to notice but other Zephyr sources very rarely use decorations, so might as well do the same...

[josuah]

I am converting source using clang-format to fix all of these. Zephyr provides a .clang-format file which is guaranteed to give code matching the coding style in almost every case,

USB area uses tabs for indenting \ though, also compatible AFAIU, so might as well do that once refactored...

[josuah]

Doxygen comments are avoided from drivers as it would effectively generate docs for it (or at least in theory) and users are not expected to use the functions directly (declard static).

The @params are further self-explanatory (i.e. can lookup the struct definition to know what it is).

I also propose to not add a Thread context and only mark Interrupt context implying everything else is in non-IRQ context.

The challenge with saying "thread context" is that some day a function is refactored and the comment is out of sync easily.

[josuah]

I propose to not make use of "lookup tables" for debug enums, which uses flash (though only if debug logs are enabled), and quickly become out of sync (i.e. here it lacks "Recovery"), which risks invalid accesses and confusing user more than needed.

They are also scarcely used in other drivers.

This reduces convenience though...

[josuah]

No typedef means that pipe_hdl_t becomes struct dwc2_pipe * directly in the code and not a separate type anymore (by just expanding the typedefs...)

[josuah]

Since there is no typedef, tokens like pipe_obj no more need to have _s suffix for structs as there is only one type name: struct typename.

I am also adding a uhc_dwc2_ prefix to locally defined types, because this namespace is going to be "assaulted" by arbitrary HALs, some in the future might define their own names that clash with these in this driver.

[josuah]

The val of the unions does not seem used at all except for setting the whole thing to 0, which can easily be done with memset(), so am tempted to withdraw it for now.

Other drivers seems to not use this method for quickly resetting bitfields to 0.

[josuah]

While having separate structs helps with reducing the scope of functions to the minimum struct they act upon (good), it also mean a higher number of types, and refactoring (i.e. accessing an extra priv->field not on this "sub-struct") might require passing the struct device anyway.

Other drivers typically have all struct device * as first argument regardless of their use, with a boilerplate to access priv (USB-specific), config (all drivers) and data (all drivers, but not used directly in USB, priv is used instead).

[josuah]

To facilitate comparing the register access sequence with the programmer guide (i.e. to write a bug report to Synopsys), I am tempted to flatten the register accesses, in particular for functions used only once, and even annotate the steps of the programmer guide in the driver.

There are a lot of "HAL"s used in Zephyr drivers, but does not seem required to get this driver to work, and in a way, the HAL here is UHC API already?

[tmon-nordic]

Beware to not copy&paste from the Programmer Guide as it is not publicly available.

[roma-jam]

Hi @josuah,

thanks a lot for looking through my code and for such a usefull feedback!

I can‘t guarantee that I will fix everything in the next couple weeks, as I am in the middle of the other task.

So, I will definetely return back to this PR and I hope to make it sooner.

[josuah]

To avoid ambiguity: I am only announcing the asethetical code changes I'm doing now, and report what I've changed (to keep this a discussion and not impose anything).

Then I will propose you a replacement PR that we can both use as a root.

Hopefully this removes you the paperwork, while give an example to use to implement the actual features! :)

[josuah]

I'm going to try to remove all k_malloc() to avoid sharing memory with the application and drivers, in both area used and resources.

This helps keeping everything more predictable under low-memory conditions (the application does not crash the drivers and other way around).

Alternative if really needed: k_heap_alloc() with a locally defined heap with size from Kconfig.

[josuah]

Comments for every DEVICE_DT_INST_DEFINE() field are rare... a shame as it helps with readability, but I will try to reduce review time and pick the same style as other drivers.

I will also try to make this multi-instance, even if it does not need to be, as this is now meant as a vendor-neutral implementation, and future SoCs could want multi-instance maybe? (i.e. some i.MXRT have USB1 and USB2).

Not sure if really needed though.

[josuah]

I am tempted to remove all bitfields that are not just 1~2-bit flags.

This might help avoid accidental bugs (I know I always miscalculate something and it integer overflows...). And uint8_t var has same footprint as uint32_t var: 8 (or maybe even fewer in case the uint32_t is not completely filled).

This also allow to use enum.

[josuah]

There is nothing called "pipe" in DWC2 handbook or databook, so we may merge channel and pipe in a single struct, and this way there is no need to cross-reference them via "context" structs.

Furthermore, instead of passing struct uhc_dwc2_pipe (new name for pipe_t), it may be possible to pass const struct device and channel_num, and then use channel = priv->channels[channel_num];, which mean there is no more need to store the chan_idx, nor the the regs (which can be retreived given the channel number).

Next, the struct uhc_dwc2_pipe can be renamed into struct uhc_dwc2_channel to keep them matching with the DWC2 native hardware naming...

I will try this and hope this makes sense.