Conversation
SergioGasquez
left a comment
SergioGasquez
left a comment
There was a problem hiding this comment.
LGTM! Should we keep the 40Mhz branch?
| const PCR_SYSCLK_CONF_REG: u32 = 0x6009_6110; // PCR_BASE_REG + 0x110 | ||
| const PCR_CLK_XTAL_FREQ_MASK: u32 = 0x7F; | ||
| const PCR_CLK_XTAL_FREQ_SHIFT: u32 = 24; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actually I just read this out of the TRM, but sure we can say it comes from esptool :D
| let norm_xtal = if est_xtal > 45 { | ||
| XtalFrequency::_48Mhz | ||
| } else { | ||
| XtalFrequency::_40Mhz | ||
| }; |
There was a problem hiding this comment.
I wonder if we still need this if we only support 48Mhz
There was a problem hiding this comment.
Pull request overview
Improves ESP32-C5 crystal frequency detection by reading the XTAL frequency field directly from the PCR sysclock configuration register, avoiding the prior UART-divisor-based estimate that could yield incorrect results (e.g., 0).
Changes:
- Update ESP32-C5
xtal_frequency()detection to readPCR_SYSCLK_CONF_REGand derive the XTAL frequency from its encoded field. - Minor formatting change to the
flash_targetre-exports. - Add an unreleased changelog entry for the ESP32-C5 crystal-frequency reporting fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
espflash/src/target/mod.rs |
Reads XTAL frequency for ESP32-C5 from PCR sysclock config register instead of estimating via UART clock divider. |
CHANGELOG.md |
Documents the fix under “Unreleased → Fixed”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I think we might want to keep the 40MHz branch just in case, I'm also not sure if there are preproduction chips floating around that should at least not be misreported. |
2 participants
The previous algorithm estimated a very inaccurate 0 as the xtal freq.