getFlashFrequencyMHz #11898
Closed
getFlashFrequencyMHz #11898
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
👋 Hello Jason2866, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 76 files 76 suites 13m 34s ⏱️ Results for commit ddbc739. ♻️ This comment has been updated with latest results. |
Updated source frequency handling for ESP32 and ESP32S3 targets.
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the flash frequency detection method from reading magic header values to reading actual hardware register values for more accurate runtime frequency reporting.
- Removes flash frequency detection based on esp_image_header_t magic values
- Implements hardware register-based flash frequency detection with support for multiple ESP32 variants
- Adds new API functions for flash frequency analysis and high-performance mode detection
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cores/esp32/chip-debug-report.h | Adds function declarations for new flash frequency detection APIs |
| cores/esp32/chip-debug-report.cpp | Replaces magic header-based frequency detection with hardware register reading implementation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| uint8_t source_freq = getFlashSourceFrequencyMHz(); | ||
| uint8_t divider = getFlashClockDivider(); | ||
|
|
||
| chip_report_printf(" Bus Speed : %lu MHz\n", actual_freq); |
There was a problem hiding this comment.
This line duplicates the frequency information already displayed in the 'Bus Speed' line above. Consider removing this redundant output or consolidating the information into a single, more comprehensive line.
Suggested change
| chip_report_printf(" Bus Speed : %lu MHz\n", actual_freq); |
Copilot uses AI. Check for mistakes.
cores/esp32/chip-debug-report.cpp
Outdated
| #elif CONFIG_IDF_TARGET_ESP32 | ||
| #define FLASH_SPI0_BASE 0x3ff42000 | ||
| #else | ||
| #define FLASH_SPI0_BASE 0x60003000 // Default for new chips |
There was a problem hiding this comment.
Using a hardcoded default address for unknown chip variants could cause crashes or incorrect behavior. Consider adding a compile-time error or runtime check for unsupported targets instead of falling back to a potentially incorrect address.
Suggested change
| #define FLASH_SPI0_BASE 0x60003000 // Default for new chips | |
| #error "Unsupported chip variant: FLASH_SPI0_BASE is not defined for this target. Please add support for your chip." |
Copilot uses AI. Check for mistakes.
cores/esp32/chip-debug-report.cpp
Outdated
| return 80; // Always 80 MHz source, divider determines 40/80 MHz | ||
| #else | ||
| volatile uint32_t* core_clk_reg = (volatile uint32_t*)(FLASH_SPI0_BASE + FLASH_CORE_CLK_SEL_OFFSET); | ||
| uint32_t core_clk_sel = (*core_clk_reg) & 0x3; // Bits 0-1 |
There was a problem hiding this comment.
The magic number 0x3 should be defined as a named constant (e.g., #define CORE_CLK_SEL_MASK 0x3) to improve code readability and maintainability.
Copilot uses AI. Check for mistakes.
cores/esp32/chip-debug-report.cpp
Outdated
| uint32_t clock_val = *clock_reg; | ||
|
|
||
| // Bit 31: if set, clock is 1:1 (no divider) | ||
| if (clock_val & (1 << 31)) { |
There was a problem hiding this comment.
The bit position 31 should be defined as a named constant (e.g., #define CLOCK_1_TO_1_BIT 31) to improve code readability and maintainability.
Copilot uses AI. Check for mistakes.
cores/esp32/chip-debug-report.cpp
Outdated
| } | ||
|
|
||
| // Bits 16-23: clkdiv_pre | ||
| uint8_t clkdiv_pre = (clock_val >> 16) & 0xFF; |
There was a problem hiding this comment.
The shift amount 16 and mask 0xFF should be defined as named constants (e.g., #define CLKDIV_PRE_SHIFT 16 and #define CLKDIV_PRE_MASK 0xFF) to improve code readability and maintainability.
Copilot uses AI. Check for mistakes.
|
Closing, trying to find higher level function which can be used |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the flash frequency is read from magic header. The value is not in every case the real used clock frequency.
The PR changes this and read the config from the hardware registers which sets the clock frequency.