drivers: usb: udc_stm32: Handle HAL callbacks on separate thread #80820
Conversation
|
A huge thanks. We'll for sure help testing on extended sample of series. |
|
Is it safe to enqueue a stack-allocated msg pointer? Perhaps the event approach is safer. Given that the combined number of IN and OUT endpoints is typically less than 32 (15 IN + 15 OUT), a Additionally, this approach helps prevent potential message queue overflows that might result in missed event/message handling and supports ordered processing. |
Yes, it is copied into the ringbuffer.
Isnt it technically up to 16+16 endpoints including the control endpoint?
While I agree about the message queue overflows, I don't see how k_event would support ordered processing? Thats mostly why I chose the msgq based approach, to make sure the events were processed in the order they arrived |
|
Not trying to block, just discussing out of curiosity.
Apologies, I just noticed the message is passed not a pointer; all good.
That fits.
Not in a fifo way, but In the processing. I.e. process |
Ah, I dont think this is what we would want though. All of these callbacks are triggered from the same IRQ, i.e. have the same priority, and should be processed in the order they arrived. It should maybe be considered how the ordering interdependency with the @erwango I believe it is ready for testing. Is there anything else I can do to help the testing effort along? |
|
@topisani It's failing on After: |
|
@erwango Alright, i believe this is my attempt at detecting USB HS support that fails. I have an l432 board i can test on, if that is the case, it should fail there as well |
|
I could reproduce the issue on a nucleo_l432kc, and it appears to have been the HS detection. It now follows the maximum-speed dts prop, like in most of the other udc drivers, which fixed the issue for me. |
|
Alright, i did some testing using
I have also done a test on an arduino_portenta_h7 based custom board, that also failed. The common factor is that both boards use HS USB. Checking with |
topisani
force-pushed
the
udc-stm32
branch
2 times, most recently
from
2275c22 to
2a056c0
Compare
zephyrbot
requested review from
erwango,
FRASTM,
gautierg-st,
GeorgeCGV,
jfischer-no,
marwaiehm-st,
mathieuchopstm and
tmon-nordic
|
Hi @topisani I've started to test on my side, here my results using
I will continue tests and report here. |
drivers/usb/udc/udc_stm32.c
Outdated
| @@ -912,6 +913,7 @@ static const struct udc_stm32_config udc0_cfg = { | |||
| .pma_offset = USB_BTABLE_SIZE, | |||
| .ep0_mps = EP0_MPS, | |||
| .ep_mps = EP_MPS, | |||
| .speed_idx = DT_ENUM_IDX(DT_DRV_INST(0), maximum_speed), | |||
There was a problem hiding this comment.
maximum_speed property is not available from all DT.
In old driver, property is check before access it:
zephyr/drivers/usb/device/usb_dc_stm32.c
Lines 70 to 72 in c0be51d
There was a problem hiding this comment.
| .speed_idx = DT_ENUM_IDX(DT_DRV_INST(0), maximum_speed), | |
| .speed_idx = DT_ENUM_IDX_OR(DT_DRV_INST(0), maximum_speed, 1), |
topisani
force-pushed
the
udc-stm32
branch
2 times, most recently
from
480c9c5 to
cfee3f3
Compare
| @@ -260,6 +243,28 @@ void HAL_PCD_DataOutStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum) | |||
| { | |||
| uint32_t rx_count = HAL_PCD_EP_GetRxCount(hpcd, epnum); | |||
| struct udc_stm32_data *priv = hpcd2data(hpcd); | |||
| struct udc_stm32_msg msg = { | |||
There was a problem hiding this comment.
I caught an issue where this check would trigger ZLP expected (only appeared on usbotg_hs), as multiple messages arriving before being handled would be merged. Storing the rx length in the msgq fixed this. It doubles the size of the messages, but I dont see a way around it. The cdc_acm test still fails on usbotg_hs.
zephyr/drivers/usb/udc/udc_common.c
Lines 1058 to 1064 in c0be51d
There was a problem hiding this comment.
Nice! I think this could potentially fix a couple of issues seen by @fpistm (particularly on Windows). He'll only be able to answer on monday though.
The cdc_acm test still fails on usbotg_hs.
Which board are you using for this test ?
There was a problem hiding this comment.
Which board are you using for this test ?
Mainly testing using stm32f769i_disco, but i see the same on a portenta_h7. I may be able to test the fs interface on the h7 to verify that it only happens on hs, but the portenta is mounted in a custom board where its a bit more difficult to perform isolated tests.
Just to clarify: when running the cdc_acm sample, descriptors are exchanged without issue, but no characters are echoed back. I am currently doing some debugging with wireshark, i will report a separate issue once i have more context.
There was a problem hiding this comment.
@topisani
I experienced the same problem of characters not being echoed back with the stm32h747i_disco board, but it ran smoothly on the nucleo_h743zi board, which means it only occurs on HS.
There was a problem hiding this comment.
i have now also confirmed that it works on the usbotg_fs interface on stm32f769i_disco, and not on usbotg_hs on the same board. This issue with HS also exists on main.
run clang-format on udc_stm32.c Signed-off-by: Tobias Pisani <[email protected]>
If both new and old stm32 usb drivers are enabled, the HAL callbacks will raise multiple definition linker errors. By adding this kconfig dependency, this will be reported as a kconfig warning that should be easier to understand. This will happen when trying to run samples with -DCONF_FILE=usbd_next_prj.conf for boards that automatically enable the legacy usb stack (e.g. arduino_portenta/stm32h747xx/m7). Signed-off-by: Tobias Pisani <[email protected]>
All HAL callback handling is offloaded to a separate thread, as they involve non isr-compatible operations such as mutexes. Fixes zephyrproject-rtos#61464 Signed-off-by: Tobias Pisani <[email protected]>
This fixes usbd_caps_speed(), which is used in sample_usbd_init, and the documentation. Without it, no high speed configuration would be added. Signed-off-by: Tobias Pisani <[email protected]>
On USB HS, the previous lower limit of 64 is too small, the value 160 has been chosen apparently through trial and error. See 1204aa2 for the original implementation in the old device driver. Signed-off-by: Tobias Pisani <[email protected]>
|
I found the usbotg_hs issue - porting this fix to device-next helped: #62530 For what it's worth, all issues I have encountered through testing have now been fixed. |
|
Thank you @topisani for your great work! Is it also required to push request to Similar as in NRF driver: zephyr/drivers/usb/udc/udc_nrf.c Lines 584 to 604 in 6dafb5f |
| @@ -6,6 +6,7 @@ config UDC_STM32 | |||
| depends on DT_HAS_ST_STM32_OTGFS_ENABLED \ | |||
| || DT_HAS_ST_STM32_OTGHS_ENABLED \ | |||
| || DT_HAS_ST_STM32_USB_ENABLED | |||
| depends on !USB_DC_STM32 | |||
There was a problem hiding this comment.
@jfischer-no I guess this issue is wider than STM32 and related to #81308. Are you fine this line ?
There was a problem hiding this comment.
It's also a bit specific to this driver though - at least theoretically it should be possible with multi-instance compatible controllers to use one with the new api, and one with the old. But the two stm32 drivers specifically cannot coexist because of the Hal callback definitions.
There was a problem hiding this comment.
USB_DC_STM32 cannot be used here, once deprecated it should fail in CI.
I considered it, but as far as I can tell it should be ok. I will take another look |
| @@ -6,6 +6,7 @@ config UDC_STM32 | |||
| depends on DT_HAS_ST_STM32_OTGFS_ENABLED \ | |||
| || DT_HAS_ST_STM32_OTGHS_ENABLED \ | |||
| || DT_HAS_ST_STM32_USB_ENABLED | |||
| depends on !USB_DC_STM32 | |||
There was a problem hiding this comment.
USB_DC_STM32 cannot be used here, once deprecated it should fail in CI.
| @@ -60,6 +60,18 @@ struct udc_stm32_config { | |||
| uint16_t ep_mps; | |||
| }; | |||
|
|
|||
| enum udc_stm32_msg_kind { | |||
There was a problem hiding this comment.
kind? enum udc_stm32_msg_type!
| void HAL_PCD_SetupStageCallback(PCD_HandleTypeDef *hpcd) | ||
| { | ||
| struct udc_stm32_data *priv = hpcd2data(hpcd); | ||
| struct udc_stm32_msg msg = {.kind = UDC_STM32_MSG_SETUP}; | ||
| int err = k_msgq_put(&priv->msgq_data, &msg, K_NO_WAIT); | ||
|
|
||
| if (err < 0) { | ||
| LOG_ERR("UDC Message queue overrun"); | ||
| } |
There was a problem hiding this comment.
There is actually better way to handle it using k_event, take a look at how dwc2 driver handle it, or just start to use it.
There was a problem hiding this comment.
The driver specifically needs to handle separate RX callbacks individually. I dont see a way to do this using events, as multiple callbacks for one endpoint can occur between the handler thread running. I assume this is the reason why the udc_nrf driver also does it this way.
| int err = k_msgq_put(&priv->msgq_data, &msg, K_NO_WAIT); | ||
|
|
||
| if (err < 0) { |
There was a problem hiding this comment.
No hidden calls, no undefined behavior.
int err;
err = k_msgq_put(&priv->msgq_data, &msg, K_NO_WAIT);
if (err) {| */ | ||
| words = MAX(0x40, cfg->ep_mps / 4); | ||
| words = MAX(160, cfg->ep_mps / 4); |
There was a problem hiding this comment.
On USB HS, the previous lower limit of 64 is too small, the value
160 has been chosen apparently through trial and error.
At a minimum, this value should be selected based on the controller type and capabilities, not "trial and error".
There was a problem hiding this comment.
See the discussion here: #62530
I've been digging through documentation and related issues from when this change was introduced on the old usb device driver, and i cannot seem to find any satisfactory explanation other than "this value works". This configuration is what worked in the old driver, which has been used a lot in practice. But maybe someone at ST can help settle where this value comes from (@erwango)?
There was a problem hiding this comment.
Unfortunately @Desvauxm-st left the company. So I don't have more information other than what is available in the PR. @marwaiehm-st maybe ?
There was a problem hiding this comment.
Unfortunately, we tried to find a satisfactory explanation other than "this value works", but until now, we haven't found an explanation for why the minimum value of FIFO RX needs to be 160.
| @@ -29,20 +29,20 @@ | |||
| LOG_MODULE_REGISTER(udc_stm32, CONFIG_UDC_DRIVER_LOG_LEVEL); | |||
|
|
|||
| #if DT_HAS_COMPAT_STATUS_OKAY(st_stm32_otghs) | |||
| #define DT_DRV_COMPAT st_stm32_otghs | |||
There was a problem hiding this comment.
No reformatting of the code that follows the coding-style.
|
Hi, In any case it fixes the assert issue reported here: #61464.
Note
Workaround 1
@@ -458,11 +458,14 @@ static int usbd_cdc_acm_ctd(struct usbd_class_data *const c_data,
if (setup->wLength != len) {
errno = -ENOTSUP;
return 0;
}
- memcpy(&data->line_coding, buf->data, len);
+ if (buf->data) {
+ printk("SET_LINE_CODING buf->data not null\n");
+ memcpy(&data->line_coding, buf->data, len);
+ }
cdc_acm_update_uart_cfg(data);
usbd_msg_pub_device(uds_ctx, USBD_MSG_CDC_ACM_LINE_CODING, dev);
return 0;
case SET_CONTROL_LINE_STATE:dts updated+
+zephyr_udc0: &usb {
+ pinctrl-0 = <&usb_dm_pa11 &usb_dp_pa12>;
+ pinctrl-names = "default";
+ status = "okay";
+};nucleo_f303re
Assert enabledlogPlug/unplugImportant On linux, second plug produce this issue (re-enumeration): log |
Thanks @fpistm. Hence to summarize, this PR actually fixes #61464, but some issues remain, likely all linked/close to #76250. For clarity, I'll open a new issue to gather all this. |
All HAL callback handling is offloaded to a separate thread.
Tested on STM32H747 (arduino portenta) with the high speed controller.
This is an attempt at fixing #61464.
Marked as a draft, as i would at least like to do some minor stylistic cleanup before submitting, but if anyone could help test, I would appreciate it. As far as i can tell, no additional locking should be required, and no timing invariants are broken, but I could use some extra eyes on that as well.