drivers: counter: siwx91x: Enable siwx91x Counter driver #88756
Conversation
github-actions
bot
added
area: Counter
platform: Silabs
github-actions
bot
requested review from
Martinhoff-maker,
asmellby,
jerome-pouiller,
jhedberg,
kartben,
nashif and
nordic-krch
drivers/counter/Kconfig.gecko
Outdated
| Enable the counter driver for Sleep Timer module for Silicon Labs | ||
| Gecko chips. | ||
|
|
||
| if SOC_FAMILY_SILABS_SIWX91X |
There was a problem hiding this comment.
I don't see why a new Kconfig option needs to be added, the existing one should be sufficient and work for both Series 2 and SiWx91x.
There was a problem hiding this comment.
Agreed, updated accordingly.
| static void counter_gecko_0_irq_config(void) | ||
| { | ||
| IRQ_DIRECT_CONNECT(DT_INST_IRQN(0), DT_INST_IRQ(0, priority), STIMER_IRQ_HANDLER, 0); | ||
| IRQ_DIRECT_CONNECT(DT_IRQ(DT_RTC, irq), DT_IRQ(DT_RTC, priority), |
There was a problem hiding this comment.
The responsibility of interrupt configuration between the Timer and Counter drivers needs to be decided, they shouldn't both configure it.
There was a problem hiding this comment.
I am using sl_sleeptimer_get_timer_frequency() to identify the driver initialization. By default, it always returns 0 unitl sl_sleeptimer_init() is called. Hope this is okay.
There was a problem hiding this comment.
The problem is that the IRQ_(DIRECT_)CONNECT macro puts things into linker sections, the code doesn't need to execute. So wrapping it in an if statement doesn't prevent it from registering the interrupt. Only one source file should contain the macro at all. IMO either a) the counter driver should depend on the OS timer driver and never touch the interrupt, b) the interrupt registration should be moved to a shared file (soc.c?) guarded by #if defined(counter_sleeptimer) || defined(timer_sleeptimer), or c) the interrupt registration in the counter driver should be guarded by #ifndef timer_sleeptimer. Not sure which option is the best.
Also consider initialization order -- the counter driver is pre-kernel 1 and the timer driver is pre-kernel 2, so the timer driver is never initialized when counter init runs with the current priorities.
There was a problem hiding this comment.
Added #ifndef CONFIG_SILABS_SLEEPTIMER_TIMER guard in counter driver. Also, changes the counter driver init priority to POST_KERNEL.
smalae
force-pushed
the
silabs_siwx91x_counter_driver
branch
from
c5be94f to
5080583
Compare
smalae
force-pushed
the
silabs_siwx91x_counter_driver
branch
2 times, most recently
from
7687c00 to
0c7525d
Compare
smalae
force-pushed
the
silabs_siwx91x_counter_driver
branch
from
0c7525d to
eada131
Compare
jerome-pouiller
left a comment
jerome-pouiller
left a comment
There was a problem hiding this comment.
The code looks fine. However, I have to admit I don't understand how interactions between gecko_stimer and sleeptimer_timer are supposed to work. Maybe we should expand the binding documentation.
will update this |
smalae
force-pushed
the
silabs_siwx91x_counter_driver
branch
2 times, most recently
from
7330676 to
aae929c
Compare
Enable sleeptimer counter driver for siwx91x device Signed-off-by: Sai Santhosh Malae <Santhosh.Malae@silabs.com>
Add detailed description for silabs,gecko-stimer.yaml binding. Signed-off-by: Sai Santhosh Malae <Santhosh.Malae@silabs.com>
smalae
force-pushed
the
silabs_siwx91x_counter_driver
branch
from
aae929c to
62dd8bf
Compare
|
6 participants
Enable sleeptimer counter driver for siwx91x device