* [PATCH 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup
@ 2018-12-18 13:10 Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup Ard Biesheuvel
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-12-18 13:10 UTC (permalink / raw)
To: edk2-devel
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)
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
.../GenericWatchdogDxe/GenericWatchdogDxe.c | 145 ++++++++------
.../GenericWatchdogDxe/GenericWatchdogDxe.inf | 11 +-
ArmPlatformPkg/ArmPlatformPkg.dec | 1 +
.../Drivers/SP805WatchdogDxe/SP805Watchdog.c | 182 +++++++++++-------
.../SP805WatchdogDxe/SP805WatchdogDxe.inf | 17 +-
5 files changed, 211 insertions(+), 145 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup
2018-12-18 13:10 [PATCH 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup Ard Biesheuvel
@ 2018-12-18 13:10 ` Ard Biesheuvel
2018-12-18 13:35 ` Leif Lindholm
2018-12-18 13:10 ` [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode Ard Biesheuvel
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2018-12-18 13:10 UTC (permalink / raw)
To: edk2-devel
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>
---
ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c | 97 ++++++++++----------
ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf | 13 +--
2 files changed, 53 insertions(+), 57 deletions(-)
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;
}
diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
index 37924f2e3cd2..99ecab477567 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 @@
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
+ ArmPkg/ArmPkg.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
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
2018-12-18 13:10 [PATCH 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup Ard Biesheuvel
@ 2018-12-18 13:10 ` Ard Biesheuvel
2018-12-18 13:39 ` Leif Lindholm
2018-12-18 15:58 ` Udit Kumar
2018-12-18 13:10 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method Ard Biesheuvel
3 siblings, 2 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-12-18 13:10 UTC (permalink / raw)
To: edk2-devel
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/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|0x00000023
gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz|32000|UINT32|0x00000021
+ gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt|0|UINT32|0x0000002E
## PL011 UART
gArmPlatformTokenSpaceGuid.PL011UartClkInHz|24000000|UINT32|0x0000001F
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);
+ }
+
+ SP805Unlock ();
+ MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears the irq
+ SP805Lock ();
+
+ mInterrupt->EndOfInterrupt (mInterrupt, Source);
+}
+
/**
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code
2018-12-18 13:10 [PATCH 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode Ard Biesheuvel
@ 2018-12-18 13:10 ` Ard Biesheuvel
2018-12-18 13:43 ` Leif Lindholm
2018-12-18 13:10 ` [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method Ard Biesheuvel
3 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2018-12-18 13:10 UTC (permalink / raw)
To: edk2-devel
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>
---
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 111 +++++++++++---------
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf | 11 +-
2 files changed, 64 insertions(+), 58 deletions(-)
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 8ccf15366dfa..717a180a64ec 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
)
{
@@ -178,8 +186,8 @@ WatchdogSetTimerPeriod (
// if TimerPeriod is 0, this is a request to stop the watchdog.
if (TimerPeriod == 0) {
- mNumTimerTicks = 0;
- WatchdogDisable ();
+ //mNumTimerTicks = 0;
+ //WatchdogDisable ();
return EFI_SUCCESS;
}
@@ -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);
- // 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
- );
- }
- }
- }
+ // 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,
+ &mEfiExitBootServicesEvent);
ASSERT_EFI_ERROR (Status);
mNumTimerTicks = 0;
WatchdogDisable ();
+ return EFI_SUCCESS;
+
+UnregisterHandler:
+ // Unregister the handler
+ mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol,
+ FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+ NULL);
return Status;
}
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
index ba0403d7fdc3..7992f3ac78bb 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
@@ -16,17 +16,16 @@
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
+ ArmPkg/ArmPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdePkg/MdePkg.dec
[LibraryClasses]
ArmGenericTimerCounterLib
@@ -46,8 +45,8 @@
gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum
[Protocols]
- gEfiWatchdogTimerArchProtocolGuid
- gHardwareInterrupt2ProtocolGuid
+ gEfiWatchdogTimerArchProtocolGuid ## ALWAYS_PRODUCES
+ gHardwareInterrupt2ProtocolGuid ## ALWAYS_CONSUMES
[Depex]
gHardwareInterrupt2ProtocolGuid
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method
2018-12-18 13:10 [PATCH 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup Ard Biesheuvel
` (2 preceding siblings ...)
2018-12-18 13:10 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code Ard Biesheuvel
@ 2018-12-18 13:10 ` Ard Biesheuvel
2018-12-18 13:44 ` Leif Lindholm
2018-12-18 16:00 ` Udit Kumar
3 siblings, 2 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-12-18 13:10 UTC (permalink / raw)
To: edk2-devel
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);
+ }
// 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup
2018-12-18 13:10 ` [PATCH 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup Ard Biesheuvel
@ 2018-12-18 13:35 ` Leif Lindholm
0 siblings, 0 replies; 16+ messages in thread
From: Leif Lindholm @ 2018-12-18 13:35 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel, Sami Mujawar, Thomas Panakamattam Abraham,
Meenakshi Aggarwal, Udit Kumar, Matteo Carlini, Nariman Poushin
On Tue, Dec 18, 2018 at 02:10:11PM +0100, Ard Biesheuvel wrote:
> 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>
> ---
> ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c | 97 ++++++++++----------
> ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf | 13 +--
> 2 files changed, 53 insertions(+), 57 deletions(-)
>
> 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;
> }
> diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> index 37924f2e3cd2..99ecab477567 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 @@
> 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
> + ArmPkg/ArmPkg.dec
Umm, move up one line?
With that, beautiful enough to bring tears to my eyes:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> + 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
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
2018-12-18 13:10 ` [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode Ard Biesheuvel
@ 2018-12-18 13:39 ` Leif Lindholm
2018-12-18 16:29 ` Ard Biesheuvel
2018-12-18 15:58 ` Udit Kumar
1 sibling, 1 reply; 16+ messages in thread
From: Leif Lindholm @ 2018-12-18 13:39 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel, Sami Mujawar, Thomas Panakamattam Abraham,
Meenakshi Aggarwal, Udit Kumar, Matteo Carlini, Nariman Poushin
On Tue, Dec 18, 2018 at 02:10:12PM +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)
The only question mark from my end is - what happens when the
interrupt isn't wired up correctly? Would it be worth to bail out and
refuse to register the driver if PcdSP805WatchdogInterrupt is set to
0?
Thomas?
/
Leif
> 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|0x00000023
> gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz|32000|UINT32|0x00000021
> + gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt|0|UINT32|0x0000002E
>
> ## PL011 UART
> gArmPlatformTokenSpaceGuid.PL011UartClkInHz|24000000|UINT32|0x0000001F
> 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);
> + }
> +
> + SP805Unlock ();
> + MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears the irq
> + SP805Lock ();
> +
> + mInterrupt->EndOfInterrupt (mInterrupt, Source);
> +}
> +
> /**
> 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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code
2018-12-18 13:10 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code Ard Biesheuvel
@ 2018-12-18 13:43 ` Leif Lindholm
2018-12-18 15:19 ` Ard Biesheuvel
0 siblings, 1 reply; 16+ messages in thread
From: Leif Lindholm @ 2018-12-18 13:43 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel, Sami Mujawar, Thomas Panakamattam Abraham,
Meenakshi Aggarwal, Udit Kumar, Matteo Carlini, Nariman Poushin
On Tue, Dec 18, 2018 at 02:10:13PM +0100, Ard Biesheuvel wrote:
> 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>
> ---
> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 111 +++++++++++---------
> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf | 11 +-
> 2 files changed, 64 insertions(+), 58 deletions(-)
>
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 8ccf15366dfa..717a180a64ec 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
> )
> {
> @@ -178,8 +186,8 @@ WatchdogSetTimerPeriod (
>
> // if TimerPeriod is 0, this is a request to stop the watchdog.
> if (TimerPeriod == 0) {
> - mNumTimerTicks = 0;
> - WatchdogDisable ();
> + //mNumTimerTicks = 0;
> + //WatchdogDisable ();
Should these be deletions?
> return EFI_SUCCESS;
> }
>
> @@ -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);
>
> - // 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
> - );
> - }
> - }
> - }
> + // 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,
> + &mEfiExitBootServicesEvent);
> ASSERT_EFI_ERROR (Status);
>
> mNumTimerTicks = 0;
> WatchdogDisable ();
>
> + return EFI_SUCCESS;
> +
> +UnregisterHandler:
> + // Unregister the handler
> + mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol,
> + FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> + NULL);
> return Status;
> }
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> index ba0403d7fdc3..7992f3ac78bb 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> @@ -16,17 +16,16 @@
> 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
> + ArmPkg/ArmPkg.dec
Same as before.
If you fix that, and the above deletions rather than comment-outs:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Nice work!
> + EmbeddedPkg/EmbeddedPkg.dec
> + MdePkg/MdePkg.dec
>
> [LibraryClasses]
> ArmGenericTimerCounterLib
> @@ -46,8 +45,8 @@
> gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum
>
> [Protocols]
> - gEfiWatchdogTimerArchProtocolGuid
> - gHardwareInterrupt2ProtocolGuid
> + gEfiWatchdogTimerArchProtocolGuid ## ALWAYS_PRODUCES
> + gHardwareInterrupt2ProtocolGuid ## ALWAYS_CONSUMES
>
> [Depex]
> gHardwareInterrupt2ProtocolGuid
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method
2018-12-18 13:10 ` [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method Ard Biesheuvel
@ 2018-12-18 13:44 ` Leif Lindholm
2018-12-18 16:00 ` Udit Kumar
1 sibling, 0 replies; 16+ messages in thread
From: Leif Lindholm @ 2018-12-18 13:44 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel, Sami Mujawar, Thomas Panakamattam Abraham,
Meenakshi Aggarwal, Udit Kumar, Matteo Carlini, Nariman Poushin
On Tue, Dec 18, 2018 at 02:10:14PM +0100, Ard Biesheuvel wrote:
> 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>
Thanks!
> ---
> 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);
> + }
>
> // 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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code
2018-12-18 13:43 ` Leif Lindholm
@ 2018-12-18 15:19 ` Ard Biesheuvel
2018-12-18 15:24 ` Leif Lindholm
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2018-12-18 15:19 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Sami Mujawar,
Thomas Panakamattam Abraham, Meenakshi Aggarwal, Udit Kumar,
Matteo Carlini, Nariman Poushin
On Tue, 18 Dec 2018 at 14:43, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Tue, Dec 18, 2018 at 02:10:13PM +0100, Ard Biesheuvel wrote:
> > 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>
> > ---
> > ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 111 +++++++++++---------
> > ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf | 11 +-
> > 2 files changed, 64 insertions(+), 58 deletions(-)
> >
> > diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> > index 8ccf15366dfa..717a180a64ec 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
> > )
> > {
> > @@ -178,8 +186,8 @@ WatchdogSetTimerPeriod (
> >
> > // if TimerPeriod is 0, this is a request to stop the watchdog.
> > if (TimerPeriod == 0) {
> > - mNumTimerTicks = 0;
> > - WatchdogDisable ();
> > + //mNumTimerTicks = 0;
> > + //WatchdogDisable ();
>
> Should these be deletions?
>
Nope. I commented them out to test whether the watchdog actually
fires, since most shell tools (and GRUB) disable it while running in
interactive mode.
So this whole hunk should be dropped.
> > return EFI_SUCCESS;
> > }
> >
> > @@ -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);
> >
> > - // 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
> > - );
> > - }
> > - }
> > - }
> > + // 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,
> > + &mEfiExitBootServicesEvent);
> > ASSERT_EFI_ERROR (Status);
> >
> > mNumTimerTicks = 0;
> > WatchdogDisable ();
> >
> > + return EFI_SUCCESS;
> > +
> > +UnregisterHandler:
> > + // Unregister the handler
> > + mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol,
> > + FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> > + NULL);
> > return Status;
> > }
> > diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> > index ba0403d7fdc3..7992f3ac78bb 100644
> > --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> > +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> > @@ -16,17 +16,16 @@
> > 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
> > + ArmPkg/ArmPkg.dec
>
> Same as before.
>
> If you fix that, and the above deletions rather than comment-outs:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> Nice work!
>
> > + EmbeddedPkg/EmbeddedPkg.dec
> > + MdePkg/MdePkg.dec
> >
> > [LibraryClasses]
> > ArmGenericTimerCounterLib
> > @@ -46,8 +45,8 @@
> > gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum
> >
> > [Protocols]
> > - gEfiWatchdogTimerArchProtocolGuid
> > - gHardwareInterrupt2ProtocolGuid
> > + gEfiWatchdogTimerArchProtocolGuid ## ALWAYS_PRODUCES
> > + gHardwareInterrupt2ProtocolGuid ## ALWAYS_CONSUMES
> >
> > [Depex]
> > gHardwareInterrupt2ProtocolGuid
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code
2018-12-18 15:19 ` Ard Biesheuvel
@ 2018-12-18 15:24 ` Leif Lindholm
0 siblings, 0 replies; 16+ messages in thread
From: Leif Lindholm @ 2018-12-18 15:24 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Sami Mujawar,
Thomas Panakamattam Abraham, Meenakshi Aggarwal, Udit Kumar,
Matteo Carlini, Nariman Poushin
On Tue, Dec 18, 2018 at 04:19:20PM +0100, Ard Biesheuvel wrote:
> > > @@ -178,8 +186,8 @@ WatchdogSetTimerPeriod (
> > >
> > > // if TimerPeriod is 0, this is a request to stop the watchdog.
> > > if (TimerPeriod == 0) {
> > > - mNumTimerTicks = 0;
> > > - WatchdogDisable ();
> > > + //mNumTimerTicks = 0;
> > > + //WatchdogDisable ();
> >
> > Should these be deletions?
> >
>
> Nope. I commented them out to test whether the watchdog actually
> fires, since most shell tools (and GRUB) disable it while running in
> interactive mode.
>
> So this whole hunk should be dropped.
Works for me.
/
Leif
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
2018-12-18 13:10 ` [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode Ard Biesheuvel
2018-12-18 13:39 ` Leif Lindholm
@ 2018-12-18 15:58 ` Udit Kumar
2018-12-18 16:23 ` Ard Biesheuvel
1 sibling, 1 reply; 16+ messages in thread
From: Udit Kumar @ 2018-12-18 15:58 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm, Sami Mujawar,
Thomas Panakamattam Abraham, Meenakshi Aggarwal, Udit Kumar,
Matteo Carlini, Nariman Poushin
> -----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
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method
2018-12-18 13:10 ` [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method Ard Biesheuvel
2018-12-18 13:44 ` Leif Lindholm
@ 2018-12-18 16:00 ` Udit Kumar
1 sibling, 0 replies; 16+ messages in thread
From: Udit Kumar @ 2018-12-18 16:00 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org
> -----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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
2018-12-18 15:58 ` Udit Kumar
@ 2018-12-18 16:23 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-12-18 16:23 UTC (permalink / raw)
To: Udit Kumar
Cc: edk2-devel@lists.01.org, Leif Lindholm, Sami Mujawar,
Thomas Panakamattam Abraham, Meenakshi Aggarwal, Matteo Carlini,
Nariman Poushin
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
2018-12-18 13:39 ` Leif Lindholm
@ 2018-12-18 16:29 ` Ard Biesheuvel
2018-12-19 9:03 ` Thomas Abraham
0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2018-12-18 16:29 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Sami Mujawar,
Thomas Panakamattam Abraham, Meenakshi Aggarwal, Udit Kumar,
Matteo Carlini, Nariman Poushin
On Tue, 18 Dec 2018 at 14:39, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Tue, Dec 18, 2018 at 02:10:12PM +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)
>
> The only question mark from my end is - what happens when the
> interrupt isn't wired up correctly? Would it be worth to bail out and
> refuse to register the driver if PcdSP805WatchdogInterrupt is set to
> 0?
>
> Thomas?
>
I have left the code in place that enables the hard reset, but the
timeout is double the programmed value (since the countdown timer is
restarted on an interrupt, and the hard reset is generated when it
reaches zero the second time)
This should cover both the miswired interrupt scenario, and the
scenario where ResetSystem() (or the handler) gets stuck and never
returns.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
2018-12-18 16:29 ` Ard Biesheuvel
@ 2018-12-19 9:03 ` Thomas Abraham
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Abraham @ 2018-12-19 9:03 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Leif Lindholm, edk2-devel@lists.01.org
Hi Ard,
On Tue, Dec 18, 2018 at 9:59 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Tue, 18 Dec 2018 at 14:39, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Tue, Dec 18, 2018 at 02:10:12PM +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)
> >
> > The only question mark from my end is - what happens when the
> > interrupt isn't wired up correctly? Would it be worth to bail out and
> > refuse to register the driver if PcdSP805WatchdogInterrupt is set to
> > 0?
> >
> > Thomas?
> >
>
> I have left the code in place that enables the hard reset, but the
> timeout is double the programmed value (since the countdown timer is
> restarted on an interrupt, and the hard reset is generated when it
> reaches zero the second time)
>
> This should cover both the miswired interrupt scenario, and the
> scenario where ResetSystem() (or the handler) gets stuck and never
> returns.
Yes, this would suffice. But the system would reset after twice the
amount of time than programmed, which probably is okay.
-Thomas.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-12-19 9:03 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-18 13:10 [PATCH 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup Ard Biesheuvel
2018-12-18 13:35 ` Leif Lindholm
2018-12-18 13:10 ` [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode Ard Biesheuvel
2018-12-18 13:39 ` Leif Lindholm
2018-12-18 16:29 ` Ard Biesheuvel
2018-12-19 9:03 ` Thomas Abraham
2018-12-18 15:58 ` Udit Kumar
2018-12-18 16:23 ` Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code Ard Biesheuvel
2018-12-18 13:43 ` Leif Lindholm
2018-12-18 15:19 ` Ard Biesheuvel
2018-12-18 15:24 ` Leif Lindholm
2018-12-18 13:10 ` [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method Ard Biesheuvel
2018-12-18 13:44 ` Leif Lindholm
2018-12-18 16:00 ` Udit Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox