Add STM32U3 platform bringup#420
Conversation
Extract common STM32 ARMv8-M console and timer routines. Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces foundational support for the STM32U3 family of microcontrollers, enabling the Phoenix-RTOS loader (PLO) to function on these new targets. It establishes core functionalities such as UART communication, SysTick timer, and program Flash access, allowing for the successful loading and launching of a decoy kernel binary. The changes also include architectural refactoring to share common ARMv8-M components and robust error handling for Flash bank switching, paving the way for future Phoenix-RTOS kernel integration. Highlights
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the STM32U3 platform, which is a significant addition. The changes include new HAL files for STM32U3, updates to the build system, and refactoring of console and timer drivers to be shared across ARMv8-M STM32 platforms. The error handling for the bankswitch command has also been improved.
My review has identified a few issues in the new STM32U3 HAL implementation. I found a potential bug in the _stm32_switchFlashBank function where CPU voltage is not restored on a timeout, which could leave the system in an inconsistent state. I've also noted some minor code quality issues in the new header files, such as duplicated function declarations and an incorrect include guard, as well as a misleading comment in the clock configuration code. Please see my detailed comments for suggestions on how to address these points.
|
@gemini-code-assist check if issues you mentioned were fixed |
|
@mkarcz-apator, thank you for reaching out! I haven't performed a detailed code review with specific issues yet. My previous comment was a summary of the pull request. If you would like me to perform a full code review and identify any potential issues, please use the command |
|
/gemini review |
|
Hello @mkarcz-apator, thank you for initiating the code review. I've thoroughly analyzed the changes in this pull request, which primarily focuses on adding support for the STM32U3 family of microcontrollers and refactoring common ARMv8-M components. Overall, the changes are well-structured and introduce significant new functionality. The approach to abstract common HAL components and use conditional compilation for platform-specific details is appropriate for maintaining a clean codebase. Here's a summary of my findings: General Observations
Minor SuggestionsI have a few minor suggestions related to maintainability and clarity:
These are minor points and do not detract from the overall quality of the pull request. The changes are a solid foundation for STM32U3 support. Let me know if you have any further questions or would like me to elaborate on any of these points. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the STM32U3 platform, which is a significant addition. The changes include new HAL implementation, device drivers, and build system updates. The code is well-structured, particularly the new HAL for STM32U3. I've found a couple of issues: one related to a potentially incorrect timeout in the flash bank switching logic, which could cause failures, and another regarding code duplication that affects maintainability. Specifically, the flash timeout should be configurable as a device-specific parameter. Addressing these points would further improve the quality of this contribution.
| enum { | ||
| uart_clk_sel_pclk = 0, /* pclk1 or pclk2 depending on peripheral */ | ||
| #if defined(__CPU_STM32N6) | ||
| uart_clk_sel_per_ck, | ||
| uart_clk_sel_ic9_ck, | ||
| uart_clk_sel_ic14_ck, | ||
| uart_clk_sel_lse_ck, | ||
| uart_clk_sel_msi_ck, | ||
| uart_clk_sel_hsi_div_ck, | ||
| #elif defined(__CPU_STM32U3) | ||
| uart_clk_sel_hsi_ck, | ||
| #endif | ||
| }; |
There was a problem hiding this comment.
This enum for UART clock selection is also defined in devices/uart-stm32/uart.c. To improve maintainability and avoid potential inconsistencies, consider moving this enum to a shared header file that can be included by both console.c and uart.c.
There was a problem hiding this comment.
HAL console and Devices are conceptually distinct subsystems.
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the STM32U3 platform. The changes are extensive and include new HAL files for the U3 series, refactoring of common ARMv8-M code for console and timer, and updates to existing drivers and commands to accommodate the new platform. The implementation of flash bank switching with error propagation is a good addition. My review focuses on improving maintainability by removing magic numbers, enhancing error handling clarity, and correcting a potentially incorrect timeout value for flash operations, with specific references to repository rules where applicable.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the STM32U3 platform, a significant new feature. The changes include adding new hardware abstraction layer files for the U3 series, refactoring common ARMv8-M code, and updating drivers. My review focused on the correctness and maintainability of the new code. I've identified a few issues that should be addressed to ensure the stability and correctness of the new platform support. These include a bug in the error logging for the bankswitch command, incorrect timeout values for flash operations which could lead to failures, a confusing bit shift operation, and a misleading comment in the clock configuration within the new STM32U3 HAL.
| if (frequency_range->range >= 0) { | ||
| /* Configure MSIS */ | ||
| v = *(stm32_common.rcc + rcc_icscr1) & ~(0x7 << 29); | ||
| v |= ((frequency_range->range & 0x7) << 29) | (1 << 23); /* Set MSISSEL | MSISDIV | MSIRGSEL */ |
There was a problem hiding this comment.
The comment here is incorrect and misleading. The code sets MSISRANGE (bits 29-31) and MSISSEL (bit 23), but the comment mentions MSISDIV and MSIRGSEL, which are not being modified. Please update the comment to accurately reflect the register bits being configured to improve code maintainability.
v |= ((frequency_range->range & 0x7) << 29) | (1 << 23); /* Set MSISRANGE | MSISSEL */There was a problem hiding this comment.
The hallucinated MSISRANGE register is actually MSISSEL | MSISDIV.
Enable basic support for STM32U3 (ARMv8-M) family of microcontrollers: - add STM32U3 vector table - initialize RCC with MSIS as SYSCLK - initialize SRAM - initialize power supplies - initialize GPIO ports - initialize systick and watchdog timer - specify UART console capabilities Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Use MSIS as the 24 MHz clock source for SYSCLK. Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Add STM32U3 specific UART count and clock source selection. Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Return zero (success) from _stm32_switchFlashBank, to align with STM32U3 that can fail to switch banks. Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Enable Flash bank switch support for STM32U3 family of microcontrollers: - add Flash controller register definition - enable bankswitch command in Makefile Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Report the error code if the call to _stm32_switchFlashBank failed. Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Allow memory size selection for STM32U3 family of microcontrollers: - FLASH_SIZE environment variable for program Flash bank size - RAM_SIZE environment variable for total SRAM capacity Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Take UARTx macros from <board_config.h>. Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
Allow using a single UART for both PLO UART and Phoenix-RTOS TTY. Signed-off-by: Mateusz Karcz <mateusz.karcz@apator.com>
|
(closed accidentally by the sake of wrong window focus) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the STM32U3 platform, including drivers for core peripherals and updates to the build system. The changes involve refactoring common ARMv8-M code for console and timer, and adding specific implementations for STM32U3 clocking, power management, GPIO, and flash bank switching. My review focuses on improving code clarity and maintainability by addressing ambiguous function naming, suggesting logic simplification, and refining comments on hardware-defined constants to align with repository rules.
| if (err == CMD_EXIT_SUCCESS) { | ||
| _stm32_switchFlashBank(targetBank); | ||
| int switchErr = _stm32_switchFlashBank(targetBank); | ||
| if (switchErr < 0) { | ||
| log_error("\n%s: Bank switch failed (%d)", argv[0], switchErr); | ||
| err = CMD_EXIT_FAILURE; | ||
| } | ||
| } | ||
|
|
||
| if (err == CMD_EXIT_SUCCESS) { | ||
| log_info("\n%s: Bank switch successful (%d -> %d)", argv[0], | ||
| (targetBank == 0) ? 1 : 0, targetBank); | ||
| } |
There was a problem hiding this comment.
The logic for handling the bank switch result can be simplified by using an if-else structure. This avoids checking the err variable twice and makes the code more straightforward and readable.
if (err == CMD_EXIT_SUCCESS) {
int switchErr = _stm32_switchFlashBank(targetBank);
if (switchErr < 0) {
log_error("\n%s: Bank switch failed (%d)", argv[0], switchErr);
err = CMD_EXIT_FAILURE;
}
else {
log_info("\n%s: Bank switch successful (%d -> %d)", argv[0],
(targetBank == 0) ? 1 : 0, targetBank);
}
}There was a problem hiding this comment.
This would break linearity
| { | ||
| /* Switch to PCLK clock */ | ||
| _stm32_rccSetIPClk(uartInfo[minor].ipclk_sel, uart_clk_sel_pclk); | ||
| return _stm32_rccGetPclkClock(); |
There was a problem hiding this comment.
For consistency with the suggested renaming of _stm32_rccGetPclkClock to _stm32_rccGetHclkClock for improved clarity, this call should be updated. The original function name is misleading as it returns the HCLK frequency.
return _stm32_rccGetHclkClock();There was a problem hiding this comment.
USART doesn't care about HCLK, it cares about PCLKx. The fact that PCLKx is equal to HCLK in this configuration is incidental.
| halconsole_common.refclkfreq = 64 * 1000 * 1000; | ||
| #elif defined(__CPU_STM32U3) | ||
| _stm32_rccSetIPClk(uarts[uart].ipclk_sel, uart_clk_sel_pclk); | ||
| halconsole_common.refclkfreq = _stm32_rccGetPclkClock(); |
There was a problem hiding this comment.
To align with the suggested renaming of _stm32_rccGetPclkClock to _stm32_rccGetHclkClock for improved clarity, this call should be updated. The original function name is misleading as it returns the HCLK frequency.
| halconsole_common.refclkfreq = _stm32_rccGetPclkClock(); | |
| halconsole_common.refclkfreq = _stm32_rccGetHclkClock(); |
| u32 _stm32_rccGetPclkClock(void) | ||
| { | ||
| return stm32_common.cpuclk / 2; | ||
| } |
There was a problem hiding this comment.
The function _stm32_rccGetPclkClock actually returns the HCLK frequency, as the PCLK prescalers are currently set to 1. To improve clarity and prevent potential confusion if prescalers are changed in the future, it's better to rename this function to _stm32_rccGetHclkClock. Corresponding changes should be made in the header file and at call sites.
u32 _stm32_rccGetHclkClock(void)
{
return stm32_common.cpuclk / 2;
}| *(stm32_common.flash + flash_cr) |= (1 << 17); /* OPTSTRT */ | ||
| while (*(stm32_common.flash + flash_sr) & (1 << 16)) { | ||
| /* Wait for BSY to clear */ | ||
| if ((hal_timerGet() - start) > 100) { |
There was a problem hiding this comment.
The timeout value 100 is hardcoded. If this value is a fixed hardware-defined timeout, it is acceptable to be hardcoded. However, defining it as a named constant (e.g., FLASH_BANK_SWITCH_TIMEOUT_MS) at the top of the file would significantly improve readability and maintainability, making its purpose explicit. This constant should also be used on line 577.
References
- Hardcoded limits are acceptable if they reflect practical hardware constraints and are unlikely to be exceeded. However, defining them as named constants still improves clarity.
There was a problem hiding this comment.
These can be two different values, as one refers to option byte programming, and the other - to option byte load.
| *(stm32_common.iwdg + iwdg_pr) = 6; /* prescaler divider / 256 */ | ||
| *(stm32_common.iwdg + iwdg_rlr) = 30 /* s */ * 32000 /* Hz */ / 256; |
There was a problem hiding this comment.
The values 6, 30, 32000, and 256 are likely hardware-defined parameters for the IWDG. While their values might be fixed by the hardware, defining them as named constants (e.g., IWDG_PRESCALER_6, IWDG_TIMEOUT_S, LSI_FREQ_HZ, IWDG_PRESCALER_DIV_256) would significantly improve code clarity and maintainability, making their purpose explicit.
References
- Hardcoded limits are acceptable if they reflect practical hardware constraints and are unlikely to be exceeded. However, defining them as named constants still improves clarity.
There was a problem hiding this comment.
Disagree, it's better than hal/armv8m/stm32/n6/stm32n6.c:1143-1145.
Single-use defines wouldn't improve anything.
| *(stm32_common.ramcfg + srams[i].erkeyrOffs) = 0xca; | ||
| *(stm32_common.ramcfg + srams[i].erkeyrOffs) = 0x53; |
There was a problem hiding this comment.
The values 0xca and 0x53 are specific hardware keys for unlocking SRAM erase. While these values are fixed by the device's design, defining them as named constants (e.g., SRAM_ERASE_KEY1, SRAM_ERASE_KEY2) would improve code readability and make their purpose explicit.
*(stm32_common.ramcfg + srams[i].erkeyrOffs) = SRAM_ERASE_KEY1;
*(stm32_common.ramcfg + srams[i].erkeyrOffs) = SRAM_ERASE_KEY2;References
- Hardcoded limits are acceptable if they reflect practical hardware constraints and are unlikely to be exceeded. However, defining them as named constants still improves clarity.
There was a problem hiding this comment.
hal/armv8m/stm32/n6/stm32n6.c:1030-1031
| /* Get frequency of the PCLKx clock in Hz */ | ||
| extern u32 _stm32_rccGetPclkClock(void); |
There was a problem hiding this comment.
To match the suggested renaming of _stm32_rccGetPclkClock to _stm32_rccGetHclkClock for clarity, the function declaration and its comment should be updated here as well. The original function name is misleading as it returns the HCLK frequency.
| /* Get frequency of the PCLKx clock in Hz */ | |
| extern u32 _stm32_rccGetPclkClock(void); | |
| /* Get frequency of the HCLK clock in Hz */ | |
| extern u32 _stm32_rccGetHclkClock(void); |
1 participant
Description
Enable basic support for STM32U3 (ARMv8-M) family of microcontrollers:
bankswitchcommand support using non-secure Flash option-byte access, with voltage level handlingbankswitchexecution error to the console (thus a minor interface change in the STM32L4 code was required)Motivation and Context
This enables bring-up of PLO for STM32U3 family of microcontrollers, with support for UART, SysTick, and program Flash.
PLO console is functional, so is loading and launching of a decoy kernel binary (NDA).
Parameterization of sizes of the memories allows targeting multiple SKUs of the MCU family.
Propagation of the Flash bank switch error status to the PLO console allows detecting mismatched NS/S assumptions.
Enabling of Phoenix-RTOS kernel is ongoing, and will be introduced in future PRs.
Types of changes
How Has This Been Tested?
Checklist:
Special treatment
Tagging: @kajaswat @anglov