soc: dts: drivers: add SoCs for amebadplus series#78036
soc: dts: drivers: add SoCs for amebadplus series#78036fabiobaltieri merged 8 commits intozephyrproject-rtos:mainfrom
Conversation
|
Hello @zjian-zhang, and thank you very much for your first pull request to the Zephyr project! |
|
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 added project Note: This message is automatically posted and updated by the Manifest GitHub Action. |
zephyrbot
requested review from
Kludentwo,
anangl,
dcpleung,
decsny,
gmarull,
henrikbrixandersen,
jfischer-no,
jukkar,
krish2718,
nordic-krch,
rlubos,
tbursztyka,
teburd and
tmon-nordic
nordicjm
left a comment
nordicjm
left a comment
There was a problem hiding this comment.
Please read and follow the contribution guidelines https://docs.zephyrproject.org/latest/contribute/guidelines.html#contribute-guidelines we do not shove everything into a single commit for multiple areas, they must be distinct
There was a problem hiding this comment.
Comments should be e.g. # Enable UART
|
|
This pull request has been marked as stale because it has been open (more than) 60 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 14 days. Note, that you can always re-open a closed pull request at any time. |
|
teburd
left a comment
teburd
left a comment
There was a problem hiding this comment.
Looked it over, many things beyond my area of maintainership so I'm going to reassign, only one comment from me.
| @@ -0,0 +1,88 @@ | |||
| # Copyright (c) 2024 Realtek Semiconductor Corp. | |||
There was a problem hiding this comment.
Lots of 2024 copyrights here, not a big deal to me but it is 2025
There was a problem hiding this comment.
That's a fair observation! The 2024 reflects when the project was initially created or files were generated. While some projects update yearly and you'll see newly added files dated 2025, others keep the original year. But I appreciate the attention to detail!
| maintainers: | ||
| - Amb-zephyr | ||
| collaborators: | ||
| - zjian-zhang | ||
| - Derek-RTK |
There was a problem hiding this comment.
hey, these have to be users, drop Amb-zephyr just move the two username as maintainers
There was a problem hiding this comment.
I've already removed Amb-zephyr and kept only the two actual usernames as maintainers.
However, the PR is currently failing the PR Metadata Check / Prevent Merging due to the DNM (manifest) label. The same issue is also present in #91933.
| #include "os_wrapper_semaphore.h" | ||
| #include "os_wrapper_critical.h" | ||
| #include "os_wrapper_memory.h" | ||
| #include "os_wrapper_mutex.h" | ||
| #include "os_wrapper_queue.h" | ||
| #include "os_wrapper_semaphore.h" | ||
| #include "os_wrapper_task.h" | ||
| #include "os_wrapper_time.h" | ||
| #include "os_wrapper_timer.h" |
There was a problem hiding this comment.
hey just realized this is the same as f2f446c, would it be possible to move with one PR at a time, it's all very confusing, and also try to dedup as much as possible
my comments there apply here too I guess
There was a problem hiding this comment.
what's this stuff for? I checked a few functions declared in here, there's no references either in the rest of the patches or the hal, should they be left out for now? if they are for supporting code in the hal then I think they should sit somewhere else, not in soc/
To avoid confusion, I’ve already converted PR #91933 into a draft and incorporated all your feedback. Once this PR is merged, there will be no duplicate code.
The functions in question are intended to support HAL code. Currently, I’ve only retained the parts that are actually needed. As noted earlier in issue #78084:
For reference: Zephyr glue code is NOT allowed in hal repos, otherwise any change in Zephyr can become a multirepo nightmare.
I have move the os wrapper tozephyr/soc/realtek/ameba, am i right? our driver need some service from OS, and we implement the glue code to support multi OS.
If this code doesn’t belong under soc/, do you have a suggestion for a more appropriate location? Alternatively, would it be acceptable to keep it there temporarily and revisit its placement once we implement the actual function definitions?
There was a problem hiding this comment.
By the way, Can the DNM (manifest) label be removed now? The PR is currently failing the PR Metadata Check / Prevent Merging due to the DNM (manifest) label.
There was a problem hiding this comment.
None of this is acceptable, anywhere. Your HAL is for the hardware, it should not be making threads or anything like that, if an application wants to make a thread, it will call the zephyr thread functions, it will not call some odd HAL-specific function which does something and redirects it
There was a problem hiding this comment.
By the way, Can the DNM (manifest) label be removed now? The PR is currently failing the
PR Metadata Check / Prevent Mergingdue to the DNM (manifest) label.
The workflow adds it with every push because it detects a new module, just ignore it, we'll remove it before merging once it's ready, we can remove it now but then it'd just be added again on the next push.
There was a problem hiding this comment.
You're absolutely right that the HAL should focus on hardware abstraction and avoid introducing OS-specific threading or scheduling logic.
These header files aren’t actually used in the current code, so I’ve gone ahead and removed them entirely.
The original intent behind the os_wrapper_* layer was to provide a thin, unified abstraction over basic OS services (like semaphores, mutexes, malloc, etc.) to make our HAL portable across different RTOSes. However, as you pointed out, this doesn’t belong in the HAL if it’s not strictly needed — and certainly not in soc/.
That said, upcoming Wi-Fi HAL changes will require more direct use of OS primitives (e.g., memory allocation, synchronization, etc). When that time comes, we’ll carefully evaluate where such wrappers should live. Based on existing patterns in Zephyr, there seem to be three common approaches:
- Inside
modules/, for example:modules/canopennode/CO_driver.c(e.g., canopen_send_lock/unlock)modules/hal_nordic/nrfx/nrfx_glue.c(nrfx_busy_wait)modules/nrf_wifi/os/shim.c(zep_shim_mem_zalloc)modules/hal_infineon/.../cyabs_rtos_zephyr.cmodules/hal_afbr/platform_malloc.c(Argus_Malloc/Free)modules/lvgl/lvgl_zephyr_osal.c(e.g., lv_mutex_lock)
- Inside
soc/– as seen with STM32’sstm32_timer.cusingUTIL_TIMER_*APIs. - Using Zephyr’s portability layer, such as
subsys/portability/cmsis_rtos_v2/, though adopting this would require significant rework of our existing HAL.
We’ll follow established Zephyr conventions when reintroducing any OS-dependent code—likely placing it under zephyr/modules/ or leveraging Zephyr’s native APIs directly, rather than adding HAL-specific wrappers in soc/.
For now, all unused includes have been cleaned up. Could you please take another look? @fabiobaltieri @nordicjm
drivers/gpio/Kconfig
Outdated
| source "drivers/gpio/Kconfig.xmc4xxx" | ||
| # zephyr-keep-sorted-stop | ||
|
|
||
| source "drivers/gpio/Kconfig.ameba" |
| else() | ||
| message(WARNING "Missing Realtek blobs. Run: `west blobs fetch hal_realtek`") |
There was a problem hiding this comment.
| else() | |
| message(WARNING "Missing Realtek blobs. Run: `west blobs fetch hal_realtek`") |
Does this build and run fully on a board without any blobs at all?
There was a problem hiding this comment.
It can be fully built without any binary blobs, but since we have another CPU involved, running it requires binaries from those blobs.
There was a problem hiding this comment.
OK then your SoC and board are not acceptable to be added to zephyr, please check https://docs.zephyrproject.org/latest/contribute/bin_blobs.html#limited-scope for acceptance pass criteria
There was a problem hiding this comment.
OK please be very clear about that in future. And this warning needs removing
| zephyr_linker_sources(SECTIONS boot_section.ld) | ||
| set(SOC_LINKER_SCRIPT ${ZEPHYR_BASE}/include/zephyr/arch/arm/cortex_m/scripts/linker.ld CACHE INTERNAL "") | ||
|
|
||
| set(PYTHON_SCRIPT_ARGS |
There was a problem hiding this comment.
in cmake UPPER CASE means GLOBAL, and lower case means local, fix, and move this inside of the if()
| typedef void *rtos_sema_t; | ||
| int rtos_sema_create_binary(rtos_sema_t *pp_handle); | ||
| int rtos_sema_give(rtos_sema_t p_handle); | ||
| int rtos_sema_take(rtos_sema_t p_handle, uint32_t timeout_ms); | ||
|
|
||
| /** | ||
| * @brief General macro definition | ||
| */ | ||
| #define RTOS_MAX_DELAY 0xFFFFFFFFUL | ||
| #define RTOS_MAX_TIMEOUT 0xFFFFFFFFUL | ||
| #define RTOS_TICK_RATE_HZ 1000UL |
There was a problem hiding this comment.
| typedef void *rtos_sema_t; | |
| int rtos_sema_create_binary(rtos_sema_t *pp_handle); | |
| int rtos_sema_give(rtos_sema_t p_handle); | |
| int rtos_sema_take(rtos_sema_t p_handle, uint32_t timeout_ms); | |
| /** | |
| * @brief General macro definition | |
| */ | |
| #define RTOS_MAX_DELAY 0xFFFFFFFFUL | |
| #define RTOS_MAX_TIMEOUT 0xFFFFFFFFUL | |
| #define RTOS_TICK_RATE_HZ 1000UL |
| #include <string.h> | ||
| #include <stdint.h> | ||
|
|
||
| #include "platform_autoconf.h" |
There was a problem hiding this comment.
As mentioned in #78036 (comment), I have currently retained only the following HAL-layer functions that are strictly necessary; otherwise, the build would fail:
typedef void *rtos_sema_t;
#define rtos_sema_create_binary(pp_handle)
#define rtos_sema_give(p_handle)
#define rtos_sema_take(p_handle, timeout_ms) 0
#define RTOS_TICK_RATE_HZ 1000UL
There was a problem hiding this comment.
why would the build fail? There is no reference to these in this PR or the hal PR
There was a problem hiding this comment.
I have already removed os_wrapper.h. However, this requires corresponding changes in hal_realtek; see zephyrproject-rtos/hal_realtek#1, Since maintainers for hal_realtek have not yet been added (which will only happen after this PR is approved), I kindly request administrator assistance to help merge that change into the hal_realtek repository.
In the meantime, since the driver code in the current PR does not actually use any functionality from os_wrapper, I have simply commented out its compilation in the CMakeLists.txt file, as mentioned in the earlier comment: #78036 (comment), We will revisit and properly address how to implement this functionality in a future update.
There was a problem hiding this comment.
you need to make the changes now and an admin will merge them to the repo, before this PR will be approved
There was a problem hiding this comment.
Thanks! The required changes have been submitted in zephyrproject-rtos/hal_realtek#1.
This PR depends on that change being merged into hal_realtek first. Could you please help merge PR #1? Thank you! @fabiobaltieri @nashif
There was a problem hiding this comment.
Done. And zephyrproject-rtos/hal_realtek#1 has been merged.
There was a problem hiding this comment.
SoC does not work without binary blobs as per #78036 (comment) therefore SoC is not acceptable in zephyr clarified below
Perhaps my previous description wasn't clear enough. As stated in boards/realtek/rtl872xda_evb/doc/index.rst, this SoC contains two distinct CPU cores:
The Real-M300 core runs Zephyr OS and can be fully built and executed without requiring any binary blobs at all. The Real-M200 core, however, is not intended to run Zephyr OS; instead, it is provided as a precompiled binary (bin file). Of course, you can choose not to run the Real-M200 core—but in that case, certain low-power features that depend on it will not be available. |
| else() | ||
| message(WARNING "Missing Realtek blobs. Run: `west blobs fetch hal_realtek`") |
There was a problem hiding this comment.
OK please be very clear about that in future. And this warning needs removing
| #define __OS_WRAPPER_H__ | ||
|
|
||
| typedef void *rtos_sema_t; | ||
| #define rtos_sema_create_binary(pp_handle) |
drivers/gpio/Kconfig
Outdated
| source "drivers/gpio/Kconfig.xmc4xxx" | ||
| # zephyr-keep-sorted-stop | ||
|
|
||
|
|
Realtek HAL (Hardware Abstraction Layer) provides a low level peripheral configuration function. Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>
add initial version of devicetree for amebadplus SOC. amebadplus devicetree file is main platform dtsi file, which should be included from board dts (e.g rtl872xda_evb.dts) Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>
Add initial version of Amebadplus Soc integration Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>
add amebadplus pin controller driver Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>
loguart driver for amebadplus Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>
GPIO driver for amebadplus Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>
add initial version of rtl872xda_evb board Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>
This commit adds maintainers for hal_realtek repository Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>
The earlier feedback on the first commit has been addressed. Thanks! |
Sorry for the confusion — I’ve already addressed all the feedback from the first commit in the latest changes, but I forgot to reply to the individual comments. |
|
|
@nordicjm @stephanosio @jfischer-no Hi! I’ve addressed all your feedback—could you please take another look when you have a moment? |
|
Hi @zjian-zhang! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
11 participants
The amebadplus microcontroller (MCU) is an advanced system-on-chip (SoC) featuring an Arm® Cortex®-M55 and Cortex®-M23, large Flash and SRAM memories, and supports dual-band Wi-Fi 4(2.4GHz + 5GHz) and BLE5.0 specifications.
The initial support for the board includes GPIO, PINCTRL and Loguart(serial).