drivers: usb: uhc: add hub topology fields and getters#102838
drivers: usb: uhc: add hub topology fields and getters#102838AidenHu wants to merge 4 commits intozephyrproject-rtos:mainfrom
Conversation
|
| #ifdef CONFIG_USBH_HUB_CLASS | ||
| /** Device's first connected hub address (root hub = 0) */ | ||
| uint8_t hub_addr; | ||
| /** Device's first connected hub's port */ | ||
| uint8_t hub_port; | ||
| /** Device's first connected high-speed hub's address */ | ||
| uint8_t hs_hub_addr; | ||
| /** Device's first connected high-speed hub's port */ | ||
| uint8_t hs_hub_port; | ||
| /** Device's level (root device = 0) */ | ||
| uint8_t level; | ||
| /** Device's first connected hub's total think time */ | ||
| uint16_t total_think_time; | ||
| #endif |
There was a problem hiding this comment.
USBH_HUB_CLASS uses the wrong namespace and must not be used in the driver API. I cannot think of any reasonable reason to have this option or to make these properties conditional. There is no reason to have hub_addr/hub_port. Hub is just a device. And having something like usb_device *hub; uint8_t portnum; uint8_t level; should be sufficient. But the purpose of this commit is unclear.
There was a problem hiding this comment.
@jfischer-no
Thank you for the reply. Actually this PR's commit is from the usb: host: class: support for usb host video class #94085. We have to consider about the split transfer and MCUX USB controllers need to get these information to calculate bandwidth allocation.
Agree that USBH_HUB_CLASS naming seems not suitable, USBH_HUB_CLASS macro can be removed to avoid breaking namespace.
There was a problem hiding this comment.
@AidenHu I think @jfischer-no means you can try to add one usb_device *parent to this struct, then some information can be get through the parent.
7124f2e to
523ca72
Compare
|
@jfischer-no |
josuah
left a comment
There was a problem hiding this comment.
Currently, the new HUB-related fields are not populated by the stack.
However, the driver is not having a bug: it behaves the same way as before, where *infoValue was set to 0 already, so maybe this works without problem to merge the commits in this order.
Some optional improvement are proposed.
523ca72 to
d2e52fd
Compare
| #define USB_HUB_GET_THINK_TIME(wHubCharacteristics) \ | ||
| (((((wHubCharacteristics) & 0x0060U) >> 5) + 1) << 3) |
There was a problem hiding this comment.
This is not technically Chapter 9 USB Device Framework but rather Chapter 11 Hub Specification. I am not sure if it should be added to usb_ch9.h or if a new header should be created.
There was a problem hiding this comment.
My mistake, I advised wrong.
|
Thank you @tmon-nordic and @josuah Currently I am thinking about location of |
I just found this by chance: zephyr/include/zephyr/usb/class/usb_hub.h Lines 7 to 44 in 7bc9e93 This already has chapter 11 tables, like That looks like reasonable to extend it.
Yes my bad I did a mistake by advising |
@josuah, |
55dcfe9 to
a6d3395
Compare
a6d3395 to
89fe98f
Compare
|
@jfischer-no |
|
@josuah and @jfischer-no |
| #define USB_HCREQ_STOP_TT 0x0B | ||
|
|
||
| /** | ||
| * @brief Get Think Time field from wHubCharacteristics. |
There was a problem hiding this comment.
* Get Think Time value from wHubCharacteristics field.
| /** | ||
| * @brief Get Think Time field from wHubCharacteristics. | ||
| * | ||
| * @param wHubCharacteristics Hub characteristics value from hub descriptor. |
There was a problem hiding this comment.
* @param wHubCharacteristics Hub descriptor field wHubCharacteristics
| /** Pointer to the hub to which this device is connected */ | ||
| struct usb_device *hub; | ||
| /** Device's hub wHubCharacteristics */ | ||
| uint16_t hub_characteristics; |
There was a problem hiding this comment.
I think the whole wHubCharacteristics should not be exposed. Also using USB_HUB_GET_THINK_TIME() in the driver look a bit odd. Only think time seems to be relevant. Change it to
/** 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.
My bad for proposing to expose wHubCharacteristics instead of only the Think Time then!
There was a problem hiding this comment.
My bad for proposing to expose
wHubCharacteristicsinstead of only the Think Time then!
Never mind, this does not take much time.
| /** 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; |
There was a problem hiding this comment.
Change in uhc.h does not seem to belong in this commit.
|
|
||
| if (ctx->root == NULL) { | ||
| ctx->root = udev; | ||
| udev->level = 1; |
There was a problem hiding this comment.
There is not udev->level. This commit is just wrong.
host: core: set initialized value of level for first udev
What host? What core?
usb: host: set level value for the first USB device ?
There was a problem hiding this comment.
Yes. Adjust the commits' order and msg. Updated.
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>
89fe98f to
3ff302c
Compare
|
@jfischer-no |
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>
3ff302c to
6f75d48
Compare
|
| /** @} */ | ||
|
|
||
| /** | ||
| * @name USB Hub Class Feature Selectors. See Table 11-17 of the specification. |
There was a problem hiding this comment.
Here is a preview of the doc after this is merged:
https://builds.zephyrproject.io/zephyr/pr/102838/docs/doxygen/html/usb__hub_8h.html
As you see here the "See Table 11-17 ..." is on the title, I would be tempted to split it to the next line:
/**
* @name USB Hub Class Feature Selectors.
*
* See Table 11-17 of the specification.
*/Here and other locations.
|
Close this pr, all of changes are in the main hub PR: #99735 |



Add hub-related fields to struct usb_device under
CONFIG_USBH_HUB_CLASS:
Update MCUX UHC common code to return these values in hub-related queries.