Conversation
|
请补充GCC的启动文件 |
|
请参考其它 补充GCC启动文件 |
|
gcc启动文件可以用MCU-GD32 Embedded Builder生成一份工程，里面会包含GCC的启动文件 |
WalkthroughAdds CMSIS-compliant GD32E51x support: new device headers and memory map, system clock implementation, MPU helpers, compiler CMSIS backends, toolchain-specific startup files (ARM/IAR/GCC), build integration, and Apache-2.0 license. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(220,235,255,0.4)
participant Reset as Reset_Handler
participant System as SystemInit
participant RCU as RCU/Clock HW
participant PLL as PLL
participant FLASH as Flash
participant APP as main/__libc_init_array
end
Reset->>System: call SystemInit()
System->>RCU: disable/reset clocks, enable oscillator (HXTAL/IRC)
RCU-->>System: oscillator ready / timeout
alt use PLL
System->>PLL: configure PLL source/multipliers/dividers
PLL-->>System: PLL lock / timeout
System->>FLASH: configure flash wait states
System->>RCU: switch SYSCLK to PLL
else direct oscillator
System->>RCU: switch SYSCLK to oscillator
end
System->>System: update SystemCoreClock variable
System->>APP: return to Reset_Handler -> continue startup (constructors/main)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (5)
GD32E51x/mpu_armv8.h (1)
97-98: Potential signed comparison issue inMAIR_ATTRmacro.The condition
x < 0is always false whenxis an unsigned type, which is the expected use case for attribute indices. Ifxis signed negative, the behavior may be unexpected.Consider simplifying to only check the upper bound:
-#define MAIR_ATTR(x) ((x > 7 || x < 0) ? 0 : x) +#define MAIR_ATTR(x) (((x) > 7) ? 0 : (x))Alternatively, cast to unsigned before comparison to avoid warnings with some compilers.
GD32E51x/cmsis_iccarm.h (2)
188-193: Minor: Double semicolon.There's a redundant semicolon at line 190 and line 212.
- *(__packed uint16_t*)(ptr) = val;; + *(__packed uint16_t*)(ptr) = val;
210-213: Minor: Double semicolon.Same redundant semicolon issue at line 212.
- *(__packed uint32_t*)(ptr) = val;; + *(__packed uint32_t*)(ptr) = val;GD32E51x/SConscript (1)
16-19: Consider supporting additional device variants.The script only includes
startup_gd32e51x_hd.s, butsystem_gd32e51x.csupports multiple GD32E51x variants (HD, CL, E518, PRTxxA). You may want to add conditional logic to select the appropriate startup file based on the target device variant (e.g., via anrtconfigoption).GD32E51x/GD/GD32E51x/Source/system_gd32e51x.c (1)
311-313: Consider adding timeouts to PLL stability waits.Multiple locations wait indefinitely for PLL/oscillator stability without timeout protection (e.g., lines 312-313, 385-386, 458-460, etc.). While infinite loops are common in embedded startup code where recovery isn't possible, adding timeouts with error handling could improve robustness for debugging clock issues.
This applies to all
while(0U == (RCU_CTL & RCU_CTL_PLLSTB))patterns and similar high-drive mode waits throughout the file.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
GD32E51x/GD/GD32E51x/Include/gd32e51x.h(1 hunks)GD32E51x/GD/GD32E51x/Include/system_gd32e51x.h(1 hunks)GD32E51x/GD/GD32E51x/Source/ARM/startup_gd32e51x_cl.s(1 hunks)GD32E51x/GD/GD32E51x/Source/ARM/startup_gd32e51x_hd.s(1 hunks)GD32E51x/GD/GD32E51x/Source/ARM/startup_gd32eprtxxa.s(1 hunks)GD32E51x/GD/GD32E51x/Source/IAR/startup_gd32e51x_cl.s(1 hunks)GD32E51x/GD/GD32E51x/Source/IAR/startup_gd32e51x_hd.s(1 hunks)GD32E51x/GD/GD32E51x/Source/IAR/startup_gd32eprtxxa.s(1 hunks)GD32E51x/GD/GD32E51x/Source/system_gd32e51x.c(1 hunks)GD32E51x/LICENSE.TXT(1 hunks)GD32E51x/SConscript(1 hunks)GD32E51x/cmsis_armcc.h(1 hunks)GD32E51x/cmsis_armclang.h(1 hunks)GD32E51x/cmsis_compiler.h(1 hunks)GD32E51x/cmsis_iccarm.h(1 hunks)GD32E51x/cmsis_version.h(1 hunks)GD32E51x/mpu_armv8.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
GD32E51x/GD/GD32E51x/Include/system_gd32e51x.h (1)
GD32E51x/GD/GD32E51x/Source/system_gd32e51x.c (3)
SystemInit(139-181)SystemCoreClockUpdate(1036-1130)gd32e51x_firmware_version_get(1139-1142)
GD32E51x/cmsis_compiler.h (2)
GD32E51x/cmsis_armclang.h (4)
T_UINT16_WRITE(75-75)T_UINT16_READ(83-83)T_UINT32_WRITE(91-91)T_UINT32_READ(99-99)GD32E51x/cmsis_iccarm.h (1)
struct(221-221)
GD32E51x/cmsis_iccarm.h (3)
GD32E51x/cmsis_armcc.h (16)
void(134-138)void(146-150)void(158-162)void(170-174)void(182-186)void(194-198)void(206-210)void(218-222)void(230-234)void(242-246)void(254-258)void(285-289)void(297-301)void(310-314)void(322-326)void(334-338)GD32E51x/mpu_armv8.h (1)
uint32_t(193-196)GD32E51x/cmsis_compiler.h (1)
struct(236-236)
🪛 Clang (14.0.6)
GD32E51x/GD/GD32E51x/Include/gd32e51x.h
[error] 38-38: "Please select chip type in project configuration"
(clang-diagnostic-error)
GD32E51x/cmsis_armcc.h
[error] 134-134: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 136-136: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 136-136: unknown register name 'control' in asm
(clang-diagnostic-error)
[error] 146-146: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 148-148: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 148-148: unknown register name 'control' in asm
(clang-diagnostic-error)
[error] 158-158: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 160-160: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 160-160: unknown register name 'ipsr' in asm
(clang-diagnostic-error)
[error] 170-170: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 172-172: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 172-172: unknown register name 'apsr' in asm
(clang-diagnostic-error)
[error] 182-182: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 184-184: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 184-184: unknown register name 'xpsr' in asm
(clang-diagnostic-error)
[error] 194-194: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 196-196: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 196-196: unknown register name 'psp' in asm
(clang-diagnostic-error)
[error] 206-206: unknown type name 'uint32_t'
(clang-diagnostic-error)
GD32E51x/cmsis_iccarm.h
[error] 30-30: This file should only be compiled by ICCARM
(clang-diagnostic-error)
[error] 94-94: "Unknown target."
(clang-diagnostic-error)
[error] 176-176: unknown type name '__intrinsic'
(clang-diagnostic-error)
[error] 176-176: expected ';' after top level declarator
(clang-diagnostic-error)
GD32E51x/mpu_armv8.h
[error] 185-185: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 186-186: unknown type name 'uint32_t'
(clang-diagnostic-error)
[error] 193-193: unknown type name '__STATIC_INLINE'
(clang-diagnostic-error)
[error] 193-193: expected ';' after top level declarator
(clang-diagnostic-error)
🪛 LanguageTool
GD32E51x/LICENSE.TXT
[style] ~161-~161: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
🔇 Additional comments (20)
GD32E51x/LICENSE.TXT (1)
1-201: LGTM - Standard Apache 2.0 license file.The license file correctly accompanies the new GD32E51x CMSIS files and matches the Apache 2.0 license referenced in the source file headers.
GD32E51x/cmsis_version.h (1)
1-39: LGTM - Standard CMSIS version header.The version definitions follow the standard CMSIS Core(M) conventions with proper include guards and compiler-specific pragmas.
GD32E51x/mpu_armv8.h (1)
1-96: Standard CMSIS MPU header implementation.The MPU API follows standard Armv8-M patterns with proper memory barrier usage in enable/disable sequences and appropriate bounds checking in
ARM_MPU_SetMemAttrEx. The static analysis errors about unknown types are expected false positives - this header is designed to be included after the device header andcmsis_compiler.h.Also applies to: 99-422
GD32E51x/cmsis_armcc.h (1)
1-869: Standard CMSIS ARMCC (Arm Compiler 5) header.This is a standard CMSIS compiler abstraction header for the ARM Compiler 5 toolchain. The static analysis errors are expected false positives - this header uses ARMCC-specific syntax (e.g.,
register uint32_t __regControl __ASM("control")) that is not compatible with Clang/GCC.The header correctly:
- Checks for minimum compiler version (V4.0.677)
- Provides architecture-specific macros and intrinsics
- Includes fallback implementations for M0 family (lines 520-535, 740-780)
- Guards FPU-related code appropriately
GD32E51x/cmsis_iccarm.h (1)
1-187: Standard CMSIS ICCARM (IAR) header.This is a standard CMSIS compiler abstraction header for the IAR EWARM toolchain. The static analysis errors are intentional - the header includes explicit guards that error out when not compiled with ICCARM (line 30) or when the target architecture cannot be determined (line 94).
The header correctly handles:
- IAR version detection (
__ICCARM_V8)- ARM architecture macro definitions
- Both intrinsics version 2 and legacy intrinsics paths
- TrustZone secure/non-secure register access for Armv8-M
- M0 family fallback implementations
Also applies to: 194-209, 214-940
GD32E51x/GD/GD32E51x/Include/system_gd32e51x.h (1)
28-56: LGTM overall.The header follows CMSIS conventions correctly with proper include guards, C++ linkage compatibility, and appropriate function declarations that align with the implementations in
system_gd32e51x.c.GD32E51x/cmsis_compiler.h (1)
1-270: Standard CMSIS compiler abstraction header.This is a standard ARM CMSIS V5.1.0 compiler header that provides toolchain-agnostic macro definitions. The implementation correctly handles multiple compilers (ARM CC, Armclang, GCC, IAR, TI ARM, TASKING, COSMIC) with appropriate fallbacks and guards.
GD32E51x/GD/GD32E51x/Source/IAR/startup_gd32eprtxxa.s (1)
157-163: Reset flow looks correct.The Reset_Handler properly calls
SystemInitfor clock and peripheral initialization before jumping to__iar_program_startfor C runtime setup.GD32E51x/GD/GD32E51x/Source/system_gd32e51x.c (2)
139-181: SystemInit implementation follows CMSIS conventions.The initialization sequence correctly:
- Enables FPU access when configured
- Resets clock configuration to default state
- Handles device variant differences (HD vs CL/E518/PRTxxA)
- Configures the vector table location
1036-1130: SystemCoreClockUpdate implementation is thorough.The function correctly handles all clock source scenarios (IRC8M, HXTAL, PLL) with proper variant-specific logic for PLL source selection, predividers, and PLL multiplier decoding. The AHB divider application at the end ensures accurate SystemCoreClock reporting.
GD32E51x/GD/GD32E51x/Include/gd32e51x.h (1)
319-327: Verify header include order.The CMSIS
core_cm33.hinclude appears after theIRQn_Typeenum definition, which is the correct order per CMSIS requirements. The enum and core configuration macros must be defined before including the core header.The typedefs on lines 325-327 use the common embedded pattern of using
!DISABLE,!RESET,!ERRORto set the alternate enum value to 1 (or non-zero), which is acceptable.GD32E51x/GD/GD32E51x/Source/ARM/startup_gd32e51x_hd.s (2)
33-44: Stack and heap sizes are modest but configurable.Stack (1KB) and heap (1KB) sizes are relatively small. These are configurable via the IDE/project settings, but users with heavy stack usage (deep call chains, large local buffers) may need to increase these values.
60-166: Vector table is complete and properly structured.The vector table correctly maps all exception and interrupt handlers for the GD32E51X_HD variant, matching the
IRQn_Typeenum ingd32e51x.h. Reserved entries are properly filled with zeros, and all handlers are declared as WEAK for user override.GD32E51x/GD/GD32E51x/Source/ARM/startup_gd32e51x_cl.s (2)
33-33: Appropriate stack size increase for CL variant.The CL (connectivity line) variant has 2KB stack versus 1KB in HD, which is appropriate given the additional networking and USB peripherals.
60-166: Vector table is complete for CL variant.The vector table properly includes all CL-specific peripherals (USBHS, ENET, CAN1, CAN2, SHRTIMER) with correct positioning and reserved slots.
GD32E51x/GD/GD32E51x/Source/ARM/startup_gd32eprtxxa.s (2)
60-166: Vector table correctly handles IRQ numbering gaps.The GD32EPRTxxA variant has significant gaps in the IRQ numbering (e.g., from ENET_WKUP at 78 to I2C2_EV at 98). The vector table properly uses reserved (0) entries to maintain correct alignment, ensuring proper interrupt dispatching.
229-295: WEAK exports match vector table entries.All external IRQ handlers are properly exported as WEAK in the Default_Handler PROC block, allowing users to override any handler without linker errors.
GD32E51x/cmsis_armclang.h (3)
30-32: Compatibility header include for ARM Compiler 5.The
arm_compat.hinclude provides backward compatibility with ARM Compiler 5 intrinsics when using ARM Compiler 6 (armclang).
147-184: TrustZone functions correctly guarded.The non-secure (NS) register access functions are properly guarded with
__ARM_FEATURE_CMSE == 3, ensuring they're only available when compiling for secure state on ARMv8-M targets with CMSE.
1256-1800: SIMD/DSP intrinsics properly guarded.The SIMD intrinsic implementations are correctly wrapped with
#if (defined (__ARM_FEATURE_DSP) && (__ARM_FEATURE_DSP == 1)), ensuring they're only compiled when the target supports DSP extensions. The Cortex-M33 in GD32E51x has DSP support (__DSP_PRESENT = 1in gd32e51x.h).
| /**************************************************************************//** | ||
| * @file cmsis_armclang.h | ||
| * @brief CMSIS compiler ARMCLANG (ARM compiler V6) header file | ||
| * @version V5.0.3 | ||
| * @date 27. March 2017 | ||
| ******************************************************************************/ |
There was a problem hiding this comment.
Update CMSIS header to a current version.
This CMSIS ARMCLANG header (V5.0.3, March 2017) is significantly outdated. Current CMSIS version is 6.1.1, and ARM recommends upgrading to CMSIS Version 6. The file lacks important compiler fixes and API improvements accumulated over 8+ years of development. Update to CMSIS 6 to align with current tooling and obtain bug fixes.
| \file gd32e51x.h | ||
| \brief general definitions for GD32E51x | ||
|
|
||
| ; \version 2024-12-30, V1.2.0, firmware for GD32E51x |
There was a problem hiding this comment.
Minor typo: stray semicolon in version comment.
Line 5 has a semicolon at the beginning which appears unintentional.
-; \version 2024-12-30, V1.2.0, firmware for GD32E51x
+ \version 2024-12-30, V1.2.0, firmware for GD32E51x📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ; \version 2024-12-30, V1.2.0, firmware for GD32E51x | |
| \version 2024-12-30, V1.2.0, firmware for GD32E51x |
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Include/gd32e51x.h around line 5 there is a stray
semicolon at the start of the version comment ("; \version 2024-12-30,
V1.2.0, firmware for GD32E51x"); remove the leading semicolon so the line is a
proper comment (or aligns with the surrounding comment style) to fix the typo.
| #if !defined (GD32EPRTxxA) &&!defined (GD32E51X_HD) && !defined (GD32E51X_CL) && !defined (GD32E518) | ||
| #error "Please select chip type in project configuration" | ||
| /* #define GD32EPRTxxA */ | ||
| /* #define GD32E51X_HD */ | ||
| /* #define GD32E51X_CL */ | ||
| /* #define GD32E518 */ | ||
| #endif /* define GD32E51X */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "gd32e51x.h" -type fRepository: RT-Thread-packages/gd32-arm-cmsis
Length of output: 118
🏁 Script executed:
head -n 350 ./GD32E51x/GD/GD32E51x/Include/gd32e51x.h | cat -nRepository: RT-Thread-packages/gd32-arm-cmsis
Length of output: 25138
🏁 Script executed:
grep -n "GD32E518" ./GD32E51x/GD/GD32E51x/Include/gd32e51x.hRepository: RT-Thread-packages/gd32-arm-cmsis
Length of output: 208
🏁 Script executed:
grep -n "GD32E518" ./GD32E51x/GD/GD32E51x/Include/*.hRepository: RT-Thread-packages/gd32-arm-cmsis
Length of output: 292
Add missing IRQn_Type block for GD32E518 variant.
The chip selection guard (line 37) lists GD32E518 as a valid option, but there is no corresponding #ifdef GD32E518 block in the IRQn_Type enum (lines 105-317). Users defining GD32E518 will only receive the common interrupt definitions, missing all variant-specific interrupts needed for that chip.
🧰 Tools
🪛 Clang (14.0.6)
[error] 38-38: "Please select chip type in project configuration"
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Include/gd32e51x.h around lines 37 and 105–317, the
preprocessor chip-selection guard mentions GD32E518 but there is no
corresponding #ifdef GD32E518 section in the IRQn_Type enum, so variant-specific
interrupts are missing; add an #elif defined (GD32E518) (or #ifdef GD32E518)
block in the IRQn_Type enum mirroring the pattern used for
GD32EPRTxxA/GD32E51X_HD/GD32E51X_CL, and populate it with the GD32E518-specific
IRQ definitions (or include the common + variant-specific interrupt list),
keeping naming/values consistent with the other variant blocks and protecting
the block with the correct preprocessor condition.
| DMA1_Channel1_IRQn = 57, /*!< DMA1 channel1 global interrupt */ | ||
| DMA1_Channel2_IRQn = 58, /*!< DMA1 channel2 global interrupt */ | ||
| DMA1_Channel3_IRQn = 59, /*!< DMA1 channel3 global interrupt */ | ||
| DMA1_Channel4_IRQn = 60, /*!< DMA1 channel3 global interrupt */ |
There was a problem hiding this comment.
Incorrect comment for DMA1_Channel4_IRQn.
The comment incorrectly states "channel3" instead of "channel4".
- DMA1_Channel4_IRQn = 60, /*!< DMA1 channel3 global interrupt */
+ DMA1_Channel4_IRQn = 60, /*!< DMA1 channel4 global interrupt */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DMA1_Channel4_IRQn = 60, /*!< DMA1 channel3 global interrupt */ | |
| DMA1_Channel4_IRQn = 60, /*!< DMA1 channel4 global interrupt */ |
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Include/gd32e51x.h around line 289, the inline comment
for the enum entry DMA1_Channel4_IRQn is wrong (it says "DMA1 channel3 global
interrupt"); update the comment to correctly reference channel4, e.g. change the
trailing comment to "/*!< DMA1 channel4 global interrupt */" so the enum
documentation matches the identifier.
|
|
||
| #include <stdint.h> | ||
|
|
||
| /* firmware version can be aquired by uncommenting the macro */ |
There was a problem hiding this comment.
Typo: "aquired" → "acquired".
Minor spelling error in the comment.
-/* firmware version can be aquired by uncommenting the macro */
+/* firmware version can be acquired by uncommenting the macro */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* firmware version can be aquired by uncommenting the macro */ | |
| /* firmware version can be acquired by uncommenting the macro */ |
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Include/system_gd32e51x.h around line 37, the comment
contains a typo: replace "aquired" with the correct spelling "acquired" so the
comment reads "firmware version can be acquired by uncommenting the macro".
| PUBWEAK EXTI4_IRQHandler | ||
| SECTION .text:CODE:NOROOT:REORDER(1) | ||
| EXTI4_IRQHandler | ||
| B EXTI1_IRQHandler |
There was a problem hiding this comment.
Critical: EXTI4_IRQHandler default stub branches to wrong handler.
The default weak implementation of EXTI4_IRQHandler incorrectly branches to EXTI1_IRQHandler instead of itself. This is a copy-paste error that could cause unexpected behavior if EXTI4 triggers without a user-defined handler.
PUBWEAK EXTI4_IRQHandler
SECTION .text:CODE:NOROOT:REORDER(1)
EXTI4_IRQHandler
- B EXTI1_IRQHandler
+ B EXTI4_IRQHandler📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PUBWEAK EXTI4_IRQHandler | |
| SECTION .text:CODE:NOROOT:REORDER(1) | |
| EXTI4_IRQHandler | |
| B EXTI1_IRQHandler | |
| PUBWEAK EXTI4_IRQHandler | |
| SECTION .text:CODE:NOROOT:REORDER(1) | |
| EXTI4_IRQHandler | |
| B EXTI4_IRQHandler |
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Source/IAR/startup_gd32eprtxxa.s around lines 260 to 263
the weak default EXTI4_IRQHandler currently branches to EXTI1_IRQHandler due to
a copy/paste error; replace the branch target so the stub branches to itself
(EXTI4_IRQHandler) or use a BX LR/appropriate default return instruction so the
default handler does not invoke the wrong IRQ handler; update the label/branch
accordingly to ensure EXTI4 invokes the correct weak stub when not overridden.
| #define RCU_MODIFY(__delay) do{ \ | ||
| volatile uint32_t i,reg;; \ | ||
| if(0 != __delay){ \ |
There was a problem hiding this comment.
Minor: Double semicolon in macro.
There's an extra semicolon after the variable declaration.
- volatile uint32_t i,reg;; \
+ volatile uint32_t i,reg; \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define RCU_MODIFY(__delay) do{ \ | |
| volatile uint32_t i,reg;; \ | |
| if(0 != __delay){ \ | |
| #define RCU_MODIFY(__delay) do{ \ | |
| volatile uint32_t i,reg; \ | |
| if(0 != __delay){ \ |
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Source/system_gd32e51x.c around lines 54 to 56, the
RCU_MODIFY macro declares "volatile uint32_t i,reg;;" which contains a double
semicolon; remove the extra semicolon so the declaration ends with a single
semicolon (i.e., change "i,reg;;" to "i,reg;") to fix the minor syntax issue.
| #elif defined (__SYSTEM_CLOCK_120M_PLL_HXTAL) | ||
| /*! | ||
| \brief configure the system clock to 72M by PLL which selects HXTAL as its clock source | ||
| \param[in] none | ||
| \param[out] none | ||
| \retval none | ||
| */ | ||
| static void system_clock_120m_hxtal(void) |
There was a problem hiding this comment.
Documentation mismatch: Comment says 72M but function configures 120M.
The doxygen comment states "configure the system clock to 72M" but this is system_clock_120m_hxtal.
- \brief configure the system clock to 72M by PLL which selects HXTAL as its clock source
+ \brief configure the system clock to 120M by PLL which selects HXTAL as its clock source🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Source/system_gd32e51x.c around lines 706 to 713, the
doxygen brief incorrectly says "configure the system clock to 72M" while the
function name and intent are for 120M; update the comment to correctly state
"configure the system clock to 120M by PLL which selects HXTAL as its clock
source" (and scan nearby comments/variants for similar copy-paste errors to
ensure all frequency mentions match the function names).
| #elif defined (__SYSTEM_CLOCK_168M_PLL_HXTAL) | ||
| /*! | ||
| \brief configure the system clock to 108M by PLL which selects HXTAL as its clock source | ||
| \param[in] none | ||
| \param[out] none | ||
| \retval none | ||
| */ | ||
| static void system_clock_168m_hxtal(void) |
There was a problem hiding this comment.
Documentation mismatch: Comment says 108M but function configures 168M.
The doxygen comment states "configure the system clock to 108M" but this is system_clock_168m_hxtal.
- \brief configure the system clock to 108M by PLL which selects HXTAL as its clock source
+ \brief configure the system clock to 168M by PLL which selects HXTAL as its clock source📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #elif defined (__SYSTEM_CLOCK_168M_PLL_HXTAL) | |
| /*! | |
| \brief configure the system clock to 108M by PLL which selects HXTAL as its clock source | |
| \param[in] none | |
| \param[out] none | |
| \retval none | |
| */ | |
| static void system_clock_168m_hxtal(void) | |
| #elif defined (__SYSTEM_CLOCK_168M_PLL_HXTAL) | |
| /*! | |
| \brief configure the system clock to 168M by PLL which selects HXTAL as its clock source | |
| \param[in] none | |
| \param[out] none | |
| \retval none | |
| */ | |
| static void system_clock_168m_hxtal(void) |
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Source/system_gd32e51x.c around lines 813 to 820, the
doxygen block incorrectly states "configure the system clock to 108M" while the
function name and intended behavior are for 168M; update the comment to
correctly state "configure the system clock to 168M by PLL which selects HXTAL
as its clock source" (and adjust any param/retval lines if needed to match
surrounding style) so documentation matches the function.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.S (1)
15-15:BootRAMconstant defined but never referenced.The
BootRAMconstant at0xF1E0F85Fis defined but not used anywhere in this startup file. If it's needed for boot mode detection or system memory boot, consider documenting its purpose; otherwise, it may be dead code.GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_cl.S (1)
15-15:BootRAMconstant unused (same as HD variant).Consider removing or documenting the purpose of this constant.
GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32eprtxxa.S (1)
15-15: Remove or document unusedBootRAMconstant.The
BootRAMconstant is defined but never referenced in this file. If it's not required for boot-from-RAM scenarios or future functionality, consider removing it to avoid confusion.#!/bin/bash # Description: Check if BootRAM is used elsewhere in the codebase or in related startup files # Search for BootRAM references echo "=== Searching for BootRAM usage ===" rg -n "BootRAM" -g "*.s" -g "*.S" -g "*.c" -g "*.h"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_cl.S(1 hunks)GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.S(1 hunks)GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32eprtxxa.S(1 hunks)GD32E51x/SConscript(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- GD32E51x/SConscript
🔇 Additional comments (14)
GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.S (5)
1-4: LGTM: Standard GCC assembly directives for Cortex-M33.The
.syntax unified,.cpu cortex-m33,.fpu softvfp, and.thumbdirectives correctly configure the assembler for the GD32E51x target.
22-57: Data/BSS initialization and startup sequence looks correct.The logic properly:
- Copies initialized data from flash (
_sidata) to RAM (_sdatato_edata)- Zeroes the BSS section (
_sbssto_ebss)- Calls
SystemInit,__libc_init_array, andmain- Falls into infinite loop on return
61-65: LGTM: Default_Handler with infinite loop.Standard pattern for unhandled interrupts that safely traps execution.
67-178: Vector table structure looks complete for HD variant.The vector table correctly includes:
- Stack pointer and core exception handlers (Reset through SysTick)
- External IRQ handlers specific to the high-density variant (USBD, SDIO, ADC2, etc.)
- Reserved entries (
.word 0) at appropriate positions
180-433: Weak aliases correctly map all handlers to Default_Handler.All interrupt handlers in the vector table have corresponding
.weakand.thumb_setdirectives, allowing user code to override any handler.GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_cl.S (3)
22-57: LGTM: Data/BSS initialization and startup sequence.Identical correct implementation as the HD variant.
67-177: Vector table correctly reflects connectivity-line peripherals.Includes CL-specific peripherals: USBHS, ENET, CAN2, TMU, and separate DMA1 channel handlers (vs combined
DMA1_Channel3_4in HD variant).
179-459: Weak aliases complete and consistent with vector table.All CL-variant handlers (including
ENET_IRQHandler,USBHS_IRQHandler,CAN2_*_IRQHandler,TMU_IRQHandler) have proper weak definitions.GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32eprtxxa.S (6)
1-4: LGTM! Correct CPU and FPU configuration.The assembly directives correctly specify Cortex-M33 with software floating point, which aligns with the GD32E51x family specifications.
17-59: LGTM! Standard CMSIS startup sequence correctly implemented.The Reset_Handler properly implements the standard startup flow:
- Copy initialized data from flash to RAM
- Zero-initialize BSS
- Call SystemInit for clock/system configuration
- Call C++ constructors via __libc_init_array
- Jump to main
- Infinite loop if main returns
61-65: LGTM! Appropriate default interrupt handler.The Default_Handler correctly implements an infinite loop, which safely halts execution if an unhandled interrupt occurs and allows debugging.
180-397: LGTM! Weak handler declarations follow CMSIS standard.All interrupt handlers are correctly declared as weak symbols aliased to Default_Handler, allowing user code to override specific handlers as needed.
67-177: Verify interrupt vector assignments against the GD32E51x reference manual. The vector table includes reserved entries at vectors 77-97, which should be confirmed against the official datasheet to ensure all interrupt positions are correctly defined for this device.
8-13: Verify the actual symbol name for initial stack pointer in the vector table.The data/BSS initialization symbols (_sidata, _sdata, _edata, _sbss, _ebss) are correctly declared in the comment section. However, the review should specify whether the vector table at line 71 references
_spor if it uses the more standard_estacksymbol. Most ARM Cortex-M startup files use_estack(defined in the linker script), not_sp. Confirm your linker script provides the correct initial stack pointer symbol and verify the startup file references the matching symbol name.
| .section .text.Reset_Handler | ||
| .weak Reset_Handler | ||
| .type Reset_Handler, %function |
There was a problem hiding this comment.
Missing .global Reset_Handler declaration (same as HD variant).
Same issue as in startup_gd32e51x_hd.S — add .global Reset_Handler before the .weak directive.
Proposed fix
.section .text.Reset_Handler
+ .global Reset_Handler
.weak Reset_Handler
.type Reset_Handler, %function📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .section .text.Reset_Handler | |
| .weak Reset_Handler | |
| .type Reset_Handler, %function | |
| .section .text.Reset_Handler | |
| .global Reset_Handler | |
| .weak Reset_Handler | |
| .type Reset_Handler, %function |
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_cl.S around lines 17 to 19,
the startup file is missing a `.global Reset_Handler` declaration before the
`.weak Reset_Handler` directive; add a `.global Reset_Handler` line immediately
before the `.weak Reset_Handler` so the symbol is exported for the linker while
remaining weakly defined here.
| .section .text.Reset_Handler | ||
| .weak Reset_Handler | ||
| .type Reset_Handler, %function |
There was a problem hiding this comment.
Missing .global Reset_Handler declaration.
Reset_Handler is declared as .weak but not .global. The linker script typically expects Reset_Handler to be visible as a global symbol for the vector table entry. Without .global, some linker configurations may fail to resolve the symbol.
Proposed fix
.section .text.Reset_Handler
+ .global Reset_Handler
.weak Reset_Handler
.type Reset_Handler, %function🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.S around lines 17 to 19,
the Reset_Handler symbol is declared .weak but not made global which can prevent
the linker from resolving the vector table entry; add a .global Reset_Handler
directive (typically placed before or alongside the .weak) so Reset_Handler is
exported as a global symbol and kept visible to the linker.
| /* Call static constructors */ | ||
| bl __libc_init_array | ||
| /*Call the main function */ | ||
| bl main |
| /* Call static constructors */ | ||
| bl __libc_init_array | ||
| /*Call the main function */ | ||
| bl main |
| /* Call static constructors */ | ||
| bl __libc_init_array | ||
| /*Call the main function */ | ||
| bl main |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.S (1)
17-19: Missing.global Reset_Handlerdeclaration.This issue was previously flagged.
Reset_Handleris declared as.weakbut not.global, which can prevent the linker from resolving the vector table entry. Add.global Reset_Handlerbefore the.weakdirective.🔎 Proposed fix
.section .text.Reset_Handler + .global Reset_Handler .weak Reset_Handler .type Reset_Handler, %function
🧹 Nitpick comments (2)
GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32eprtxxa.S (1)
46-46: Minor: Inconsistent spacing in operand.There's an extra space before
_ebssthat differs from other similar lines.🔎 Proposed fix
- ldr r3, = _ebss + ldr r3, =_ebssGD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.S (1)
15-15: Remove unusedBootRAMconstant.The
BootRAMconstant is defined but never referenced in this startup file. Unless it's part of a required CMSIS pattern for GD32E51x, consider removing it to avoid dead code.🔎 Proposed fix
- -.equ BootRAM, 0xF1E0F85F
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_cl.SGD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.SGD32E51x/GD/GD32E51x/Source/GCC/startup_gd32eprtxxa.SGD32E51x/SConscript
🚧 Files skipped from review as they are similar to previous changes (2)
- GD32E51x/SConscript
- GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_cl.S
🔇 Additional comments (11)
GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32eprtxxa.S (6)
1-4: LGTM!The assembly directives are appropriate for the Cortex-M33 based GD32E51x series. The
.fpu softvfpis a reasonable default for startup code.
6-15: LGTM!The global symbol declaration and linker symbol references follow standard GCC ARM startup conventions. The
.worddirectives correctly establish literal pool entries for the data/BSS initialization loop.
22-57: Reset_Handler implementation is correct.The startup sequence properly:
- Copies initialized data from flash to RAM
- Zeros BSS section
- Calls
SystemInit,__libc_init_array, andentryThe previous review comment requesting
bl entryhas been addressed.
61-65: LGTM!Standard default handler pattern with infinite loop for unhandled interrupts.
67-178: LGTM!The vector table is properly structured with:
- Initial stack pointer and core exception vectors (1-15)
- External interrupt handlers for GD32E51x peripherals
- Reserved slots correctly set to 0
- Clear comments documenting each vector
180-397: LGTM!All interrupt handlers are correctly declared as weak symbols and aliased to
Default_Handlerusing.thumb_set, allowing application code to override any handler as needed.GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.S (5)
61-65: LGTM!The
Default_Handlerimplementation is correct. Unhandled interrupts will enter an infinite loop, which is the standard safe behavior for ARM Cortex-M startup files.
180-433: LGTM!All weak interrupt handler definitions are correctly implemented. Each handler is properly declared as
.weakand aliased toDefault_Handlerusing.thumb_set, allowing application code to override them while providing safe defaults. The handler list matches the vector table completely.
9-13: Verify whether these.worddeclarations serve a specific purpose or can be removed.Lines 9-13 define
.worddirectives for linker script symbols (_sidata, _sdata, _edata, _sbss, _ebss), consistent with the pattern used instartup_gd32eprtxxa.S. However, the Reset_Handler code (lines 11, 17–18, 22, 30) references these symbols directly vialdr r0, =_symbolsyntax, which resolves them at link time. The.worddeclarations create physical data entries in the binary that are never read by the code, making them functionally redundant. While the inline comment labels them as "necessary," clarify whether they're required for toolchain-specific reasons (e.g., symbol availability in the binary) or if they're legacy code that could be simplified by removing them. The code would work identically without these declarations.
54-54: This is correct for RT-Thread integration—no changes needed.The
entryfunction is an external symbol that will be provided by the RT-Thread kernel, not by this CMSIS package. All GD32 startup files in this repository consistently callbl entryrather thanmain, confirming this is the standard pattern for RT-Thread integration across this device family.
71-71: Ensure linker script defines_spfor the vector table.The vector table at line 71 references
_spas the initial stack pointer. This symbol must be provided by your linker script. Add to your.ldfile:PROVIDE(_sp = ORIGIN(RAM) + LENGTH(RAM));or similar based on your RAM configuration. The GCC startup files in this CMSIS package expect linker script symbols (_sp,_sdata,_edata,_sbss,_ebss); the ARM variant shows an alternative pattern with locally-defined__initial_sp.
| .section .vectors,"a",%progbits | ||
| .global __gVectors |
There was a problem hiding this comment.
Use standard __Vectors symbol name for CMSIS compliance.
The vector table uses __gVectors instead of the CMSIS-standard __Vectors. Standard toolchains and debuggers expect __Vectors for the interrupt vector table. Using a non-standard name may cause issues with:
- Debugger symbol resolution
- CMSIS-compliant linker scripts
- Toolchain compatibility
🔎 Proposed fix
.section .vectors,"a",%progbits
- .global __gVectors
+ .global __Vectors
-__gVectors:
+__Vectors:Also update line 178:
- .size __gVectors, . - __gVectors
+ .size __Vectors, . - __VectorsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In GD32E51x/GD/GD32E51x/Source/GCC/startup_gd32e51x_hd.S around lines 67-68 (and
also update the corresponding label at line 178), the vector table symbol is
named __gVectors which is non‑standard; rename the symbol to the CMSIS standard
__Vectors by replacing the .global and any label/definition occurrences of
__gVectors with __Vectors so the linker/debugger and CMSIS toolchain find the
interrupt vector table correctly.
3 participants
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.