driver: sleeptimer: siwx917: Add siwx91x Sleeptimer driver #87603

fimohame · 2025-03-25T06:08:57Z

This commit enables the Sleeptimer driver support for the siwx917 device.

zephyrbot · 2025-03-25T06:10:00Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_silabs	zephyrproject-rtos/hal_silabs@`9d32354` (`main`)	zephyrproject-rtos/hal_silabs#93	zephyrproject-rtos/hal_silabs#93/files

⛔ DNM label due to: 1 project with PR revision

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

jhedberg · 2025-03-25T10:16:25Z

@fimohame how is this different from #87550? Why did you close that one?

fimohame · 2025-03-25T10:27:57Z

I was not able to checkout to that branch... ran into some issues. Wasn't able to access it. Thus created an another branch resolving the compliance issue

jhedberg · 2025-03-25T10:45:47Z

I was not able to checkout to that branch... ran into some issues. Wasn't able to access it. Thus created an another branch resolving the compliance issue

You shouldn't need to create a new branch or PR to fix such issues, rather you should be able to just git push --force to the original branch.

fimohame · 2025-03-25T10:52:00Z

Noted.

jerome-pouiller · 2025-03-25T10:34:49Z

dts/arm/silabs/siwg917.dtsi

+			reg = <0x24048c00 0x78>;
+			interrupts = <22 0>;
+			interrupt-parent = <&nvic>;
+			interrupt-names = "irq022";


Can you provide a meaningful name for the user?

IRQ_CONNECT(DT_IRQ(DT_RTC, irq), DT_IRQ(DT_RTC, priority),
CONCAT(DT_STRING_UPPER_TOKEN_BY_IDX(DT_RTC, interrupt_names, 0), _IRQHandler),
0, 0);
for 917 it requires to be _Handler instead...
dtsi we could keep it as systrc & in dts we could name it as irq022.. @jerome-pouiller what's your say on it?

See my comment above: if the upstream is okay, we solve the issue by defining an alias to IRQ022_Handler.

jerome-pouiller · 2025-03-25T10:35:29Z

dts/arm/silabs/siwg917.dtsi

+			interrupts = <22 0>;
+			interrupt-parent = <&nvic>;
+			interrupt-names = "irq022";
+			clock-frequency = <32768>;


You can use <DT_FREQ_K(32)>

Use will take it up.. will be addressed

jerome-pouiller · 2025-03-25T10:37:13Z

dts/arm/silabs/siwg917.dtsi

+			compatible = "silabs,gecko-stimer";
+			reg = <0x24048c00 0x78>;
+			interrupts = <22 0>;
+			interrupt-parent = <&nvic>;


Is it required? Until now, we didn't specified the interrupt controller.

I have updated the changes

Agree with jérome, actually in the dtsi of other board, we do not used interrupt-parent, why it is needed ?

jerome-pouiller · 2025-03-25T10:50:16Z

soc/silabs/silabs_siwx91x/Kconfig.defconfig

+	default n if SILABS_SLEEPTIMER_TIMER
+
+configdefault SYS_CLOCK_TICKS_PER_SEC
+	default 1024 if SILABS_SLEEPTIMER_TIMER


@asmellby has found 1024 was not enough to pass Zephyr timer tests. Maybe:

configdefault SYS_CLOCK_TICKS_PER_SEC default 128 if !TICKLESS_KERNEL && SILABS_SLEEPTIMER_TIMER default 32768 if SILABS_SLEEPTIMER_TIMER

When checked zephyr timer fails with the above configurations, where with 1024 it passes.

which zephyr timer test have you done ?

behaviour_timer test app

32768 makes sense on EFR32, since that's what is used in SiSDK. Wiseconnect uses 1024 in FreeRTOS, though, so IMO it makes sense to keep it the same here.

jerome-pouiller · 2025-03-25T10:51:21Z

soc/silabs/silabs_siwx91x/Kconfig.defconfig

@@ -3,6 +3,15 @@

 if SOC_FAMILY_SILABS_SIWX91X

+configdefault SILABS_SLEEPTIMER_TIMER
+	default y


SLEEPTIMER has some drawbacks. Maybe default y if PM?

jerome-pouiller · 2025-03-25T10:57:48Z

modules/hal_silabs/wiseconnect/CMakeLists.txt

@@ -151,5 +152,32 @@ if(CONFIG_WISECONNECT_NETWORK_STACK)
  )
 endif() # CONFIG_WISECONNECT_NETWORK_STACK

+# Sleeptimer for 917 devices


This comment only repeats the code. Drop it.

jerome-pouiller · 2025-03-25T11:03:17Z

soc/silabs/Kconfig.siwx91x

+	bool
+	select CODE_DATA_RELOCATION
+	help
+		Set if the Sleeptimer HAL module is used for SIWX91X.


Bad indentation

jerome-pouiller · 2025-03-25T11:15:30Z

drivers/timer/silabs_sleeptimer_timer.c

@@ -121,9 +121,15 @@ static int sleeptimer_init(void)
 	sl_status_t status = SL_STATUS_OK;
 	struct sleeptimer_timer_data *timer = &g_sleeptimer_timer_data;

+#ifdef CONFIG_SOC_FAMILY_SILABS_SIWX91X


I am annoyed with that. A driver should be independent of the underlying SoC.

IRQ_CONNECT(DT_IRQ(DT_RTC, irq), DT_IRQ(DT_RTC, priority), CONCAT(DT_STRING_UPPER_TOKEN_BY_IDX(DT_RTC, interrupt_names, 0), _IRQHandler), 0, 0); for efr it's been concat with _IRQHandler so I have defined with way for 91x.. Is there any other ways we could handle it?

It seems wiseconnect define some aliases for compliance with cmsis:

#define UDMA0_IRQHandler IRQ033_Handler #define UDMA1_IRQHandler IRQ010_Handler #define I2S0_IRQHandler IRQ064_Handler #define I2S1_IRQHandler IRQ014_Handler #define I2C0_IRQHandler IRQ042_Handler #define I2C1_IRQHandler IRQ061_Handler #define I2C2_IRQHandler IRQ013_Handler #define WDT_IRQHandler IRQ020_Handler

Do you think the upstream would be okay to add an alias for IRQ022?

I'm certainly not sure, @asmellby could you please provide your thoughts

I agree that if we can put such a define in an appropriate header, that would make it cleaner. It would honestly be even better if the function name for the ISR was part of the sleeptimer interface, but that's not the case.
I suggest putting the alias in soc/silabs/silabs_siwx91x/siwg917/soc.h for now.

jerome-pouiller · 2025-03-25T11:22:54Z

modules/hal_silabs/wiseconnect/CMakeLists.txt

@@ -16,6 +16,7 @@ zephyr_compile_definitions(
  SLI_SI91X_MCU_INTERFACE
  SLI_SI917
  SLI_SI917B0
+  SLI_SI91X_ENABLE_OS


I have not been able to find documentation for this symbols. What it is doing?

In sl_sleeptimer_hal_si91x_sysrtc.c as part of sleeptimer_hal_init_timer
#ifdef SLI_SI91X_ENABLE_OS
//Giving higher priority to SYSRTC to service wakeup
NVIC_SetPriority(NPSS_TO_MCU_SYRTC_INTR_IRQn, SYSRTC_IRQ_PRIORITY);
#endif

To enable OS --> Add SLI_SI91X_ENABLE_OS in preprocessor */

Interrupts are normally managed by Zephyr. Typically, we have to apply the priority defined in the DT (interrupts = <22 0>).

jerome-pouiller · 2025-03-25T11:32:35Z

modules/hal_silabs/wiseconnect/CMakeLists.txt

+    SL_CODE_COMPONENT_SLEEPTIMER=sleeptimer
+    SL_CODE_COMPONENT_HAL_SYSRTC=hal_sysrtc
+  )
+endif() # CONFIG_SOC_SILABS_SLEEPTIMER


This section is redundant with modules/hal_silabs/simplicity_sdk/CMakeLists.txt. Maybe we should split the SOC_SILABS_SLEEPTIMER symbol in smaller parts:

config SOC_SILABS_SLEEPTIMER depends on SOC_SILABS_SIWX91X_SYSRTC || SOC_SILABS_S2_SYSRTC

@asmellby what is your opinion here?

We need to clean up the HAL level Kconfig symbols in general. Symbols are being mixed between the GSDK and SiSDK HALs, and the scope of symbols is often tied to their use in Zephyr, rather than the feature they provide from the HAL. We should redo all the SiSDK symbols to properly map to SiSDK drivers, which will make them more easily composable and reusable for SiWx91x. But that should be a separate PR from this one.

This commit enables the Sleeptimer driver support for the siwx917 device. Signed-off-by: S Mohamed Fiaz <fiaz.mohamed@silabs.com>

jerome-pouiller · 2025-03-27T09:21:27Z

soc/silabs/Kconfig

+if SOC_FAMILY_SILABS_SIWX91X
+	source "soc/silabs/silabs_siwx91x/Kconfig"
+endif
+


I believe this addition is redundant with rsource "*/Kconfig" above.

zephyrbot added manifest manifest-hal_silabs DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Mar 25, 2025

fimohame force-pushed the siwx91x-sleeptimer branch from 349e9e6 to 4a67912 Compare March 25, 2025 06:20

fimohame marked this pull request as ready for review March 25, 2025 08:01

zephyrbot added platform: Silabs Silicon Labs area: Timer Timer labels Mar 25, 2025

zephyrbot requested review from andyross, asmellby, jerome-pouiller, jhedberg, Martinhoff-maker, mnkp, rettichschnidi, teburd and yonsch March 25, 2025 08:01

zephyrbot assigned jhedberg and asmellby Mar 25, 2025

jerome-pouiller reviewed Mar 25, 2025

View reviewed changes

fimohame force-pushed the siwx91x-sleeptimer branch from 4a67912 to 0116259 Compare March 25, 2025 16:01

driver: sleeptimer: siwx917: Add siwx91x Sleeptimer driver

1e31504

This commit enables the Sleeptimer driver support for the siwx917 device. Signed-off-by: S Mohamed Fiaz <fiaz.mohamed@silabs.com>

fimohame force-pushed the siwx91x-sleeptimer branch from 0116259 to 1e31504 Compare March 26, 2025 11:15

jerome-pouiller reviewed Mar 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver: sleeptimer: siwx917: Add siwx91x Sleeptimer driver #87603

driver: sleeptimer: siwx917: Add siwx91x Sleeptimer driver #87603

fimohame commented Mar 25, 2025

zephyrbot commented Mar 25, 2025 •

edited by github-actions bot

Loading

jhedberg commented Mar 25, 2025

fimohame commented Mar 25, 2025

jhedberg commented Mar 25, 2025

fimohame commented Mar 25, 2025

jerome-pouiller Mar 25, 2025

fimohame Mar 25, 2025

jerome-pouiller Mar 25, 2025

jerome-pouiller Mar 25, 2025

fimohame Mar 25, 2025

jerome-pouiller Mar 25, 2025

fimohame Mar 25, 2025 •

edited

Loading

Martinhoff-maker Mar 26, 2025

jerome-pouiller Mar 25, 2025

fimohame Mar 25, 2025

Martinhoff-maker Mar 26, 2025

fimohame Mar 26, 2025

asmellby Mar 28, 2025

jerome-pouiller Mar 25, 2025

jerome-pouiller Mar 25, 2025

jerome-pouiller Mar 25, 2025

jerome-pouiller Mar 25, 2025

fimohame Mar 25, 2025

jerome-pouiller Mar 25, 2025

fimohame Mar 25, 2025

asmellby Mar 28, 2025

jerome-pouiller Mar 25, 2025

fimohame Mar 25, 2025

jerome-pouiller Mar 25, 2025

jerome-pouiller Mar 25, 2025

asmellby Mar 28, 2025

jerome-pouiller Mar 27, 2025

driver: sleeptimer: siwx917: Add siwx91x Sleeptimer driver #87603

Are you sure you want to change the base?

driver: sleeptimer: siwx917: Add siwx91x Sleeptimer driver #87603

Conversation

fimohame commented Mar 25, 2025

zephyrbot commented Mar 25, 2025 • edited by github-actions bot Loading

jhedberg commented Mar 25, 2025

fimohame commented Mar 25, 2025

jhedberg commented Mar 25, 2025

fimohame commented Mar 25, 2025

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

fimohame Mar 25, 2025 • 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

Choose a reason for hiding this comment

zephyrbot commented Mar 25, 2025 •

edited by github-actions bot

Loading

fimohame Mar 25, 2025 •

edited

Loading