drivers: usb: udc: stm32: driver cleanup and EP MPS fixes #96926
Conversation
The EP0 max packet size was de facto a constant because its value was the same regardless of which USB IP was in use. However, it was stored as part of the instance configuration anyways which is wasteful and slower. Create new "UDC_STM32_EP0_MAX_PACKET_SIZE" driver-level constant with which all usage of the per-instance configuration field is replaced. Signed-off-by: Mathieu Choplain <[email protected]>
The ST USB controller (compatible 'st,stm32-usb') is fitted with private
SRAM called the Private Memory Area or PMA. This is primarily used to hold
data transfered over USB, but it also contains the so-called 'buffer table'
(a.k.a. BTABLE) which holds the base address and size of buffers in the PMA
allocated to each endpoint. The BTABLE is placed by the driver at the start
of the PMA and occupies a fixed size ('USB_BTABLE_SIZE'), which was stored
in the driver configuration field 'pma_offset'. This mechanism is unused on
non-ST USB controllers from STM32 microcontrollers, but USB_BTABLE_SIZE is
still defined (to a dummy value) and stored in the 'pma_offset' field which
becomes unused.
Remove the 'USB_BTABLE_SIZE' definition and the 'pma_offset' field from the
driver configuration, and update the ST USB controller-specific verison of
'udc_stm32_mem_init' to derive the BTABLE size from the number of endpoints
that the controller has instead.
Signed-off-by: Mathieu Choplain <[email protected]>
STM32 OTG USB controllers use word-addressable RAM with a 32-bit word size so all allocations from the USB SRAM must be 32-bit aligned. The driver did not accept unaligned values of wMaxPacketSize, and FIFO allocation code was implicitly expecting FIFO sizes to be aligned as well (since it allocated "bytes / 4" without rounding up). Update driver to accept values of wMaxPacketSize that aren't word-aligned and to allocate properly sized FIFOs sizes when an unaligned size has been requested. Signed-off-by: Mathieu Choplain <[email protected]>
The maximal packet size (for non-control endpoints) was obtained by the driver from HAL definitions which appear to not properly reflect hardware capabilities. Update driver to allow endpoints with wMaxPacketSize up to the maximal value allowed by the USB Specification depending on operation mode, since all STM32 USB controllers always support these values. Also move the EP max packet size field in the 'struct udc_stm32_config' to avoid implicit padding and add a documentation comment. Signed-off-by: Mathieu Choplain <[email protected]>
mathieuchopstm
changed the title
zephyrbot
requested review from
djiatsaf-st,
erwango,
etienne-lms,
FRASTM,
gautierg-st,
GeorgeCGV,
jfischer-no,
marwaiehm-st and
tmon-nordic
| * beginning of Private Memory Area and consumes | ||
| * 8 bytes for each endpoint. | ||
| */ | ||
| priv->occupied_mem = (8 * cfg->num_endpoints); |
There was a problem hiding this comment.
Nitpicking: could drop parentheses.
There was a problem hiding this comment.
My bad, copy-paste artifact from expanding USB_BTABLE_SIZE = (8 * USB_NUM_BIDIR_ENDPOINTS).
| #define UDC_STM32_NODE_EP_MPS(node_id) \ | ||
| ((UDC_STM32_NODE_SPEED(node_id) == PCD_SPEED_HIGH) ? 1024U : 1023U) | ||
|
|
||
|
|
There was a problem hiding this comment.
Nitpicking: could you remove this extra empty line?
| * | ||
| * WARNING: Don't mix USB defined in STM32Cube HAL and CONFIG_USB_* from Zephyr | ||
| * Kconfig system. | ||
| */ |
There was a problem hiding this comment.
Are these inline comment still useful? (non-blocking, liekly a bit outside the scope of this P-R)
There was a problem hiding this comment.
IMO the comment still applies because there is some usage remaining of the Cube macros (see L1115 in new file). But eventually it should be removed yes.
|
I am seeing a regression on zephyr/drivers/usb/udc/udc_stm32.c Lines 667 to 670 in 8940344 This SoC has only 1280 bytes of DRAM. Beforehand, In addition, 64 bytes are reserved for EP0-OUT TX FIFO, so the RAM usage after initialization is now 1088 bytes (192 bytes free) instead of 708 (576 bytes free). The zephyr/drivers/usb/udc/udc_stm32.c Line 682 in 36bc2f3 This means that [1] I suspect the "mysterious line" is here to increase TX FIFO sizes as recommended in RefMan: "More space allocated in the transmit IN endpoint FIFO results in better performance on the I will modify the driver to implement similar logic to the DWC2 driver:
|
Create a new Kconfig option allowing to tweak the RxFIFO size on OTG_FS and OTG_HS instances, and replace the old hardcoded method with this new mecanism. The default value of 600 bytes yields a similar size to the the previous hardcoded default of 160 words (= 640 bytes) when combined with the fixed overhead computed by the driver (~56 bytes on OTG_FS with 6 endpoints). Also fix a tiny error in a logging message (DRAM size in bytes, not bits). Signed-off-by: Mathieu Choplain <[email protected]>
|
Went for a different solution which allows tweaking RxFIFO size via Kconfig. This is acceptable for now since the driver is single-instance anyways, and more flexible than hardcoding a value in the driver. Testing will be done at a later time to confirm no regression is observed... |
|
|
I tested it on the apex_pro_mini / stm32l412 target and it works! 🥳. The exact test being: checking that lsusb gives me a "NordicSemiconductor Zephyr testusb sample", and also verifying that the testusb enumeration disappears when I disconnect the device. |
|
With the usual cherry-pick from 94001, I see one regression compared to #96028 (comment): test 13 no longer passes on This is surprising given that the board uses the ST USB IP which (should!) see 0 functional change (it isn't affected by the Note: I did not re-test N6 since it should be almost identical to U5A5 (same IP, only PHY differs). |
Cleans up various parts of the STM32 UDC shim driver while also fixing issues related to endpoint sizes.