bsp: [gd32] add GD32VW553 hw timer support. #11157
Conversation
|
pomin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
There was a problem hiding this comment.
Pull request overview
This pull request adds hardware timer (HWTIMER) driver support for the GD32VW553 RISC-V BSP. The implementation provides a standard RT-Thread hardware timer device driver interface for timers 0, 1, 2, 5, 15, and 16.
Changes:
- Adds new hardware timer driver implementation (drv_hwtimer.c/h) with timer initialization, start/stop operations, and interrupt handlers
- Adds timer configuration header (hwtimer_config.h) defining timer parameters and hardware mappings
- Updates SConscript to include timer driver compilation and config directory path
- Adds Kconfig options to enable/disable individual hardware timers
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| bsp/gd32/risc-v/libraries/gd32_drivers/drv_hwtimer.h | Header file defining timer data structures and index enumeration |
| bsp/gd32/risc-v/libraries/gd32_drivers/drv_hwtimer.c | Main driver implementation with timer operations and interrupt handlers |
| bsp/gd32/risc-v/libraries/gd32_drivers/config/hwtimer_config.h | Configuration macros for each supported timer instance |
| bsp/gd32/risc-v/libraries/gd32_drivers/SConscript | Build script updated to include timer driver and config path |
| bsp/gd32/risc-v/gd32vw553h-eval/board/Kconfig | Configuration options for enabling hardware timers |
| #ifdef BSP_USING_HWTIMER6 | ||
| void TIMER6_IRQHandler(void) | ||
| { | ||
| rt_interrupt_enter(); | ||
| rt_device_hwtimer_isr(&g_gd32_hwtimer[TIM6_INDEX].hwtimer_dev); | ||
| timer_flag_clear((uint32_t)g_gd32_hwtimer[TIM6_INDEX].hwtimer_dev.parent.user_data, \ | ||
| TIMER_INT_UP); | ||
| rt_interrupt_leave(); | ||
| } | ||
| #endif | ||
|
|
||
| #ifdef BSP_USING_HWTIMER7 | ||
| void TIMER7_UP_IRQHandler(void) | ||
| { | ||
| rt_interrupt_enter(); | ||
| rt_device_hwtimer_isr(&g_gd32_hwtimer[TIM7_INDEX].hwtimer_dev); | ||
| timer_flag_clear((uint32_t)g_gd32_hwtimer[TIM7_INDEX].hwtimer_dev.parent.user_data, \ | ||
| TIMER_INT_UP); | ||
| rt_interrupt_leave(); | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
These interrupt handlers (TIMER3_IRQHandler, TIMER4_IRQHandler, TIMER6_IRQHandler, TIMER7_UP_IRQHandler) are defined in the code but:
- They reference TIM3_INDEX, TIM4_INDEX, TIM6_INDEX, TIM7_INDEX which are not defined in drv_hwtimer.h enum
- There are no corresponding BSP_USING_HWTIMER3/4/6/7 guards in drv_hwtimer.c g_gd32_hwtimer array
- There are no HWTIMER3/4/6/7_CONFIG macros in hwtimer_config.h
- There are no configuration options in Kconfig for these timers
This creates dead code that will never be compiled or used. Either remove these unused interrupt handlers or add the complete support (enum entries, config macros, Kconfig options) for TIMER3, TIMER4, TIMER6, and TIMER7.
这些中断处理程序（TIMER3_IRQHandler、TIMER4_IRQHandler、TIMER6_IRQHandler、TIMER7_UP_IRQHandler）在代码中定义，但是：
- 它们引用的 TIM3_INDEX、TIM4_INDEX、TIM6_INDEX、TIM7_INDEX 未在 drv_hwtimer.h 枚举中定义
- 在 drv_hwtimer.c 的 g_gd32_hwtimer 数组中没有对应的 BSP_USING_HWTIMER3/4/6/7 保护
- 在 hwtimer_config.h 中没有 HWTIMER3/4/6/7_CONFIG 宏
- 在 Kconfig 中没有这些定时器的配置选项
这会创建永远不会被编译或使用的死代码。请删除这些未使用的中断处理程序，或者为 TIMER3、TIMER4、TIMER6 和 TIMER7 添加完整的支持（枚举条目、配置宏、Kconfig 选项）。
| #ifdef BSP_USING_HWTIMER6 | |
| void TIMER6_IRQHandler(void) | |
| { | |
| rt_interrupt_enter(); | |
| rt_device_hwtimer_isr(&g_gd32_hwtimer[TIM6_INDEX].hwtimer_dev); | |
| timer_flag_clear((uint32_t)g_gd32_hwtimer[TIM6_INDEX].hwtimer_dev.parent.user_data, \ | |
| TIMER_INT_UP); | |
| rt_interrupt_leave(); | |
| } | |
| #endif | |
| #ifdef BSP_USING_HWTIMER7 | |
| void TIMER7_UP_IRQHandler(void) | |
| { | |
| rt_interrupt_enter(); | |
| rt_device_hwtimer_isr(&g_gd32_hwtimer[TIM7_INDEX].hwtimer_dev); | |
| timer_flag_clear((uint32_t)g_gd32_hwtimer[TIM7_INDEX].hwtimer_dev.parent.user_data, \ | |
| TIMER_INT_UP); | |
| rt_interrupt_leave(); | |
| } | |
| #endif |
| /* whether should frequency doubling */ | ||
| temp = (temp < 4) ? 0 : 1; | ||
|
|
||
| prescaler = (ap2freq << temp) / freq - 1; |
There was a problem hiding this comment.
Potential division by zero vulnerability. If freq parameter is 0, this will cause a division by zero error. Add validation to check that freq is not zero before performing the division.
潜在的除零漏洞。如果 freq 参数为 0，这将导致除零错误。在执行除法之前添加验证以检查 freq 不为零。
| static rt_err_t gd32_hwtimer_control(struct rt_hwtimer_device *timer, rt_uint32_t cmd, \ | ||
| void *args) | ||
| { | ||
| int ret = RT_EOK; | ||
| rt_int32_t freq; | ||
| rt_hwtimer_mode_t mode; | ||
|
|
||
| switch (cmd) | ||
| { | ||
| case HWTIMER_CTRL_FREQ_SET: | ||
| freq = *(rt_uint32_t *)args; | ||
| __set_timerx_freq((uint32_t)timer->parent.user_data, freq); | ||
| break; | ||
| default: | ||
| rt_kprintf("invalid cmd:%x\n", cmd); | ||
| ret = -RT_EINVAL; | ||
| break; | ||
| } | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
The control function only handles HWTIMER_CTRL_FREQ_SET but does not handle other standard control commands defined in the hwtimer framework:
- HWTIMER_CTRL_STOP (stop timer)
- HWTIMER_CTRL_INFO_GET (get timer feature information)
- HWTIMER_CTRL_MODE_SET (setting the timing mode)
Consider implementing these control commands for completeness, or at least handle HWTIMER_CTRL_INFO_GET which could return the timer info structure, as this is commonly used by applications to query timer capabilities.
控制函数只处理 HWTIMER_CTRL_FREQ_SET，但不处理 hwtimer 框架中定义的其他标准控制命令：
- HWTIMER_CTRL_STOP（停止定时器）
- HWTIMER_CTRL_INFO_GET（获取定时器功能信息）
- HWTIMER_CTRL_MODE_SET（设置定时模式）
考虑实现这些控制命令以保证完整性，或至少处理 HWTIMER_CTRL_INFO_GET，它可以返回定时器信息结构，因为应用程序通常使用它来查询定时器功能。
| { | ||
| int ret = RT_EOK; | ||
| rt_int32_t freq; | ||
| rt_hwtimer_mode_t mode; |
There was a problem hiding this comment.
The variable mode is declared but never used in the function. This will generate a compiler warning. Remove this unused variable declaration.
变量 mode 已声明但从未在函数中使用。这将产生编译器警告。删除此未使用的变量声明。
| rt_hwtimer_mode_t mode; |
| @@ -0,0 +1,274 @@ | |||
| /* | |||
There was a problem hiding this comment.
PR title format issue: The title "bsp: [gd32] add GD32VW553 hw timer support." does not follow the required format. Based on RT-Thread coding guidelines and the modified files, the title should follow the pattern [module][subsystem] Description in lowercase.
Suggested title: [gd32][drivers] Add GD32VW553 hardware timer support or [gd32vw553h-eval][hwtimer] Add hardware timer driver support
PR 标题格式问题：标题 "bsp: [gd32] add GD32VW553 hw timer support." 不符合要求的格式。根据 RT-Thread 编码指南和修改的文件，标题应遵循小写的 [模块][子系统] 描述 格式。
建议标题：[gd32][drivers] Add GD32VW553 hardware timer support 或 [gd32vw553h-eval][hwtimer] Add hardware timer driver support
| timer_flag_clear((uint32_t)g_gd32_hwtimer[TIM0_INDEX].hwtimer_dev.parent.user_data, \ | ||
| TIMER_INT_UP); |
There was a problem hiding this comment.
The line continuation backslash is inconsistent with the rest of the function calls in the codebase. In this file, timer_flag_clear should not need line continuation as it fits within reasonable line length. Consider removing the line continuation for better readability:
timer_flag_clear((uint32_t)g_gd32_hwtimer[TIM0_INDEX].hwtimer_dev.parent.user_data, TIMER_INT_UP);行继续反斜杠与代码库中其余函数调用不一致。在此文件中，timer_flag_clear 不需要行继续，因为它在合理的行长度内。为了更好的可读性，请考虑删除行继续。
| timer_flag_clear((uint32_t)g_gd32_hwtimer[TIM1_INDEX].hwtimer_dev.parent.user_data, \ | ||
| TIMER_INT_UP); |
There was a problem hiding this comment.
The line continuation backslash is inconsistent with the rest of the function calls in the codebase. In this file, timer_flag_clear should not need line continuation as it fits within reasonable line length. Consider removing the line continuation for better readability. This issue appears in multiple interrupt handlers (lines 176-177, 187-188, 198-199, 209-210, 220-221, 231-232, 242-243).
行继续反斜杠与代码库中其余函数调用不一致。在此文件中，timer_flag_clear 不需要行继续，因为它在合理的行长度内。为了更好的可读性，请考虑删除行继续。此问题出现在多个中断处理程序中（第176-177、187-188、198-199、209-210、220-221、231-232、242-243行）。
| /* whether should frequency doubling */ | ||
| temp = (temp < 4) ? 0 : 1; | ||
|
|
||
| prescaler = (ap1freq << temp) / freq - 1; |
There was a problem hiding this comment.
Potential division by zero vulnerability. If freq parameter is 0, this will cause a division by zero error. Add validation to check that freq is not zero before performing the division. This is the same issue as line 40.
潜在的除零漏洞。如果 freq 参数为 0，这将导致除零错误。在执行除法之前添加验证以检查 freq 不为零。这与第40行是同样的问题。
| @@ -0,0 +1,274 @@ | |||
| /* | |||
| * Copyright (c) 2006-2022, RT-Thread Development Team | |||
There was a problem hiding this comment.
Copyright year inconsistency: This file uses "2006-2022" while drv_hwtimer.h uses "2006-2025". For new files created in 2025/2026, the copyright should consistently use "2006-2025" or the current year range.
版权年份不一致：此文件使用"2006-2022"，而 drv_hwtimer.h 使用"2006-2025"。对于 2025/2026 年创建的新文件，版权应一致使用"2006-2025"或当前年份范围。
| * Copyright (c) 2006-2022, RT-Thread Development Team | |
| * Copyright (c) 2006-2025, RT-Thread Development Team |
| timer_flag_clear((uint32_t)g_gd32_hwtimer[TIM0_INDEX].hwtimer_dev.parent.user_data, \ | ||
| TIMER_INT_UP); |
There was a problem hiding this comment.
The interrupt flag being cleared is TIMER_INT_UP, but this appears to be an interrupt enable flag, not an interrupt status flag. According to GD32 timer peripheral conventions, the correct flag to clear should be TIMER_FLAG_UP or similar status flag. Using TIMER_INT_UP may not properly clear the interrupt status, potentially causing repeated interrupts or incorrect behavior.
Please verify the correct flag name for clearing timer update interrupt status in the GD32VW553 HAL library.
清除的中断标志是 TIMER_INT_UP，但这似乎是中断使能标志，而不是中断状态标志。根据 GD32 定时器外设约定，清除的正确标志应该是 TIMER_FLAG_UP 或类似的状态标志。使用 TIMER_INT_UP 可能无法正确清除中断状态，可能导致重复中断或不正确的行为。
请验证 GD32VW553 HAL 库中清除定时器更新中断状态的正确标志名称。
3 participants
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
add GD32VW553 hw timer support.
bsp/gd32/risc-v/gd32vw553h-eval TEST OK.
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up