Skip to content

Add support for UDC for Apollo510 family #89023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 26, 2025

Conversation

zeonchew
Copy link
Contributor

@zeonchew zeonchew commented Apr 24, 2025

This PR adds support for UDC for Ambiq Apollo510 family. The boards tested are:

  • apollo510_evb

Followings are the USB samples tested:

  • cdc_acm
  • console-next
  • mass
  • hid-keyboard (with CONFIG_PM_DEVICE=n)
  • hid-mouse (with CONFIG_PM_DEVICE=n)
  • webusb-next

Copy link

github-actions bot commented Apr 24, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_ambiq zephyrproject-rtos/hal_ambiq@f46941f zephyrproject-rtos/hal_ambiq@84ccbfc (main) zephyrproject-rtos/[email protected]

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for UDC for the Ambiq Apollo510 family and updates several USB, MSPI, and flash drivers along with DTS bindings to support new hardware features and improved functionality.

  • Updated DTS bindings for Ambiq MSPI devices and controllers
  • Modified USB device controller driver for cache handling and enhanced clock configuration
  • Updated flash and MSPI drivers to use new priority definitions and timing configuration macros

Reviewed Changes

Copilot reviewed 46 out of 57 changed files in this pull request and generated no comments.

Show a summary per file
File Description
dts/bindings/mspi/ambiq,mspi-device.yaml Updated default array length for timing-config property
dts/bindings/mspi/ambiq,mspi-controller.yaml Added new properties for MSPI controller configuration
drivers/usb/udc/udc_ambiq.c Enhanced cache management, type definitions, and clock handling
drivers/mspi/mspi_ambiq.h Removed unused structure field and added timing scan types
drivers/memc/memc_mspi_aps_z8.h Added support definitions for new memory controller features
drivers/memc/memc_mspi_aps6404l.c Updated timing configuration macros using COND_CODE_1
drivers/flash/flash_mspi_emul_device.c Updated transfer priority definitions
drivers/flash/flash_mspi_atxp032.c Switched to memcpy for JEDEC ID handling and adjusted timing macros
boards/ambiq/apollo510_evb/apollo510_evb.yaml Added mspi and usbd support to board configuration
Files not reviewed (11)
  • boards/ambiq/apollo510_evb/Kconfig.defconfig: Language not supported
  • boards/ambiq/apollo510_evb/apollo510_evb-pinctrl.dtsi: Language not supported
  • boards/ambiq/apollo510_evb/apollo510_evb.dts: Language not supported
  • drivers/flash/CMakeLists.txt: Language not supported
  • drivers/flash/Kconfig.mspi: Language not supported
  • drivers/memc/CMakeLists.txt: Language not supported
  • drivers/memc/Kconfig.mspi: Language not supported
  • drivers/mspi/CMakeLists.txt: Language not supported
  • drivers/mspi/Kconfig.ambiq: Language not supported
  • drivers/usb/udc/Kconfig.ambiq: Language not supported
  • dts/arm/ambiq/ambiq_apollo510.dtsi: Language not supported
Comments suppressed due to low confidence (3)

drivers/usb/udc/udc_ambiq.c:1027

  • The callback parameter type for 'xfer_len' was updated to use XFER_LEN_CB_TYPE; please verify that this type change is consistently used across all related USB transfer functions to avoid potential type mismatches.
uint8_t ep_addr, XFER_LEN_CB_TYPE xfer_len, am_hal_usb_xfer_code_e code, void *param)

dts/bindings/mspi/ambiq,mspi-device.yaml:51

  • The default timing configuration array length changed from 8 to 7 values; please verify that this change is consistent with the updated structure in mspi_ambiq_timing_cfg and does not break expected configurations.
default: [0, 0, 0, 0, 0, 0, 0]

drivers/mspi/mspi_ambiq.h:60

  • The field 'ui32RXDQSDelayEXT' has been removed from the mspi_ambiq_timing_cfg structure; please ensure that all related code and DTS bindings have been updated accordingly to reflect this change.
struct mspi_ambiq_timing_cfg {

@zeonchew zeonchew force-pushed the ambiq-ap510-udc-merge branch from fa4c752 to aa5d823 Compare April 30, 2025 03:31
@zeonchew zeonchew force-pushed the ambiq-ap510-udc-merge branch 2 times, most recently from 25817a8 to 2699e9d Compare April 30, 2025 03:49
@zeonchew zeonchew marked this pull request as ready for review April 30, 2025 05:19
@github-actions github-actions bot added platform: Ambiq Ambiq area: USB Universal Serial Bus labels Apr 30, 2025
@zeonchew zeonchew force-pushed the ambiq-ap510-udc-merge branch from 2699e9d to cc46305 Compare April 30, 2025 05:49
@zeonchew zeonchew force-pushed the ambiq-ap510-udc-merge branch from cc46305 to aee8d56 Compare April 30, 2025 06:57
@zeonchew zeonchew requested a review from swift-tk April 30, 2025 07:01
swift-tk
swift-tk previously approved these changes Apr 30, 2025
Comment on lines 54 to 68
config UDC_AMBIQ_PIO_MODE
bool "PIO transfer mode"
help
Select this config when cache coherency handling is to be
avoided.
endchoice

config UDC_AMBIQ_HANDLE_CACHE
bool "Cache handling enable for USB non-control endpoint buffers"
default y
depends on CACHE_MANAGEMENT
depends on DCACHE
depends on !UDC_BUF_FORCE_NOCACHE
depends on UDC_AMBIQ_DMA0_MODE || UDC_AMBIQ_DMA1_MODE
help
Disable this if non-cacheable memory is selected as default system memory
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary options, no need for NOCACHE if you can handle DCACHE on your platform/driver.

Copy link
Contributor Author

@zeonchew zeonchew May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no,
The option is to disable cache handling fully when they are not needed, saving CPU cycles used for the cache handling operations, when any of the depends on conditions above is false, and also when non-cacheable memory is selected as default system memory.
Since the cache handling is executed every time data is move from/to endpoint buffer, disabling it when it is not needed will improve the throughput for USB endpoints.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Either there is data cache and you driver has to handle it properly or there is no data cache enabled and all sys_cache_data_* will be optimized away.

Copy link
Contributor Author

@zeonchew zeonchew May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no, I think I understand what you are envisioning for the cache handling now. I have removed the cache handling option from this PR in the latest push.

Disable this if non-cacheable memory is selected as default system memory


config UDC_AMBIQ_DEB_ENABLE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you want to split this commit into several.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no, I have split the Double Endpoint Buffer change to a separate commit. Please have a look again.

Comment on lines 46 to 50
#if CONFIG_SOC_SERIES_APOLLO5X
#define XFER_LEN_CB_TYPE uint32_t
#else
#define XFER_LEN_CB_TYPE uint16_t
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use uint32_t as parameter or fix your HAL, or just write a native driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no, This part has been changed to use uint32_t as parameter.

@@ -542,10 +587,74 @@ static int udc_ambiq_init(const struct device *dev)
am_hal_usb_hardware_unreset();
/* Release USB PHY reset */
am_hal_usb_disable_phy_reset_override();

#if CONFIG_SOC_SERIES_APOLLO5X
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to init_apollo5x()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no, done moving to init_apollo5x() as suggested.

am_hal_usb_phy_clock_enable(priv->usb_handle, true, priv->usb_speed);
#endif

#if CONFIG_UDC_AMBIQ_DEB_ENABLE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to init_double_buffers()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jfischer-no, moved to init_double_buffers() as suggested. Thanks.

@zeonchew zeonchew force-pushed the ambiq-ap510-udc-merge branch 3 times, most recently from 7bd64c2 to 0058eb9 Compare May 6, 2025 07:41
@zeonchew zeonchew requested a review from jfischer-no May 6, 2025 07:52
@zeonchew zeonchew force-pushed the ambiq-ap510-udc-merge branch from 76e0cbb to 6eceb6a Compare May 28, 2025 10:38
@zeonchew zeonchew force-pushed the ambiq-ap510-udc-merge branch 4 times, most recently from 03518f0 to bd32849 Compare May 28, 2025 14:06
@zeonchew
Copy link
Contributor Author

Hi @jfischer-no , the comments have been addressed. Please help to re-review the changes.

There are some failures on twister, but upon checking, the 2 failures aren't related to this PR, my colleagues are working on fixing that issue right now.
I believe we could continue the review in parallel while my colleagues are fixing the CI issue.

Thank you.

@AlessandroLuo AlessandroLuo force-pushed the ambiq-ap510-udc-merge branch from bd32849 to 0670310 Compare June 17, 2025 05:47
@AlessandroLuo
Copy link
Collaborator

@jfischer-no please help to review again, thank you

AlessandroLuo
AlessandroLuo previously approved these changes Jun 18, 2025
zeonchew added 2 commits June 23, 2025 11:35
sync latest hal to take in implementaiton of unified usb isr handling

Signed-off-by: Chew Zeh Yang <[email protected]>
Add USB node to apollo510 qualifier, and apollo510_evb board to enable
USB support on the SoC and its EVB.

Signed-off-by: Chew Zeh Yang <[email protected]>
@zeonchew
Copy link
Contributor Author

Hi @jfischer-no, ping. Please help to review again. Thank you.

Copy link
Collaborator

@jfischer-no jfischer-no left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dkalowsk
Copy link
Collaborator

@zeonchew can you fix the stray change called out by @jfischer-no ? Once fixed happy to merge in.

zeonchew added 2 commits June 26, 2025 10:10
Added UDC support for Ambiq apollo510 family with USB HS and DMA support.

Signed-off-by: Chew Zeh Yang <[email protected]>
implements option to enable double endpoint buffer to improve throughtput
for non-control endpoints.

Signed-off-by: Chew Zeh Yang <[email protected]>
@zeonchew zeonchew force-pushed the ambiq-ap510-udc-merge branch from 877d42d to 2fb3d40 Compare June 26, 2025 02:12
Copy link

@zeonchew
Copy link
Contributor Author

zeonchew commented Jun 26, 2025

@zeonchew can you fix the stray change called out by @jfischer-no ? Once fixed happy to merge in.

Hi @dkalowsk, the stray change fixed in correct commit as pointed out. Thank you.

@kartben kartben merged commit 5e5c40c into zephyrproject-rtos:main Jun 26, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants