usb: host: class: support for usb host hub class#99735
usb: host: class: support for usb host hub class#99735AidenHu wants to merge 6 commits intozephyrproject-rtos:mainfrom
Conversation
|
I think this hub class implementation can be good helper to be used for testing the new host stack of 94590 for multiple devices attachment and detachment. This hub PR can grow up with your 94590. I will keep updating it for this PR based on your newest changes for host stack. Hope your any comments here. |
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Here is an incomplete review with a bit of feedback.
|
@josuah |
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
3bd611d to
35a2747
Compare
|
@josuah |
There was a problem hiding this comment.
If we look at the commit subsys: usb: host: add two APIs for connect/disconnect, it looks like the connect() and disconnect() functions are practically the same.
It is possible to declare the previous functions as public with a prototype in .h to avoid duplicating the code.
There was a problem hiding this comment.
If we look at the commit
subsys: usb: host: add two APIs for connect/disconnect, it looks like theconnect()anddisconnect()functions are practically the same.It is possible to declare the previous functions as public with a prototype in
.hto avoid duplicating the code.
Hi @josuah
I have created another commit #100072 about this requirement. Please help to review in this thread. So you can ignore subsys: usb: host: add two APIs for connect/disconnect in current thread.
| } | ||
|
|
||
| static void dev_connected_handler(struct usbh_context *const ctx, | ||
| const struct uhc_event *const event) |
There was a problem hiding this comment.
If this function needs to be used outside, by the HUB, then it is possible to remove the static, give it an API name such as usbh_device_connected(), then add it to usbh_device.h, as an example.
There was a problem hiding this comment.
@josuah
Thank you for your feedback.
I am thinking about this solution. If we change this function into a public API, there will be some structural issues. dev_connected_handler is tied to the event handler running in the USB bus thread, triggered by low-level events. Also, the speed information comes from the second parameter event. Making it an API would require dropping or modifying this parameter, which means extra changes.
Instead, if adding a new API in the commit “subsys: usb: host: add two APIs for connect/disconnect”, we can reuse the same logic as the caller to reduce code duplication and keep the structure clean.
This is my opinion, hope your any comment for this topic.
| static void dev_removed_handler(struct usbh_context *const ctx) | ||
| { |
There was a problem hiding this comment.
Likewise, if this function needs to be used outside, by the HUB code, then it is possible to remove the static, give it an API name such as usbh_device_remove(), then add it to usbh_device.h.
|
Understood, thank you! Here above is some short illustration of what was discussed in #99775 (comment) |
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
2 times, most recently
from
cb6a55e to
edb59fb
Compare
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
This is still an incomplete review but here is a few feedback in the meantime I can gather more time for a complete review.
Thank you for your patience!
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
edb59fb to
6836384
Compare
|
Have updated the basement from #99775 for this PR. |
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
6836384 to
6acdb74
Compare
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
2 times, most recently
from
d984e65 to
6b42426
Compare
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
6b42426 to
496905e
Compare
|
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
496905e to
7d47bb7
Compare
AidenHu
changed the title
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
3 times, most recently
from
c57debb to
c79d27a
Compare
zephyrbot
added
area: USB
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
4 times, most recently
from
b486ac4 to
33c45f7
Compare
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
33c45f7 to
a07e6a1
Compare
| /** Pointer to the hub to which this device is connected */ | ||
| struct usb_device *hub; | ||
| /** Device's hub wHubCharacteristics */ | ||
| uint16_t hub_characteristics; | ||
| /** Device's hub port */ | ||
| uint8_t hub_port; | ||
| /** Device's level (root device = 0) */ | ||
| uint8_t level; |
| @@ -1,4 +1,5 @@ | |||
| # SPDX-FileCopyrightText: Copyright Nordic Semiconductor ASA | |||
| # Copyright 2025 NXP | |||
There was a problem hiding this comment.
Do not add Copyright to trivial files like CMakeLists.txt or Kconfig.
There was a problem hiding this comment.
Thanks, update and will follow this.
| @@ -0,0 +1,78 @@ | |||
| # Copyright 2025 - 2026 NXP | |||
There was a problem hiding this comment.
SPDX-FileCopyrightText: Copyright The Zephyr Project Contributors
or
SPDX-FileCopyrightText: Copyright 2025 - 2026 NXP
There was a problem hiding this comment.
Updated, use SPDX-FileCopyrightText: Copyright 2025 - 2026 NXP
| config USBH_HUB_INIT_PRIORITY | ||
| int "Hub manager initialization priority" | ||
| default 85 | ||
| help | ||
| Initialization priority for the Hub manager. Should be | ||
| higher than the USB host stack but lower than device drivers | ||
|
|
There was a problem hiding this comment.
That does not make sense for a hub driver and does not seem to be used.
There was a problem hiding this comment.
Yes, delete it now.
| return -EINVAL; | ||
| } | ||
|
|
||
| k_mutex_lock(&hub_instance->ctrl_lock, K_FOREVER); |
There was a problem hiding this comment.
The request is blocking. What is this mutex intended to protect? Also, I think we do not need another wrapper for usbh_req_setup().
| enum usbh_port_state state; /* Port overall state */ | ||
| uint8_t reset_count; /* Reset retry count */ | ||
| uint8_t speed; /* Device speed */ | ||
| uint8_t port_num; /* Port number */ |
| #include <zephyr/sys/slist.h> | ||
| #include "usbh_hub.h" | ||
|
|
||
| enum usbh_hub_app_status { |
There was a problem hiding this comment.
What is hub application?
There was a problem hiding this comment.
rename it as usbh_hub_run_status
| HUB_PORT_RUN_CHECK_CHILD_HUB, | ||
| }; | ||
|
|
||
| enum usbh_hub_prime_status { |
There was a problem hiding this comment.
What does prime status mean? I see a lot of code copied from the mcux middleware and I really doubt that this approach makes any sense.
There was a problem hiding this comment.
The original aim is using this prime status to control transfer status, but now I think it is useless.
Delete enum usbh_hub_prime_status and the related.
| /* Port management - unified using port_list */ | ||
| struct usbh_hub_port_instance *port_list; /* Port instance list */ |
There was a problem hiding this comment.
Please get rid or all the redundant comments.
| struct usb_device *hub_udev; | ||
| struct usbh_context *uhs_ctx; | ||
|
|
||
| /* Hub instance data */ | ||
| struct usbh_hub_instance hub_instance; |
There was a problem hiding this comment.
Are hub_udev different to hub_instance.hub_udev? Why does hub_instance.hub_udev to be initialized separately?
There was a problem hiding this comment.
Updated. Remove the struct usb_device *hub_udev; from usbh_hub_mgr_data
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
a07e6a1 to
f3834a1
Compare
|
Thank you for the comment above. Expect something confusing about host hub feature (need your more clarification, thanks), I have checked the other all comments and updated code. For |
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
3 times, most recently
from
ea4d028 to
bdc7193
Compare
|
@jfischer-no |
This change extends the usb_device structure to keep track of its hub relationship, including the parent hub device, hub Think Time, port number, and topology level. Signed-off-by: Aiden Hu <weiwei.hu@nxp.com>
when the first device is attached, its level should be set as 1. Signed-off-by: Aiden Hu <weiwei.hu@nxp.com>
This change updates usb_hub.h by adding missing USB Hub class definitions from the specification, including hub class codes, descriptor types, status and change bits, and related data structures. The goal is to make the public header more complete and easier to use for USB hub implementations. Signed-off-by: Aiden Hu <weiwei.hu@nxp.com>
This change enhances USB_HostHelperGetPeripheralInformation() so the MCUX USB host driver can correctly report hub-related topology data, including the parent hub address, port number, nearest high speed hub, HS hub port, hub think time and level. The goal is to ensure proper device identification and routing, especially when full speed or low speed devices are connected behind multi level or high speed hubs. Signed-off-by: Aiden Hu <weiwei.hu@nxp.com>
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
bdc7193 to
6a76ea2
Compare
@tmon-nordic and @jfischer-no |
Remove usbh_req_set_hcfs_ppwr() and usbh_req_set_hcfs_prst() from usbh_ch9 as they are hub class-specific requests. These functions will be implemented in usbh_hub module instead. Signed-off-by: Aiden Hu <weiwei.hu@nxp.com>
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
6a76ea2 to
2b4f8dc
Compare
|
Firstly, thank you much for the detailed review about this PR. Now based on the current information, I have done a adjustment. If I am wrong, please add your comment. |
|
@tmon-nordic |
Add USB Host Hub Class implementation to enable connecting USB hubs and managing downstream devices. This supports hub enumeration and configuration, port management with automatic device detection and removal, recursive hub topology with configurable chain depth, and hub interrupt endpoint monitoring for port status changes. The implementation consists of core hub control request functions in usbh_hub.c and hub manager with port state machine in usbh_hub_mgr.c. Signed-off-by: Aiden Hu <weiwei.hu@nxp.com>
AidenHu
force-pushed
the
usb_host_hub_class_enablement
branch
from
2b4f8dc to
b11a44b
Compare
|
|
Please firstly ignore the CI run failure, it is caused by the removal of two functions from |
roma-jam
left a comment
roma-jam
left a comment
There was a problem hiding this comment.
Hi @AidenHu,
I decided to review and share my ideas regarding the Hub feature.
There are couple of notes and one general idea about the architectural approach.
Maybe you will find them helpful.
Please do not hesitate to poke me, when something is not clear or any additional information from my side is required.
| @@ -96,6 +96,14 @@ struct usb_device { | |||
| struct usb_host_ep ep_out[16]; | |||
| /** Pointers to device IN endpoints */ | |||
| struct usb_host_ep ep_in[16]; | |||
| /** Pointer to the hub to which this device is connected */ | |||
| struct usb_device *hub; | |||
There was a problem hiding this comment.
From the architectural point of view, any hub device is the same usb device as anything else. So that means, that after connection, we already have it in the sys_dlist_t udevs.
What gives us the option to keep the pointer to the "parent", which can be the pointer to the another udev in the list in case the device is a downstream device.
For the device, connected to the root port, it is possible to keep this pointer NULL.
From the uhc logic, there should be no difference, when a new device is connected (to directly to the root port or via external hub port) and it might be handled with the same dev_connected_handler().
This flag (parent == NULL or parent == *another udev) might be used to build the device tree.
Maybe, it makes sense to think about the new abstraction object: device node, which represents the combination: parent + port number? Thus, the root node will be [NULL, 0], and any other node will be [*udev, port_num].
PS. As a bonus, if the hardware has two usb controllers, we can distinguish them by port number, when parent is NULL.
| /** Pointer to the hub to which this device is connected */ | ||
| struct usb_device *hub; | ||
| /** Device's hub Think Time */ | ||
| uint16_t tt; |
There was a problem hiding this comment.
Maybe, it makes sense to introduce the whole hub-related part of the struct? Or to introduce them as "getters", because they are part of the Hub Descriptor that we need to request in the potential class driver?
Because, there are several other important hub characteristics, that we might be able to use in the driver.
Such as:
- number of ports (relevant, when we handle the port(s) connection)
- time from power on to power good (relevant, when hub can power on each port or all ports altogether)
- characteristics (tt includes here, as well as port logical switching and led support)
- current consumption
| /** Device's hub port */ | ||
| uint8_t hub_port; | ||
| /** Device's level (root device = 0) */ | ||
| uint8_t level; |
There was a problem hiding this comment.
Just out of curiosity, what is the purpose of having level in explicit form?
There was a problem hiding this comment.
This file seems like a candidate to be usb_ch11.h, not usb/class/usb_hub.h?
|
|
||
| LOG_DBG("USB HUB device probe at interface %u", target_iface); | ||
|
|
||
| desc_start = usbh_desc_get_iface(udev, target_iface); |
There was a problem hiding this comment.
Some hubs might have several interfaces with different TT. Especially on High speed configuration.
| HUB_RUN_CLEAR_DONE, | ||
| }; | ||
|
|
||
| enum usbh_hub_port_app_status { |
There was a problem hiding this comment.
Chapter 11 provides the diagram of downstream port in Figure 11-10. Downstream Facing Hub Port State Machine.
This is a part of it:
Maybe it is better to follow names and port states from the specification to provide easier maintenance in the future?
| hub_mgr.total_hubs++; | ||
| k_mutex_unlock(&hub_mgr.lock); | ||
|
|
||
| k_work_submit(&hub_mgr_data->hub_work.work); |
There was a problem hiding this comment.
There are 14 k_work_submit() throughout the code. Maybe, it might be done simpler, because usually the hub require port handling only after port status changed and ports have quite fixate determine states (according the the USB 2.0 Figure 11-10). So it allows to handle them one by one till they all are in any final state (powered off, disconnected [no device], connected [port has a device and device completed the reset] or disabled [overcurrent or unable to reset the device in port])
Maybe, with the following approach it might be simplified to two (or at elast three) k_work_sumbits:
-
k_work_submit(&hub_mgr_data->hub_work...): New hub appeared -> we need to get descriptor, configure the internal object and prepare ports for handling (allocate their objects and put in the list of pending ports), enable the interrupt EP IN to be able to catch the port or hub changes. (We need to be careful, as the hub already might have a device attached, so we can postpone enabling EP IN till we handle the port with a device - it might be another Hub to enable) -
k_work_submit(&hub_mgr_data->port_work...): Hubs port/ports have something, that needs to be handled. Usually, starts after the EP IN transfer complete. We can add he port to pending list and then the work might handle the pending port list till it is empty. Resubmit the transfer again when no more port to handle.
This might help to simplify the logic and make it more linear.
Otherwise it is easy to miss the important details or maintain the code IMHO.
|
|
||
| /* Port state enumeration */ | ||
| enum usbh_port_state { | ||
| PORT_STATE_DISCONNECTED, |
There was a problem hiding this comment.
There are two more possible port states: Powered Off and Disabled.
We can merge the PORT_STATE_ERROR with PORT_STATE_DISABLED, but this limits the legitimate state of changing the port to disable state by ClearPortFeature(PORT_ENABLE).
6 participants
Dependency: