drivers: watchdog: nrfx: add synchronization after stop #83776
Conversation
zephyrbot
requested review from
anangl,
jaz1-nordic,
katsuster,
kl-cruz,
magp-nordic,
martinjaeger,
masz-nordic and
nika-nordic
mstasiaknordic
force-pushed
the
wdt_stop_sync
branch
from
ded831a to
ae34586
Compare
drivers/watchdog/wdt_nrfx.c
Outdated
| @@ -73,6 +73,12 @@ static int wdt_nrf_disable(const struct device *dev) | |||
| return -EFAULT; | |||
| } | |||
|
|
|||
| #if !CONFIG_WDT_NRFX_NO_IRQ | |||
| while (!nrfy_wdt_events_process(config->wdt.p_reg, | |||
There was a problem hiding this comment.
if CONFIG_WDT_NRFX_NO_IRQ is not set (default setting), nrfx WDT IRQ handler is installed and will execute once NRF_WDT_EVENT_STOPPED. NRF_WDT_EVENT_STOPPED will be cleared in IRQ context, so this busy loop might execute forever.
Instead of polling event, k_sem_take should be called here. Corresponding k_sem_give should be executed in wdt_event_handler upon reception on NRF_WDT_EVENT_STOPPED.
Also, not all nRF devices can stop WDT so this extra code in wdt_event_handler needs to be enclosed in #if NRFX_WDT_HAS_STOP
|
@mstasiaknordic could you add Kconfig option to set |
mstasiaknordic
force-pushed
the
wdt_stop_sync
branch
from
ae34586 to
6c18f0d
Compare
Such Kconfig option already exists. From what I see, all existing watchdog tests use callbacks, thus none of them would work with |
mstasiaknordic
force-pushed
the
wdt_stop_sync
branch
from
6c18f0d to
5b4e48a
Compare
drivers/watchdog/wdt_nrfx.c
Outdated
| struct wdt_nrfx_data { | ||
| wdt_callback_t m_callbacks[NRF_WDT_CHANNEL_NUMBER]; | ||
| uint32_t m_timeout; | ||
| uint8_t m_allocated_channels; | ||
| bool enabled; | ||
| #if defined(WDT_NRFX_SYNC_STOP) | ||
| struct k_sem stop_sync; |
There was a problem hiding this comment.
maybe sync_stop would be better? stop_sync could mean that it stops synchronizing, which is not true
In order to ensure that watchdog channels are freed in proper driver state, synchronization in form of simple loop needs to be added after stopping. In no irq variant, it is already done on nrfx level. NRFY function can be replaced by NRFX one in the future. Signed-off-by: Michał Stasiak <michal.stasiak@nordicsemi.no>
mstasiaknordic
force-pushed
the
wdt_stop_sync
branch
from
5b4e48a to
0ab028e
Compare
In order to ensure that watchdog channels are freed in proper driver state, synchronization in form of semaphore needs to be added after stopping. In no irq variant, it is already done on nrfx level. NRFY function can be replaced by NRFX one in the future.