| Message ID | 20241108191024.2931097-2-roqueh@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make WDOGCONTROL.INTEN the counter enable of the CMSDK APB Watchdog | expand |
On Fri, 8 Nov 2024 at 19:10, Roque Arcudia Hernandez <roqueh@google.com> wrote: > > Current watchdog is free running out of reset, this combined with the > fact that current implementation also ensures the counter is running > when programing WDOGLOAD creates issues when the firmware defer the > programing of WDOGCONTROL.INTEN much later after WDOGLOAD. Arm > Programmer's Model documentation states that INTEN is also the > counter enable: > > > INTEN > > > > Enable the interrupt event, WDOGINT. Set HIGH to enable the counter > > and the interrupt, or LOW to disable the counter and interrupt. > > Reloads the counter from the value in WDOGLOAD when the interrupt > > is enabled, after previously being disabled. > > Source of the time of writing: > > https://developer.arm.com/documentation/ddi0479/d/apb-components/apb-watchdog/programmers-model I see that the URL in the current sources https://developer.arm.com/products/system-design/system-design-kits/cortex-m-system-design-kit is no longer working -- would you mind including a patch that updates the URL in the comment at the top of the file to the new one https://developer.arm.com/documentation/ddi0479/ please? > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com> > Reviewed-by: Stephen Longfield <slongfield@google.com> > Reviewed-by: Joe Komlodi <komlodi@google.com> > --- > hw/watchdog/cmsdk-apb-watchdog.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/hw/watchdog/cmsdk-apb-watchdog.c b/hw/watchdog/cmsdk-apb-watchdog.c > index 7ad46f9410..e6ddc0a53b 100644 > --- a/hw/watchdog/cmsdk-apb-watchdog.c > +++ b/hw/watchdog/cmsdk-apb-watchdog.c > @@ -202,10 +202,10 @@ static void cmsdk_apb_watchdog_write(void *opaque, hwaddr offset, > */ > ptimer_transaction_begin(s->timer); > ptimer_set_limit(s->timer, value, 1); > - ptimer_run(s->timer, 0); > ptimer_transaction_commit(s->timer); > break; This looks like a correct change, but the comment just above here needs to be updated to match it (it currently says "and make sure we're counting"). Otherwise this change looks good. -- PMM
diff --git a/hw/watchdog/cmsdk-apb-watchdog.c b/hw/watchdog/cmsdk-apb-watchdog.c index 7ad46f9410..e6ddc0a53b 100644 --- a/hw/watchdog/cmsdk-apb-watchdog.c +++ b/hw/watchdog/cmsdk-apb-watchdog.c @@ -202,10 +202,10 @@ static void cmsdk_apb_watchdog_write(void *opaque, hwaddr offset, */ ptimer_transaction_begin(s->timer); ptimer_set_limit(s->timer, value, 1); - ptimer_run(s->timer, 0); ptimer_transaction_commit(s->timer); break; - case A_WDOGCONTROL: + case A_WDOGCONTROL: { + uint32_t prev_control = s->control; if (s->is_luminary && 0 != (R_WDOGCONTROL_INTEN_MASK & s->control)) { /* * The Luminary version of this device ignores writes to @@ -215,8 +215,25 @@ static void cmsdk_apb_watchdog_write(void *opaque, hwaddr offset, break; } s->control = value & R_WDOGCONTROL_VALID_MASK; + if (R_WDOGCONTROL_INTEN_MASK & (s->control ^ prev_control)) { + ptimer_transaction_begin(s->timer); + if (R_WDOGCONTROL_INTEN_MASK & s->control) { + /* + * Set HIGH to enable the counter and the interrupt. Reloads + * the counter from the value in WDOGLOAD when the interrupt + * is enabled, after previously being disabled. + */ + ptimer_set_count(s->timer, ptimer_get_limit(s->timer)); + ptimer_run(s->timer, 0); + } else { + /* Or LOW to disable the counter and interrupt. */ + ptimer_stop(s->timer); + } + ptimer_transaction_commit(s->timer); + } cmsdk_apb_watchdog_update(s); break; + } case A_WDOGINTCLR: s->intstatus = 0; ptimer_transaction_begin(s->timer); @@ -305,8 +322,14 @@ static void cmsdk_apb_watchdog_reset(DeviceState *dev) s->resetstatus = 0; /* Set the limit and the count */ ptimer_transaction_begin(s->timer); + /* + * We need to stop the ptimer before setting its limit reset value. If the + * order is the opposite when the code executes the stop after setting a new + * limit it may want to recalculate the count based on the current time (if + * the timer was currently running) and it won't get the proper reset value. + */ + ptimer_stop(s->timer); ptimer_set_limit(s->timer, 0xffffffff, 1); - ptimer_run(s->timer, 0); ptimer_transaction_commit(s->timer); }