-
Notifications
You must be signed in to change notification settings - Fork 37
[RFC] Clocks: Remove remaining hard coding, add partial NXP fsl_power library support #512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[RFC] Clocks: Remove remaining hard coding, add partial NXP fsl_power library support #512
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds power optimization features to reduce VDDCore voltage through slower system PLL speeds. It introduces a new fsl_power module (ported from NXP's fsl_power.c) for voltage/frequency management and removes lingering hard coding so all fields of a ClockConfig are used when clocks are initialized
Key changes:
- New
fsl_powermodule with LDO voltage management and body bias control - main clock mult, div, and src values pulled from self.config
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/fsl_power.rs |
New power management module with voltage scaling, LVD control, and body bias functions |
src/clocks.rs |
Added voltage management integration |
src/lib.rs |
Exported the new fsl_power module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bf0e92e to
530fc00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6360282 to
0172f48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
74ca4bd to
6809e01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove some hardcoded clock reg settings and use the config blob fields instead. Experimental slow-clocks feature to be removed after more thorough validation Add default impl for ClockConfig
Remove slow-clocks feature as end user defines the clock config blob which is now utilized fully. Use the requested main clock frequency to set the LDO voltage level for optimal power consumption at that speed. Restructure LDO voltage test to better cover voltage calculations.
… comment Was accidentally put in wrong order and does not match C implementation of fsl_power
…RST then adjust bias mode
c8bbb2c to
6cd5fc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asasine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me. No obvious UB in the fsl_power port. I don't know a ton about the HAL and Clocks API.
| let pmc = unsafe { crate::pac::Pmc::steal() }; | ||
|
|
||
| // Enter FBB mode if not already in FBB | ||
| if get_body_bias_mode(&pmc) != BodyBiasMode::Fbb { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the default behavior of the boot ROM? Are we already in fbb mode by default?
| self.mult = MainPllClkMult::Mult33; | ||
| } | ||
| _ => { | ||
| warn!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is reasonable to panic here as the value input should be constrained by the enum in the configuration struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or better yet, make set_clock_rate take a MainPllClkMult instead of a u8 (unless there's a good reason not to that I'm missing).
|
|
||
| // Configure power domains for sleep: MAINCLK_SHUTOFF=1, FBB_PD=0 (keep FBB powered) | ||
| let pdsleepcfg0 = (sysctl0.pdruncfg0().read().bits() | ||
| | (1 << 20)) // MAINCLK_SHUTOFF_MASK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't SYSCTL0_PDSLEEPCFG0_MAINCLK_SHUTOFF be bit position 0?
#define SYSCTL0_PDSLEEPCFG0_MAINCLK_SHUTOFF_MASK (0x1U)
#define SYSCTL0_PDSLEEPCFG0_MAINCLK_SHUTOFF_SHIFT (0U)
And SYSCTL0_PDSLEEPCFG0_FBB be bit position 12?
#define SYSCTL0_PDSLEEPCFG0_FBB_PD_MASK (0x1000U)
#define SYSCTL0_PDSLEEPCFG0_FBB_PD_SHIFT (12U)
We should update PAC to add these field definition so they are not as error prone.
| // Configure PMC: enable auto-wakeup, disable LVD during transition | ||
| pmc.ctrl().write(|w| unsafe { | ||
| w.bits( | ||
| (pmc_ctrl | (1 << 15)) // AUTOWKEN_MASK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be bit 28?
#define PMC_CTRL_AUTOWKEN_MASK (0x10000000U)
#define PMC_CTRL_AUTOWKEN_SHIFT (28U)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or pmc.ctrl() .modify(|_, w| w.autowken().set_bit().lvdcorere().clear_bit().lvdcoreie().clear_bit());
| }); | ||
|
|
||
| // Enable PMC interrupt for auto-wake (IRQ 49, bit 49-32=17 in STARTEN1) | ||
| sysctl0.starten1_set().write(|w| unsafe { w.bits(1 << 17) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or sysctl0.starten1_set().write(|w| w.pmic().set_bit());
| .write(|w| unsafe { w.bits(sysctl0.pdruncfg3().read().bits()) }); | ||
|
|
||
| // PDWAKECFG: Keep FBB state after wake (FBBKEEPST_MASK = bit 17) | ||
| sysctl0.pdwakecfg().write(|w| unsafe { w.bits(1 << 17) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is bit position 1:
#define SYSCTL0_PDWAKECFG_FBBKEEPST_MASK (0x2U)
#define SYSCTL0_PDWAKECFG_FBBKEEPST_SHIFT (1U)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or sysctl0.pdwakecfg().write(|w| w.fbbkeepst().fbbkeepst_1());
| sysctl0.starten1_set().write(|w| unsafe { w.bits(1 << 17) }); | ||
|
|
||
| // Execute WFI - will wake automatically via PMC timer | ||
| cortex_m::asm::wfi(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ]; | ||
|
|
||
| /// Sentinel for invalid/unsupported voltage level | ||
| pub const POWER_INVALID_VOLT_LEVEL: u32 = 0xFFFFFFFF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? Can we just return Results instead where this is used?
| let pmc_ctrl = pmc.ctrl().read().bits(); | ||
|
|
||
| // Enable deep sleep mode (set SLEEPDEEP bit in SCR) | ||
| const SCB_SCR_SLEEPDEEP: u32 = 1 << 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing this, we could have do this instead:
{
let mut cortex = cortex_m::Peripherals::take().unwrap();
cortex.SCB.set_sleepdeep();
}
| } | ||
|
|
||
| /// Returns the current body bias mode. | ||
| pub fn get_body_bias_mode(pmc: &crate::pac::Pmc) -> BodyBiasMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the SDK, this is actually reading PDSLEEPCFG0 or PDRUNCFG0, and not PMC runctrl.
static inline body_bias_mode_t POWER_GetBodyBiasMode(pmic_mode_reg_t reg)
{
uint32_t mode = (uint32_t)reg;
uint32_t bbMode = (SYSCTL0_TUPLE_REG(mode) & (SYSCTL0_PDSLEEPCFG0_RBB_PD_MASK | SYSCTL0_PDSLEEPCFG0_FBB_PD_MASK)) >>
SYSCTL0_PDSLEEPCFG0_RBB_PD_SHIFT;
return (body_bias_mode_t)bbMode;
}
| enter_fbb(&pmc); | ||
| } | ||
|
|
||
| let idx = temp_range as usize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already an enum, matching to the num or implement into() is simpler.
| ); | ||
|
|
||
| // Find the first frequency threshold that freq is greater than | ||
| let position = freq_levels.iter().position(|&threshold| freq > threshold); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is relatively straightforward. However, the code is very unreadable in its current form. It is traversing through frequency levels and picking a voltage level based on desired frequency.
We can simplify the logic and avoid POWER_INVALID_VOLT_LEVEL (let it return an option) .
Define POWER_LDO_VOLT_LEVEL as 2 instances, one for full and one for low:
const LOW_POWER_LDO_VOLT_LEVEL: [LDOVoltLevel; 3] = [
LDOVoltLevel::Ldo0p9v,
LDOVoltLevel::Ldo0p8v,
LDOVoltLevel::Ldo0p7v,
];
const FULL_POWER_LDO_VOLT_LEVEL: [LDOVoltLevel; 5] = [
LDOVoltLevel::Ldo1p13v,
LDOVoltLevel::Ldo1p0v,
LDOVoltLevel::Ldo0p9v,
LDOVoltLevel::Ldo0p8v,
LDOVoltLevel::Ldo0p7v,
];
The logic of function can be something like this
let freq_iter = freq_levels.iter();
let volt_level_iter = POWER_LDO_VOLT_LEVEL.iter();
if let Some(max_freq) = freq_iter.next() {
if freq > *max_freq {
return None;
}
}
for (freq_threshold, volt_level) in freq_iter.zip(volt_level_iter) {
if freq > *freq_threshold {
return Some(*volt_level as u32);
}
}
return POWER_LDO_VOLT_LEVEL.last();
| }; | ||
|
|
||
| let cm33_volt = calc_volt_level(cm33_levels, cm33_freq); | ||
| let dsp_volt = calc_volt_level(dsp_levels, dsp_freq); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just ignore the DSP freq and voltage as it is not currently being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let true_main_clk_freq: u32 = (config.main_pll_clk.freq.load(Ordering::Relaxed) / config.main_pll_clk.pfd0 as u32) | ||
| * SYSPLL_MULTIPLIER_FOR_PFDS; |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential division by zero if config.main_pll_clk.pfd0 is set to 0. While the default configuration sets pfd0 to 19, a user could create a custom ClockConfig with pfd0 set to 0, which would cause a panic at runtime. Consider adding a validation check or documenting that pfd0 must be non-zero.
| let true_main_clk_freq: u32 = (config.main_pll_clk.freq.load(Ordering::Relaxed) / config.main_pll_clk.pfd0 as u32) | |
| * SYSPLL_MULTIPLIER_FOR_PFDS; | |
| let pfd0 = config.main_pll_clk.pfd0 as u32; | |
| if pfd0 == 0 { | |
| panic!("config.main_pll_clk.pfd0 must be non-zero to avoid divide-by-zero in main clock frequency calculation"); | |
| } | |
| let true_main_clk_freq: u32 = | |
| (config.main_pll_clk.freq.load(Ordering::Relaxed) / pfd0) * SYSPLL_MULTIPLIER_FOR_PFDS; |
| Some(0) => POWER_INVALID_VOLT_LEVEL, | ||
| // freq is between thresholds - calculate voltage index | ||
| Some(i) => { | ||
| let idx = POWER_LDO_VOLT_LEVEL.len() - freq_levels.len() + i - 1; |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation POWER_LDO_VOLT_LEVEL.len() - freq_levels.len() + i - 1 could underflow and panic in debug mode if freq_levels.len() > POWER_LDO_VOLT_LEVEL.len(). While the current usage with predefined frequency tables ensures this won't happen, this function is public and could be called with arbitrary slices. Consider adding a validation check at the start of the function to ensure freq_levels.len() <= POWER_LDO_VOLT_LEVEL.len(), or documenting this requirement.
| /// Applies PMC power domain config changes, waiting for FSM idle. | ||
| pub fn apply_pd(pmc: &crate::pac::Pmc) { | ||
| // Cannot set APPLYCFG when ACTIVEFSM is 1 | ||
| while pmc.status().read().activefsm().bit_is_set() {} | ||
|
|
||
| // Set APPLYCFG bit | ||
| pmc.ctrl().modify(|_, w| w.applycfg().set_bit()); | ||
|
|
||
| // Wait all PMC finite state machines finished |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The busy-wait loops at lines 104 and 110 could potentially hang indefinitely if the hardware FSM never becomes idle. While this is expected hardware behavior, consider adding a timeout mechanism or a comment documenting that the caller should ensure the PMC is in a valid state before calling this function, to help prevent potential hangs in error conditions.
| /// Applies PMC power domain config changes, waiting for FSM idle. | |
| pub fn apply_pd(pmc: &crate::pac::Pmc) { | |
| // Cannot set APPLYCFG when ACTIVEFSM is 1 | |
| while pmc.status().read().activefsm().bit_is_set() {} | |
| // Set APPLYCFG bit | |
| pmc.ctrl().modify(|_, w| w.applycfg().set_bit()); | |
| // Wait all PMC finite state machines finished | |
| /// Applies PMC power domain configuration changes and waits for all PMC FSMs to become idle. | |
| /// | |
| /// This function **busy-waits** on the `ACTIVEFSM` status bit before and after asserting | |
| /// `APPLYCFG`. If the PMC finite state machines never become idle (for example, due to a | |
| /// hardware fault or invalid PMC state), these waits will not terminate and the function | |
| /// will block indefinitely. | |
| /// | |
| /// Callers are responsible for ensuring that the PMC is in a valid state such that | |
| /// `ACTIVEFSM` will eventually be cleared by hardware before invoking this function. | |
| pub fn apply_pd(pmc: &crate::pac::Pmc) { | |
| // Cannot set APPLYCFG when ACTIVEFSM is 1; this busy-wait may block indefinitely if | |
| // the hardware FSM never returns to idle. | |
| while pmc.status().read().activefsm().bit_is_set() {} | |
| // Set APPLYCFG bit | |
| pmc.ctrl().modify(|_, w| w.applycfg().set_bit()); | |
| // Wait until all PMC finite state machines have finished; this busy-wait may block | |
| // indefinitely if the hardware FSM never returns to idle. |
| static OSC_SETTLING_TIME: AtomicU32 = AtomicU32::new(0); | ||
| static PMIC_VDDCORE_RECOVERY_TIME: AtomicU32 = AtomicU32::new(0); | ||
| static LVD_CHANGE_FLAG: AtomicU32 = AtomicU32::new(0); | ||
|
|
||
| // Public setters mirroring the C APIs | ||
| /// Set oscillator settling time (µs) | ||
| pub fn update_osc_settling_time(us: u32) { | ||
| OSC_SETTLING_TIME.store(us, Ordering::SeqCst); | ||
| } | ||
|
|
||
| /// Set PMIC VDDCORE recovery time (µs) | ||
| pub fn update_pmic_recovery_time(us: u32) { | ||
| PMIC_VDDCORE_RECOVERY_TIME.store(us, Ordering::SeqCst); | ||
| } |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static variables OSC_SETTLING_TIME and PMIC_VDDCORE_RECOVERY_TIME are set via public functions but are never read. This suggests incomplete implementation. Consider adding a TODO comment or documentation explaining the intended use of these values for future implementation, or remove them if they're not yet needed.
| //! It includes lookup tables for supported CPU and DSP frequencies at different voltage levels, | ||
| //! and functions to calculate appropriate LDO voltage settings based on target frequencies. | ||
| //! | ||
| //! Portions |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment "Portions" on line 5 appears incomplete. It should likely be "Portions of this code are" or "Portions Copyright" to form a complete sentence explaining the origin of the code portions.
| //! Portions | |
| //! Portions of this code are: |
| fsl_power::VoltOpRange::Low | ||
| }; | ||
|
|
||
| if !fsl_power::set_ldo_voltage_for_freq(fsl_power::TempRange::TempN20CtoP85C, volt_range, true_main_clk_freq, 0) |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temperature range is hardcoded to TempRange::TempN20CtoP85C (-20°C to 85°C). This may not be appropriate for all use cases, particularly if the hardware is rated for the narrower 0°C to 85°C range (TempRange::Temp0Cto85C). Using the wider temperature range when not needed could result in conservative (higher) voltage settings that waste power. Consider making the temperature range configurable via the ClockConfig struct.
| return Err(ClockError::InvalidFrequency); | ||
| } | ||
| // SAFETY: unsafe needed to write the bits for the num and demon fields | ||
| clkctl0.syspll0num().write(|w| unsafe { w.num().bits(0b0) }); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "demon" should be "denom" (short for denominator).



No description provided.