drivers: usb: udc: Add UDPHS driver#99620
drivers: usb: udc: Add UDPHS driver#99620MCHP-MPU-Solutions-SHA wants to merge 4 commits intozephyrproject-rtos:mainfrom
Conversation
| USB packets system data bursts will be locked for | ||
| maximum optimization of the bus bandwidth usage. |
There was a problem hiding this comment.
I have no idea what this really means. From code it seems that you set some bit. Is it essentially affecting DMA operation/memory access arbitrage?
There was a problem hiding this comment.
Hi, this is a chip related feature, for more details please refer to the datasheet, see bit BURST_LCK in register UDPHS_DMACONTROL.
This will allow the UDPHS DMA channel has max burst length when accessing the bus.
And I made a switch for this feature in the Kconfig, customer can enable it for higher USB bandwidth under heavy system load.
347563c to
5a56317
Compare
nandojve
left a comment
There was a problem hiding this comment.
Add a report from sample/subsys/usb/testusb HS. FS is it should work like that too.
A full report usually tests from 0 up to 29.
| }; | ||
| }; | ||
|
|
||
| udphsa: usbd@200000 { |
There was a problem hiding this comment.
Convention is order by address.
There was a problem hiding this comment.
The devices in sama7g5.dtsi have been listed in alphabetical order, so I put the udphs device at the end of it.
Do you still need me order the udphs devices by space address?
|
|
||
| compatible: "microchip,sam-udphs" | ||
|
|
||
| include: [usb-ep.yaml, pinctrl-device.yaml] |
There was a problem hiding this comment.
zephyr/dts/bindings/ethernet/atmel,gmac-common.yaml
Lines 5 to 7 in db0c34d
-include: [usb-ep.yaml, pinctrl-device.yaml]
+include:
+ - name: usb-ep.yaml
+ - name: pinctrl-device.yamlThere was a problem hiding this comment.
Updated.
5a56317 to
9ca0f9a
Compare
Hi Gerson, Many thanks. |
If I'm not mistake, 10 is about data out in control EP. The below could give you some hints. |
| * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries | ||
| * | ||
| * SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
SPDX-FileCopyrightText: Copyright Microchip Technology Inc. and its subsidiaries
https://docs.zephyrproject.org/latest/contribute/guidelines.html#copyright-and-license-notices
There was a problem hiding this comment.
Fixed.
| #define ENABLE 1 | ||
| #define DISABLE 0 | ||
|
|
||
| #define SETUP_PKT_SIZE sizeof(struct usb_setup_packet) | ||
|
|
||
| #define EP(addr) USB_EP_GET_IDX(addr) |
There was a problem hiding this comment.
I do not see any benefits having these macros, please remove them and use 1, 0, true, false, sizeof(struct usb_setup_packet), sizeof(*setup).
There was a problem hiding this comment.
Fixed.
| #define DMA(addr) (EP(addr) - 1) | ||
| #define MAX_DMA_LEN ((UDPHS_DMACONTROL_BUFF_LENGTH_Msk >> UDPHS_DMACONTROL_BUFF_LENGTH_Pos) + 1) | ||
| #define BYTE_COUNT(x) (((x) & UDPHS_EPTSTA_BYTE_COUNT_Msk) >> UDPHS_EPTSTA_BYTE_COUNT_Pos) | ||
| #define BUSY_BANKS(x) (((x) & UDPHS_EPTSTA_BUSY_BANK_STA_Msk) >> UDPHS_EPTSTA_BUSY_BANK_STA_Pos) | ||
| #define BUFF_COUNT(x) (((x) & UDPHS_DMASTATUS_BUFF_COUNT_Msk) >> UDPHS_DMASTATUS_BUFF_COUNT_Pos) |
There was a problem hiding this comment.
Prefix them with UDC_SAM, for those that are too long, please use following style
#define UDC_SAM_MAX_DMA_LEN \
((UDPHS_DMACONTROL_BUFF_LENGTH_Msk >> UDPHS_DMACONTROL_BUFF_LENGTH_Pos) + 1)There was a problem hiding this comment.
Fixed.
| ept->UDPHS_EPTCTLENB = UDPHS_EPTCTLENB_RX_SETUP_Msk; | ||
| } | ||
|
|
||
| static void udphs_ep_fifo_in(udphs_ept_registers_t *const ept, int enable) |
There was a problem hiding this comment.
Fixed.
| } | ||
| } | ||
|
|
||
| static void udphs_ep_fifo_out(udphs_ept_registers_t *const ept, int enable) |
There was a problem hiding this comment.
Fixed.
| static void udphs_ep_set_halt(udphs_ept_registers_t *const ept) | ||
| { | ||
| ept->UDPHS_EPTSETSTA = UDPHS_EPTSETSTA_FRCESTALL_Msk; | ||
| } | ||
|
|
||
| static void udphs_ep_clear_halt(udphs_ept_registers_t *const ept) | ||
| { | ||
| ept->UDPHS_EPTCLRSTA = UDPHS_EPTCLRSTA_TOGGLESQ_Msk | | ||
| UDPHS_EPTCLRSTA_FRCESTALL_Msk; | ||
| } |
There was a problem hiding this comment.
move to udc_sam_ep_set_halt/udc_sam_ep_clear_halt
There was a problem hiding this comment.
Please may I keep these lines in udphs_ function?
The idea is put all register accessing related codes in the udphs_ prefixed function, and the udphs_ functions works like HAL interface function.
| udc_ep_buf_has_zlp(buf) ? "zlp" : ""); | ||
|
|
||
| if (buf->len == 0) { | ||
| udc_ep_buf_set_zlp(buf); |
There was a problem hiding this comment.
Why? Drivers should not use it.
There was a problem hiding this comment.
I use "bi->zlp" to notify handle_ep_irq() to send a ZLP when an IN packet with "buf->len == 0" comes up.
(I don't anticipate that "buf->len == 0" and "bi->zlp == true" will come up at the same time in an IN packet.)
Or should I create a new variable to use for the notification?
There was a problem hiding this comment.
I did not get it, why should a driver do something like that? There is nothing wrong with zero length IN transfer, ZLP flag is set by the stack in special cases like for control transfer handling.
There was a problem hiding this comment.
Yes, it's weird to set the ZLP flag in a UDC controller driver, I have remove these code, and added the xfer_zero flag for internal use in the driver.
| return 0; | ||
| } | ||
|
|
||
| static void udc_ep0_internal(const struct device *dev, int enable) |
There was a problem hiding this comment.
static void udc_ep0_internal(const struct device *dev, const bool enable)
There was a problem hiding this comment.
Fixed.
Hi Gerson, For example, this is the test result of FIFO transfer: I got two errors (test 14 and 21) in the test. Many thanks. |
|
Hi Xing,
|
9ca0f9a to
e0daa34
Compare
Hi Gerson, Many thanks for your comments.
Now I passed all the tests for FS and HS, here are the logs: (.venv) user@202:~/prj/zephyr/zephyr$ sudo ~/repo/linux/tools/usb/testusb -v 512 -D /dev/bus/usb/001/005 Please help check. Many thanks. |
nandojve
left a comment
There was a problem hiding this comment.
Is there anything open from your side?
| clocks = <&pmc PMC_TYPE_PERIPHERAL 105>, <&usb_clk>; | ||
| clock-names = "pclk", "hclk"; | ||
| maximum-speed = "high-speed"; | ||
| num-bidir-endpoints = <16>; |
There was a problem hiding this comment.
If the number of endpoints always 16, then do not use num-bidir-endpoints property and do not include usb-ep.yaml
There was a problem hiding this comment.
This cannot be guaranteed (it is not always 16); different IP instances on the same chip may have different EP values.
I have add num_of_eps variable in udc_sam_config for taking over the "num-bidir-endpoints" from the DTS.
| .lock = udc_sam_lock, | ||
| .unlock = udc_sam_unlock, | ||
| .device_speed = udc_sam_device_speed, | ||
| .init = udc_sam_init, | ||
| .enable = udc_sam_enable, | ||
| .disable = udc_sam_disable, | ||
| .shutdown = udc_sam_shutdown, | ||
| .set_address = udc_sam_set_address, | ||
| .host_wakeup = udc_sam_host_wakeup, | ||
| .ep_enable = udc_sam_ep_enable, | ||
| .ep_disable = udc_sam_ep_disable, | ||
| .ep_set_halt = udc_sam_ep_set_halt, | ||
| .ep_clear_halt = udc_sam_ep_clear_halt, | ||
| .ep_enqueue = udc_sam_ep_enqueue, | ||
| .ep_dequeue = udc_sam_ep_dequeue, | ||
| }; |
There was a problem hiding this comment.
test_mode implementation is missed, 4e90b4b#r2560534330
There was a problem hiding this comment.
test_mode function has been implemented.
| udphs->UDPHS_CTRL &= ~UDPHS_CTRL_DEV_ADDR_Msk; | ||
| if (addr != 0) { | ||
| udphs->UDPHS_CTRL |= UDPHS_CTRL_FADDR_EN_Msk | | ||
| UDPHS_CTRL_DEV_ADDR(addr); | ||
| } |
|
|
||
| static void udphs_send_wakeup(udphs_registers_t *const udphs) | ||
| { | ||
| udphs->UDPHS_CTRL |= UDPHS_CTRL_REWAKEUP_Msk; |
| { | ||
| udphs_ept_registers_t *const ept = &udphs->UDPHS_EPT[0]; | ||
|
|
||
| ept->UDPHS_EPTCTLENB = UDPHS_EPTCTLENB_RX_SETUP_Msk; |
|
|
||
| udc_ep0_internal(dev, 1); | ||
| udc_submit_event(dev, UDC_EVT_RESET, 0); | ||
| } |
There was a problem hiding this comment.
SOF interrupts are missed.
There was a problem hiding this comment.
SOF event report function has been added.
| data->caps.hs = true; | ||
| } | ||
|
|
||
| for (i = 0; i < UDPHS_EPT_NUMBER / 2; i++) { |
There was a problem hiding this comment.
How I understood, there are 16 "hw endpoint blocks".
Each of these "hw" endpoints can be IN or OUT?
Or are there 8 dedicated IN endpoints and 8 dedicated OUT endpoints?
Or are there 16 "hw" endpoints that can handle up to 16 IN and 16 OUT endpoints?
There was a problem hiding this comment.
Yes, I DO want to discuss about these weird codes.
There are 16 EPs in an UDPHS instance:
EP0 --> control endpoint, Bidirectional (IN and OUT)
EP1 ~ EP15 --> data endpoint, Unidirectional (IN or OUT), must be configured as IN or OUT before using.
(May I say that the EP0 is full-duplex and EP1 ~ EP15 is half-duplex?)
For EP1 ~ EP15, once an EP has been configured as IN, then it can not be used as OUT at the same time, the reverse is also true.
So I have to register these EPs in this way.
Please what does the "num-bidir-endpoints" mean in the usb-ep.yaml?
Does it mean the number of bidirectional EPs?
Or it means the number of EPs which can be configured as IN or OUT?
Many thanks.
| } | ||
| } | ||
|
|
||
| if (udc_ctrl_stage_is_setup(dev)) { |
There was a problem hiding this comment.
That does not seem to be right. Why do you need to always check it?
There was a problem hiding this comment.
In the last commit, the Setup package cannot be processed in time, so I disable the Setup package interrupt until the current one is finished.
With this rebase, issue cannot be reproduced anymore, and this code has been removed.
| } | ||
| } | ||
|
|
||
| if ((udc_ctrl_stage_is_setup(dev)) && (buf->data)) { |
There was a problem hiding this comment.
caps.out_ack has bee set to "true" in this UDC driver, in the last commit, the udc_ctrl_update_stage() won't help to free the allocated buffer, then I have to free it manually.
With this rebase, issue cannot be reproduced anymore, and this code has been removed.
| return udc_submit_ep_event(dev, buf, 0); | ||
| } | ||
|
|
||
| bool udc_ctrl_stage_is_setup(const struct device *dev) |
There was a problem hiding this comment.
I looked how you used it in the driver and it looks a bit fishy. I doubt this function is needed. You could move it to your driver for now. This part is likely to be reworked anyway.
There was a problem hiding this comment.
This code has been removed.
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time. |
Update mmu region for udphsa and udphsb, add usb phy clock configuration Signed-off-by: CHEN Xing <xing.chen@microchip.com>
be8eb81 to
cad6a5e
Compare
cad6a5e to
98ff77b
Compare
|
Hi @jfischer-no, Thank you very much for the very good reference driver udc_sam0.c, it has greatly accelerated our progress. |
nandojve
left a comment
There was a problem hiding this comment.
Is the the same UDPHS from Arduino Due?
If that is the case, could you add the entries for that board too?
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
|
|
||
| description: Microchip SAM UDPHS USB Device High Speed Port |
There was a problem hiding this comment.
Add examples entry:
zephyr/dts/bindings/usb/atmel,sam-udp.yaml
Lines 21 to 40 in 7bbe9da
There was a problem hiding this comment.
Example for dts file has been added.
Add udphsa udphsb udc devices to sama7g5 Signed-off-by: CHEN Xing <xing.chen@microchip.com>
Add driver for sama7g5 UDPHS Signed-off-by: CHEN Xing <xing.chen@microchip.com>
Add udphs udc devices and enable udphsa Signed-off-by: CHEN Xing <xing.chen@microchip.com>
98ff77b to
0f08eb4
Compare
Please correct me if there is issue. |
|



Add driver support for Sama7g5 USB UDC (UDPHS).
Many thanks.
Xing.