drivers: clock_control: Introduce revision 3 of CCM driver

[LaurentiuM1234]

This commit introduces a new version of the CCM driver. The purpose of this is to make the CCM driver usable by all NXP SoCs despite the changes in the HAL API.

With this driver, each SoC will have to define its own clock tree and a few common operations such as clock enable/disable, set_rate and get_rate.

Hopefully, in time, this driver will replace clock_control_mcux_ccm.c and clock_control_mcux_ccm_rev2.c.

[danieldegrasse]

Thanks for this contribution- I left a few initial comments after review.

Key questions I have:

• does this driver intend to enable the user to configure the PLL frequencies at runtime?
• if we implement the set_rate functionality, but do not allow reconfiguring the MUX selection or PLL frequencies, will we lock the range of frequencies set_rate supports given the frequency of the PLL and the mux that was configured at boot time?

[LaurentiuM1234]

@danieldegrasse Thanks for the feedback.

• does this driver intend to enable the user to configure the PLL frequencies at runtime?

Please see answer above.

• if we implement the set_rate functionality, but do not allow reconfiguring the MUX selection or PLL frequencies, will we lock the range of frequencies set_rate supports given the frequency of the PLL and the mux that was configured at boot time?

If you don't want to change the MUX and the PLL configuration then yes you're stuck with the range of frequencies resulting from only changing DIV. Whether you want to do this or not should be entirely up to you.

[LaurentiuM1234]

Updates:

• added an example of how this driver could be used on i.MX93.
• changed PR to draft since now there are some HAL dependencies (please see Add support for newly introduced CCM Rev3 on i.MX93 hal_nxp#247 for said changes)

Initially, the main idea was to have the SoC specific CCM Rev3 implementations in the soc hierarchy but since we have the same SoC with different architectures (i.e: xtensa and arm64) this would imply having duplicate files which is not good.

[zephyrbot]

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@0ef57e8	zephyrproject-rtos/hal_nxp#247	zephyrproject-rtos/hal_nxp#247/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[LaurentiuM1234]

FYI: another example of how the CCM Rev3 driver may be used can be seen in #60136

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[LaurentiuM1234]

V2 changes:

• driver was restructured completely. ATM, it offers the following features:

  1. Support for PLL initialization.
  2. Linux-like clock initialization and parent assignment using the asssign-clock* properties which can be scattered through the DTS.
  3. Support for clocks which are already initialized by another instance (e.g: the ROM code for i.MX93)
  4. Support for ungating clocks for which there's no Zephyr native drivers.

• for details regarding the interface layer please see clock_control_mcux_ccm_rev3.h. For SoC layer details and tutorial-like information please see imx93_ccm.c.

• for clock configuration examples please see imx93_ccm.c.

Please let me know if there's any pieces of documentation that are unclear and/or could be improved.

[danieldegrasse]

The deviation from the clocks = <xx> standard property here concerns me. I think we should still be using the clocks property for clock data, unless there is a good reason to deviate from the standard property nameused by other SOCs

[LaurentiuM1234]

The problem with the clocks property is that it doesn't offer enough flexibility. For instance, the assigned-clock-parents property is optional (unless a node needs this property in which case it becomes mandatory). If we were to use the clocks then it would be mandatory to specify a clock's parent as the number of elements in the resulting clocks array would have to be divisible by some value. Also, the drivers don't need to have access to all of the fields (e.g: the clock's frequency or the clock's parent). A handle to their clock is the only thing they need.

Assuming the following example in which the clocks property has the following structure: clocks = <CLOCK_ID CLOCK_PARENT_ID CLOCK_RATE>,...:

ccm: clock-controller@base {
    /* some properties go here */
};

label1: ip1@base1 {
    clocks = <&ccm ROOT_IP1 SOURCE_ROOT_IP1 ROOT_IP1_RATE>, <&ccm CLOCK_IP1 ROOT_IP1 CLOCK_IP1_RATE>;
};

The same thing could be achieved using the assigned-clock* properties as follows:

ccm: clock-controller@base {
    /* some properties go here */
};

label1: ip1@base1 {
    assigned-clocks = <ROOT_IP1>;
    assigned-clock-parents = <SOURCE_ROOT_IP1>;
    assigned-clock-rates = <ROOT_IP1_RATE>;

    clocks = <&ccm CLOCK_IP1>;
};

In the first approach you might end up having a mixture of clocks the driver needs to actually use and clocks that are just used by the CCM driver to perform initialization. IMO, separating them is much cleaner. Also, in the case of SoCs such as i.MX93 configuring the root clock's rate will also configure the rate of the associated IP clock. As such, CLOCK_IP1 would end up being pointlessly initialized.

Of course, example 1 could be modified as follows (not sure this would actually work)

ccm: clock-controller@base {
    /* some properties go here */
   clocks = <&ccm ROOT_IP1 SOURCE_ROOT_IP1 ROOT_IP1_RATE>; /* not sure if this is right */
};

label1: ip1@base1 {
    clocks = <&ccm CLOCK_IP1 ROOT_IP1 CLOCK_IP1_RATE>;
};

This would mean the CCM node would end up containing a bunch of clocks which would reduce the readability of the DTS (which is why I chose to add support for parsing distributed assigned-clock* properties)

Also, some SoCs may not want to assign parents to clocks through the DTS. This would mean we'd end up with a bunch of clocks properties having the following value:

clocks = <CLOCK_ID DUMMY_CLK_ID RATE>...

where DUMMY_CLK_ID is just a value ignored by the SoC layer.

Considering all of this, I think using a combination of clocks and assigned-clock* may be worth it. I'm not so sure about the disadvantages though. Is there a particular case in your mind for which things may become bad if we use both clocks and the assigned-clock* properties?

[danieldegrasse]

It seems like what we are trying to do here is encode a clock tree with these two properties. Rather than having a clock and clock-parent property, why not describe the clock node as a devicetree node, then have the parent of that node be the source clock?

[LaurentiuM1234]

Do you mean something like this?

source1: source-clock {
    root1: root-clock {
    };
};

I'm not sure this is the right way.
First, shouldn't the device tree nodes be actual physical devices? (or at least try to be). I'm not sure these clocks can be considered physical devices?
Second, we have lots of root clocks. Because of this, specifying the clock hierarchy in the DTS would lead to some pretty nasty device trees.
Third, parsing the DTS is already somewhat difficult since it requires macro logic (see IMX_CCM_FOREACH_ASSIGNED_CLOCK and friends). I'm assuming this case may be harder to handle.

I'm not sure setting the root clock sources through the DTS is the way to go. Parsing the DTS is already somewhat tricky due to all of the macro magic (e.g: IMX_CCM_FOREACH_ASSIGNED_CLOCK and friends). Also, having all of those clock nodes would make the device trees messier as there's plenty of root clocks.

Regarding the assigned-clock* properties: I wouldn't necessarily say their main goal is to describe the entire clock tree. In theory, you could achieve that and many other things using these properties thanks to their flexibility, which is why I kinda like them.

[dbaluta]

It seems like what we are trying to do here is encode a clock tree with these two properties. Rather than having a clock and clock-parent property, why not describe the clock node as a devicetree node, then have the parent of that node be the source clock?

I think we are trying to follow here the Linux implementation which is proved to work for a while.

[danieldegrasse]

Ok, I've taken a look at the Linux implementation, and I think I understand the goals here a bit better now. With that being said, Linux clearly has different clock drivers for each SOC (since the clock driver itself encodes the clock tree data, and as far as I can see Linux provides a generic framework to handle a lot of common functionalities with clocks: https://docs.kernel.org/driver-api/clk.html.)

Given that, wouldn't we want this PR to have two components:

• a common clock framework of some form (that we can initially use with NXP clock drivers, then attempt to expand to be a generic functionality)
• an SOC specific driver for the iMX 93 part

[danieldegrasse]

Additionally- just a general question about Linux clock setup- how does Linux handle a case where the initial clock frequency of the SOC should be produced via a PLL (something like 24MHz OSC -> PLL0 -> output clock)? Is the idea that the clock framework would call set_rate for the PLL, and then the PLL divider and multiplier would be calculated at runtime?

My concern with this approach in general is that for larger SOCs like the iMX RT series (or iMX itself), this approach makes sense for the ease of use. However, on smaller cores like the LPC parts I am concerned about the overhead of performing these clock calculations on boot

[LaurentiuM1234]

Ok, I've taken a look at the Linux implementation, and I think I understand the goals here a bit better now. With that being said, Linux clearly has different clock drivers for each SOC (since the clock driver itself encodes the clock tree data, and as far as I can see Linux provides a generic framework to handle a lot of common functionalities with clocks: https://docs.kernel.org/driver-api/clk.html.)

Given that, wouldn't we want this PR to have two components:

• a common clock framework of some form (that we can initially use with NXP clock drivers, then attempt to expand to be a generic functionality)
• an SOC specific driver for the iMX 93 part

It does. clock_control_mcux_ccm_rev3.c is supposed to contain the SoC-independent operations such as parsing the clock tree to set a clock's rate, managing the software states of the clocks (GATED, UNGATED) etc....(basically the common clock framework). The SoC specific driver is in imx93_ccm.c which implements the SoC-specific operations such as setting a clock's rate, setting a clock's parent etc...

Additionally- just a general question about Linux clock setup- how does Linux handle a case where the initial clock frequency of the SOC should be produced via a PLL (something like 24MHz OSC -> PLL0 -> output clock)? Is the idea that the clock framework would call set_rate for the PLL, and then the PLL divider and multiplier would be calculated at runtime?

Not sure how helpful I can be here since I'm not entirely familiar with how Linux initializes the clocks (also this approach isn't meant to be a 1:1 recreation of the Linux CCF, but rather a very simplified version that would still be powerful enough to serve our purposes). Not sure if this would help but the way I see things there's 3 possible approaches:

1. During initialization every clock is set to a default configuration which can then be altered later on using the assigned-clock* properties.
2. During initialization only clocks configured through assigned-clock* properties are made usable (also the clocks which would be configured through propagation). This would leave unconfigured clocks basically unusable so the driver should raise an error when attempting to use a clock which depends on one of these clocks.
3. The SoC layer itself knows the default configuration of the clocks so it just informs the upper layer such that no initialization is required.

I tend to think option 1 (or even 3 maybe?) would be more plausible in Linux's case. As a side note, my current approach is based on number 2. Ideally, it should be based on 1 or 3.

My concern with this approach in general is that for larger SOCs like the iMX RT series (or iMX itself), this approach makes sense for the ease of use. However, on smaller cores like the LPC parts I am concerned about the overhead of performing these clock calculations on boot

Yeah this is indeed concerning. We could optimize the driver such that it initializes only clocks used by consumers, instead of initializing all of the clocks declared in the SoC layer's clock tree. Other than that, you could reduce the overhead by using a toned-down version of the clock tree and reducing the amount of configurations that need to be done through assigned-clock* properties.

[danieldegrasse]

It does. clock_control_mcux_ccm_rev3.c is supposed to contain the SoC-independent operations such as parsing the clock tree to set a clock's rate, managing the software states of the clocks (GATED, UNGATED) etc....(basically the common clock framework). The SoC specific driver is in imx93_ccm.c which implements the SoC-specific operations such as setting a clock's rate, setting a clock's parent etc...

This makes sense- thank you for the clarification

1. During initialization every clock is set to a default configuration which can then be altered later on using the assigned-clock* properties.

2. During initialization only clocks configured through assigned-clock* properties are made usable (also the clocks which would be configured through propagation). This would leave unconfigured clocks basically unusable so the driver should raise an error when attempting to use a clock which depends on one of these clocks.
3. The SoC layer itself knows the default configuration of the clocks so it just informs the upper layer such that no initialization is required.

Of these, I actually prefer 2), since that would result in the lowest runtime overhead for the platform, and the least amount of definitions needed in code.

The key thing I am worried about here is a need to implement set_rate on all clock devices. For example, if we define the clock tree itself in the DTS, we could end up with something like this for a PLL:

osc_24m {
	clock-frequency = <24000000>;
};

rtc_32k {
	clock-frequency = <32768>;
};


pll_clk_sel {
	input-sources = <&osc_24m &rtc_32k>;
	pll0: pll0 {
		mult = <25>;
		div = <1>;
	};
};

with something like this, the SOC initialization code could easily determine the PLL settings needed. If we used set_rate, then the SOC code would need to calculate the PLL multiplier and divider at runtime. The other issue I am concerned about is code size. With a devicetree, the C code will come down to a function call (and could be as simple as a few register writes on some platforms). With set_rate implementations, the code will need to include logic to perform these calculations at runtime

[danieldegrasse]

Why return an error here? The clock is at the rate we need, so I would argue the function can just return 0 here, since the clock is configured how the user needs it to be

[LaurentiuM1234]

According to https://docs.zephyrproject.org/latest/hardware/peripherals/clock_control.html, set_rate() should return -EALREADY if clock was already configured to requested frequency. It would make more sense to just return 0, but I guess we need to be consistent with the clock controller API.

[danieldegrasse]

I'm unclear why we go to the clock parent to set the rate- will this cause dependency problems if the parent clock has other child clock dependencies?

[LaurentiuM1234]

It depends on the situation I guess. The SoC layer should prevent set_rate() from trying to configure clocks (i.e: go up in the tree structure) which are depended upon by returning -EPERM via the imx_ccm_get_parent_rate() call. Of course, if the tree structure gets more complex things may get out of hand (e.g: a lot of complexity added to the SoC layer which is against the driver's philosophy)

The main reason I chose this design was because clocks with no dependencies (e.g: some root clocks) can be configured during runtime so they don't require configuration through the DTS.

[iuliana-prodan]

"generally clocks from higher levels are depended upon by multiple clocks from higer levels." - can you rephrase, please. This is confusing

[LaurentiuM1234]

Thanks for pointing this out! It was mean to say: generally clocks from higher levels are depended upon by multiple clocks from lower levels. Will fix in the next iteration.

[dbaluta]

@mfischer Hi, Care to have a look? I remeber you had a talk about clocks at ZDS.

[danieldegrasse]

I think rather than sprinkling comments throughout the PR, it would make more sense to highlight the specific concerns I have with this approach to making clocking more generic

If we are going to implement clocking using runtime calculations like this, I think we should probably implement more of the ideas present in the common clock framework. In particular, I think we should probably implement support for things like notifications when clock rates change- without features like this, I don't think we can safely change most clock parent assignments. That will prevent utilization of a lot of low power functionality on SOCs when using this framework.

However, my primary concern is that by implementing more of the common clock framework we will end up with a rather large subsystem that isn't usable by smaller SOCs, which may need to reconfigure clock sources for features like DVFS.

The core use cases I am aware of for expanding the clock framework are the following:

1. support better power management (features like DVFS)
2. enable dynamically setting clock rates for peripherals
   Please feel free to highlight other use cases here, if you are aware of them

From my point of view, we can support 1) relatively well with the following rule: if a peripheral is the only consumer of its parent divider/mux, we can freely change that mux/divider setting. Otherwise, we don't permit changing the frequency via the clock_control API.

Use case 2) is harder to support, but I think the idea of discrete set points (where each set point corresponds to a clock tree configuration) makes sense. I've opened a draft PR on the LPC55S69 (#62946) to demonstrate how clocks could be configured using a devicetree based clock tree. We could logically extend this to support multiple clock tree "set points" which reconfigure the clock tree to enable better power savings at reduced frequencies.

[danieldegrasse]

You could use CODE_UNREACHABLE here

[danieldegrasse]

Here when you say "relatives", does this mean that children and parents will be ungated? That is, if I ungate a root clock source will all children also be enabled?

[LaurentiuM1234]

The wording is just so bad, will have to redo the documentation. The term relatives is used to refer to the clock's parent, grand-parent etc.... For example, assuming the following tree:

A ---> B ---> C

where PARENT(C) = B, PARENT(B) = A. Trying to do a UNGATE(C) will result in also ungating B and A.

[danieldegrasse]

This documentation is slightly confusing to me. If we have a clock tree like the following:

LPUART1->LPUART1_DIV->PLL0->OSC

when calling set_rate(LPUART1, MHZ(24)) would we then call set_rate(LPUART1_DIV, MHZ(24)), and then set_rate(PLL0, MHZ(24)? Or is the intention here that PLL0 would recognize it has multiple children consuming its clock, and return -EPERM?

[LaurentiuM1234]

The rate setting starts from the most distant parent which in your example would theoretically be OSC. Also, you don't propagate the child's rate, but rather the parent's theoretical rate (i.e: the rate required for the parent such that the child has the requested rate).

As things are now, according to the SOC code, the set_rate operation would stop at your divider (a.k.a the root clock) (that is if you start the operation from the IP clock).

[danieldegrasse]

Why have a NULL parent? Can't traversal stop at the clock that is marked as a root source?

[LaurentiuM1234]

The clock that has a NULL parent is a root clock. For example:

A ---> B ---> C

where PARENT(C) = B and PARENT(B) = A, A is the root clock and has PARENT(A) = NULL. There's no point in encoding this information through another variable.

[LaurentiuM1234]

The core use cases I am aware of for expanding the clock framework are the following:

1. support better power management (features like DVFS)
2. enable dynamically setting clock rates for peripherals
   Please feel free to highlight other use cases here, if you are aware of them

Mm, as far as I can tell, dynamically changing clock rates is a nice to have feature but the cases in which you'd actually need to do this may not be many, so I wouldn't say this is the main requirement for the clock framework.

From my POV, the clock framework should aim to do the following things:

1. Try to reduce duplicate code as much as possible.
2. Try to make the lives of the developers as easy as possible by implementing most of the complexity in the upper layers and leaving only the basic SoC-specific operations to the SoC layer.
3. Try to make the lives of the users as easy as possible. They shouldn't have to care about the entire clock hierarchy. All they need to care about is configuring the clocks used by their IPs (if the default configuration is not ok)
4. Try to be as flexible as possible to allow multiple use-cases. For example, in one case you might need FREQ(IP_CLOCK_X) = Y, while in other cases you may need FREQ(IP_CLOCK_X) = y

IMO, the current implementation is pretty mediocre when talking about points 2 and 3 (possibly even 1) so until I address this the patch series is not in a mergeable state.

I've opened a draft PR on the LPC55S69 (#62946) to demonstrate how clocks could be configured using a devicetree based clock tree. We could logically extend this to support multiple clock tree "set points" which reconfigure the clock tree to enable better power savings at reduced frequencies.

Thanks for sharing this with me! I'm not sure I fully understand the idea of set points. Do you mean something like a clock tree snapshot or configuration? For example:

A ---> B ---> C

would be the first configuration, while

D ---> B ---> C

would be the second configuration supported? (assuming B can have A and D as parents). Would you have a clock_init() function for each of these configurations? At a first glance, the problem I see with this approach is that we might end up with a lot of files like lpc55Sxx_clocks.c (1 for each SoC or maybe clock tree?), which seems like a pain to write it yourself (unless you somehow manage to do some macro magic which in itself is a pain to do) (maybe generate it?). Also, for example, the i.MX93 CCM has a bunch of clocks, won't that make the clock_init() function basically unreadable if we were to encode the whole tree in the DTS?

[mfischer]

@mfischer Hi, Care to have a look? I remeber you had a talk about clocks at ZDS.

I'll hopefully get around to it. $dayjob is crazy at the moment and I was out of office the last two weeks. Appreciate the tag.

[danieldegrasse]

Thanks for sharing this with me! I'm not sure I fully understand the idea of set points. Do you mean something like a clock tree snapshot or configuration? For example:

Yes, exactly- each "set point" would be a snapshot of clock settings. For example, set point 1 might configure the core system clock to use PLL0, and set point 2 might migrate the system clock to a low frequency internal oscillator (for low power consumption)

Would you have a clock_init() function for each of these configurations? At a first glance, the problem I see with this approach is that we might end up with a lot of files like lpc55Sxx_clocks.c (1 for each SoC or maybe clock tree?)

Yes, we would absolutely have a file like lpc55Sxx_clocks.c for most SoCs. We would not need one for each clock tree, the idea would be that we can initialize some form of struct describing the clock state for a given clock tree (each struct inst would be considered a "set point"), and then switch between the "set points" at runtime.

which seems like a pain to write it yourself (unless you somehow manage to do some macro magic which in itself is a pain to do) (maybe generate it?)

This is a concern I have as well. The clock tree itself is automatically generated, and I may look at how to autogenerate the clock initialization code as well (since it really is just configuring dividers and muxes for the most part, the only bits that need much hand editing are the PLL setup). The benefit of this approach is that after optimization, the clock_init function can come down to 2-3 function calls to setup the core clock source and switch the core clock mux to that option.

Truthfully, I think your approach is a lot more developer-friendly. Looking at the CCF in Linux, it seems like code reuse is very high (IE you can reuse the clock driver for a PLL if the IP is reused). Part of the issue I expect us to run up against with code reuse here is that our fsl_clock.h APIs have a nasty habit of changing between SOC families, even if the underlying clock structure is similar enough

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[danieldegrasse]

At a first glance, the problem I see with this approach is that we might end up with a lot of files like lpc55Sxx_clocks.c (1 for each SoC or maybe clock tree?), which seems like a pain to write it yourself (unless you somehow manage to do some macro magic which in itself is a pain to do) (maybe generate it?). Also, for example, the i.MX93 CCM has a bunch of clocks, won't that make the clock_init() function basically unreadable if we were to encode the whole tree in the DTS?

Yes, you definitely will- however, I'm not sure how we avoid that in either case. With something like this design, we still need some type of clock tree definition in C data structures, right? The reason I took the setpoint approach is to optimize flash usage as much as possible. One way we can do this is to avoid pulling in any clock configuration code from the fsl_clock.c driver that we don't need in order to apply clock settings requested by the user

I've opened another PR in pursuit of this, #66732. This no longer uses the "hierarchical" clock structure in devicetree, but instead uses a compatible file to define all possible clock setpoint attributes. This file is being generated from the MCUXpresso configuration tools clock data (as was the devicetree clock structure). This PR demonstrates how setpoints could be supported. Each setpoint is implemented by a function, which is defined from an (admittedly very ugly) macro: https://github.com/zephyrproject-rtos/zephyr/pull/66732/files#diff-3e9e59adc512d595d49ada8ed3c5e92a0d6702a5cd0e924783ae1e483b0ba5cbR211. The advantage of this approach is flash savings: one clock function configuring a set point takes around 200 bytes of flash, and there is no need to link in clock configuration functions that won't be used (which we would have to do if we parsed the clock configuration structures at runtime)

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.