Re: [PATCH 1/3] hw/watchdog/cmsdk_apb_watchdog: Fix INTEN issues

[Peter Maydell]

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