[RFC] Introduce Clock Management Subsystem (Clock Driver Based)

[danieldegrasse]

Introduction

This PR proposes a clock management subsystem. It is opened as an alternative to #70467. The eventual goal would be to replace the existing clock control drivers with implementations using the clock management subsystem. This subsystem defines clock control hardware within the devicetree, and abstracts configuration of clocks to "clock states", which reference the clock control devicetree nodes in order to configure the clock tree.

Problem description

The core goal of this change is to provide a more device-agnostic way to manage clocks. Although the clock control subsystem does define clocks as an opaque type, drivers themselves still often need to be aware of the underlying type behind this opaque definition, and there is no standard for how many cells will be present on a given clock controller, so implementation details of the clock driver are prone to leaking into drivers. This presents a problem for vendors that reuse IP blocks across SOC lines with different clock controller drivers.

Beyond this, the clock control subsystem doesn't provide a simple way to configure clocks. clock_control_configure and clock_control_set_rate are both available, but clock_control_configure is ripe for leaking implementation details to the driver, and clock_control_set_rate will likely require runtime calculations to achieve requested clock rates that aren't realistic for small embedded devices (or leak implementation details, if clock_control_subsys_rate_t isn't an integer)

Proposed change

This proposal provides the initial infrastructure for clock management, as well as an implementation on the LPC55S69 and an initial driver conversion for the Flexcomm serial driver (mostly for demonstration purposes). Long term, the goal would be to transition all SOCs to this subsystem, and deprecate the clock control API. The subsystem has been designed so it can exist alongside clock control (much like pinmux and pinctrl) in order to make this transition smoother.

The application is expected to assign clock states within devicetree, so the driver should have no awareness of the contents of a clock state, only the target state name. Clock outputs are also assigned within the SOC devicetree, so drivers do not see the details of these either.

In order to fully abstract clock management details from consumers, the clock management layer is split into two layers:

• clock management layer: public facing, used by consumers to query rates from and reconfigure their clocks
• clock driver layer: internal to clock management, used by clock drivers to query rates from and reconfigure their parent sources

This split is required because not all applications want the flash overhead of enabling runtime rate resolution, so clock states need to be opaque to the consumer. When a consumer requests a rate directly via clock_mgmt_req_rate, the request will be satisfied by one of the predefined states for the clock, unless runtime rate resolution is enabled. Consumers can also apply states directly via clock_mgmt_apply_state.

Detailed RFC

Clock Management Layer

The clock management layer is the public API that consumers use to interact with clocks. Each consumer will have a set of clock states defined, along with an array of clock outputs. Consumers can query rates of their output clocks, and apply new clock states at any time.

The Clock Management API exposes the following functions:

• clock_mgmt_get_rate: Reads a clock rate from a given clock output in Hz
• clock_mgmt_apply_state: Applies a new clock state from those defined in the consumer's devicetree node
• clock_mgmt_set_callback: Sets a callback to fire before any of the clock outputs defined for this consumer are reconfigured. A negative return value from this callback will prevent the clock from being reconfigured.
• clock_mgmt_disabled_unused: Disable any clock sources that are not actively in use
• clock_mgmt_req_rate: Request a frequency range from a clock output

A given clock consumer might define clocks states and outputs like so:

&mydev_clock_source {
    mydev_state_default: mydev-state-default {
        compatible = "clock-state";
	clock-frequency = <DT_FREQ_M(10)>;
	clocks = <&mydev_div 1 &mydev_mux 3>;
    };
    
    mydev_state_sleep: mydev-state-sleep {
        compatible = "clock-state";
	clock-frequency = <DT_FREQ_M(1)>;
	clocks = <&mydev_div 2 &mydev_mux 0>;
    };
}

mydev {
    ...
    compatible = "vnd,mydev";
    clock-outputs = <&mydev_clock_source>;
    clock-output-names = "default";
    clock-state-0 = <&mydev_state_default>;
    clock-state-1 = <&mydev_state_sleep>;
    clock-state-names = "default", "sleep";
    ...
};

Note that the cells for each node within the clocks property of a state are specific to that node's compatible. It is expected that this values will be used to configure settings like multiplexer selections or divider values directly.

The consumer's driver would then interact with the clock management API like so:

/* A driver for the "vnd,mydev" compatible device */
#define DT_DRV_COMPAT vnd_mydev

...
#include <zephyr/drivers/clock_mgmt.h>
...

struct mydev_config {
    ...
    /* Reference to default clock */
    const struct clock_output *clk;
    /* clock default state */
    const clock_mgmt_state_t default_state;
    /* clock sleep state */
    const clock_mgmt_state_t sleep_state;
    ...
};

...

int mydev_clock_cb(const struct clock_mgmt_event *ev, const void *data)
{
    const struct device *dev = (const struct device *)data;

    if (ev->new_rate > HS_MAX_CLK_RATE) {
        /* Can't support this new rate */
        return -ENOTSUP;
    }
    if (ev->type == CLOCK_MGMT_POST_RATE_CHANGE) {
        /* Handle clock rate change ... */
    }
    return 0;
}

static int mydev_init(const struct device *dev)
{
    const struct mydev_config *config = dev->config;
    int clk_rate;
    ...
    /* Set default clock to default state */
    hs_clock_rate = clock_mgmt_apply_state(config->clk, config->default_state);
    if (hs_clock_rate < 0) {
        return hs_clock_rate;
    }
    /* Register for a callback if default clock changes rate */
    ret = clock_mgmt_set_callback(config->hs_clk, hs_clock_cb, dev);
    if (ret < 0) {
        return ret;
    }
    ...
}

#define MYDEV_DEFINE(i)                                                    \
    /* Define clock output for default clock */                            \
    CLOCK_MGMT_DT_INST_DEFINE_OUTPUT_BY_NAME(i, default);                  \
    ...                                                                    \
    static const struct mydev_config mydev_config_##i = {                  \
        ...                                                                \
        /* Initialize clock output */                                      \
        .clk = CLOCK_MGMT_DT_INST_GET_OUTPUT_BY_NAME(i, default),          \
        /* Read states for default and sleep */                            \
        .default_state = CLOCK_MGMT_DT_INST_GET_STATE(i, default,          \
						         default),         \
        .sleep_state = CLOCK_MGMT_DT_INST_GET_STATE(i, default,            \
						       sleep),             \
        ...                                                                \
    };                                                                     \
    static struct mydev_data mydev_data##i;                                \
    ...                                                                    \
								           \
    DEVICE_DT_INST_DEFINE(i, mydev_init, NULL, &mydev_data##i,             \
		          &mydev_config##i, ...);

    DT_INST_FOREACH_STATUS_OKAY(MYDEV_DEFINE)

Requesting Clock Rates versus Configuring the Clock Tree

Clock states can be defined using one of two methods: either clock rates can be requested from using clock_mgmt_req_rate, or states can be applied directly using clock_mgmt_apply_state. If CONFIG_CLOCK_MGMT_SET_RATE is enabled, clock rate requests can also be handled at runtime, which may result in more accurate clocks for a given request. However, some clock configurations may only be possibly by directly applying a state using clock_mgmt_apply_state.

Directly Configuring Clock Tree

For flash optimization or advanced use cases, the devicetree can be used to configure clock nodes directly with driver-specific data. Each clock node in the tree defines a set of specifiers within its compatible, which can be used to configure node specific settings. Each node defines two macros to parse these specifiers, based on its compatible: Z_CLOCK_MGMT_xxx_DATA_DEFINE and Z_CLOCK_MGMT_xxx_DATA_GET (where xxx is the device compatible string as an uppercase token). The expansion of Z_CLOCK_MGMT_xxx_DATA_GET for a given node and set of specifiers will be passed to the clock_configure function as a void * when that clock state is applied. This allows the user to configure clock node specific settings directly (for example, the precision targeted by a given oscillator or the frequency generation method used by a PLL). It can also be used to reduce flash usage, as parameters like PLL multiplication and division factors can be set in the devicetree, rather than being calculated at runtime.

Defining a clock state that directly configures the clock tree might look like so:

&mydev_clock_source {
    mydev_state_default: mydev-state-default {
            compatible = "clock-state";
	    clock-frequency = <DT_FREQ_M(10)>;
	    clocks = <&mydev_div 1 &mydev_mux 3>;
    };
};

This would setup the mydev_mux and mydev_div using hardware specific settings (given by their specifiers). In this case, these settings might be selected so that the clock output of mydev_clock_source would be 1MHz.

Runtime Clock Requests

When CONFIG_CLOCK_MGMT_RUNTIME is enabled, clock requests issued via clock_mgmt_req_rate will be aggregated, so that each request from a consumer is combined into one set of clock constraints. This means that if a consumer makes a request, that request is "sticky", and the clock output will reject an attempt to reconfigure it to a range outside of the requested frequency. For clock states in devicetree, the same "sticky" behavior can be achieved by adding the locking-state property to the state definition. This should be done for states on critical clocks, such as the CPU core clock, that should not be reconfigured due to another consumer applying a clock state.

Clock Driver Layer

The clock driver layer describes clock producers available on the system. Within an SOC clock tree, individual clock nodes (IE clock multiplexers, dividers, and PLLs) are considered separate producers, and should have separate devicetree definitions and drivers. Clock drivers can implement the following API functions:

• notify: Called by parent clock to notify child it is about to reconfigure to a new clock rate. Child clock can return error if this rate is not supported, or simply calculate its new rate and forward the notification to its own children
• get_rate: Called by child clock to request frequency of this clock in Hz
• configure: Called directly by clock management subsystem to reconfigure the clock node. Clock node should notify children of its new rate
• round_rate: Called by a child clock to request best frequency in Hz a parent can produce, given a requested target frequency
• set_rate: Called by a child clock to set parent to best frequency in Hz it can produce, given a requested target frequency

To implement these APIs, the clock drivers are expected to make use of the clock driver API. This API has the following functions:

• clock_get_rate: Read the rate of a given clock
• clock_round_rate: Get the best clock frequency a clock can produce given a requested target frequency
• clock_set_rate: Set a clock to the best clock frequency it can produce given a requested target frequency. Also calls clock_lock on the clock to prevent future reconfiguration by clocks besides the one taking ownership
• clock_notify_children: Notify all clock children that this clock is about to reconfigure to produce a new rate.
• clock_children_check_rate: Verify that children can accept a new rate
• clock_children_notify_pre_change: Notify children a clock is about to reconfigure
• clock_children_notify_post_change: Notify children a clock has reconfigured

As an example, a vendor multiplexer driver might get its rate like so:

static int vendor_mux_get_rate(const struct clk *clk_hw)
{
	const struct vendor_mux_get_rate *data = clk_hw->hw_data;
	int sel = *data->reg & VND_MUX_MASK;
	
	/* Return rate of active parent */
	return clock_get_rate(data->parents[sel]);
}

SOC devicetrees must define all clock outputs in devicetree. This approach is required because clock producers can reference each node directly in a clock state, in order to configure the clock tree without needing to request a clock frequency and have it applied at runtime.

An SOC clock tree therefore might look like the following:

mydev_clock_mux: mydev-clock-mux@400002b0 {
	compatible = "vnd,clock-mux";
	#clock-cells = <1>;
	reg = <0x400002b0 0x3>;
	offset = <0x0>;
	/* Other clock nodes that provide inputs to this multiplexer */
	input-sources = <&fixed_12m_clk &pll &fixed_96m_clk &no_clock>;
	#address-cells = <1>;
	#size-cells = <1>;

	/* Divider whose parent is this multiplexer */
	mydev_clock_div: mydev-clock-div@40000300 {
		compatible = "vnd,clock-div";
		#clock-cells = <1>;
		reg = <0x40000300 0x8>;

		mydev_clock_source: mydev-clock-source {
			compatible = "clock-output";
			#clock-cells = <1>;
		};
	};
};

Producers can provide specifiers when configuring a node, which will be used by the clock subsystem to determine how to configure the clock. For a clock node with the compatible vnd,clock-compat, the following
macros must be defined:

• Z_CLOCK_MGMT_VND_CLOCK_COMPAT_DATA_DEFINE: Defines any static data that is needed to configure this clock
• Z_CLOCK_MGMT_VND_CLOCK_COMPAT_DATA_GET: Gets reference to previously defined static data to configure this clock. Cast to a void* and passed to clock_configure. For simple clock drivers, this may be the only definition needed.

For example, the vnd,clock-mux compatible might have one specifier: "selector", and the following macros defined:

/* No data structure needed for mux */
#define Z_CLOCK_MGMT_VND_CLOCK_MUX_DATA_DEFINE(node_id, prop, idx)
/* Get mux configuration value */
#define Z_CLOCK_MGMT_VND_CLOCK_MUX_DATA_GET(node_id, prop, idx)         \
	DT_PHA_BY_IDX(node_id, prop, idx, selector)

The value that Z_CLOCK_MGMT_VND_CLOCK_MUX_DATA_GET expands to will be passed to the clock_configure API call for the driver implementing the vnd,clock-mux compatible. Such an implementation might look like the following:

static int vendor_mux_configure(const struct clk *clk_hw, const void *mux)
{
	const struct vendor_mux_get_rate *data = clk_hw->hw_data;
	int ret;
	uint32_t mux_val = (uint32_t)mux;
	int current_rate = clock_get_rate(clk_hw);
	int new_rate;

	if (mux_val > data->parent_count) {
		return -EINVAL;
	}
	
	new_rate = clock_get_rate(data->parents[mux_val]);

	/* Notify children of new rate, and check if they can accept it */
	ret = clock_children_check_rate(clk_hw, new_rate);
	if (ret < 0) {
		return ret;
	}
	
	/* Notify children we are about to change rate */
	ret = clock_children_notify_pre_change(clk_hw, current_rate, new_rate);
	if (ret < 0) {
		return ret;
	}

	(*data->reg) = mux_val;
	
	/* Notify children we have changed rate */
	ret = clock_children_notify_post_change(clk_hw, current_rate, new_rate);
	if (ret < 0) {
		return ret;
	}
	return 0;
}

A clock state to set the mydev_clock_mux to use the pll clock as input would then look like this:

clocks = <&mydev_clock_mux 1>;

Note the mydev_clock_source leaf node in the clock tree above. These nodes must exist as children of any clock node that can be used by a peripheral, and the peripheral must reference the mydev_clock_source node in its clock-outputs property. The clock management subsystem implements clock drivers for nodes with the clock-output compatible, which handles mapping the clock management APIs to internal clock driver APIs.

Framework Configuration

Since the clock management framework would likely be included with every SOC build, several Kconfigs are defined to enable/disable features that will not be needed for every application, and increase flash usage when enabled. These Kconfig are the following:

• CONFIG_CLOCK_MGMT_RUNTIME: Enables clocks to notify children of reconfiguration. Needed any time that peripherals will reconfigure clocks at runtime, or if clock_mgmt_disable_unused is used. Also makes requests from consumers to clock_mgmt_req_rate aggregate, so that if a customer makes a request that the clock accepts it is guaranteed the clock will not change frequency outside of those parameters.
• CONFIG_CLOCK_MGMT_SET_RATE: Enables clocks to calculate a new rate and apply it at runtime. When enabled, clock_mgmt_req_rate will use
  runtime rate resolution if no statically defined clock states satisfy a request. Also enables CONFIG_CLOCK_MGMT_RUNTIME.

Dependencies

This is of course a large change. I'm opening the RFC early for review, but if we choose this route for clock management we will need to create a tracking issue and follow a transition process similar to how we did for pin control.

Beyond this, there are a few key dependencies I'd like to highlight:

• in order to reduce flash utilization (by avoiding linking in children clock nodes an application does not need), I am referencing child clocks by clock handles (which work in a manner similar to device handles). This requires a 2 stage link process for runtime clock support as the first stage link must discard unused clock structures and the second stage link adds clock handles where needed
• Some minimal devicetree scripting changes are needed to handle clock states

Flash usage

NOTE: these numbers are subject to change! This is simply present to provide a benchmark of the rough flash impact of the clock framework with/without certain features

The below builds all configure the clock tree to output 96MHz using the internal oscillator to drive the flexcomm0 serial, and configure the LPC55S69's PLL1 to output a core clock at 144MHz (derived from the 16MHz external crystal)

# Baseline, without clock management enabled
$ west build -p always -b lpcxpresso55s69//cpu0 samples/hello_world/ -DCONFIG_CLOCK_MANAGEMENT=n -DCONFIG_CLOCK_CONTROL=y
Memory region         Used Size  Region Size  %age Used
           FLASH:       16578 B       160 KB     10.12%
             RAM:        4240 B       192 KB      2.16%
        USB_SRAM:          0 GB        16 KB      0.00%
        IDT_LIST:          0 GB        32 KB      0.00%
# Disable clock control, enable clock management. 170 additional bytes of FLASH, 16 bytes of extra RAM
$ west build -p always -b lpcxpresso55s69//cpu0 samples/hello_world/ -DCONFIG_CLOCK_MANAGEMENT=y -DCONFIG_CLOCK_CONTROL=n
Memory region         Used Size  Region Size  %age Used
           FLASH:       16748 B       160 KB     10.22%
             RAM:        4256 B       192 KB      2.16%
        USB_SRAM:          0 GB        16 KB      0.00%
        IDT_LIST:          0 GB        32 KB      0.00%
# Clock Management with notification support. 2356 additional bytes of FLASH, 56 bytes of RAM		
$ west build -p always -b lpcxpresso55s69//cpu0 samples/hello_world/ -DCONFIG_CLOCK_MANAGEMENT=y -DCONFIG_CLOCK_CONTROL=n -DCONFIG_CLOCK_MANAGEMENT_RUNTIME=y
Memory region         Used Size  Region Size  %age Used
           FLASH:       19104 B       160 KB     11.66%
             RAM:        4312 B       192 KB      2.19%
        USB_SRAM:          0 GB        16 KB      0.00%
        IDT_LIST:          0 GB        32 KB      0.00%
# Clock Management with runtime rate setting and notification support. 4632 additional bytes of FLASH, 0 bytes of RAM		
$ west build -p always -b lpcxpresso55s69//cpu0 samples/hello_world/ -DCONFIG_CLOCK_MANAGEMENT=y -DCONFIG_CLOCK_CONTROL=n -DCONFIG_CLOCK_MANAGEMENT_RUNTIME=y -DCONFIG_CLOCK_MANAGEMENT_SET_RATE=y
Memory region         Used Size  Region Size  %age Used
           FLASH:       23736 B       160 KB     14.49%
             RAM:        4312 B       192 KB      2.19%
        USB_SRAM:          0 GB        16 KB      0.00%
        IDT_LIST:          0 GB        32 KB      0.00%

Concerns and Unresolved Questions

I'm unsure what the implications of requiring a 2 stage link process for all builds with the clock control framework that have runtime clocking enabled will be for build/testing time overhead.

In many ways, the concept of clock states duplicates operating points in Linux. I'm not sure if we want to instead define clock states as operating points. The benefit of this would be for peripherals (or CPU cores) that support multiple operating points and use a regulator to select them, since we could define the target voltage with the clock state.

Currently, we aggregate clock requests sent via clock_mgmt_req_rate within the clock output driver, and the clock output driver will handle rejecting any attempt to configure a frequency outside of the constraints that have been set on it. While this results in simple application usage, I am not sure if it would instead be better to rely on consumers to reject rates they cannot handle. An approach like this would likely use less flash.

Currently we also issue callbacks at three points:

• to validate children can accept a new frequency
• before setting a new rate for the clock
• after setting a new rate for the clock

We could potentially issue only one callback, directly before reconfiguring the clock. The question here is if this would satisfy all use cases, or are there consumers that need to take a certain action right before their clock reconfigures, and a different action after? One example I can think of is a CPU that needs to raise core voltage before its frequency rises, but would reduce core voltage after its core frequency drops

Alternatives

The primary alternative to this PR would be #70467. That PR implements uses functions to apply clock states, while this PR implements the SOC clock backend using a method much more similar to the common clock framework, but wraps the implementation in a clock management subsystem using states similar to how #70467 does. This allows us to work around the "runtime rate setting" issue, since this feature can now be optional

[mathieuchopstm]

As follow-up to the presentation I have done yesterday during the Architecture Working Group meeting,
I am posting this comment to summarize the results of my analysis of the Clock Management Subsystem,
as proposed in this PR's current state, and share the points we find problematic with the current proposal.
Those interested in the "live" presentation can find the meeting's recording here.

• Incomplete feature parity between Clock Control and Clock Management Subsystems
  • clock_control_{off/on} removed: proposed replacement is clock states
    • Overloads one interface with two different purposes (configuration and "enablement")
    • ==> additional complexity/inefficiency in Device Tree and/or driver code
    • ==> abstraction provided by off/on lost: could create more coupling with hardware (issue for vendor-agnostic drivers?)
  • clock_control_get_status removed: proposed replacement is clock callbacks
    • Fundamental difference between get_status() (active) and callbacks (passive)
    • No get_status() if the (very heavy!) CONFIG_CLOCK_MANAGEMENT_RUNTIME is not enabled
  • clock_control_async_on removed: proposed replacement is clock callbacks
    • Callbacks work for the get notification that clock is on part of async_on
    • What about the trigger a clock start operation to be performed in background?
• (Too?) Many subsystem-level responsabilities fall on clock drivers
  • vnd_xxx_configure has to call clock_children_check_rate/clock_children_notify_pre_change/clock_children_notify_post_change
  • vnd_xxx_notify has to check that caller is truly the active parent, craft a new notification, forward it, gate itself...
  • ==> Lots of code patterns are repeated across drivers and thus across the image, leading to presumably higher ROM consumption
• Highly recursive code
  • ==> high stack consumption in some scenarios
  • Some recursivity might be avoidable by moving things from driver- to framework-side?
• Pain points for static clock configuration users (major use case in Zephyr?)
  • clock-frequency is required: true on clock states ==> must be provided to build
    • Always emitted in the binary (4 bytes per clock state)
    • Mostly unused in static configuration scenarios which just call apply_state once, possibly ignoring its return value
    • Changing a clock frequency near the root of clock tree requires updating all subnodes
  • Single big locking-state for entire clock tree is not user-friendly
    • Example in PR's ported board (this state is applied by SoC initialization code)
    • Clock cells are not labelled ==> reading the device tree requires cross-referencing bindings
      • ...which requires finding the binding that applies to EACH node in the first place!
      • i.e., read the build-generated zephyr.dts / find and open all applicable DTSI files
    • Little functional difference with existing STM32 solution (example) which uses nodes and properties instead
      • Easier to understand: properties in DTS have an explicit label (+ scoped to specific nodes)
      • This existing solution could possibly be kept as-is (with some code to bridge with the subsystem, e.g. for get_rate())
        • ==> Risk: vendors don't migrate/use the subsystem's solution due to complexity

Additionally, the semantics of the subsystem are not defined precisely enough yet:

• The subsystem doesn't use locks/concurrency protection mechanisms
  • Problematic on SMP platforms, which are expected consumers of this new subsystem
• No information about ISR-safeness of APIs
• What's allowed in and expected from drivers by different APIs isn't clear
  • What should a driver do to the event in notify?
    • One driver ignores it; another driver recomputes all fields; a third driver recomputes only one
  • Is a driver allowed to cache its frequency for get_rate(), as done here?
  • Is calling back into the subsystem allowed? For all functions? Only some functions?
• Are interactions/collaboration between this subsystem and the Power Management planned?

From our side, the next actions to be taken for this PR to move forward are:

• Implementation of a platform covering more(/"all") "real-world" usages
  • Board porting provided in PR doesn't cover some common usage patterns of the Clock Control API
    • e.g., no clock_control_{off/on} usage in UART driver
    • (because peripheral clock gates are managed in NXP HAL)
  • Goal: evaluate impact of new subsystem on more platforms where Clock Control was used
    • ==> PoC porting of STM32 board to Clock Management Subsystem is ongoing (by me)
    • This will allow comparing binary size, performance, ease-of-use, driver maintainability, etc between old and new subsystem
• Involve more vendors into this discussion to collect opinions and feedback
  • Problems encountered with the current Clock Control API, nice-to-have/must-have for new subsystem, planned use cases...
  • All ideas/comments/remarks are helpful!

[danieldegrasse]

As follow-up to the presentation I have done yesterday during the Architecture Working Group meeting, I am posting this comment to summarize the results of my analysis of the Clock Management Subsystem, as proposed in this PR's current state, and share the points we find problematic with the current proposal. Those interested in the "live" presentation can find the meeting's recording here.

Again, I really appreciate this in depth analysis. I think this is the type of review this framework needs to really get things moving forwards :) I've tried to respond to each of your points inline- hope that is readable enough.

• Incomplete feature parity between Clock Control and Clock Management Subsystems

  • clock_control_{off/on} removed: proposed replacement is clock states

  • Overloads one interface with two different purposes (configuration and "enablement")

  • ==> additional complexity/inefficiency in Device Tree and/or driver code

  • ==> abstraction provided by off/on lost: could create more coupling with hardware (issue for vendor-agnostic drivers?)

The point you raise here about clock gates is a good one- especially the example you gave of configuring a clock (like a PLL) once, then turning it on/off.

Let's add one API pointer (for size considerations) to toggle the clock, called onoff. The clock management subsystem can wrap this API with two functions- clock_management_on and clock_management_off.

I'd propose we also use this API for reference counting when RUNTIME features are enabled- IE calls to clock_management_on must be balanced with clock_management_off.

• clock_control_get_status removed: proposed replacement is clock callbacks

  • Fundamental difference between get_status() (active) and callbacks (passive)
  • No get_status() if the (very heavy!) CONFIG_CLOCK_MANAGEMENT_RUNTIME is not enabled

• clock_control_async_on removed: proposed replacement is clock callbacks

  • Callbacks work for the get notification that clock is on part of async_on
  • What about the trigger a clock start operation to be performed in background?

I guess my core question here is what the usecase is? If we are waiting for a clock to start, we should (IMO) just require that calls to clock_configure don't return until the clock is started- if we are polling on a get_status() call that is functionally equivalent, but adds a new API to the framework (and more overhead)

Is there a case where we would wait for a clock to start in the background? In my experience clock start (like PLL lock) is pretty quick, and usually performed as a synchronous operation.

• (Too?) Many subsystem-level responsabilities fall on clock drivers

  • vnd_xxx_configure has to call clock_children_check_rate/clock_children_notify_pre_change/clock_children_notify_post_change
  • vnd_xxx_notify has to check that caller is truly the active parent, craft a new notification, forward it, gate itself...
  • ==> Lots of code patterns are repeated across drivers and thus across the image, leading to presumably higher ROM consumption

• Highly recursive code

  • ==> high stack consumption in some scenarios
  • Some recursivity might be avoidable by moving things from driver- to framework-side?

This is probably the biggest thing we need to solve (IMO). The main issue is that clock_configure is functionally a required API to make this framework work, but it is also a pain point since it does not use integer frequencies like the code in CCF does. What I'd propose is the following:

• add an API called clock_configure_recalc(struct clk *clk, void *data), which accepts a void * with the data that will then be passed to clock_configure, and This function calculates the frequency that the clock will use once clock_configure is called
• add an API called clock_recalc_rate(struct clk *clk, uint32_t parent_rate, struct clk *parent), which does exactly what the API in the Linux CCF does- recalculates the clock's frequency given a new parent rate. Allow this API to return an error to indicate the parent clock is not currently connected (for the case of muxes)
• cache the rate of clocks when calling clock_get_rate, and update it when clock_configure or clock_set_rate are called on a clock. We would only do this when RUNTIME features were enabled.
• remove the notify API entirely- these APIs make it redundant.

I think this would vastly reduce the recursion in the framework, but it would expand the code size when RUNTIME features are enabled. Personally I'm ok with this- the MINIMAL case is really the only one where I care deeply about keeping code size small. In the MINIMAL case, we would still have recursion with clock_get_rate, but I think that is acceptable, right?

• Pain points for static clock configuration users (major use case in Zephyr?)

  • clock-frequency is required: true on clock states ==> must be provided to build

    • Always emitted in the binary (4 bytes per clock state)
    • Mostly unused in static configuration scenarios which just call apply_state once, possibly ignoring its return value
    • Changing a clock frequency near the root of clock tree requires updating all subnodes

  • Single big locking-state for entire clock tree is not user-friendly

The logic behind putting clock-frequency as a required property came from earlier discussions, where we determined it made sense to expose an API directly to the user for requesting a clock rate. In the first iterations of this framework it was not possible to do this, instead to request a given frequency you had to define a state with a "clock output" node, and place the frequency as the specifier for that node.

The reason we need clock-frequency to be required is that even in the MINIMAL configuration we support requesting a frequency, but the only frequencies the framework can select from are those defined as part of a state. The vision here is that a system running with the MINIMAL configuration could still feasibly do things like runtime CPU reclocking.

• Example in PR's ported board (this state is applied by SoC initialization code)

• Clock cells are not labelled ==> reading the device tree requires cross-referencing bindings

  • ...which requires finding the binding that applies to EACH node in the first place!
  • i.e., read the build-generated zephyr.dts / find and open all applicable DTSI files

• Little functional difference with existing STM32 solution (example) which uses nodes and properties instead

  • Easier to understand: properties in DTS have an explicit label (+ scoped to specific nodes)

  • This existing solution could possibly be kept as-is (with some code to bridge with the subsystem, e.g. for get_rate())

    • ==> Risk: vendors don't migrate/use the subsystem's solution due to complexity

I fully agree that the devicetree defined within this PR is harder to read than the one used for static configuration on parts like STM32- this point has been mentioned before, and I welcome any proposed alternative.

The requirements for how we describe clock states in devicetree are the following (IMO):

• per node configuration (IE ability to directly define settings for a specific PLL or clock mux)
• ability to define multiple configurations per each clock node

One alternative we could use to describe configurations would be something like the following:

/* Clock node for a divisor */
&abhclkdiv {
		abhclkdiv_3: ahbclkdiv-3 {
				div = <3>;
		};
		abhclkdiv_4: ahbclkdiv-4 {
				div = <4>;
		};
};
/* Clock node for a multiplexer */
&mainclksela {
		mainclksela_pll0: mainclksela-pll0 {
			mux = <3>;
		};
		mainclksela_fro12m: mainclksela-fro12m {
			mux = <4>;
		};
};

/* Clock node for a PLL */
&pll0 {
		pll0_96m: pll0-96m {
			vnd,pdiv = <3>;
			vnd,ndiv = <2>;
			vnd,ssg = <0>;
			vnd,ssg_b = <2>;
		};
};

sys_clk_96mhz: sys-clk-96mhz {
		compatible = "clock-state";
		clocks = <&mainclksela_fro12m &pll0_96m &abhclkdiv_3 &mainclksela_pll0>;
		clock-frequency = <DT_FREQ_M(96)>;
};

Do you prefer this? Personally I find it painfully verbose for most clock nodes- the only place where it really becomes useful is PLLs, which are always going to have a bunch of configuration parameters

Additionally, the semantics of the subsystem are not defined precisely enough yet:

• The subsystem doesn't use locks/concurrency protection mechanisms

  • Problematic on SMP platforms, which are expected consumers of this new subsystem

This is a fair critique. Right now, clock consumers can lock a clock to a given frequency when the RUNTIME features are enabled, but no mutexes are used when configuring clocks. I think a global lock makes the most sense- IE only one thread/core can touch the clock tree at a time. I would argue for a simple mutex. This contrasts to the CCF, where there is a global spinlock taken for the clock prepare phase and a separate mutex lock used for the enable phase.

• No information about ISR-safeness of APIs

This is a good point- I will add documentation here as the PR progresses.

• What's allowed in and expected from drivers by different APIs isn't clear

• What should a driver do to the event in notify?

  • One driver ignores it; another driver recomputes all fields; a third driver recomputes only one

Right now, a driver is expected to recalculate its frequency in notify, and pass that on to child clocks via clock_notify_children, unless the clock is a "root clock"- in that case, it should calculate its own frequency, and pass that on to child devices. If the child devices indicate they aren't connected (IE a mux), the clock can power down. The notify API effectively is overloaded- it is both used to determine if a clock can power down safely, and to notify children of a clock reconfiguration. If we redesign the framework as I described above we would remove the notify API though.

• Is a driver allowed to cache its frequency for get_rate(), as done here?

This is something we need to define, I agree. PLLs get tricky, because calculating their rate at runtime from a set of configuration values is often expensive.

For the LPC PLL I cheated, and encode the frequency in the node specifier. In hindsight, I would say this should not be allowed. Let's instead set a firm restriction- a driver may encode a fixed factor within its node specifier (to avoid calculating it at runtime) but not a frequency. So drivers are not permitted to cache their own frequency, but may cache a fixed factor multiplier to apply to a given input frequency. Seem reasonable?

• Is calling back into the subsystem allowed? For all functions? Only some functions?

Drivers can call back into the subsystem, but only the APIs defined in clock_driver.h. I think this is followed throughout the device specific clock drivers (not in common code), if not please point it out- and I can document this more clearly.

• Are interactions/collaboration between this subsystem and the Power Management planned?

Not formally, but I expect there will be changes to enable that. Long term I'd like to see clock states expanded to "power management states", which include voltage targets like operating points in Linux.

From our side, the next actions to be taken for this PR to move forward are:

• Implementation of a platform covering more(/"all") "real-world" usages

  • Board porting provided in PR doesn't cover some common usage patterns of the Clock Control API

    • e.g., no clock_control_{off/on} usage in UART driver
    • (because peripheral clock gates are managed in NXP HAL)

  • Goal: evaluate impact of new subsystem on more platforms where Clock Control was used

    • ==> PoC porting of STM32 board to Clock Management Subsystem is ongoing (by me)
    • This will allow comparing binary size, performance, ease-of-use, driver maintainability, etc between old and new subsystem

• Involve more vendors into this discussion to collect opinions and feedback

  • Problems encountered with the current Clock Control API, nice-to-have/must-have for new subsystem, planned use cases...
  • All ideas/comments/remarks are helpful!

I've described this inline above, but here are my proposed changes summarized:

Added APIs

• clock_configure_recalc(struct clk *clk, void *data): This function calculates the frequency that the clock will use once clock_configure is called.
• clock_recalc_rate(struct clk *clk, uint32_t parent_rate, struct clk *parent): This function calculates the frequency that the clock will use once the parent clock has the new rate applied. Can return an error if the parent isn't connected (IE for a multiplexer), or if the clock can't accept the new rate (IE if a clock has been locked to a given rate)
• clock_onoff(struct clk *clk, bool on): Turn the clock on/off. This API blocks until the clock is locked and stable.

Removed APIs

• notify API- this will be redundant with the recalc APIs above and functional changes to the framework
• clock_notify* APIs- these will all be handled in generic code.

Functional changes

• cache the rate of clocks when calling clock_get_rate, and update it when clock_configure or clock_set_rate are called on a clock. We would only do this when RUNTIME features were enabled.
• When clock_management_apply_state is called with RUNTIME features enabled, first call clock_configure_recalc on the clock, then call clock_recalc_rate on its children. Traverse down the clock tree until we find leaf nodes (clock outputs), then notify the leaf nodes we are about to reconfigure the clock, reconfigure it, and store the new frequencies for all clocks.

This will still result in recursion down the clock tree as we notify child clocks, but it will significantly reduce the code duplication.

@mathieuchopstm can you provide some feedback on these proposed changes before I begin work on them? I will likely do so on another branch, then can provide some ROM/RAM overhead numbers once I have an initial implementation.

[dleach02]

@danieldegrasse another use case that came up internally would be for dual core SOCs where they share a common source clock. Changing the frequency on one core can have an impact of the frequency on the other core.

This should be possible to handle, but I don't think it is the responsibility of the subsystem. Instead, both cores would register notification callbacks for the clock they shared, and in the notification callback remotely message the other core to confirm that the requested frequency was valid. If a notification callback returns an error, the clock management subsystem won't apply the requested frequency.

The only potential change we could consider here would be to forward the clock management callbacks with type CLOCK_MANAGEMENT_QUERY_RATE_CHANGE to all consumers, so that consumers with special requirements like these could reject a proposed frequency before the clock management subsystem tried to apply it

Wouldn't the clock management subsystem be owned by one core? What I'm wondering is not a distributed management across the cores but that one core owns it and some mechanism to communicate from the other core the requested change.

[JoeyFreeland-DojoFive]

I was asked by NXP to put some of my I2S use case comments here, as it may aid in the development of your clock API. Basically I am trying to use an I2S device on the NXP RW612, which per the driver implementation sources its clock from the chip's audio PLL. However, that PLL will need to be properly configured based on the input sample rate. This would probably not be done in the device tree, since the sampling rate is configured at runtime through Zephyr's I2S API. I am not fully aware of the end purpose/use of your API but hopefully my comment is helpful to some degree.

[bjarki-andreasen]

For peripherals which have strict accuracy/precision requirements, which is usually the reason we care about choosing specific source clocks and trees which are not just the "lowest power" ones, we need to specify and track more than the "rate", which seems to "just" be the target frequency in this PR. The output frequency alone is not enough for a device like CAN controller or I2S controller, or a subsystem like Bluetooth, to determine if whatever new clock setting is appropriate.

I don't see how this API can be used to, for example, decide the approproate source clock, potential PLL settings, etc. to meet the strict requirements of a CAN controller. An example from experience, is configuring the clock of a CAN peripheral on an NXP S32 to run at 1MHz. Trying to source the PLL from an internal 8MHz RC oscillator could produce the correct target frequency, but the accuracy was simply to low, so if enough 1's where sent on the bus, synchronization was lost.

If this subsystem worked with a struct like

struct clock_driver_rate {
        uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
        uint32_t accuracy_ppb;  /**<< max error from 0 to frequency_hz in 10^-9 intervals */
        uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

instead of "just" the frequency, all information describing a clock signal is accounted for. In the simplest systems where accuracy and precision are presumed to be "good enough", accuracy and precision could just be set to 0, which means "ideal clock rate" basically :)

Is there a way to achieve this with this PR already? or is the aim of the PR something different than what I'm envisioning?

[danieldegrasse]

@danieldegrasse another use case that came up internally would be for dual core SOCs where they share a common source clock. Changing the frequency on one core can have an impact of the frequency on the other core.

This should be possible to handle, but I don't think it is the responsibility of the subsystem. Instead, both cores would register notification callbacks for the clock they shared, and in the notification callback remotely message the other core to confirm that the requested frequency was valid. If a notification callback returns an error, the clock management subsystem won't apply the requested frequency.
The only potential change we could consider here would be to forward the clock management callbacks with type CLOCK_MANAGEMENT_QUERY_RATE_CHANGE to all consumers, so that consumers with special requirements like these could reject a proposed frequency before the clock management subsystem tried to apply it

Wouldn't the clock management subsystem be owned by one core? What I'm wondering is not a distributed management across the cores but that one core owns it and some mechanism to communicate from the other core the requested change.

Ah ok- that does make more sense, yeah. The alternative here would be placing data for the clock subsystem in shared memory, so each core could access it.

I suppose other cores would issue requests for a state or frequency, and the primary core would field those requests. We could build a generic subsystem based on IPC on top of this framework, but I think that is somewhat out of scope of the initial PR. Shared devices is sort of a broader issue within Zephyr, it just is more likely to affect clocks- we don't really have any facility for forwarding API requests for devices between cores. I think in the initial design of this framework, the expectation would be that only one core is configuring root clocks, and secondary cores only touch clock configuration for peripherals they are using.

[danieldegrasse]

I was asked by NXP to put some of my I2S use case comments here, as it may aid in the development of your clock API. Basically I am trying to use an I2S device on the NXP RW612, which per the driver implementation sources its clock from the chip's audio PLL. However, that PLL will need to be properly configured based on the input sample rate. This would probably not be done in the device tree, since the sampling rate is configured at runtime through Zephyr's I2S API. I am not fully aware of the end purpose/use of your API but hopefully my comment is helpful to some degree.

This would still go through the clock API, but not necessarily in the devicetree- it is why the proposed API supports runtime rate requests. There are two ways to support runtime rate requests, which I'll describe below:

Option A: select from predefined states

When clock states are statically defined in the devicetree, they must have an output frequency set as well. This is required so that if a consumer requests a frequency from the clock management subsystem when CONFIG_CLOCK_MANAGEMENT_SET_RATE is disabled, the subsystem will select the static clock state with the output frequency closest to what is requested

Option B: Enable CONFIG_CLOCK_MANAGEMENT_SET_RATE

This option comes with an appreciable flash/runtime overhead, but it will enable the audio PLL (or any other PLL) to calculate the best possible settings for a requested frequency- this would enable the use case you describe well

Generally, if the I2S sampling rate can be known at compile time, I'd recommend the user add a predefined state with the output frequency they expect the PLL to be configured for, but option B is always available (and would be important for cases where the frequency isn't know at compile time)

[danieldegrasse]

For peripherals which have strict accuracy/precision requirements, which is usually the reason we care about choosing specific source clocks and trees which are not just the "lowest power" ones, we need to specify and track more than the "rate", which seems to "just" be the target frequency in this PR. The output frequency alone is not enough for a device like CAN controller or I2S controller, or a subsystem like Bluetooth, to determine if whatever new clock setting is appropriate.

I don't see how this API can be used to, for example, decide the approproate source clock, potential PLL settings, etc. to meet the strict requirements of a CAN controller. An example from experience, is configuring the clock of a CAN peripheral on an NXP S32 to run at 1MHz. Trying to source the PLL from an internal 8MHz RC oscillator could produce the correct target frequency, but the accuracy was simply to low, so if enough 1's where sent on the bus, synchronization was lost.

If this subsystem worked with a struct like

struct clock_driver_rate {
        uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
        uint32_t accuracy_ppb;  /**<< max error from 0 to frequency_hz in 10^-9 intervals */
        uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

instead of "just" the frequency, all information describing a clock signal is accounted for. In the simplest systems where accuracy and precision are presumed to be "good enough", accuracy and precision could just be set to 0, which means "ideal clock rate" basically :)

Is there a way to achieve this with this PR already? or is the aim of the PR something different than what I'm envisioning?

The only way to do this in the PR currently is directly defining a clock state that configures the PLL and multiplexers in the required manner, and applying that state. It is worth mentioning that this is always possible- you can bypass all the rate selection logic even if CONFIG_CLOCK_MANAGEMENT_SET_RATE is turned on, and apply a static static/lock the clock setting to prevent a new state from being selected

So I guess to answer your question, the aim of this PR is currently not to cover precision. I'm open to expanding it to do so via a struct like you suggested- this solves my principal worry with the change, which is that I want to avoid adding another API to all clock drivers.

If we do expand the PR like this, I would advocate that we only define a struct like so:

struct clock_driver_rate {
       uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
       uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

We don't need an accuracy specification as far as I understand- a clock should always be reporting the frequency it is producing, so the only accuracy loss will be for clocks producing a frequency like 333.333 Hz. Precision is relevant though, a clock with low precision will indeed not work for all timing requirements.

I guess the question I have for you is do you think this is required, or would defining and applying a clock state be sufficient? I think it is worth noting that as far as I know the Linux common clock framework has no support for selecting clocks based on precision- I believe the accepted way to handle this is by calling clk_set_parent directly: https://github.com/torvalds/linux/blob/627277ba7c2398dc4f95cc9be8222bb2d9477800/sound/soc/samsung/smdk_spdif.c#L55.

If you feel that this is a common enough use case we should support requesting precision at runtime the same way we request frequency, I am ok to add support for it- I just want to make sure we support keeping the framework footprint small for SOCs that need it

[henrikbrixandersen]

I don't see how this API can be used to, for example, decide the approproate source clock, potential PLL settings, etc. to meet the strict requirements of a CAN controller. An example from experience, is configuring the clock of a CAN peripheral on an NXP S32 to run at 1MHz. Trying to source the PLL from an internal 8MHz RC oscillator could produce the correct target frequency, but the accuracy was simply to low, so if enough 1's where sent on the bus, synchronization was lost.

Just for the record, you would never configure a CAN clock to be 1 MHz due to the nature of CAN bit timing (each bit is divided into time quanta, which duration is determined by the CAN frequency). The recommended CAN core clock frequency is 80 MHz, 40 MHz, or 20 MHz - the higher being the best. The recommended CAN clock tolerance is 0.3% or better (see CiA 603-1).

In my experience, you would never let it be up to some subsystem to "select the best clock" to use for e.g. a CAN controller; selecting the best suitable clock is part of the system design and should be static (per power state, if needed) once selected.

[bjarki-andreasen]

I don't see how this API can be used to, for example, decide the approproate source clock, potential PLL settings, etc. to meet the strict requirements of a CAN controller. An example from experience, is configuring the clock of a CAN peripheral on an NXP S32 to run at 1MHz. Trying to source the PLL from an internal 8MHz RC oscillator could produce the correct target frequency, but the accuracy was simply to low, so if enough 1's where sent on the bus, synchronization was lost.

Just for the record, you would never configure a CAN clock to be 1 MHz due to the nature of CAN bit timing (each bit is divided into time quanta, which duration is determined by the CAN frequency). The recommended CAN core clock frequency is 80 MHz, 40 MHz, or 20 MHz - the higher being the best. The recommended CAN clock tolerance is 0.3% or better (see CiA 603-1).

Sorry, my bad, The source/quanta clock was 16MHz if I remember correctly, the CAN bitrate was 1MHz :)

In my experience, you would never let it be up to some subsystem to "select the best clock" to use for e.g. a CAN controller; selecting the best suitable clock is part of the system design and should be static (per power state, if needed) once selected.

This has been my experience so far as well, but this PR seems quite a bit more complicated than just applying a "complete" clock tree at once, or? That's where I'm a bit confused :)

[bjarki-andreasen]

For peripherals which have strict accuracy/precision requirements, which is usually the reason we care about choosing specific source clocks and trees which are not just the "lowest power" ones, we need to specify and track more than the "rate", which seems to "just" be the target frequency in this PR. The output frequency alone is not enough for a device like CAN controller or I2S controller, or a subsystem like Bluetooth, to determine if whatever new clock setting is appropriate.
I don't see how this API can be used to, for example, decide the approproate source clock, potential PLL settings, etc. to meet the strict requirements of a CAN controller. An example from experience, is configuring the clock of a CAN peripheral on an NXP S32 to run at 1MHz. Trying to source the PLL from an internal 8MHz RC oscillator could produce the correct target frequency, but the accuracy was simply to low, so if enough 1's where sent on the bus, synchronization was lost.
If this subsystem worked with a struct like

struct clock_driver_rate {
        uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
        uint32_t accuracy_ppb;  /**<< max error from 0 to frequency_hz in 10^-9 intervals */
        uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

instead of "just" the frequency, all information describing a clock signal is accounted for. In the simplest systems where accuracy and precision are presumed to be "good enough", accuracy and precision could just be set to 0, which means "ideal clock rate" basically :)
Is there a way to achieve this with this PR already? or is the aim of the PR something different than what I'm envisioning?

The only way to do this in the PR currently is directly defining a clock state that configures the PLL and multiplexers in the required manner, and applying that state. It is worth mentioning that this is always possible- you can bypass all the rate selection logic even if CONFIG_CLOCK_MANAGEMENT_SET_RATE is turned on, and apply a static static/lock the clock setting to prevent a new state from being selected

So I guess to answer your question, the aim of this PR is currently not to cover precision. I'm open to expanding it to do so via a struct like you suggested- this solves my principal worry with the change, which is that I want to avoid adding another API to all clock drivers.

If we do expand the PR like this, I would advocate that we only define a struct like so:

struct clock_driver_rate {
       uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
       uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

We don't need an accuracy specification as far as I understand- a clock should always be reporting the frequency it is producing, so the only accuracy loss will be for clocks producing a frequency like 333.333 Hz. Precision is relevant though, a clock with low precision will indeed not work for all timing requirements.

This is incorrect. Clocks are fundamentally analog devices, sinusoidal oscillators going through an edge detection circuit to produce a square wave output. They are affected by temperature and produced to meet a tolerance, just check the datasheet of any crystal oscillator and you will find an accuracy in PPM/PPB plus a host of other analog properties like temperature effects.

Accuracy of a clock is not the same as the closest ideal frequency can be produced throug PLLs, multiplexers and dividers, its the accuracy of the source clock itself, which propagates through the entire clock tree. It has nothing to do with set_rate :) Precision is a question of how good the edge detector is, in other words, is the duty cycle of the square wave exactly 0.5, or is it 0.499.

In nordic devices, and probably other vendors as well, clocks can be optimized for power or accuracy/precision. Applying more current to the clock makes its wave larger, thus better SNR and accuracy, similarly for the edge detector, more power, higher slew rate, more precise :)

I guess the question I have for you is do you think this is required, or would defining and applying a clock state be sufficient? I think it is worth noting that as far as I know the Linux common clock framework has no support for selecting clocks based on precision- I believe the accepted way to handle this is by calling clk_set_parent directly: https://github.com/torvalds/linux/blob/627277ba7c2398dc4f95cc9be8222bb2d9477800/sound/soc/samsung/smdk_spdif.c#L55.

If you feel that this is a common enough use case we should support requesting precision at runtime the same way we request frequency, I am ok to add support for it- I just want to make sure we support keeping the framework footprint small for SOCs that need it

For Nordic devices, we can't use any framework which does not take precision and accuracy into account, that's most of what we care about at runtime, to optimize for power consumption, hence the nrf_clock_control_request() APIs we use.

[XenuIsWatching]

shouldn't this be made in to atomic otherwise you could have a potential race condition here with the RMW ( as well as the other places it's tested and cleared) as I'd imagine other drivers could be looking at the same reg

[XenuIsWatching]

As you are 'gating' the gate, should this be passing in 0 rather than 1

[mathieuchopstm]

Here's my feedback on @danieldegrasse 's proposal: (I have abbreviated parts of the quotes to make the comment a bit smaller)

• Incomplete feature parity between Clock Control and Clock Management Subsystems
  [...]

The point you raise here about clock gates is a good one- especially the example you gave of configuring a clock (like a PLL) once, then turning it on/off.

Let's add one API pointer (for size considerations) to toggle the clock, called onoff. The clock management subsystem can wrap this API with two functions- clock_management_on and clock_management_off.

I'd propose we also use this API for reference counting when RUNTIME features are enabled- IE calls to clock_management_on must be balanced with clock_management_off.

Sounds good to me. I had the same idea of onoff to compress both features in a single API pointer 🙂

• clock_control_get_status removed: proposed replacement is clock callbacks
  [...]
  I guess my core question here is what the usecase is? [...]

The use case I had in mind was not at all to wait for a clock to be ready, but rather to check whether a peripheral's clock is enabled or not. This can be seen for example in the STM32 XSPI flash driver, and I have a similar usage in a not-published-yet patch for the TRNG driver.

• clock_control_async_on removed: proposed replacement is clock callbacks
  [...]
  Is there a case where we would wait for a clock to start in the background? In my experience clock start (like PLL lock) is pretty quick, and usually performed as a synchronous operation.

I'm not really bothered by clock_control_async_on's disappearance - I was mainly mentioning it for the sake of completeness.

I can only envision two ways to use this API:

1. Initiate clock start, do some work, then synchronize to "clock started" event
2. Initiate clock start and return, work in "clock started" callback

In code parlance:

/* (1) */
int vnd_drv_init(struct device *dev) {
   clock_control_async_on(/* ... */); // trigger clock start-up
      /* do some work that doesn't require clock */
   k_sem_get(/* some semaphore signaled by the clock_control_async_on callback */); // or any other synchronization mechanism
     /* do some work that requires clock */
}

/* (2) */
void clock_ready_cb() {
  /* do whatever (init hw, configure driver state, ...) */
}

int vnd_drv_init(struct device *dev) {
   clock_control_async_on(/* ... */, &clock_ready_cb);
   return 0;
}

I'm not even sure the usecase in (1) is correct during early boot (e.g., if IRQs are disabled); even if it is, I agree with your remark that the time wasted by a synchronous clock_control_on shouldn't be very significant.

As for the usecase in (2), I don't think it's valid with regard to driver init function semantics, since it returns "device is ready" without actually making the device ready (--> if code in clock_ready_cb() fails, device is marked as ready, but actually "broken").

• (Too?) Many subsystem-level responsabilities fall on clock drivers
  [...]

This is probably the biggest thing we need to solve (IMO). The main issue is that clock_configure is functionally a required API to make this framework work, but it is also a pain point since it does not use integer frequencies like the code in CCF does. What I'd propose is the following:

• add an API called clock_configure_recalc(struct clk *clk, void *data), which accepts a void * with the data that will then be passed to clock_configure, and This function calculates the frequency that the clock will use once clock_configure is called
• add an API called clock_recalc_rate(struct clk *clk, uint32_t parent_rate, struct clk *parent), which does exactly what the API in the Linux CCF does- recalculates the clock's frequency given a new parent rate. Allow this API to return an error to indicate the parent clock is not currently connected (for the case of muxes)
• cache the rate of clocks when calling clock_get_rate, and update it when clock_configure or clock_set_rate are called on a clock. We would only do this when RUNTIME features were enabled.
• remove the notify API entirely- these APIs make it redundant.

I think this would vastly reduce the recursion in the framework, but it would expand the code size when RUNTIME features are enabled. Personally I'm ok with this- the MINIMAL case is really the only one where I care deeply about keeping code size small. In the MINIMAL case, we would still have recursion with clock_get_rate, but I think that is acceptable, right?

From this description, and the one below in "Functional changes", there are still some points that are not clear to me. As far as I understand, a clock_management_configure(hw) call would now perform:

1. clock_configure_recalc(hw, data) to obtain new rate of node we're reconfiguring
2. clock_recalc_rate on its children - Q: how would this propagate down the tree?
3. clock_configure(hw, data) to apply new configuration
4. "store the new frequency for all [children?] clocks", i.e. notify them? - Q: how are they notified?

I'm all in for clock_configure_recalc; in fact, I had the exact same idea come to mind while brainstorming ideas for the subsystem. (Minor nit: shouldn't it also accept the parent's rate as parameter?) Clock caching when RUNTIME is enabled also seems acceptable.

Recursion in clock_get_rate is a tricky point. CCF seems to avoid this problem entirely by caching frequency on all nodes, but I don't think is something we want. I think it might be something we just have to pay... if done well, the cost should be as low as 8 bytes per call on ARM (and free for mux-like devices thanks to tail-call optimization), and that sounds reasonable: small devices should have small trees and thus little RAM consumption, whereas bigger targets might have deeper trees but also more RAM to compensate.

I'll mention here a silly(?) idea I had. It removes the need for recursion, but comes with the following restrictions:

• clock nodes must be of the form Fout = k * Fin where k > 0
  • i.e., multiplication and division only
    • (dividers work because (Fin / d) = (Fin * (1 / d)) = Fin * k with k = (1 / d) > 0)
  • The exception is root nodes which return a frequency, as already implemented
• the subsystem must be able to walk up to the parent node of any node
  • this could be implemented either by having all clk_foo_data start with a pointer to parent node, or via a get_parent callback, or another solution

The TL;DR is that each node returns its k via the (replacement API for) clock_get_rate, and the subsystem walks up the tree querying one node at a time, keeping track of the total factor and performing a single calculation upon reaching the root clock. The implementation could be something like:

/** 
 * Return value: top 16 bits is DIV; bottom 16 bits is MUL.
 * Node's output frequency is `Fout = (MUL/DIV) * Fin`.
 * (Pure multipliers return DIV=1; pure dividers return MUL=1)
 *
 * For root clocks, this returns the clock frequency instead.
 */
unsigned (*clock_get_muldiv)(struct clk_hw *hw);

unsigned clock_get_rate(struct clk_hw *hw)
{
  unsigned mul = 0, div = 0;
  /* the "->" notation here should be understood as "whatever mechanism we'd choose" */
  struct clk_hw *cur = hw, *parent = hw->parent;

  while (parent != NULL) {
     unsigned muldiv = hw->api->get_muldiv(hw);
     unsigned this_mul = (muldiv & 0xFFFF), this_div = (muldiv >> 16) & 0xFFFF;

     /* TBD: bad performance?! see below */
     mul *= this_mul; div *= this_div;

     cur = parent; parent = cur->parent;
  }

  /* parent == NULL: cur is a root clock */
  unsigned root_freq = hw->api->get_muldiv(hw);
  
  /* TBD: are there reasonable scenarios where `root_freq * mul` may overflow? */
  return root_freq * mul / div;
}

I'm a bit worried about the cost of two-multiplications-per-node, although we could reduce it to one-per-node (with if (this_{mul/div} != 1) checks). On paper, the drivers would have performed these multiplications anyways, so maybe it's not actually significant; more investigation in that direction could be conducted if this solution sounds acceptable.

• Pain points for static clock configuration users (major use case in Zephyr?)

  • clock-frequency is required: true on clock states ==> must be provided to build
    [...]

The logic behind putting clock-frequency as a required property came from earlier discussions, where we determined it made sense to expose an API directly to the user for requesting a clock rate. In the first iterations of this framework it was not possible to do this, instead to request a given frequency you had to define a state with a "clock output" node, and place the frequency as the specifier for that node.

The reason we need clock-frequency to be required is that even in the MINIMAL configuration we support requesting a frequency, but the only frequencies the framework can select from are those defined as part of a state. The vision here is that a system running with the MINIMAL configuration could still feasibly do things like runtime CPU reclocking.

IMO this is a bit moot: if I want to use the frequency-based system (which, in MINIMAL, is basically an indirect way of referring to states), I would add the property on the nodes where it is interesting; why should this require me to also provide it on the nodes where I don't care about it?

(But I do see your point about frequencies in states being useful, and not always replaceable with direct references to states)

• Single big locking-state for entire clock tree is not user-friendly
• Example in PR's ported board (this state is applied by SoC initialization code)
• Clock cells are not labelled ==> reading the device tree requires cross-referencing bindings
  [...]

I fully agree that the devicetree defined within this PR is harder to read than the one used for static configuration on parts like STM32- this point has been mentioned before, and I welcome any proposed alternative.

The requirements for how we describe clock states in devicetree are the following (IMO):

• per node configuration (IE ability to directly define settings for a specific PLL or clock mux)
• ability to define multiple configurations per each clock node

One alternative we could use to describe configurations would be something like the following: [...example...]

Do you prefer this? Personally I find it painfully verbose for most clock nodes- the only place where it really becomes useful is PLLs, which are always going to have a bunch of configuration parameters

The way I envisioned it was that, since all options you could configure from DTS should also be configurable via clock_management_configure(?), then we could derive a clock_configure blob from the node's properties themselves, and devices would register themselves to be clock_configure()'ed by the subsystem during boot with this blob through a to-be-defined mechanism.

However, I haven't found any idea on how to apply this idea without unsatisfactory drawbacks. A first issue is how to distinguish nodes that should be statically initialized from those that shouldn't. We can't use status because all nodes need to be "okay" for the whole subsystem to work properly (the alternative would be explicitly marking all clock tree nodes in use as status = "okay" manually, which I think we can agree is terrible). An idea could be that all configuration-related (in reality, only such properties should exist anyways?) properties be marked as optional. If no such properties are present, the device instance doesn't register to be initialized; otherwise, it asserts all properties are present and registers itself. (the assertion would have to be done in C due to bindings limitations - I think dt-schema would allow describing this set property as all-or-nothing, rather than optional? but that's another subject...)

A second issue is that it makes static configuration pretty, but the states would still use the "ugly" clock-cells-based mechanism (although, since we'd have a node-properties-to-configure-blob, maybe nodes holding properties could be used instead?). I'd argue that "complex devices" (e.g., PLLs) would usually be configured statically, and dynamic configuration is an "advanced use case" so the additional complexity is "acceptable", but I'm not sure everyone will agree with this 😅

A third (potential?) issue is that, since devices could be regrouped/bundled in a single node, configure would start encompassing too many things on a single device; at this point, it almost feels like we'd need a mechanism to configure a specific property rather than the entire device, but I don't know if we can make this in a lightweight-enough fashion to be available in MINIMAL configuration.

Maybe a middle ground is to figure out some way to instantiate a dual-role struct device/struct clk. Complex hardware (again: PLLs) would be such devices, and could have an init function, while the rest of the clock tree would use only the lightweight struct clk. (though, in a sense, that's basically the same thing as my proposal?)

• The subsystem doesn't use locks/concurrency protection mechanisms

This is a fair critique. Right now, clock consumers can lock a clock to a given frequency when the RUNTIME features are enabled, but no mutexes are used when configuring clocks. I think a global lock makes the most sense- IE only one thread/core can touch the clock tree at a time. I would argue for a simple mutex. This contrasts to the CCF, where there is a global spinlock taken for the clock prepare phase and a separate mutex lock used for the enable phase.

A simple mutex/spinlock sounds good to me.

• Is a driver allowed to cache its frequency for get_rate(), as done here?

This is something we need to define, I agree. PLLs get tricky, because calculating their rate at runtime from a set of configuration values is often expensive.

For the LPC PLL I cheated, and encode the frequency in the node specifier. In hindsight, I would say this should not be allowed. Let's instead set a firm restriction- a driver may encode a fixed factor within its node specifier (to avoid calculating it at runtime) but not a frequency. So drivers are not permitted to cache their own frequency, but may cache a fixed factor multiplier to apply to a given input frequency. Seem reasonable?

Sounds reasonable, especially if we are going towards a CCF-like u32 get_rate(clk* hw, u32 parent_rate).

• Is calling back into the subsystem allowed? For all functions? Only some functions?

Drivers can call back into the subsystem, but only the APIs defined in clock_driver.h. I think this is followed throughout the device specific clock drivers (not in common code), if not please point it out- and I can document this more clearly.

• Are interactions/collaboration between this subsystem and the Power Management planned?

Not formally, but I expect there will be changes to enable that. Long term I'd like to see clock states expanded to "power management states", which include voltage targets like operating points in Linux.

Sounds good to me.

I've described this inline above, but here are my proposed changes summarized:

Added APIs

• clock_configure_recalc(struct clk *clk, void *data): This function calculates the frequency that the clock will use once clock_configure is called.
• clock_recalc_rate(struct clk *clk, uint32_t parent_rate, struct clk *parent): This function calculates the frequency that the clock will use once the parent clock has the new rate applied. Can return an error if the parent isn't connected (IE for a multiplexer), or if the clock can't accept the new rate (IE if a clock has been locked to a given rate)
• clock_onoff(struct clk *clk, bool on): Turn the clock on/off. This API blocks until the clock is locked and stable.

Removed APIs

• notify API- this will be redundant with the recalc APIs above and functional changes to the framework
• clock_notify* APIs- these will all be handled in generic code.

Functional changes

• cache the rate of clocks when calling clock_get_rate, and update it when clock_configure or clock_set_rate are called on a clock. We would only do this when RUNTIME features were enabled.
• When clock_management_apply_state is called with RUNTIME features enabled, first call clock_configure_recalc on the clock, then call clock_recalc_rate on its children. Traverse down the clock tree until we find leaf nodes (clock outputs), then notify the leaf nodes we are about to reconfigure the clock, reconfigure it, and store the new frequencies for all clocks.

This will still result in recursion down the clock tree as we notify child clocks, but it will significantly reduce the code duplication.

@mathieuchopstm can you provide some feedback on these proposed changes before I begin work on them? I will likely do so on another branch, then can provide some ROM/RAM overhead numbers once I have an initial implementation.

Sounds good to me, with the remarks from earlier in this comment 🙂

Let me know if any point in my comment is not clear.

[danieldegrasse]

For peripherals which have strict accuracy/precision requirements, which is usually the reason we care about choosing specific source clocks and trees which are not just the "lowest power" ones, we need to specify and track more than the "rate", which seems to "just" be the target frequency in this PR. The output frequency alone is not enough for a device like CAN controller or I2S controller, or a subsystem like Bluetooth, to determine if whatever new clock setting is appropriate.
I don't see how this API can be used to, for example, decide the approproate source clock, potential PLL settings, etc. to meet the strict requirements of a CAN controller. An example from experience, is configuring the clock of a CAN peripheral on an NXP S32 to run at 1MHz. Trying to source the PLL from an internal 8MHz RC oscillator could produce the correct target frequency, but the accuracy was simply to low, so if enough 1's where sent on the bus, synchronization was lost.
If this subsystem worked with a struct like

struct clock_driver_rate {
        uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
        uint32_t accuracy_ppb;  /**<< max error from 0 to frequency_hz in 10^-9 intervals */
        uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

instead of "just" the frequency, all information describing a clock signal is accounted for. In the simplest systems where accuracy and precision are presumed to be "good enough", accuracy and precision could just be set to 0, which means "ideal clock rate" basically :)
Is there a way to achieve this with this PR already? or is the aim of the PR something different than what I'm envisioning?

The only way to do this in the PR currently is directly defining a clock state that configures the PLL and multiplexers in the required manner, and applying that state. It is worth mentioning that this is always possible- you can bypass all the rate selection logic even if CONFIG_CLOCK_MANAGEMENT_SET_RATE is turned on, and apply a static static/lock the clock setting to prevent a new state from being selected
So I guess to answer your question, the aim of this PR is currently not to cover precision. I'm open to expanding it to do so via a struct like you suggested- this solves my principal worry with the change, which is that I want to avoid adding another API to all clock drivers.
If we do expand the PR like this, I would advocate that we only define a struct like so:

struct clock_driver_rate {
       uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
       uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

We don't need an accuracy specification as far as I understand- a clock should always be reporting the frequency it is producing, so the only accuracy loss will be for clocks producing a frequency like 333.333 Hz. Precision is relevant though, a clock with low precision will indeed not work for all timing requirements.

This is incorrect. Clocks are fundamentally analog devices, sinusoidal oscillators going through an edge detection circuit to produce a square wave output. They are affected by temperature and produced to meet a tolerance, just check the datasheet of any crystal oscillator and you will find an accuracy in PPM/PPB plus a host of other analog properties like temperature effects.

Accuracy of a clock is not the same as the closest ideal frequency can be produced throug PLLs, multiplexers and dividers, its the accuracy of the source clock itself, which propagates through the entire clock tree. It has nothing to do with set_rate :) Precision is a question of how good the edge detector is, in other words, is the duty cycle of the square wave exactly 0.5, or is it 0.499.

In nordic devices, and probably other vendors as well, clocks can be optimized for power or accuracy/precision. Applying more current to the clock makes its wave larger, thus better SNR and accuracy, similarly for the edge detector, more power, higher slew rate, more precise :)

I see what you mean here now- most NXP devices I'm familiar with only quote a precision. Essentially nordic clocks can be configured to be more or less accurate to their advertised frequency, and more or less precise (IE less drift) as well, right?

I guess the question I have for you is do you think this is required, or would defining and applying a clock state be sufficient? I think it is worth noting that as far as I know the Linux common clock framework has no support for selecting clocks based on precision- I believe the accepted way to handle this is by calling clk_set_parent directly: https://github.com/torvalds/linux/blob/627277ba7c2398dc4f95cc9be8222bb2d9477800/sound/soc/samsung/smdk_spdif.c#L55.
If you feel that this is a common enough use case we should support requesting precision at runtime the same way we request frequency, I am ok to add support for it- I just want to make sure we support keeping the framework footprint small for SOCs that need it

For Nordic devices, we can't use any framework which does not take precision and accuracy into account, that's most of what we care about at runtime, to optimize for power consumption, hence the nrf_clock_control_request() APIs we use.

Do Nordic devices need the ability to request an integer precision and accuracy (in PPB) at runtime, or the ability to select a predefined state with a given precision and accuracy? I ask because if only the latter is required, we can probably just apply clock states directly for these cases.

[bjarki-andreasen]

For peripherals which have strict accuracy/precision requirements, which is usually the reason we care about choosing specific source clocks and trees which are not just the "lowest power" ones, we need to specify and track more than the "rate", which seems to "just" be the target frequency in this PR. The output frequency alone is not enough for a device like CAN controller or I2S controller, or a subsystem like Bluetooth, to determine if whatever new clock setting is appropriate.
I don't see how this API can be used to, for example, decide the approproate source clock, potential PLL settings, etc. to meet the strict requirements of a CAN controller. An example from experience, is configuring the clock of a CAN peripheral on an NXP S32 to run at 1MHz. Trying to source the PLL from an internal 8MHz RC oscillator could produce the correct target frequency, but the accuracy was simply to low, so if enough 1's where sent on the bus, synchronization was lost.
If this subsystem worked with a struct like

struct clock_driver_rate {
        uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
        uint32_t accuracy_ppb;  /**<< max error from 0 to frequency_hz in 10^-9 intervals */
        uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

instead of "just" the frequency, all information describing a clock signal is accounted for. In the simplest systems where accuracy and precision are presumed to be "good enough", accuracy and precision could just be set to 0, which means "ideal clock rate" basically :)
Is there a way to achieve this with this PR already? or is the aim of the PR something different than what I'm envisioning?

The only way to do this in the PR currently is directly defining a clock state that configures the PLL and multiplexers in the required manner, and applying that state. It is worth mentioning that this is always possible- you can bypass all the rate selection logic even if CONFIG_CLOCK_MANAGEMENT_SET_RATE is turned on, and apply a static static/lock the clock setting to prevent a new state from being selected
So I guess to answer your question, the aim of this PR is currently not to cover precision. I'm open to expanding it to do so via a struct like you suggested- this solves my principal worry with the change, which is that I want to avoid adding another API to all clock drivers.
If we do expand the PR like this, I would advocate that we only define a struct like so:

struct clock_driver_rate {
       uint32_t frequency_hz;  /**<< target output frequency in Hertz*/
       uint32_t precision_ppb; /**<< max error from 0 to 1/frequency_hz in 10^-9 intervals  */
};

We don't need an accuracy specification as far as I understand- a clock should always be reporting the frequency it is producing, so the only accuracy loss will be for clocks producing a frequency like 333.333 Hz. Precision is relevant though, a clock with low precision will indeed not work for all timing requirements.

This is incorrect. Clocks are fundamentally analog devices, sinusoidal oscillators going through an edge detection circuit to produce a square wave output. They are affected by temperature and produced to meet a tolerance, just check the datasheet of any crystal oscillator and you will find an accuracy in PPM/PPB plus a host of other analog properties like temperature effects.
Accuracy of a clock is not the same as the closest ideal frequency can be produced throug PLLs, multiplexers and dividers, its the accuracy of the source clock itself, which propagates through the entire clock tree. It has nothing to do with set_rate :) Precision is a question of how good the edge detector is, in other words, is the duty cycle of the square wave exactly 0.5, or is it 0.499.
In nordic devices, and probably other vendors as well, clocks can be optimized for power or accuracy/precision. Applying more current to the clock makes its wave larger, thus better SNR and accuracy, similarly for the edge detector, more power, higher slew rate, more precise :)

I see what you mean here now- most NXP devices I'm familiar with only quote a precision. Essentially nordic clocks can be configured to be more or less accurate to their advertised frequency, and more or less precise (IE less drift) as well, right?

yep, we provide a "race to the lowest power consumption" request API

zephyr/include/zephyr/drivers/clock_control/nrf_clock_control.h

Lines 235 to 237 in 0c82c35

	int nrf_clock_control_request(const struct device *dev,
	const struct nrf_clock_spec *spec,
	struct onoff_client *cli)

which lets multiple users specify what their worst allowed accuracy/precision, and lowest allowed frequency of a clock may be. So rather than specifying a specific clock rate or precision, we specify a minimum spec. This is quite different from many vendors since multiplexers and dividers are automatically handled by hardware in our case :)

This is not the case for all of our clocks though, we also have variable frequency clocks used for TDM etc. which do require setting a very specific frequency (again, we need to know the accuracy and precision to account for potential drift) which is even shared between modules which will need to know the frequency change, this PR does fit that usecase quite well for us to :)

I guess the question I have for you is do you think this is required, or would defining and applying a clock state be sufficient? I think it is worth noting that as far as I know the Linux common clock framework has no support for selecting clocks based on precision- I believe the accepted way to handle this is by calling clk_set_parent directly: https://github.com/torvalds/linux/blob/627277ba7c2398dc4f95cc9be8222bb2d9477800/sound/soc/samsung/smdk_spdif.c#L55.
If you feel that this is a common enough use case we should support requesting precision at runtime the same way we request frequency, I am ok to add support for it- I just want to make sure we support keeping the framework footprint small for SOCs that need it

For Nordic devices, we can't use any framework which does not take precision and accuracy into account, that's most of what we care about at runtime, to optimize for power consumption, hence the nrf_clock_control_request() APIs we use.

Do Nordic devices need the ability to request an integer precision and accuracy (in PPB) at runtime, or the ability to select a predefined state with a given precision and accuracy? I ask because if only the latter is required, we can probably just apply clock states directly for these cases.

If we use the nrf_clock_control_request API as a baseline, then yes, that is how it works currently. However, the accuracy and frequency are not completely analog, each clock has states, which result in predefined accuracies and precisions, see

zephyr/drivers/clock_control/clock_control_nrf2_lfclk.c

Lines 34 to 55 in 0c82c35

	static struct clock_options {
	uint16_t accuracy : 15;
	uint16_t precision : 1;
	nrfs_clock_src_t src;
	} clock_options[LFCLK_MAX_OPTS] = {
	{
	.accuracy = LFCLK_LFLPRC_ACCURACY,
	.precision = 0,
	.src = NRFS_CLOCK_SRC_LFCLK_LFLPRC,
	},
	{
	.accuracy = LFCLK_LFRC_ACCURACY,
	.precision = 0,
	.src = NRFS_CLOCK_SRC_LFCLK_LFRC,
	},
	{
	/* NRFS will request FLL16M use HFXO in bypass mode if SYNTH src is used */
	.accuracy = LFCLK_HFXO_ACCURACY,
	.precision = 1,
	.src = NRFS_CLOCK_SRC_LFCLK_SYNTH,
	},
	};

If the clock_control API in this PR allows us to define these internal states generically, we could instead have a higher layer API which works like nrf_clock_control_request, but looks at these clock states and applies them instead. So, if the subsystem in this PR allows us to define clock states for each clock that describe accuracy, precision and output frequency, that would match what we have now, which it looks like it does? However, this still leaves out the possibility of informing consumers of the clock that not only the frequency has changed, but also its accuracy and precision right?

BTW, note the comment in our driver /* NRFS will request FLL16M use HFXO in bypass mode if SYNTH src is used */, man I want that out of this driver with all of my heart, which again seems to be possible with this PR :)

[danieldegrasse]

I see what you mean here now- most NXP devices I'm familiar with only quote a precision. Essentially nordic clocks can be configured to be more or less accurate to their advertised frequency, and more or less precise (IE less drift) as well, right?

yep, we provide a "race to the lowest power consumption" request API

zephyr/include/zephyr/drivers/clock_control/nrf_clock_control.h

Lines 235 to 237 in 0c82c35

	int nrf_clock_control_request(const struct device *dev,
	const struct nrf_clock_spec *spec,
	struct onoff_client *cli)

which lets multiple users specify what their worst allowed accuracy/precision, and lowest allowed frequency of a clock may be. So rather than specifying a specific clock rate or precision, we specify a minimum spec. This is quite different from many vendors since multiplexers and dividers are automatically handled by hardware in our case :)

This is not the case for all of our clocks though, we also have variable frequency clocks used for TDM etc. which do require setting a very specific frequency (again, we need to know the accuracy and precision to account for potential drift) which is even shared between modules which will need to know the frequency change, this PR does fit that usecase quite well for us to :)

Thanks for pointing to this API- I see what you mean now, the framework as currently defined doesn't have a way to support this- we'd need to expand the scope of what a "request" means, and pass the frequency, accuracy, and precision within requests as well as notifications.

One concern I have with this is how we will handle multiple consumer requests when the requests have 3 parameters. For the case of frequency, we simply need to find the highest frequency that fits within the restrictions given by all consumers. However once we add precision and accuracy in this becomes a multivariable optimization problem- we will need some form of "scoring" system for clocks that weighs precision, accuracy, and frequency to determine which clock is best given all the constraints. This should be possible to add, it just may result in higher overhead.

Do Nordic devices need the ability to request an integer precision and accuracy (in PPB) at runtime, or the ability to select a predefined state with a given precision and accuracy? I ask because if only the latter is required, we can probably just apply clock states directly for these cases.

If we use the nrf_clock_control_request API as a baseline, then yes, that is how it works currently. However, the accuracy and frequency are not completely analog, each clock has states, which result in predefined accuracies and precisions, see

zephyr/drivers/clock_control/clock_control_nrf2_lfclk.c

Lines 34 to 55 in 0c82c35

	static struct clock_options {
	uint16_t accuracy : 15;
	uint16_t precision : 1;
	nrfs_clock_src_t src;
	} clock_options[LFCLK_MAX_OPTS] = {
	{
	.accuracy = LFCLK_LFLPRC_ACCURACY,
	.precision = 0,
	.src = NRFS_CLOCK_SRC_LFCLK_LFLPRC,
	},
	{
	.accuracy = LFCLK_LFRC_ACCURACY,
	.precision = 0,
	.src = NRFS_CLOCK_SRC_LFCLK_LFRC,
	},
	{
	/* NRFS will request FLL16M use HFXO in bypass mode if SYNTH src is used */
	.accuracy = LFCLK_HFXO_ACCURACY,
	.precision = 1,
	.src = NRFS_CLOCK_SRC_LFCLK_SYNTH,
	},
	};

If the clock_control API in this PR allows us to define these internal states generically, we could instead have a higher layer API which works like nrf_clock_control_request, but looks at these clock states and applies them instead. So, if the subsystem in this PR allows us to define clock states for each clock that describe accuracy, precision and output frequency, that would match what we have now, which it looks like it does? However, this still leaves out the possibility of informing consumers of the clock that not only the frequency has changed, but also its accuracy and precision right?

BTW, note the comment in our driver /* NRFS will request FLL16M use HFXO in bypass mode if SYNTH src is used */, man I want that out of this driver with all of my heart, which again seems to be possible with this PR :)

You could define the states generically- "software defined clock outputs" are supported by this PR, and would allow defining multiple states as part of a framework layer. You're correct that the generic callback API would only notify about frequency in this case though.

It seems like we probably will need to define precision and accuracy as part of the clock state to make this PR work for Nordic SOCs- I'll look at adding this in an updated revision, and let you know if I run into any implementation issues that are worth considering. I think it should be possible if we do the following though:

• define clock requests to include frequency, precision, and accuracy
• add a "score" function to compare a clock's frequency, precision, and accuracy to a requested setting so we can select the best one

[bjarki-andreasen]

I see what you mean here now- most NXP devices I'm familiar with only quote a precision. Essentially nordic clocks can be configured to be more or less accurate to their advertised frequency, and more or less precise (IE less drift) as well, right?

yep, we provide a "race to the lowest power consumption" request API

zephyr/include/zephyr/drivers/clock_control/nrf_clock_control.h

Lines 235 to 237 in 0c82c35

	int nrf_clock_control_request(const struct device *dev,
	const struct nrf_clock_spec *spec,
	struct onoff_client *cli)

which lets multiple users specify what their worst allowed accuracy/precision, and lowest allowed frequency of a clock may be. So rather than specifying a specific clock rate or precision, we specify a minimum spec. This is quite different from many vendors since multiplexers and dividers are automatically handled by hardware in our case :)

This is not the case for all of our clocks though, we also have variable frequency clocks used for TDM etc. which do require setting a very specific frequency (again, we need to know the accuracy and precision to account for potential drift) which is even shared between modules which will need to know the frequency change, this PR does fit that usecase quite well for us to :)

Thanks for pointing to this API- I see what you mean now, the framework as currently defined doesn't have a way to support this- we'd need to expand the scope of what a "request" means, and pass the frequency, accuracy, and precision within requests as well as notifications.

One concern I have with this is how we will handle multiple consumer requests when the requests have 3 parameters. For the case of frequency, we simply need to find the highest frequency that fits within the restrictions given by all consumers. However once we add precision and accuracy in this becomes a multivariable optimization problem- we will need some form of "scoring" system for clocks that weighs precision, accuracy, and frequency to determine which clock is best given all the constraints. This should be possible to add, it just may result in higher overhead.

Do Nordic devices need the ability to request an integer precision and accuracy (in PPB) at runtime, or the ability to select a predefined state with a given precision and accuracy? I ask because if only the latter is required, we can probably just apply clock states directly for these cases.

If we use the nrf_clock_control_request API as a baseline, then yes, that is how it works currently. However, the accuracy and frequency are not completely analog, each clock has states, which result in predefined accuracies and precisions, see

zephyr/drivers/clock_control/clock_control_nrf2_lfclk.c

Lines 34 to 55 in 0c82c35

	static struct clock_options {
	uint16_t accuracy : 15;
	uint16_t precision : 1;
	nrfs_clock_src_t src;
	} clock_options[LFCLK_MAX_OPTS] = {
	{
	.accuracy = LFCLK_LFLPRC_ACCURACY,
	.precision = 0,
	.src = NRFS_CLOCK_SRC_LFCLK_LFLPRC,
	},
	{
	.accuracy = LFCLK_LFRC_ACCURACY,
	.precision = 0,
	.src = NRFS_CLOCK_SRC_LFCLK_LFRC,
	},
	{
	/* NRFS will request FLL16M use HFXO in bypass mode if SYNTH src is used */
	.accuracy = LFCLK_HFXO_ACCURACY,
	.precision = 1,
	.src = NRFS_CLOCK_SRC_LFCLK_SYNTH,
	},
	};

If the clock_control API in this PR allows us to define these internal states generically, we could instead have a higher layer API which works like nrf_clock_control_request, but looks at these clock states and applies them instead. So, if the subsystem in this PR allows us to define clock states for each clock that describe accuracy, precision and output frequency, that would match what we have now, which it looks like it does? However, this still leaves out the possibility of informing consumers of the clock that not only the frequency has changed, but also its accuracy and precision right?

BTW, note the comment in our driver /* NRFS will request FLL16M use HFXO in bypass mode if SYNTH src is used */, man I want that out of this driver with all of my heart, which again seems to be possible with this PR :)

You could define the states generically- "software defined clock outputs" are supported by this PR, and would allow defining multiple states as part of a framework layer. You're correct that the generic callback API would only notify about frequency in this case though.

It seems like we probably will need to define precision and accuracy as part of the clock state to make this PR work for Nordic SOCs- I'll look at adding this in an updated revision, and let you know if I run into any implementation issues that are worth considering. I think it should be possible if we do the following though:

• define clock requests to include frequency, precision, and accuracy
• add a "score" function to compare a clock's frequency, precision, and accuracy to a requested setting so we can select the best one

Thr score part I don't even know where to start on :D In nordic, the score is the lowest power consuming clock state which meets the "highest" required spec :)

[dleach02]

@bjarki-andreasen are there any other scoring use cases that should be considered? I understand power so I could imagine that the subsystem could, by default, target the best power profile but would there be other targets to optimize for? Best precision and/or accuracy might be another use case where power considerations are not needed.

Maybe the subsystem could take in a "what to optimize" for parameter and the underlying algorithm would then do the tuning.

[bjarki-andreasen]

@bjarki-andreasen are there any other scoring use cases that should be considered? I understand power so I could imagine that the subsystem could, by default, target the best power profile but would there be other targets to optimize for? Best precision and/or accuracy might be another use case where power considerations are not needed.

If power considerations are not needed, then scoring is not needed, just always have all clocks at highest everything :)

Maybe the subsystem could take in a "what to optimize" for parameter and the underlying algorithm would then do the tuning.

I think the scoring could be done at the "application" level, as long as the clock state options are available to the "user", be that driver, subsystem or app, these choices can be made with regards for whatever the user cares about.

[mathieuchopstm]

A first (partial) implementation of the Clock Management Subsystem on STM32 hardware (STM32C0 series) is available here: https://github.com/mathieuchopstm/zephyr/tree/clock_mgmt_stm32

Only the static configuration is supported; RUNTIME/SET_RATE are (mostly) not implemented. For certain nodes, I implemented both a "generic" driver, and an STM32-optimized driver that uses common code for register R/W operations. I expected such centralization to reduce the footprint, and preliminary results appear to confirm my assumptions.

You should be able to reproduce my results by using west build -b nucleo_c071rb samples/hello_world/ <config> -p where:

• -DCONFIG_CLOCK_CONTROL=y -DCONFIG_CLOCK_MANAGEMENT=n for ClkCtrl
• -DCONFIG_CLOCK_CONTROL=n -DCONFIG_CLOCK_MANAGEMENT=y for ClkMgmt
• -DCONFIG_CLOCK_CONTROL=n -DCONFIG_CLOCK_MANAGEMENT=y -DCONFIG_CLOCK_MANAGEMENT_STM32_INLINE=y for ClkMgmt+inline
• by default, +optdrv; set #if 0 ==> #if 1 here to use generic drivers
  • +optdrv uses STM32-optimized drivers that call the centralized code for register RW operations, and use compressed register fields representation

	ClkCtrl	ClkMgmt	ClkMgmt+inline	ClkMgmt+optdrv	ClkMgmt+optdrv+inline
FLASH	13280	13758	13778	13674	13758
RAM	4136	4192	4192	4176	4176