Re: [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method

[Udit Kumar <udit.kumar@nxp.com>]

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Tuesday, December 18, 2018 6:40 PM
> To: edk2-devel@lists.01.org
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm
> <leif.lindholm@linaro.org>; Sami Mujawar <sami.mujawar@arm.com>; Thomas
> Panakamattam Abraham <thomas.abraham@arm.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Udit Kumar <udit.kumar@nxp.com>; Matteo
> Carlini <Matteo.Carlini@arm.com>; Nariman Poushin
> <nariman.poushin@linaro.org>
> Subject: [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement
> RegisterHandler() method
> 
> Even though UEFI does not appear to use it, let's implement the complete PI
> watchdog protocol, including handler registration, which will be invoked instead
> of the ResetSystem() runtime service when the watchdog timer expires.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 34
> ++++++++++++++------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 717a180a64ec..21118a3c88d1 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -42,6 +42,7 @@ STATIC UINTN mTimerFrequencyHz = 0;  STATIC UINT64
> mNumTimerTicks = 0;
> 
>  STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
> +STATIC EFI_WATCHDOG_TIMER_NOTIFY        mWatchdogNotify;
> 
>  STATIC
>  VOID
> @@ -107,17 +108,25 @@ WatchdogInterruptHandler (
>    )
>  {
>    STATIC CONST CHAR16 ResetString[]= L"The generic watchdog timer ran out.";
> +  UINT64              TimerPeriod;
> 
>    WatchdogDisable ();
> 
>    mInterruptProtocol->EndOfInterrupt (mInterruptProtocol, Source);
> 
> -  gRT->ResetSystem (
> -         EfiResetCold,
> -         EFI_TIMEOUT,
> -         StrSize (ResetString),
> -         (VOID *) &ResetString
> -         );
> +  //
> +  // The notify function should be called with the elapsed number of
> + ticks  // since the watchdog was armed, which should exceed the timer period.
> +  // We don't actually know the elapsed number of ticks, so let's
> + return  // the timer period plus 1.
> +  //
> +  if (mWatchdogNotify != NULL) {
> +    TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) *
> mNumTimerTicks);
> +    mWatchdogNotify (TimerPeriod + 1);
> +  } else {
> +    gRT->ResetSystem (EfiResetCold, EFI_TIMEOUT, StrSize (ResetString),
> +           (CHAR16 *)ResetString);
> +  }

Here too, please handle reset in all cases 
 
>    // If we got here then the reset didn't work
>    ASSERT (FALSE);
> @@ -155,9 +164,16 @@ WatchdogRegisterHandler (
>    IN EFI_WATCHDOG_TIMER_NOTIFY                NotifyFunction
>    )
>  {
> -  // ERROR: This function is not supported.
> -  // The watchdog will reset the board
> -  return EFI_UNSUPPORTED;
> +  if (mWatchdogNotify == NULL && NotifyFunction == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (mWatchdogNotify != NULL && NotifyFunction != NULL) {
> +    return EFI_ALREADY_STARTED;
> +  }
> +
> +  mWatchdogNotify = NotifyFunction;
> +  return EFI_SUCCESS;
>  }
> 
>  /**
> --
> 2.17.1