-
Notifications
You must be signed in to change notification settings - Fork 8.2k
[DNM] RFC clock management drivers #94679
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?
[DNM] RFC clock management drivers #94679
Conversation
66ffa2c to
d7095cc
Compare
a98c655 to
7764d24
Compare
|
|
||
| .. graphviz:: | ||
|
|
||
| digraph G { |
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.
If possible, I think the diagram would be clearer with two unidirectional arrows like
+-----------------+ +-----------------+
|clk_mgmt_get_rate| ----> |clk_mgmt_clk_rate|
| | <---- | |
+-----------------+ +-----------------+
instead of the bidirectional ones (bonus points if we can make more visually obvious about the order in which operations are executed)
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.
Bidirectional arrows added. I played with a lot of methods here for rendering this, and I haven't really found a good way to indicate the order. I've added labels to the arrows now, let me know your thoughts.
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.
Still slightly confusing because of the arrows below. If I understand correctly, the call order is as annotated here:

If the arrows were colored depending on direction - if that's supported -, I think it would be more readable, e.g.:

The two arrows with blue dots in there are what I would add (along with text saying return rate) - they should be plain orange, I just marked them to make it more obvious. (the colors also don't have to be red/orange, that is merely for illustration)
Generate macros needed to support Zephyr's clock management subsystem.
The following new macros will be generated to support clock management:
- {DT_NODE_ID}_SUPPORTS_CLK_ORDS: a comma separated list of DT ordinal
numbers for clock nodes that a given DT node supports (generally the
clock children for that node)
- {DT_NODE_ID}_CLOCK_OUTPUT_NAME_{NAME}_IDX: the index of a string
within the `clock-output-names` property for a given node, used to
access clock output phandles by their name
- {DT_NODE_ID}_CLOCK_STATE_NAME_{NAME}_IDX: the index of a string
within the `clock-state-names` property for a given node, used to
access clock state phandles by their name
Signed-off-by: Daniel DeGrasse <[email protected]>
The clock management subsystem references clock children via clock handles, which work in a manner similar to device handles, but use different section names (and only track clock children). Add gen_clock_deps.py and update elf_parser.py to handle clock handles. Update CMakeLists.txt to use a two stage link process when clock management is enabled. gen_clock_deps.py will parse a list of clock ordinals provided in the first link stage of the build, and replace the weak symbol defining those ordinals with a strong symbol defining the array of clock handles. This approach is required for clock children because directly referencing children via a pointer would result in all clock children being linked into every build, and significantly increase the image size. By only referencing children via clock handles, clocks that are not needed for a specific build (IE not referenced by any clock consumer) will be discarded during the link phase. Signed-off-by: Daniel DeGrasse <[email protected]>
Define common clock management bindings for clock producer nodes and clock consumer devices. Signed-off-by: Daniel DeGrasse <[email protected]>
Add devicetree helpers for clock management support. These helpers expose access to the new devicetree macros needed for the clock management subsystem. Also add testcases for these helpers to the devicetree lib test Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock model header, which describes macros for defining and accessing clock objects within the clock driver layer of the clock management subsystem. Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock driver API header, which defines all APIs that clock node drivers should implement, and will have access to in order to configure and request rates from their parent clocks. These APIs are all considered internal to the clock management subsystem, and should not be accessed by clock consumers (such as peripheral drivers) Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock management subsystem header. This header defines the API that will be exposed to clock consumers, and is the external facing portion of the clock subsystem. Signed-off-by: Daniel DeGrasse <[email protected]>
Add add_clock_management_header() function to cmake extensions. This can be used by drivers to register their header files as part of the clock management subsystem. Clock driver headers should be used to define the `Z_CLOCK_MANAGEMENT_XXX` macros needed for each clock to read its configuration data. Signed-off-by: Daniel DeGrasse <[email protected]>
Add initial clock management infrastructure for clock drivers. This includes common clock management functions, and the Kconfig/CMake infrastructure to define drivers. Note that all SOC clock trees should define leaf nodes within their clock tree using the "clock-output" compatible, which will be handled by the common clock management drivers. These clock output nodes can have clock states defined as child nodes. Signed-off-by: Daniel DeGrasse <[email protected]>
Implement common clock management drivers. Currently three generic drivers are available: - fixed clock source driver. Represents a non-configurable root clock source - clock source drivers. Represents a fixed clock source, with a single bit to enable or disable its output Signed-off-by: Daniel DeGrasse <[email protected]>
The NXP syscon peripheral manages clock control for LPC SOCs, as well as some iMX RT series and MCX series SOCs. Add clock node drivers for common clock types present on SOCs implementing the syscon IP. Signed-off-by: Daniel DeGrasse <[email protected]>
Implement driver for the LPC55Sxx PLL IP. These SOCs have two PLLs, one of which includes a spread spectrum generator that permits better precision when setting output clock rates, and can be used to reduce EMI in spread spectrum mode. These PLLs are specific to the LPC55Sxx series, so the drivers and compatibles are named accordingly. Signed-off-by: Daniel DeGrasse <[email protected]>
Add LPC55Sxx clock tree. This clock tree is automatically generated from MCUX clock data, and includes all clock nodes within the SOC, as well as clock output nodes where required. Signed-off-by: Daniel DeGrasse <[email protected]>
CPU nodes will be clock consumers, so add clock-device binding include to pull in properties needed by clock consumers in clock management code Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock-output property for CPU0 on the LPC55S6x, which sources its clock from the system_clock node. Signed-off-by: Daniel DeGrasse <[email protected]>
Apply default CPU0 clock state at boot for the LPC55sxx. This will allow the core clock to be configured using the clock management subsystem. Signed-off-by: Daniel DeGrasse <[email protected]>
Make clock setup for each peripheral on the LPC55S69 dependent on if the Kconfig for that peripheral driver is enabled. This reduces the flash size of the LPC55S69 hello world image, since the code to setup these clocks no longer needs to run at boot. It also better mirrors how clocks will be setup within clock management, IE where each peripheral will setup its own clocks. Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock-device include to flexcomm, as it can be used as a clock consumer within the clock subsystem. Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock outputs for all LPC55Sxx flexcomm nodes, so these nodes can request their frequency via the clock management subsystem Signed-off-by: Daniel DeGrasse <[email protected]>
Add support for clock management to the serial flexcomm driver, dependent on CONFIG_CLOCK_MGMT. When clock management is not enabled, the flexcomm driver will fall back to the clock control API. Signed-off-by: Daniel DeGrasse <[email protected]>
Add support for clock management on CPU0. This requires adding clock setup for the CPU0 core clock to run at 144MHz from PLL1, and adding clock setup for the flexcomm0 uart node to use the FROHF clock input. Signed-off-by: Daniel DeGrasse <[email protected]>
For most builds, CONFIG_CLOCK_CONTROL is still required. However, for simple applications that only use UART on the lpcxpresso55s69, it is now possible to build with CONFIG_CLOCK_CONTROL=n and run the application as expected. Move to implying this symbol so applications can opt to disable it. Signed-off-by: Daniel DeGrasse <[email protected]>
Don't configure UART clocks if clock management is on, since these clocks are handled by the framework Signed-off-by: Daniel DeGrasse <[email protected]>
The native_sim board will be used within the clock mgmt API test to verify the clock management subsystem (using emulated clock node drivers). Therefore, indicate support for clock mgmt in the board YAML so that twister will run the clock mgmt API test on it. Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock_management_api test. This test is intended to verify features of the clock management API, including the following: - verify that clock notification callbacks work as expected when a clock root is reconfigured - verify that if a driver returns an error when configuring a clock, this will be propagated to the user. - verify that consumer constraints will be able to block other consumers from reconfiguring clocks - verify that consumers can remove constraints on their clocks The test is supported on the `native_sim` target using emulated clock drivers for testing purposes in CI, and on the `lpcxpresso55s69/lpc55s69/cpu0` target to verify the clock management API on real hardware. Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock management hardware test. This test applies a series of clock states for a given consumer, and verifies that each state produces the expected rate. This test is intended to verify that each clock node driver within an SOC implementation works as expected. Boards should define their test overlays for this test to exercise as much of the clock tree as possible, and ensure some clock states do not define an explicit clocks property, to test their `clock_set_rate` and `clock_round_rate` implementations. Initial support is added for the `lpcxpresso55s69/lpc55s69/cpu0` target, as this is the only hardware supporting clock management. Signed-off-by: Daniel DeGrasse <[email protected]>
Add clock_management_minimal test. This test is intended to verify that clock management functions correctly when runtime notifications and rate setting are disabled. It also verifies that support for multiple clock outputs on a device works as expected. The test has the following phases: - apply default clock state for both clock outputs of the emulated consumer. Verify that the resulting clock frequencies match what is expected. - apply sleep clock state for both clock outputs of the emulated consumer. Verify that the resulting clock frequencies match what is expected. - Request a clock frequency from each clock output, which should match the frequency of one of the defined states exactly. Verify that the expected state is applied. The test is supported on the `native_sim` target using emulated clock drivers for testing purposes in CI, and on the `lpcxpresso55s69/lpc55s69/cpu0` target to verify the clock management API on real hardware. Signed-off-by: Daniel DeGrasse <[email protected]>
Add documentation for clock management subsystem. This documentation includes descriptions of the clock management consumer API, as well as implementation guidelines for clock drivers themselves Signed-off-by: Daniel DeGrasse <[email protected]>
Rework clock definition to include 3 clock types. These types are the following: - root clocks (clocks with no parent) - standard clocks (clocks with one parent) - mux clocks (clocks with multiple parents) The clocks are split up like this in order to reduce footprint, since each type of clock does not need certain API functions. For example: root clocks can have their rate queried by get_rate, and never need to recalculate their rate in the context of a parent standard clocks can always have their rate queried by recalculating it in the context of their parent's rate mux clocks need to indicate which parent they are using, but they won't modify this parent's frequency so that frequency can be used directly when querying the mux rate This change does somewhat restrict mux clock drivers, essentially these drivers will only vary in how they select a clock input. Mux clocks are assumed to be unable to scale their clock input whatsoever, and can only reject a proposed parent frequency. Signed-off-by: Daniel DeGrasse <[email protected]>
With the 3 clock classes, APIs for clock drivers need to be vastly expanded to 3 API types. All clocks are still defined as "struct clk", but much like "struct device" the underlying API can differ. One key difference here is that "struct clk" structures share some generic subsystem data within each clock type, which must be stored at the start of the private clock data pointer. This includes the parent information for clocks that have it, and runtime rate data when CONFIG_CLOCK_MANAGEMENT_RUNTIME is enabled. Signed-off-by: Daniel DeGrasse <[email protected]>
clock_management_disable_unused will change behavior slightly, as we now have real usage counting for all clocks in the system. Signed-off-by: Daniel DeGrasse <[email protected]>
7764d24 to
318e530
Compare
|
@mathieuchopstm thanks for the detailed doc review- I think all comments have been addressed in this push (which also included a rebase, sorry) |
318e530 to
465f5eb
Compare
Heavily rework clock_management_common to use new APIs for clock drivers. We move a lot of driver specific code into common code here. Key changes: - clock drivers no longer call the clock driver API themselves, they simply implement the functions. The clock management framework should be the only one calling these functions - getting clock rate is handled generically within the clock management subsystem code, using a combination of clock_get_parent, clock_get_rate, and clock_recalc_rate - clock_set_rate/clock_round_rate have been heavily reworked. In particular multiplexer clocks have clock_round_rate implemented generically- they only support setting their parent now - clock notifications are handled generically. If a clock can't support a new parent rate, it can report this fact in clock_recalc_rate. Muxes can't reject new clock rates (as previously mentioned) Signed-off-by: Daniel DeGrasse <[email protected]>
Rework all clock management drivers used in the emulated clock management driver test to use the new APIs. Note that drivers need to use a different CLOCK_DT_DEFINE macro based on the type of clock they fall into. Signed-off-by: Daniel DeGrasse <[email protected]>
Add a series of clock management helpers. These API functions can be used by clock drivers that cannot rely on the generic clock subsystem for some portion of their implementation. This permits clock drivers to directly interface with other clocks. This may be needed in cases like the following: - requesting a different rate from the parent then what is offered by default - manually checking a rate the clock will reconfigure to in the process of setting a rate with children - directly checking frequency of another clock in the subsystem Signed-off-by: Daniel DeGrasse <[email protected]>
Rework NXP LPC clock management drivers to use the new API definitions. Signed-off-by: Daniel DeGrasse <[email protected]>
Add the following properties to all clock nodes: - clock-ranking: Fixed integer rank of the clock node - clock-rank-factor: Multiplier to apply to clock frequency when determining rank. Add a new user API, clock_management_request_ranked. This API will return the best clock configuration for a frequency request based on the clock node ranks. The ranking of a clock configuration is calculated as the sum of the following for each clock used in the configuration: clock-ranking + (<clock-output-frequency> * clock-rank-factor) / 255 Rank requests are also "sticky" - this means if a consumer requests a maximum rank from an output, no configuration with a sum exceeding that rank can be applied. Clock rankings are lowest first, so clocks with the default rank property values of 0 will be assumed to be "perfect" clocks. The user can assign any meaning to these properties, for example they could be used to encode clock power utilization. Signed-off-by: Daniel DeGrasse <[email protected]>
Add testcase for clock_management_req_ranked, to verify that clock ranking works as expected. Signed-off-by: Daniel DeGrasse <[email protected]>
Add testcase to verify support for clock_management on/off into the API testsuite. Signed-off-by: Daniel DeGrasse <[email protected]>
Add framework-wide mutex that locks access to the clock framework when one thread is utilizing user APIs. Signed-off-by: Daniel DeGrasse <[email protected]>
Update clock subsystem documentation to reflect the new subsystem implementation Signed-off-by: Daniel DeGrasse <[email protected]>
465f5eb to
378067c
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.
A few typos remaining + some remarks which might belong better in the other thread, though some are more related to the implementation than the subsystem itself:
| /** | ||
| * @brief Get a clock's only parent. | ||
| * | ||
| * This macro gets a pointer to the clock's only parent, handling the difference | ||
| * in access between when CONFIG_CLOCK_MANAGEMENT_RUNTIME is enabled or not. | ||
| * It should only be used by "standard type" clocks. | ||
| */ | ||
| #define GET_CLK_PARENT(clk) (((struct clk_standard_subsys_data *)clk->hw_data)->parent) | ||
| /** | ||
| * @brief Get a clock's parent array. | ||
| * | ||
| * This macro gets a pointer to the clock's parent array, handling the difference | ||
| * in access between when CONFIG_CLOCK_MANAGEMENT_RUNTIME is enabled or not. | ||
| * It should only be used by "multiplexer type" clocks. | ||
| */ | ||
| #define GET_CLK_PARENTS(clk) (((struct clk_mux_subsys_data *)clk->hw_data)->parents) |
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 macro gets a pointer to the clock's only parent, handling the difference in access between when CONFIG_CLOCK_MANAGEMENT_RUNTIME is enabled or not. It should only be used by "standard type" clocks.
I don't see two definitions of the macro though. Am I missing how this is achieved? Otherwise, I don't really see why this macro exists.
(If the point was "do not let drivers touch the fields directly", then another macro to access mux->hw_data->parent_cnt should be added)
Minor nit: it's a bit wasteful to consume clk because usually the driver will have pre-fetched clk->hw_data.
| static int syscon_clock_mux_configure_recalc(const struct clk *clk_hw, | ||
| const void *mux) | ||
| { | ||
| const struct syscon_clock_mux_config *config = clk_hw->hw_data; | ||
|
|
||
| if (((uint32_t)mux) > config->parent_cnt) { | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| return (int)(uintptr_t)mux; | ||
| } |
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.
Could be made subsystem-wide as most (if not all) muxes will have this as configure_recalc, even if they are distinct (e.g., because they support the NXP safemux feature). Such drivers would do api = { .configure_recalc = cms_mux_default_configure_recalc. I'd even argue this check could be done by the subsystem beforehand - since parents are now managed by subsystem - and the routine be only provided if the mux has special needs... if any such mutex exists!
Nit: I don't like configure_recalc as a name for muxes
| #if defined(CONFIG_CLOCK_MANAGEMENT_SET_RATE) | ||
| static int syscon_clock_mux_set_parent(const struct clk *clk_hw, | ||
| uint8_t new_idx) | ||
| { | ||
| return syscon_clock_mux_configure(clk_hw, (const void *)(uintptr_t)new_idx); | ||
| } | ||
| #endif |
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 really wonder if there are muxes where this won't always be the case.
| /** | ||
| * @brief Macro to initialize standard clock data fields | ||
| * | ||
| * This macro initializes standard clock data fields used by the clock subsystem. | ||
| * It should be placed within clock data structure initializations like so: | ||
| * struct myclk_hw_data hw_data = { | ||
| * STANDARD_CLK_SUBSYS_DATA_INIT(parent_clk) | ||
| * } | ||
| * @param parent_clk Pointer to the parent clock @ref clk structure for this clock | ||
| */ | ||
| #define STANDARD_CLK_SUBSYS_DATA_INIT(parent_clk) \ | ||
| .parent = parent_clk, |
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's very little added value in this macro. Instead, it should consume the DT node as parameter and do the CLOCK_DT_GET(DT_NODE_PARENT()) on behalf of the caller.
| #define MUX_CLK_SUBSYS_DATA_INIT(parent_clks, parent_count) \ | ||
| .parents = parent_clks, \ | ||
| .parent_cnt = parent_count, |
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.
Ditto here, though this one is arguably more tricky (because the array can't be defined inline... unless?). Maybe we'd need a second macro.
| /* | ||
| * To calculate a target multiplier value, we use the formula: | ||
| * MULT = DIV(in_clk - out_clk)/out_clk | ||
| */ | ||
| mult = SYSCON_FLEXFRGXCTRL_DIV_MASK * ((parent_rate - rate_req) / | ||
| rate_req); | ||
|
|
||
| return syscon_clock_frg_calc_rate(parent_rate, mult); | ||
| } | ||
|
|
||
| static clock_freq_t syscon_clock_frg_set_rate(const struct clk *clk_hw, | ||
| clock_freq_t rate_req, | ||
| clock_freq_t parent_rate) | ||
| { | ||
| const struct syscon_clock_frg_config *config = clk_hw->hw_data; | ||
| uint32_t mult, mult_val; | ||
| clock_freq_t output_rate; | ||
|
|
||
| /* FRG rate is calculated as out_clk = in_clk / (1 + (MULT/DIV)) */ | ||
| if (rate_req < parent_rate / 2) { | ||
| /* We can't support this request */ | ||
| return -ENOTSUP; | ||
| } | ||
| /* | ||
| * To calculate a target multiplier value, we use the formula: | ||
| * MULT = DIV(in_clk - out_clk)/out_clk | ||
| */ | ||
| mult = SYSCON_FLEXFRGXCTRL_DIV_MASK * ((parent_rate - rate_req) / | ||
| rate_req); | ||
|
|
||
| mult_val = FIELD_PREP(SYSCON_FLEXFRGXCTRL_MULT_MASK, mult); | ||
|
|
||
| /* Check if multiplier value exceeds mask range- if so, the FRG will | ||
| * generate a rate equal to input clock divided by 2 | ||
| */ | ||
| if (mult > 255) { | ||
| output_rate = parent_rate / 2; | ||
| } else { | ||
| output_rate = syscon_clock_frg_calc_rate(parent_rate, mult); | ||
| } |
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.
my previous "merge both routines" suggestion doesn't work with this though, but maybe that's a bug in this implementation of round_rate?
| static clock_freq_t syscon_clock_gate_recalc_rate(const struct clk *clk_hw, | ||
| clock_freq_t parent_rate) | ||
| { | ||
| const struct syscon_clock_gate_config *config = clk_hw->hw_data; | ||
|
|
||
| return ((*config->reg) & BIT(config->enable_offset)) ? | ||
| parent_rate : 0; | ||
| } | ||
|
|
||
| static int syscon_clock_gate_configure(const struct clk *clk_hw, const void *data) | ||
| { | ||
| const struct syscon_clock_gate_config *config = clk_hw->hw_data; | ||
| bool ungate = (bool)data; | ||
|
|
||
| if (ungate) { | ||
| (*config->reg) |= BIT(config->enable_offset); | ||
| } else { | ||
| (*config->reg) &= ~BIT(config->enable_offset); | ||
| } | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| #if defined(CONFIG_CLOCK_MANAGEMENT_RUNTIME) | ||
| static clock_freq_t syscon_clock_gate_configure_recalc(const struct clk *clk_hw, | ||
| const void *data, | ||
| clock_freq_t parent_rate) | ||
| { | ||
| bool ungate = (bool)data; | ||
|
|
||
| return (ungate) ? parent_rate : 0; | ||
| } |
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'd make a similar remark with recalc_rate vs configure_recalc, where really both could be:
recalc_rate() { return rate_compute(*clk_hw->reg); }
configure_recalc() { return rate_compute((uintptr_t)data); }(We could reserve a magic value of data (e.g. 0xFFFFFFFFu) to indicate "compute from HW regs")
| ********************** | ||
|
|
||
| In order to interact with the clock tree, clock consumers must define and | ||
| initialize a clock output device. For devices defined in devicetree, which |
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.
Re-reading this: maybe described?
| initialize a clock output device. For devices defined in devicetree, which | |
| initialize a clock output device. For devices described in devicetree, which |
| periph0: periph@0 { | ||
| compatible = "vnd,mydev"; | ||
| /* Clock outputs */ | ||
| clock-outputs= <&periph_hs_clock &periph_lp_clock>; |
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.
| clock-outputs= <&periph_hs_clock &periph_lp_clock>; | |
| clock-outputs = <&periph_hs_clock &periph_lp_clock>; |
|
|
||
| .. graphviz:: | ||
|
|
||
| digraph G { |
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.
Still slightly confusing because of the arrows below. If I understand correctly, the call order is as annotated here:

If the arrows were colored depending on direction - if that's supported -, I think it would be more readable, e.g.:

The two arrows with blue dots in there are what I would add (along with text saying return rate) - they should be plain orange, I just marked them to make it more obvious. (the colors also don't have to be red/orange, that is merely for illustration)



This PR should not be merged, it is simply being used to validate CI before I push to the primary PR since I no longer can access that branch (
#72102)