Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Introduce Clock Management Subsystem #70467

Closed

Conversation

danieldegrasse
Copy link
Collaborator

@danieldegrasse danieldegrasse commented Mar 20, 2024

Introduction

This PR proposes a clock management subsystem. The eventual goal would be to replace the existing clock control drivers with implementations using the clock management subsystem. The subsystem abstracts clock management via "setpoints" and "subsystems".

A "setpoint" is defined as a clock tree state, which will typically contain clock settings for a given peripheral but may configure clock generators such as PLLs as well
A "subsystem" is defined as a clock output- IE a clock source that can be used directly by peripherals on the system.

Setpoints are defined in terms of power states, with a DEFAULT and SLEEP state defined by default. Drivers can define additional custom states if needed for their use case.

Drivers can both query clock rates from subsystems, and register for a callback when the state of a subsystem changes. This allows a driver to respond to clock rate changes that occur as a consequence of another driver or subsystem applying a clock setpoint.

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 setpoints within devicetree, so the driver should have no awareness of the contents of a clock setpoint, only the target power state. Subsystems are also assigned within the SOC devicetree, so drivers do not see the details of these either.

Detailed RFC

Driver/Application Side

From the driver side, clocks are fully abstracted to a set of subsystems and setpoints. The driver has no awareness of the underlying clock device.

The clock management subsystem provides the following functions for drivers to interact with clocks:

  • clock_mgmt_get_rate: Read the rate of a clock
  • clock_mgmt_apply_setpoint: Apply a clock setpoint
  • clock_mgmt_init_callback: initialize a clock callback structure
  • clock_mgmt_add_callback: add a clock callback structure, so callbacks will be issued for a given subsystem
  • clock_mgmt_remove_callback: remove a clock callback structure

Beyond this, the driver should only interact with the clock subsystem to define data structures for it, by calling CLOCK_MGMT_DEFINE and CLOCK_MGMT_INIT macros within its initialization macro (this is intentionally very similar to pinctrl).

The devicetree definitions for clocks are where hardware specific details are described. For example, a node might have the following properties:

mydev {
    ...
    clocks = <&mydev_clock_source>;
    clock-names = "default";
    clock-setpoint-0 = <&mydev_clock_mux 3>;
    clock-setpoint-1 = <&mydev_clock_mux 2>;
    clock-setpoint-names = "default", "sleep";
    ...
};

Note that the cells for each clock setpoint node are intentionally device specific. It is expected that this values will be used to configure settings like multiplexer selections or divider values directly.

The driver could then interact with the clocks defined in devicetree like so:

struct mydev_config {
    struct clock_mgmt *clk_cfg;
};

struct mdev_data {
   struct clock_mgmt_callback clock_cb;
};
...

/* Apply clock sleep setpoint */
clock_mgmt_apply_setpoint(config->clk_cfg, CLOCK_MGMT_STATE_SLEEP);
/* Get clock rate */
int clock_rate = clock_mgmt_get_rate(config->clk_cfg, CLOCK_MGMT_SUBSYS_DEFAULT);
/* Register for callback (assuming struct clock_mgmt_callback was already initialized)
clock_mgmt_add_callback(config->clk_cfg, CLOCK_MGMT_SUBSYS_DEFAULT, &data->clock_cb);

SOC Implementation Side

Note- much like pinctrl, SOCs can only have one clock management driver. This design choice was intentional- by implementing clock management like this, nodes do not need to reference a device driver phandle when configuring clocks but can instead reference clock nodes

Clock nodes are heavily based around the concept of a "clock-id". Every clock node must have this property. It is used in a similar manner to how a node label might be used by an application, but is specific to the clock subsystem.

The SOC clock tree might look like so:

pll0clksel: pll0clksel {
           clock-id = "PLL0CLKSEL";
           compatible = "clock-mux";
           #clock-cells = <1>;
           input-sources = <&fro_12m &clk_in_en &fro_1m &rtcosc32ksel
                                &no_clock &no_clock &no_clock &no_clock>;

            pll0: pll0 {
                    clock-id = "PLL0";
                    compatible = "nxp,lpc55xxx-pll";
                    #clock-cells = <6>;

                    pll0_pdec: pll0-pdec {
                            clock-id = "PLL0_PDEC";
                            compatible = "clock-div";
                            #clock-cells = <1>;
                    };
            };
    };

Note that the number of specifier cells is node specific, as is the significance. For example, the 6 clock cells on the PLL will configure standard features such as the PLL multiplier and divider, as well as parameters specific to this PLL implementation.

In order to minimize flash usage, both setpoint and subsystem handlers are defined as functions. These functions are implemented by the SOC clock management driver, and are expected to heavily utilize devicetree to result in optimized function implementations. For example, a function for setpoints might look like so:

/*
 * This template declares a clock management setpoint. The parameters
 * passed to this template are as follows:
 * @param node: node ID for device with clock-control-state-<n> property
 * @param state: identifier for current state
 */
Z_CLOCK_MGMT_SETPOINT_TEMPL(node, state)
{
    /* Checks if the setpoint has a clock node with ID CLOCK_SOURCE */
    if (DT_CLOCK_STATE_HAS_ID(node, state, CLOCK_SOURCE)) {
           set_my_clock_freq(DT_CLOCK_STATE_ID_READ_CELL_OR(node, CLOCK_SOURCE,
                             freq, state, 0));
           /* Fire callback for all clock subsystems */
           CLOCK_MGMT_FIRE_ALL_CALLBACKS(CLOCK_MGMT_RATE_CHANGED);
    }
    /* Checks if the setpoint has a clock node with ID CLOCK_DIV*/
    if (DT_CLOCK_STATE_HAS_ID(node, state, CLOCK_DIV)) {
           set_my_clock_div(DT_CLOCK_STATE_ID_READ_CELL_OR(node, CLOCK_DIV,
                            divider, state));
           /* Fire callback for clock output */
           CLOCK_MGMT_FIRE_CALLBACK(CLOCK_OUTPUT, CLOCK_MGMT_RATE_CHANGED);
    }
}

For a setpoint that only configures the CLOCK_DIV clock ID to 3, the this would optimize to the following:

if (0) {
      set_my_clock_freq(0);
      /* Fire callback for all clock subsystems */
      CLOCK_MGMT_FIRE_ALL_CALLBACKS(CLOCK_MGMT_RATE_CHANGED);
}

if (1) {
     set_my_clock_div(3);
     /* Fire callback for clock output */
     CLOCK_MGMT_FIRE_CALLBACK(CLOCK_OUTPUT, CLOCK_MGMT_RATE_CHANGED);
}

By leverage devicetree macros and GCC optimization, most setpoint functions should come out to a few function calls, rather than parsing a large C structure to determine how the clock tree should be configured.

A similar concept can be used for the subsystem handlers:

#define CLK_OUTPUT_ID 0x0

/*
 * This template declares a clock management setpoint. The parameters
 * passed to this template are as follows:
 * @param node: node ID for device with clock-control-state-<n> property
 * @param state: identifier for current state
 */
Z_CLOCK_MGMT_SUBSYS_TEMPL(node, prop, idx)
{
    /* Should expand to "CLK_OUTPUT_ID" */
    switch (CONCAT(DT_STRING_TOKEN(DT_PHANDLE_BY_IDX(node, prop, idx),
                               clock_id), _ID) == CLK_OUTPUT_ID)
    case CLK_OUTPUT_ID :
        return 1000;
    ...
    default:
       return -ENOTSUP;
    }
    /* Otherwise, querying rate not supported */
    return -ENOTSUP;
}

At this point, you may have noticed the "function" definitions look a bit strange. This is because these functions are not truly functions that will be compiled into the final image, but something I describe as "templates". These templates are the same concept as defining a function within a macro, like so:

#define Z_CLOCK_MGMT_SUBSYS_MACRO(node, prop, idx) \
     switch (CONCAT(DT_STRING_TOKEN(DT_PHANDLE_BY_IDX(node, prop, idx),
                               clock_id), _ID) == CLK_OUTPUT_ID) {
     ....

The reason for these "templates" is that macros like the above are awful to debug. Therefore, the build system will automatically take the function templates defined for an SOC, and create per-setpoint and per-subsystem function implementations, which will then be expanded by the GCC preprocessor and optimized by the complier to produce the functions in the final image.

Proposed change (Detailed)

Here I wanted to dive a bit deeper into how the clock management code works on the SOC side. There are two python scripting changes within this PR:

  • scripts/dts/gen_defines.py
  • scripts/build/gen_clock_mgmt.py

Changes to gen_defines

The changes to the gen_defines scripting are required to enable the concept of "clock-ids" covered above. The key issue here is that SOC clock drivers need a way to check if a given clock node is configured by a setpoint, and take action if so. Crucially, this information must be available at compile time to enable optimizing each setpoint function. By providing new devicetree macros to check for the presence of the clock node with a given clock ID in a setpoint, and extract properties from it if such a node exists, setpoint functions can be written that will be optimized to one or two function calls by the compiler.

The other change to the devicetree tooling is to generate a macro to iterate over every clock ID in use as a subystem. This is needed to enable callback handling, which I describe in more detail below

gen_clock_mgmt

The new gen_clock_mgmt script is a bit strange. While implementing this support, I initially tried to define clock management handlers as macros, which expand to functions. While this is possible to implement, debugging something as simple as a missed parenthesis becomes a long process of staring at code, since the compiler can't effectively report errors on a line by line basis. What gen_clock_mgmt does is take a clock driver template like the following:

clock_mgmt_lpc55xxx.c:

Z_CLOCK_MGMT_SUBSYS_TEMPL(node, prop, idx)
{
        switch (CONCAT(NXP_CLOCK_, DT_STRING_TOKEN(
                DT_PHANDLE_BY_IDX(node, clocks, idx), clock_id))) {
        case NXP_CLOCK_FXCOM0_CLOCK:
                return CLOCK_GetFlexCommClkFreq(0);
        default:
                return -ENOTSUP;
}

And expand it into function definitions like the following:
clock_mgmt_soc_generated.c:

int Z_CLOCK_MGMT_SUBSYS_FUNC_NAME(DT_N_S_soc_S_peripheral_50000000_S_spi_9f000, clocks,0)(void)
{

        switch (CONCAT(NXP_CLOCK_, DT_STRING_TOKEN(
                DT_PHANDLE_BY_IDX(DT_N_S_soc_S_peripheral_50000000_S_spi_9f000, clocks, 0), clock_id))) {
        case NXP_CLOCK_FXCOM0_CLOCK:
                return CLOCK_GetFlexCommClkFreq(0);
        default:
                return -ENOTSUP;
 }     

int Z_CLOCK_MGMT_SUBSYS_FUNC_NAME(DT_N_S_soc_S_peripheral_50000000_S_flexcomm_98000, clocks,0)(void)
{

        switch (CONCAT(NXP_CLOCK_, DT_STRING_TOKEN(
                DT_PHANDLE_BY_IDX(DT_N_S_soc_S_peripheral_50000000_S_flexcomm_98000, clocks, 0), clock_id))) {
        case NXP_CLOCK_FXCOM0_CLOCK:
                return CLOCK_GetFlexCommClkFreq(0);
        default:
                return -ENOTSUP;
}

The function definitions are placed inline within clock_mgmt_soc_generated.c, at the same location where the templates previously were in the file. Although this script effectively duplicates part of the functionality of the GCC preprocessor, I think it is justified in order to make the lives of SOC implementers much easier.

Clock Callbacks

Finally, I wanted to touch on how clock callbacks are implemented. The goal of the implementation was to enable clock callbacks to be on a per-subsystem basis, without needing to define data structures for clock subsystems that will never be used. Therefore, we use the linker to "register" clock management structures (which are simply sys_slist_t linked lists). The clock_callbacks.c common code defines a sys_slist_t structure for every clock ID present in the clock tree. Then, the CLOCK_MGMT_DEFINE macro will reference these sys_slist_t structures for a given clock subsystem. What this means in effect is that the linker will discard any sys_slist_t structure for a clock ID that is not referenced by a driver. The sys_slist_t structures are placed in named sections, so the macro to fire a clock callback for a given ID can access the correct sys_slist_t structure without actually referencing it by name (and therefore pulling it into the build)

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.

Concerns and Unresolved Questions

There are a few things I'm not sure of with this PR's implementation:

  • is the gen_clock_mgmt script worth the tradeoffs? I have concerns about parsing C code from python- although the script needs to do very little (essentially locate the bounds of two functions, and then run a find and replace), we still may run into nuances in the differences between how the GCC preprocessor handles token pasting, versus the naive find and replace used by the scripting
  • Should we instead make the clock subsystem handler and clock setpoint handler opaque types? This is similar to pin control, and gives SOC implementers more freedom. The downside I see to this is that anyone wishing to implement clock management using macros that define functions will have to debug very unwieldy macros. Additionally, I don't see another way besides functions to implement support for features like setpoints that can still take advantage of heavy optimization by the compiler.

Alternatives

I have considered extending the existing clock control API with setpoints, and in fact have a draft PR to do so here: #66732. I think this approach makes more sense because while implementing that RFC, I realized that clocks were still exposed to the driver within the clock control subsystem, so even if setpoints abstracted clock configuration drivers would still have some dependencies on the clock control driver they were written to use.

I have also considered something like Linux's common clock framework: https://www.kernel.org/doc/Documentation/clk.txt. I think with enough devicetree trickery (to optimize out clock structures that weren't needed) we could efficiently implement support for reading clock rates, but support for setting clock rates seems more challenging. Needing each clock element to keep a record of its parents and children will eat up flash space fast. The other issue with the common clock framework is that we really don't want to be calculating "best possible" rates, or anything similar. The specifiers on clock nodes within setpoints are vendor specific by design. The idea is that setpoints will directly set multiplexer and divider values for a clock state, rather than having this calculated at runtime.

The clock management subsystem requires some additional devicetree
macros be generated, in order to enable SOC clock drivers to access
clock setpoint data. The following new macros are generated:

- DT_N{node_path}_CLOCK_STATE_{idx}_{clk_id}_EXISTS: defined to 1 if a
  clock node with the clock-id property "clk_id" is referenced in the
  setpoint index "idx" for the node at "node_path". Used by SOC clock
  management code.

- DT_N{node_path}_CLOCK_STATE_{idx}_{clk_id}_IDX: set to the phandle
  index within the setpoint property at index "idx" for the node at
  "node_path", if DT_N{node_path}_CLOCK_STATE_{idx}_{clk_id}_EXISTS is
  defined for the given "clk_id". Used by SOC clock management code.

- DT_CLOCK_ID_{clk_id}_USED: defined to 1 if some node in the DTS
  references a clock node with the clock-id property "clk_id" in its
  "clocks" property. Used by SOC clock management code.

- DT_FOREACH_CLOCK_ID(fn): expands to an invocation of "fn" with each
  "clock-id" present in the devicetree. Used by generic clock callback
  code.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Add script to generate clock management code based on SOC clock
management implementation. This script has the following steps:

1. Identifies clock setpoint and clock subsystem handler implementations
   within the SOC clock management file set during the build
2. Iterates though each node in the devicetree, and:
   a) for each clock referenced in the "clocks" property, generate a
      function based on the clock subsystem handler template
   b) for each clock setpoint on the node, generate a function based on
      the clock setpoint handler template
3. Write the file "clock_mgmt_soc_generated.c", which contains the
   original contents of the SOC clock management file, but replaces the
   clock setpoint and clock subsystem handler templates with generated
   functions

If the clock setpoint and subsystem handlers were instead replaced with
macros that expanded to functions, the GCC preprocessor could fill the
role of this script. The reason for implementing it is to simplify
debugging errors with SOC clock handler implementations, as diagnosing
issues with large functions defined in macros can be very difficult.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Introduce clock management driver class. Clock management is intended to
abstract clocks via "setpoints", which correspond to a clock
configuration for a given power state (usually only for a specific
peripheral clock). The clock management driver system supports setting
clock setpoints, querying clock rates, and subscribing to callbacks on
clock rate changes.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Add common CMake and KConfig infrastructure for clock management, as
well as code for handling clock management callbacks.

Clock management callbacks are handled via a linker registration system.
A singly linked list is defined for each clock output node in the
clock_callbacks.c file. However, the linker will discard all these
linked lists, unless a driver references one within the clock callback
registration function. Therefore, only the singly linked lists needed
for clock callbacks will actually be included in the final image.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Add generic clock node bindings for common clock controller elements.
These bindings include the following:
- clock-div: for clock dividers
- clock-gate: for clock gates
- clock-multiplier: for clock multipliers
- clock-mux: for clock multiplexers capable of selecting from multiple
  inputs to generate an output
- clock-output: clock output, a clock node which will have a defined
  output frequency and can drive clocks used within the system
- clock-source: a fixed or configurable clock source output (such as an
  internal 1MHz oscillator)
- clock-device: similar to pinctrl-device, defines the clock setpoint
  properties used by nodes that leverage the clock management subsystem

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Add documentation for clock management subsystem, which describes how to
create a clock management SOC implementation and utilize clock
management within drivers

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Add devicetree clock definitions for the LPC55S6x series. These clock
definitions describe the full clock tree for the SOC, and reuse generic
compatibles where possible. A NXP specific PLL compatible was added
since the PLL has non-generic configuration parameters.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Add code to handle clock setpoints and subsystems on the LPC55xxx series
SOCs. The clock setpoint implementation handles all clocks in the clock
tree, but should optimize to a few function calls for most setpoints, as
should the clock subsystem handler.

Note that the clock subsystem handler currently only implements support
for the Flexcomm clock frequencies, as this will be the initial driver
ported to the clock management framework.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Convert flexcomm driver to use clock management API. This conversion
changes the "clock" properties for the flexcomm nodes, as they no longer
query clock rate using the clock control API.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
Enable clock management for the LPCxpresso55s69 board.

Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
@zephyrbot zephyrbot requested a review from yvanderv March 20, 2024 02:14
@nordicjm nordicjm requested review from erwango and removed request for decsny March 20, 2024 14:35
# Helper macro to set clock management driver
macro(zephyr_clock_mgmt_driver_ifdef toggle driver)
if(${toggle})
set(clock_mgmt_driver ${CMAKE_CURRENT_SOURCE_DIR}/${driver})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit confused by this, it seems it's only possible to have 1 clock mgmt driver?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes- I should have made that more clear in the RFC body. Like pin control, this subsystem is designed to be single instance. The idea is that an soc clock control driver is capable of handling all SOC clocking configurations and clock subsystems

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this work for complex systems involving external clock generators? Or FPGAs with multiple clocking subsystems?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both good questions. For an FPGA with multiple clocking subsystems, how would this differ from an SOC? My understanding is that once a given bitstream is loaded to an FPGA, you could then load a firmware that handled the clock tree implemented within that bitstream.

The external clock generator issue is a bit more complicated, and a good case to motivate multiple clock devices. The core reason I wanted to avoid this (as mentioned above) was that I want to use the phandle references within a node setpoint to refer to clock nodes, not the clock device. I guess a general question would be does this approach make sense? I like the simplicity it lends to most configurations, but it comes with tradeoffs in terms of new tooling (as I hope this PR makes clear)

Since we are already changing the devicetree tooling for this PR, perhaps I could add code to find the "clock controller" device for a given clock node (which should be above the clock node in the device tree hierarchy), and generate a reference to that node?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this work for complex systems involving external clock generators? Or FPGAs with multiple clocking subsystems?

My interpretation of this RFC is that it is an SOC clock management framework. What would need to be addressed regarding external clock sources in this system? Wouldn't the external devices still be configured by some driver?

@erwango erwango requested a review from gmarull March 20, 2024 16:41
@hubertmis
Copy link
Member

hubertmis commented Mar 22, 2024

Do I understand correctly that a "setpoint" introduced in this RFC describes the full configuration of a given clock, meaning its frequency, accuracy, and precision?

If that's the case I don't think the proposed solution is flexible enough. Let's discuss an example:

In an MCU we have peripheral X and peripheral Y, both clocked by the same clock source (CS). CS can be configured in modes: A, B, and C. Modes A and C result in good accuracy, modes B and C in high frequency, while A and B in low power. During one operation, the device driver X requires good accuracy, while the device driver Y requires high frequency.

If I understand this proposal correctly, the device driver X would call clock_mgmt_apply_setpoint(CS, A) and at the same time, driver Y would call clock_mgmt_apply_setpoint(CS, B), both requesting conflicting setpoints from the CS.

I think instead of using the predefined clock setpoints, the device drivers should report their requirements regarding their clock sources: clock_mgmt_set_min_accuracy(CS, 100 /* ppm */);, clock_mgmt_set_min_frequency(CS, 16 /* MHz */). This way if the device driver X requests accuracy, the clock driver would set the clock mode A or C. If the device driver Y requests high frequency, the clock driver could set the clock mode B or C. But when both request at the same time, the only available choice is mode C.

@carlescufi carlescufi requested review from anangl, hubertmis and nika-nordic and removed request for EmilioCBen, 57300 and tejlmand March 22, 2024 09:32
@danieldegrasse
Copy link
Collaborator Author

I think instead of using the predefined clock setpoints, the device drivers should report their requirements regarding their clock sources: clock_mgmt_set_min_accuracy(CS, 100 /* ppm /);, clock_mgmt_set_min_frequency(CS, 16 / MHz */). This way if the device driver X requests accuracy, the clock driver would set the clock mode A or C. If the device driver Y requests high frequency, the clock driver could set the clock mode B or C. But when both request at the same time, the only available choice is mode C.

Hi @hubertmis, thanks for the feedback here- yes, you are understanding the idea behind setpoints correctly. This is a good point- conflicts are easily possible with this solution.

However, one of the core ideas behind setpoints (and this solution as a whole) is to avoid runtime calculation wherever possible. This is why the API doesn't include something like clock_mgmt_set_rate- for higher power devices algorithms to solve clock solutions at runtime are perfectly feasible, but I want to make this solution scale well between low end embedded devices and high power ones.

For a case like you mentioned, it is the responsibility of the user to determine that both devices ought to use clock mode "C", and set that in the devicetree setpoints. Of course this puts some complexity on the user that otherwise could be handled by software, but I expect that tooling can be written to do these types of "clock solver" problems for the user, and generate logical setpoints for their devicetree.

@hubertmis
Copy link
Member

For a case like you mentioned, it is the responsibility of the user to determine that both devices ought to use clock mode "C", and set that in the devicetree setpoints.

In the example I sketched device driver X should use mode "A" whenever possible, device driver Y should use mode "B" whenever possible, and both should use mode "C" only while both are active. Considering the separation of concerns, device driver X should not have visibility in device driver Y activity, so they should never explicitly request mode "C".

I think the API should operate on clock properties, not setpoints. In simple devices, to minimize processing overhead, the implementation hidden below the API could be as simple as

any_accuracy_request(_accuracy_value)
{
  set_setpoint(N);
}

accuracy_released()
{
  set_setpoint(M);
}

While in more complicated devices like the one I sketched the calculations in the clock_mgmt driver would need to be accordingly more complex.

@danieldegrasse
Copy link
Collaborator Author

In the example I sketched device driver X should use mode "A" whenever possible, device driver Y should use mode "B" whenever possible, and both should use mode "C" only while both are active. Considering the separation of concerns, device driver X should not have visibility in device driver Y activity, so they should never explicitly request mode "C".

I agree that drivers should not have visibility into each other's operation/activity. In my view, an application enabling device X and device Y would configure the setpoint for each device to request clock mode C. The base board devicetree would likely setup device X with a setpoint to use mode A, and device Y with a setpoint to use mode B. Then, an application enabling device X and device Y at the same time would provide an overlay to select clock mode C.

I want to emphasize that keeping this framework very simple was an intentional choice- I want to avoid complicated clock algorithms, because I feel that implementations like that will use up excessive flash space on embedded devices. Do you feel the tradeoff there is worth it? If so, I'm happy to look at expanding this framework to support managing clock dependencies with runtime code

@hubertmis
Copy link
Member

Then, an application enabling device X and device Y at the same time would provide an overlay to select clock mode C.

I understand your proposal, but I see a few obstacles we should fix:

  1. All the time device driver X or Y is active, they request clock mode C, even though they are rarely active simultaneously. It results in increased power consumption because the only low power clock modes are A and B. This is unacceptable for battery-powered devices. In a battery-powered device, most of the time mode A or B should be selected, and C only while both device drivers are enabled simultaneously.
  2. Separation of concerns: an application developer should not configure clocks if clocks are required not by the application logic, but by the device drivers. Creating an application would get complicated if enabling a filesystem would require not only setting =y in Kconfig, but also following all dependency trees, down to SDcard, or SPI and GPIO drivers to check if they require clocks, which clocks, and how to configure these clocks in cooperation with a USB and fuel gauge drivers. The application developer, instead of focusing on the application logic, would need to study the middleware, drivers, and hardware details.

Do you feel the tradeoff there is worth it?

I think it depends. We have simple devices without power constraints, with limited processing power, and a simple clock tree. In these devices, we should focus on performance and create low-overhead clock drivers, avoiding unnecessary algorithms occupying memory and CPU cycles. But Zephyr also runs in more complex devices with greater memory size, complicated clock trees, and run-time quality requirements that cannot be hardcoded in the device tree. In these devices, the clock tree must be reconfigured based on a run-time algorithm.

The clock API in Zephyr must be suitable for all types of supported devices to allow portability of the device drivers and applications. The consequence is that it won't be optimal for either, but achieving portability is worth this price. We should find a solution that is as close to optimal as possible for all use cases supported by Zephyr.

@danieldegrasse
Copy link
Collaborator Author

All the time device driver X or Y is active, they request clock mode C, even though they are rarely active simultaneously. It results in increased power consumption because the only low power clock modes are A and B. This is unacceptable for battery-powered devices. In a battery-powered device, most of the time mode A or B should be selected, and C only while both device drivers are enabled simultaneously.

This feels like a case where device power management could be used- IE if device X does into sleep state, it selects a setpoint that uses clock mode B (since device Y is now the only consumer). Device Y could have a sleep setpoint that uses clock mode A, and both devices could select a setpoint with clock mode C when they wake from sleep. This way when one of the devices becomes inactive, the lower power clock mode is selected.

Separation of concerns: an application developer should not configure clocks if clocks are required not by the application logic, but by the device drivers. Creating an application would get complicated if enabling a filesystem would require not only setting =y in Kconfig, but also following all dependency trees, down to SDcard, or SPI and GPIO drivers to check if they require clocks, which clocks, and how to configure these clocks in cooperation with a USB and fuel gauge drivers. The application developer, instead of focusing on the application logic, would need to study the middleware, drivers, and hardware details.

Personally, I think this is totally normal when building a devicetree for a new application- devicetree is intended to describe the hardware, so you need to understand the hardware to write your devicetree. If you want to enable SPI or I2C on your board, you need to determine which pins the hardware will use for SPI/I2C based on your board design. Do you feel that clock setpoints would place a more significant burden than understanding pin control for a given device does?

But Zephyr also runs in more complex devices with greater memory size, complicated clock trees, and run-time quality requirements that cannot be hardcoded in the device tree. In these devices, the clock tree must be reconfigured based on a run-time algorithm.

This is a good point- Zephyr runs on cores capable of running Linux, all the way down to embedded devices under 100MHz. What I have yet to determine is how to create a clock framework that can optimize effectively on smaller devices, while still accommodating larger ones. I guess a few key questions here:

  • For smaller devices, do you think the idea of setpoints makes sense? The core reasoning behind setpoints is that as much of the clock selection logic should be possible to determine without runtime code.
  • Do you think we should define a clock framework with runtime and setpoint component? I worry a framework like this could duplicate functionality, but it would certainly be flexible

@hubertmis
Copy link
Member

This feels like a case where device power management could be used- IE if device X does into sleep state, it selects a setpoint that uses clock mode B (since device Y is now the only consumer). Device Y could have a sleep setpoint that uses clock mode A, and both devices could select a setpoint with clock mode C when they wake from sleep. This way when one of the devices becomes inactive, the lower power clock mode is selected.

Right, but it does not scale for a system in which we have more, e.g. 3 devices clocked from the same source: X and Z requiring high accuracy (modes A or C), while Y requires high frequency (mode B). Also, it does not cover the case when all of the devices are disabled and their clock source can get disabled as well.

Personally, I think this is totally normal when building a devicetree for a new application- devicetree is intended to describe the hardware, so you need to understand the hardware to write your devicetree.

I see creating a device as an activity separated from designing application logic. A device tree should be created by a hardware expert who knows the SoC internals well. The application logic can be designed by another department, or by another company. Or it can be integrated using modules from multiple software vendors. So the device tree creator should not need to know if device drivers X, Y, and Z are going to be used simultaneously or not. This information should not leak to the device tree.

Do you feel that clock setpoints would place a more significant burden than understanding pin control for a given device does?

Yes. If in the SOC I use peripheral X and Y in separate application states, I would need to configure clock setpoints like "sleep = disabled; active = A". If I use X and Y simultaneously I would set "sleep = B; active = C" (which I think is still incorrect). So the device tree would not reflect only the device, but also the potential application states.

For smaller devices, do you think the idea of setpoints makes sense?

This idea is one of the valid solutions for the smaller systems. But I don't see a good way to scale it up for the complex systems. That's why I think we should find another good solution for small systems that scales up well.

  • Do you think we should define a clock framework with runtime and setpoint component?

We need a framework capable of a runtime component. In small systems, in which compile-time setpoints configuration is enough, the framework would need to create zero or minimal overhead for performance and memory consumption.

@danieldegrasse
Copy link
Collaborator Author

Right, but it does not scale for a system in which we have more, e.g. 3 devices clocked from the same source: X and Z requiring high accuracy (modes A or C), while Y requires high frequency (mode B). Also, it does not cover the case when all of the devices are disabled and their clock source can get disabled as well.

This is a good point, thank you for raising it. I think we could solve this with some form of reference counting on each clock- what are your thoughts there? If we stuck with the existing implementation this would be implemented in the SOC clock driver level.

Yes. If in the SOC I use peripheral X and Y in separate application states, I would need to configure clock setpoints like "sleep = disabled; active = A". If I use X and Y simultaneously I would set "sleep = B; active = C" (which I think is still incorrect). So the device tree would not reflect only the device, but also the potential application states.

I see your point here. What would your thoughts be on an system like the following be?

  • We support runtime clock frequency configuration, but guard the feature with a Kconfig. This way drivers or application developers that need this support can enable it, and those that don't can keep it off.
  • Clocks are stored in per node data structures, similar to the way the common clock framework handles them. This will introduce overhead, but I think it is required for runtime clock management. These structures have some common fields (IE reference counters and parent/child pointers) and a device specific data field.
  • Setpoints still exist- however if you attempt to use a setpoint on a "clock output" node, then the setpoint is expected to use a clock frequency. This way, application developers who desire ease of use can setup clocks using a set frequency (which will require runtime clock configuration to be enabled), but developers who want to optimize their application and avoid runtime clock configuration overhead can design setpoints that directly configure clock dividers and multiplexer selections.

The above is only a rough idea, just wanted to get your feedback before I investigated how we could implement it.

@hubertmis
Copy link
Member

hubertmis commented Mar 27, 2024

I think we could solve this with some form of reference counting on each clock- what are your thoughts there?

Yes, we need to count requests for this kind of implementation. However, my point is that one size does not fit all. We should create a flexible API/framework with the possibility to implement the clock drivers differently for different MCUs or families. Some implementations should be as simple as setting the matching setpoint (if there is only one requestor of the given clock in the system), others more complicated ones need to count requests for a given setpoint, and others even more complicated need to switch setpoints runtime depending on multiple requests.

The above is only a rough idea, just wanted to get your feedback before I investigated how we could implement it.

Could you post a sketch of the device tree nodes and the API illustrating the points you described? I want to make sure I understood your proposal before I comment.

@danieldegrasse
Copy link
Collaborator Author

danieldegrasse commented Mar 27, 2024

Could you post a sketch of the device tree nodes and the API illustrating the points you described? I want to make sure I understood your proposal before I comment.

Sure- I've tried to write out the changes in more detail below. The TL;DR of the change is that we essentially do the following:

  • allow users to configure clock frequency on "clock output" nodes when CONFIG_CLOCK_MGMT_SET_RATE=y
  • rework the entire SOC level infrastructure to be based on struct clk data structures, which are a stripped down version of the ones used by Linux.
  • leverage devicetree to initialize the struct clk data structures, which will be done by SOC specific clock drivers.

If we want to implement this, I will likely do so in a separate RFC, as I think the underlying implementation is different enough to justify creating another PR

Driver API

The sole change to the driver API is that the clock management callback handlers now accept a user data pointer as well. This is done to make setup of callbacks within the SOC level clock implementation (described below) simpler. Drivers still use setpoints to configure clocks, and all clock management is abstracted from the driver. (all the driver is aware of is the power state the setpoint targets).

Application devicetree

Application devicetrees will still use setpoints, but clock outputs will now have a single clock cell, which allows a frequency to be specified.

Therefore, an application devicetree may look like the following:

&uart {
	clocks = <&uart_clk>;
	clock-names = "default";
	clock-setpoint-0 = <&uart_clk 20000000>; /* Set clock to 20MHz */
	clock-setpoint-1 = <&uart_clk 10000000>; /* Set clock to 10 MHz */
	clock-setpoint-names = "default", "sleep";
};

Setting frequency like this will require a developer to enable CONFIG_CLOCK_MGMT_SET_RATE=y. All SOC clock drivers must support this feature.

Alternatively, an application looking to optimize for reduced resource utilization could do the following:

&uart {
	clocks = <&uart_clk>;
	clock-names = "default";
	clock-setpoint-0 = <&uart_clk_mux 3>; /* Select input source 3 for the UART clock */
	clock-setpoint-1 = <&uart_clk_mux 2>; /* Select input source 2 for the UART clock */
	clock-setpoint-names = "default", "sleep";
};

The second setting here would configure the "uart_clk_mux" clock node directly, to select a different input source.
This would avoid any runtime clock solver algorithms being required.

SOC devicetree

The SOC devicetree will have the following changes:

  • clock nodes will now use device-specific compatibles
  • the "clock-id" property is no longer present (since the compatible serves the same purpose)
  • more hardware properties will be present on each clock node
  • clock outputs will use a generic compatible, and a generic clock driver.
    this way the interface to set clock frequency will be standardized

For example, a section of the LPC55S69 devicetree might look like so:

fcclksel0: fcclksel0@500002b0 {
	compatible = "nxp,syscon-clock-mux";
	#clock-cells = <1>; /* Cell describes mux selection */
	input-sources = <&mainclkselb &pll0div &fro_12m &frohfdiv &fro_1m
			&mclk_in &rtcosc32ksel &no_clock>;
	reg = <500002b0 0x4>; /* hardware reg where div value is stored */
	mux-mask = <0x7>; /* Mask to apply to register to get div value */

	frgctrl0: frgctrl0@50000320 {
		compatible = "nxp,syscon-clock-frg";
		#clock-cells = <1>; /* Cell desribes FRG baud rate multiplier */
		reg = <50000320 0x4>; /* hardware reg where multiplier value is stored */

		fxcom0_clock: fxcom0-clock {
			compatible = "clock-output";
			#clock-cells = <1>; /* Clock frequency */
		};
	};
};

Internal Clock Management Infrastructure

Rather than using the templated setpoint functions like are currently present in this RFC, the implementation will use a
slimmed down version of the "struct clk" concept in the Linux common clock framework.

The "struct clk" definition will look similar to the following:

struct clk {
	/* Can include anything, usually will have data like the following:
	 * - list of clock parents and children
	 * - hardware registers
	 * - reference count
	 */
	void *clk_data;
	sys_slist_t callbacks; /* Linked list of callbacks to fire on clock reconfiguration */
	/* API functions */
	int (*get_rate)(struct clk* ); /* Gets clock rate */
	int (*configure) (struct clk*, void *); /* Configures clock with device specific data */
#ifdef CONFIG_CLOCK_MGMT_SET_RATE
	int (*round_rate) (struct clk*, uint32_t rate); /* Get nearest rate clock can support */
	int (*set_rate) (struct clk*, uint32_t rate); /* Set clock to given rate */
#endif
};

Note that many common fields in the Linux clock framework are absent- this was intentional. I want to avoid common fields
as much as possible, because any unused common field will result in wasted space within the application. For example,
clock muxes likely need an array of parent pointers, while clock sources need no parent pointer whatsoever.

SOCs will supply clock drivers, which can use devicetree to define clock structures. This will look similar to the
DEVICE_DT_DEFINE infrastructure we have right now. I'd like to avoid reusing the device model infrastructure here
primarily because there will be a lot of clock nodes on most SOCs, and using a custom solution allows us to reduce
overhead per each clock node.

The clock management framework will expose a set of APIs to clock drivers only, which should not be used by
device drivers. These APIs will include the following:

  • clock_get_rate(struct clk *clk): get the rate of a given clock
  • clock_configure(struct clk* clk, void *data): configure a clock with driver specific data.
  • clock_notify_changed(struct clk *clk): notify a clock that its parent's rate has changed
  • clock_register_callback(struct clk *clk, clock_mgmt_callback cb): register a callback for when a clock is reconfigured
  • clock_unregister_callback(struct clk *clk, clock_mgmt_callback cb): remove callback registration
  • clock_fire_callbacks(sys_slist_t *list): Fire clock callbacks for a given clock linked list
    If CONFIG_CLOCK_MGMT_SET_RATE is enabled, the following API functions will be added:
  • clock_round_rate: determine the closest rate actually supported by hardware for a given requested rate
  • clock_set_rate: set a clock rate to given frequency. If clock is disabled, this call should enable it. Calling with a frequency of 0 disables the clock.

The clock management framework will then define structures like the following to enable wrapping internal clock APIs:

struct clock_configuration {
	const struct *clk clock; /* Clock to configure */
	const void *setting; /* Data to pass to clock during configuration */
};

struct clock_setpoint {
	const struct clock_configuration *configs; /* Clock configuration settings */
	uint8_t setting_count; /* Number of settings stored */
};

struct clock_mgmt {
	const struct clk *outputs; /* Array of clock outputs to read rates from */
	uint8_t output_count; /* Count of outputs rates can be read from */
	struct clock_setpoint *setpoints; /* Array of setpoint structures (one per each setpoint) */
	uint8_t setpoint_count; /* Number of setpoints */
};

Initializing these setpoints will require some SOC specific implementation. I think the easiest way to do this will be
to read the compatible string from devicetree, and require the SOC to define a macro to initialize the setpoint setting based
on that. For example, if an application references a clock node like so:

uart0 {
	setpoint-0 = <&uart_mux 3>;
};

And the node is defined in the clock tree like so:

uart_mux: uart_mux {
	compatible = "vnd,uart-mux";
};

Then the SOC clock driver would need to define a macro like the following:

/*
 * @param node_id: node identifier for the node with the setpoint (in this case, uart0)
 * @param pha: prop name setpoint is defined on (in this case, setpoint-0)
 * @param idx: index of the phandle in the property (in this case, 0)
 */
#define CLOCK_MGMT_SETPOINT_vnd_uart_mux(node_id, pha, idx) /* Read data from the specifier cells on the node */

SOC implementation

SOC implementation will include definitions for CLOCK_MGMT_SETPOINT_xxx macros for each clock node compatible, as described above. Beyond this, SOCs will supply clock drivers, which read the clock nodes in the devicetree to initialize a set of struct clk structures. The clock management framework will then reference these (via devicetree phandles) in order to initialize the struct clock_mgmt structure for a given driver.

Such an implementation for a mux might look like the following:

/* clock select implementation */
struct syscon_mux_data {
        volatile uint32_t *clksel_reg;
        uint8_t mask;
        const struct clk *parents[];
        struct clock_mgmt_callback callback;
};

int syscon_mux_get_rate(const struct clk *clk)
{
        struct syscon_mux_data *data= clk->clk_data;

        /* Return parent rate */
        return clock_get_rate(data->parents[*data->clksel_reg & data->mask]);
}

int syscon_mux_configure(const struct clk *clk, void *mux_setting)
{
	struct syscon_mux_data *data= clk->clk_data;
	
	/* Remove existing callback */
	clock_unregister_callback(data->parents[*data->clksel_reg & data->mask], &data->callback);
        /* Set mux */
        *data->clksel_reg = ((uint32_t) mux_setting) & data->mask;
	/* Register callback with new parent */
	clock_register_callback(data->parents[(uint32_t)mux_setting], &data->callback);
	/* Call any callbacks to notify of clock change */
	clock_fire_callbacks(&clk->callbacks);
}

void syscon_mux_callback(struct clock_mgmt_callback *cb, void *user_data)
{
	/* Forward callback to children */
	struct clk *clk = user_data
	
	clock_fire_callbacks(&clk->callbacks);
}

void syscon_mux_init(const struct clk *clk)
{
	/* Setup clock callback */
	clock_mgmt_init_callback(&data->callback, syscon_mux_callback, clk);
	/* Add callback to parent */
	clock_register_callback(data->parents[*data->clksel_reg & data->mask], &data->callback);
}

/* CLK_GET would get a reference to a clock device, like DEVICE_DT_GET */
#define GET_PARENT(node_id, prop, idx) CLK_GET(DT_PHANDLE_BY_IDX(node, prop, idx)

#define SYSCON_MUX_DEFINE(node_id) \
	struct syscon_mux_data mux_##node_id = { \
		.clksel_reg = DT_REG_ADDR(node_id), \
		.mask = DT_PROP(node_id, mask), \
		.parents = {DT_FOREACH_PROP_ELEM_SEP(node_id, input_sources, GET_PARENT, (,))}, \
	};
	\
	/* Defines a struct clk, much like DEVICE_DT_DEFINE */ \
	CLK_DT_DEFINE(node_id, &mux_##node_id);
	
/* Calls given macro for each clock with provided compatible string,
 * provided the clock is referenced (directly or indirectly) from an enabled node in the devicetree
 */
DT_FOREACH_CLK(SYSCON_MUX_DEFINE, nxp_lpc_syscon_mux)

@hubertmis
Copy link
Member

Thank you. The new proposal looks much more scalable, including most of the needs of the complex systems.

The last remaining limitation I see is that the solution focuses mostly on the clock frequency and ignores other clock properties like accuracy or precision. I'm curious if the device tree properties you proposed could be extended to something like

	clock-setpoint-0 = <&uart_clk 20000000 100>; /* Set clock to 20MHz, 100 ppm accuracy or better*/
	clock-setpoint-1 = <&uart_clk 20000000 0>; /* Set clock to 20MHz, any accuracy */
	clock-setpoint-2 = <&uart_clk 0 0>; /* Set clock to any frequency and any accuracy (the clock can be disabled) */
	clock-setpoint-names = "precise", "default", "sleep";

Then the driver could select the "precise" clock for fast baud rates, "default" for slower operations, and "sleep" while inactive.

The accuracy control makes sense in some systems, but in others, it does not. So the list of clock properties described in clock-setpoint-N would depend on the SoC.

Then the functions would differ a little because they would be not only about setting and getting clock rate, and notifications on parent rate changes, but also about other properties that would need to be included preferably in generic function calls.

Could other clock properties than the frequency be included in the framework you propose?

@danieldegrasse
Copy link
Collaborator Author

Thank you. The new proposal looks much more scalable, including most of the needs of the complex systems.

The last remaining limitation I see is that the solution focuses mostly on the clock frequency and ignores other clock properties like accuracy or precision. I'm curious if the device tree properties you proposed could be extended to something like

	clock-setpoint-0 = <&uart_clk 20000000 100>; /* Set clock to 20MHz, 100 ppm accuracy or better*/
	clock-setpoint-1 = <&uart_clk 20000000 0>; /* Set clock to 20MHz, any accuracy */
	clock-setpoint-2 = <&uart_clk 0 0>; /* Set clock to any frequency and any accuracy (the clock can be disabled) */
	clock-setpoint-names = "precise", "default", "sleep";

Then the driver could select the "precise" clock for fast baud rates, "default" for slower operations, and "sleep" while inactive.

The accuracy control makes sense in some systems, but in others, it does not. So the list of clock properties described in clock-setpoint-N would depend on the SoC.

Then the functions would differ a little because they would be not only about setting and getting clock rate, and notifications on parent rate changes, but also about other properties that would need to be included preferably in generic function calls.

Could other clock properties than the frequency be included in the framework you propose?

Yes, I think this would be possible. Essentially, the configure API combined with the SOC CLOCK_MGMT_SETPOINT_xxx macros can be combined to use a given output node's properties however is needed. The clock management framework only knows that it needs to call clock_configure with the struct clk* for a given node and the opaque void* defined by CLOCK_MGMT_SETPOINT_xxx based on the node's compatible. Everything else is under vendor control.

For example, I imagine we will have a generic "clock-output" compatible for most cases, with one "frequency" cell. The "clock-output" driver will look something like the following:

/* clock output implementation */
struct clock_output_data {
        struct clk *parent;
        struct clock_mgmt_callback callback;
};

int clock_output_get_rate(const struct clk *clk)
{
        struct clock_output_data *data= clk->clk_data;

        /* Return parent rate */
        return clock_get_rate(data->parent);
}

int clock_output_configure(const struct clk *clk, void *frequency_int)
{
	struct clock_output_data *data= clk->clk_data;
	
	/*
         * Set the output rate of the parent. This trick is key here- this way, the "clock-output" node looks just like
         * any generic clock node to the clock management subsystem, and can be used with setpoints normally.
         */
	return clock_set_rate(data->parent, (uint32_t)frequency_int);
}

void clock_output_callback(struct clock_mgmt_callback *cb, void *user_data)
{
	/* Forward callback to children */
	struct clk *clk = user_data
	
	clock_fire_callbacks(&clk->callbacks);
}

void clock_output_init(const struct clk *clk)
{
	/* Setup clock callback */
	clock_mgmt_init_callback(&data->callback, clock_output_callback, clk);
	/* Add callback to parent */
	clock_register_callback(data->parent, &data->callback);
}

#define CLOCK_OUTPUT_DEFINE(node_id) \
	struct clock_output_data output_##node_id = { \
		.parent = CLK_GET(DT_PARENT(node_id), \
	};
	\
	/* Defines a struct clk, much like DEVICE_DT_DEFINE */ \
	CLK_DT_DEFINE(node_id, &output_##node_id);
	
/* Calls given macro for each clock with provided compatible string,
 * provided the clock is referenced (directly or indirectly) from an enabled node in the devicetree
 */
DT_FOREACH_CLK(CLOCK_OUTPUT_DEFINE, clock_output)

Then there would be a definition in the clock framework like so:

#define CLOCK_MGMT_SETPOINT_clock_output(node_id, pha, idx) DT_PHA_BY_IDX(node_id, pha, idx, frequency)

This would be the generic driver for clock outputs, but a vendor could easily provide a different compatible for their clock outputs, and a different driver. Such a driver's implementation for the configure function might look like so:

struct clk_req {
	uint32_t ppm; /* Clock accuracy, in parts per million */
	uint32_t rate; /* clock rate request */
};

int vnd_clock_configure(const struct clk *clk, void *clock_req)
{
	struct vnd_clk_data *data = clk->clk_data;
	/* Get the clock accuracy and frequency request */
	struct clk_reg *req = (struct clk_req *)clock_req;
	
	/* Try setting the parent rate to requested value */
	int real_rate = clock_set_rate(data->parent, req->rate);
	
	/* Check accuracy */
	if (abs(real_rate - req->rate) > ((req->rate * req->ppm) / 1000000)) {
		/* Can't hit accuracy */
		return -ENOTSUP;
	}
	return 0;
}

If a vendor needs to pass the accuracy or precision parameter to parent clocks, they could instead call clock_configure on that clock, and provide it with those parameters along with the frequency. Since the parameter passed between clock_configure calls is under vendor control, this should be simple enough to accomplish.

| | | power or sleep modes |
+-------------+-------------------------------------+-------------------------+

Devicetree Representation
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is out of sync with the actual dts changes and bindings you are introducing.

Try to figure out if you're suggesting we go away from clocks properties or not.

@danieldegrasse
Copy link
Collaborator Author

Closing this version in favor of #72102, as that seems like a more logical direction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: UART Universal Asynchronous Receiver-Transmitter platform: NXP Drivers NXP Semiconductors, drivers platform: NXP NXP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants