Move STM32 MCO configuration from Kconfig to dts

[benjaminbjornsson]

This PR is intended to resolve the STM32 MCO clock issue discussed here #31912, #58443.

A couple of things come to mind while working on this so I decided to make this a draft before continuing:

• Do we want to have pinctrl applied during boot, maybe a Kconfig to switch the SYS_INIT part on/off?
• As @GeorgeCGV points out here, the reference manual states that the MCO configuration should happen prior clock configurations, so the init priority should maybe be one less that of the rcc driver? But this should cause a compile time error if the rcc node is referenced from the mco node with lower priority?
• Also pointed out by @GeorgeCGV here is the question about power management.
• Also not sure about the mco nodes inside the clocks node?

[zephyrbot]

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

Name	Old Revision	New Revision	Diff
hal_stm32	zephyrproject-rtos/hal_stm32@60c9634 (main)	zephyrproject-rtos/hal_stm32#195	zephyrproject-rtos/hal_stm32#195/files

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

[erwango]

Great thanks @benjaminbjornsson for working on this. We'll try to provide feedback soon on the points raised.

[erwango]

Do we want to have pinctrl applied during boot, maybe a Kconfig to switch the SYS_INIT part on/off?

As a first shot, I'd propose apply pinctrl during boot and make it evolve later if required.

As @GeorgeCGV points out #31912 (comment), the reference manual states that the MCO configuration should happen prior clock configurations, so the init priority should maybe be one less that of the rcc driver? But this should cause a compile time error if the rcc node is referenced from the mco node with lower priority?

One possible way to workaround this could be to use this specific table

zephyr/scripts/build/check_init_priorities.py

Lines 49 to 53 in 4a72cb1

	_IGNORE_COMPATIBLES = frozenset([
	# There is no direct dependency between the CDC ACM UART and the USB
	# device controller, the logical connection is established after USB
	# device support is enabled.
	"zephyr,cdc-acm-uart",

Also pointed out by @GeorgeCGV #31912 (comment) is the question about power management.

Same here, I propose to start w/o PM.

Also not sure about the mco nodes inside the clocks node?

See my comments on this part.

[erwango]

Note on the bindings:

	clocks = <&rcc 0 MCO_DIV(3)>,
		 <&rcc 0 MCO_SEL(4)>;

If you're not actually using rcc as clock controller then you don't need to specify it, and you don't need to use same cells neither, so it can just be:

	clocks = <MCO_DIV(3)>,
		 <MCO_SEL(4)>;

Or even :

	clocks = <MCO_DIV(3) MCO_SEL(4)>;

or whatever finally. This would help to get rid of the initialization priority issue.

[erwango]

Thinking about it more, it would be interesting to reusing the existing format and specifying the clock source:

	clocks = <&rcc STM32_SRC_PLL_Q MCO_SEL(4)>,

This way, it is clearer for the user to know what the exact clock source is (or what value has been set 2 years ago w/o opening the RM). It would also prevent to set clocks source that don't exist.
Then for prescaler, 2 options:

• Use a dedicated property:

        st,mco-prescaler = <MCO_DIV(3)>;

• Reuse existing rcc phandle array with a specificier:

       <&rcc PRESCALER MCO_DIV(3)>;

[erwango]

Altenatively, this could remain a routine of the clock_control diver (called before clock configuretion) and as such, reuse stm32_clock_control_on internal calls.

[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.

[mrrosen]

@erwango @benjaminbjornsson I recently needed this functionality for a projects I was working on for the STM32G4 family (since that family doesnt have support in Zephyr for the MCO right now and wanted to do all the configuration (clock source, prescaler, pinmux) from devicetree only to find it wasnt there and that only this PR and a few other issues were addressing it. However, I think it might be nicer to take a slightly different approach wrt binding and devicetree structure which I was able to implement for the STM32G4 that should work for most STM32 (from what my quick skimming through a few reference manuals suggestions):

First, very similar to this PR, I do think the MCO should sit alone in the devicetree clocks section:
stm32<family>.dtsi

clk_mco: clk-mco {
	#clock-cells = <0>;
	compatible = "st,stm32-mco-clock", "st,stm32g4-mco-clock";
	status = "disabled";
};

Then, each board can set up its MCO has it sees fit using a style similar to the STM32 PLL (and can setup dependencies using linking it as the clock source):
<board>.dts

&clk_mco: {
	pinctrl-0 = <&rcc_mco_pa8>;
	pinctrl-names = "default";
	prescaler = <1>; // This could be st,prescaler but the PLL settings dont prefix with "st," so its a bit mixed here...
	clocks = <&clk_hse>;
	status = "okay";
};
...
device-clocked-by-mco {
	clocks = <&clk_mco>;
};

As each MCO has its own set of prescalers depending on the family, the addition of the family-specific compatible is needed, the bindings for both looks like this:
st,stm32-mco-clock.yaml

description: STM32 MCO Clock

include: [pinctrl-device.yaml, clock-controller.yaml, base.yaml]

properties:
  pinctrl-0:
    required: true

  pinctrl-names:
    required: true

st,stm32g4-mco-clock.yaml

description: STM32G4 MCO Clock

compatible: "st,stm32g4-mco-clock"

include: st,stm32-mco-clock.yaml

properties:
  prescaler:
    type: int
    required: true
    enum:
      - 1
      - 2
      - 4
      - 8
      - 16

To handle the setting up of the pin control, I think its best to split as was done in the PR since setting up the MCO clock source and prescaler are supposed to be done along with all the other clocks (I think some families recommend doing it before the PLL even but thats not currently happening, it could by just moving the call to stm32_clock_control_mco_init() earlier). So, I setup a separate initialization just like the PR for the most part for the pin (but I dont do the setup of the clock in a separate device from the stm32 clock control, only the pin control is deferred to let the GPIO device come up in between:
clock_stm32_ll_mco.c

// This is why we still need the "base" compatible instead of just the family one, to make this much smoother
#define DT_DRV_COMPAT st_stm32_mco_clock

#include <zephyr/drivers/pinctrl.h>

/* The main RCC driver will initialize and configure the MCO clock, but the MCO clock pin
 * needs to be initialized separately. This function initializes the MCO clock pin.
 */
DT_INST_FOREACH_STATUS_OKAY(PINCTRL_DT_INST_DEFINE);

static int st_stm32_mco_clock_pin_init(void)
{
#define PINCTRL_APPLY_STATE(index)                                      \
    {                                                                   \
        const struct pinctrl_dev_config *pcfg = PINCTRL_DT_INST_DEV_CONFIG_GET(index); \
        int ret = pinctrl_apply_state(pcfg, PINCTRL_STATE_DEFAULT);     \
        if (ret < 0) {                                                  \
            return ret;                                                 \
        }                                                               \
    }

    DT_INST_FOREACH_STATUS_OKAY(PINCTRL_APPLY_STATE);

    return 0;
}

SYS_INIT(st_stm32_mco_clock_pin_init, POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT);

After that, its just needing to remove all the code handling the configuration from Kconfig and replacing it with code to take from the device tree instead. And restrictions on the source can be added to the code here if needed (not all familes support all sources so if we want to warn users about an invalid configuration, we would have to do what the PLLQ clock source does for all families). There are a few other hiccups here though that Ill mention below:
stm32_clock_control.h

// Just like the other sources do it:
/** MCO/MCO1 clock source */
#if DT_NODE_HAS_STATUS(DT_NODELABEL(clk_mco), okay) && \
    DT_NODE_HAS_PROP(DT_NODELABEL(clk_mco), clocks)
#define DT_MCO_CLOCKS_CTRL     DT_CLOCKS_CTLR(DT_NODELABEL(clk_mco))
#if DT_SAME_NODE(DT_MCO_CLOCKS_CTRL, DT_NODELABEL(clk_lse))
#define STM32_MCO_SRC_LSE      1
#endif
#if DT_SAME_NODE(DT_MCO_CLOCKS_CTRL, DT_NODELABEL(clk_hse))
#define STM32_MCO_SRC_HSE      1
#endif
#if DT_SAME_NODE(DT_MCO_CLOCKS_CTRL, DT_NODELABEL(clk_lsi))
#define STM32_MCO_SRC_LSI      1
#endif
#if DT_SAME_NODE(DT_MCO_CLOCKS_CTRL, DT_NODELABEL(clk_msi))
#define STM32_MCO_SRC_MSI      1
#endif
#if DT_SAME_NODE(DT_MCO_CLOCKS_CTRL, DT_NODELABEL(clk_hsi))
#define STM32_MCO_SRC_HSI      1
#endif
#if DT_SAME_NODE(DT_MCO_CLOCKS_CTRL, DT_NODELABEL(clk_hsi48))
#define STM32_MCO_SRC_HSI48    1
#endif
#if DT_SAME_NODE(DT_MCO_CLOCKS_CTRL, DT_NODELABEL(clk_pll))
#define STM32_MCO_SRC_PLL      1
#endif
#if DT_SAME_NODE(DT_MCO_CLOCKS_CTRL, DT_NODELABEL(rcc))
#define STM32_MCO_SRC_SYSCLK   1
#endif

#endif

clock_stm32_ll_mco.h (changes)

@@ -10,66 +11,64 @@
 
#include <stm32_ll_utils.h>

-#if CONFIG_CLOCK_STM32_MCO1_SRC_NOCLOCK
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_NOCLOCK
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_EXT_HSE
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_EXT_HSE
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_LSE
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_LSE
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_HSE
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_HSE
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_LSI
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_LSI
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_MSI
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_MSI
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_HSI
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_HSI
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_HSI16
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_HSI
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_HSI48
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_HSI48
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_PLLCLK
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_PLLCLK
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_PLLQCLK
+#if STM32_MCO_SRC_EXT_HSE
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_EXT_HSE
+#elif STM32_MCO_SRC_LSE
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_LSE
+#elif STM32_MCO_SRC_HSE
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_HSE
+#elif STM32_MCO_SRC_LSI
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_LSI
+#elif STM32_MCO_SRC_MSI
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_MSI
+#elif STM32_MCO_SRC_HSI
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_HSI
+#elif STM32_MCO_SRC_HSI16
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_HSI
+#elif STM32_MCO_SRC_HSI48
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_HSI48
+#elif STM32_MCO_SRC_PLLCLK
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_PLLCLK
+#elif STM32_MCO_SRC_PLLQCLK
        #if (CONFIG_SOC_SERIES_STM32G0X || CONFIG_SOC_SERIES_STM32WLX)
-               #define MCO1_SOURCE     LL_RCC_MCO1SOURCE_PLLQCLK
+               #define MCO_SOURCE      LL_RCC_MCO1SOURCE_PLLQCLK
        #elif (CONFIG_SOC_SERIES_STM32H5X || CONFIG_SOC_SERIES_STM32H7X)
-               #define MCO1_SOURCE     LL_RCC_MCO1SOURCE_PLL1QCLK
+               #define MCO_SOURCE      LL_RCC_MCO1SOURCE_PLL1QCLK
        #else
                #error "PLLQCLK is not a valid clock source on your SOC"
        #endif
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_PLLCLK_DIV2
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_PLLCLK_DIV_2
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_PLL2CLK
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_PLL2CLK
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_PLLI2SCLK
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_PLLI2SCLK
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_PLLI2SCLK_DIV2
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_PLLI2SCLK_DIV2
-#elif CONFIG_CLOCK_STM32_MCO1_SRC_SYSCLK
-       #define MCO1_SOURCE             LL_RCC_MCO1SOURCE_SYSCLK
+#elif STM32_MCO_SRC_PLLCLK_DIV2
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_PLLCLK_DIV_2
+#elif STM32_MCO_SRC_PLL2CLK
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_PLL2CLK
+#elif STM32_MCO_SRC_PLLI2SCLK
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_PLLI2SCLK
+#elif STM32_MCO_SRC_PLLI2SCLK_DIV2
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_PLLI2SCLK_DIV2
+#elif STM32_MCO_SRC_SYSCLK
+       #define MCO_SOURCE              LL_RCC_MCO1SOURCE_SYSCLK
#endif
 
-#if CONFIG_CLOCK_STM32_MCO2_SRC_SYSCLK
+#if STM32_MCO2_SRC_SYSCLK
        #define MCO2_SOURCE             LL_RCC_MCO2SOURCE_SYSCLK
-#elif CONFIG_CLOCK_STM32_MCO2_SRC_PLLI2S
+#elif STM32_MCO2_SRC_PLLI2S
        #define MCO2_SOURCE             LL_RCC_MCO2SOURCE_PLLI2S
-#elif CONFIG_CLOCK_STM32_MCO2_SRC_HSE
+#elif STM32_MCO2_SRC_HSE
        #define MCO2_SOURCE             LL_RCC_MCO2SOURCE_HSE
-#elif CONFIG_CLOCK_STM32_MCO2_SRC_LSI
+#elif STM32_MCO2_SRC_LSI
        #define MCO2_SOURCE             LL_RCC_MCO2SOURCE_LSI
-#elif CONFIG_CLOCK_STM32_MCO2_SRC_CSI
+#elif STM32_MCO2_SRC_CSI
        #define MCO2_SOURCE             LL_RCC_MCO2SOURCE_CSI
-#elif CONFIG_CLOCK_STM32_MCO2_SRC_PLLCLK
+#elif STM32_MCO2_SRC_PLLCLK
        #define MCO2_SOURCE             LL_RCC_MCO2SOURCE_PLLCLK
-#elif CONFIG_CLOCK_STM32_MCO2_SRC_PLLPCLK
+#elif STM32_MCO2_SRC_PLLPCLK
        #define MCO2_SOURCE             LL_RCC_MCO2SOURCE_PLL1PCLK
-#elif CONFIG_CLOCK_STM32_MCO2_SRC_PLL2PCLK

+#elif STM32_MCO2_SRC_PLL2PCLK

        #define MCO2_SOURCE             LL_RCC_MCO2SOURCE_PLL2PCLK
#endif
 
-#define fn_mco1_prescaler(v) LL_RCC_MCO1_DIV_ ## v
-#define mco1_prescaler(v) fn_mco1_prescaler(v)
+#define fn_mco_prescaler(v) LL_RCC_MCO1_DIV_ ## v
+#define mco_prescaler(v) fn_mco_prescaler(v)
 
#define fn_mco2_prescaler(v) LL_RCC_MCO2_DIV_ ## v
#define mco2_prescaler(v) fn_mco2_prescaler(v)

@@ -85,19 +84,19 @@ extern "C" {
__unused
static inline void stm32_clock_control_mco_init(void)
{
-#ifndef CONFIG_CLOCK_STM32_MCO1_SRC_NOCLOCK
+#ifdef CONFIG_CLOCK_STM32_MCO
#ifdef CONFIG_SOC_SERIES_STM32F1X
-       LL_RCC_ConfigMCO(MCO1_SOURCE);
+       LL_RCC_ConfigMCO(MCO_SOURCE);
#else
-       LL_RCC_ConfigMCO(MCO1_SOURCE,
-                        mco1_prescaler(CONFIG_CLOCK_STM32_MCO1_DIV));
+       LL_RCC_ConfigMCO(MCO_SOURCE,
+                     mco_prescaler(DT_PROP(DT_NODELABEL(clk_mco), prescaler)));
#endif
-#endif /* CONFIG_CLOCK_STM32_MCO1_SRC_NOCLOCK */
+#endif /* CONFIG_CLOCK_STM32_MCO */
 
-#ifndef CONFIG_CLOCK_STM32_MCO2_SRC_NOCLOCK
-       LL_RCC_ConfigMCO(MCO2_SOURCE,
-                        mco2_prescaler(CONFIG_CLOCK_STM32_MCO2_DIV));
-#endif /* CONFIG_CLOCK_STM32_MCO2_SRC_NOCLOCK */
+#ifdef CONFIG_CLOCK_STM32_MCO2
+    LL_RCC_ConfigMCO(MCO2_SOURCE,
+                     mco2_prescaler(DT_PROP(DT_NODELABEL(clk_mco2), prescaler)));
+#endif /* CONFIG_CLOCK_STM32_MCO2 */
}

(Then just remove all the Kconfig variables, leaving just ones for enabling MCO and MCO2 defaulting to status okay in the devicetree)

A significant problems I can see with the above is that the actual clock source for the MCO can be a few things not well represented in the device tree for some families (PLLQ, SYSCLK, PLLXDIV2). We could just do the approximate thing for all of these, but it might be a bit awkward:

• For SYSCLK, just do clocks = <&rcc 0 0>; since the RCC node as a clock source is somewhat just the SYSCLK, and AFAIK there arent any families with sources for the MCO that are also clocks that come from the RCC, but even so, we could leverage the other parts of the clock cell to indicate the right source
• For the PLLQ (or other PLL outputs), Im not sure if any families have MCO sourced from multiple outputs of the same PLL (ie, something like either PLLR or PLLQ output). If so, we might need to give the PLLs a clock cell parameter to differentiate their outputs if the PLL outputs more than one clock (which we should probably have anyway...)
• For PLLXDIV2, Im not sure if any family has both PLL and PLLXDIV2, so just sourcing it to PLL might just work (ie clocks = <&pll>; but its really DIV2. Not sure if this might be a problem for calculating frequencies by tracing back or anything?

Otherwise, this solution is just a bit cleaner and clearer IMO than what was done in this PR; just a bit more cleanup and trying to take a bit more advantage of the code that was there and moving everything to the device tree (would need to update all supported families to have the right MCO node(s) as well though). Minor other questions:

1. Should it be MCO or MCO1 since it seems a bit mixed and when a family has 2, its MCO1 and MCO2 but if its a family with just one its MCO...
2. I noticed the default slew rate for the PR on the hal_stm32 was left at the slowest value, which for some of the sources is probably okay but for others that are a bit higher speed might fall out of the switching speed of that rate. Im not sure though if it should be the fastest speed either since that would really only come into play if running the SYSCLK out the MCO. I feel like a compromise of medium-speed might be good as that can range up into the tens of MHz.

[erwango]

@mrrosen Thanks for digging into that. Your proposal looks globally fine but for the clock source selection, I was proposing to reuse existing clock-control API. See #68846 (comment) (which one point remaining to be settled about the presecaler).
What do you think ?

[mrrosen]

@mrrosen Thanks for digging into that. Your proposal looks globally fine but for the clock source selection, I was proposing to reuse existing clock-control API. See #68846 (comment) (which one point remaining to be settled about the presecaler).
What do you think ?

I think using a separate prescaler property would be the best way to handle it since it lets you limit it nicely in the binding as in my example above while being very human-reabable without macros in the device tree since it seems it can be converted nicely into STM LL macros as is done in the driver now.

As for selecting the clock source, using clocks = <&rcc STM32_SRC_PLL_Q MCO_SEL(3)>; is ok but without macros, it's a bit more involved to know what MCO_SEL(3) does exactly. I generally like how the PLL and some of the other higher-level clocks are handled by just referencing each other via phandles as it shows a clear dependency and is generally pretty clear. It just has the pitfalls I mentioned in a few specific cases.

Technically, it could do something more common to how the rcc cells would now by doing clocks = <&rcc {REG_CONTAINING_MCO_SEL} LL_RCC_MCO1SOURCE_LSE>; or something like that (could use intermediate macros too), and that would be consistent with how the RCC works kinda; but then it feels like MCO should be selected within the RCC node itself let some of the other clocks are. I feel like it should be like the PLL as I tried to do above, or it seems like it should be a child of the RCC and be treated differently from the other source clocks like PLL, HSE, etc.

[erwango]

I think using a separate prescaler property would be the best way to handle it since it lets you limit it nicely in the binding as in my example above while being very human-reabable without macros in the device tree since it seems it can be converted nicely into STM LL macros as is done in the driver now.

Sure, fine.

As for selecting the clock source, using clocks = <&rcc STM32_SRC_PLL_Q MCO_SEL(3)>; is ok but without macros, it's a bit more involved to know what MCO_SEL(3) does exactly.

Ack on the complain about the syntax. Though, since it's there for other nodes, people have to deal with it anyway. Though, it'll just work and allow to provide the configuration w/o further implementation than adding the right MCO macros. It has no impact on clock control driver and all the system to check if the clock is enabled is already there.
Also, maintenance will be easier as it doesn't require to add/maintain a new system of macros, and user to take into account a new way of configuring clocks (even if more simple).

Hopefully, once #72102 is completed, we can get a more friendly syntax.

Edit: Note: It is not technically possible to use LL headers in device tree such as clocks = <&rcc {REG_CONTAINING_MCO_SEL} LL_RCC_MCO1SOURCE_LSE>;. You need to provide alternate headers anyway.