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.

Sign up for GitHub

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

Jump to bottom

drivers: clock_control: Introduce revision 3 of CCM driver #59787

Closed
wants to merge 4 commits into from
Closed

drivers: clock_control: Introduce revision 3 of CCM driver #59787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0424aa
modules: Kconfig.imx: Add configuration for disabling implicit clocking
LaurentiuM1234 Sep 11, 2023
8bca890
drivers: clock_control: Add revision 3 of CCM driver
LaurentiuM1234 Sep 11, 2023
cf5cd6a
nxp: mimx93: Switch to using CCM Rev3
LaurentiuM1234 Sep 11, 2023
018fe79
manifest: Update hal_nxp
LaurentiuM1234 Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions boards/arm64/mimx93_evk/mimx93_evk_a55.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,7 @@
/* clocks = <&ccm IMX_CCM_UART4_CLK 0x6c 24>; */
pinctrl-0 = <&uart2_default>;
pinctrl-names = "default";
assigned-clocks = <IMX93_CCM_LPUART2_ROOT>;
assigned-clock-parents = <IMX93_CCM_OSC_24M>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@LaurentiuM1234 LaurentiuM1234 Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

assigned-clock-rates = <24000000>;
};
3 changes: 3 additions & 0 deletions boards/arm64/mimx93_evk/mimx93_evk_a55_sof.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,7 @@
/* clocks = <&ccm IMX_CCM_UART4_CLK 0x6c 24>; */
pinctrl-0 = <&uart2_default>;
pinctrl-names = "default";
assigned-clocks = <IMX93_CCM_LPUART2_ROOT>;
assigned-clock-parents = <IMX93_CCM_OSC_24M>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@LaurentiuM1234 LaurentiuM1234 Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.
  1. 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.
  2. 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

assigned-clock-rates = <24000000>;
};
3 changes: 3 additions & 0 deletions drivers/clock_control/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_LPC11U6X clock_cont
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCHP_XEC clock_control_mchp_xec.c)
LaurentiuM1234 marked this conversation as resolved.
Show resolved Hide resolved
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_CCM clock_control_mcux_ccm.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_CCM_REV2 clock_control_mcux_ccm_rev2.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_CCM_REV3 clock_control_mcux_ccm_rev3.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_MCG clock_control_mcux_mcg.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_PCC clock_control_mcux_pcc.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_SCG clock_control_mcux_scg.c)
Expand Down Expand Up @@ -71,3 +72,5 @@ if(CONFIG_CLOCK_CONTROL_RCAR_CPG_MSSR)
endif()

zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_AST10X0 clock_control_ast10x0.c)

add_subdirectory_ifdef(CONFIG_CLOCK_CONTROL_MCUX_CCM_REV3 imx)
2 changes: 2 additions & 0 deletions drivers/clock_control/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,6 @@ source "drivers/clock_control/Kconfig.nxp_s32"

source "drivers/clock_control/Kconfig.agilex5"

source "drivers/clock_control/Kconfig.mcux_ccm_rev3"

endif # CLOCK_CONTROL
10 changes: 10 additions & 0 deletions drivers/clock_control/Kconfig.mcux_ccm_rev3
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Copyright 2023 NXP
# SPDX-License-Identifier: Apache-2.0

config CLOCK_CONTROL_MCUX_CCM_REV3
bool "MCUX CCM Rev3 driver"
default y
select NXP_HAL_DISABLE_IMPLICIT_CLOCKING
depends on DT_HAS_NXP_IMX_CCM_REV3_ENABLED
help
Enable support for mcux ccm rev3 driver.
Loading
Loading