soc: silabs: Initialize oscillators and clock tree from DT, clean up init sequence #79415

asmellby · 2024-10-04T10:15:43Z

This PR cleans up the Series 2 init sequence by

Unifying all Series 2 devices on using the device init HAL (adding EFR32MG21, all the others already used it)
- DCDC is added to DeviceTree to be able to only initialize it if present
Migrating oscillator init from device init HAL to clock manager HAL based on DT
Migrating clock tree init from ad hoc board and driver config to clock manager HAL based on DT
Separating soc.c for Series 2 from Series 0 and 1, since the init sequence is fully disjoint

zephyrbot · 2024-10-04T10:16:48Z

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

Name	Old Revision	New Revision	Diff
hal_silabs	zephyrproject-rtos/hal_silabs@`d07d744`	zephyrproject-rtos/hal_silabs@`5c7a783` (main)	zephyrproject-rtos/hal_silabs@d07d744a..5c7a7834

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

jerome-pouiller · 2024-10-04T12:18:38Z

dts/bindings/regulator/silabs,series2-dcdc.yaml

+
+  silabs,pfmx-peak-current-microamp:
+    type: int
+    description: Peak current draw in PFMX mode.


I suggest to expand PFMX acronym here.

jerome-pouiller · 2024-10-04T12:30:47Z

soc/silabs/silabs_s2/config/sl_device_init_dcdc_config.h

+ * Copyright (c) 2024 Silicon Laboratories Inc.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */


If I understand right, this file is implicitly consumed by sl_device_init_dcdc(). Maybe you could add a comment explaining that.

That's said, I believe this file is not in the spirit of Zephyr. I feel we should create a new driver. This driver could directly call EMU_DCDCInit(), EMU_DCDCSetPFMXModePeakCurrent(), EMU_DCDCPowerOff() and EMU_DCDCBoostInit(). So, we could get rid of this layer of translation between DT and SL_DEVICE_INIT_DCDC_* definitions.

jerome-pouiller · 2024-10-04T12:34:44Z

dts/bindings/regulator/silabs,series2-dcdc.yaml

+    description: |
+      Operating mode. Allowed values are:
+        - 0: buck mode
+        - 1: boost mode


We could introduce symbols for that (see include/zephyr/dt-bindings/regulator/nrf5x.h)

(not a big deal)

jerome-pouiller · 2024-10-04T12:40:51Z

boards/silabs/radio_boards/xg24_rb4187c/xg24_rb4187c.dts

@@ -60,13 +60,55 @@
 };

 &cpu0 {
-	clock-frequency = <39000000>;
+	clock-frequency = <78000000>;


Doesn't this change be placed in the previous commit?

jerome-pouiller · 2024-10-04T12:44:36Z

soc/silabs/silabs_s2/config/sl_clock_manager_oscillator_config.h

+#define SL_CLOCK_MANAGER_OSCILLATOR_CONFIG_H
+
+#include <zephyr/devicetree.h>
+#include <soc.h>


I have the same comment than with DC-DC. I believe we should add a driver and directly call the inner functions.

However, after I have seen the upstream file (sl_clock_manager_init_hal_s2.c), I don't exactly if it is possible to do a proper integration. Maybe we should with upstream on this file.

jerome-pouiller · 2024-10-04T13:02:50Z

boards/silabs/dev_kits/sltb010a/board.c

jerome-pouiller · 2024-10-04T13:07:42Z

soc/silabs/silabs_s2/soc.c

+#ifdef CONFIG_PM
+#include <sl_hfxo_manager.h>
+#include <sl_power_manager.h>
+#endif


Probably it does not hurt to include sl_hfxo_manager.h and sl_power_manager.h even if !CONFIG_PM.

jerome-pouiller · 2024-10-04T13:08:57Z

soc/silabs/silabs_s2/soc.c

+#ifdef CONFIG_PM
+	sl_power_manager_init();
+	sl_hfxo_manager_init();
+#endif


If you include sl_hfxo_manager.h and sl_power_manager.h unconditionally, you can change the #ifdef CONFIG_PM in if (IS_ENABLED(CONFIG_PM)) and make the code easier.

jerome-pouiller · 2024-10-04T13:10:24Z

soc/silabs/silabs_s2/soc.c

+
+#if CONFIG_HW_HAS_SILABS_DCDC
+	sl_device_init_dcdc();
+#endif


I suggest:

if (IS_ENABLED(CONFIG_HW_HAS_SILABS_DCDC)) { sl_device_init_dcdc(); }

gmarull · 2024-10-09T07:59:50Z

boards/silabs/dev_kits/sltb010a/sltb010a.dts

+&dcdc {
+	status = "okay";
+	regulator-boot-on;
+	regulator-initial-mode = <0>; /* buck */


please, provide mode definitions to make DT readable, /* buck */ comment proofs it is not right now.

gmarull · 2024-10-09T08:00:25Z

dts/bindings/regulator/silabs,series2-dcdc.yaml

+    description: |
+      Output voltage for boost mode. Not used in buck mode.


no descriptions in standard properties please

gmarull · 2024-10-09T08:00:57Z

dts/bindings/regulator/silabs,series2-dcdc.yaml

+      - 2300000
+      - 2400000
+
+  silabs,pfmx-peak-current-microamp:


why not milliamps? all values have a x1000 scale factor

I used microamp for symmetry with the standard regulator properties regulator-*-microamp. I agree that the scale factor of 1000 is silly, and if milliamp is OK, I will change to it.

gmarull · 2024-10-09T08:01:36Z

soc/silabs/silabs_s2/Kconfig

+config HW_HAS_SILABS_DCDC
+	def_bool $(dt_compat_enabled,$(DT_COMPAT_SILABS_SERIES2_DCDC))


please avoid these redundant Kconfig options and just use DT natively in C code

I had been pointed to https://github.com/zephyrproject-rtos/zephyr/blob/main/soc/nordic/common/Kconfig.peripherals in the past, but if this is a pattern that we don't want to propagate, and just use DT directly instead, that is completely fine by me :)

gmarull · 2024-10-09T08:01:47Z

soc/silabs/silabs_s2/config/sl_device_init_dcdc_config.h

+#define SL_DEVICE_INIT_DCDC_CONFIG_H
+
+#include <zephyr/devicetree.h>
+#include <soc.h>


no soc.h please

gmarull · 2024-10-09T08:05:08Z

soc/silabs/silabs_s2/config/sl_device_init_dcdc_config.h

+#define SL_DEVICE_INIT_DCDC_BOOST_OUTPUT                                                           \
+	(DT_ENUM_HAS_VALUE(DCDC_NODE, regulator_init_microvolt, 1800000)                           \
+		 ? emuDcdcBoostOutputVoltage_1v8                                                   \
+	 : DT_ENUM_HAS_VALUE(DCDC_NODE, regulator_init_microvolt, 1900000)                         \
+		 ? emuDcdcBoostOutputVoltage_1v9                                                   \
+	 : DT_ENUM_HAS_VALUE(DCDC_NODE, regulator_init_microvolt, 2000000)                         \
+		 ? emuDcdcBoostOutputVoltage_2v0                                                   \
+	 : DT_ENUM_HAS_VALUE(DCDC_NODE, regulator_init_microvolt, 2100000)                         \
+		 ? emuDcdcBoostOutputVoltage_2v1                                                   \
+	 : DT_ENUM_HAS_VALUE(DCDC_NODE, regulator_init_microvolt, 2200000)                         \
+		 ? emuDcdcBoostOutputVoltage_2v2                                                   \
+	 : DT_ENUM_HAS_VALUE(DCDC_NODE, regulator_init_microvolt, 2300000)                         \
+		 ? emuDcdcBoostOutputVoltage_2v3                                                   \
+	 : DT_ENUM_HAS_VALUE(DCDC_NODE, regulator_init_microvolt, 2400000)                         \
+		 ? emuDcdcBoostOutputVoltage_2v4                                                   \
+		 : emuDcdcBoostOutputVoltage_1v8)


aren't enums correlated to voltages so that something like linear range can be used? This looks seriously ugly.

gmarull · 2024-10-09T08:05:15Z

soc/silabs/silabs_s2/config/sl_device_init_dcdc_config.h

+	(DT_ENUM_HAS_VALUE(DCDC_NODE, silabs_pfmx_peak_current_microamp, 50000)                    \
+		 ? DCDC_PFMXCTRL_IPKVAL_LOAD50MA                                                   \
+	 : DT_ENUM_HAS_VALUE(DCDC_NODE, silabs_pfmx_peak_current_microamp, 65000)                  \
+		 ? DCDC_PFMXCTRL_IPKVAL_LOAD65MA                                                   \
+	 : DT_ENUM_HAS_VALUE(DCDC_NODE, silabs_pfmx_peak_current_microamp, 73000)                  \


gmarull · 2024-10-09T08:05:51Z

boards/silabs/radio_boards/slwrb4180a/slwrb4180a_defconfig

@@ -5,6 +5,5 @@ CONFIG_CONSOLE=y
 CONFIG_UART_CONSOLE=y
 CONFIG_SERIAL=y
 CONFIG_GPIO=y
-CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=38400000
-CONFIG_CMU_HFCLK_HFXO=y
+CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=76800000


this setting should be grabbed from DT, see other examples in tree

gmarull · 2024-10-09T08:06:32Z

dts/arm/silabs/efr32mg21.dtsi

+	clocks {
+		clk_hfxo: clk-hfxo {
+			#clock-cells = <0>;
+			compatible = "silabs,hfxo";


is this device addressable? if so, it should have a @reg.

jerome-pouiller

I still have some concerns with use of config/sl_*_config.h. I believe we should work on a long term fix for these files. However, we can probably live with that for now.

That's said, I am happy to see much ugly code disappeared with this PR :-).

jerome-pouiller · 2024-10-09T15:30:51Z

dts/bindings/clock/silabs,series2-lfrco.yaml

+compatible: "silabs,series2-lfrco"
+
+description: |
+  Silicon Labs LFRCO peripheral.


Help messages are the right place to expand acronyms (in many case it provide a sufficient documentation).

jerome-pouiller · 2024-10-09T15:35:56Z

dts/bindings/clock/silabs,series2-lfxo.yaml

+  mode:
+    type: string
+    description: |
+      HFXO mode. Defaults to "xtal", expecting a 32768 Hz crystal oscillator on XI and XO pins.


HFXO? (BTW, you can probably get rid of this description. "LFXO mode" is not very helpful when the attribute is named "mode" and the device "LFXO").

asmellby · 2024-10-21T08:50:27Z

@gmarull Is my latest change what you had in mind? CMakeLists.txt will be updated to avoid referring to modules/hal_silabs from inside soc/ in the follow-on PR that introduces cmake-ext for hal_silabs.

asmellby · 2024-10-21T13:00:27Z

Rebased on top of latest main to resolve conflict.

Move the CMakeLists.txt and Kconfig files from the hal_silabs tree under modules/hal_silabs in the main tree. This also includes all Zephyr shim code from the hal_silabs tree. Signed-off-by: Johan Hedberg <johan.hedberg@silabs.com>

asmellby · 2024-10-22T11:05:02Z

Rebased on top of @jhedberg's change in #80148, adding DNM label until his PR is merged.

The DC-DC converter was unconditionally initialized with default settings on Series 2. Add device tree binding and nodes, and guard call to init function. Map DT options to config header from HAL. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Defconfig was only available at the vendor and series level, make it possible to have family specific definitions too. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

On Series 2, set the SYS_CLOCK_HW_CYCLES_PER_SEC Kconfig option from DeviceTree, rather than separately configuring it in board-level defconfig. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Switch EFR32MG21 to use the device init HAL. This makes the init sequence the same as the rest of Series 2. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Introduce bindings for Series 2 oscillators: HFRCODPLL, HFRCOEM23, LFRCO and LFXO. Add clock tree representation in devicetree `clocks` node. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Swap from the deprecated device_init_* functions to clock manager for clock tree configuration. Populate config headers using device tree representation of clock tree and oscillator config. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Clock setup is now done by the clock manager based on device tree configuration. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Series 2 always uses the device init HAL, while Series 0/1 never do. Create a separate soc.c for Series 2 to make both versions easier to read. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

rettichschnidi

Still works for the board sim3u1xx_dk 👍

asmellby requested review from jhedberg and jerome-pouiller October 4, 2024 10:15

asmellby requested review from kgugala and fkokosinski as code owners October 4, 2024 10:15

zephyrbot added area: Regulators platform: Silabs Silicon Labs area: Clock Control area: Timer Timer labels Oct 4, 2024

zephyrbot requested review from aasinclair, andyross, danieldegrasse, gmarull, nordic-krch and teburd October 4, 2024 10:16

zephyrbot assigned andyross Oct 4, 2024

zephyrbot added manifest manifest-hal_silabs DNM This PR should not be merged (Do Not Merge) labels Oct 4, 2024

asmellby force-pushed the feature/series2-init-cleanup branch from 2611b2f to 603fede Compare October 4, 2024 10:25

asmellby mentioned this pull request Oct 4, 2024

soc: silabs: Add soc_prep_hook() to initialize SMU #79418

Merged

asmellby force-pushed the feature/series2-init-cleanup branch from 603fede to 45e1fab Compare October 4, 2024 12:46

jerome-pouiller reviewed Oct 4, 2024

View reviewed changes

gmarull requested changes Oct 9, 2024

View reviewed changes

asmellby force-pushed the feature/series2-init-cleanup branch from 45e1fab to f8e8e43 Compare October 9, 2024 14:25

zephyrbot added the platform: Silabs SiM3U Silicon Labs SiM3U label Oct 9, 2024

zephyrbot requested a review from rettichschnidi October 9, 2024 14:25

jerome-pouiller previously approved these changes Oct 9, 2024

View reviewed changes

asmellby dismissed jerome-pouiller’s stale review via 17569f1 October 10, 2024 10:31

asmellby force-pushed the feature/series2-init-cleanup branch from f8e8e43 to 17569f1 Compare October 10, 2024 10:31

asmellby dismissed jhedberg’s stale review via eff9265 October 21, 2024 08:48

asmellby force-pushed the feature/series2-init-cleanup branch from eff9265 to 2eaa3d0 Compare October 21, 2024 12:58

jhedberg mentioned this pull request Oct 21, 2024

modules: hal_silabs: Move CMake and Kconfig to the main tree #80148

Closed

jhedberg added this to the v4.0.0 milestone Oct 21, 2024

asmellby force-pushed the feature/series2-init-cleanup branch from 2eaa3d0 to 6ededac Compare October 22, 2024 11:01

zephyrbot added the area: Process label Oct 22, 2024

zephyrbot requested review from MaureenHelm and nashif October 22, 2024 11:01

asmellby added the DNM This PR should not be merged (Do Not Merge) label Oct 22, 2024

asmellby added 9 commits October 22, 2024 13:06

soc: silabs: Introduce family specific defconfig

7bc4026

Defconfig was only available at the vendor and series level, make it possible to have family specific definitions too. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

soc: silabs: Derive SYS_CLOCK_HW_CYCLES_PER_SEC from DT

a640cf7

On Series 2, set the SYS_CLOCK_HW_CYCLES_PER_SEC Kconfig option from DeviceTree, rather than separately configuring it in board-level defconfig. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

soc: silabs: Enable device init on EFR32MG21

f9d7cf8

Switch EFR32MG21 to use the device init HAL. This makes the init sequence the same as the rest of Series 2. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

dts: silabs: Add clock bindings and clock tree

766695b

Introduce bindings for Series 2 oscillators: HFRCODPLL, HFRCOEM23, LFRCO and LFXO. Add clock tree representation in devicetree `clocks` node. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

boards: silabs: Remove clock configuration

b9740b6

Clock setup is now done by the clock manager based on device tree configuration. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

drivers: timer: gecko: Remove clock configuration

099d34b

Clock setup is now done by the clock manager based on device tree configuration. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

soc: silabs: Separate Series 2 soc.c

144e17a

Series 2 always uses the device init HAL, while Series 0/1 never do. Create a separate soc.c for Series 2 to make both versions easier to read. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

asmellby force-pushed the feature/series2-init-cleanup branch from 6ededac to 144e17a Compare October 22, 2024 11:07

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Oct 22, 2024

jhedberg approved these changes Oct 22, 2024

View reviewed changes

gmarull approved these changes Oct 22, 2024

View reviewed changes

asmellby mentioned this pull request Oct 22, 2024

drivers: timer: silabs: Add sleeptimer timer driver #80227

Merged

jerome-pouiller approved these changes Oct 22, 2024

View reviewed changes

henrikbrixandersen merged commit a11f0e6 into zephyrproject-rtos:main Oct 22, 2024
32 of 33 checks passed

rettichschnidi reviewed Oct 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

soc: silabs: Initialize oscillators and clock tree from DT, clean up init sequence #79415

soc: silabs: Initialize oscillators and clock tree from DT, clean up init sequence #79415

asmellby commented Oct 4, 2024

zephyrbot commented Oct 4, 2024 •

edited

Loading

jerome-pouiller Oct 4, 2024

jerome-pouiller Oct 4, 2024

jerome-pouiller Oct 4, 2024

jerome-pouiller Oct 4, 2024

jerome-pouiller Oct 4, 2024

jerome-pouiller Oct 4, 2024

jerome-pouiller Oct 4, 2024

jerome-pouiller Oct 4, 2024

jerome-pouiller Oct 4, 2024

gmarull Oct 9, 2024

gmarull Oct 9, 2024

gmarull Oct 9, 2024

asmellby Oct 9, 2024

gmarull Oct 9, 2024

asmellby Oct 9, 2024

gmarull Oct 9, 2024

gmarull Oct 9, 2024

gmarull Oct 9, 2024

gmarull Oct 9, 2024

gmarull Oct 9, 2024

jerome-pouiller left a comment

jerome-pouiller Oct 9, 2024

jerome-pouiller Oct 9, 2024

asmellby commented Oct 21, 2024

asmellby commented Oct 21, 2024

asmellby commented Oct 22, 2024

rettichschnidi left a comment •

edited

Loading

		description: \|
		Output voltage for boost mode. Not used in buck mode.

		config HW_HAS_SILABS_DCDC
		def_bool $(dt_compat_enabled,$(DT_COMPAT_SILABS_SERIES2_DCDC))

soc: silabs: Initialize oscillators and clock tree from DT, clean up init sequence #79415

soc: silabs: Initialize oscillators and clock tree from DT, clean up init sequence #79415

Conversation

asmellby commented Oct 4, 2024

zephyrbot commented Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerome-pouiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmellby commented Oct 21, 2024

asmellby commented Oct 21, 2024

asmellby commented Oct 22, 2024

rettichschnidi left a comment • edited Loading

Choose a reason for hiding this comment

zephyrbot commented Oct 4, 2024 •

edited

Loading

rettichschnidi left a comment •

edited

Loading