drivers: usb: udc: Add SAM UDP driver#102041
drivers: usb: udc: Add SAM UDP driver#102041aescolar merged 7 commits intozephyrproject-rtos:mainfrom
Conversation
|
ping |
jfischer-no
left a comment
There was a problem hiding this comment.
Thanks, it looks very interesting. I initially thought it was port of usb_dc_sam_usbc driver.
| * EP4 and EP5 are the only endpoints with 512-byte capacity. | ||
| * Reserve them for ISO transfers or requests needing > 64 bytes. |
There was a problem hiding this comment.
I am not convinced that it makes sense. "EP4" and "EP5" are the last resources that stack would allocate.
There was a problem hiding this comment.
The testusb fail without this checks if I'm mistake.
There was a problem hiding this comment.
I looked at it again, and I still do not think it is necessary. Therefore, I am reluctant to add something like 108734d. Please recheck it.
a348e11 to
d895312
Compare
|
Needs rebase, and likely another one after #101731 |
|
Tested in $ sudo dmesg -c
[ 610.392333] usb 9-1: new full-speed USB device number 3 using xhci_hcd
[ 610.554407] usb 9-1: New USB device found, idVendor=2fe3, idProduct=0009, bcdDevice= 4.03
[ 610.554418] usb 9-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 610.554425] usb 9-1: Product: Zephyr testusb sample
[ 610.554430] usb 9-1: Manufacturer: Zephyr Project
[ 610.554434] usb 9-1: SerialNumber: 00310053594A51353032303233303830
[ 610.564479] usbtest 9-1:1.0: Linux gadget zero
[ 610.564482] usbtest 9-1:1.0: full-speed {control in/out bulk-in bulk-out} tests (+alt)
[ 610.564605] usbtest 9-1:1.1: Linux gadget zero
[ 610.564606] usbtest 9-1:1.1: full-speed {control in/out int-in int-out} tests (+alt)
[ 610.564720] usbtest 9-1:1.2: Linux gadget zero
[ 610.564722] usbtest 9-1:1.2: full-speed {control in/out iso-in iso-out} tests (+alt)
$ sudo ./testusb -v 512 -D /dev/bus/usb/009/003
./testusb: /dev/bus/usb/009/003 may see only control tests
full speed /dev/bus/usb/009/003 0
/dev/bus/usb/009/003 test 0, 0.000008 secs
/dev/bus/usb/009/003 test 1, 1.166576 secs
/dev/bus/usb/009/003 test 2, 1.272667 secs
/dev/bus/usb/009/003 test 3, 0.600458 secs
/dev/bus/usb/009/003 test 4, 0.637901 secs
/dev/bus/usb/009/003 test 5, 35.728922 secs
/dev/bus/usb/009/003 test 6, 40.840142 secs
/dev/bus/usb/009/003 test 7, 18.411204 secs
/dev/bus/usb/009/003 test 8, 21.045679 secs
/dev/bus/usb/009/003 test 9, 18.000992 secs
/dev/bus/usb/009/003 test 10, 86.007103 secs
/dev/bus/usb/009/003 test 11, 3.078470 secs
/dev/bus/usb/009/003 test 12, 3.524887 secs
/dev/bus/usb/009/003 test 13, 26.000590 secs
/dev/bus/usb/009/003 test 14, 6.468361 secs
/dev/bus/usb/009/003 test 17, 1.165068 secs
/dev/bus/usb/009/003 test 18, 1.272749 secs
/dev/bus/usb/009/003 test 19, 1.166568 secs
/dev/bus/usb/009/003 test 20, 1.272739 secs
/dev/bus/usb/009/003 test 21, 6.491996 secs
/dev/bus/usb/009/003 test 24, 33.768859 secs
/dev/bus/usb/009/003 test 27, 35.555712 secs
/dev/bus/usb/009/003 test 28, 40.729050 secs
/dev/bus/usb/009/003 test 29, 4.000164 secs
$ sudo dmesg -c
[ 618.485394] usbtest 9-1:1.0: TEST 0: NOP
[ 618.487379] usbtest 9-1:1.0: TEST 1: write 1024 bytes 1000 times
[ 619.654364] usbtest 9-1:1.0: TEST 2: read 1024 bytes 1000 times
[ 620.927376] usbtest 9-1:1.0: TEST 3: write/512 0..1024 bytes 1000 times
[ 621.528369] usbtest 9-1:1.0: TEST 4: read/512 0..1024 bytes 1000 times
[ 622.167378] usbtest 9-1:1.0: TEST 5: write 1000 sglists 32 entries of 1024 bytes
[ 657.897430] usbtest 9-1:1.0: TEST 6: read 1000 sglists 32 entries of 1024 bytes
[ 698.738408] usbtest 9-1:1.0: TEST 7: write/512 1000 sglists 32 entries 0..1024 bytes
[ 717.150412] usbtest 9-1:1.0: TEST 8: read/512 1000 sglists 32 entries 0..1024 bytes
[ 738.197410] usbtest 9-1:1.0: TEST 9: ch9 (subset) control tests, 1000 times
[ 756.200404] usbtest 9-1:1.0: TEST 10: queue 32 control calls, 1000 times
[ 842.209422] usbtest 9-1:1.0: TEST 11: unlink 1000 reads of 1024
[ 845.288418] usbtest 9-1:1.0: TEST 12: unlink 1000 writes of 1024
[ 848.814424] usbtest 9-1:1.0: TEST 13: set/clear 1000 halts
[ 874.816445] usbtest 9-1:1.0: TEST 14: 1000 ep0out, 1..1024 vary 512
[ 881.289441] usbtest 9-1:1.0: TEST 17: write odd addr 1024 bytes 1000 times core map
[ 882.455430] usbtest 9-1:1.0: TEST 18: read odd addr 1024 bytes 1000 times core map
[ 883.729455] usbtest 9-1:1.0: TEST 19: write odd addr 1024 bytes 1000 times premapped
[ 884.896432] usbtest 9-1:1.0: TEST 20: read odd addr 1024 bytes 1000 times premapped
[ 886.170454] usbtest 9-1:1.0: TEST 21: 1000 ep0out odd addr, 1..1024 vary 512
[ 892.668444] usbtest 9-1:1.0: TEST 24: unlink from 1000 queues of 32 1024-byte writes
[ 926.442433] usbtest 9-1:1.0: TEST 27: bulk write 31Mbytes
[ 961.998461] usbtest 9-1:1.0: TEST 28: bulk read 31Mbytes
[ 1002.728482] usbtest 9-1:1.0: TEST 29: Clear toggle between bulk writes 1000 times
|
| /* | ||
| * Call driver-specific callback if basic check passed. | ||
| * This allows drivers to apply additional restrictions, | ||
| * such as reserving high-capacity endpoints for ISO transfers. | ||
| */ | ||
| if (ret == true && api->ep_try_config != NULL) { | ||
| uint8_t saved_attributes = cfg->attributes; | ||
| uint16_t saved_mps = cfg->mps; | ||
| uint8_t saved_interval = cfg->interval; | ||
|
|
||
| /* Temporarily fill in request values for callback */ | ||
| cfg->attributes = attributes; | ||
| cfg->mps = *mps; | ||
| cfg->interval = interval; | ||
|
|
||
| if (api->ep_try_config(dev, cfg) != 0) { | ||
| ret = false; | ||
| } | ||
|
|
||
| /* Restore original values */ | ||
| cfg->attributes = saved_attributes; | ||
| cfg->mps = saved_mps; | ||
| cfg->interval = saved_interval; | ||
| } | ||
|
|
There was a problem hiding this comment.
You are facing just one instance of the problem where current endpoint allocation gives bad results. The problem I would describe as "higher capacity endpoint assigned to class that does not need it, prevents use of the higher capacity endpoint by class that needs it".
There is also different category of problems that the current handling cannot account for. I would describe it as "isochronous endpoints consume resources necessary for two bulk/interrupt endpoints". See #105151 for workaround.
I believe just patching things like you do here just won't scale up. I think the endpoint assignment in general needs rework and I would prefer this changes to be out-of-scoped from here.
There was a problem hiding this comment.
Agreed to remove this commit.
| static const uint8_t in_ep_hw_map[] = {0, 1, 3, 5, 7}; | ||
| static const uint8_t out_ep_hw_map[] = {0, 2, 4, 6}; |
There was a problem hiding this comment.
Is the ep_mps_map bound to particular endpoint number, or is it just something like "buffer instance"?
Would it be possible to put the indexes 4 and 5 at the end of each list? If yes, then doing so would be good enough workaround to "higher capacity endpoint assigned to class that does not need it, prevents use of the higher capacity endpoint by class that needs it" without changing the current endpoint assignment logic.
There was a problem hiding this comment.
I'll implement a simple option with Kconfig to reserve that EP to iso operations only. This will have the lowest impact in the driver at moment. In future, I would decouple the physical EP and create some kind of Logical EP to handle the allocation then this suggestion will work.
There was a problem hiding this comment.
Kconfig sounds like the best workaround for the time being.
In future, I would decouple the physical EP and create some kind of Logical EP to handle the allocation then this suggestion will work.
USB stack uses the endpoint address and puts it in class endpoint descriptor. Endpoint address as seen by the USB stack must match endpoint number as seen on the bus.
The feasibility of this workaround depends on whether the fifos capable of 512 byte transfers can only ever work as endpoint 0x04/0x84 or 0x05/0x85 or any endpoint. If they are limited to endpoint 4 or 5 (regardless of IN or OUT) then the logical decouple won't help.
Add devicetree nodes for the UDP peripheral on SAM4S and SAM4E SoCs, along with the corresponding devicetree binding. The UDP controller is present at: - SAM4S: 0x40034000, IRQ 34 - SAM4E: 0x40084000, IRQ 35 Both variants share the same compatible "atmel,sam-udp" and feature: - 8 hardware endpoints (EP0-EP7) - 1 bidirectional endpoint (EP0 for control) - 5 IN endpoints, 4 OUT endpoints - Full-speed USB 2.0 (12 Mbps) Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
|
New version without the commit about |
| if (IS_ENABLED(CONFIG_UDC_SAM_UDP_ISO_EP_RESERVE) | ||
| && mps > 64) { | ||
| /* Reserve 512B endpoints for ISO only */ | ||
| config->ep_cfg_in[i].caps.iso = (hw_ep != 3); | ||
| } else { | ||
| config->ep_cfg_in[i].caps.bulk = 1; | ||
| config->ep_cfg_in[i].caps.interrupt = 1; | ||
| config->ep_cfg_in[i].caps.iso = (hw_ep != 3); |
There was a problem hiding this comment.
Do not introduce a new Kconfig option. It does not solve any actual problems and there is no way it could be used in the tree. Just use what is in the else branch.
There was a problem hiding this comment.
The reservation is necessary to avoid an issue with some classes that try allocate all bulk/interrupt before the iso ones. A bulk/int can pick this EP and then there won't be any other available to iso - which requires big sizes.
The Kconfig is specific for this driver and not for the whole tree. It can be simple used like below and a final user can simple add in their prj.conf or board.conf depending on their project.
west build -b sam4s_xplained samples/subsys/usb/testusb -- -DCONFIG_UDC_SAM_UDP_ISO_EP_RESERVE=yThe testusb is the classic class that does not enumerate without this.
There was a problem hiding this comment.
It does workaround actual problem of the fact that endpoint assignment goes with endpoints from lowest numbered one, without choosing the "least capable one". There is currently no way for user to force specific endpoint assignments (which would be one way of working around the problem).
If the hardware in question had the most capable endpoint as the last one, then testusb would work.
A workaround without Kconfig (i.e. always reserve the endpoint for iso) would be detrimental IMHO for applications that don't need isochronous (they wouldn't be able to use the most capable endpoint).
There was a problem hiding this comment.
The testusb is the classic class that does not enumerate which this.
okay, I will keep it in mind.
There was a problem hiding this comment.
Just as a follow up,
All this problem could be solved if the enumeration can be made using the following sequence:
1- setup (obvious reasons)
2- iso (biggest Ep capabilities first)
3- bulk (possible bigger than 64 in HS)
4- int
With above, the core could complete remove the shim workarounds about Ep sizes.
There was a problem hiding this comment.
Bulk is always 512 at High-Speed, 1024 at Super-Speed. Only at Full-Speed, it is up to device to choose between 8/16/32/64. Interrupt can have alternate configuration requiring up to 3 * 1024 bytes (High-Speed High-Bandwidth interrupt endpoint). Isochronous is not necessarily requiring larger fifos than High-Speed bulk, it all depends on actual application.
Therefore if there is heuristic to endpoint assignments then I think better change would be to go with highest wMaxPacketSize (or better: total payload length, as it also covers High-Bandwidth endpoints) first.
There was a problem hiding this comment.
Hi @tmon-nordic
Yes, you are 100% correct! I did not remember about the "gray" area in HS and that was the main reason about the Kconfig simple solution. Now I implemented your suggestion from yesterday.
In fact, it will not work due to the SAM UDP the hardware endpoint IS bound to the token address there's no address remapping in the hardware. Thanks for the hint @tmon-nordic !
I agreed that a good heuristic could help a lot.
Add a new USB Device Controller (UDC) driver for Atmel SAM4S, SAM4E, and SAM3S microcontrollers with the UDP (USB Device Port) peripheral. This driver replaces the deprecated legacy USB stack before removal. Hardware Specifications: - USB 2.0 Full-Speed only (12 Mbps) - 8 hardware endpoints (EP0-EP7) - 2668 bytes embedded DPRAM/FIFO - No DMA - all transfers via PIO - Integrated transceiver with pull-up on DDP - Dual-bank (ping-pong) support on EP1,2,4,5,6,7 Endpoint Configuration (per datasheet Table 40-1): - EP0: Control, no dual-bank, 64 bytes - EP1,2: Bulk/Iso/Int, dual-bank, 64 bytes each bank - EP3: Bulk/Int, no dual-bank, 64 bytes - EP4,5: Bulk/Iso/Int, dual-bank, 512 bytes each bank - EP6,7: Bulk/Iso/Int, dual-bank, 64 bytes each bank Key Implementation Details: 1. Dual-Bank Pipelining (IN transfers): - Track filled banks with tx_banks counter (0-2) - Set TXPKTRDY after each bank fill per datasheet 40.6.2.2 - Pre-fill second bank while first is transmitting 2. OUT Processing: - Bulk/Int endpoints processed directly in ISR - EP0 uses thread-based processing for control transfer flexibility - Bank alternation tracking for dual-bank endpoints 3. Clock Management: - PLLB configured for 48MHz USB clock (UDPCK) - Proper suspend/resume with clock gating - Transceiver disable during disconnect for power saving 4. CSR Register Handling: - Special "write 1 to preserve" bits handled correctly - Polling after set/clear for MCK/UDPCK synchronization - RX_DATA_BK0, RX_DATA_BK1, RXSETUP, STALLSENT, TXCOMP preserved 5. Endpoint Allocation Strategy: - Odd endpoints (1,3,5,7) for IN, even (2,4,6) for OUT - EP0 for control transfers Performance: - Achieves ~800-900 KB/s throughput - Near USB Full-Speed theoretical maximum (~1.2 MB/s) - All testusb tests (0-29) passing Tested on sam4s_xplained board with Linux testusb utility. Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Add Atmel USB UDP configurations: - pinctrl - devicetree - yaml config Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Add CONFIG_SOC_ATMEL_SAM_MCK_PRESCALLER to allow configure the Master Clock (MCK) prescaler. This is needed for SAM4E which requires a prescaler of 2 to achieve the correct clock frequency. Changes: - Add SOC_ATMEL_SAM_MCK_PRESCALLER Kconfig option with default of 2 for SAM4E and 1 for other SAM series - Fix SOC_ATMEL_SAM_PLLA_MULA default for SAM4E from 9 to 19 to achieve 120MHz: 12MHz * 20 / 2 = 120MHz - Use CONFIG_SOC_ATMEL_SAM_MCK_PRESCALLER in SoC init for SAM4E, SAM4S, SAMX7X Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
SAM4E doesn't have PLLB, so the USB clock must be derived from PLLA which is shared with the system clock. Use PMC_MCKR_CSS_PLLB_CLK to detect PLLB availability at compile time: - SAM4S (has PLLB): Use dedicated PLLB, 96MHz / 2 = 48MHz - SAM4E (no PLLB): Use PLLA, ~240MHz / 5 = 48MHz For SAM4E, PLLA is already configured by the SoC init code for the system clock, so we just verify it's locked and configure the USB divider. On shutdown, we don't disable PLLA since it's needed for the system clock. Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Add Atmel USB UDP configurations: - pinctrl - devicetree - yaml config Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Add sam4e_xpro in the integration_platform list to build the atmel,sam_udp compabitle driver. Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
|



Summary
Add USB Device Controller (UDC) driver for SAM4S/E UDP peripheral, enabling USB device_next stack support for these MCUs.
drivers/usb/udc/udc_sam_udp.cTest Plan