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
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;
- 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);
}
--
2.47.0.277.g8800431eea-googOn 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
I'll be removing the extra line in the comment and adding a new patch modifying the link in version 2. On Thu, Nov 14, 2024 at 4:53 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > 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
© 2016 - 2024 Red Hat, Inc.