Add Dynamic Voltage and Frequency Scaling Driver Class - #567
Conversation
Signed-off-by: Cheng <lichengchaoreng@gmail.com>
Signed-off-by: Cheng <lichengchaoreng@gmail.com>
Signed-off-by: Cheng <lichengchaoreng@gmail.com>
Signed-off-by: Cheng <lichengchaoreng@gmail.com>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
|
Same comment as previous: is there a design written up for this that we can look at? |
Writing the doc now. Should be available soon. |
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
884d1cb to
50a87d3
Compare
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
|
I have added a section about DVFS in the design doc. |
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
Signed-off-by: Cheng Li <cheng.li10@unsw.edu.au>
|
The description of what DVFS in the design doc could use a prune. Dynamic and static power (static in particular) aren't super important and things like defining CMOS seem totally irrelevant (all semiconductors have dynamic and static power usage!). I would drop the static power bits and just go straight into "The dynamic power of a computer is roughly given by ...." The equation for dynamic power is also wrong as it omits the activity factor, but tbh I think that doesn't really matter. I would reword the description below however as it is misleading
Power itself has a quadratic relationship to voltage when current is omitted (P=V^2/R), but crucially the voltage doesn't "scale" in the same way frequency does. There simply needs to be sufficient voltage for all transistors to switch at a given frequency (as you already point out). I think you should just omit mentioning the linear vs exponential scaling because this might cause a reader to falsely conclude that these are independent quantities, when in reality a truer expression of dynamic power should express voltage as a function of frequency since it should be minimised. |
|
|
||
| \paragraph{Direct interact with the clock/regulator:} The DVFS driver directly interacts | ||
| with the clock and the regulator that control the frequency and the voltage the processing | ||
| units are operating on. This approach is used on most of the ARM or RISC-V platforms. |
| procedures that can be called: | ||
| \begin{description} | ||
| \item[\texttt{get\_freq(core\_identifier)}] | ||
| Retrieves the current actual frequency of a specific processing unit. |
| \item[\texttt{get\_freq(core\_identifier)}] | ||
| Retrieves the current actual frequency of a specific processing unit. | ||
| \item[\texttt{set\_freq(core\_identifier, frequency)}] | ||
| Sets the designated processing unit to a specific target frequency. The frequency |
|
I see that cpufreq implements this internally ... still seems clumsy this way though. Maybe it should be in Third note ... I see that this driver actually hardcodes Xilinx things so just making it rust would also be wrong. I will just make it zcu102 in my branch based on this. |
|
|
||
| #[derive(Debug)] | ||
| pub enum Error { | ||
| EINVAL = 0, |
There was a problem hiding this comment.
I am not a rust person ... but what is the point of this error? Isn't it simpler to use a builtin error type given that this enum only has one member?
There was a problem hiding this comment.
Yeah, I agree that it would be better to use a standardized error type instead of having their own error types in the driver. The thing is that I don't think(I might be wrong) that there is a built-in error enum in Rust, nor is there a universal error enum in our system. I would be more than happy to switch to a universal error enum in our system.
| // SPDX-License-Identifier: BSD-2-Clause | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct OppEntry { |
There was a problem hiding this comment.
Should rename this to OpEntry for clarity, since "Operating point" only has one P :P
midnightveil
left a comment
There was a problem hiding this comment.
The design notes should be improved.
| \paragraph{Core Configuration (CoreInfo)} | ||
| Describes the topology of the processor. The configuration describes | ||
| the number of the processing units, the clock source they are operating under | ||
| (one clock source may provide clock for multiple processing units, which means | ||
| changing the frequency for one unit would change the frequency of all the units | ||
| which has the same clock source), and the Operating Performance Point table. | ||
|
|
||
| \begin{lstlisting} | ||
| typedef struct { | ||
| uint64_t core_ident; // Logical Core ID | ||
| uint64_t clock_source_ident; // ID of the clock source | ||
| const OppEntry *opptable; // Pointer to valid OPPs for this core | ||
| size_t opptable_len; // Number of OPPs | ||
| } CoreInfo; | ||
| \end{lstlisting} |
There was a problem hiding this comment.
Peter cleared this up for me: the reason why we have this structure is because this is how the DTS describes things.
The reason why we have described things in terms of this structure is because this is how the DTS does generically for Linux. I think we should be explicit about this.
There was a problem hiding this comment.
Yeah, Lesley told me that it would be best to read the struct from the dts. For now, I will stress this structure and the info is derived from Linux DTS.
| \paragraph{Firmware Abstraction:} For some very new ARM platforms, System Control and | ||
| Management Interface (SCMI) is supported, providing a unified framework to manage the | ||
| the power state of the processing units. For x86 platforms, ACPI and HWP are the common | ||
| framework to provide platform agnostic power management interfaces. |
There was a problem hiding this comment.
Does this make any sense to include here if we don't use it at all?
There was a problem hiding this comment.
It is possible that we might have a board that actually supports SCMI in the future, though.
| EINVAL = 0, | ||
| } | ||
|
|
||
| pub trait FreqOps { |
There was a problem hiding this comment.
This doesn't need to be a trait.
As I've mentioned before, our API is cross-process, not through code.
There's zero reason for a trait here, especially not one with a bunch of helpers that have panic!() for no reason.
There was a problem hiding this comment.
The trait defines the behaviors of the drivers, and the drivers following the sddf protocol do not necessarily mean that the driver cannot have a set of behaviors that they need to follow internally. I can ask around to see whether others are unsatisfied with the design, and if so, I will change it.
There was a problem hiding this comment.
The trait is unnecessary, though?
| #![no_std] // Don't link the standard library | ||
| #![no_main] // Don't use the default entry point |
There was a problem hiding this comment.
Again as I've mentioned this before you don't need to say what these are.
| cpufreq = { path = "cpufreq" } | ||
| sddf-rust = { path = "../../include/sddf-rust" } | ||
| sel4-microkit = { git = "https://github.com/seL4/rust-sel4.git", tag = "v3.0.0" } | ||
| sddf-ipc-types = { git = "https://github.com/seL4/rust-sel4.git", tag = "v3.0.0" } No newline at end of file |
There was a problem hiding this comment.
(code nit) these should be part of workspace dependenies for the whole of seL4, and should not hardcode relative paths.
| [toolchain] | ||
| channel = "nightly-2025-10-20" | ||
| profile = "default" | ||
| components = [ | ||
| "rust-src", | ||
| "rustc-dev", | ||
| "llvm-tools-preview", | ||
| "rust-analyzer", | ||
| ] | ||
| targets = [ | ||
| "x86_64-unknown-linux-musl", | ||
| "aarch64-unknown-linux-musl", | ||
| "armv7-unknown-linux-musleabi", | ||
| "riscv64gc-unknown-linux-musl", | ||
| # tier 2, not available via rustup | ||
| # "riscv32gc-unknown-linux-musl", | ||
| ] No newline at end of file |
| echo "MICROKIT SDK config directory: $(microkit_sdk_config_dir)" && \ | ||
| echo "SEl4 include directories: $(sel4_include_dirs)" && \ |
There was a problem hiding this comment.
these echo are unneededed
| * @param core_ident The unique identifier of the CPU core. | ||
| * @param freq A pointer to a unsighed integer to pass back the returned frequency. | ||
| */ | ||
| static inline int32_t sddf_dvfs_get_freq(unsigned int channel, uint64_t core_ident, uint64_t *freq) |
There was a problem hiding this comment.
There is a critical problem of motivation with this protocol: why are we giving frequencies at all? This driver is meant to change the voltage AND frequency, and more importantly it can only do that for values in the operating point table. Surely we should be setting an operating point index instead right?
|
This PR is now redundant due to #667 - this version doesn't have a design that is portable to other platforms and isn't mergable due to the state of our rust support. Given that this PR has remained untouched for months and is replaced, I think we should close this PR |
|
Closing as this PR is redundant. |
Add the DVFS(Dynamic Voltage and Frequency Scaling) driver class and associated interfaces, and the implementation of the DVFS driver for the ZCU102 platform.