[PATCH v2 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup

[PATCH v2 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup

[Ard Biesheuvel]

This series cleans up the code of the two watchdog drivers we have for
ARM systems, and brings them in compliance with the PI spec, which
specifies that the default action of the watchdog can be overridden
by registering a handler.

Note that the TC2 code in edk2-platforms will have to be brought up to
date. The SP805 on the FVP model seems terminally broken (it is 'wired'
to the 24 MHz APB clock instead of the 32 kHz WDOG clock, so I'll switch
that one over to use the SBSA watchdog instead)

Changes since v1:
- always fall back to calling gRT_>ResetSystem() if the registered handler
  returns
- WARN() if running the SP805 driver with interrupt handling disabled
- add some R-bs

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>
Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
Cc: Udit Kumar <udit.kumar@nxp.com>
Cc: Matteo Carlini <Matteo.Carlini@arm.com>
Cc: Nariman Poushin <nariman.poushin@linaro.org>

Ard Biesheuvel (4):
  ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup
  ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
  ArmPkg/GenericWatchdogDxe: clean up the code
  ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method

 ArmPlatformPkg/ArmPlatformPkg.dec             |   1 +
 .../GenericWatchdogDxe/GenericWatchdogDxe.inf |   9 +-
 .../SP805WatchdogDxe/SP805WatchdogDxe.inf     |  15 +-
 .../GenericWatchdogDxe/GenericWatchdogDxe.c   | 143 ++++++++------
 .../Drivers/SP805WatchdogDxe/SP805Watchdog.c  | 187 +++++++++++-------
 5 files changed, 213 insertions(+), 142 deletions(-)

-- 
2.19.2

[PATCH v2 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup

[Ard Biesheuvel]

Before fixing the SP805 driver, let's clean it up a bit. No
functional changes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf | 11 +--
 ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c      | 97 ++++++++++----------
 2 files changed, 52 insertions(+), 56 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
index 37924f2e3cd2..c3971fb035d3 100644
--- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
+++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
@@ -1,6 +1,7 @@
 /** @file
 *
 *  Copyright (c) 2011-2012, ARM Limited. All rights reserved.
+*  Copyright (c) 2018, Linaro Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -18,35 +19,29 @@ [Defines]
   FILE_GUID                      = ebd705fb-fa92-46a7-b32b-7f566d944614
   MODULE_TYPE                    = DXE_DRIVER
   VERSION_STRING                 = 1.0
-
   ENTRY_POINT                    = SP805Initialize
 
 [Sources.common]
   SP805Watchdog.c
 
 [Packages]
-  MdePkg/MdePkg.dec
-  EmbeddedPkg/EmbeddedPkg.dec
   ArmPkg/ArmPkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
+  MdePkg/MdePkg.dec
 
 [LibraryClasses]
   BaseLib
-  BaseMemoryLib
   DebugLib
   IoLib
-  PcdLib
-  UefiLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
-  UefiRuntimeServicesTableLib
 
 [Pcd]
   gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase
   gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz
 
 [Protocols]
-  gEfiWatchdogTimerArchProtocolGuid
+  gEfiWatchdogTimerArchProtocolGuid       ## ALWAYS_PRODUCES
 
 [Depex]
   TRUE
diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
index 0a9f64095bf8..12c2f0a1fe49 100644
--- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
+++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
@@ -1,6 +1,7 @@
 /** @file
 *
 *  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
+*  Copyright (c) 2018, Linaro Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -19,16 +20,13 @@
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/IoLib.h>
-#include <Library/PcdLib.h>
 #include <Library/UefiBootServicesTableLib.h>
-#include <Library/UefiRuntimeServicesTableLib.h>
-#include <Library/UefiLib.h>
 
 #include <Protocol/WatchdogTimer.h>
 
 #include "SP805Watchdog.h"
 
-EFI_EVENT                           EfiExitBootServicesEvent = (EFI_EVENT)NULL;
+STATIC EFI_EVENT          mEfiExitBootServicesEvent;
 
 /**
   Make sure the SP805 registers are unlocked for writing.
@@ -43,8 +41,8 @@ SP805Unlock (
   VOID
   )
 {
-  if( MmioRead32(SP805_WDOG_LOCK_REG) == SP805_WDOG_LOCK_IS_LOCKED ) {
-    MmioWrite32(SP805_WDOG_LOCK_REG, SP805_WDOG_SPECIAL_UNLOCK_CODE);
+  if (MmioRead32 (SP805_WDOG_LOCK_REG) == SP805_WDOG_LOCK_IS_LOCKED) {
+    MmioWrite32 (SP805_WDOG_LOCK_REG, SP805_WDOG_SPECIAL_UNLOCK_CODE);
   }
 }
 
@@ -61,9 +59,9 @@ SP805Lock (
   VOID
   )
 {
-  if( MmioRead32(SP805_WDOG_LOCK_REG) == SP805_WDOG_LOCK_IS_UNLOCKED ) {
+  if (MmioRead32 (SP805_WDOG_LOCK_REG) == SP805_WDOG_LOCK_IS_UNLOCKED) {
     // To lock it, just write in any number (except the special unlock code).
-    MmioWrite32(SP805_WDOG_LOCK_REG, SP805_WDOG_LOCK_IS_LOCKED);
+    MmioWrite32 (SP805_WDOG_LOCK_REG, SP805_WDOG_LOCK_IS_LOCKED);
   }
 }
 
@@ -77,8 +75,8 @@ SP805Stop (
   )
 {
   // Disable interrupts
-  if ( (MmioRead32(SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) != 0 ) {
-    MmioAnd32(SP805_WDOG_CONTROL_REG, ~SP805_WDOG_CTRL_INTEN);
+  if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) != 0) {
+    MmioAnd32 (SP805_WDOG_CONTROL_REG, ~SP805_WDOG_CTRL_INTEN);
   }
 }
 
@@ -94,8 +92,8 @@ SP805Start (
   )
 {
   // Enable interrupts
-  if ( (MmioRead32(SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) == 0 ) {
-    MmioOr32(SP805_WDOG_CONTROL_REG, SP805_WDOG_CTRL_INTEN);
+  if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) == 0) {
+    MmioOr32 (SP805_WDOG_CONTROL_REG, SP805_WDOG_CTRL_INTEN);
   }
 }
 
@@ -103,6 +101,7 @@ SP805Start (
     On exiting boot services we must make sure the SP805 Watchdog Timer
     is stopped.
 **/
+STATIC
 VOID
 EFIAPI
 ExitBootServicesEvent (
@@ -110,9 +109,9 @@ ExitBootServicesEvent (
   IN VOID       *Context
   )
 {
-  SP805Unlock();
-  SP805Stop();
-  SP805Lock();
+  SP805Unlock ();
+  SP805Stop ();
+  SP805Lock ();
 }
 
 /**
@@ -142,10 +141,11 @@ ExitBootServicesEvent (
                                 previously registered.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 SP805RegisterHandler (
-  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
+  IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL         *This,
   IN EFI_WATCHDOG_TIMER_NOTIFY                NotifyFunction
   )
 {
@@ -182,22 +182,24 @@ SP805RegisterHandler (
   @retval EFI_DEVICE_ERROR      The timer period could not be changed due to a device error.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 SP805SetTimerPeriod (
-  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
+  IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL         *This,
   IN UINT64                                   TimerPeriod   // In 100ns units
   )
 {
-  EFI_STATUS  Status = EFI_SUCCESS;
+  EFI_STATUS  Status;
   UINT64      Ticks64bit;
 
-  SP805Unlock();
+  SP805Unlock ();
 
-  if( TimerPeriod == 0 ) {
+  Status = EFI_SUCCESS;
+
+  if (TimerPeriod == 0) {
     // This is a watchdog stop request
-    SP805Stop();
-    goto EXIT;
+    SP805Stop ();
   } else {
     // Calculate the Watchdog ticks required for a delay of (TimerTicks * 100) nanoseconds
     // The SP805 will count down to ZERO once, generate an interrupt and
@@ -211,10 +213,11 @@ SP805SetTimerPeriod (
     //
     // WatchdogTicks = (TimerPeriod * SP805_CLOCK_FREQUENCY) / 20 MHz ;
 
-    Ticks64bit = DivU64x32(MultU64x32(TimerPeriod, (UINTN)PcdGet32(PcdSP805WatchdogClockFrequencyInHz)), 20000000);
+    Ticks64bit = MultU64x32 (TimerPeriod, PcdGet32 (PcdSP805WatchdogClockFrequencyInHz));
+    Ticks64bit = DivU64x32 (Ticks64bit, 20000000);
 
     // The registers in the SP805 are only 32 bits
-    if(Ticks64bit > (UINT64)0xFFFFFFFF) {
+    if (Ticks64bit > MAX_UINT32) {
       // We could load the watchdog with the maximum supported value but
       // if a smaller value was requested, this could have the watchdog
       // triggering before it was intended.
@@ -224,15 +227,15 @@ SP805SetTimerPeriod (
     }
 
     // Update the watchdog with a 32-bit value.
-    MmioWrite32(SP805_WDOG_LOAD_REG, (UINT32)Ticks64bit);
+    MmioWrite32 (SP805_WDOG_LOAD_REG, (UINT32)Ticks64bit);
 
     // Start the watchdog
-    SP805Start();
+    SP805Start ();
   }
 
-  EXIT:
+EXIT:
   // Ensure the watchdog is locked before exiting.
-  SP805Lock();
+  SP805Lock ();
   return Status;
 }
 
@@ -251,14 +254,14 @@ SP805SetTimerPeriod (
   @retval EFI_INVALID_PARAMETER TimerPeriod is NULL.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 SP805GetTimerPeriod (
-  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
+  IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL         *This,
   OUT UINT64                                  *TimerPeriod
   )
 {
-  EFI_STATUS  Status = EFI_SUCCESS;
   UINT64      ReturnValue;
 
   if (TimerPeriod == NULL) {
@@ -266,19 +269,19 @@ SP805GetTimerPeriod (
   }
 
   // Check if the watchdog is stopped
-  if ( (MmioRead32(SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) == 0 ) {
+  if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) == 0) {
     // It is stopped, so return zero.
     ReturnValue = 0;
   } else {
     // Convert the Watchdog ticks into TimerPeriod
     // Ensure 64bit arithmetic throughout because the Watchdog ticks may already
     // be at the maximum 32 bit value and we still need to multiply that by 600.
-    ReturnValue = MultU64x32( MmioRead32(SP805_WDOG_LOAD_REG), 600 );
+    ReturnValue = MultU64x32 (MmioRead32 (SP805_WDOG_LOAD_REG), 600);
   }
 
   *TimerPeriod = ReturnValue;
 
-  return Status;
+  return EFI_SUCCESS;
 }
 
 /**
@@ -313,10 +316,10 @@ SP805GetTimerPeriod (
   Retrieves the period of the timer interrupt in 100 nS units.
 
 **/
-EFI_WATCHDOG_TIMER_ARCH_PROTOCOL    gWatchdogTimer = {
-  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER) SP805RegisterHandler,
-  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD) SP805SetTimerPeriod,
-  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD) SP805GetTimerPeriod
+STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL mWatchdogTimer = {
+  SP805RegisterHandler,
+  SP805SetTimerPeriod,
+  SP805GetTimerPeriod
 };
 
 /**
@@ -347,12 +350,12 @@ SP805Initialize (
   SP805Stop ();
 
   // Set the watchdog to reset the board when triggered
-  if ((MmioRead32(SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_RESEN) == 0) {
+  if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_RESEN) == 0) {
     MmioOr32 (SP805_WDOG_CONTROL_REG, SP805_WDOG_CTRL_RESEN);
   }
 
   // Prohibit any rogue access to SP805 registers
-  SP805Lock();
+  SP805Lock ();
 
   //
   // Make sure the Watchdog Timer Architectural Protocol has not been installed in the system yet.
@@ -361,28 +364,26 @@ SP805Initialize (
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiWatchdogTimerArchProtocolGuid);
 
   // Register for an ExitBootServicesEvent
-  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY, ExitBootServicesEvent, NULL, &EfiExitBootServicesEvent);
-  if (EFI_ERROR(Status)) {
+  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY,
+                  ExitBootServicesEvent, NULL, &mEfiExitBootServicesEvent);
+  if (EFI_ERROR (Status)) {
     Status = EFI_OUT_OF_RESOURCES;
     goto EXIT;
   }
 
   // Install the Timer Architectural Protocol onto a new handle
   Handle = NULL;
-  Status = gBS->InstallMultipleProtocolInterfaces(
+  Status = gBS->InstallMultipleProtocolInterfaces (
                   &Handle,
-                  &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
+                  &gEfiWatchdogTimerArchProtocolGuid, &mWatchdogTimer,
                   NULL
                   );
-  if (EFI_ERROR(Status)) {
+  if (EFI_ERROR (Status)) {
     Status = EFI_OUT_OF_RESOURCES;
     goto EXIT;
   }
 
 EXIT:
-  if(EFI_ERROR(Status)) {
-    // The watchdog failed to initialize
-    ASSERT(FALSE);
-  }
+  ASSERT_EFI_ERROR (Status);
   return Status;
 }
-- 
2.19.2

[PATCH v2 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode

[Ard Biesheuvel]

The SP805 watchdog driver doesn't implement the PI watchdog protocol
fully, but always simply resets the system if the watchdog time runs
out.

However, the hardware does support the intended usage model, as long
as the SP805 is wired up correctly. So let's implement interrupt based
mode involving a handler that is registered by the DXE core and invoked
when the watchdog runs out. In the interrupt handler, we invoke the
notify function if one was registered, or call the ResetSystem()
runtime service otherwise (as per the UEFI spec)

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmPlatformPkg.dec                            |   1 +
 ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf |   6 +-
 ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c      | 100 +++++++++++++++-----
 3 files changed, 80 insertions(+), 27 deletions(-)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index 5f67e7415469..44c00bd0c133 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -70,6 +70,7 @@ [PcdsFixedAtBuild.common]
   ## SP805 Watchdog
   gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase|0x0|UINT32|0x00000023
   gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz|32000|UINT32|0x00000021
+  gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt|0|UINT32|0x0000002E
 
   ## PL011 UART
   gArmPlatformTokenSpaceGuid.PL011UartClkInHz|24000000|UINT32|0x0000001F
diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
index c3971fb035d3..0e744deeca8d 100644
--- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
+++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
@@ -27,6 +27,7 @@ [Sources.common]
 [Packages]
   ArmPkg/ArmPkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
 
 [LibraryClasses]
@@ -35,13 +36,16 @@ [LibraryClasses]
   IoLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
+  UefiRuntimeServicesTableLib
 
 [Pcd]
   gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase
   gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz
+  gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt
 
 [Protocols]
+  gHardwareInterruptProtocolGuid          ## ALWAYS_CONSUMES
   gEfiWatchdogTimerArchProtocolGuid       ## ALWAYS_PRODUCES
 
 [Depex]
-  TRUE
+  gHardwareInterruptProtocolGuid
diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
index 12c2f0a1fe49..5bbb12af6019 100644
--- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
+++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
@@ -21,12 +21,17 @@
 #include <Library/DebugLib.h>
 #include <Library/IoLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
 
+#include <Protocol/HardwareInterrupt.h>
 #include <Protocol/WatchdogTimer.h>
 
 #include "SP805Watchdog.h"
 
-STATIC EFI_EVENT          mEfiExitBootServicesEvent;
+STATIC EFI_EVENT                        mEfiExitBootServicesEvent;
+STATIC EFI_HARDWARE_INTERRUPT_PROTOCOL  *mInterrupt;
+STATIC EFI_WATCHDOG_TIMER_NOTIFY        mWatchdogNotify;
+STATIC UINT32                           mTimerPeriod;
 
 /**
   Make sure the SP805 registers are unlocked for writing.
@@ -65,6 +70,33 @@ SP805Lock (
   }
 }
 
+STATIC
+VOID
+EFIAPI
+SP805InterruptHandler (
+  IN  HARDWARE_INTERRUPT_SOURCE   Source,
+  IN  EFI_SYSTEM_CONTEXT          SystemContext
+  )
+{
+  SP805Unlock ();
+  MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears the irq
+  SP805Lock ();
+
+  mInterrupt->EndOfInterrupt (mInterrupt, Source);
+
+  //
+  // 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) {
+    mWatchdogNotify (mTimerPeriod + 1);
+  }
+
+  gRT->ResetSystem (EfiResetCold, EFI_TIMEOUT, 0, NULL);
+}
+
 /**
   Stop the SP805 watchdog timer from counting down by disabling interrupts.
 **/
@@ -149,9 +181,16 @@ SP805RegisterHandler (
   IN EFI_WATCHDOG_TIMER_NOTIFY                NotifyFunction
   )
 {
-  // ERROR: This function is not supported.
-  // The hardware watchdog will reset the board
-  return EFI_INVALID_PARAMETER;
+  if (mWatchdogNotify == NULL && NotifyFunction == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (mWatchdogNotify != NULL && NotifyFunction != NULL) {
+    return EFI_ALREADY_STARTED;
+  }
+
+  mWatchdogNotify = NotifyFunction;
+  return EFI_SUCCESS;
 }
 
 /**
@@ -202,19 +241,16 @@ SP805SetTimerPeriod (
     SP805Stop ();
   } else {
     // Calculate the Watchdog ticks required for a delay of (TimerTicks * 100) nanoseconds
-    // The SP805 will count down to ZERO once, generate an interrupt and
-    // then it will again reload the initial value and start again.
-    // On the second time when it reaches ZERO, it will actually reset the board.
-    // Therefore, we need to load half the required delay.
+    // The SP805 will count down to zero and generate an interrupt.
     //
-    // WatchdogTicks = ((TimerPeriod * 100 * SP805_CLOCK_FREQUENCY) / 1GHz) / 2 ;
+    // WatchdogTicks = ((TimerPeriod * 100 * SP805_CLOCK_FREQUENCY) / 1GHz);
     //
     // i.e.:
     //
-    // WatchdogTicks = (TimerPeriod * SP805_CLOCK_FREQUENCY) / 20 MHz ;
+    // WatchdogTicks = (TimerPeriod * SP805_CLOCK_FREQUENCY) / 10 MHz ;
 
     Ticks64bit = MultU64x32 (TimerPeriod, PcdGet32 (PcdSP805WatchdogClockFrequencyInHz));
-    Ticks64bit = DivU64x32 (Ticks64bit, 20000000);
+    Ticks64bit = DivU64x32 (Ticks64bit, 10 * 1000 * 1000);
 
     // The registers in the SP805 are only 32 bits
     if (Ticks64bit > MAX_UINT32) {
@@ -233,9 +269,12 @@ SP805SetTimerPeriod (
     SP805Start ();
   }
 
+  mTimerPeriod = TimerPeriod;
+
 EXIT:
   // Ensure the watchdog is locked before exiting.
   SP805Lock ();
+  ASSERT_EFI_ERROR (Status);
   return Status;
 }
 
@@ -262,25 +301,11 @@ SP805GetTimerPeriod (
   OUT UINT64                                  *TimerPeriod
   )
 {
-  UINT64      ReturnValue;
-
   if (TimerPeriod == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  // Check if the watchdog is stopped
-  if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) == 0) {
-    // It is stopped, so return zero.
-    ReturnValue = 0;
-  } else {
-    // Convert the Watchdog ticks into TimerPeriod
-    // Ensure 64bit arithmetic throughout because the Watchdog ticks may already
-    // be at the maximum 32 bit value and we still need to multiply that by 600.
-    ReturnValue = MultU64x32 (MmioRead32 (SP805_WDOG_LOAD_REG), 600);
-  }
-
-  *TimerPeriod = ReturnValue;
-
+  *TimerPeriod = mTimerPeriod;
   return EFI_SUCCESS;
 }
 
@@ -343,6 +368,11 @@ SP805Initialize (
   EFI_STATUS  Status;
   EFI_HANDLE  Handle;
 
+  // Find the interrupt controller protocol.  ASSERT if not found.
+  Status = gBS->LocateProtocol (&gHardwareInterruptProtocolGuid, NULL,
+                  (VOID **)&mInterrupt);
+  ASSERT_EFI_ERROR (Status);
+
   // Unlock access to the SP805 registers
   SP805Unlock ();
 
@@ -350,13 +380,31 @@ SP805Initialize (
   SP805Stop ();
 
   // Set the watchdog to reset the board when triggered
+  // This is a last resort in case the interrupt handler fails
   if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_RESEN) == 0) {
     MmioOr32 (SP805_WDOG_CONTROL_REG, SP805_WDOG_CTRL_RESEN);
   }
 
+  // Clear any pending interrupts
+  MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears the irq
+
   // Prohibit any rogue access to SP805 registers
   SP805Lock ();
 
+  if (PcdGet32 (PcdSP805WatchdogInterrupt) > 0) {
+    Status = mInterrupt->RegisterInterruptSource (mInterrupt,
+                           PcdGet32 (PcdSP805WatchdogInterrupt),
+                           SP805InterruptHandler);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: failed to register watchdog interrupt - %r\n",
+        __FUNCTION__, Status));
+      return Status;
+    }
+  } else {
+    DEBUG ((DEBUG_WARN, "%a: no interrupt specified, running in RESET mode only\n",
+      __FUNCTION__));
+  }
+
   //
   // Make sure the Watchdog Timer Architectural Protocol has not been installed in the system yet.
   // This will avoid conflicts with the universal watchdog
-- 
2.19.2

[PATCH v2 3/4] ArmPkg/GenericWatchdogDxe: clean up the code

[Ard Biesheuvel]

Clean up the code, by adding missing STATIC modifiers, drop
redundant casts, and get rid of the 'success handling' anti
pattern in the entry point code.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |   9 +-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 109 +++++++++++---------
 2 files changed, 62 insertions(+), 56 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
index ba0403d7fdc3..171bf5b9e183 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
@@ -16,17 +16,16 @@ [Defines]
   FILE_GUID                      = 0619f5c2-4858-4caa-a86a-73a21a18df6b
   MODULE_TYPE                    = DXE_DRIVER
   VERSION_STRING                 = 1.0
-
   ENTRY_POINT                    = GenericWatchdogEntry
 
 [Sources.common]
   GenericWatchdogDxe.c
 
 [Packages]
-  MdePkg/MdePkg.dec
-  EmbeddedPkg/EmbeddedPkg.dec
   ArmPkg/ArmPkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
+  MdePkg/MdePkg.dec
 
 [LibraryClasses]
   ArmGenericTimerCounterLib
@@ -46,8 +45,8 @@ [Pcd.common]
   gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum
 
 [Protocols]
-  gEfiWatchdogTimerArchProtocolGuid
-  gHardwareInterrupt2ProtocolGuid
+  gEfiWatchdogTimerArchProtocolGuid       ## ALWAYS_PRODUCES
+  gHardwareInterrupt2ProtocolGuid         ## ALWAYS_CONSUMES
 
 [Depex]
   gHardwareInterrupt2ProtocolGuid
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 8ccf15366dfa..285727fc0e84 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -34,15 +34,16 @@
 #define TIME_UNITS_PER_SECOND 10000000
 
 // Tick frequency of the generic timer basis of the generic watchdog.
-UINTN mTimerFrequencyHz = 0;
+STATIC UINTN mTimerFrequencyHz = 0;
 
 /* In cases where the compare register was set manually, information about
    how long the watchdog was asked to wait cannot be retrieved from hardware.
    It is therefore stored here. 0 means the timer is not running. */
-UINT64 mNumTimerTicks = 0;
+STATIC UINT64 mNumTimerTicks = 0;
 
-EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
+STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
 
+STATIC
 VOID
 WatchdogWriteOffsetRegister (
   UINT32  Value
@@ -51,6 +52,7 @@ WatchdogWriteOffsetRegister (
   MmioWrite32 (GENERIC_WDOG_OFFSET_REG, Value);
 }
 
+STATIC
 VOID
 WatchdogWriteCompareRegister (
   UINT64  Value
@@ -60,6 +62,7 @@ WatchdogWriteCompareRegister (
   MmioWrite32 (GENERIC_WDOG_COMPARE_VALUE_REG_HIGH, (Value >> 32) & MAX_UINT32);
 }
 
+STATIC
 VOID
 WatchdogEnable (
   VOID
@@ -68,6 +71,7 @@ WatchdogEnable (
   MmioWrite32 (GENERIC_WDOG_CONTROL_STATUS_REG, GENERIC_WDOG_ENABLED);
 }
 
+STATIC
 VOID
 WatchdogDisable (
   VOID
@@ -79,6 +83,7 @@ WatchdogDisable (
 /** On exiting boot services we must make sure the Watchdog Timer
     is stopped.
 **/
+STATIC
 VOID
 EFIAPI
 WatchdogExitBootServicesEvent (
@@ -93,6 +98,7 @@ WatchdogExitBootServicesEvent (
 /* This function is called when the watchdog's first signal (WS0) goes high.
    It uses the ResetSystem Runtime Service to reset the board.
 */
+STATIC
 VOID
 EFIAPI
 WatchdogInterruptHandler (
@@ -141,10 +147,11 @@ WatchdogInterruptHandler (
   @retval EFI_UNSUPPORTED       The code does not support NotifyFunction.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 WatchdogRegisterHandler (
-  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
+  IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL         *This,
   IN EFI_WATCHDOG_TIMER_NOTIFY                NotifyFunction
   )
 {
@@ -167,10 +174,11 @@ WatchdogRegisterHandler (
                                 in TimerPeriod 100ns units.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 WatchdogSetTimerPeriod (
-  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
+  IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL         *This,
   IN UINT64                                   TimerPeriod   // In 100ns units
   )
 {
@@ -222,10 +230,11 @@ WatchdogSetTimerPeriod (
   @retval EFI_INVALID_PARAMETER TimerPeriod is NULL.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 WatchdogGetTimerPeriod (
-  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
+  IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL         *This,
   OUT UINT64                                  *TimerPeriod
   )
 {
@@ -270,13 +279,13 @@ WatchdogGetTimerPeriod (
   Retrieves the period of the timer interrupt in 100ns units.
 
 **/
-EFI_WATCHDOG_TIMER_ARCH_PROTOCOL    gWatchdogTimer = {
-  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER)WatchdogRegisterHandler,
-  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD)WatchdogSetTimerPeriod,
-  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD)WatchdogGetTimerPeriod
+STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL mWatchdogTimer = {
+  WatchdogRegisterHandler,
+  WatchdogSetTimerPeriod,
+  WatchdogGetTimerPeriod
 };
 
-EFI_EVENT                           EfiExitBootServicesEvent = (EFI_EVENT)NULL;
+STATIC EFI_EVENT mEfiExitBootServicesEvent;
 
 EFI_STATUS
 EFIAPI
@@ -288,6 +297,10 @@ GenericWatchdogEntry (
   EFI_STATUS                      Status;
   EFI_HANDLE                      Handle;
 
+  Status = gBS->LocateProtocol (&gHardwareInterrupt2ProtocolGuid, NULL,
+                  (VOID **)&mInterruptProtocol);
+  ASSERT_EFI_ERROR (Status);
+
   /* Make sure the Watchdog Timer Architectural Protocol has not been installed
      in the system yet.
      This will avoid conflicts with the universal watchdog */
@@ -296,51 +309,45 @@ GenericWatchdogEntry (
   mTimerFrequencyHz = ArmGenericTimerGetTimerFreq ();
   ASSERT (mTimerFrequencyHz != 0);
 
+  // Install interrupt handler
+  Status = mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol,
+                                 FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+                                 WatchdogInterruptHandler);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = mInterruptProtocol->SetTriggerType (mInterruptProtocol,
+                                 FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+                                 EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
+  if (EFI_ERROR (Status)) {
+    goto UnregisterHandler;
+  }
+
+  // Install the Timer Architectural Protocol onto a new handle
+  Handle = NULL;
+  Status = gBS->InstallMultipleProtocolInterfaces (&Handle,
+                  &gEfiWatchdogTimerArchProtocolGuid, &mWatchdogTimer,
+                  NULL);
+  if (EFI_ERROR (Status)) {
+    goto UnregisterHandler;
+  }
+
   // Register for an ExitBootServicesEvent
-  Status = gBS->CreateEvent (
-                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
-                  TPL_NOTIFY,
-                  WatchdogExitBootServicesEvent,
-                  NULL,
-                  &EfiExitBootServicesEvent
-                  );
-  if (!EFI_ERROR (Status)) {
-    // Install interrupt handler
-    Status = gBS->LocateProtocol (
-                    &gHardwareInterrupt2ProtocolGuid,
-                    NULL,
-                    (VOID **)&mInterruptProtocol
-                    );
-    if (!EFI_ERROR (Status)) {
-      Status = mInterruptProtocol->RegisterInterruptSource (
-                 mInterruptProtocol,
-                 FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
-                 WatchdogInterruptHandler
-                 );
-      if (!EFI_ERROR (Status)) {
-        Status = mInterruptProtocol->SetTriggerType (
-                   mInterruptProtocol,
-                   FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
-                   EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
-                   );
-        if (!EFI_ERROR (Status)) {
-          // Install the Timer Architectural Protocol onto a new handle
-          Handle = NULL;
-          Status = gBS->InstallMultipleProtocolInterfaces (
-                          &Handle,
-                          &gEfiWatchdogTimerArchProtocolGuid,
-                          &gWatchdogTimer,
-                          NULL
-                          );
-        }
-      }
-    }
-  }
-
+  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY,
+                  WatchdogExitBootServicesEvent, NULL,
+                  &mEfiExitBootServicesEvent);
   ASSERT_EFI_ERROR (Status);
 
   mNumTimerTicks = 0;
   WatchdogDisable ();
 
+  return EFI_SUCCESS;
+
+UnregisterHandler:
+  // Unregister the handler
+  mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol,
+                        FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+                        NULL);
   return Status;
 }
-- 
2.19.2

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

[Ard Biesheuvel]

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>
Reviewed-by: Leif Lindholm <leif.lindholm@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 285727fc0e84..a1ef0363eb39 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);
+  }
+
+  gRT->ResetSystem (EfiResetCold, EFI_TIMEOUT, StrSize (ResetString),
+         (CHAR16 *)ResetString);
 
   // 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.19.2

Re: [PATCH v2 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode

[Leif Lindholm]

On Wed, Dec 19, 2018 at 09:40:21PM +0100, Ard Biesheuvel wrote:
> The SP805 watchdog driver doesn't implement the PI watchdog protocol
> fully, but always simply resets the system if the watchdog time runs
> out.
> 
> However, the hardware does support the intended usage model, as long
> as the SP805 is wired up correctly. So let's implement interrupt based
> mode involving a handler that is registered by the DXE core and invoked
> when the watchdog runs out. In the interrupt handler, we invoke the
> notify function if one was registered, or call the ResetSystem()
> runtime service otherwise (as per the UEFI spec)
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPlatformPkg/ArmPlatformPkg.dec                            |   1 +
>  ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf |   6 +-
>  ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c      | 100 +++++++++++++++-----
>  3 files changed, 80 insertions(+), 27 deletions(-)
> 
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 5f67e7415469..44c00bd0c133 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -70,6 +70,7 @@ [PcdsFixedAtBuild.common]
>    ## SP805 Watchdog
>    gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase|0x0|UINT32|0x00000023
>    gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz|32000|UINT32|0x00000021
> +  gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt|0|UINT32|0x0000002E
>  
>    ## PL011 UART
>    gArmPlatformTokenSpaceGuid.PL011UartClkInHz|24000000|UINT32|0x0000001F
> diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> index c3971fb035d3..0e744deeca8d 100644
> --- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> +++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> @@ -27,6 +27,7 @@ [Sources.common]
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmPlatformPkg/ArmPlatformPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
>    MdePkg/MdePkg.dec
>  
>  [LibraryClasses]
> @@ -35,13 +36,16 @@ [LibraryClasses]
>    IoLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> +  UefiRuntimeServicesTableLib
>  
>  [Pcd]
>    gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase
>    gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz
> +  gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt
>  
>  [Protocols]
> +  gHardwareInterruptProtocolGuid          ## ALWAYS_CONSUMES
>    gEfiWatchdogTimerArchProtocolGuid       ## ALWAYS_PRODUCES
>  
>  [Depex]
> -  TRUE
> +  gHardwareInterruptProtocolGuid
> diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> index 12c2f0a1fe49..5bbb12af6019 100644
> --- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> +++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> @@ -21,12 +21,17 @@
>  #include <Library/DebugLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
>  
> +#include <Protocol/HardwareInterrupt.h>
>  #include <Protocol/WatchdogTimer.h>
>  
>  #include "SP805Watchdog.h"
>  
> -STATIC EFI_EVENT          mEfiExitBootServicesEvent;
> +STATIC EFI_EVENT                        mEfiExitBootServicesEvent;
> +STATIC EFI_HARDWARE_INTERRUPT_PROTOCOL  *mInterrupt;
> +STATIC EFI_WATCHDOG_TIMER_NOTIFY        mWatchdogNotify;
> +STATIC UINT32                           mTimerPeriod;
>  
>  /**
>    Make sure the SP805 registers are unlocked for writing.
> @@ -65,6 +70,33 @@ SP805Lock (
>    }
>  }
>  
> +STATIC
> +VOID
> +EFIAPI
> +SP805InterruptHandler (
> +  IN  HARDWARE_INTERRUPT_SOURCE   Source,
> +  IN  EFI_SYSTEM_CONTEXT          SystemContext
> +  )
> +{
> +  SP805Unlock ();
> +  MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears the irq
> +  SP805Lock ();
> +
> +  mInterrupt->EndOfInterrupt (mInterrupt, Source);
> +
> +  //
> +  // 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) {
> +    mWatchdogNotify (mTimerPeriod + 1);
> +  }
> +
> +  gRT->ResetSystem (EfiResetCold, EFI_TIMEOUT, 0, NULL);
> +}
> +
>  /**
>    Stop the SP805 watchdog timer from counting down by disabling interrupts.
>  **/
> @@ -149,9 +181,16 @@ SP805RegisterHandler (
>    IN EFI_WATCHDOG_TIMER_NOTIFY                NotifyFunction
>    )
>  {
> -  // ERROR: This function is not supported.
> -  // The hardware watchdog will reset the board
> -  return EFI_INVALID_PARAMETER;
> +  if (mWatchdogNotify == NULL && NotifyFunction == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (mWatchdogNotify != NULL && NotifyFunction != NULL) {
> +    return EFI_ALREADY_STARTED;
> +  }
> +
> +  mWatchdogNotify = NotifyFunction;
> +  return EFI_SUCCESS;
>  }
>  
>  /**
> @@ -202,19 +241,16 @@ SP805SetTimerPeriod (
>      SP805Stop ();
>    } else {
>      // Calculate the Watchdog ticks required for a delay of (TimerTicks * 100) nanoseconds
> -    // The SP805 will count down to ZERO once, generate an interrupt and
> -    // then it will again reload the initial value and start again.
> -    // On the second time when it reaches ZERO, it will actually reset the board.
> -    // Therefore, we need to load half the required delay.
> +    // The SP805 will count down to zero and generate an interrupt.
>      //
> -    // WatchdogTicks = ((TimerPeriod * 100 * SP805_CLOCK_FREQUENCY) / 1GHz) / 2 ;
> +    // WatchdogTicks = ((TimerPeriod * 100 * SP805_CLOCK_FREQUENCY) / 1GHz);
>      //
>      // i.e.:
>      //
> -    // WatchdogTicks = (TimerPeriod * SP805_CLOCK_FREQUENCY) / 20 MHz ;
> +    // WatchdogTicks = (TimerPeriod * SP805_CLOCK_FREQUENCY) / 10 MHz ;
>  
>      Ticks64bit = MultU64x32 (TimerPeriod, PcdGet32 (PcdSP805WatchdogClockFrequencyInHz));
> -    Ticks64bit = DivU64x32 (Ticks64bit, 20000000);
> +    Ticks64bit = DivU64x32 (Ticks64bit, 10 * 1000 * 1000);
>  
>      // The registers in the SP805 are only 32 bits
>      if (Ticks64bit > MAX_UINT32) {
> @@ -233,9 +269,12 @@ SP805SetTimerPeriod (
>      SP805Start ();
>    }
>  
> +  mTimerPeriod = TimerPeriod;
> +
>  EXIT:
>    // Ensure the watchdog is locked before exiting.
>    SP805Lock ();
> +  ASSERT_EFI_ERROR (Status);
>    return Status;
>  }
>  
> @@ -262,25 +301,11 @@ SP805GetTimerPeriod (
>    OUT UINT64                                  *TimerPeriod
>    )
>  {
> -  UINT64      ReturnValue;
> -
>    if (TimerPeriod == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -  // Check if the watchdog is stopped
> -  if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) == 0) {
> -    // It is stopped, so return zero.
> -    ReturnValue = 0;
> -  } else {
> -    // Convert the Watchdog ticks into TimerPeriod
> -    // Ensure 64bit arithmetic throughout because the Watchdog ticks may already
> -    // be at the maximum 32 bit value and we still need to multiply that by 600.
> -    ReturnValue = MultU64x32 (MmioRead32 (SP805_WDOG_LOAD_REG), 600);
> -  }
> -
> -  *TimerPeriod = ReturnValue;
> -
> +  *TimerPeriod = mTimerPeriod;
>    return EFI_SUCCESS;
>  }
>  
> @@ -343,6 +368,11 @@ SP805Initialize (
>    EFI_STATUS  Status;
>    EFI_HANDLE  Handle;
>  
> +  // Find the interrupt controller protocol.  ASSERT if not found.
> +  Status = gBS->LocateProtocol (&gHardwareInterruptProtocolGuid, NULL,
> +                  (VOID **)&mInterrupt);
> +  ASSERT_EFI_ERROR (Status);
> +
>    // Unlock access to the SP805 registers
>    SP805Unlock ();
>  
> @@ -350,13 +380,31 @@ SP805Initialize (
>    SP805Stop ();
>  
>    // Set the watchdog to reset the board when triggered
> +  // This is a last resort in case the interrupt handler fails
>    if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_RESEN) == 0) {
>      MmioOr32 (SP805_WDOG_CONTROL_REG, SP805_WDOG_CTRL_RESEN);
>    }
>  
> +  // Clear any pending interrupts
> +  MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears the irq
> +
>    // Prohibit any rogue access to SP805 registers
>    SP805Lock ();
>  
> +  if (PcdGet32 (PcdSP805WatchdogInterrupt) > 0) {
> +    Status = mInterrupt->RegisterInterruptSource (mInterrupt,
> +                           PcdGet32 (PcdSP805WatchdogInterrupt),
> +                           SP805InterruptHandler);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: failed to register watchdog interrupt - %r\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +  } else {
> +    DEBUG ((DEBUG_WARN, "%a: no interrupt specified, running in RESET mode only\n",
> +      __FUNCTION__));
> +  }
> +
>    //
>    // Make sure the Watchdog Timer Architectural Protocol has not been installed in the system yet.
>    // This will avoid conflicts with the universal watchdog
> -- 
> 2.19.2
> 

Re: [PATCH v2 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup

[Ard Biesheuvel]

On Wed, 19 Dec 2018 at 21:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> This series cleans up the code of the two watchdog drivers we have for
> ARM systems, and brings them in compliance with the PI spec, which
> specifies that the default action of the watchdog can be overridden
> by registering a handler.
>
> Note that the TC2 code in edk2-platforms will have to be brought up to
> date. The SP805 on the FVP model seems terminally broken (it is 'wired'
> to the 24 MHz APB clock instead of the 32 kHz WDOG clock, so I'll switch
> that one over to use the SBSA watchdog instead)
>
> Changes since v1:
> - always fall back to calling gRT_>ResetSystem() if the registered handler
>   returns
> - WARN() if running the SP805 driver with interrupt handling disabled
> - add some R-bs
>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: Udit Kumar <udit.kumar@nxp.com>
> Cc: Matteo Carlini <Matteo.Carlini@arm.com>
> Cc: Nariman Poushin <nariman.poushin@linaro.org>
>
> Ard Biesheuvel (4):
>   ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup
>   ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
>   ArmPkg/GenericWatchdogDxe: clean up the code
>   ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method
>

Pushed as  87b920fe22be..ba808d11f6a3

Thanks all