Re: [PATCH 04/10] Platform/NXP : Add support for Watchdog driver

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

On 7 November 2017 at 14:42, Meenakshi Aggarwal
<meenakshi.aggarwal@nxp.com> wrote:
> Installs watchdog timer arch protocol
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> ---
>  Platform/NXP/Drivers/WatchDog/WatchDog.c      | 386 ++++++++++++++++++++++++++
>  Platform/NXP/Drivers/WatchDog/WatchDog.h      |  37 +++
>  Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf |  47 ++++
>  3 files changed, 470 insertions(+)
>  create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDog.c
>  create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDog.h
>  create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf
>
> diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> new file mode 100644
> index 0000000..f257a1d
> --- /dev/null
> +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> @@ -0,0 +1,386 @@
> +/** WatchDog.c
> +*
> +*  Based on Watchdog driver implemenation available in
> +*  ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> +*
> +*  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +*  Copyright 2017 NXP
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD License
> +*  which accompanies this distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#include <PiDxe.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BeIoLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/WatchdogTimer.h>
> +
> +#include "WatchDog.h"
> +

STATIC please, and drop the redundant initializer.

> +EFI_EVENT  EfiExitBootServicesEvent = (EFI_EVENT)NULL;
> +

STATIC

In general, any function that is not exported from the driver should
be STATIC, even if it is referenced from a protocol struct

> +UINT16
> +EFIAPI
> +WdogRead (
> +  IN  UINTN     Address
> +  )
> +{
> +  if (FixedPcdGetBool(PcdWdogBigEndian)) {

In general, but I will mention it here: every function call or macro
invocation requires a space before (

> +    return BeMmioRead16(Address);
> +  } else {
> +    return MmioRead16(Address);
> +  }
> +}
> +
> +UINT16
> +EFIAPI
> +WdogWrite (
> +  IN  UINTN     Address,
> +  IN  UINT16    Value
> +  )
> +{
> +  if (FixedPcdGetBool(PcdWdogBigEndian)) {
> +    return BeMmioWrite16(Address, Value);
> +  } else {
> +    return MmioWrite16(Address, Value);
> +  }
> +}
> +
> +STATIC
> +VOID
> +WdogPing (
> +  VOID
> +  )
> +{
> +  WdogWrite(PcdGet64(PcdWdog1BaseAddr) + WDOG_WSR_OFFSET,
> +                     WDOG_SERVICE_SEQ1);
> +  WdogWrite(PcdGet64(PcdWdog1BaseAddr) + WDOG_WSR_OFFSET,
> +                     WDOG_SERVICE_SEQ2);
> +}
> +
> +/**
> +  Stop the Wdog watchdog timer from counting down.
> +**/
> +STATIC
> +VOID
> +WdogStop (
> +  VOID
> +  )
> +{
> +  //  Watchdog cannot be disabled by software once started.
> +  // At best, we can keep pinging the watchdog
> +  WdogPing();

How is this supposed to work when entering the OS?

> +}
> +
> +/**
> +  Starts the Wdog counting down by feeding Service register with
> +  desired pattern.
> +  The count down will start from the value stored in the Load register,
> +  not from the value where it was previously stopped.
> +**/
> +STATIC
> +VOID
> +WdogStart (
> +  VOID
> +  )
> +{
> +  /* Reload the timeout value */
> +  WdogPing();
> +}
> +
> +/**
> +    On exiting boot services we must make sure the Wdog Watchdog Timer
> +    is stopped.
> +**/
> +VOID
> +EFIAPI
> +ExitBootServicesEvent (
> +  IN EFI_EVENT  Event,
> +  IN VOID       *Context
> +  )
> +{
> +  WdogStop();

The comment above says this is impossible.

> +}
> +
> +/**
> +  This function registers the handler NotifyFunction so it is called every time
> +  the watchdog timer expires.  It also passes the amount of time since the last
> +  handler call to the NotifyFunction.
> +  If NotifyFunction is not NULL and a handler is not already registered,
> +  then the new handler is registered and EFI_SUCCESS is returned.
> +  If NotifyFunction is NULL, and a handler is already registered,
> +  then that handler is unregistered.
> +  If an attempt is made to register a handler when a handler is already registered,
> +  then EFI_ALREADY_STARTED is returned.
> +  If an attempt is made to unregister a handler when a handler is not registered,
> +  then EFI_INVALID_PARAMETER is returned.
> +
> +  @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
> +  @param  NotifyFunction   The function to call when a timer interrupt fires. This
> +                           function executes at TPL_HIGH_LEVEL. The DXE Core will
> +                           register a handler for the timer interrupt, so it can know
> +                           how much time has passed. This information is used to
> +                           signal timer based events. NULL will unregister the handler.
> +
> +  @retval EFI_SUCCESS           The watchdog timer handler was registered.
> +  @retval EFI_ALREADY_STARTED   NotifyFunction is not NULL, and a handler is already
> +                                registered.
> +  @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler was not
> +                                previously registered.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +WdogRegisterHandler (
> +  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
> +  IN EFI_WATCHDOG_TIMER_NOTIFY                NotifyFunction
> +  )
> +{
> +  // ERROR: This function is not supported.
> +  // The hardware watchdog will reset the board
> +  return EFI_INVALID_PARAMETER;

Doesn't this violate the protocol? Why does it make sense to implement
the protocol based on this hardware?

> +}
> +
> +/**
> +
> +  This function adjusts the period of timer interrupts to the value specified
> +  by TimerPeriod.  If the timer period is updated, then the selected timer
> +  period is stored in EFI_TIMER.TimerPeriod, and EFI_SUCCESS is returned.  If
> +  the timer hardware is not programmable, then EFI_UNSUPPORTED is returned.
> +  If an error occurs while attempting to update the timer period, then the
> +  timer hardware will be put back in its state prior to this call, and
> +  EFI_DEVICE_ERROR is returned.  If TimerPeriod is 0, then the timer interrupt
> +  is disabled.  This is not the same as disabling the CPU's interrupts.
> +  Instead, it must either turn off the timer hardware, or it must adjust the
> +  interrupt controller so that a CPU interrupt is not generated when the timer
> +  interrupt fires.
> +
> +  @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
> +  @param  TimerPeriod      The rate to program the timer interrupt in 100 nS units. If
> +                           the timer hardware is not programmable, then EFI_UNSUPPORTED is
> +                           returned. If the timer is programmable, then the timer period
> +                           will be rounded up to the nearest timer period that is supported
> +                           by the timer hardware. If TimerPeriod is set to 0, then the
> +                           timer interrupts will be disabled.
> +
> +
> +  @retval EFI_SUCCESS           The timer period was changed.
> +  @retval EFI_UNSUPPORTED       The platform cannot change the period of the timer interrupt.
> +  @retval EFI_DEVICE_ERROR      The timer period could not be changed due to a device error.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +WdogSetTimerPeriod (
> +  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
> +  IN UINT64                                   TimerPeriod   // In 100ns units
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT64      TimerPeriodInSec;
> +  UINT16      Val;
> +
> +  Status = EFI_SUCCESS;
> +
> +  if (TimerPeriod == 0) {
> +    // This is a watchdog stop request
> +    WdogStop();
> +    goto EXIT;
> +  } else {
> +    // Convert the TimerPeriod (in 100 ns unit) to an equivalent second value
> +
> +    TimerPeriodInSec = DivU64x32(TimerPeriod, NANO_SECOND_BASE);
> +
> +    // The registers in the Wdog are only 32 bits
> +    if(TimerPeriodInSec > WT_MAX_TIME) {
> +      // 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.
> +      // Better generate an error to let the caller know.
> +      Status = EFI_DEVICE_ERROR;
> +      goto EXIT;
> +    }
> +
> +    // set the new timeout value in the WCR
> +    Val = WdogRead(PcdGet64(PcdWdog1BaseAddr) + WDOG_WCR_OFFSET);
> +    Val &= ~WDOG_WCR_WT;
> +    // Convert the timeout value from Seconds to timer count
> +    Val |= ((WD_COUNT(TimerPeriodInSec) & WD_COUNT_MASK) << 8);
> +    WdogWrite(PcdGet64(PcdWdog1BaseAddr) + WDOG_WCR_OFFSET, Val);
> +
> +    // Start the watchdog
> +    WdogStart();
> +  }
> +
> +  EXIT:
> +  return Status;
> +}
> +
> +/**
> +  This function retrieves the period of timer interrupts in 100 ns units,
> +  returns that value in TimerPeriod, and returns EFI_SUCCESS.  If TimerPeriod
> +  is NULL, then EFI_INVALID_PARAMETER is returned.  If a TimerPeriod of 0 is
> +  returned, then the timer is currently disabled.
> +
> +  @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
> +  @param  TimerPeriod      A pointer to the timer period to retrieve in 100 ns units. If
> +                           0 is returned, then the timer is currently disabled.
> +
> +
> +  @retval EFI_SUCCESS           The timer period was returned in TimerPeriod.
> +  @retval EFI_INVALID_PARAMETER TimerPeriod is NULL.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +WdogGetTimerPeriod (
> +  IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL   *This,
> +  OUT UINT64                                  *TimerPeriod
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT64      ReturnValue;
> +  UINT16      Val;
> +
> +  Status = EFI_SUCCESS;
> +
> +  if (TimerPeriod == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  // Check if the watchdog is stopped
> +  if ((WdogRead(PcdGet64(PcdWdog1BaseAddr) + WDOG_WCR_OFFSET)
> +              & WDOG_WCR_WDE) == 0 ) {
> +    // It is stopped, so return zero.
> +    ReturnValue = 0;
> +  } else {
> +    // Convert the Watchdog ticks into equivalent TimerPeriod second value.
> +    Val = (WdogRead(PcdGet64(PcdWdog1BaseAddr) + WDOG_WCR_OFFSET)
> +            & WDOG_WCR_WT ) >> 8;
> +    ReturnValue = WD_SEC(Val);
> +  }
> +
> +  *TimerPeriod = ReturnValue;
> +  return Status;
> +}
> +
> +/**
> +  Interface structure for the Watchdog Architectural Protocol.
> +
> +  @par Protocol Description:
> +  This protocol provides a service to set the amount of time to wait
> +  before firing the watchdog timer, and it also provides a service to
> +  register a handler that is invoked when the watchdog timer fires.
> +
> +  @par When the watchdog timer fires, control will be passed to a handler
> +  if one has been registered.  If no handler has been registered,
> +  or the registered handler returns, then the system will be
> +  reset by calling the Runtime Service ResetSystem().
> +
> +  @param RegisterHandler
> +  Registers a handler that will be called each time the
> +  watchdogtimer interrupt fires.  TimerPeriod defines the minimum
> +  time between timer interrupts, so TimerPeriod will also
> +  be the minimum time between calls to the registered
> +  handler.
> +  NOTE: If the watchdog resets the system in hardware, then
> +        this function will not have any chance of executing.
> +
> +  @param SetTimerPeriod
> +  Sets the period of the timer interrupt in 100 nS units.
> +  This function is optional, and may return EFI_UNSUPPORTED.
> +  If this function is supported, then the timer period will
> +  be rounded up to the nearest supported timer period.
> +
> +  @param GetTimerPeriod
> +  Retrieves the period of the timer interrupt in 100 nS units.
> +
> +**/
> +EFI_WATCHDOG_TIMER_ARCH_PROTOCOL    gWatchdogTimer = {

STATIC please


> +  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER) WdogRegisterHandler,
> +  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD) WdogSetTimerPeriod,
> +  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD) WdogGetTimerPeriod

The () casts shouldn't be necessary.

> +};
> +
> +/**
> +  Initialize state information for the Watchdog Timer Architectural Protocol.
> +
> +  @param  ImageHandle   of the loaded driver
> +  @param  SystemTable   Pointer to the System Table
> +
> +  @retval EFI_SUCCESS           Protocol registered
> +  @retval EFI_OUT_OF_RESOURCES  Cannot allocate protocol data structure
> +  @retval EFI_DEVICE_ERROR      Hardware problems
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +WdogInitialize (
> +  IN EFI_HANDLE         ImageHandle,
> +  IN EFI_SYSTEM_TABLE   *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EFI_HANDLE  Handle;
> +  UINT16      Val;
> +
> +
> +  Val = WdogRead(PcdGet64(PcdWdog1BaseAddr) + WDOG_WCR_OFFSET);
> +
> +  Val &= ~WDOG_WCR_WT;
> +
> +  Val &= ~WDOG_WCR_WDE;
> +
> +  Val |= WD_COUNT(WT_MAX_TIME) & WD_COUNT_MASK;
> +
> +  WdogWrite(PcdGet64(PcdWdog1BaseAddr) + WDOG_WCR_OFFSET, Val);
> +

Can you use MmioAndThenOr16 () here?

> +  Val |= WDOG_WCR_WDE;
> +

What is the purpose of this assignment?

> +  //
> +  // Make sure the Watchdog Timer Architectural Protocol
> +  // has not been installed in the system yet.
> +  // This will avoid conflicts with the universal watchdog
> +  //
> +  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 = EFI_OUT_OF_RESOURCES;

Why not return Status directly?

> +    goto EXIT;
> +  }
> +
> +  // Install the Timer Architectural Protocol onto a new handle
> +  Handle = NULL;
> +  Status = gBS->InstallMultipleProtocolInterfaces(
> +                  &Handle,
> +                  &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
> +                  NULL
> +                  );
> +  if (EFI_ERROR(Status)) {
> +    Status = EFI_OUT_OF_RESOURCES;

Why not return Status directly?

> +    goto EXIT;
> +  }
> +
> +EXIT:
> +  if(EFI_ERROR(Status)) {
> +    // The watchdog failed to initialize
> +    ASSERT(FALSE);
> +  }
> +
> +  WdogPing();
> +
> +  return Status;

Please close the event and/or uninstall the protocol before returning
with an error.

> +}
> diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.h b/Platform/NXP/Drivers/WatchDog/WatchDog.h
> new file mode 100644
> index 0000000..56ddbde
> --- /dev/null
> +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.h
> @@ -0,0 +1,37 @@
> +/** WatchDog.h
> +*
> +*  Copyright 2017 NXP
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD License
> +*  which accompanies this distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#ifndef __WATCHDOG_H__
> +#define __WATCHDOG_H__
> +
> +#define WDOG_SIZE           0x1000
> +#define WDOG_WCR_OFFSET     0
> +#define WDOG_WSR_OFFSET     2
> +#define WDOG_WRSR_OFFSET    4
> +#define WDOG_WICR_OFFSET    6
> +#define WDOG_WCR_WT         (0xFF << 8)
> +#define WDOG_WCR_WDE        (1 << 2)
> +#define WDOG_SERVICE_SEQ1   0x5555
> +#define WDOG_SERVICE_SEQ2   0xAAAA
> +#define WDOG_WCR_WDZST      0x1
> +#define WDOG_WCR_WRE        (1 << 3)  /* -> WDOG Reset Enable */
> +
> +#define WT_MAX_TIME         128
> +#define WD_COUNT(Sec)       (((Sec) * 2 - 1) << 8)
> +#define WD_COUNT_MASK       0xff00
> +#define WD_SEC(Cnt)         (((Cnt) + 1) / 2)
> +
> +#define NANO_SECOND_BASE    10000000
> +
> +#endif //__WATCHDOG_H__
> diff --git a/Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf b/Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf
> new file mode 100644
> index 0000000..4cf7d84
> --- /dev/null
> +++ b/Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf
> @@ -0,0 +1,47 @@
> +#  WatchDog.inf
> +#
> +#  Component description file for  WatchDog module
> +#
> +#  Copyright 2017 NXP
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD License
> +#  which accompanies this distribution.  The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005

0x0001001A

> +  BASE_NAME                      = WatchDogDxe
> +  FILE_GUID                      = ebd705fb-fa92-46a7-b32b-7f566d944614

Please don't reuse the SP805WatchdogDxe GUID

> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = WdogInitialize
> +
> +[Sources.common]
> +  WatchDog.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  edk2-platforms/Platform/NXP/NxpQoriqLs.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  BeIoLib
> +  PcdLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +
> +[Pcd]
> +  gNxpQoriqLsTokenSpaceGuid.PcdWdog1BaseAddr
> +  gNxpQoriqLsTokenSpaceGuid.PcdWdogBigEndian
> +
> +[Protocols]
> +  gEfiWatchdogTimerArchProtocolGuid
> +
> +[Depex]
> +  TRUE
> --
> 1.9.1
>