drivers: rcc: Improve the RCC definition for the sf32lb platform #99542

HalfSweet · 2025-11-18T06:11:18Z

Improve the RCC definition for the sf32lb platform.
All RCC configurations in sf32lb have been completed, with the following additions:

sys-clk-src
peri-clk-src
mpi1/2-clk-src
usb-clk-src
usb-div

ck-telecom · 2025-11-20T07:09:50Z

drivers: clock_control: sf3232lb_rcc: Add RCC configuration driver
Add RCC configuration driver.

Signed-off-by: Haoran Jiang <halfsweet@halfsweet.cn>

fix typo and commit format, also rebase and resovle conflicts.

gmarull

good job on extending this, see my comments

gmarull · 2025-11-20T10:12:46Z

dts/bindings/clock/sifli,sf32lb-rcc-clk.yaml

+      requires the corresponding DLL child node to be enabled. Defaults to
+      ``peri`` if not provided.


defaults need to be justified (e.g. boot time default) per bindings rules

gmarull · 2025-11-20T10:12:55Z

dts/bindings/clock/sifli,sf32lb-rcc-clk.yaml

+      requires the corresponding DLL child node to be enabled. Defaults to
+      ``peri`` if not provided.


gmarull · 2025-11-20T10:13:07Z

dts/bindings/clock/sifli,sf32lb-rcc-clk.yaml

+      Source used for the HPSYS peripheral clock domain. Defaults to ``hxt48`` if
+      not provided.


same, default actually not set

gmarull · 2025-11-20T10:13:18Z

dts/bindings/clock/sifli,sf32lb-rcc-clk.yaml

+    enum: ["hrc48", "hxt48", "lpclk", "dll1"]
+    description: |
+      Source used for the HPSYS system clock. When set to ``dll1`` the DLL1 child
+      node must be enabled. Defaults to ``hxt48`` if not provided.


same, default not set?

gmarull · 2025-11-20T10:14:01Z

dts/bindings/clock/sifli,sf32lb-rcc-clk.yaml

+      Divider applied to the selected USB clock source. The divided clock must
+      be 60 MHz. For example, when ``clk_dll2`` runs at 240 MHz the divider must
+      be ``4``.


Default should not be set if its value depends on how other things are configured.

Also, why isn't this computed automatically if target clock freq is known?

done. all settings have been assigned default values and corresponding descriptions.

gmarull · 2025-11-20T10:25:51Z

drivers/clock_control/clock_control_sf32lb_rcc.c

+enum sf32lb_sys_clk_idx {
+	SF32LB_SYS_CLK_IDX_HRC48,
+	SF32LB_SYS_CLK_IDX_HXT48,
+	SF32LB_SYS_CLK_IDX_LPCLK,
+	SF32LB_SYS_CLK_IDX_DLL1,
+};
+
+enum sf32lb_peri_clk_idx {
+	SF32LB_PERI_CLK_IDX_HRC48,
+	SF32LB_PERI_CLK_IDX_HXT48,
+};
+
+enum sf32lb_mpi_clk_idx {
+	SF32LB_MPI_CLK_IDX_DLL1,
+	SF32LB_MPI_CLK_IDX_DLL2,
+	SF32LB_MPI_CLK_IDX_PERI,
+};
+
+enum sf32lb_usb_clk_idx {
+	SF32LB_USB_CLK_IDX_SYSCLK,
+	SF32LB_USB_CLK_IDX_DLL2,
+};


since this seems to mimic DT enums, you may consider using this approach (so everything resides on a DT header): https://wiki.st.com/stm32mpu/wiki/Clock_device_tree_configuration (ref st,clksrc). I think it's better.

I have a slightly different perspective on this matter... While listing all data in the header file does significantly simplify the driver code, it makes editing the device tree less intuitive and prone to errors (such as misconfiguring the corresponding macro definitions).
Is there a recommended best practice for handling this scenario in Zephyr?

I don’t think there’s really a standard here, but it’s a common thing to look into other projects using devicetree to see how they have solved the same or similar problems.

After internal discussion, we concluded that using strings as enumerations would be preferable
I adjusted the order of the enumeration values so that each enum matches the final register setting value, making the driver code easier to read

gmarull · 2025-11-20T14:11:27Z

note, for future this is also needed:

It's weird it's controlled by RTC/PMUC, maybe RTC/PMUC need a clock control sub-node, and a separate driver can manage that, and then nodes like RTC would have clocks = <&rcc_clk XXX_ENABLE_CLOCK>, <&pmuc_clk XXX_ACTUAL_CLOCK>?

Add the RCC configuration binding on the sf32lb52 platform. - sys-clk-src - peri-clk-src - mpi1/2-clk-src - A - usb-div Signed-off-by: Haoran Jiang <halfsweet@halfsweet.cn>

Add RCC configuration driver. Signed-off-by: Haoran Jiang <halfsweet@halfsweet.cn>

Add sys-clk-src. Signed-off-by: Haoran Jiang <halfsweet@halfsweet.cn>

sonarqubecloud · 2025-11-25T12:51:21Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

gmarull · 2025-11-25T13:47:35Z

dts/bindings/clock/sifli,sf32lb-rcc-clk.yaml

+      Divider applied to the selected USB clock source. The divided clock must
+      be 60 MHz. For example, when ``clk_dll2`` runs at 240 MHz the divider must
+      be ``4`` (reset default).


as said, providing default here may not be ideal if clk_dll2 is configured to other frequencies.

Calculating the USB clock source and frequency division during compilation in C is simply too complex. Perhaps the correct allocation of the clock tree should be ensured by the person writing the device tree—when they enable the USB module, the corresponding frequency division should already be calculated. Of course, if it's not enabled, nothing happens. The default value of 4 here is merely because that's the default value after startup.

fair enough

zephyrbot added platform: SF32LB area: Devicetree Bindings area: Clock Control area: Boards/SoCs labels Nov 18, 2025

zephyrbot requested review from cameled, ck-telecom, gmarull and nordic-krch November 18, 2025 06:12

zephyrbot assigned gmarull Nov 18, 2025

HalfSweet marked this pull request as draft November 18, 2025 06:13

HalfSweet force-pushed the sf32lb/rcc branch from 989c861 to 4e1673d Compare November 18, 2025 06:28

HalfSweet marked this pull request as ready for review November 18, 2025 06:56

HalfSweet force-pushed the sf32lb/rcc branch from 4e1673d to bebd11b Compare November 18, 2025 07:33

ck-telecom previously approved these changes Nov 19, 2025

View reviewed changes

ck-telecom changed the title ~~drivers: rcc: Improve the RCC definition for the sf32lb52 platform~~ drivers: rcc: Improve the RCC definition for the sf32lb platform Nov 20, 2025

gmarull reviewed Nov 20, 2025

View reviewed changes

HalfSweet dismissed ck-telecom’s stale review via 27fee2d November 25, 2025 10:04

HalfSweet force-pushed the sf32lb/rcc branch 6 times, most recently from 0ad0230 to ecd0f69 Compare November 25, 2025 11:16

dts: arm: sifli: sf32lb52x: Improve the RCC definition for sf32lb

76b7696

Add the RCC configuration binding on the sf32lb52 platform. - sys-clk-src - peri-clk-src - mpi1/2-clk-src - A - usb-div Signed-off-by: Haoran Jiang <halfsweet@halfsweet.cn>

HalfSweet force-pushed the sf32lb/rcc branch from ecd0f69 to e969966 Compare November 25, 2025 11:26

HalfSweet added 2 commits November 25, 2025 20:21

drivers: clock_control: sf32lb_rcc: Add RCC configuration driver

9596d59

Add RCC configuration driver. Signed-off-by: Haoran Jiang <halfsweet@halfsweet.cn>

boards: sifli: sf32lb52_devkit_lcd: Add sys-clk-src

e10be0c

Add sys-clk-src. Signed-off-by: Haoran Jiang <halfsweet@halfsweet.cn>

HalfSweet force-pushed the sf32lb/rcc branch from e969966 to e10be0c Compare November 25, 2025 12:22

ck-telecom approved these changes Nov 25, 2025

View reviewed changes

gmarull reviewed Nov 25, 2025

View reviewed changes

gmarull approved these changes Nov 25, 2025

View reviewed changes

fabiobaltieri merged commit d70234f into zephyrproject-rtos:main Nov 25, 2025
27 checks passed

HalfSweet mentioned this pull request Dec 13, 2025

add HalfSweet as contributor #100979

Open

		requires the corresponding DLL child node to be enabled. Defaults to
		``peri`` if not provided.

		Source used for the HPSYS peripheral clock domain. Defaults to ``hxt48`` if
		not provided.

drivers: rcc: Improve the RCC definition for the sf32lb platform #99542

drivers: rcc: Improve the RCC definition for the sf32lb platform #99542

Uh oh!

Conversation

HalfSweet commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ck-telecom commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmarull commented Nov 20, 2025

Uh oh!

sonarqubecloud bot commented Nov 25, 2025

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HalfSweet commented Nov 18, 2025 •

edited

Loading

ck-telecom commented Nov 20, 2025 •

edited

Loading