Rework MPU config for unaligned access to external memory #3044
Conversation
josesimoes
added
Series: STM32xx
WalkthroughThe pull request introduces several modifications to the ChibiOS target configuration for STM32F7 microcontrollers, focusing on Memory Protection Unit (MPU) configurations. Key changes include the addition of new source files and functions for configuring the MPU, updates to existing function declarations, and alterations to microcontroller settings related to cache behavior. Specifically, the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (12)
🧰 Additional context used📓 Learnings (3)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (2)🔇 Additional comments (10)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (3)
The added includes are appropriate for the HAL MPU APIs and SDRAM initialization. Line range hint The extensive comments provide valuable context about the STM32F7's unaligned access requirements and the rationale behind the MPU configuration. This documentation will be helpful for future maintenance.
Two aspects of the MPU configuration should be verified:
✅ Verification successfulMPU region configuration needs adjustment The verification reveals that region 0 is consistently used across different target boards for external memory configuration, suggesting this is an intentional design pattern. However, there are overlapping concerns:
The regions don't overlap in memory space, so there's no conflict there. However, let's verify if there's any documentation or initialization code that explains the region 0 usage pattern. Let's check the MPU configuration initialization code since we found references to MPU region configuration is correct After thorough verification, the use of MPU_REGION_NUMBER0 for external memory is intentional and correct:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other MPU region configurations in the codebase
echo "Searching for other MPU region configurations..."
rg -l "MPU_REGION_NUMBER" | while read -r file; do
echo "=== $file ==="
rg "MPU_REGION_NUMBER|MPU_InitStruct.BaseAddress|MPU_InitStruct.Size" "$file"
done
Length of output: 2238 Script: #!/bin/bash
# Search for MPU initialization and documentation
echo "Searching for MPU initialization..."
rg -l "MPU_Init|HAL_MPU_Enable"
echo -e "\nSearching for MPU-related comments..."
rg -B2 -A2 "MPU.*Region.*0|Background.*Region"
Length of output: 3132 Script: #!/bin/bash
# Check MPU configuration initialization
echo "Checking MPU configuration in target_mpu_config.c files..."
for file in $(fd target_mpu_config.c); do
echo "=== $file ==="
cat "$file"
done
Length of output: 2677 targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (4)
The added headers are necessary for using the STM32 HAL MPU configuration functions.
The function declaration follows the project's naming conventions and clearly indicates its purpose.
The initialization sequence is properly ordered:
This sequence ensures proper memory protection before any SDRAM access occurs.
The MPU configuration is well-implemented with appropriate settings:
The implementation addresses the core objective of handling unaligned access to external memory while maintaining security best practices. targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (3)
The changes properly reflect the system-level nature of these headers and add the necessary STM32F7 HAL support for MPU configuration.
The documentation thoroughly explains the unaligned access problem specific to STM32F7 MCUs and references the relevant ARM architecture specifications. This will be valuable for future maintenance.
The MPU configuration correctly:
Let's verify the memory mapping configuration: ✅ Verification successfulBased on the search results, I can now verify the MPU configuration. The search revealed:
MPU configuration is correctly implemented and consistent with memory mapping The MPU settings properly configure the SDRAM region with appropriate memory attributes and security constraints. The configuration is consistent with both the hardware architecture requirements and the memory layout defined in the build system. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify memory mapping configuration across the codebase
# Look for any conflicting MPU configurations or memory mappings
# Search for MPU region configurations
rg -A 5 "MPU_InitStruct.BaseAddress.*0[xX][CD]"
# Search for SDRAM address definitions
rg -A 2 "FSMC_Bank6_MAP_BASE|0[xX][CD][0-9A-F]{7}"
Length of output: 34996 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c (2)
11-11: Add documentation for the external function declaration.Consider adding a comment block describing the purpose, requirements, and effects of
Target_ExternalMemoryConfigMPU(). This will help maintainers understand the expected behavior and dependencies.+// Configures MPU regions for external memory access +// Must be called before enabling MPU to ensure proper memory access permissions extern void Target_ExternalMemoryConfigMPU();
27-40: Add error handling and validation to MPU configuration.While the configuration sequence is correct, the function could benefit from error handling and validation to ensure robust operation.
Consider enhancing the implementation:
+/// Configures all MPU regions for the target +/// @return true if configuration successful, false otherwise -void Target_ConfigMPU() +bool Target_ConfigMPU() { // disable MPU HAL_MPU_Disable(); // config MPU for external memory Target_ExternalMemoryConfigMPU(); + + // verify external memory configuration + if ((MPU->RBAR & MPU_RBAR_ADDR_Msk) != EXTERNAL_MEMORY_START) { + return false; + } // config MPU for non cacheable memory Target_ConfigNonCacheableMemory(); + + // verify non-cacheable memory configuration + if ((MPU->RNR = MPU_REGION_1) && + ((MPU->RASR & MPU_RASR_ENABLE_Msk) == 0)) { + return false; + } // enable MPU HAL_MPU_Enable(MPU_PRIVILEGED_DEFAULT); + + // verify MPU is enabled + if (!(MPU->CTRL & MPU_CTRL_ENABLE_Msk)) { + return false; + } + + return true; }targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt (1)
21-21: Consider adding a clarifying comment for the MPU configuration file.Similar to how
target_external_memory.chas a comment explaining its purpose, consider adding a comment for the MPU configuration file to maintain consistency and improve documentation.# the next one is required is the target implements and it's using external memory ${CMAKE_CURRENT_SOURCE_DIR}/target_external_memory.c + # required for configuring Memory Protection Unit for external memory access ${CMAKE_CURRENT_SOURCE_DIR}/target_mpu_config.ctargets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (1)
134-136: Consider cache maintenance requirements.The configuration enables buffering, caching, and sharing for the SDRAM region. While this can improve performance, please ensure:
- Proper cache maintenance (clean/invalidate) is implemented for DMA operations
- Cache coherency is maintained in multi-master scenarios (CPU/DMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c(1 hunks)targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c(3 hunks)
🔇 Additional comments (20)
targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c (3)
1-11: LGTM! Headers and declarations are properly structured.
The file includes all necessary headers for STM32 HAL and ChibiOS, and correctly declares the external memory configuration function.
1-40: Verify integration with external memory unaligned access support.
The implementation aligns with the PR objectives of supporting unaligned access to external memory. However, we should verify:
- The interaction between non-cacheable and external memory configurations
- The impact of these changes on existing memory access patterns
Let's verify the external memory configuration:
✅ Verification successful
Implementation correctly handles unaligned memory access requirements
The implementation aligns with the PR objectives and correctly handles unaligned memory access requirements. The verification shows:
-
The external memory configuration (
Target_ExternalMemoryConfigMPU()) properly configures memory attributes to handle unaligned access by setting:- Correct memory type and access permissions
- Proper cache attributes (bufferable, cacheable, shareable)
- Appropriate TEX level for memory type
-
The non-cacheable memory configuration in the reviewed file complements the external memory setup by:
- Using a separate MPU region (Region 1)
- Setting appropriate access permissions and attributes
- Not interfering with the external memory region
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the external memory configuration implementation
ast-grep --pattern 'void Target_ExternalMemoryConfigMPU() {
$$$
}'
# Look for memory access patterns in the codebase
rg "UNALIGNED|aligned" --type c
Length of output: 48233
13-25: Consider adding safety checks and documentation for MPU configuration.
The MPU configuration for non-cacheable memory could benefit from additional safety measures and clarity:
- Consider adding runtime validation to ensure region 1 is available
- Document why this specific memory region (0x20000000U) needs to be non-cacheable
- Consider if RW access for both privileged and unprivileged modes is intentional
Let's verify if this memory region conflicts with other MPU configurations:
Consider adding these improvements:
void Target_ConfigNonCacheableMemory()
{
+ // Validate MPU is present and region is available
+ if ((MPU->TYPE & MPU_TYPE_DREGION_Msk) < 2) {
+ // Handle error - MPU doesn't support enough regions
+ return;
+ }
+
+ // Region 1: Non-cacheable RAM region at 0x20000000 (STM32F7 SRAM1)
// region
MPU->RNR = ((uint32_t)MPU_REGION_1);✅ Verification successful
MPU configuration is correctly implemented and follows platform requirements
The review concerns can be addressed:
- Region 1 usage is intentional and doesn't conflict - it's part of a coordinated MPU setup where external memory and non-cacheable regions are configured sequentially
- The 0x20000000 address corresponds to the DTCM-RAM region (128K) as defined in the linker script, which is appropriately marked as non-cacheable for DMA operations
- RW access for both privileged/unprivileged is appropriate as this region is used for DMA buffers and shared memory operations
The implementation follows a proper sequence:
- MPU is disabled before configuration
- External memory is configured first
- Non-cacheable memory is configured next
- MPU is enabled with proper privileges
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other MPU region 1 configurations or uses of address 0x20000000
rg -l "MPU_REGION_1|0x20000000" --type c
Length of output: 1126
Script:
#!/bin/bash
# Let's check the actual MPU configurations in these files
rg -A 10 "MPU.*REGION_1|MPU.*RNR.*1" --type c
# Also check for any MPU initialization or setup functions
ast-grep --pattern 'void $_(void) {
$$$
MPU->$$ = $$$;
$$$
}'
Length of output: 1828
Script:
#!/bin/bash
# Let's check the MPU initialization and overall configuration context
rg -B 5 -A 15 "void.*MPU.*Config|void.*MPU.*Init" --type c
# Also check for any documentation or header files that might explain the memory layout
rg -l "SRAM|0x20000000.*RAM" targets/ChibiOS/ORGPAL_PALTHREE/
Length of output: 486
Script:
#!/bin/bash
# Let's check the linker scripts to understand the memory layout
cat targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/STM32F76xx_CLR.ld
# And check if there are any other MPU configurations in the target
rg -A 10 "MPU" targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c
Length of output: 6045
targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c (2)
1-10: LGTM! Headers and includes are appropriate.
The necessary headers for ChibiOS, HAL, and STM32 are properly included, providing access to the required MPU configuration interfaces.
1-40: Verify MPU configuration enables unaligned access.
The PR's main objective is to enable unaligned access to external memory. Let's verify the MPU configuration settings achieve this.
✅ Verification successful
MPU configuration correctly enables unaligned access for external memory
The implementation properly addresses unaligned memory access concerns:
- The external memory MPU configuration in
Target_ExternalMemoryConfigMPU()specifically configures the region as normal memory type (usingMPU_TEX_LEVEL1) instead of device memory, which allows unaligned access. - The code includes detailed comments explaining the unaligned access issue with STM32F7 and its solution.
- The configuration enables full access permissions with proper caching and buffering attributes, which is the correct setup for normal memory operations including unaligned access.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MPU configuration in external memory setup
ast-grep --pattern 'Target_ExternalMemoryConfigMPU() {
$$$
}'
# Search for any alignment requirements in memory access
rg -l "UNALIGNED|aligned|alignment" --type c
Length of output: 4242
Script:
#!/bin/bash
# Let's check the external memory configuration implementation
cat targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
# Also check for any MPU-related definitions that might affect alignment
rg "MPU_RASR|MPU_ATTR" --type=h
Length of output: 7031
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt (2)
21-22: LGTM! Addition of MPU configuration source file.
The addition of target_mpu_config.c aligns well with the PR objective to rework MPU configuration for unaligned access to external memory. Its placement alongside target_external_memory.c is logical as both files are related to external memory management.
21-21: Verify the existence of the new source file.
Let's ensure that the new MPU configuration source file exists in the correct location.
✅ Verification successful
Let me verify the content of the file to ensure it contains the expected MPU configuration code.
File exists and contains proper MPU configuration implementation
The file target_mpu_config.c exists in the correct location and contains the expected MPU configuration functions:
Target_ConfigMPU(): Properly implements MPU configuration with disable/enable sequenceTarget_ConfigNonCacheableMemory(): Correctly configures non-cacheable memory region
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of target_mpu_config.c and check its contents
# Expected: File exists and contains MPU configuration functions
# Check if the file exists
fd --type f "target_mpu_config.c" targets/ChibiOS/ORGPAL_PALTHREE/
# If file exists, verify it contains the expected MPU configuration functions
ast-grep --pattern 'void Target_ConfigMPU()'
ast-grep --pattern 'void Target_ConfigNonCacheableMemory()'
Length of output: 227
Script:
#!/bin/bash
# Check the content of the MPU configuration file
cat targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c
Length of output: 1003
targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt (1)
21-21: LGTM! Addition of MPU configuration source file.
The addition of target_mpu_config.c aligns with the PR objective to rework MPU configuration for unaligned access to external memory.
targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c (2)
22-22: LGTM: Function declaration follows conventions
The external function declaration is properly placed with other external declarations and follows C naming conventions.
97-99:
Critical initialization sequence change - Verify system stability
The placement of Target_ConfigMPU() is logically correct (before external memory initialization but after kernel initialization). However, this represents a significant change in the initialization sequence that could affect system stability.
Please verify:
- System stability across power cycles and reset scenarios
- No timing issues with subsequent external memory initialization
- Impact on existing applications that might rely on previous MPU behavior
Let's verify the initialization sequence across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the initialization sequence:
MPU configuration placement is correct and safe
The MPU configuration sequence in Target_ConfigMPU() follows the correct pattern across the codebase:
- Disables MPU first
- Configures external memory MPU settings via
Target_ExternalMemoryConfigMPU() - Configures non-cacheable memory regions
- Re-enables MPU
The placement before external memory initialization is intentional and required to prevent hard faults from unaligned memory access, as evidenced by the comment in target_external_memory.c:
// ARM: STM32F7: hard fault caused by unaligned Memory Access
// If you use an STM32F7xx microcontroller with an external SDRAM,🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other MPU configuration patterns that might conflict
rg -l "MPU" | xargs rg -A 5 -B 5 "ConfigMPU|MPU_Config|InitMPU"
# Look for external memory initialization patterns to ensure proper sequencing
rg -A 3 "Target_ExternalMemoryInit|External.*Memory"
# Check for any TODO/FIXME comments related to MPU or memory initialization
rg -l "TODO|FIXME" | xargs rg "MPU|memory|unaligned"
Length of output: 52645
targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c (2)
22-22: LGTM! Function declaration is clear and well-placed.
The external function declaration follows proper conventions and its name clearly indicates its purpose.
97-99: Verify the MPU initialization sequence.
The placement of Target_ConfigMPU() before external memory initialization is correct and aligns with the PR's objective of enabling unaligned access to external memory. However, given this is a breaking change affecting memory access, let's verify the initialization sequence.
✅ Verification successful
MPU configuration sequence is correctly placed and consistent with other targets
The initialization sequence is verified to be correct based on the following findings:
- In ORGPAL_PALX and ORGPAL_PALTHREE targets, MPU configuration is consistently placed before external memory initialization
- Other targets like ST_STM32F769I_DISCOVERY handle MPU configuration within
Target_ExternalMemoryInit()viaTarget_ExternalMemoryConfigMPU() - The comment "this has to be called after osKernelInitialize" refers to
Target_ExternalMemoryInit(), not the MPU configuration - The MPU configuration implementation shows it properly disables MPU before reconfiguring it, which is the safe approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the initialization sequence across other target platforms
# to ensure consistency and identify potential issues.
# Check for similar MPU initialization patterns in other targets
echo "Checking MPU initialization patterns in other targets:"
rg -B 5 -A 5 "Target_ConfigMPU|Target_ExternalMemoryInit" "targets/ChibiOS/"
# Look for any potential initialization order dependencies
echo -e "\nChecking for initialization order dependencies:"
rg "void\s+Target_ConfigMPU|void\s+Target_ExternalMemoryInit" "targets/ChibiOS/"
Length of output: 11863
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (2)
72-72: LGTM! Function declaration follows codebase conventions.
The function declaration is well-placed and follows the project's naming conventions.
97-99: LGTM! Correct initialization sequence.
The MPU configuration is properly placed before SDRAM initialization, which is essential for ensuring correct memory access behavior from the start.
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (1)
128-128: Verify the SDRAM base address configuration.
The MPU is configured for base address 0xD0000000, but the documentation mentions the SDRAM range as 0xC0000000-0xC03FFFFF. Please verify if this is intentional and update either the code or documentation accordingly.
✅ Verification successful
Based on the search results, I can now generate the final response:
The SDRAM base address 0xD0000000 is correct and matches the target configuration
The base address of 0xD0000000 in the MPU configuration is intentional and correct. This is verified by multiple pieces of evidence:
- The linker scripts for this target (ORGPAL_PALTHREE) consistently define external SDRAM at 0xD0000000
- The target's common header defines RAM1_MEMORY_StartAddress as 0xD0000000
- STM32 FSMC (Flexible Static Memory Controller) documentation in hal_stm32_fsmc.h shows FSMC_Bank6_MAP_BASE is 0xD0000000
The documentation mentioning 0xC0000000 appears to be generic and not specific to this target's memory layout.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of these memory addresses
rg -g '!*.{o,exe,bin}' '0x[CD][0-9A-F]000000'
Length of output: 7698
targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h (2)
Line range hint 12-24: Verify external memory configurations remain compatible.
The STM32F7xx configuration indicates this is for a high-performance device with external memory capabilities. Given the MPU rework, please verify these memory-related configurations remain compatible:
- The HAL driver settings (particularly clock configurations)
- The SDMMC driver's unaligned support setting
Run the following to verify memory-related configurations:
✅ Verification successful
Let me gather more information about the memory configurations, particularly focusing on the HAL driver settings and clock configurations.
Memory configurations are properly aligned with MPU settings
The verification shows that the memory configurations are compatible with the MPU rework:
- SDMMC unaligned support is correctly enabled (
STM32_SDC_SDMMC_UNALIGNED_SUPPORT TRUE) - External memory is properly configured:
- SDRAM is configured with appropriate MPU settings (cacheable, bufferable, shareable)
- FSMC is enabled with correct DMA channel and IRQ priority settings
- Clock configurations are properly set with required clock signals (
STM32_CLOCK48_REQUIRED TRUE) - MPU regions are properly configured for both external memory access and protection
The implementation maintains compatibility between HAL drivers, memory configurations, and the MPU settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify memory-related configurations across the codebase
# Check for SDMMC configurations
rg "SDMMC.*UNALIGNED"
# Search for memory bus configuration
rg "STM32_FSMC|STM32_FMC"
# Look for external memory initialization
ast-grep --pattern 'void $_(void) {
$$$
// Look for memory initialization
$$$_Memory$$$
$$$
}'
Length of output: 4997
Script:
#!/bin/bash
# Check HAL driver memory-related settings and clock configurations
# Look for memory clock settings
rg "STM32_SDRAM|STM32_CLOCK|STM32_MCO" targets/ChibiOS/ORGPAL_PALX/nanoCLR/
# Check memory-related HAL settings
rg "HAL_USE_SDRAM|HAL_USE_FSMC|HAL_LLD" targets/ChibiOS/ORGPAL_PALX/nanoCLR/
# Look for MPU configurations
rg "MPU|STM32_MPU" targets/ChibiOS/ORGPAL_PALX/
Length of output: 5605
30-30: Memory caching configuration change aligns with MPU rework.
The change to disable no-cache mode (STM32_NOCACHE_ENABLE = FALSE) is consistent with the PR's objective to rework MPU configuration for unaligned access to external memory. This setting will now rely on the new MPU configuration to handle memory access patterns.
Let's verify the related memory configurations:
✅ Verification successful
Let me analyze the MPU configuration changes further by looking at the related MPU configuration files.
Let me check the MPU configuration in the external memory and littlefs files since they may be affected by this change.
Memory caching configuration change is consistent with MPU settings.
The change to disable no-cache mode (STM32_NOCACHE_ENABLE = FALSE) is correctly aligned with the existing MPU configuration in the codebase:
- The target already has proper MPU configuration for external memory access in
target_external_memory.cwith cacheable settings:
MPU_InitStruct.IsCacheable = MPU_ACCESS_CACHEABLE;- The QSPI memory region is configured with non-cacheable settings in
target_littlefs.c:
MPU_InitStruct.IsCacheable = MPU_ACCESS_NOT_CACHEABLE;- Other STM32F7 targets (ST_STM32F769I_DISCOVERY, ORGPAL_PALTHREE) also have
STM32_NOCACHE_ENABLEset to FALSE.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining no-cache related configurations and MPU settings
# that might need to be updated along with this change
# Search for any remaining NOCACHE configurations
rg -l "NOCACHE"
# Look for MPU configuration files
fd -e c -e h "mpu|memory"
# Search for memory region configurations
rg "REGION|RBAR|RASR"
Length of output: 95141
Script:
#!/bin/bash
# Check MPU configuration files for ORGPAL_PALX target
rg -A 10 "MPU_InitStruct" targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
# Check for any other MPU configuration files that might be affected
rg -l "MPU_InitStruct" targets/ChibiOS/
# Look for cache-related configurations in mcuconf.h files
rg -A 5 "STM32_NOCACHE" targets/ChibiOS/ORGPAL_PALX/
Length of output: 844
Script:
#!/bin/bash
# Check the content of external memory and littlefs configuration files
cat targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
cat targets/ChibiOS/ORGPAL_PALX/target_littlefs.c
# Check for any cache-related configurations in other mcuconf.h files for comparison
rg "STM32_NOCACHE_ENABLE" targets/ChibiOS/
Length of output: 25465
targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h (2)
Line range hint 1-400: Configuration appears comprehensive and well-structured.
The overall configuration file is well-organized and includes all necessary settings for the STM32F7xx microcontroller, with appropriate settings for:
- Memory and cache configuration
- Peripheral configurations
- DMA settings
- IRQ priorities
- Clock settings
32-32: Memory caching configuration change aligns with MPU rework.
The change to disable no-cache (STM32_NOCACHE_ENABLE = FALSE) is consistent with the PR's objective of reworking the MPU configuration for unaligned access to external memory. This change will allow the MPU to handle caching behavior instead.
Let's verify this change is consistent with other memory-related configurations:
✅ Verification successful
Let me analyze the results to verify the memory caching configuration change.
Memory caching configuration change is properly integrated with MPU setup
The change to disable no-cache (STM32_NOCACHE_ENABLE = FALSE) is correctly integrated with the MPU configuration:
- The MPU is properly configured in
target_mpu_config.cwhich handles memory protection and caching attributes - External memory regions have explicit cache settings through MPU configuration
- The change aligns with the target's memory architecture where caching is managed by the MPU instead of a global no-cache setting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related memory configuration settings
# Expected: Find MPU configuration files and verify they handle caching
# Look for MPU configuration files
echo "=== Looking for MPU configuration files ==="
fd -e c -e h -e cpp "mpu|memory" --exec rg -l "MPU_|Cache"
# Check for cache-related settings in target files
echo "=== Checking cache-related settings ==="
rg -A 3 "NOCACHE|MPU_|Cache" targets/
Length of output: 95929
Script:
#!/bin/bash
# Check for MPU configuration in target files
echo "=== Looking for MPU configuration in target files ==="
rg -A 5 "Target_ConfigMPU|HAL_MPU_ConfigRegion" targets/ChibiOS/ORGPAL_PALTHREE/
# Check for memory-related settings in mcuconf.h
echo "=== Checking memory settings in mcuconf.h ==="
rg "STM32_NOCACHE|STM32_SRAM2_NOCACHE" targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h
Length of output: 2502
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (1)
124-139:
Potential Incorrect Base Address for SDRAM Configuration
The MPU_InitStruct.BaseAddress is set to 0xD0000000, but according to the comments, the external SDRAM is mapped to the address range 0xC0000000 - 0xC03FFFFF (max 4MB). Please verify that the base address and size correctly correspond to the actual SDRAM mapping for this target.
Run the following script to check the SDRAM base address and size definitions:
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c
Outdated
Show resolved
Hide resolved
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c (2)
27-40: Add error handling and atomic protection for MPU configuration.While the configuration sequence is correct, consider these improvements:
- Add error handling for configuration failures
- Validate successful configuration before re-enabling MPU
- Protect the configuration sequence from interrupts
Here's a suggested improvement:
void Target_ConfigMPU() { + // Save interrupt state and disable interrupts + chSysLock(); + // disable MPU HAL_MPU_Disable(); - // config MPU for external memory - Target_ExternalMemoryConfigMPU(); + // Configure external memory MPU + if (HAL_OK != Target_ExternalMemoryConfigMPU()) { + // Handle error + goto error; + } - // config MPU for non cacheable memory + // Configure non-cacheable memory Target_ConfigNonCacheableMemory(); + // Verify configuration + if ((MPU->CTRL & MPU_CTRL_ENABLE_Msk) == 0) { + // Handle error + goto error; + } + // enable MPU HAL_MPU_Enable(MPU_PRIVILEGED_DEFAULT); + + // Restore interrupt state + chSysUnlock(); + return; + +error: + // Restore interrupt state + chSysUnlock(); + // Handle error condition }
1-40: Consider standardizing MPU configuration across targets.Since this is part of a larger effort to rework MPU configuration for unaligned access across multiple targets (ORGPAL3 and ORGPALX), consider:
- Creating a common MPU configuration interface that can be shared across targets
- Implementing target-specific variations through a well-defined abstraction layer
- Adding configuration validation tests to ensure consistent behavior across targets
This would improve maintainability and ensure consistent MPU behavior across the codebase.
targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c (2)
11-11: Add documentation for the external function declaration.Consider adding documentation to clarify the purpose, parameters, and expected behavior of
Target_ExternalMemoryConfigMPU(). This is particularly important as it's a critical function for MPU configuration that's implemented elsewhere.+// Configures the MPU settings for external memory regions +// This function must be implemented by the specific target +// Returns: void extern void Target_ExternalMemoryConfigMPU();
1-40: Consider implementing a more comprehensive MPU management system.Given the critical nature of MPU configuration for memory access, consider implementing a more comprehensive MPU management system that includes:
- A centralized MPU configuration validation layer
- Runtime verification of MPU settings
- Logging or diagnostics for MPU-related issues
- Recovery mechanisms for MPU configuration failures
This would improve system reliability and make it easier to maintain MPU configurations across different targets.
Would you like assistance in designing this enhanced MPU management system?
targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c (1)
22-22: Add documentation for the MPU configuration function.Consider adding a brief comment explaining the purpose and requirements of the
Target_ConfigMPU()function, especially since it's a critical component for memory protection.+// Configures the Memory Protection Unit (MPU) to enable unaligned access to external memory extern void Target_ConfigMPU();targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)
137-137: Consider disabling instruction executionGiven this is an external SDRAM region primarily used for data storage (frame buffers, RAM filesystem), consider setting
MPU_INSTRUCTION_ACCESS_DISABLEfor better security.- MPU_InitStruct.DisableExec = MPU_INSTRUCTION_ACCESS_ENABLE; + MPU_InitStruct.DisableExec = MPU_INSTRUCTION_ACCESS_DISABLE;
98-140: Architectural consideration: Error handlingThe function lacks error handling for the MPU configuration. A failure in
HAL_MPU_ConfigRegioncould leave the system in an undefined state.Consider adding error handling:
void Target_ExternalMemoryConfigMPU() { // ... existing configuration code ... - HAL_MPU_ConfigRegion(&MPU_InitStruct); + if (HAL_MPU_ConfigRegion(&MPU_InitStruct) != HAL_OK) + { + // Handle error condition + // At minimum, log the error or set an error flag + // Consider system reset if MPU is critical for operation + } }targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h (1)
30-30: Consider documenting the memory model changesThis configuration change is part of a larger MPU rework. Consider adding documentation that explains:
- The new memory access model
- Cache coherency requirements
- Impact on DMA operations
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
128-129: Use defined constants forBaseAddressandSizeFor better maintainability and readability, consider using predefined constants instead of hardcoding values. You can use
SDRAM_STARTfor the base address and create a utility function to determine the MPU region size based onSDRAM_SIZE.Apply this diff to use the defined constants:
// Configure the MPU attributes for SDRAM MPU_InitStruct.Enable = MPU_REGION_ENABLE; - MPU_InitStruct.BaseAddress = 0xD0000000; - MPU_InitStruct.Size = MPU_REGION_SIZE_8MB; + MPU_InitStruct.BaseAddress = (uint32_t)SDRAM_START; + MPU_InitStruct.Size = GetMPURegionSize(SDRAM_SIZE);You'll need to implement the
GetMPURegionSizefunction to mapSDRAM_SIZEto the appropriateMPU_REGION_SIZEvalue.Example implementation:
MPU_Region_Size GetMPURegionSize(uint32_t size) { // Map the size (in bytes) to MPU region size definitions // For example: // 8MB -> MPU_REGION_SIZE_8MB // Implement logic to determine the correct MPU region size }
Line range hint
99-139: Consider handling unaligned access in code commentsThe extensive comment block from lines 104 to 123 provides valuable context about unaligned access issues. However, it duplicates information available in external documentation. Consider summarizing the key points and providing a reference link to keep the code concise.
Apply this diff to condense the comments:
- // [Extensive comments about unaligned access and hard faults] + // Configure MPU to treat SDRAM as Normal Memory to allow unaligned access. + // Reference: https://www.keil.com/support/docs/3777.htm
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c(1 hunks)targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c(3 hunks)
🔇 Additional comments (19)
targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c (2)
1-10: LGTM! Headers and includes are appropriate.
The file includes all necessary headers for MPU configuration in a ChibiOS environment.
11-11: Verify the external function implementation.
Let's ensure the external function Target_ExternalMemoryConfigMPU is properly implemented.
✅ Verification successful
Let me check the implementation details of this function to ensure it's properly implemented.
Let me check the complete implementation as it seems to be truncated.
Let me check one final part to see the complete MPU configuration.
External function is properly implemented
The function Target_ExternalMemoryConfigMPU is correctly implemented in targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c. It configures the MPU attributes for SDRAM with proper memory protection settings, including:
- Well-documented explanation of STM32F7 unaligned memory access handling
- Correct MPU region configuration for SDRAM at 0xD0000000
- Proper access permissions, caching, and execution settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of Target_ExternalMemoryConfigMPU
echo "Searching for Target_ExternalMemoryConfigMPU implementation..."
rg -p "void\s+Target_ExternalMemoryConfigMPU\s*\(" --type c
Length of output: 744
Script:
#!/bin/bash
# Get the implementation details from target_external_memory.c
echo "Implementation in ORGPAL_PALTHREE:"
rg -A 10 "void\s+Target_ExternalMemoryConfigMPU\s*\(\)" targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c
Length of output: 860
Script:
#!/bin/bash
# Get the complete implementation with more context lines
echo "Complete implementation in ORGPAL_PALTHREE:"
rg -A 30 "void\s+Target_ExternalMemoryConfigMPU\s*\(\)" targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c
Length of output: 1963
Script:
#!/bin/bash
# Get the complete implementation with even more context lines
echo "Complete implementation in ORGPAL_PALTHREE:"
rg -A 50 "void\s+Target_ExternalMemoryConfigMPU\s*\(\)" targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c
Length of output: 2455
targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c (2)
13-25: 🛠️ Refactor suggestion
Consider improving safety and maintainability of MPU configuration.
Several improvements could enhance the robustness of this function:
- The magic numbers and register values should be documented or defined as constants
- Consider adding validation to ensure MPU configuration was successful
- The function might need synchronization primitives to prevent concurrent access
Consider applying these improvements:
+// MPU memory region definitions
+#define MPU_SRAM_BASE_ADDRESS 0x20000000U
+#define MPU_SRAM_REGION_SIZE MPU_RASR_SIZE_128K
+#define MPU_REGION_SRAM MPU_REGION_1
+
void Target_ConfigNonCacheableMemory()
{
+ // Verify MPU is present and enabled
+ if ((MPU->TYPE & MPU_TYPE_DREGION_Msk) == 0) {
+ // Handle error - no MPU regions available
+ return;
+ }
+
// region
- MPU->RNR = ((uint32_t)MPU_REGION_1);
+ MPU->RNR = MPU_REGION_SRAM;
// base address
- MPU->RBAR = ((uint32_t)0x20000000U);
+ MPU->RBAR = MPU_SRAM_BASE_ADDRESS;
// size and other configs
MPU->RASR =
- ((uint32_t)MPU_RASR_ATTR_AP_RW_RW | MPU_RASR_ATTR_NON_CACHEABLE | MPU_RASR_ATTR_S | MPU_RASR_SIZE_128K |
+ ((uint32_t)MPU_RASR_ATTR_AP_RW_RW | MPU_RASR_ATTR_NON_CACHEABLE | MPU_RASR_ATTR_S | MPU_SRAM_REGION_SIZE |
MPU_RASR_ENABLE);
+
+ // Verify configuration
+ if ((MPU->RASR & MPU_RASR_ENABLE) == 0) {
+ // Handle error - region not enabled
+ return;
+ }
}Let's verify if similar MPU configurations exist in other targets:
27-40: 🛠️ Refactor suggestion
Add error handling and minimize MPU disabled window.
The current implementation has a few potential issues:
- No error handling for configuration failures
- MPU is disabled for longer than necessary
- No memory barriers to ensure configuration is complete
Consider applying these improvements:
+// Status codes for MPU configuration
+typedef enum {
+ MPU_CONFIG_SUCCESS = 0,
+ MPU_CONFIG_ERROR_EXTERNAL = -1,
+ MPU_CONFIG_ERROR_NONCACHE = -2
+} mpu_config_status_t;
+
-void Target_ConfigMPU()
+mpu_config_status_t Target_ConfigMPU()
{
+ mpu_config_status_t status = MPU_CONFIG_SUCCESS;
+
// disable MPU
HAL_MPU_Disable();
- // config MPU for external memory
- Target_ExternalMemoryConfigMPU();
+ __DSB();
+ __ISB();
- // config MPU for non cacheable memory
- Target_ConfigNonCacheableMemory();
+ // Minimize window where MPU is disabled by preparing configuration first
+ status = Target_ExternalMemoryConfigMPU();
+ if (status == MPU_CONFIG_SUCCESS) {
+ Target_ConfigNonCacheableMemory();
+ }
// enable MPU
HAL_MPU_Enable(MPU_PRIVILEGED_DEFAULT);
+
+ __DSB();
+ __ISB();
+
+ return status;
}Let's verify the MPU configuration sequence in other targets:
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt (1)
21-22: LGTM! Verify the new source file exists.
The addition of target_mpu_config.c aligns with the PR objectives for reworking MPU configuration. The placement after target_external_memory.c is logical as they work together to handle external memory configuration.
Let's verify the existence of the new source file:
✅ Verification successful
Verified: MPU configuration file exists with required functions
The target_mpu_config.c file exists in the correct location and contains the expected MPU configuration functions:
Target_ConfigMPU()which handles the overall MPU configurationTarget_ConfigNonCacheableMemory()which configures non-cacheable memory regions
The implementation looks correct with proper MPU register configurations and follows the expected pattern for memory protection setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that target_mpu_config.c exists in the correct location
# and contains the expected MPU configuration functions
# Check if the file exists
fd "target_mpu_config.c" --type f
# Verify the file contains the expected MPU configuration functions
ast-grep --pattern 'void Target_ConfigMPU(void) {
$$$
}'
ast-grep --pattern 'void Target_ConfigNonCacheableMemory(void) {
$$$
}'
Length of output: 270
Script:
#!/bin/bash
# Let's check the content of the specific target_mpu_config.c file for PALTHREE
cat targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c
# Also check if there are any references to MPU configuration functions in header files
rg -l "Target_ConfigMPU|Target_ConfigNonCacheableMemory" --type h
Length of output: 1072
targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt (1)
21-21: LGTM! Consider adding a descriptive comment.
The addition of target_mpu_config.c aligns with the PR objectives for supporting unaligned access to external memory.
Let's verify the existence of the new source file:
Consider adding a descriptive comment similar to the one for target_external_memory.c:
# the next one is required is the target implements and it's using external memory
${CMAKE_CURRENT_SOURCE_DIR}/target_external_memory.c
+# MPU configuration for unaligned access to external memory
${CMAKE_CURRENT_SOURCE_DIR}/target_mpu_config.c✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information to verify the review comment:
LGTM! The target_mpu_config.c file exists and contains the required MPU configuration.
The file exists at targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c and contains the necessary MPU configuration code for handling unaligned access to external memory, including:
- Proper MPU region configuration for external memory via
Target_ExternalMemoryConfigMPU() - Non-cacheable memory configuration via
Target_ConfigNonCacheableMemory() - Correct MPU enable/disable sequences
The implementation matches the one in ORGPAL_PALTHREE target and follows best practices for STM32F7 MPU configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of target_mpu_config.c and check its contents
# for MPU-related configurations
# Check if the file exists
fd "target_mpu_config.c" --type f
# Look for MPU-related configurations in the file
rg -A 5 "MPU" --type c
Length of output: 54274
targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c (2)
22-22: LGTM! Function declaration is clear and well-placed.
The external declaration of Target_ConfigMPU() follows proper conventions and is appropriately placed with other external function declarations.
97-99: Verify MPU configuration timing and unaligned access support.
The placement of Target_ConfigMPU() is strategically correct - after system initialization but before external memory configuration. However, please verify:
- The MPU configuration properly enables unaligned access as intended
- The configuration remains valid across all reset scenarios
Let's verify the MPU configuration implementation:
✅ Verification successful
Based on the search results, I can now verify the MPU configuration implementation. Let me generate the final response:
MPU configuration is properly implemented and safe for all reset scenarios
The MPU configuration is correctly implemented with:
- Proper sequence in boot process - called before external memory initialization but after basic system init
- Robust implementation that handles unaligned access:
- Configures MPU for external memory with correct access permissions
- Enables instruction and data cache access
- Sets up proper memory attributes for SDRAM region
The configuration remains valid across reset scenarios as:
- MPU is reconfigured on each boot
- Reset handlers properly reinitialize the system state
- Memory protection settings are restored after any system reset
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MPU configuration in the target implementation
ast-grep --pattern 'void Target_ConfigMPU() {
$$$
}'
# Check for any MPU-related constants or settings
rg -l "MPU" --type c --type h
# Look for reset handling code that might affect MPU
rg "MPU|Reset" -A 5 -B 5 --type c --type h
Length of output: 104322
targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c (1)
97-99: Consider adding error handling for MPU configuration.
The MPU configuration is a critical system setup step. While the placement in the initialization sequence is correct (before external memory init), consider:
- Adding error handling to detect and handle MPU configuration failures
- Verifying that the configuration was applied successfully
Let's verify the MPU configuration implementation:
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (3)
72-72: LGTM: Function declaration follows conventions
The declaration is well-placed and follows the project's naming conventions.
Line range hint 102-126: Excellent documentation of the unaligned access issue
The comments provide valuable context by:
- Referencing official ARM documentation
- Explaining the symptom and cause
- Detailing the memory mapping specifics
127-142: Consider optimizing MPU configuration
While the basic configuration is correct, consider these improvements:
- Execution permission might be unnecessary for SDRAM
- Verify TEX level 1 is optimal for external SDRAM
Consider this modification:
- MPU_InitStruct.DisableExec = MPU_INSTRUCTION_ACCESS_ENABLE;
+ MPU_InitStruct.DisableExec = MPU_INSTRUCTION_ACCESS_DISABLE;Let's verify the MPU settings across other STM32F7 targets:
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (1)
128-129:
Critical: Address and size configuration mismatch
There are several inconsistencies in the memory configuration:
- The base address (0xD0000000) doesn't match the documented SDRAM range (0xC0000000-0xC03FFFFF)
- The configured size (8MB) exceeds the documented maximum (4MB)
Please verify these settings against your hardware specifications.
targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h (2)
Line range hint 1-24: Configuration is well-structured and consistent
The overall configuration shows good alignment with the project's objectives:
- Proper support for unaligned access
- Consistent IRQ priority assignments
- Well-organized DMA stream allocations
30-30: Verify cache behavior with external memory access
The change to disable no-cache (STM32_NOCACHE_ENABLE FALSE) aligns with the PR objective to rework MPU configuration. However, this change requires careful verification of memory access patterns.
✅ Verification successful
Let me gather more specific information about the cache and MPU configurations in the target platform.
Cache configuration is properly aligned with MPU settings
The verification shows that the STM32_NOCACHE_ENABLE FALSE setting is consistent with the MPU configuration where external memory is explicitly set as cacheable (MPU_ACCESS_CACHEABLE). This alignment between MCU configuration and MPU settings indicates proper cache behavior implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for memory access patterns and cache-related configurations
# that might be affected by this change
# Check for direct memory access operations
echo "Checking for DMA operations..."
rg -l "DMA" --type c --type cpp --type h
# Look for cache management functions
echo "Checking for cache management..."
ast-grep --pattern 'SCB_CleanDCache$$$'
ast-grep --pattern 'SCB_InvalidateDCache$$$'
# Search for MPU configuration
echo "Checking for MPU configurations..."
rg -l "MPU_Config" --type c --type cpp --type h
Length of output: 6926
Script:
#!/bin/bash
# Check the MPU configuration implementation for ORGPAL_PALX
echo "Checking MPU configuration in ORGPAL_PALX..."
rg -A 10 "MPU_Config" targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
# Check for cache-related operations specifically in external memory configuration
echo "Checking cache operations in external memory setup..."
rg -i "cache|clean|invalidate|SCB" targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
# Look for other cache configuration settings
echo "Checking other cache-related settings..."
rg "NOCACHE|CACHE" targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h
Length of output: 691
targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h (2)
Line range hint 249-253: SDC configuration complements MPU changes.
The SDC configuration with STM32_SDC_SDMMC_UNALIGNED_SUPPORT = TRUE aligns well with the PR's objective to support unaligned access. This ensures consistent handling of unaligned access across both external memory and SD card interfaces.
32-32: Memory configuration change aligns with MPU rework.
The change to disable no-cache (STM32_NOCACHE_ENABLE = FALSE) is consistent with the PR's objective to rework MPU configuration for unaligned access to external memory. The caching behavior will now be managed by the MPU configuration instead.
Let's verify the memory access configuration across the codebase:
✅ Verification successful
Memory configuration change is correctly aligned with MPU setup
The change to disable no-cache (STM32_NOCACHE_ENABLE = FALSE) is verified to be correct as:
-
The MPU is explicitly configured in
target_mpu_config.cto handle memory regions with specific cache attributes:- Non-cacheable region is configured for 128KB at 0x20000000
- External SDRAM is configured as cacheable (MPU_ACCESS_CACHEABLE) at 0xD0000000
-
The memory initialization sequence in
main.cshows proper ordering:- MPU configuration is set up before external memory initialization
- Target_ConfigMPU() is called which configures both non-cacheable and external memory regions
-
The external memory configuration in
target_external_memory.cshows proper handling of unaligned access concerns through MPU configuration, as evidenced by the detailed comments about STM32F7 unaligned access handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related memory configuration settings and their usage
# Search for MPU configuration and memory access patterns
echo "Checking MPU configuration files..."
rg -l "MPU|Memory Protection Unit"
# Search for cache-related configurations
echo "Checking cache-related settings..."
rg "NOCACHE|MPU_NOCACHE|CACHE_MAINTENANCE"
# Look for external memory configuration
echo "Checking external memory configuration..."
rg "EXTERNAL_RAM|EXT_RAM|TARGET_EXTERNAL"
Length of output: 8765
Script:
#!/bin/bash
# Let's check the MPU configuration and external memory settings specifically for ORGPAL_PALTHREE
# Check MPU configuration in target_mpu_config.c
echo "=== MPU Configuration ==="
cat targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c
# Check external memory configuration
echo -e "\n=== External Memory Configuration ==="
cat targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c
# Check main.c for memory initialization
echo -e "\n=== Memory Initialization in main.c ==="
cat targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c
Length of output: 12406
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
131-133:
Ensure MPU attributes correctly define Normal Memory
The MPU attributes set on lines 131 to 133 configure the SDRAM region. To allow unaligned access and optimal performance, the SDRAM should be configured as Normal Memory. Verify that the combination of TypeExtField, IsCacheable, IsBufferable, and IsShareable correctly sets the memory type to Normal Memory with appropriate caching strategies.
Refer to the ARM Cortex-M7 MPU documentation to confirm the attribute settings. The memory attributes should reflect the desired behavior for SDRAM.
Line range hint 99-139: Verify the correctness of BaseAddress and SDRAM mapping
The BaseAddress set on line 128 is 0xD0000000, but in the comments above, SDRAM is mapped to the address range 0xC0000000 - 0xC03FFFFF. Please verify that 0xD0000000 is the correct starting address for the external SDRAM on the ORGPAL_PALX target.
Run the following script to check the SDRAM base address definitions:
✅ Verification successful
The SDRAM base address 0xD0000000 is correct for ORGPAL_PALX target
The code is correct. While the comments mention 0xC0000000 - 0xC03FFFFF, they are generic documentation about STM32F7 devices. The actual SDRAM configuration for ORGPAL_PALX target uses Bank6 which is correctly mapped to 0xD0000000:
- The target defines
SDRAM_STARTasFSMC_Bank6_MAP_BASE FSMC_Bank6_MAP_BASEis defined as0xD0000000inhal_stm32_fsmc.h- This is consistent across multiple files:
target_common.h.in:RAM1_MEMORY_StartAddressis0xD0000000- Linker scripts: External SDRAM is mapped to
0xD0000000 - Other ORGPAL targets also use the same address
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SDRAM base address definitions in the codebase.
# Search for SDRAM base address definitions
rg 'SDRAM_START|FSMC_Bank6_MAP_BASE'
# Search for hardcoded addresses matching 0xD0000000 or 0xC0000000
rg '0xD0000000|0xC0000000'
Length of output: 8534
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c (2)
11-11: Add documentation for the external function declaration.Consider adding documentation to explain the purpose and behavior of
Target_ExternalMemoryConfigMPU().+// Configures MPU regions for external memory access extern void Target_ExternalMemoryConfigMPU();
27-40: Add error handling and ensure thread safety during MPU configuration.While the configuration sequence is correct, consider these improvements:
- Add error handling for configuration failures
- Ensure thread safety during configuration
- Document the expected behavior of external memory configuration
Here's a suggested improvement:
+__attribute__((used)) // Prevent optimization removal void Target_ConfigMPU() { + // Save and disable interrupts during MPU configuration + chSysLock(); + // disable MPU HAL_MPU_Disable(); // config MPU for external memory Target_ExternalMemoryConfigMPU(); // config MPU for non cacheable memory Target_ConfigNonCacheableMemory(); // enable MPU HAL_MPU_Enable(MPU_PRIVILEGED_DEFAULT); + + // Restore interrupts + chSysUnlock(); }targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c (3)
11-11: Add documentation for the external function declaration.Consider adding documentation to explain the purpose, requirements, and expected behavior of
Target_ExternalMemoryConfigMPU.+// Configures MPU regions for external memory access +// Must be called before enabling MPU extern void Target_ExternalMemoryConfigMPU();
13-25: Consider improving robustness and documentation of MPU configuration.Several improvements could enhance the maintainability and reliability of this function:
- The magic numbers and MPU configuration values should be documented or defined as constants
- Consider adding error handling for MPU configuration failures
+// Base address for SRAM1 in STM32F7 +#define SRAM1_BASE_ADDRESS 0x20000000U + +// MPU region size (128KB matches SRAM1 size) +#define MPU_REGION_SIZE_SRAM1 MPU_RASR_SIZE_128K + void Target_ConfigNonCacheableMemory() { + // Configure MPU region 1 for non-cacheable SRAM1 access // region MPU->RNR = ((uint32_t)MPU_REGION_1); // base address - MPU->RBAR = ((uint32_t)0x20000000U); + MPU->RBAR = SRAM1_BASE_ADDRESS; // size and other configs MPU->RASR = - ((uint32_t)MPU_RASR_ATTR_AP_RW_RW | MPU_RASR_ATTR_NON_CACHEABLE | MPU_RASR_ATTR_S | MPU_RASR_SIZE_128K | + ((uint32_t)MPU_RASR_ATTR_AP_RW_RW | MPU_RASR_ATTR_NON_CACHEABLE | MPU_RASR_ATTR_S | MPU_REGION_SIZE_SRAM1 | MPU_RASR_ENABLE); }
1-40: Consider implementing MPU configuration validation.To ensure system stability, consider adding a validation function that verifies the MPU configuration after setup. This could include:
- Checking if all required regions are properly configured
- Validating memory access permissions
- Testing memory access patterns
This would help catch any configuration issues early in the boot process before they can cause system instability.
targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c (1)
97-99: Consider documenting migration steps for breaking changes.Since this MPU configuration change is a breaking change that affects memory access patterns, consider:
- Documenting the new memory access requirements and limitations
- Providing migration guidelines for existing applications
- Adding debug assertions or runtime checks for detecting misaligned access during development
targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h (1)
32-32: Architecture recommendation for cache configurationThe cache configuration is now handled through MPU, which is a more flexible approach. Consider documenting the following in the codebase:
- The relationship between
STM32_NOCACHE_ENABLEand MPU configuration- Which memory regions are cached/non-cached
- The impact on performance and memory access patterns
Also applies to: 396-396
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
Line range hint
99-139: Align SDRAM Base Address in MPU Configuration with Actual SDRAM AddressThe comments mention the SDRAM address range as
0xC0000000 - 0xC03FFFFF, but in the MPU configuration at line 128,MPU_InitStruct.BaseAddressis set to0xD0000000. Ensure that the base address in the MPU configuration matches the actual SDRAM address range to prevent potential access violations and hard faults.Suggested fixes:
Option 1: If the SDRAM is actually mapped to
0xC0000000, update theBaseAddressin the MPU configuration:- MPU_InitStruct.BaseAddress = 0xD0000000; + MPU_InitStruct.BaseAddress = 0xC0000000;Option 2: If the SDRAM is correctly mapped to
0xD0000000, update the comments to reflect the correct address range to avoid confusion:- // located into the SDRAM address range 0xC0000000 - 0xC03FFFFF (max. 4MB). + // located into the SDRAM address range 0xD0000000 - 0xD07FFFFF (max. 8MB).Ensure that the
MPU_InitStruct.Sizematches the actual size of the SDRAM in use.
Line range hint
99-123: Refine Comments for Clarity and RelevanceThe extensive comments from lines 99 to 123 provide detailed information about unaligned memory access issues. While informative, consider summarizing the comments to focus on how the issue relates specifically to this code and the implemented solution. This will improve readability and maintainability.
Suggested approach:
- Summarize the problem and solution in a few concise sentences.
- Provide a reference link for those who need more detailed information.
Example:
- // [Existing detailed comments] + // Workaround for hard faults caused by unaligned memory access to SDRAM on STM32F7xx devices. + // Configuring the MPU for the SDRAM region allows unaligned access without faults. + // Reference: https://www.keil.com/support/docs/3777.htmtargets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (2)
72-72: Consider function scope adjustmentThe function
Target_ExternalMemoryConfigMPU()appears to be used only within this file. Consider declaring it asstaticto limit its scope and prevent potential naming conflicts.
Line range hint
102-126: Update comment to reflect correct SDRAM size and address rangeThe comment mentions the SDRAM address range as
0xC0000000 - 0xC03FFFFF (max. 4MB), which does not match the actual SDRAM size of 16MB (0xC0000000 - 0xC0FFFFFF). Updating the address range in the comment will improve clarity and accuracy.Apply this change to the comment:
-// located into the SDRAM address range 0xC0000000 - 0xC03FFFFF (max. 4MB). +// located into the SDRAM address range 0xC0000000 - 0xC0FFFFFF (max. 16MB).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c(1 hunks)targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c(3 hunks)
🔇 Additional comments (19)
targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c (2)
1-10: LGTM! Headers and includes are appropriate.
The file includes all necessary headers for MPU configuration and follows project conventions.
1-40: Verify consistent MPU configuration across targets.
Since this is part of a larger change for unaligned access support, let's verify the implementation consistency across different targets.
✅ Verification successful
Let me gather more information about the implementation details to ensure consistency.
MPU configuration implementation is consistent across targets
The verification shows that the MPU configuration is properly implemented and consistent:
- Both ORGPAL targets (PALTHREE and PALX) share identical MPU configuration code for non-cacheable memory (128KB region)
- External memory MPU configuration is consistently implemented across STM32F7 targets with proper handling of unaligned access issues
- MPU initialization is properly integrated in the startup sequence for all targets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check MPU configuration implementations across targets
# Find all MPU configuration files
echo "=== MPU Configuration Files ==="
fd "target_mpu_config[.]c$" targets/
# Check for similar MPU region configurations
echo -e "\n=== MPU Region Configurations ==="
rg "MPU->RNR.*REGION_1" targets/
# Check for external memory configuration patterns
echo -e "\n=== External Memory Configuration Patterns ==="
rg "Target_ExternalMemoryConfigMPU" targets/
# Check for MPU initialization in startup sequences
echo -e "\n=== MPU Initialization Patterns ==="
rg "Target_ConfigMPU" targets/
Length of output: 2103
Script:
#!/bin/bash
# Check implementation details of MPU configuration
# Compare MPU configuration implementations
echo "=== MPU Configuration Implementations ==="
for file in $(fd "target_mpu_config[.]c$" targets/); do
echo -e "\n=== Content of $file ===\n"
cat "$file"
done
# Check external memory configuration implementations
echo -e "\n=== External Memory Configuration Implementations ==="
for file in $(fd "target_external_memory[.]c$" targets/ChibiOS/); do
echo -e "\n=== Content of $file ===\n"
cat "$file"
done
# Check MPU region size configurations
echo -e "\n=== MPU Region Size Configurations ==="
rg "MPU_RASR_SIZE" targets/
Length of output: 29399
targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c (2)
1-10: LGTM! Headers are appropriate for MPU configuration.
The necessary headers for STM32, ChibiOS, and community HAL are properly included.
27-40: Verify MPU configuration sequence against ST documentation.
The function follows the correct sequence for MPU configuration, but consider adding error handling and documentation for privileged access settings.
+/**
+ * Configures the Memory Protection Unit (MPU) for the STM32F7.
+ *
+ * This function performs the following steps:
+ * 1. Disables the MPU
+ * 2. Configures external memory regions
+ * 3. Configures non-cacheable memory regions
+ * 4. Enables the MPU with privileged access
+ *
+ * @note Must be called before accessing external memory
+ * @return None
+ */
void Target_ConfigMPU()
{
// disable MPU
HAL_MPU_Disable();
+
+ // Ensure MPU is successfully disabled
+ if ((MPU->CTRL & MPU_CTRL_ENABLE_Msk) != 0) {
+ // Handle error or retry
+ return;
+ }
// config MPU for external memory
Target_ExternalMemoryConfigMPU();
// config MPU for non cacheable memory
Target_ConfigNonCacheableMemory();
// enable MPU
HAL_MPU_Enable(MPU_PRIVILEGED_DEFAULT);
+
+ // Ensure MPU is successfully enabled
+ if ((MPU->CTRL & MPU_CTRL_ENABLE_Msk) == 0) {
+ // Handle error or retry
+ return;
+ }
}targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt (1)
21-22: LGTM! Verify new source file exists.
The addition of target_mpu_config.c to CLR_EXTRA_SOURCE_FILES is correct and aligns with the PR's objective to rework MPU configuration. The placement after target_external_memory.c is logical as they're both related to memory management.
Let's verify the existence of the new source file:
✅ Verification successful
✓ Source file exists and is properly placed
The verification confirms that target_mpu_config.c exists in the correct location at targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c. The CMake integration is valid and the build system will be able to locate the file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that target_mpu_config.c exists in the correct location
# Expected: File should exist in the ORGPAL_PALTHREE target directory
fd --type f "target_mpu_config.c" --exec ls -l {}
Length of output: 251
targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt (1)
21-21: LGTM! Addition of MPU configuration source file.
The addition of target_mpu_config.c aligns with the PR objectives to support unaligned access to external memory for ORGPAL targets.
Let's verify the existence of the new source file:
✅ Verification successful
Let me verify the implementation of the MPU configuration source file for ORGPAL_PALX target.
Let me check the implementation of the external memory MPU configuration that's referenced in this file.
Verified: MPU configuration implementation is complete and correct
The target_mpu_config.c file contains a proper implementation that:
- Configures external memory MPU settings to handle unaligned access
- Sets up non-cacheable memory region
- Properly sequences MPU disable/enable operations
The implementation matches the PR objectives for reworking MPU configuration to support unaligned access to external memory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the MPU configuration source file
# and check its implementation
# Check if the file exists
fd "target_mpu_config.c" --type f
# If found, examine its content for MPU configuration
rg -A 5 "MPU|Memory Protection Unit"
Length of output: 60924
Script:
#!/bin/bash
# Check the content of target_mpu_config.c for ORGPAL_PALX
cat targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
Length of output: 999
Script:
#!/bin/bash
# Check the content of target_external_memory.c for ORGPAL_PALX to verify MPU config
cat targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
Length of output: 6627
targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c (2)
97-99: Verify initialization sequence and timing dependencies.
The MPU configuration is correctly placed before external memory initialization, which is essential for preventing memory access violations. However, since this is a breaking change affecting memory access patterns, we should verify:
- No memory operations between watchdog init and MPU config require protected memory access
- External memory initialization properly waits for MPU configuration
✅ Verification successful
MPU configuration sequence is correctly placed and implemented
The verification confirms that:
- Between watchdog init and MPU config, only CRC initialization occurs which doesn't involve protected memory access
- The MPU configuration is consistently implemented across targets (ORGPAL_PALTHREE, ORGPAL_PALX) with proper sequencing:
- First disables MPU
- Configures external memory MPU settings
- Configures non-cacheable memory settings
- External memory initialization explicitly waits for MPU configuration as evidenced by the comment "this has to be called after osKernelInitialize, otherwise a hard fault will occur"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify initialization sequence across targets and potential memory access between watchdog and MPU config
# Check for any memory operations between watchdog and MPU initialization
echo "Checking for memory operations between watchdog and MPU config..."
rg -B 10 -A 10 "Target_ConfigMPU" --type c
# Verify external memory initialization dependencies
echo "Checking external memory initialization sequence..."
rg "Target_ExternalMemoryInit|Target_ConfigMPU" --type c
# Look for similar patterns in other targets
echo "Checking initialization sequence in other targets..."
fd -e c -e h . | xargs rg "ExternalMemoryInit|ConfigMPU|Memory Protection Unit"
Length of output: 13975
22-22: LGTM! Verify implementation details.
The function declaration follows proper conventions. However, since this is a critical MPU configuration function, let's verify its implementation.
✅ Verification successful
Based on the shell script results, I can now generate the final response as I have sufficient information about the MPU configuration implementation:
MPU configuration implementation verified and properly structured
The implementation follows a well-organized pattern across ORGPAL targets:
- Proper MPU initialization sequence in
target_mpu_config.c - Correct configuration for external memory access
- Appropriate handling of non-cacheable memory regions
The function declaration is correctly implemented in both ORGPAL_PALTHREE and ORGPAL_PALX targets with identical structure:
- Disabling MPU
- Configuring external memory MPU settings
- Setting up non-cacheable memory regions
- Re-enabling MPU with proper privileges
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Target_ConfigMPU implementation and its usage across the codebase
# Find the implementation file
echo "Searching for Target_ConfigMPU implementation..."
rg -p "Target_ConfigMPU" --type c
# Look for any MPU-related configurations in other targets
echo "Checking MPU configurations in other targets..."
fd -e c -e h . | xargs rg "MPU|Memory Protection Unit"
Length of output: 21785
targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c (2)
97-99: Strategic placement of MPU configuration
The MPU configuration is correctly placed:
- After kernel initialization and watchdog setup ensures system stability
- Before external memory initialization, which is essential as the MPU needs to be configured before any external memory access occurs
However, since this is a breaking change that modifies memory access patterns:
Consider adding error handling and validation:
// MPU configuration
-Target_ConfigMPU();
+if (Target_ConfigMPU() != HAL_SUCCESS) {
+ // Handle MPU configuration failure
+ Error_Handler();
+}Let's verify the impact on external memory initialization:
22-22: Verify MPU configuration implementation
The function declaration suggests a hardcoded MPU configuration. Let's verify the implementation to ensure it properly handles the unaligned access requirements.
✅ Verification successful
Let me check the implementation of the external memory configuration to ensure it properly handles unaligned access. Let me generate another script to verify this.
Let me check the complete implementation of the external memory MPU configuration to verify the unaligned access settings.
MPU configuration implementation is correct for handling unaligned access
The implementation properly configures the MPU for external memory access with appropriate settings:
- Addresses the known STM32F7 unaligned access issue
- Sets up cacheable and bufferable access for better performance
- Configures full access permissions with proper memory type (TEX Level 1)
- Base address and size are correctly set for the external memory region
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the Target_ConfigMPU implementation
rg -t c "void\s+Target_ConfigMPU\s*\(\s*\)" -A 10
Length of output: 3421
Script:
#!/bin/bash
# Search for the Target_ExternalMemoryConfigMPU implementation in PALX target
rg -t c "void\s+Target_ExternalMemoryConfigMPU\s*\(\s*\)" -A 20 targets/ChibiOS/ORGPAL_PALX/
Length of output: 3979
Script:
#!/bin/bash
# Get the complete implementation of Target_ExternalMemoryConfigMPU
rg -t c "void\s+Target_ExternalMemoryConfigMPU\s*\(\s*\)" -A 50 targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
Length of output: 2362
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)
99-140: LGTM! Well-documented MPU configuration for unaligned access
The implementation includes excellent documentation explaining the unaligned access issue and its solution. The MPU configuration appears correct with appropriate memory attributes for SDRAM access.
128-128: Verify the SDRAM base address mapping
The MPU is configured to map SDRAM at 0xD0000000, but the documentation mentions the problematic range as 0xC0000000-0xC03FFFFF. Please confirm if this is intentional and if the SDRAM is actually mapped to this new address range.
✅ Verification successful
Base address mapping is correct and consistent
The SDRAM base address of 0xD0000000 is correct and consistent across the target configuration. This is confirmed by:
- The target_common.h.in defines RAM1_MEMORY_StartAddress as 0xD0000000
- The linker script maps external SDRAM to 0xD0000000
- The MPU configuration in target_external_memory.c uses the same address
The documentation's mention of 0xC0000000-0xC03FFFFF likely refers to a different issue or target, as the ORGPAL_PALTHREE consistently uses 0xD0000000 for its SDRAM mapping.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SDRAM base address definitions across the codebase
rg -l "0xD0000000|0xC0000000" | grep -i "target\|memory\|mpu"
Length of output: 1519
Script:
#!/bin/bash
# Check the content of target_common.h.in and linker script for SDRAM mapping
cat targets/ChibiOS/ORGPAL_PALTHREE/target_common.h.in
echo "=== Linker Script ==="
cat targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/STM32F76xx_CLR.ld | grep -A 5 "SDRAM"
Length of output: 2230
targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h (3)
Line range hint 284-284: Confirm compatibility with SDMMC unaligned access
The configuration shows that SDMMC unaligned access support is enabled (STM32_SDC_SDMMC_UNALIGNED_SUPPORT), which complements the MPU changes for unaligned access support. This is a good alignment of features.
Line range hint 397-398: Verify MPU configurations in included files
The configuration includes additional settings from mcuconf_nf.h and mcuconf_community.h. These files might contain complementary MPU configurations that should be reviewed for consistency.
✅ Verification successful
MPU and cache configurations are consistent across included files
The verification shows that the included configuration files exist and contain only standard cache-related settings:
STM32_SRAM2_NOCACHEis set to FALSESTM32_NOCACHE_ENABLEis set to FALSE
These settings are consistent and don't indicate any conflicts or issues with memory protection or cache configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MPU-related configurations in included files
fd mcuconf_nf.h targets/ChibiOS/ORGPAL_PALX/ -x cat {} \; | rg -i "mpu|cache|memory"
fd mcuconf_community.h targets/ChibiOS/ORGPAL_PALX/ -x cat {} \; | rg -i "mpu|cache|memory"
Length of output: 186
Script:
#!/bin/bash
# First, check if these files exist in the specified directory
fd "mcuconf_(nf|community).h" targets/ChibiOS/ORGPAL_PALX/
# Check for any MPU-related configurations in all mcuconf files in the target directory
fd "mcuconf.*\.h" targets/ChibiOS/ORGPAL_PALX/ -x cat {} \; | rg -i "mpu|cache|memory"
# Also check for MPU configurations in the parent directories
fd "mcuconf_(nf|community).h" targets/ChibiOS/ -x cat {} \; | rg -i "mpu|cache|memory"
Length of output: 522
30-30: Verify cache behavior with peripherals
The change to disable STM32_NOCACHE_ENABLE aligns with the MPU rework for unaligned access. However, we should verify that all peripherals that require non-cached memory regions are properly configured in the new MPU setup.
✅ Verification successful
The search revealed multiple DMA-capable peripherals (ADC, DAC, I2C, SPI, UART, QSPI) and the presence of MPU configuration files. Let's examine their implementations to verify proper handling of non-cacheable regions for DMA operations.
MPU configuration properly handles non-cacheable regions
The verification shows that the MPU configuration is correctly implemented:
- Non-cacheable region is configured at 0x20000000 with 128KB size for DMA operations
- External SDRAM is properly configured with cacheable attributes at 0xD0000000
- MPU initialization occurs before any peripheral usage in the correct sequence
The change to disable STM32_NOCACHE_ENABLE is safe because:
- DMA-capable peripherals are protected by the MPU configuration in
Target_ConfigNonCacheableMemory() - The implementation uses a dedicated MPU region (Region 1) with non-cacheable attributes
- The configuration is applied early in the boot sequence before any peripheral initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DMA-capable peripherals that might require non-cached memory
rg "DMA_STREAM" targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h
# Look for any MPU configuration in the new implementation
rg -l "Target_ConfigNonCacheableMemory|ConfigMPU" targets/ChibiOS/ORGPAL_PALX/
Length of output: 3238
Script:
#!/bin/bash
# Check the MPU configuration implementation for DMA buffers
cat targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
# Check external memory configuration
cat targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
# Check main.c for initialization sequence
cat targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c
Length of output: 12147
targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h (1)
32-32: Verify cache behavior with external memory access
The change to disable no-cache mode (STM32_NOCACHE_ENABLE set to FALSE) aligns with the PR's objective to rework MPU configuration for unaligned access. This change implies that caching will be managed through the MPU configuration instead.
✅ Verification successful
Cache configuration change is properly handled by MPU configuration
The verification confirms that the change to disable STM32_NOCACHE_ENABLE is safe as the non-cacheable memory regions are now properly managed through the MPU configuration in Target_ConfigNonCacheableMemory(). The implementation correctly:
- Configures MPU Region 1 for non-cacheable memory at address 0x20000000
- Sets appropriate attributes including
MPU_RASR_ATTR_NON_CACHEABLE - Maintains read-write access permissions and proper size configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the MPU configuration implementation for cache management
# Look for the Target_ConfigNonCacheableMemory function that should now handle non-cached regions
# Search for the new function that handles non-cached memory regions
ast-grep --pattern 'void Target_ConfigNonCacheableMemory() {
$$$
}'
# Search for any cache-related configurations in target files
rg -l "MPU|CACHE|NOCACHE" "targets/ChibiOS/ORGPAL_PALTHREE/"
Length of output: 3089
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
135-137: Validate MPU TypeExtField and Access Permissions
At line 135, MPU_InitStruct.TypeExtField is set to MPU_TEX_LEVEL1. Verify that this setting, along with the bufferable, cacheable, and shareable attributes, correctly represents the memory type and access permissions for SDRAM according to the ARM Cortex-M7 MPU programming model.
Consider consulting the STM32F7 MPU configuration guidelines to ensure optimal performance and correct memory behavior. Incorrect settings might lead to issues like unexpected cache behavior or memory access faults.
134-134: Verify MPU Region Number Usage to Avoid Conflicts
At line 134, MPU_InitStruct.Number = MPU_REGION_NUMBER0; assigns MPU region number 0. Ensure that this region number is not used elsewhere in the codebase to prevent unintended overwriting of MPU settings.
Run the following script to check for other usages of MPU_REGION_NUMBER0:
If conflicts are found, consider using an unused MPU region number, such as MPU_REGION_NUMBER1.
✅ Verification successful
Let me gather more context about these MPU configurations to understand if they are mutually exclusive or if there's a potential conflict.
Let me check if these configurations are in different target-specific implementations and how they're used.
MPU Region 0 Usage is Safe Across Different Target Boards
The usage of MPU_REGION_NUMBER0 is correct and safe. Each occurrence is in a different target board's implementation (ST_STM32F769I_DISCOVERY, ORGPAL_PALX, and ORGPAL_PALTHREE), and these are mutually exclusive as only one target board can be built at a time. Each board correctly configures its SDRAM region using region 0, with appropriate board-specific base addresses (0xC0000000 for ST_STM32F769I_DISCOVERY and 0xD0000000 for ORGPAL boards).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other configurations using MPU_REGION_NUMBER0.
# Expectation: No other MPU configurations should use region number 0.
# Search the codebase for assignments to MPU_REGION_NUMBER0
rg -A2 "MPU_InitStruct\.Number\s*=\s*MPU_REGION_NUMBER0"
Length of output: 1023
Script:
#!/bin/bash
# Check the surrounding context of these MPU configurations to understand their usage
rg -B5 -A10 "MPU_InitStruct\.Number\s*=\s*MPU_REGION_NUMBER0"
Length of output: 3788
Script:
#!/bin/bash
# Check the function names and broader context where these MPU configurations are used
rg -B10 -A2 "MPU_InitStruct\.Number\s*=\s*MPU_REGION_NUMBER0" --context-separator="=========="
Length of output: 4209
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (1)
97-97: Verify the initialization order of MPU configuration
Calling Target_ExternalMemoryConfigMPU(); before fsmcSdramInit(); may be essential to configure the MPU correctly before initializing the SDRAM. Ensure that this order aligns with the hardware requirements and does not introduce any side effects.
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c
Outdated
Show resolved
Hide resolved
josesimoes
force-pushed
the
rework-mpu-config
branch
from
ec25d8a to
213c19c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (1)
134-134: Consider using a constant for the MPU region numberUsing a hardcoded region number (0) makes it harder to maintain and understand the MPU region allocation strategy.
+// MPU region number for external SDRAM +#define MPU_REGION_EXTERNAL_SDRAM MPU_REGION_NUMBER0 + void Target_ExternalMemoryConfigMPU() { MPU_Region_InitTypeDef MPU_InitStruct; // ... - MPU_InitStruct.Number = MPU_REGION_NUMBER0; + MPU_InitStruct.Number = MPU_REGION_EXTERNAL_SDRAM;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c(1 hunks)targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h
- targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c
- targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h
- targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
🧰 Additional context used
📓 Learnings (3)
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
🔇 Additional comments (9)
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
Line range hint 99-123: Well-documented implementation with clear explanation of the unaligned access issue!
The documentation thoroughly explains the STM32F7 hard fault issue with unaligned memory access, including symptoms, cause, and technical details. This is valuable for future maintenance and understanding the implementation.
127-137: Correct MPU configuration for external SDRAM!
The configuration properly addresses the unaligned access issue by:
- Setting appropriate memory attributes (non-bufferable, non-cacheable, non-shareable)
- Configuring correct permissions and size
- Disabling instruction execution for security
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (4)
72-72: LGTM: Clear and consistent function declaration.
The function name clearly indicates its purpose and follows the target-specific naming convention.
97-99: LGTM: Correct initialization sequence.
The sequence properly configures MPU before any SDRAM access, which is crucial for Cortex-M7 based devices to prevent hard faults from unaligned access.
127-142: LGTM: Comprehensive MPU configuration with proper security settings.
The MPU configuration:
- Correctly matches SDRAM size (16MB)
- Follows security best practices by disabling execution
- Sets appropriate memory attributes for external SDRAM
- Uses the ST HAL API for better maintainability
Line range hint 102-126: Excellent documentation of ARM requirements.
The detailed comments effectively explain the rationale behind the MPU configuration, referencing official ARM documentation and specific hardware requirements for STM32F7 devices.
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (3)
98-98: LGTM: Function updated to include MPU configuration.
The update to Target_ExternalMemoryInit() correctly maintains the initialization sequence.
139-139: Add error handling for MPU configuration.
Referencing previous review: The HAL_MPU_ConfigRegion call should include error handling to ensure the MPU is correctly configured.
128-128: Verify the base address configuration.
The configured base address (0xD0000000) differs from the documented SDRAM range (0xC0000000-0xC03FFFFF) in the comments. Please verify if this is intentional and update either the comments or the configuration accordingly.
✅ Verification successful
Base address configuration is correct
The base address of 0xD0000000 is correct as it matches the FSMC Bank6 mapping (STM32_SDRAM2_MAP_BASE) used in the configuration. The code and configuration are consistent:
- The mcuconf shows STM32_SDRAM_USE_FSMC_SDRAM2 is TRUE
- The fsmc_sdram_lld.h defines SDRAM2 mapping to FSMC_Bank6_MAP_BASE
- The MPU configuration at 0xD0000000 aligns with Bank6 mapping
The comment about 0xC0000000-0xC03FFFFF is a reference example from ARM documentation and not specific to this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SDRAM address configurations in other targets
rg -A 2 -B 2 "0x[CD][0-9A-F]000000.*SDRAM" --type c
Length of output: 53
Script:
#!/bin/bash
# Search for any memory address configurations in the target file
cat "targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c"
Length of output: 6644
Script:
#!/bin/bash
# Search for SDRAM or memory configurations in header files
fd -e h -e hpp . --exec rg -l "SDRAM|0x[CD][0-9A-F]000000"
Length of output: 868
Script:
#!/bin/bash
# Search for memory configurations in board files
fd "board" -e c -e h . --exec rg -l "SDRAM|0x[CD][0-9A-F]000000"
Length of output: 109
Script:
#!/bin/bash
# Check the FSMC bank mapping definitions
cat "targets/ChibiOS/_nf-overlay/os/hal/ports/STM32/LLD/FSMCv1/fsmc_sdram_lld.h"
Length of output: 4791
Script:
#!/bin/bash
# Check mcuconf for SDRAM configuration
cat "targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf_nf.h"
Length of output: 656
josesimoes
force-pushed
the
rework-mpu-config
branch
from
213c19c to
05317aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h (1)
Line range hint
20-36: Add missing include directives for new modules.The newly enabled modules (
HAL_SDRAM_MODULE_ENABLEDandHAL_CORTEX_MODULE_ENABLED) require their corresponding header includes.Add the following includes after the existing module includes:
#ifdef HAL_QSPI_MODULE_ENABLED #include "stm32f7xx_hal_qspi.h" #endif /* HAL_QSPI_MODULE_ENABLED */ + +#ifdef HAL_SDRAM_MODULE_ENABLED + #include "stm32f7xx_hal_sdram.h" +#endif /* HAL_SDRAM_MODULE_ENABLED */ + +#ifdef HAL_CORTEX_MODULE_ENABLED + #include "stm32f7xx_hal_cortex.h" +#endif /* HAL_CORTEX_MODULE_ENABLED */targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h (1)
Line range hint
1-24: Consider adding documentation about MPU configuration changesSince this file is part of significant changes to MPU configuration, it would be helpful to add a comment in the header section explaining the relationship between SDRAM enablement and MPU configuration for unaligned access.
Add documentation like this:
// // Copyright (c) .NET Foundation and Contributors // Portions Copyright (c) 2016 STMicroelectronics. All rights reserved. // See LICENSE file in the project root for full license information. // +// This configuration enables SDRAM module which is required for the MPU configuration +// that supports unaligned access to external memory on ORGPAL3 and ORGPALX targets. +//
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c(1 hunks)targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c(3 hunks)targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h
- targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c
- targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h
- targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
🧰 Additional context used
📓 Learnings (3)
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
🔇 Additional comments (16)
targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h (1)
17-17: LGTM! The new module enables align with PR objectives.
The addition of HAL_SDRAM_MODULE_ENABLED and HAL_CORTEX_MODULE_ENABLED is appropriate for:
- Supporting external memory (SDRAM) access
- Enabling MPU configuration through ST HAL API
Also applies to: 20-20
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (3)
8-8: LGTM: Necessary header inclusion for HAL MPU APIs
The STM32F7 HAL header is correctly included to access the MPU configuration APIs.
Line range hint 99-123: LGTM: Well-documented rationale for MPU configuration
The comments provide excellent context about the unaligned access issue in STM32F7 and reference the relevant ARM documentation. This helps future maintainers understand the purpose of the MPU configuration.
124-139: Verify MPU region configuration
The MPU configuration looks correct for allowing unaligned access, but please verify:
- The base address (0xD0000000) differs from the SDRAM_START constant (FSMC_Bank6_MAP_BASE)
- Using MPU_REGION_NUMBER0 might conflict with the background region
Let's verify the SDRAM memory mapping and MPU region usage:
✅ Verification successful
Let me gather more information about the SDRAM base address definition and memory mapping.
MPU configuration is correctly aligned with memory mapping
The verification confirms:
- The base address 0xD0000000 matches RAM1_MEMORY_StartAddress in target_common.h.in
- MPU_REGION_NUMBER0 is used consistently across ORGPAL targets and doesn't conflict since:
- It's the first configured region
- Region 1 is used for different purpose (littlefs)
- Background region is handled separately by MPU initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SDRAM memory mapping and MPU region usage across the codebase
# Check for FSMC_Bank6_MAP_BASE definition
rg -l "FSMC_Bank6_MAP_BASE|0xD0000000"
# Check for other MPU region number definitions/usage
rg "MPU_REGION_NUMBER[0-9]"
# Check for background region configuration
rg "MPU_REGION_NUMBER0|MPU_InitStruct.Number\s*=\s*0"
Length of output: 2219
Script:
#!/bin/bash
# Check for SDRAM base address definitions in target headers
rg -A 5 "FSMC_Bank6_MAP_BASE|SDRAM_BASE|0xD0000000" targets/ChibiOS/ORGPAL_PALX/target_common.h.in
# Check linker script memory mapping
cat targets/ChibiOS/ORGPAL_PALX/nanoCLR/STM32F76xx_CLR.ld | grep -A 10 "MEMORY"
# Check for any other MPU configurations in the same file
rg -A 10 "MPU_InitStruct" targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
Length of output: 1898
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (4)
7-8: LGTM: Required HAL header added
The addition of stm32f7xx_hal.h is appropriate for using the ST HAL MPU APIs.
72-72: LGTM: Clear function declaration
The function declaration follows the project's naming conventions and correctly declares the MPU configuration function.
97-99: LGTM: Correct initialization sequence
The sequence is properly ordered:
- MPU configuration
- SDRAM initialization
- SDRAM driver start
This ensures proper MPU setup before any SDRAM access.
127-142: LGTM: MPU configuration is correct and secure
The MPU configuration:
- Correctly matches SDRAM size (16MB)
- Properly disables code execution
- Uses appropriate memory attributes for external SDRAM
- Follows ARM guidelines for Device Memory Type
Let's verify the configuration matches other STM32F7 targets:
✅ Verification successful
MPU configuration is consistent with other STM32F7 targets
The verification confirms that the MPU configuration for ST_STM32F769I_DISCOVERY aligns with other STM32F7 targets:
- All targets consistently disable code execution with
MPU_INSTRUCTION_ACCESS_DISABLE - Memory attributes are uniform across targets: non-cacheable and non-bufferable
- Region sizes vary appropriately based on hardware (8MB for PALX/PALTHREE, 16MB for F769I)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MPU configurations in other STM32F7 targets
echo "Searching for MPU region configurations..."
rg "MPU_REGION_SIZE_|MPU_INSTRUCTION_ACCESS_" "targets/ChibiOS/"
echo "Searching for MPU memory attributes..."
rg "MPU_ACCESS_NOT_BUFFERABLE|MPU_ACCESS_NOT_CACHEABLE" "targets/ChibiOS/"
Length of output: 2587
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (4)
8-8: LGTM: Include change aligns with HAL usage
The change to include stm32f7xx_hal.h is appropriate as it provides the necessary HAL APIs for MPU configuration.
99-140: LGTM: Well-documented MPU configuration for unaligned access
The implementation correctly addresses the unaligned access issue for STM32F7 external memory:
- Comprehensive documentation explaining the architectural constraints
- Correct memory attributes for Device memory type (non-bufferable, non-cacheable, non-shareable)
- Proper security settings (instruction execution disabled)
139-139: 🛠️ Refactor suggestion
Add error handling for MPU configuration
The HAL_MPU_ConfigRegion call should include error handling to ensure the MPU is correctly configured.
- HAL_MPU_ConfigRegion(&MPU_InitStruct);
+ if (HAL_MPU_ConfigRegion(&MPU_InitStruct) != HAL_OK)
+ {
+ Error_Handler();
+ }Likely invalid or redundant comment.
128-128: Verify SDRAM base address configuration
The configured base address (0xD0000000) differs from the documented SDRAM range (0xC0000000-0xC03FFFFF) in the comments. Please verify if this is intentional for the ORGPAL3 target.
✅ Verification successful
Let me search for more context about the memory mapping.
Let me search for more context about the SDRAM configuration.
Based on the gathered information, I can now provide the final response:
SDRAM base address configuration is correct
The configured base address (0xD0000000) matches the target's memory configuration as evidenced by:
- The linker script defines external SDRAM at 0xD0000000 with 8MB size
- The target_common.h.in defines RAM1_MEMORY_StartAddress as 0xD0000000
- The documented range 0xC0000000-0xC03FFFFF in comments refers to the general ARM architecture memory map, not the specific board configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SDRAM base address definitions
rg -A 2 "SDRAM.*BASE|FSMC.*BASE|FMC.*BASE" targets/ChibiOS/ORGPAL_PALTHREE/
Length of output: 342
Script:
#!/bin/bash
# Search for memory mapping definitions and configurations
rg -l "FSMC_Bank6_MAP_BASE|0xD0000000|0xC0000000" targets/ChibiOS/ORGPAL_PALTHREE/
# Check board configuration files
fd -e h "board" targets/ChibiOS/ORGPAL_PALTHREE/ -x cat {}
Length of output: 113508
Script:
#!/bin/bash
# Search for SDRAM configuration and memory mapping
rg -A 5 "SDRAM.*CONFIG|FSMC.*MAP|FMC.*MAP|0xD0000000|0xC0000000" targets/ChibiOS/ORGPAL_PALTHREE/
# Check linker script for memory regions
cat targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/STM32F76xx_CLR.ld
Length of output: 10324
targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h (2)
37-37: LGTM! Enabling SDRAM module is necessary for external memory access.
The enablement of HAL_SDRAM_MODULE_ENABLED aligns with the PR's objective to support unaligned access to external memory.
Let's verify that all required dependencies for SDRAM operation are enabled:
✅ Verification successful
Required HAL modules are properly enabled for SDRAM operation
Both HAL_DMA_MODULE_ENABLED and HAL_CORTEX_MODULE_ENABLED are defined in the configuration, which are necessary dependencies for SDRAM operation. This confirms that enabling the SDRAM module is safe and properly supported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SDRAM dependencies in HAL configuration
# Expected: Should find HAL_DMA_MODULE_ENABLED and HAL_CORTEX_MODULE_ENABLED
echo "Checking for required HAL modules..."
rg "^#define HAL_(DMA|CORTEX)_MODULE_ENABLED" targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h
Length of output: 249
Line range hint 146-148: Verify SDRAM callback configuration
The SDRAM callback registration is disabled (USE_HAL_SDRAM_REGISTER_CALLBACKS = 0U). This is generally fine for basic SDRAM operations, but let's verify this aligns with the MPU configuration requirements.
✅ Verification successful
SDRAM callback configuration aligns with MPU setup
The disabled SDRAM callbacks (USE_HAL_SDRAM_REGISTER_CALLBACKS = 0U) are consistent with the target's MPU configuration. The SDRAM is properly configured with:
- Non-cacheable, non-bufferable, non-shareable memory attributes
- Full access permissions
- Disabled instruction execution
- Proper MPU region setup (Region 0) for SDRAM at 0xD0000000
This configuration follows STM32F7 best practices for external SDRAM access and doesn't require callback registration for normal operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any other targets use SDRAM callbacks
# This helps verify our configuration aligns with other implementations
echo "Checking SDRAM callback usage across targets..."
rg "USE_HAL_SDRAM_REGISTER_CALLBACKS\s+1" targets/ChibiOS/
Length of output: 165
Script:
#!/bin/bash
# Let's check for SDRAM-related configurations and MPU settings in the target
# First, check for any SDRAM callback configurations (including 0)
echo "Checking all SDRAM callback configurations..."
rg "USE_HAL_SDRAM_REGISTER_CALLBACKS" targets/ChibiOS/
# Check for SDRAM and MPU related configurations
echo -e "\nChecking SDRAM and MPU configurations..."
rg -l "SDRAM|FMC_SDRAM|MPU" targets/ChibiOS/ORGPAL_PALTHREE/
# If we find relevant files, let's examine their content
echo -e "\nChecking MPU configuration details..."
rg "MPU" targets/ChibiOS/ORGPAL_PALTHREE/ -A 3
# Check for SDRAM initialization or configuration code
echo -e "\nChecking SDRAM initialization patterns..."
ast-grep --pattern 'void SDRAM_Init'
Length of output: 9850
targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h (2)
Line range hint 109-112: LGTM: Optimal configuration for external memory access.
The enabled prefetch and ART (Adaptive Real-Time) accelerator settings are appropriate for optimizing external memory access performance:
- Prefetch will help reduce memory access latency
- ART accelerator will improve instruction fetching from external memory
34-38: Verify required dependencies for SRAM operation.
The SRAM module requires certain other HAL modules to be enabled for proper operation. Currently enabled modules that support SRAM operation:
- HAL_DMA_MODULE_ENABLED
- HAL_RCC_MODULE_ENABLED
- HAL_CORTEX_MODULE_ENABLED
However, GPIO module is commented out which might be needed depending on your SRAM configuration.
Let's check the SRAM usage patterns to determine if GPIO is required:
✅ Verification successful
GPIO module is correctly disabled for SRAM operation
The HAL configuration and GPIO setup for SRAM operation is correct as-is. The GPIO module does not need to be explicitly enabled in HAL because:
-
The FMC pins are already properly configured in board.h with:
- Correct alternate function mode (AF12) for all FMC pins
- Proper pin configurations (speed, pull-up/down, etc.)
- All required SRAM control signals (SDNWE, SDNRAS, SDNCAS, etc.)
-
The essential HAL modules for SRAM operation are enabled:
- HAL_SRAM_MODULE_ENABLED
- HAL_DMA_MODULE_ENABLED
- HAL_RCC_MODULE_ENABLED
- HAL_CORTEX_MODULE_ENABLED
The GPIO module being disabled in the HAL configuration does not impact SRAM functionality since the pin configurations are handled by the board initialization code through ChibiOS.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SRAM initialization and usage patterns
# Expected: Find SRAM configuration code that might require GPIO
# Search for SRAM initialization code
echo "Checking SRAM initialization patterns:"
rg -A 5 "SRAM_HandleTypeDef" "targets/ChibiOS/"
Length of output: 132
Script:
#!/bin/bash
# Let's broaden our search to find SRAM-related code and configurations
# Search for SRAM-related code patterns
echo "Searching for SRAM usage patterns:"
rg -l "SRAM" targets/ChibiOS/ORGPAL_PALX/
# Check if there are any FSMC/FMC configurations (commonly used for external SRAM)
echo -e "\nChecking FSMC/FMC configurations:"
rg -A 5 "FSMC|FMC" targets/ChibiOS/ORGPAL_PALX/
# Look for GPIO configurations that might be related to SRAM
echo -e "\nChecking GPIO configurations in board files:"
rg "GPIO" targets/ChibiOS/ORGPAL_PALX/board.*
# Check the HAL configuration file for more context
echo -e "\nChecking complete HAL configuration:"
cat targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h
Length of output: 132880
Script:
#!/bin/bash
# Let's check the HAL configuration and SRAM-related settings
# Check HAL configuration file
echo "Checking HAL configuration:"
cat targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h
# Check if there are any SRAM initialization or configuration files
echo -e "\nChecking for SRAM configuration files:"
rg -l "SRAM" "targets/ChibiOS/ORGPAL_PALX/"
# Look for FMC/FSMC GPIO configurations
echo -e "\nChecking FMC/FSMC GPIO configurations:"
rg "FMC.*GPIO" "targets/ChibiOS/ORGPAL_PALX/"
Length of output: 30625
josesimoes
force-pushed
the
rework-mpu-config
branch
2 times, most recently
from
d464509 to
111322e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c (1)
11-11: Document external function requirements.Add documentation for
Target_ExternalMemoryConfigMPU()to clarify:
- Function's purpose
- Expected state of MPU when called (disabled)
- Memory regions it configures
+// Configures MPU regions for external memory +// Note: Must be called while MPU is disabled extern void Target_ExternalMemoryConfigMPU();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c(1 hunks)targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c(3 hunks)targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/CMakePresets.json(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h(2 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h
- targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h
- targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h
- targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h
- targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
- targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h
🧰 Additional context used
📓 Learnings (4)
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
🔇 Additional comments (13)
targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c (1)
31-44: Verify MPU configuration allows unaligned access.
The PR's objective is to support unaligned access to external memory. Let's verify the configuration.
✅ Verification successful
MPU configuration correctly allows unaligned access to external memory
The MPU configuration in target_external_memory.c is specifically designed to handle unaligned memory access for STM32F7xx with external memory, as evidenced by:
- The code includes explicit comments addressing unaligned access:
// ARM: STM32F7: hard fault caused by unaligned Memory Access
// the Cortex-M7 core may unexpectedly run into the hard fault handler because of unaligned access.- The MPU configuration sets:
- Non-cacheable access (
MPU_ACCESS_NOT_CACHEABLE) - Full access permissions (
MPU_REGION_FULL_ACCESS) - Non-bufferable access (
MPU_ACCESS_NOT_BUFFERABLE)
These settings follow the recommended configuration to prevent hard faults from unaligned access while maintaining proper memory access capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MPU configuration patterns across targets
# Look for TEX, C, B bits that control memory attributes and alignment requirements
# Search in all target MPU configuration files
rg -A 10 "MPU.*RASR.*=" --type c
# Search for alignment-related MPU settings
rg "UNALIGNED|ALIGNED|TEX|CACHEABLE|NON_CACHEABLE" --type c
Length of output: 71414
Script:
#!/bin/bash
# Search for MPU configuration functions in the target files
rg -A 10 "Target_ExternalMemoryConfigMPU|Target_ConfigNonCacheableMemory" --type c
# Search for MPU region attributes and access permissions
rg "MPU_RASR_ATTR|MPU_ACCESS|MPU_REGION" --type c targets/ChibiOS/ORGPAL_PALTHREE/
Length of output: 11850
targets/ChibiOS/ST_STM32F769I_DISCOVERY/CMakePresets.json (1)
22-22: LGTM! Enabling STM32 Cube package is required for HAL-based MPU configuration.
The change to enable the STM32 Cube package is necessary to support the new HAL-based MPU configuration for this target, as mentioned in the PR objectives.
Let's verify if other STM32F7 targets that use HAL-based MPU configuration also have this dependency:
✅ Verification successful
Enabling STM32 Cube package is consistent with other STM32F7 targets
The verification confirms that all STM32F7 targets in the codebase (ORGPAL_PALTHREE, ORGPAL_PALX, and ST_STM32F769I_DISCOVERY) consistently have STM32_CUBE_PACKAGE_REQUIRED set to "ON". This change maintains uniformity across STM32F7 targets that require HAL-based MPU configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check STM32_CUBE_PACKAGE_REQUIRED setting in other STM32F7 targets
# Expected: Similar targets should have this enabled for consistency
# Search for CMakePresets.json files in STM32F7 target directories
fd -e json CMakePresets.json targets/ChibiOS | while read -r preset; do
echo "=== $preset ==="
# Extract target series and cube package requirement
cat "$preset" | jq -r '.configurePresets[].cacheVariables | select(."TARGET_SERIES" == "STM32F7xx") | ."STM32_CUBE_PACKAGE_REQUIRED"'
done
Length of output: 2036
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (3)
7-9: LGTM! Necessary includes added for MPU and SDRAM configuration.
The new includes are correctly ordered and provide the required HAL and FSMC SDRAM functionality.
Line range hint 100-124: Excellent documentation of the unaligned access issue!
The comments provide a comprehensive explanation of the STM32F7 unaligned access problem, including symptoms, causes, and references to ARM documentation. This will be valuable for future maintenance.
125-140: Verify SDRAM memory mapping configuration
The MPU configuration looks correct for handling unaligned access, but there's a potential discrepancy:
- Comments mention SDRAM at 0xC0000000-0xC03FFFFF
- Configuration uses base address 0xD0000000
- SDRAM_START constant uses FSMC_Bank6_MAP_BASE
Please verify that 0xD0000000 is the correct base address for your SDRAM configuration.
Run this script to verify the memory mapping:
✅ Verification successful
Memory mapping configuration is correct
The configuration is correct. The code shows consistent memory mapping:
target_common.h.indefines RAM1_MEMORY_StartAddress as 0xD0000000- The MPU configuration uses the same base address 0xD0000000
- The RAM size is defined as 8MB (0x00800000) which matches the MPU region size
The comment referring to 0xC0000000-0xC03FFFFF appears to be a generic documentation note about STM32F7xx devices, not specific to this board's configuration. The ORGPAL_PALX board specifically maps its SDRAM to 0xD0000000.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check memory mapping configuration across the codebase
# Search for memory mapping definitions
echo "Searching for memory mapping definitions..."
rg -l "FSMC_Bank6_MAP_BASE|0xD0000000|0xC0000000"
# Search for other MPU configurations
echo "Searching for other MPU configurations..."
ast-grep --pattern 'MPU_InitStruct.BaseAddress = $_'
Length of output: 2001
Script:
#!/bin/bash
# Let's check the specific memory mapping in the target files
# Check target_common.h.in for ORGPAL_PALX
echo "=== ORGPAL_PALX target_common.h.in ==="
cat targets/ChibiOS/ORGPAL_PALX/target_common.h.in
# Check linker script for memory regions
echo -e "\n=== ORGPAL_PALX linker script ==="
cat targets/ChibiOS/ORGPAL_PALX/nanoCLR/STM32F76xx_CLR.ld | grep -A 10 "MEMORY"
# Check for SDRAM related definitions
echo -e "\n=== SDRAM definitions in target_external_memory.c ==="
cat targets/ChibiOS/ORGPAL_PALX/target_external_memory.c | grep -B 2 -A 2 "SDRAM"
Length of output: 4634
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (3)
7-8: LGTM!
The added headers are necessary for the HAL MPU functions and are correctly ordered.
97-99: LGTM! Correct initialization sequence.
The MPU configuration is correctly placed before SDRAM initialization, which is essential for STM32F7 targets to prevent hard faults from unaligned memory access.
134-136: Consider performance implications of memory attributes.
The current configuration sets memory as non-cacheable, non-bufferable, and non-shareable. While this ensures correct behavior for unaligned access, it may impact performance. Consider if caching can be enabled safely for specific use cases.
Let's verify if other STM32F7 targets use similar memory attributes:
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (5)
8-9: LGTM! Include changes are appropriate.
The include changes correctly reflect the dependencies needed for MPU configuration using the ST HAL API.
102-124: Excellent documentation of the unaligned access issue!
The comments provide comprehensive explanation of:
- The specific problem with unaligned access in STM32F7
- The architectural constraints from ARMv7-M manual
- The memory type requirements for the SDRAM region
127-139: LGTM! MPU attributes are correctly configured for Device memory.
The configuration correctly sets:
- Non-bufferable, non-cacheable, non-shareable attributes
- Instruction execution disabled for security
- Full access permissions
140-140: 🛠️ Refactor suggestion
Add error handling for MPU configuration.
The HAL_MPU_ConfigRegion call should include error handling to ensure the MPU is correctly configured.
- HAL_MPU_ConfigRegion(&MPU_InitStruct);
+ if (HAL_MPU_ConfigRegion(&MPU_InitStruct) != HAL_OK)
+ {
+ Error_Handler();
+ }Likely invalid or redundant comment.
100-141: 🛠️ Refactor suggestion
Add MPU initialization and memory barrier sequence.
The MPU configuration should be wrapped with proper initialization and memory barriers.
void Target_ExternalMemoryConfigMPU()
{
+ /* Disable the MPU */
+ HAL_MPU_Disable();
+
// ... existing MPU configuration ...
- HAL_MPU_ConfigRegion(&MPU_InitStruct);
+ if (HAL_MPU_ConfigRegion(&MPU_InitStruct) != HAL_OK)
+ {
+ Error_Handler();
+ }
+
+ /* Enable the MPU */
+ HAL_MPU_Enable(MPU_PRIVILEGED_DEFAULT);
+
+ /* Ensure MPU settings take effect */
+ __DSB();
+ __ISB();
}⛔ Skipped due to learnings
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
111322e to
f3eba87
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h (1)
Line range hint
1-500: Architecture recommendation for future improvements.While the current changes appropriately move MPU configuration to dedicated functions, consider adding runtime validation of MPU settings to ensure memory access patterns remain safe across different scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c(1 hunks)targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c(3 hunks)targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/CMakePresets.json(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h(2 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h
- targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h
- targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c
- targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h
- targets/ChibiOS/ORGPAL_PALX/target_external_memory.c
- targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
- targets/ChibiOS/ST_STM32F769I_DISCOVERY/CMakePresets.json
- targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h
🧰 Additional context used
📓 Learnings (2)
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
🔇 Additional comments (11)
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (3)
7-8: LGTM!
The added includes and function declaration are appropriate for the HAL-based MPU configuration.
Also applies to: 72-72
97-99: LGTM! Correct initialization sequence.
The MPU configuration is correctly placed before SDRAM initialization, which is essential for preventing unaligned access issues on STM32F7 (Cortex-M7) targets.
127-142: LGTM! Secure MPU configuration with appropriate settings.
The MPU configuration is well-implemented with security-conscious settings:
- Region size correctly matches SDRAM_SIZE (16MB)
- Instruction execution is disabled, preventing code injection
- Cache/buffer settings are appropriate for external memory
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (6)
8-9: LGTM! Include changes are appropriate.
The changes correctly update the include path style and add the necessary HAL header for MPU configuration.
102-124: Excellent documentation of the unaligned access issue!
The comments thoroughly explain the STM32F7 hard fault issue with unaligned memory access, providing:
- Clear problem description
- Root cause analysis
- References to ARM architecture specifications
132-134: LGTM! Memory attributes correctly configured for Device memory type.
The memory attributes are properly set for Device memory type as per ARM v7-M Architecture Reference Manual:
- Non-bufferable
- Non-cacheable
- Non-shareable
140-140: 🛠️ Refactor suggestion
Add error handling for MPU configuration.
The HAL_MPU_ConfigRegion call should include error handling to ensure the MPU is correctly configured.
- HAL_MPU_ConfigRegion(&MPU_InitStruct);
+ if (HAL_MPU_ConfigRegion(&MPU_InitStruct) != HAL_OK)
+ {
+ Error_Handler();
+ }Likely invalid or redundant comment.
129-129: Verify SDRAM base address configuration.
The configured base address (0xD0000000) differs from the documented SDRAM range (0xC0000000-0xC03FFFFF) in the comments. Please verify if this is intentional.
100-141: 🛠️ Refactor suggestion
Add MPU enable/disable sequence.
The MPU configuration should be wrapped with proper initialization and enable/disable calls.
void Target_ExternalMemoryConfigMPU()
{
+ /* Disable the MPU */
+ HAL_MPU_Disable();
+
// ... existing MPU configuration ...
- HAL_MPU_ConfigRegion(&MPU_InitStruct);
+ if (HAL_MPU_ConfigRegion(&MPU_InitStruct) != HAL_OK)
+ {
+ Error_Handler();
+ }
+
+ /* Enable the MPU */
+ HAL_MPU_Enable(MPU_PRIVILEGED_DEFAULT);
+
+ /* Enable fault exceptions */
+ SCB->SHCSR |= SCB_SHCSR_MEMFAULTENA_Msk;
+
+ /* Ensure MPU settings take effect */
+ __DSB();
+ __ISB();
}Note: Based on the retrieved learning, we know that other MPU regions are being configured elsewhere, and the MPU is intentionally disabled before calling this function. However, it's still good practice to ensure proper MPU state management within this function.
⛔ Skipped due to learnings
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h (2)
Line range hint 300-304: Verified: Unaligned memory access support is enabled.
The SDC configuration has STM32_SDC_SDMMC_UNALIGNED_SUPPORT TRUE which is essential for the PR's objective of supporting unaligned access to external memory.
30-30: Memory configuration change aligns with MPU rework.
The change to STM32_NOCACHE_ENABLE FALSE is consistent with moving the MPU configuration to the dedicated Target_ConfigNonCacheableMemory() function as mentioned in the PR objectives.
Let's verify that the MPU configuration has been properly moved:
✅ Verification successful
MPU configuration has been properly moved to dedicated functions
The change to STM32_NOCACHE_ENABLE FALSE is correct as the MPU configuration has been properly moved to dedicated functions in target_mpu_config.c. The implementation shows:
- Non-cacheable memory configuration is handled by
Target_ConfigNonCacheableMemory() - Main MPU setup is managed by
Target_ConfigMPU() - The old hardcoded defines have been removed from mcuconf.h
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the new MPU configuration function
ast-grep --pattern 'Target_ConfigNonCacheableMemory() {
$$$
}'
# Verify no other files are still using the old configuration
rg "STM32_NOCACHE_MPU_REGION|STM32_NOCACHE_RBAR|STM32_NOCACHE_RASR" --type c --type h
Length of output: 1230
Script:
#!/bin/bash
# Let's search for the Target_ConfigNonCacheableMemory implementation in a broader way
rg -l "Target_ConfigNonCacheableMemory" --type c --type h
# Also search for any MPU configuration related code
rg "MPU_Config|ConfigureMPU|MPU_Region_Config" -A 5 --type c
Length of output: 1586
Script:
#!/bin/bash
# Let's examine the implementation of Target_ConfigNonCacheableMemory in PALX
cat targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
# Also check if there are any other MPU-related functions in this file
rg "void.*MPU" targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
Length of output: 1231
josesimoes
force-pushed
the
rework-mpu-config
branch
from
f3eba87 to
811e98f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (1)
140-140: Add error handling for HAL_MPU_ConfigRegionConsider adding error handling for the HAL_MPU_ConfigRegion call to ensure the MPU is configured correctly.
- HAL_MPU_ConfigRegion(&MPU_InitStruct); + if (HAL_MPU_ConfigRegion(&MPU_InitStruct) != HAL_OK) + { + // Handle error condition + // Consider logging or setting an error flag + }targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h (1)
Line range hint
385-386: Consider adjusting USB thread configuration comments.The comments indicate that some USB configurations are not in the latest version but are kept "just in case". Consider removing these if they are no longer needed or document why they are being preserved.
-#define STM32_USB_OTG_THREAD_PRIO LOWPRIO // NOT IN LATEST VERSION, KEEPING JUST INCASE -#define STM32_USB_OTG_THREAD_STACK_SIZE 128 // NOT IN LATEST VERSION, KEEPING JUST INCASE -#define STM32_USB_OTGFIFO_FILL_BASEPRI 0 // NOT IN LATEST VERSION, KEEPING JUST INCASEAlso applies to: 392-393
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c(2 hunks)targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c(1 hunks)targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt(1 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c(2 hunks)targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h(1 hunks)targets/ChibiOS/ORGPAL_PALX/target_external_memory.c(3 hunks)targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/CMakePresets.json(1 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h(2 hunks)targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- targets/ChibiOS/ORGPAL_PALTHREE/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALTHREE/nanoCLR/mcuconf.h
- targets/ChibiOS/ORGPAL_PALTHREE/stm32f7xx_hal_conf.h
- targets/ChibiOS/ORGPAL_PALTHREE/target_mpu_config.c
- targets/ChibiOS/ORGPAL_PALX/CMakeLists.txt
- targets/ChibiOS/ORGPAL_PALX/nanoCLR/main.c
- targets/ChibiOS/ORGPAL_PALX/stm32f7xx_hal_conf.h
- targets/ChibiOS/ORGPAL_PALX/target_mpu_config.c
- targets/ChibiOS/ST_STM32F769I_DISCOVERY/CMakePresets.json
🧰 Additional context used
📓 Learnings (3)
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c:97-99
Timestamp: 2024-11-21T19:49:52.471Z
Learning: On the `ST_STM32F429I_DISCOVERY` target, which uses a Cortex-M4 MCU, MPU configuration is not required during external SDRAM initialization because the Cortex-M4 does not have issues with unaligned access to external memory.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3044
File: targets/ChibiOS/ORGPAL_PALX/target_external_memory.c:124-139
Timestamp: 2024-11-21T19:44:59.786Z
Learning: In `targets/ChibiOS/ORGPAL_PALX/target_external_memory.c`, within the function `Target_ExternalMemoryConfigMPU()`, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured. Therefore, it's by design that there are no calls to disable and re-enable the MPU within this function.
🔇 Additional comments (16)
targets/ChibiOS/ST_STM32F769I_DISCOVERY/stm32f7xx_hal_conf.h (4)
23-33: LGTM: System configuration settings are appropriate
The system configuration settings, particularly PREFETCH_ENABLE and ART_ACCELERATOR_ENABLE, are correctly enabled to optimize memory access performance, which is crucial for the MPU configuration changes.
51-57: LGTM: Header includes are properly structured
The SDRAM and CORTEX module header includes are correctly guarded with their respective module enable checks, maintaining clean compilation when modules are disabled.
37-37: Verify callback configuration consistency
The SDRAM callback registration is disabled, which is the typical configuration. However, we should verify this setting is consistent across other STM32F7 targets using SDRAM.
✅ Verification successful
SDRAM callback configuration is consistent across targets
All STM32F7 targets with SDRAM enabled have the callback registration consistently disabled (set to 0U):
- targets/AzureRTOS/ST/ORGPAL_PALTHREE
- targets/ChibiOS/ORGPAL_PALTHREE
- targets/ChibiOS/ORGPAL_PALX
- targets/ChibiOS/ST_STM32F769I_DISCOVERY
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SDRAM callback configuration consistency
# Expected: Consistent callback configuration across targets using SDRAM
echo "Checking SDRAM callback configuration in STM32F7 targets..."
fd -t f "stm32f7xx_hal_conf.h" "targets" | while read -r config_file; do
if rg "HAL_SDRAM_MODULE_ENABLED" "$config_file" > /dev/null; then
echo "=== $config_file ==="
rg "USE_HAL_SDRAM_REGISTER_CALLBACKS" "$config_file" || echo "Callback configuration missing"
fi
done
Length of output: 1900
18-21: Verify SDRAM module consistency across STM32F7 targets
While enabling both SDRAM and CORTEX modules aligns with the PR's objective for MPU configuration and external memory access, there's an inconsistency in SDRAM module configuration across STM32F7 targets as noted in previous reviews.
✅ Verification successful
Based on the script results, I can now generate the final response:
SDRAM module configuration is consistent with external memory usage
The SDRAM module configuration is correctly enabled in all STM32F7 targets that have external memory configuration:
- ChibiOS/ORGPAL_PALTHREE: Has external memory config and SDRAM enabled
- ChibiOS/ORGPAL_PALX: Has external memory config and SDRAM enabled
- ChibiOS/ST_STM32F769I_DISCOVERY: Has external memory config and SDRAM enabled
- AzureRTOS/ORGPAL_PALTHREE: No external memory config and SDRAM is disabled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if all STM32F7 targets requiring external memory access have SDRAM module enabled
# Expected: Consistent SDRAM module configuration across related targets
echo "Checking SDRAM module configuration in STM32F7 targets..."
fd -t f "stm32f7xx_hal_conf.h" "targets" | while read -r config_file; do
if rg "STM32F7" "$config_file" > /dev/null; then
echo "=== $config_file ==="
# Check for external memory usage and SDRAM module
echo "External memory configuration:"
rg "HAL_SDRAM_MODULE_ENABLED" "$config_file" || echo "SDRAM module not enabled"
echo "Memory configuration:"
rg "Target_ExternalMemoryConfig" "$(dirname "$config_file")" -l || echo "No external memory config found"
fi
done
Length of output: 3288
targets/ChibiOS/ORGPAL_PALX/target_external_memory.c (3)
7-9: LGTM! Required headers are properly included
The new includes are necessary for the MPU and SDRAM functionality, following a logical include order.
Line range hint 100-140: Well-implemented MPU configuration for handling unaligned access
The implementation:
- Is well-documented with clear explanations of the unaligned access issue
- Correctly configures the MPU region for external SDRAM
- Properly disables caching and buffering as required
- Follows the STM32F7 hardware requirements
Note: As learned from previous feedback, the MPU is intentionally disabled before calling this function, and other MPU regions are being configured, so the absence of MPU enable/disable calls is by design.
129-129: Verify the SDRAM base address
The configured base address (0xD0000000) differs from the address range mentioned in the comments (0xC0000000-0xC03FFFFF). Please verify if this is intentional.
targets/ChibiOS/ST_STM32F769I_DISCOVERY/target_external_memory.c (4)
7-8: LGTM: Required headers added
The added headers are necessary for the HAL MPU functions used in the implementation.
97-99: LGTM: Correct initialization sequence
The MPU is properly configured before SDRAM initialization, which is essential to prevent hard faults from unaligned access on Cortex-M7 devices.
127-142: LGTM: Comprehensive MPU configuration
The implementation:
- Correctly configures a 16MB region for SDRAM
- Properly disables code execution from SDRAM
- Sets appropriate memory attributes for external SDRAM access
- Uses the ST HAL API as intended
The configuration effectively addresses the unaligned access issues on Cortex-M7 devices while maintaining security best practices.
129-142: Verify MPU configuration across STM32F7 variants
While the implementation looks correct, let's verify the MPU configuration works across all STM32F7 variants.
✅ Verification successful
The initial search revealed different SDRAM base addresses being used (0xC0000000 and 0xD0000000). Let's gather more context about the memory configurations across different boards to ensure this specific configuration is appropriate for this target.
MPU configuration is correctly board-specific
The MPU configuration in the ST_STM32F769I_DISCOVERY board is correct and matches its hardware configuration:
- It uses FSMC Bank5 (0xC0000000) with 16MB size, which aligns with the board's SDRAM definition
- Other STM32F7 boards (ORGPAL_PALX, PALTHREE, F429I) use FSMC Bank6 (0xD0000000) with 8MB size
- The memory mapping is defined in the HAL and properly differentiated between boards
The configuration is intentionally different across boards as it matches their specific hardware implementations and memory sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for different MPU configurations across STM32F7 targets
echo "Searching for MPU configurations in STM32F7 targets..."
rg -A 5 "MPU_InitStruct.BaseAddress.*0xC0000000" "targets/ChibiOS/"
echo "Searching for SDRAM configurations..."
rg -A 5 "FSMC_Bank5_MAP_BASE|SDRAM_START" "targets/ChibiOS/"
Length of output: 4794
Script:
#!/bin/bash
# Let's verify the MPU configuration in other STM32F7 boards and their memory configurations
echo "Searching for other STM32F7 board configurations..."
fd -t f "mcuconf.h" "targets/ChibiOS/" --exec grep -l "STM32F7"
echo "Checking MPU configurations in these boards..."
fd -t f "target_external_memory.c" "targets/ChibiOS/" --exec grep -A 15 "MPU_InitStruct"
echo "Checking memory map definitions..."
rg "FSMC_Bank[56]_MAP_BASE" "targets/ChibiOS/" -B 2 -A 2
Length of output: 6469
targets/ChibiOS/ORGPAL_PALTHREE/target_external_memory.c (2)
8-9: LGTM! Include changes are appropriate.
The changes to include paths and addition of STM32 HAL header are necessary for the MPU configuration functionality.
102-124: LGTM! Excellent documentation of the unaligned access issue.
The comments thoroughly explain the root cause of hard faults with unaligned memory access on STM32F7 devices and the rationale behind the MPU configuration.
targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h (3)
Line range hint 1-24: LGTM: Header configuration and license information.
The header configuration and license information are properly maintained.
30-30: Verify cache configuration impact on external memory access.
The change from STM32_NOCACHE_ENABLE TRUE to FALSE aligns with the PR's objective to rework MPU configuration. However, we should verify that this change doesn't adversely affect performance-critical operations.
✅ Verification successful
Based on the search results, I can now generate the final response:
Cache configuration change is consistent with MPU rework
The change from STM32_NOCACHE_ENABLE TRUE to FALSE is part of a broader MPU configuration rework across the codebase. The search results confirm:
- The change is consistent with other ORGPAL targets that use similar cache configurations
- The removal of NOCACHE settings is aligned with the target's memory management strategy
- The configuration is properly coordinated with the MPU setup in
target_mpu_config.cwhich handles non-cacheable memory regions explicitly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other cache-related configurations and potential dependencies
# Look for related MPU or cache configurations in other files
# Search for MPU configuration files
echo "Searching for MPU configuration files..."
fd -e c -e h -e cpp . | rg -l "MPU|Cache|NOCACHE"
# Search for references to nocache settings
echo "Searching for nocache references..."
rg "NOCACHE|MPU_NOCACHE|Cache|CacheEnable"
Length of output: 38010
Line range hint 426-428: Verify inclusion of required configuration headers.
The inclusion of mcuconf_nf.h and mcuconf_community.h at the end of the file is correct, but we should verify that these files contain any necessary MPU configurations that were removed from this file.
✅ Verification successful
MPU configurations are properly handled in the main mcuconf.h file
The MPU-related configuration is present in the main mcuconf.h file through the STM32_NOCACHE_ENABLE setting, which is set to FALSE. This is the standard way to configure MPU/cache behavior in STM32F7xx series MCUs. No additional MPU configurations are required in the included configuration files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the content of included configuration files
# Look for the included configuration files
echo "Searching for mcuconf_nf.h and mcuconf_community.h..."
fd -e h "mcuconf_(nf|community)\.h$"
# Check their contents for MPU configurations
echo "Checking MPU configurations in these files..."
rg "MPU|NOCACHE|Cache" $(fd -e h "mcuconf_(nf|community)\.h$")
Length of output: 2071
Script:
#!/bin/bash
# Let's check the specific configuration files for the ORGPAL_PALX target
echo "Content of mcuconf_nf.h:"
cat targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf_nf.h
echo -e "\nContent of mcuconf_community.h:"
cat targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf_community.h
# Also check the parent mcuconf.h to understand the context better
echo -e "\nContent of mcuconf.h:"
cat targets/ChibiOS/ORGPAL_PALX/nanoCLR/mcuconf.h
Length of output: 17932
- Add MPU config to ORGPAL3 and ORGPALX to allow unaligned access to external memory. - Remove MPU config for non cached memory in ChibiOS mcu config for ORGPAL3 and ORGPALX. Now are in Target_ConfigNonCacheableMemory(). - Rework call to MPU config in ORGPAL3 and ORGPALX. - Migrate ST_STM32F769I_DISCOVERY MPU config to use ST HAL API. - Enable use ST HAL in STM32F7 targets.
josesimoes
force-pushed
the
rework-mpu-config
branch
from
811e98f to
70c9116
Compare
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation