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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 18 Dec 2018 at 16:58, Udit Kumar <udit.kumar@nxp.com> wrote:
>
>
>
> > -----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 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt
> > mode
> >
> > 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)
>
> Specs, says
> If no handler has been registered, or the registered handler returns, then the system will be reset by calling the Runtime Service ResetSystem().
> My understanding is that we have to ResetSystem() always irrespective of notify function
>

Indeed. Thanks for spotting that.

>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmPlatformPkg/ArmPlatformPkg.dec                            |  1 +
> >  ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c      | 95
> > ++++++++++++++------
> >  ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf |  6 +-
> >  3 files changed, 75 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 @@
> >    ## SP805 Watchdog
> >
> > gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase|0x0|UINT32|0x000000
> > 23
> >
> > gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz|32000|
> > UINT32|0x00000021
> > +
> > +
> > gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt|0|UINT32|0x0000
> > 00
> > + 2E
> >
> >    ## PL011 UART
> >
> > gArmPlatformTokenSpaceGuid.PL011UartClkInHz|24000000|UINT32|0x000000
> > 1F
> > diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> > b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> > index 12c2f0a1fe49..4f09acf1fa28 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
> > +  )
> > +{
> > +  //
> > +  // 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);
> > +  } else {
> > +    gRT->ResetSystem (EfiResetCold, EFI_TIMEOUT, 0, NULL);
> > +  }
>
> Shouldn't we ResetSystem in all cases.
>
>
> > +
> > +  SP805Unlock ();
> > +  MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears
> > + the irq  SP805Lock ();
> > +
> > +  mInterrupt->EndOfInterrupt (mInterrupt, Source); }
> > +
>
> IMO, this routine should be
> 1/ Handle IT
> 2/ Do callback mWatchdogNotify
> 3/ Reset the system.
>
> >  /**
> >    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,26 @@ 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 ();
> >
> > +  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;
> > +  }
> > +
> >    //
> >    // Make sure the Watchdog Timer Architectural Protocol has not been installed
> > in the system yet.
> >    // This will avoid conflicts with the universal watchdog diff --git
> > a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> > b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> > index 99ecab477567..1373e267612f 100644
> > --- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> > +++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> > @@ -27,6 +27,7 @@
> >  [Packages]
> >    ArmPlatformPkg/ArmPlatformPkg.dec
> >    ArmPkg/ArmPkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> >    MdePkg/MdePkg.dec
> >
> >  [LibraryClasses]
> > @@ -35,13 +36,16 @@
> >    IoLib
> >    UefiBootServicesTableLib
> >    UefiDriverEntryPoint
> > +  UefiRuntimeServicesTableLib
> >
> >  [Pcd]
> >    gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase
> >    gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz
> > +  gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt
> >
> >  [Protocols]
> > +  gHardwareInterruptProtocolGuid          ## ALWAYS_CONSUMES
> >    gEfiWatchdogTimerArchProtocolGuid       ## ALWAYS_PRODUCES
> >
> >  [Depex]
> > -  TRUE
> > +  gHardwareInterruptProtocolGuid
> > --
> > 2.17.1
>