feature(usb_host): Support multiple DWC OTG peripheral ports in Host mode #269

roma-jam · 2025-09-26T11:56:15Z

Description

Follow-up for: 35401#note_1958915

Isolated root port handling in HUB Driver
Changed root_port_hdl to static buffer of root_port_hdls[HUB_ROOT_PORTS] where HUB_ROOT_PORTS is configured statically, via #define (supports <= 4 by design, but can be increased)
Added peripheral map handling during USB Host lib install (install PHY to every peripheral port in peripheral map)

Limitation

All HCD ports are handled in a single thread (via usb_host_lib_handle_events())
Added TODO: Refactor HUB_PORTS_NUM: make them dynamically configurable by hub_config_t::port_map in usb_host.c

Testing

Common usb host tests.

Checklist

Before submitting a Pull Request, please ensure the following:

🚨 This PR does not introduce breaking changes.
All CI checks (GH Actions) pass.
Documentation is updated as needed.
Tests are updated or added as necessary.
Code is well-commented, especially in complex areas.
Git history is clean — commits are squashed to the minimum necessary.

…with node uid

…rent_dev_hdl

…_hdl

…parent_dev_hdl)

…uld come any moment

cursor · 2025-09-29T11:56:28Z

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 20.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

host/usb/src/hub.c

- HUB: Added periph_idx argument to hub_root_start() and hub_root_stop()

peter-marcisovsky

I presume, the suspend/resume feature will have to be supported for multiple peripherals.

peter-marcisovsky · 2025-09-30T18:48:28Z

host/usb/private_include/hcd.h

+ *
+ * @return uint8_t Port number
+ */
+uint8_t hcd_port_get_number(hcd_port_handle_t port_hdl);


Maybe you can add get_port_number to the function name? Right now, if you don't' look at the prefix hcd_port, the function name is just get_numer. But up to you.

Suggested change

uint8_t hcd_port_get_number(hcd_port_handle_t port_hdl);

uint8_t hcd_port_get_port_number(hcd_port_handle_t port_hdl);

peter-marcisovsky · 2025-09-30T18:51:32Z

host/usb/private_include/hub.h

 *    - ESP_ERR_INVALID_STATE: Hub driver is not installed, or root port is in other state than not powered
 */
-esp_err_t hub_root_start(void);
+esp_err_t hub_root_start(uint8_t port_idx);


During the usb_host_install() we define peripheral map by esp_bit_defs.h, BIT0 and BIT1 respectively. Would it make sense to unify the peripherals selection?

peter-marcisovsky · 2025-09-30T18:56:31Z

host/usb/private_include/usbh.h

 esp_err_t usbh_dev_get_addr(usb_device_handle_t dev_hdl, uint8_t *dev_addr);

+/**
+ * @brief Get the root port handle of a device


In this context, the device is the external hub? Getting a root port handle of device is bit misleading term.

peter-marcisovsky · 2025-09-30T19:04:45Z

host/usb/src/hcd_dwc.c

+uint8_t hcd_port_get_number(hcd_port_handle_t port_hdl)
+{
+    port_t *port = (port_t *)port_hdl;
+    return port->port_num;


In (I think) all the functions in hcd_dwc.c, whenever we are working with port handle, we are using critical section. hcd_port_get_state() is a perfect example which matches this scenario.

peter-marcisovsky · 2025-09-30T19:06:39Z

host/usb/src/usb_host.c

        SemaphoreHandle_t mux_lock;
-        usb_phy_handle_t phy_handle;    // Will be NULL if host library is installed with skip_phy_setup
+        unsigned peripheral_map;        // Peripheral map
+        usb_phy_handle_t phy_hdls[4 /* TODO: 1 */];    // Will be NULL if host library is installed with skip_phy_setup


Why 4, if we have only 2 USB-OTG peripherals?

peter-marcisovsky · 2025-09-30T19:12:43Z

host/usb/src/usb_host.c

+        // Start the root hub according to the peripheral map
+        for (unsigned idx = 0; idx < 4 /* TODO: 1 */; idx++) {
+            if (p_host_lib_obj->constant.peripheral_map & (BIT0 << idx)) {
+                ESP_ERROR_CHECK(hub_root_start(idx));


You can also break form the cycle, once you find the correct peripheral.

Suggested change

ESP_ERROR_CHECK(hub_root_start(idx));

ESP_ERROR_CHECK(hub_root_start(idx));

break;

peter-marcisovsky · 2025-10-01T08:51:00Z

host/usb/src/usb_host.c

+        }
    }

    return ret;


In case the usb_host_install() has not been called prior to the usb_host_lib_set_root_port_power() call, this function returns just ret, which would be undefined.
Since the for cycle never finds the peripheral map and the if (p_host_lib_obj->constant.peripheral_map & (BIT0 << idx)) is never executed.

You can consider declaring esp_err_t ret as esp_err_t ret = ESP_ERR_NOT_FOUND for example.

Maybe there will be similar unsafe condition, whenever using the similar for cycle for some further action based on peripheral map index.

peter-marcisovsky · 2025-10-01T09:05:18Z

host/usb/src/hub.c

+static void root_port_handle_events(root_port_t *root_port)
 {
+    hcd_port_handle_t root_port_hdl = root_port->constant.hdl;
+    uint8_t port_idx = hcd_port_get_number(root_port_hdl);


You could use const to indicate the port_index is not changing in the function

Suggested change

uint8_t port_idx = hcd_port_get_number(root_port_hdl);

const uint8_t port_idx = hcd_port_get_number(root_port_hdl);

igi540 · 2025-10-01T10:26:52Z

host/usb/src/usb_host.c

        SemaphoreHandle_t mux_lock;
-        usb_phy_handle_t phy_handle;    // Will be NULL if host library is installed with skip_phy_setup
+        unsigned peripheral_map;        // Peripheral map
+        usb_phy_handle_t phy_hdls[4 /* TODO: 1 */];    // Will be NULL if host library is installed with skip_phy_setup


Instead of hardcoding the USB instance count, could we use SOC_USB_OTG_PERIPH_NUM from soc/soc_caps.h? It’s defined per-SoC, so this stays portable across targets.

igi540 · 2025-10-01T10:33:12Z

host/usb/src/hub.c

-implement the bare minimum to control the root HCD port.
-*/
+/* Max support value based on the implementation: 4 */
+#define HUB_ROOT_PORTS                    2


you can use soc_caps define instead

Suggested change

#define HUB_ROOT_PORTS 2

#define HUB_ROOT_PORTS SOC_USB_OTG_PERIPH_NUM

tore-espressif

@roma-jam Thank you for your work!
For this feature we were expecting some breaking changes. The is an incomplete list of what must be changed:

usb_host_lib_set_root_port_power() should accept root port index. (or, alternatively, it can always power on/off both ports, but this can be too limiting)
Following members of usb_host_config_t are specific to USB-DWC configuration, and therefore it should be possible to configure them for each peripheral separately:

root_power_unpowered
intr_flags
fifo_settings_custom

In case we want to avoid the breaking changes, we can just extend the API for dual host. Eg. introduce new function usb_host_install_dual() and usb_host_lib_set_root_power_dual().
This would make the code less cumbersome while providing the feature to users in a clear way.

We can discuss offline when you're ready

roma-jam added 3 commits September 26, 2025 11:09

refactor(enum): Removed parent_dev_hdl and parent_port_num, replaced …

508a1b4

…with node uid

refactor(hub): Removed parent_dev_hdl from dev tree node structure

663be6a

refactor(ext_hub): Removed unused calls as hub driver doesn't have pa…

d23d9fb

…rent_dev_hdl

roma-jam self-assigned this Sep 26, 2025

roma-jam changed the title ~~feature(usb_host): Support multiple DWC OTG peripheral ports [WIP]~~ feature(usb_host): Support multiple DWC OTG peripheral ports in Hub Driver [WIP] Sep 26, 2025

roma-jam force-pushed the feature/usb_host_multiple_root_ports branch from 47ba0b1 to 6e126f3 Compare September 26, 2025 12:55

roma-jam added Component: usb_host Issue affects usb_host component Status: In Progress Issue is being worked on and removed Status: In Progress Issue is being worked on labels Sep 26, 2025

roma-jam added 5 commits September 29, 2025 11:04

refactor(ext_hub): Added device speed getter to stop using parent_dev…

24e14ff

…_hdl

refactor(ext_port): Removed parent_dev_hdl and port_num from event

173aa63

change(ext_port_test): Applied new ext_port_event callback

4ae9d24

refactor(ext_port): Removed parent_dev_hdl from the port object

ad2f788

change(ext_port_test): Applied new port alloc configuration (without …

3e13334

…parent_dev_hdl)

roma-jam force-pushed the refactor/usb_host_cleanup_hub_parent_child_dependencies branch from 33e40ea to a6bb309 Compare September 29, 2025 09:04

roma-jam added 3 commits September 29, 2025 11:08

refactor(usbh): Moved parent information from usbh to hub driver

c4d4297

refactor(hub): Moved list to dymanic member as the get parent info co…

838c260

…uld come any moment

refactor(usbh): Cleaned up parent_dev_hdl from the usbh object

e928ef9

roma-jam force-pushed the refactor/usb_host_cleanup_hub_parent_child_dependencies branch from a6bb309 to e928ef9 Compare September 29, 2025 09:08

roma-jam added 3 commits September 29, 2025 11:12

refactor(ext_hub): Added hcd port handle getter for external hub

1e2be8f

refactor(usbh): Added hcd port handle getter for external hub

4cbacdb

refactor(hcd): Added getter for the port number

5eecf45

roma-jam force-pushed the feature/usb_host_multiple_root_ports branch from 0da5710 to fbc08d7 Compare September 29, 2025 11:50

roma-jam changed the title ~~feature(usb_host): Support multiple DWC OTG peripheral ports in Hub Driver [WIP]~~ feature(usb_host): Support multiple DWC OTG peripheral ports in Hub Driver Sep 29, 2025

roma-jam marked this pull request as ready for review September 29, 2025 11:56

roma-jam changed the title ~~feature(usb_host): Support multiple DWC OTG peripheral ports in Hub Driver~~ feature(usb_host): Support multiple DWC OTG peripheral ports in Host mode Sep 29, 2025

roma-jam marked this pull request as draft September 29, 2025 13:58

roma-jam force-pushed the feature/usb_host_multiple_root_ports branch from 415021a to dcf010b Compare September 29, 2025 14:43

github-advanced-security bot found potential problems Sep 29, 2025

View reviewed changes

host/usb/src/hub.c Fixed Show fixed Hide fixed

host/usb/src/hub.c Fixed Show fixed Hide fixed

roma-jam added 3 commits September 29, 2025 16:50

refactor(hub): Isolated root_port object to have multiple hcd handles

82402a0

feature(usb_host): Added handling of peripheral map during install

60465ad

- HUB: Added periph_idx argument to hub_root_start() and hub_root_stop()

refactor(usb_host_mock): Adapted hub_root_start and hub_root_stop api

85b3cc0

roma-jam force-pushed the feature/usb_host_multiple_root_ports branch from dcf010b to 85b3cc0 Compare September 29, 2025 14:50

roma-jam marked this pull request as ready for review September 29, 2025 15:14

roma-jam added this to the usb_host v1.0.1 milestone Sep 29, 2025

roma-jam requested review from igi540, peter-marcisovsky and tore-espressif September 30, 2025 13:27

peter-marcisovsky reviewed Oct 1, 2025

View reviewed changes

igi540 reviewed Oct 1, 2025

View reviewed changes

tore-espressif reviewed Oct 6, 2025

View reviewed changes

tore-espressif modified the milestones: usb_host v1.0.1, usb_host 1.1.0 Oct 9, 2025

roma-jam force-pushed the refactor/usb_host_cleanup_hub_parent_child_dependencies branch 5 times, most recently from 1e9a582 to d9045f6 Compare October 10, 2025 13:15

	uint8_t hcd_port_get_number(hcd_port_handle_t port_hdl);
	uint8_t hcd_port_get_port_number(hcd_port_handle_t port_hdl);

	ESP_ERROR_CHECK(hub_root_start(idx));
	ESP_ERROR_CHECK(hub_root_start(idx));
	break;

	uint8_t port_idx = hcd_port_get_number(root_port_hdl);
	const uint8_t port_idx = hcd_port_get_number(root_port_hdl);

	#define HUB_ROOT_PORTS 2
	#define HUB_ROOT_PORTS SOC_USB_OTG_PERIPH_NUM

feature(usb_host): Support multiple DWC OTG peripheral ports in Host mode #269

Are you sure you want to change the base?

feature(usb_host): Support multiple DWC OTG peripheral ports in Host mode #269

Uh oh!

Conversation

roma-jam commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Limitation

Related

Testing

Checklist

Uh oh!

cursor bot commented Sep 29, 2025

Uh oh!

Uh oh!

Uh oh!

peter-marcisovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tore-espressif left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

roma-jam commented Sep 26, 2025 •

edited

Loading