Re: [PATCH 4/5] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Sep 11, 2017 at 04:23:34PM +0100, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Utilise the new HardwareInterrupt2 protocol to adjust the
> Edge/Level characteristics of the Watchdog interrupt.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>

Looks plausible, but I would loke to see it tested by someone not
directly involved in development.
Armada70x0 also uses this, so adding Marcin on cc.

Beyond that, I'm told in Ryan's absence Arvind, Daniil and Thomas are
"platform owners" for Juno, so if one of you could have a spin, that'd
be great.

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

/
    Leif

> ---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |  6 ++---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 28 ++++++++++++--------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> index fece14cc18315cd15510680c438288687b60c018..ba0403d7fdc3589803c643c27a44918e73afa97e 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> @@ -1,5 +1,5 @@
>  #
> -#  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
> +#  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -47,7 +47,7 @@ [Pcd.common]
>  
>  [Protocols]
>    gEfiWatchdogTimerArchProtocolGuid
> -  gHardwareInterruptProtocolGuid
> +  gHardwareInterrupt2ProtocolGuid
>  
>  [Depex]
> -  gHardwareInterruptProtocolGuid
> +  gHardwareInterrupt2ProtocolGuid
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 6d102e25047253048ac555d6fb5de7223d78f381..69844db2e11f51907e6c8bff5c67d27ceb498150 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
> +*  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD
> @@ -24,8 +24,8 @@
>  #include <Library/UefiLib.h>
>  #include <Library/ArmGenericTimerCounterLib.h>
>  
> +#include <Protocol/HardwareInterrupt2.h>
>  #include <Protocol/WatchdogTimer.h>
> -#include <Protocol/HardwareInterrupt.h>
>  
>  #include "GenericWatchdog.h"
>  
> @@ -41,7 +41,7 @@ UINTN mTimerFrequencyHz = 0;
>  // It is therefore stored here. 0 means the timer is not running.
>  UINT64 mNumTimerTicks = 0;
>  
> -EFI_HARDWARE_INTERRUPT_PROTOCOL *mInterruptProtocol;
> +EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
>  
>  EFI_STATUS
>  WatchdogWriteOffsetRegister (
> @@ -320,7 +320,7 @@ GenericWatchdogEntry (
>    if (!EFI_ERROR (Status)) {
>      // Install interrupt handler
>      Status = gBS->LocateProtocol (
> -                    &gHardwareInterruptProtocolGuid,
> +                    &gHardwareInterrupt2ProtocolGuid,
>                      NULL,
>                      (VOID **)&mInterruptProtocol
>                      );
> @@ -331,13 +331,19 @@ GenericWatchdogEntry (
>                                      WatchdogInterruptHandler
>                                      );
>        if (!EFI_ERROR (Status)) {
> -        // Install the Timer Architectural Protocol onto a new handle
> -        Handle = NULL;
> -        Status = gBS->InstallMultipleProtocolInterfaces (
> -                        &Handle,
> -                        &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
> -                        NULL
> -                        );
> +        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
> +                          );
> +        }
>        }
>      }
>    }
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>