Installs watchdog timer arch protocol
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
---
Platform/NXP/Drivers/WatchDog/WatchDog.c | 421 ++++++++++++++++++++++++++
Platform/NXP/Drivers/WatchDog/WatchDog.h | 37 +++
Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf | 47 +++
3 files changed, 505 insertions(+)
create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDog.c
create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDog.h
create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf
diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c b/Platform/NXP/Drivers/WatchDog/WatchDog.c
new file mode 100644
index 0000000..a9c70ef
--- /dev/null
+++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
@@ -0,0 +1,421 @@
+/** WatchDog.c
+*
+* Based on Watchdog driver implemenation available in
+* ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
+*
+* Copyright (c) 2011-2013, ARM Limited. All rights reserved.
+* Copyright 2017 NXP
+*
+* This program and the accompanying materials
+* are licensed and made available under the terms and conditions of the BSD License
+* which accompanies this distribution. The full text of the license may be found at
+* http://opensource.org/licenses/bsd-license.php
+*
+* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+*
+**/
+
+#include <PiDxe.h>
+#include <Library/BaseLib.h>
+#include <Library/BeIoLib.h>
+#include <Library/DebugLib.h>
+#include <Library/IoLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/WatchdogTimer.h>
+
+#include "WatchDog.h"
+
+STATIC EFI_EVENT EfiExitBootServicesEvent;
+
+STATIC
+UINT16
+EFIAPI
+WdogRead (
+ IN UINTN Address
+ )
+{
+ if (FixedPcdGetBool (PcdWdogBigEndian)) {
+ return BeMmioRead16 (Address);
+ } else {
+ return MmioRead16(Address);
+ }
+}
+
+STATIC
+UINT16
+EFIAPI
+WdogWrite (
+ IN UINTN Address,
+ IN UINT16 Value
+ )
+{
+ if (FixedPcdGetBool (PcdWdogBigEndian)) {
+ return BeMmioWrite16 (Address, Value);
+ } else {
+ return MmioWrite16 (Address, Value);
+ }
+}
+
+STATIC
+UINT16
+EFIAPI
+WdogAndThenOr (
+ IN UINTN Address,
+ IN UINT16 And,
+ IN UINT16 Or
+ )
+{
+ if (FixedPcdGetBool (PcdWdogBigEndian)) {
+ return BeMmioAndThenOr16 (Address, And, Or);
+ } else {
+ return MmioAndThenOr16 (Address, And, Or);
+ }
+}
+
+STATIC
+UINT16
+EFIAPI
+WdogOr (
+ IN UINTN Address,
+ IN UINT16 Or
+ )
+{
+ if (FixedPcdGetBool (PcdWdogBigEndian)) {
+ return BeMmioOr16 (Address, Or);
+ } else {
+ return MmioOr16 (Address, Or);
+ }
+}
+
+STATIC
+VOID
+WdogPing (
+ VOID
+ )
+{
+ //
+ // To reload a timeout value to the counter the proper service sequence begins by
+ // writing 0x_5555 followed by 0x_AAAA to the Watchdog Service Register (WDOG_WSR).
+ // This service sequence will reload the counter with the timeout value WT[7:0] of
+ // Watchdog Control Register (WDOG_WCR).
+ //
+
+ WdogWrite (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WSR_OFFSET,
+ WDOG_SERVICE_SEQ1);
+ WdogWrite (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WSR_OFFSET,
+ WDOG_SERVICE_SEQ2);
+}
+
+/**
+ Stop the Wdog watchdog timer from counting down.
+**/
+STATIC
+VOID
+WdogStop (
+ VOID
+ )
+{
+ // Watchdog cannot be disabled by software once started.
+ // At best, we can keep reload counter with maximum value
+
+ WdogAndThenOr (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WCR_OFFSET,
+ (UINT16)(~WDOG_WCR_WT),
+ (WD_COUNT (WT_MAX_TIME) & WD_COUNT_MASK));
+ WdogPing ();
+}
+
+/**
+ Starts the Wdog counting down by feeding Service register with
+ desired pattern.
+ The count down will start from the value stored in the Load register,
+ not from the value where it was previously stopped.
+**/
+STATIC
+VOID
+WdogStart (
+ VOID
+ )
+{
+ //Reload the timeout value
+ WdogPing ();
+}
+
+/**
+ On exiting boot services we must make sure the Wdog Watchdog Timer
+ is stopped.
+**/
+STATIC
+VOID
+EFIAPI
+ExitBootServicesEvent (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ WdogStop ();
+}
+
+/**
+ This function registers the handler NotifyFunction so it is called every time
+ the watchdog timer expires. It also passes the amount of time since the last
+ handler call to the NotifyFunction.
+ If NotifyFunction is not NULL and a handler is not already registered,
+ then the new handler is registered and EFI_SUCCESS is returned.
+ If NotifyFunction is NULL, and a handler is already registered,
+ then that handler is unregistered.
+ If an attempt is made to register a handler when a handler is already registered,
+ then EFI_ALREADY_STARTED is returned.
+ If an attempt is made to unregister a handler when a handler is not registered,
+ then EFI_INVALID_PARAMETER is returned.
+
+ @param This The EFI_TIMER_ARCH_PROTOCOL instance.
+ @param NotifyFunction The function to call when a timer interrupt fires. This
+ function executes at TPL_HIGH_LEVEL. The DXE Core will
+ register a handler for the timer interrupt, so it can know
+ how much time has passed. This information is used to
+ signal timer based events. NULL will unregister the handler.
+
+ @retval EFI_SUCCESS The watchdog timer handler was registered.
+ @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a handler is already
+ registered.
+ @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler was not
+ previously registered.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+WdogRegisterHandler (
+ IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
+ IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
+ )
+{
+ // ERROR: This function is not supported.
+ // The hardware watchdog will reset the board
+ return EFI_INVALID_PARAMETER;
+}
+
+/**
+
+ This function adjusts the period of timer interrupts to the value specified
+ by TimerPeriod. If the timer period is updated, then the selected timer
+ period is stored in EFI_TIMER.TimerPeriod, and EFI_SUCCESS is returned. If
+ the timer hardware is not programmable, then EFI_UNSUPPORTED is returned.
+ If an error occurs while attempting to update the timer period, then the
+ timer hardware will be put back in its state prior to this call, and
+ EFI_DEVICE_ERROR is returned. If TimerPeriod is 0, then the timer interrupt
+ is disabled. This is not the same as disabling the CPU's interrupts.
+ Instead, it must either turn off the timer hardware, or it must adjust the
+ interrupt controller so that a CPU interrupt is not generated when the timer
+ interrupt fires.
+
+ @param This The EFI_TIMER_ARCH_PROTOCOL instance.
+ @param TimerPeriod The rate to program the timer interrupt in 100 nS units. If
+ the timer hardware is not programmable, then EFI_UNSUPPORTED is
+ returned. If the timer is programmable, then the timer period
+ will be rounded up to the nearest timer period that is supported
+ by the timer hardware. If TimerPeriod is set to 0, then the
+ timer interrupts will be disabled.
+
+
+ @retval EFI_SUCCESS The timer period was changed.
+ @retval EFI_UNSUPPORTED The platform cannot change the period of the timer interrupt.
+ @retval EFI_DEVICE_ERROR The timer period could not be changed due to a device error.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+WdogSetTimerPeriod (
+ IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
+ IN UINT64 TimerPeriod // In 100ns units
+ )
+{
+ EFI_STATUS Status;
+ UINT64 TimerPeriodInSec;
+ UINT16 Val;
+
+ Status = EFI_SUCCESS;
+
+ if (TimerPeriod == 0) {
+ // This is a watchdog stop request
+ WdogStop ();
+ return Status;
+ } else {
+ // Convert the TimerPeriod (in 100 ns unit) to an equivalent second value
+
+ TimerPeriodInSec = DivU64x32 (TimerPeriod, NANO_SECOND_BASE);
+
+ // The registers in the Wdog are only 32 bits
+ if (TimerPeriodInSec > WT_MAX_TIME) {
+ // We could load the watchdog with the maximum supported value but
+ // if a smaller value was requested, this could have the watchdog
+ // triggering before it was intended.
+ // Better generate an error to let the caller know.
+ Status = EFI_DEVICE_ERROR;
+ return Status;
+ }
+
+ // set the new timeout value in the WCR
+ // Convert the timeout value from Seconds to timer count
+ Val = ((WD_COUNT(TimerPeriodInSec) & WD_COUNT_MASK) << 8);
+
+ WdogAndThenOr (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WCR_OFFSET,
+ (UINT16)(~WDOG_WCR_WT),
+ Val);
+ // Start the watchdog
+ WdogStart ();
+ }
+
+ return Status;
+}
+
+/**
+ This function retrieves the period of timer interrupts in 100 ns units,
+ returns that value in TimerPeriod, and returns EFI_SUCCESS. If TimerPeriod
+ is NULL, then EFI_INVALID_PARAMETER is returned. If a TimerPeriod of 0 is
+ returned, then the timer is currently disabled.
+
+ @param This The EFI_TIMER_ARCH_PROTOCOL instance.
+ @param TimerPeriod A pointer to the timer period to retrieve in 100 ns units. If
+ 0 is returned, then the timer is currently disabled.
+
+
+ @retval EFI_SUCCESS The timer period was returned in TimerPeriod.
+ @retval EFI_INVALID_PARAMETER TimerPeriod is NULL.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+WdogGetTimerPeriod (
+ IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
+ OUT UINT64 *TimerPeriod
+ )
+{
+ EFI_STATUS Status;
+ UINT64 ReturnValue;
+ UINT16 Val;
+
+ Status = EFI_SUCCESS;
+
+ if (TimerPeriod == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ // Check if the watchdog is stopped
+ if ((WdogRead (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WCR_OFFSET)
+ & WDOG_WCR_WDE) == 0 ) {
+ // It is stopped, so return zero.
+ ReturnValue = 0;
+ } else {
+ // Convert the Watchdog ticks into equivalent TimerPeriod second value.
+ Val = (WdogRead (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WCR_OFFSET)
+ & WDOG_WCR_WT ) >> 8;
+ ReturnValue = WD_SEC(Val);
+ }
+
+ *TimerPeriod = ReturnValue;
+ return Status;
+}
+
+/**
+ Interface structure for the Watchdog Architectural Protocol.
+
+ @par Protocol Description:
+ This protocol provides a service to set the amount of time to wait
+ before firing the watchdog timer, and it also provides a service to
+ register a handler that is invoked when the watchdog timer fires.
+
+ @par When the watchdog timer fires, control will be passed to a handler
+ if one has been registered. If no handler has been registered,
+ or the registered handler returns, then the system will be
+ reset by calling the Runtime Service ResetSystem().
+
+ @param RegisterHandler
+ Registers a handler that will be called each time the
+ watchdogtimer interrupt fires. TimerPeriod defines the minimum
+ time between timer interrupts, so TimerPeriod will also
+ be the minimum time between calls to the registered
+ handler.
+ NOTE: If the watchdog resets the system in hardware, then
+ this function will not have any chance of executing.
+
+ @param SetTimerPeriod
+ Sets the period of the timer interrupt in 100 nS units.
+ This function is optional, and may return EFI_UNSUPPORTED.
+ If this function is supported, then the timer period will
+ be rounded up to the nearest supported timer period.
+
+ @param GetTimerPeriod
+ Retrieves the period of the timer interrupt in 100 nS units.
+
+**/
+STATIC
+EFI_WATCHDOG_TIMER_ARCH_PROTOCOL gWatchdogTimer = {
+ WdogRegisterHandler,
+ WdogSetTimerPeriod,
+ WdogGetTimerPeriod
+};
+
+/**
+ Initialize state information for the Watchdog Timer Architectural Protocol.
+
+ @param ImageHandle of the loaded driver
+ @param SystemTable Pointer to the System Table
+
+ @retval EFI_SUCCESS Protocol registered
+ @retval EFI_OUT_OF_RESOURCES Cannot allocate protocol data structure
+ @retval EFI_DEVICE_ERROR Hardware problems
+
+**/
+EFI_STATUS
+EFIAPI
+WdogInitialize (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+ EFI_HANDLE Handle;
+
+ WdogAndThenOr (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WCR_OFFSET,
+ (UINT16)(~WDOG_WCR_WT),
+ (WD_COUNT (WT_MAX_TIME) & WD_COUNT_MASK));
+
+ WdogOr (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WCR_OFFSET, WDOG_WCR_WDE);
+
+ //
+ // Make sure the Watchdog Timer Architectural Protocol
+ // has not been installed in the system yet.
+ // This will avoid conflicts with the universal watchdog
+ //
+ ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiWatchdogTimerArchProtocolGuid);
+
+ // Register for an ExitBootServicesEvent
+ Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY,
+ ExitBootServicesEvent, NULL, &EfiExitBootServicesEvent);
+ if (EFI_ERROR (Status)) {
+ Status = EFI_OUT_OF_RESOURCES;
+ return Status;
+ }
+
+ // Install the Timer Architectural Protocol onto a new handle
+ Handle = NULL;
+ Status = gBS->InstallMultipleProtocolInterfaces (
+ &Handle,
+ &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ gBS->CloseEvent (EfiExitBootServicesEvent);
+ Status = EFI_OUT_OF_RESOURCES;
+ return Status;
+ }
+
+ WdogPing ();
+
+ return Status;
+}
diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.h b/Platform/NXP/Drivers/WatchDog/WatchDog.h
new file mode 100644
index 0000000..1de308b
--- /dev/null
+++ b/Platform/NXP/Drivers/WatchDog/WatchDog.h
@@ -0,0 +1,37 @@
+/** WatchDog.h
+*
+* Copyright 2017 NXP
+*
+* This program and the accompanying materials
+* are licensed and made available under the terms and conditions of the BSD License
+* which accompanies this distribution. The full text of the license may be found at
+* http://opensource.org/licenses/bsd-license.php
+*
+* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+*
+**/
+
+#ifndef __WATCHDOG_H__
+#define __WATCHDOG_H__
+
+#define WDOG_SIZE 0x1000
+#define WDOG_WCR_OFFSET 0
+#define WDOG_WSR_OFFSET 2
+#define WDOG_WRSR_OFFSET 4
+#define WDOG_WICR_OFFSET 6
+#define WDOG_WCR_WT (0xFF << 8)
+#define WDOG_WCR_WDE (1 << 2)
+#define WDOG_SERVICE_SEQ1 0x5555
+#define WDOG_SERVICE_SEQ2 0xAAAA
+#define WDOG_WCR_WDZST 0x1
+#define WDOG_WCR_WRE (1 << 3) /* -> WDOG Reset Enable */
+
+#define WT_MAX_TIME 128
+#define WD_COUNT(Sec) (((Sec) * 2 - 1) << 8)
+#define WD_COUNT_MASK 0xff00
+#define WD_SEC(Cnt) (((Cnt) + 1) / 2)
+
+#define NANO_SECOND_BASE 10000000
+
+#endif //__WATCHDOG_H__
diff --git a/Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf b/Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf
new file mode 100644
index 0000000..63b0816
--- /dev/null
+++ b/Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf
@@ -0,0 +1,47 @@
+# WatchDog.inf
+#
+# Component description file for WatchDog module
+#
+# Copyright 2017 NXP
+#
+# This program and the accompanying materials
+# are licensed and made available under the terms and conditions of the BSD License
+# which accompanies this distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#
+
+[Defines]
+ INF_VERSION = 0x0001001A
+ BASE_NAME = WatchDogDxe
+ FILE_GUID = 0358b544-ec65-4339-89cd-cad60a3dd787
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = WdogInitialize
+
+[Sources.common]
+ WatchDog.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ Platform/NXP/NxpQoriqLs.dec
+
+[LibraryClasses]
+ BaseLib
+ BeIoLib
+ PcdLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Pcd]
+ gNxpQoriqLsTokenSpaceGuid.PcdWdog1BaseAddr
+ gNxpQoriqLsTokenSpaceGuid.PcdWdogBigEndian
+
+[Protocols]
+ gEfiWatchdogTimerArchProtocolGuid
+
+[Depex]
+ TRUE
--
1.9.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Mike, Liming, as MdePkg mainteiners - one question below:
On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> new file mode 100644
> index 0000000..a9c70ef
> --- /dev/null
> +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> @@ -0,0 +1,421 @@
...
> +/**
> + This function registers the handler NotifyFunction so it is called every time
> + the watchdog timer expires. It also passes the amount of time since the last
> + handler call to the NotifyFunction.
> + If NotifyFunction is not NULL and a handler is not already registered,
> + then the new handler is registered and EFI_SUCCESS is returned.
> + If NotifyFunction is NULL, and a handler is already registered,
> + then that handler is unregistered.
> + If an attempt is made to register a handler when a handler is already registered,
> + then EFI_ALREADY_STARTED is returned.
> + If an attempt is made to unregister a handler when a handler is not registered,
> + then EFI_INVALID_PARAMETER is returned.
> +
> + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> + @param NotifyFunction The function to call when a timer interrupt fires. This
> + function executes at TPL_HIGH_LEVEL. The DXE Core will
> + register a handler for the timer interrupt, so it can know
> + how much time has passed. This information is used to
> + signal timer based events. NULL will unregister the handler.
> +
> + @retval EFI_SUCCESS The watchdog timer handler was registered.
> + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a handler is already
> + registered.
> + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler was not
> + previously registered.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +WdogRegisterHandler (
> + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> + )
> +{
> + // ERROR: This function is not supported.
> + // The hardware watchdog will reset the board
> + return EFI_INVALID_PARAMETER;
Michael, Liming - what's your take on this?
Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a pure-hw
watchdog such as this?
If so, what would be a suitable return code here?
EFI_INVALID_PARAMETER does not look ideal.
/
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif:
I suggest return EFI_UNSUPPORTED for this case. The protocol implementation could return its status besides spec defined status.
Thanks
Liming
> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Monday, December 4, 2017 10:36 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> udit.kumar@nxp.com; v.sethi@nxp.com
> Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
>
> Mike, Liming, as MdePkg mainteiners - one question below:
>
> On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > new file mode 100644
> > index 0000000..a9c70ef
> > --- /dev/null
> > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > @@ -0,0 +1,421 @@
>
> ...
>
> > +/**
> > + This function registers the handler NotifyFunction so it is called every time
> > + the watchdog timer expires. It also passes the amount of time since the last
> > + handler call to the NotifyFunction.
> > + If NotifyFunction is not NULL and a handler is not already registered,
> > + then the new handler is registered and EFI_SUCCESS is returned.
> > + If NotifyFunction is NULL, and a handler is already registered,
> > + then that handler is unregistered.
> > + If an attempt is made to register a handler when a handler is already registered,
> > + then EFI_ALREADY_STARTED is returned.
> > + If an attempt is made to unregister a handler when a handler is not registered,
> > + then EFI_INVALID_PARAMETER is returned.
> > +
> > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> > + @param NotifyFunction The function to call when a timer interrupt fires. This
> > + function executes at TPL_HIGH_LEVEL. The DXE Core will
> > + register a handler for the timer interrupt, so it can know
> > + how much time has passed. This information is used to
> > + signal timer based events. NULL will unregister the handler.
> > +
> > + @retval EFI_SUCCESS The watchdog timer handler was registered.
> > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a handler is already
> > + registered.
> > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler was not
> > + previously registered.
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +WdogRegisterHandler (
> > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > + )
> > +{
> > + // ERROR: This function is not supported.
> > + // The hardware watchdog will reset the board
> > + return EFI_INVALID_PARAMETER;
>
> Michael, Liming - what's your take on this?
>
> Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a pure-hw
> watchdog such as this?
>
> If so, what would be a suitable return code here?
> EFI_INVALID_PARAMETER does not look ideal.
>
> /
> Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
> I suggest return EFI_UNSUPPORTED for this case. The protocol implementation
> could return its status besides spec defined status.
Thanks to help me , how core will treat this error
1/ Wdt not available
2/ ignoring this error
3/ core is not registering handler
I guess 3 is valid,
On side track, looks wdt is not used by core services then do we really need this
as part of arch protocol ?
regards
Udit
> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Monday, December 04, 2017 8:53 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support
> for Watchdog driver
>
> Leif:
> I suggest return EFI_UNSUPPORTED for this case. The protocol implementation
> could return its status besides spec defined status.
>
> Thanks
> Liming
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: Monday, December 4, 2017 10:36 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>
> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > udit.kumar@nxp.com; v.sethi@nxp.com
> > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > support for Watchdog driver
> >
> > Mike, Liming, as MdePkg mainteiners - one question below:
> >
> > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > new file mode 100644
> > > index 0000000..a9c70ef
> > > --- /dev/null
> > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > @@ -0,0 +1,421 @@
> >
> > ...
> >
> > > +/**
> > > + This function registers the handler NotifyFunction so it is
> > > +called every time
> > > + the watchdog timer expires. It also passes the amount of time
> > > +since the last
> > > + handler call to the NotifyFunction.
> > > + If NotifyFunction is not NULL and a handler is not already
> > > +registered,
> > > + then the new handler is registered and EFI_SUCCESS is returned.
> > > + If NotifyFunction is NULL, and a handler is already registered,
> > > + then that handler is unregistered.
> > > + If an attempt is made to register a handler when a handler is
> > > +already registered,
> > > + then EFI_ALREADY_STARTED is returned.
> > > + If an attempt is made to unregister a handler when a handler is
> > > +not registered,
> > > + then EFI_INVALID_PARAMETER is returned.
> > > +
> > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> > > + @param NotifyFunction The function to call when a timer interrupt fires.
> This
> > > + function executes at TPL_HIGH_LEVEL. The DXE Core will
> > > + register a handler for the timer interrupt, so it can know
> > > + how much time has passed. This information is used to
> > > + signal timer based events. NULL will unregister the handler.
> > > +
> > > + @retval EFI_SUCCESS The watchdog timer handler was registered.
> > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> handler is already
> > > + registered.
> > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler
> was not
> > > + previously registered.
> > > +
> > > +**/
> > > +STATIC
> > > +EFI_STATUS
> > > +EFIAPI
> > > +WdogRegisterHandler (
> > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > > + )
> > > +{
> > > + // ERROR: This function is not supported.
> > > + // The hardware watchdog will reset the board
> > > + return EFI_INVALID_PARAMETER;
> >
> > Michael, Liming - what's your take on this?
> >
> > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a pure-hw
> > watchdog such as this?
> >
> > If so, what would be a suitable return code here?
> > EFI_INVALID_PARAMETER does not look ideal.
> >
> > /
> > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> > I suggest return EFI_UNSUPPORTED for this case. The protocol implementation
> > could return its status besides spec defined status.
>
> Thanks to help me , how core will treat this error
> 1/ Wdt not available
> 2/ ignoring this error
> 3/ core is not registering handler
> I guess 3 is valid,
Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
//
// Attempt to set the timeout
//
Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
//
// Check for errors
//
if (EFI_ERROR (Status)) {
return EFI_DEVICE_ERROR;
}
The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
> On side track, looks wdt is not used by core services then do we
> really need this as part of arch protocol ?
Yes, that was ultimately what I was implying with my question
regarding whether this protocol is relevant for a watchdog that can
only ever reset the system on timeout.
The protocol looks to me to be designed to use a dedicated generic
timer as backing for a software watchdog.
Liming, Mike?
If that is the case, then I agree this driver should probably not
implement this protocol, but rather set up a timer event (or a
dedicated timer) to stroke the watchdog.
Regards,
Leif
> regards
> Udit
>
> > -----Original Message-----
> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > Sent: Monday, December 04, 2017 8:53 PM
> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support
> > for Watchdog driver
> >
> > Leif:
> > I suggest return EFI_UNSUPPORTED for this case. The protocol implementation
> > could return its status besides spec defined status.
> >
> > Thanks
> > Liming
> > > -----Original Message-----
> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > Sent: Monday, December 4, 2017 10:36 PM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>
> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > > udit.kumar@nxp.com; v.sethi@nxp.com
> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > > support for Watchdog driver
> > >
> > > Mike, Liming, as MdePkg mainteiners - one question below:
> > >
> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > new file mode 100644
> > > > index 0000000..a9c70ef
> > > > --- /dev/null
> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > @@ -0,0 +1,421 @@
> > >
> > > ...
> > >
> > > > +/**
> > > > + This function registers the handler NotifyFunction so it is
> > > > +called every time
> > > > + the watchdog timer expires. It also passes the amount of time
> > > > +since the last
> > > > + handler call to the NotifyFunction.
> > > > + If NotifyFunction is not NULL and a handler is not already
> > > > +registered,
> > > > + then the new handler is registered and EFI_SUCCESS is returned.
> > > > + If NotifyFunction is NULL, and a handler is already registered,
> > > > + then that handler is unregistered.
> > > > + If an attempt is made to register a handler when a handler is
> > > > +already registered,
> > > > + then EFI_ALREADY_STARTED is returned.
> > > > + If an attempt is made to unregister a handler when a handler is
> > > > +not registered,
> > > > + then EFI_INVALID_PARAMETER is returned.
> > > > +
> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> > > > + @param NotifyFunction The function to call when a timer interrupt fires.
> > This
> > > > + function executes at TPL_HIGH_LEVEL. The DXE Core will
> > > > + register a handler for the timer interrupt, so it can know
> > > > + how much time has passed. This information is used to
> > > > + signal timer based events. NULL will unregister the handler.
> > > > +
> > > > + @retval EFI_SUCCESS The watchdog timer handler was registered.
> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> > handler is already
> > > > + registered.
> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler
> > was not
> > > > + previously registered.
> > > > +
> > > > +**/
> > > > +STATIC
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +WdogRegisterHandler (
> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > > > + )
> > > > +{
> > > > + // ERROR: This function is not supported.
> > > > + // The hardware watchdog will reset the board
> > > > + return EFI_INVALID_PARAMETER;
> > >
> > > Michael, Liming - what's your take on this?
> > >
> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a pure-hw
> > > watchdog such as this?
> > >
> > > If so, what would be a suitable return code here?
> > > EFI_INVALID_PARAMETER does not look ideal.
> > >
> > > /
> > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Liming, Mike,
Please share your inputs on this.
Thanks,
Meenakshi
> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, December 05, 2017 4:36 PM
> To: Udit Kumar <udit.kumar@nxp.com>
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> support for Watchdog driver
>
> On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> > > I suggest return EFI_UNSUPPORTED for this case. The protocol
> implementation
> > > could return its status besides spec defined status.
> >
> > Thanks to help me , how core will treat this error
> > 1/ Wdt not available
> > 2/ ignoring this error
> > 3/ core is not registering handler
> > I guess 3 is valid,
>
> Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> //
> // Attempt to set the timeout
> //
> Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
>
> //
> // Check for errors
> //
> if (EFI_ERROR (Status)) {
> return EFI_DEVICE_ERROR;
> }
>
> The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
>
> > On side track, looks wdt is not used by core services then do we
> > really need this as part of arch protocol ?
>
> Yes, that was ultimately what I was implying with my question
> regarding whether this protocol is relevant for a watchdog that can
> only ever reset the system on timeout.
>
> The protocol looks to me to be designed to use a dedicated generic
> timer as backing for a software watchdog.
>
> Liming, Mike?
>
> If that is the case, then I agree this driver should probably not
> implement this protocol, but rather set up a timer event (or a
> dedicated timer) to stroke the watchdog.
>
> Regards,
>
> Leif
>
> > regards
> > Udit
> >
> > > -----Original Message-----
> > > From: Gao, Liming [mailto:liming.gao@intel.com]
> > > Sent: Monday, December 04, 2017 8:53 PM
> > > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>
> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> > > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> support
> > > for Watchdog driver
> > >
> > > Leif:
> > > I suggest return EFI_UNSUPPORTED for this case. The protocol
> implementation
> > > could return its status besides spec defined status.
> > >
> > > Thanks
> > > Liming
> > > > -----Original Message-----
> > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > Sent: Monday, December 4, 2017 10:36 PM
> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > > <liming.gao@intel.com>
> > > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > > > udit.kumar@nxp.com; v.sethi@nxp.com
> > > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> Add
> > > > support for Watchdog driver
> > > >
> > > > Mike, Liming, as MdePkg mainteiners - one question below:
> > > >
> > > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal
> wrote:
> > > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > > new file mode 100644
> > > > > index 0000000..a9c70ef
> > > > > --- /dev/null
> > > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > > @@ -0,0 +1,421 @@
> > > >
> > > > ...
> > > >
> > > > > +/**
> > > > > + This function registers the handler NotifyFunction so it is
> > > > > +called every time
> > > > > + the watchdog timer expires. It also passes the amount of time
> > > > > +since the last
> > > > > + handler call to the NotifyFunction.
> > > > > + If NotifyFunction is not NULL and a handler is not already
> > > > > +registered,
> > > > > + then the new handler is registered and EFI_SUCCESS is returned.
> > > > > + If NotifyFunction is NULL, and a handler is already registered,
> > > > > + then that handler is unregistered.
> > > > > + If an attempt is made to register a handler when a handler is
> > > > > +already registered,
> > > > > + then EFI_ALREADY_STARTED is returned.
> > > > > + If an attempt is made to unregister a handler when a handler is
> > > > > +not registered,
> > > > > + then EFI_INVALID_PARAMETER is returned.
> > > > > +
> > > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> > > > > + @param NotifyFunction The function to call when a timer
> interrupt fires.
> > > This
> > > > > + function executes at TPL_HIGH_LEVEL. The DXE Core
> will
> > > > > + register a handler for the timer interrupt, so it can
> know
> > > > > + how much time has passed. This information is used to
> > > > > + signal timer based events. NULL will unregister the
> handler.
> > > > > +
> > > > > + @retval EFI_SUCCESS The watchdog timer handler was
> registered.
> > > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> > > handler is already
> > > > > + registered.
> > > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a
> handler
> > > was not
> > > > > + previously registered.
> > > > > +
> > > > > +**/
> > > > > +STATIC
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +WdogRegisterHandler (
> > > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > > > > + )
> > > > > +{
> > > > > + // ERROR: This function is not supported.
> > > > > + // The hardware watchdog will reset the board
> > > > > + return EFI_INVALID_PARAMETER;
> > > >
> > > > Michael, Liming - what's your take on this?
> > > >
> > > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
> pure-hw
> > > > watchdog such as this?
> > > >
> > > > If so, what would be a suitable return code here?
> > > > EFI_INVALID_PARAMETER does not look ideal.
> > > >
> > > > /
> > > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif:
I don't see the core driver uses WatchdogTimer->RegisterHandler(). When it returns unsupported, it means the additional handler can't be registered. DxeCore uses WatchdogTimer->SetTimerPeriod(). This service is implemented in your driver.
Watchdog protocol is defined in PI spec. Spec describes that this protocol provides the services required to implement the Boot Service SetWatchdogTimer(). It provides a service to set the amount of time to wait before firing the watchdog timer, and it also provides a service to register a handler that is invoked when the watchdog timer fires. This protocol can implement the watchdog timer by using the event and timer Boot Services, or it can make use of custom hardware. If no handler has been registered, or the registered handler returns, then the system will be reset by calling the Runtime Service ResetSystem(). So, this protocol is required.
Thanks
Liming
>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: Tuesday, December 05, 2017 7:06 PM
>To: Udit Kumar <udit.kumar@nxp.com>
>Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; Meenakshi Aggarwal
><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
>devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
>Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
>support for Watchdog driver
>
>On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
>> > I suggest return EFI_UNSUPPORTED for this case. The protocol
>implementation
>> > could return its status besides spec defined status.
>>
>> Thanks to help me , how core will treat this error
>> 1/ Wdt not available
>> 2/ ignoring this error
>> 3/ core is not registering handler
>> I guess 3 is valid,
>
>Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> //
> // Attempt to set the timeout
> //
> Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
>
> //
> // Check for errors
> //
> if (EFI_ERROR (Status)) {
> return EFI_DEVICE_ERROR;
> }
>
>The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
>
>> On side track, looks wdt is not used by core services then do we
>> really need this as part of arch protocol ?
>
>Yes, that was ultimately what I was implying with my question
>regarding whether this protocol is relevant for a watchdog that can
>only ever reset the system on timeout.
>
>The protocol looks to me to be designed to use a dedicated generic
>timer as backing for a software watchdog.
>
>Liming, Mike?
>
>If that is the case, then I agree this driver should probably not
>implement this protocol, but rather set up a timer event (or a
>dedicated timer) to stroke the watchdog.
>
>Regards,
>
>Leif
>
>> regards
>> Udit
>>
>> > -----Original Message-----
>> > From: Gao, Liming [mailto:liming.gao@intel.com]
>> > Sent: Monday, December 04, 2017 8:53 PM
>> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
>> > <michael.d.kinney@intel.com>
>> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
>> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
>> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
>> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
>support
>> > for Watchdog driver
>> >
>> > Leif:
>> > I suggest return EFI_UNSUPPORTED for this case. The protocol
>implementation
>> > could return its status besides spec defined status.
>> >
>> > Thanks
>> > Liming
>> > > -----Original Message-----
>> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>> > > Sent: Monday, December 4, 2017 10:36 PM
>> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> > > <liming.gao@intel.com>
>> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
>> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
>> > > udit.kumar@nxp.com; v.sethi@nxp.com
>> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
>> > > support for Watchdog driver
>> > >
>> > > Mike, Liming, as MdePkg mainteiners - one question below:
>> > >
>> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
>> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
>> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
>> > > > new file mode 100644
>> > > > index 0000000..a9c70ef
>> > > > --- /dev/null
>> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
>> > > > @@ -0,0 +1,421 @@
>> > >
>> > > ...
>> > >
>> > > > +/**
>> > > > + This function registers the handler NotifyFunction so it is
>> > > > +called every time
>> > > > + the watchdog timer expires. It also passes the amount of time
>> > > > +since the last
>> > > > + handler call to the NotifyFunction.
>> > > > + If NotifyFunction is not NULL and a handler is not already
>> > > > +registered,
>> > > > + then the new handler is registered and EFI_SUCCESS is returned.
>> > > > + If NotifyFunction is NULL, and a handler is already registered,
>> > > > + then that handler is unregistered.
>> > > > + If an attempt is made to register a handler when a handler is
>> > > > +already registered,
>> > > > + then EFI_ALREADY_STARTED is returned.
>> > > > + If an attempt is made to unregister a handler when a handler is
>> > > > +not registered,
>> > > > + then EFI_INVALID_PARAMETER is returned.
>> > > > +
>> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
>> > > > + @param NotifyFunction The function to call when a timer interrupt
>fires.
>> > This
>> > > > + function executes at TPL_HIGH_LEVEL. The DXE Core
>will
>> > > > + register a handler for the timer interrupt, so it can know
>> > > > + how much time has passed. This information is used to
>> > > > + signal timer based events. NULL will unregister the
>handler.
>> > > > +
>> > > > + @retval EFI_SUCCESS The watchdog timer handler was
>registered.
>> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
>> > handler is already
>> > > > + registered.
>> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a
>handler
>> > was not
>> > > > + previously registered.
>> > > > +
>> > > > +**/
>> > > > +STATIC
>> > > > +EFI_STATUS
>> > > > +EFIAPI
>> > > > +WdogRegisterHandler (
>> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
>> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
>> > > > + )
>> > > > +{
>> > > > + // ERROR: This function is not supported.
>> > > > + // The hardware watchdog will reset the board
>> > > > + return EFI_INVALID_PARAMETER;
>> > >
>> > > Michael, Liming - what's your take on this?
>> > >
>> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
>pure-hw
>> > > watchdog such as this?
>> > >
>> > > If so, what would be a suitable return code here?
>> > > EFI_INVALID_PARAMETER does not look ideal.
>> > >
>> > > /
>> > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Liming,
On Thu, Dec 07, 2017 at 07:11:38AM +0000, Gao, Liming wrote:
> Leif:
> I don't see the core driver uses
> WatchdogTimer->RegisterHandler(). When it returns unsupported, it
> means the additional handler can't be registered. DxeCore uses
> WatchdogTimer->SetTimerPeriod(). This service is implemented in
> your driver.
>
> Watchdog protocol is defined in PI spec. Spec describes that this
> protocol provides the services required to implement the Boot
> Service SetWatchdogTimer(). It provides a service to set the
> amount of time to wait before firing the watchdog timer, and it
> also provides a service to register a handler that is invoked when
> the watchdog timer fires. This protocol can implement the watchdog
> timer by using the event and timer Boot Services, or it can make
> use of custom hardware. If no handler has been registered, or the
> registered handler returns, then the system will be reset by
> calling the Runtime Service ResetSystem(). So, this protocol is
> required.
I am not disputing that the protocol is not required. I am suggesting
that this hardware watchdog _cannot_ be used to register a handler.
If this hardware watchdog does not get updated in time, that causes an
immediate hardware reset of the processor.
Because of this, I believe EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is not the
appropriate way to make use of it.
Please let me know whether you agree.
Regards,
Leif
> Thanks
> Liming
> >-----Original Message-----
> >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >Sent: Tuesday, December 05, 2017 7:06 PM
> >To: Udit Kumar <udit.kumar@nxp.com>
> >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> ><michael.d.kinney@intel.com>; Meenakshi Aggarwal
> ><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> >devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> >support for Watchdog driver
> >
> >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> >implementation
> >> > could return its status besides spec defined status.
> >>
> >> Thanks to help me , how core will treat this error
> >> 1/ Wdt not available
> >> 2/ ignoring this error
> >> 3/ core is not registering handler
> >> I guess 3 is valid,
> >
> >Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> > //
> > // Attempt to set the timeout
> > //
> > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> > MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
> >
> > //
> > // Check for errors
> > //
> > if (EFI_ERROR (Status)) {
> > return EFI_DEVICE_ERROR;
> > }
> >
> >The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
> >
> >> On side track, looks wdt is not used by core services then do we
> >> really need this as part of arch protocol ?
> >
> >Yes, that was ultimately what I was implying with my question
> >regarding whether this protocol is relevant for a watchdog that can
> >only ever reset the system on timeout.
> >
> >The protocol looks to me to be designed to use a dedicated generic
> >timer as backing for a software watchdog.
> >
> >Liming, Mike?
> >
> >If that is the case, then I agree this driver should probably not
> >implement this protocol, but rather set up a timer event (or a
> >dedicated timer) to stroke the watchdog.
> >
> >Regards,
> >
> >Leif
> >
> >> regards
> >> Udit
> >>
> >> > -----Original Message-----
> >> > From: Gao, Liming [mailto:liming.gao@intel.com]
> >> > Sent: Monday, December 04, 2017 8:53 PM
> >> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> >> > <michael.d.kinney@intel.com>
> >> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> >> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> >support
> >> > for Watchdog driver
> >> >
> >> > Leif:
> >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> >implementation
> >> > could return its status besides spec defined status.
> >> >
> >> > Thanks
> >> > Liming
> >> > > -----Original Message-----
> >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >> > > Sent: Monday, December 4, 2017 10:36 PM
> >> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> >> > > <liming.gao@intel.com>
> >> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> >> > > udit.kumar@nxp.com; v.sethi@nxp.com
> >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> >> > > support for Watchdog driver
> >> > >
> >> > > Mike, Liming, as MdePkg mainteiners - one question below:
> >> > >
> >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> >> > > > new file mode 100644
> >> > > > index 0000000..a9c70ef
> >> > > > --- /dev/null
> >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> >> > > > @@ -0,0 +1,421 @@
> >> > >
> >> > > ...
> >> > >
> >> > > > +/**
> >> > > > + This function registers the handler NotifyFunction so it is
> >> > > > +called every time
> >> > > > + the watchdog timer expires. It also passes the amount of time
> >> > > > +since the last
> >> > > > + handler call to the NotifyFunction.
> >> > > > + If NotifyFunction is not NULL and a handler is not already
> >> > > > +registered,
> >> > > > + then the new handler is registered and EFI_SUCCESS is returned.
> >> > > > + If NotifyFunction is NULL, and a handler is already registered,
> >> > > > + then that handler is unregistered.
> >> > > > + If an attempt is made to register a handler when a handler is
> >> > > > +already registered,
> >> > > > + then EFI_ALREADY_STARTED is returned.
> >> > > > + If an attempt is made to unregister a handler when a handler is
> >> > > > +not registered,
> >> > > > + then EFI_INVALID_PARAMETER is returned.
> >> > > > +
> >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> >> > > > + @param NotifyFunction The function to call when a timer interrupt
> >fires.
> >> > This
> >> > > > + function executes at TPL_HIGH_LEVEL. The DXE Core
> >will
> >> > > > + register a handler for the timer interrupt, so it can know
> >> > > > + how much time has passed. This information is used to
> >> > > > + signal timer based events. NULL will unregister the
> >handler.
> >> > > > +
> >> > > > + @retval EFI_SUCCESS The watchdog timer handler was
> >registered.
> >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> >> > handler is already
> >> > > > + registered.
> >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a
> >handler
> >> > was not
> >> > > > + previously registered.
> >> > > > +
> >> > > > +**/
> >> > > > +STATIC
> >> > > > +EFI_STATUS
> >> > > > +EFIAPI
> >> > > > +WdogRegisterHandler (
> >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> >> > > > + )
> >> > > > +{
> >> > > > + // ERROR: This function is not supported.
> >> > > > + // The hardware watchdog will reset the board
> >> > > > + return EFI_INVALID_PARAMETER;
> >> > >
> >> > > Michael, Liming - what's your take on this?
> >> > >
> >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
> >pure-hw
> >> > > watchdog such as this?
> >> > >
> >> > > If so, what would be a suitable return code here?
> >> > > EFI_INVALID_PARAMETER does not look ideal.
> >> > >
> >> > > /
> >> > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif:
I don't review the whole patch serial. Could you point your usage case on watch dog timer protocol?
Thanks
Liming
> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Thursday, December 7, 2017 7:04 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Udit Kumar <udit.kumar@nxp.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
>
> Hi Liming,
>
> On Thu, Dec 07, 2017 at 07:11:38AM +0000, Gao, Liming wrote:
> > Leif:
> > I don't see the core driver uses
> > WatchdogTimer->RegisterHandler(). When it returns unsupported, it
> > means the additional handler can't be registered. DxeCore uses
> > WatchdogTimer->SetTimerPeriod(). This service is implemented in
> > your driver.
> >
> > Watchdog protocol is defined in PI spec. Spec describes that this
> > protocol provides the services required to implement the Boot
> > Service SetWatchdogTimer(). It provides a service to set the
> > amount of time to wait before firing the watchdog timer, and it
> > also provides a service to register a handler that is invoked when
> > the watchdog timer fires. This protocol can implement the watchdog
> > timer by using the event and timer Boot Services, or it can make
> > use of custom hardware. If no handler has been registered, or the
> > registered handler returns, then the system will be reset by
> > calling the Runtime Service ResetSystem(). So, this protocol is
> > required.
>
> I am not disputing that the protocol is not required. I am suggesting
> that this hardware watchdog _cannot_ be used to register a handler.
>
> If this hardware watchdog does not get updated in time, that causes an
> immediate hardware reset of the processor.
>
> Because of this, I believe EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is not the
> appropriate way to make use of it.
>
> Please let me know whether you agree.
>
> Regards,
>
> Leif
>
> > Thanks
> > Liming
> > >-----Original Message-----
> > >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > >Sent: Tuesday, December 05, 2017 7:06 PM
> > >To: Udit Kumar <udit.kumar@nxp.com>
> > >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > ><michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > ><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> > >devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > >support for Watchdog driver
> > >
> > >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > >implementation
> > >> > could return its status besides spec defined status.
> > >>
> > >> Thanks to help me , how core will treat this error
> > >> 1/ Wdt not available
> > >> 2/ ignoring this error
> > >> 3/ core is not registering handler
> > >> I guess 3 is valid,
> > >
> > >Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> > > //
> > > // Attempt to set the timeout
> > > //
> > > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> > > MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
> > >
> > > //
> > > // Check for errors
> > > //
> > > if (EFI_ERROR (Status)) {
> > > return EFI_DEVICE_ERROR;
> > > }
> > >
> > >The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
> > >
> > >> On side track, looks wdt is not used by core services then do we
> > >> really need this as part of arch protocol ?
> > >
> > >Yes, that was ultimately what I was implying with my question
> > >regarding whether this protocol is relevant for a watchdog that can
> > >only ever reset the system on timeout.
> > >
> > >The protocol looks to me to be designed to use a dedicated generic
> > >timer as backing for a software watchdog.
> > >
> > >Liming, Mike?
> > >
> > >If that is the case, then I agree this driver should probably not
> > >implement this protocol, but rather set up a timer event (or a
> > >dedicated timer) to stroke the watchdog.
> > >
> > >Regards,
> > >
> > >Leif
> > >
> > >> regards
> > >> Udit
> > >>
> > >> > -----Original Message-----
> > >> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > >> > Sent: Monday, December 04, 2017 8:53 PM
> > >> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> > >> > <michael.d.kinney@intel.com>
> > >> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> > >> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > >support
> > >> > for Watchdog driver
> > >> >
> > >> > Leif:
> > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > >implementation
> > >> > could return its status besides spec defined status.
> > >> >
> > >> > Thanks
> > >> > Liming
> > >> > > -----Original Message-----
> > >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > >> > > Sent: Monday, December 4, 2017 10:36 PM
> > >> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > >> > > <liming.gao@intel.com>
> > >> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > >> > > udit.kumar@nxp.com; v.sethi@nxp.com
> > >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > >> > > support for Watchdog driver
> > >> > >
> > >> > > Mike, Liming, as MdePkg mainteiners - one question below:
> > >> > >
> > >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> > >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > >> > > > new file mode 100644
> > >> > > > index 0000000..a9c70ef
> > >> > > > --- /dev/null
> > >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > >> > > > @@ -0,0 +1,421 @@
> > >> > >
> > >> > > ...
> > >> > >
> > >> > > > +/**
> > >> > > > + This function registers the handler NotifyFunction so it is
> > >> > > > +called every time
> > >> > > > + the watchdog timer expires. It also passes the amount of time
> > >> > > > +since the last
> > >> > > > + handler call to the NotifyFunction.
> > >> > > > + If NotifyFunction is not NULL and a handler is not already
> > >> > > > +registered,
> > >> > > > + then the new handler is registered and EFI_SUCCESS is returned.
> > >> > > > + If NotifyFunction is NULL, and a handler is already registered,
> > >> > > > + then that handler is unregistered.
> > >> > > > + If an attempt is made to register a handler when a handler is
> > >> > > > +already registered,
> > >> > > > + then EFI_ALREADY_STARTED is returned.
> > >> > > > + If an attempt is made to unregister a handler when a handler is
> > >> > > > +not registered,
> > >> > > > + then EFI_INVALID_PARAMETER is returned.
> > >> > > > +
> > >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> > >> > > > + @param NotifyFunction The function to call when a timer interrupt
> > >fires.
> > >> > This
> > >> > > > + function executes at TPL_HIGH_LEVEL. The DXE Core
> > >will
> > >> > > > + register a handler for the timer interrupt, so it can know
> > >> > > > + how much time has passed. This information is used to
> > >> > > > + signal timer based events. NULL will unregister the
> > >handler.
> > >> > > > +
> > >> > > > + @retval EFI_SUCCESS The watchdog timer handler was
> > >registered.
> > >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> > >> > handler is already
> > >> > > > + registered.
> > >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a
> > >handler
> > >> > was not
> > >> > > > + previously registered.
> > >> > > > +
> > >> > > > +**/
> > >> > > > +STATIC
> > >> > > > +EFI_STATUS
> > >> > > > +EFIAPI
> > >> > > > +WdogRegisterHandler (
> > >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > >> > > > + )
> > >> > > > +{
> > >> > > > + // ERROR: This function is not supported.
> > >> > > > + // The hardware watchdog will reset the board
> > >> > > > + return EFI_INVALID_PARAMETER;
> > >> > >
> > >> > > Michael, Liming - what's your take on this?
> > >> > >
> > >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
> > >pure-hw
> > >> > > watchdog such as this?
> > >> > >
> > >> > > If so, what would be a suitable return code here?
> > >> > > EFI_INVALID_PARAMETER does not look ideal.
> > >> > >
> > >> > > /
> > >> > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Liming,
https://www.mail-archive.com/edk2-devel@lists.01.org/msg32761.html
Search for WdogRegisterHandler.
This topic is entirely unrelated to any _usage_ of watchdog timer
protocol.
The topic is only whether it is reasonable to _implement_
EFI_WATCHDOG_TIMER_ARCH_PROTOCOL for a hardware watchdog that *cannot*
cause a callback to a handler function.
Because when the hardware watchdog times out, it triggers a hard
system reset, without any software interaction.
/
Leif
On Thu, Dec 07, 2017 at 02:54:08PM +0000, Gao, Liming wrote:
> Leif:
> I don't review the whole patch serial. Could you point your usage
> case on watch dog timer protocol?
>
> Thanks
> Liming
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: Thursday, December 7, 2017 7:04 PM
> > To: Gao, Liming <liming.gao@intel.com>
> > Cc: Udit Kumar <udit.kumar@nxp.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
> >
> > Hi Liming,
> >
> > On Thu, Dec 07, 2017 at 07:11:38AM +0000, Gao, Liming wrote:
> > > Leif:
> > > I don't see the core driver uses
> > > WatchdogTimer->RegisterHandler(). When it returns unsupported, it
> > > means the additional handler can't be registered. DxeCore uses
> > > WatchdogTimer->SetTimerPeriod(). This service is implemented in
> > > your driver.
> > >
> > > Watchdog protocol is defined in PI spec. Spec describes that this
> > > protocol provides the services required to implement the Boot
> > > Service SetWatchdogTimer(). It provides a service to set the
> > > amount of time to wait before firing the watchdog timer, and it
> > > also provides a service to register a handler that is invoked when
> > > the watchdog timer fires. This protocol can implement the watchdog
> > > timer by using the event and timer Boot Services, or it can make
> > > use of custom hardware. If no handler has been registered, or the
> > > registered handler returns, then the system will be reset by
> > > calling the Runtime Service ResetSystem(). So, this protocol is
> > > required.
> >
> > I am not disputing that the protocol is not required. I am suggesting
> > that this hardware watchdog _cannot_ be used to register a handler.
> >
> > If this hardware watchdog does not get updated in time, that causes an
> > immediate hardware reset of the processor.
> >
> > Because of this, I believe EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is not the
> > appropriate way to make use of it.
> >
> > Please let me know whether you agree.
> >
> > Regards,
> >
> > Leif
> >
> > > Thanks
> > > Liming
> > > >-----Original Message-----
> > > >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > >Sent: Tuesday, December 05, 2017 7:06 PM
> > > >To: Udit Kumar <udit.kumar@nxp.com>
> > > >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > ><michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > > ><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> > > >devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > > >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > > >support for Watchdog driver
> > > >
> > > >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> > > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > > >implementation
> > > >> > could return its status besides spec defined status.
> > > >>
> > > >> Thanks to help me , how core will treat this error
> > > >> 1/ Wdt not available
> > > >> 2/ ignoring this error
> > > >> 3/ core is not registering handler
> > > >> I guess 3 is valid,
> > > >
> > > >Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> > > > //
> > > > // Attempt to set the timeout
> > > > //
> > > > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> > > > MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
> > > >
> > > > //
> > > > // Check for errors
> > > > //
> > > > if (EFI_ERROR (Status)) {
> > > > return EFI_DEVICE_ERROR;
> > > > }
> > > >
> > > >The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
> > > >
> > > >> On side track, looks wdt is not used by core services then do we
> > > >> really need this as part of arch protocol ?
> > > >
> > > >Yes, that was ultimately what I was implying with my question
> > > >regarding whether this protocol is relevant for a watchdog that can
> > > >only ever reset the system on timeout.
> > > >
> > > >The protocol looks to me to be designed to use a dedicated generic
> > > >timer as backing for a software watchdog.
> > > >
> > > >Liming, Mike?
> > > >
> > > >If that is the case, then I agree this driver should probably not
> > > >implement this protocol, but rather set up a timer event (or a
> > > >dedicated timer) to stroke the watchdog.
> > > >
> > > >Regards,
> > > >
> > > >Leif
> > > >
> > > >> regards
> > > >> Udit
> > > >>
> > > >> > -----Original Message-----
> > > >> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > > >> > Sent: Monday, December 04, 2017 8:53 PM
> > > >> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> > > >> > <michael.d.kinney@intel.com>
> > > >> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> > > >> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > > >support
> > > >> > for Watchdog driver
> > > >> >
> > > >> > Leif:
> > > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > > >implementation
> > > >> > could return its status besides spec defined status.
> > > >> >
> > > >> > Thanks
> > > >> > Liming
> > > >> > > -----Original Message-----
> > > >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > >> > > Sent: Monday, December 4, 2017 10:36 PM
> > > >> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > >> > > <liming.gao@intel.com>
> > > >> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > > >> > > udit.kumar@nxp.com; v.sethi@nxp.com
> > > >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > > >> > > support for Watchdog driver
> > > >> > >
> > > >> > > Mike, Liming, as MdePkg mainteiners - one question below:
> > > >> > >
> > > >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> > > >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > >> > > > new file mode 100644
> > > >> > > > index 0000000..a9c70ef
> > > >> > > > --- /dev/null
> > > >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > >> > > > @@ -0,0 +1,421 @@
> > > >> > >
> > > >> > > ...
> > > >> > >
> > > >> > > > +/**
> > > >> > > > + This function registers the handler NotifyFunction so it is
> > > >> > > > +called every time
> > > >> > > > + the watchdog timer expires. It also passes the amount of time
> > > >> > > > +since the last
> > > >> > > > + handler call to the NotifyFunction.
> > > >> > > > + If NotifyFunction is not NULL and a handler is not already
> > > >> > > > +registered,
> > > >> > > > + then the new handler is registered and EFI_SUCCESS is returned.
> > > >> > > > + If NotifyFunction is NULL, and a handler is already registered,
> > > >> > > > + then that handler is unregistered.
> > > >> > > > + If an attempt is made to register a handler when a handler is
> > > >> > > > +already registered,
> > > >> > > > + then EFI_ALREADY_STARTED is returned.
> > > >> > > > + If an attempt is made to unregister a handler when a handler is
> > > >> > > > +not registered,
> > > >> > > > + then EFI_INVALID_PARAMETER is returned.
> > > >> > > > +
> > > >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> > > >> > > > + @param NotifyFunction The function to call when a timer interrupt
> > > >fires.
> > > >> > This
> > > >> > > > + function executes at TPL_HIGH_LEVEL. The DXE Core
> > > >will
> > > >> > > > + register a handler for the timer interrupt, so it can know
> > > >> > > > + how much time has passed. This information is used to
> > > >> > > > + signal timer based events. NULL will unregister the
> > > >handler.
> > > >> > > > +
> > > >> > > > + @retval EFI_SUCCESS The watchdog timer handler was
> > > >registered.
> > > >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> > > >> > handler is already
> > > >> > > > + registered.
> > > >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a
> > > >handler
> > > >> > was not
> > > >> > > > + previously registered.
> > > >> > > > +
> > > >> > > > +**/
> > > >> > > > +STATIC
> > > >> > > > +EFI_STATUS
> > > >> > > > +EFIAPI
> > > >> > > > +WdogRegisterHandler (
> > > >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > > >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > > >> > > > + )
> > > >> > > > +{
> > > >> > > > + // ERROR: This function is not supported.
> > > >> > > > + // The hardware watchdog will reset the board
> > > >> > > > + return EFI_INVALID_PARAMETER;
> > > >> > >
> > > >> > > Michael, Liming - what's your take on this?
> > > >> > >
> > > >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
> > > >pure-hw
> > > >> > > watchdog such as this?
> > > >> > >
> > > >> > > If so, what would be a suitable return code here?
> > > >> > > EFI_INVALID_PARAMETER does not look ideal.
> > > >> > >
> > > >> > > /
> > > >> > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
> Because when the hardware watchdog times out, it triggers a hard system reset,
> without any software interaction.
Little more complexity around this piece of h/w
e.g once watchdog is started it cannot be stopped.
Most caller seems to set timeout of 5 mins and later stopping watchdog.
But actually watchdog is not stopped and OS needs to be loaded within this time or some
specific application needs to ping it.
Thx
Udit
> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Thursday, December 07, 2017 9:04 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Udit Kumar <udit.kumar@nxp.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support
> for Watchdog driver
>
> Liming,
>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> mail-archive.com%2Fedk2-
> devel%40lists.01.org%2Fmsg32761.html&data=02%7C01%7Cudit.kumar%40nxp
> .com%7Cb5a84bfc5cdc435a605e08d53d87ff95%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C636482576674878205&sdata=w3k%2B7Aw6D78uaTty
> GOh%2F8JUSiHVIdpCPkBudMth6m%2Fw%3D&reserved=0
> Search for WdogRegisterHandler.
>
> This topic is entirely unrelated to any _usage_ of watchdog timer protocol.
>
> The topic is only whether it is reasonable to _implement_
> EFI_WATCHDOG_TIMER_ARCH_PROTOCOL for a hardware watchdog that
> *cannot* cause a callback to a handler function.
> Because when the hardware watchdog times out, it triggers a hard system reset,
> without any software interaction.
>
> /
> Leif
>
> On Thu, Dec 07, 2017 at 02:54:08PM +0000, Gao, Liming wrote:
> > Leif:
> > I don't review the whole patch serial. Could you point your usage
> > case on watch dog timer protocol?
> >
> > Thanks
> > Liming
> > > -----Original Message-----
> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > Sent: Thursday, December 7, 2017 7:04 PM
> > > To: Gao, Liming <liming.gao@intel.com>
> > > Cc: Udit Kumar <udit.kumar@nxp.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org;
> > > edk2-devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> > > Add support for Watchdog driver
> > >
> > > Hi Liming,
> > >
> > > On Thu, Dec 07, 2017 at 07:11:38AM +0000, Gao, Liming wrote:
> > > > Leif:
> > > > I don't see the core driver uses
> > > > WatchdogTimer->RegisterHandler(). When it returns unsupported, it
> > > > means the additional handler can't be registered. DxeCore uses
> > > > WatchdogTimer->SetTimerPeriod(). This service is implemented in
> > > > your driver.
> > > >
> > > > Watchdog protocol is defined in PI spec. Spec describes that this
> > > > protocol provides the services required to implement the Boot
> > > > Service SetWatchdogTimer(). It provides a service to set the
> > > > amount of time to wait before firing the watchdog timer, and it
> > > > also provides a service to register a handler that is invoked when
> > > > the watchdog timer fires. This protocol can implement the watchdog
> > > > timer by using the event and timer Boot Services, or it can make
> > > > use of custom hardware. If no handler has been registered, or the
> > > > registered handler returns, then the system will be reset by
> > > > calling the Runtime Service ResetSystem(). So, this protocol is
> > > > required.
> > >
> > > I am not disputing that the protocol is not required. I am
> > > suggesting that this hardware watchdog _cannot_ be used to register a
> handler.
> > >
> > > If this hardware watchdog does not get updated in time, that causes
> > > an immediate hardware reset of the processor.
> > >
> > > Because of this, I believe EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is not
> > > the appropriate way to make use of it.
> > >
> > > Please let me know whether you agree.
> > >
> > > Regards,
> > >
> > > Leif
> > >
> > > > Thanks
> > > > Liming
> > > > >-----Original Message-----
> > > > >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > >Sent: Tuesday, December 05, 2017 7:06 PM
> > > > >To: Udit Kumar <udit.kumar@nxp.com>
> > > > >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > ><michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > > > ><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> > > > >devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > > > >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> > > > >Add support for Watchdog driver
> > > > >
> > > > >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> > > > >> > I suggest return EFI_UNSUPPORTED for this case. The
> > > > >> > protocol
> > > > >implementation
> > > > >> > could return its status besides spec defined status.
> > > > >>
> > > > >> Thanks to help me , how core will treat this error 1/ Wdt not
> > > > >> available 2/ ignoring this error 3/ core is not registering
> > > > >> handler I guess 3 is valid,
> > > > >
> > > > >Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> > > > > //
> > > > > // Attempt to set the timeout
> > > > > //
> > > > > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> > > > > MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
> > > > >
> > > > > //
> > > > > // Check for errors
> > > > > //
> > > > > if (EFI_ERROR (Status)) {
> > > > > return EFI_DEVICE_ERROR;
> > > > > }
> > > > >
> > > > >The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
> > > > >
> > > > >> On side track, looks wdt is not used by core services then do
> > > > >> we really need this as part of arch protocol ?
> > > > >
> > > > >Yes, that was ultimately what I was implying with my question
> > > > >regarding whether this protocol is relevant for a watchdog that
> > > > >can only ever reset the system on timeout.
> > > > >
> > > > >The protocol looks to me to be designed to use a dedicated
> > > > >generic timer as backing for a software watchdog.
> > > > >
> > > > >Liming, Mike?
> > > > >
> > > > >If that is the case, then I agree this driver should probably not
> > > > >implement this protocol, but rather set up a timer event (or a
> > > > >dedicated timer) to stroke the watchdog.
> > > > >
> > > > >Regards,
> > > > >
> > > > >Leif
> > > > >
> > > > >> regards
> > > > >> Udit
> > > > >>
> > > > >> > -----Original Message-----
> > > > >> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > > > >> > Sent: Monday, December 04, 2017 8:53 PM
> > > > >> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael
> > > > >> > D <michael.d.kinney@intel.com>
> > > > >> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > > >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit
> > > > >> > Kumar <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > > >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9]
> > > > >> > Platform/NXP : Add
> > > > >support
> > > > >> > for Watchdog driver
> > > > >> >
> > > > >> > Leif:
> > > > >> > I suggest return EFI_UNSUPPORTED for this case. The
> > > > >> > protocol
> > > > >implementation
> > > > >> > could return its status besides spec defined status.
> > > > >> >
> > > > >> > Thanks
> > > > >> > Liming
> > > > >> > > -----Original Message-----
> > > > >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > >> > > Sent: Monday, December 4, 2017 10:36 PM
> > > > >> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> > > > >> > > Liming <liming.gao@intel.com>
> > > > >> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > > >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > > > >> > > udit.kumar@nxp.com; v.sethi@nxp.com
> > > > >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9]
> > > > >> > > Platform/NXP : Add support for Watchdog driver
> > > > >> > >
> > > > >> > > Mike, Liming, as MdePkg mainteiners - one question below:
> > > > >> > >
> > > > >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal
> wrote:
> > > > >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > >> > > > new file mode 100644
> > > > >> > > > index 0000000..a9c70ef
> > > > >> > > > --- /dev/null
> > > > >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > >> > > > @@ -0,0 +1,421 @@
> > > > >> > >
> > > > >> > > ...
> > > > >> > >
> > > > >> > > > +/**
> > > > >> > > > + This function registers the handler NotifyFunction so
> > > > >> > > > +it is called every time
> > > > >> > > > + the watchdog timer expires. It also passes the amount
> > > > >> > > > +of time since the last
> > > > >> > > > + handler call to the NotifyFunction.
> > > > >> > > > + If NotifyFunction is not NULL and a handler is not
> > > > >> > > > +already registered,
> > > > >> > > > + then the new handler is registered and EFI_SUCCESS is returned.
> > > > >> > > > + If NotifyFunction is NULL, and a handler is already
> > > > >> > > > +registered,
> > > > >> > > > + then that handler is unregistered.
> > > > >> > > > + If an attempt is made to register a handler when a
> > > > >> > > > +handler is already registered,
> > > > >> > > > + then EFI_ALREADY_STARTED is returned.
> > > > >> > > > + If an attempt is made to unregister a handler when a
> > > > >> > > > +handler is not registered,
> > > > >> > > > + then EFI_INVALID_PARAMETER is returned.
> > > > >> > > > +
> > > > >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL
> instance.
> > > > >> > > > + @param NotifyFunction The function to call when a timer
> interrupt
> > > > >fires.
> > > > >> > This
> > > > >> > > > + function executes at
> > > > >> > > > + TPL_HIGH_LEVEL. The DXE Core
> > > > >will
> > > > >> > > > + register a handler for the timer interrupt, so it can
> know
> > > > >> > > > + how much time has passed. This information is
> used to
> > > > >> > > > + signal timer based events.
> > > > >> > > > + NULL will unregister the
> > > > >handler.
> > > > >> > > > +
> > > > >> > > > + @retval EFI_SUCCESS The watchdog timer handler was
> > > > >registered.
> > > > >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL,
> and a
> > > > >> > handler is already
> > > > >> > > > + registered.
> > > > >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL,
> > > > >> > > > + and a
> > > > >handler
> > > > >> > was not
> > > > >> > > > + previously registered.
> > > > >> > > > +
> > > > >> > > > +**/
> > > > >> > > > +STATIC
> > > > >> > > > +EFI_STATUS
> > > > >> > > > +EFIAPI
> > > > >> > > > +WdogRegisterHandler (
> > > > >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > > > >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > > > >> > > > + )
> > > > >> > > > +{
> > > > >> > > > + // ERROR: This function is not supported.
> > > > >> > > > + // The hardware watchdog will reset the board
> > > > >> > > > + return EFI_INVALID_PARAMETER;
> > > > >> > >
> > > > >> > > Michael, Liming - what's your take on this?
> > > > >> > >
> > > > >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
> > > > >pure-hw
> > > > >> > > watchdog such as this?
> > > > >> > >
> > > > >> > > If so, what would be a suitable return code here?
> > > > >> > > EFI_INVALID_PARAMETER does not look ideal.
> > > > >> > >
> > > > >> > > /
> > > > >> > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif:
I have no strong opinion. PI spec doesn't require WdogRegisterHandler must be supported. So, this implementation doesn't break spec. For this platform, if there is no register handler or no critical register handler, this Watchdog driver can be used.
Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif Lindholm
> Sent: Thursday, December 7, 2017 11:34 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
>
> Liming,
>
> https://www.mail-archive.com/edk2-devel@lists.01.org/msg32761.html
> Search for WdogRegisterHandler.
>
> This topic is entirely unrelated to any _usage_ of watchdog timer
> protocol.
>
> The topic is only whether it is reasonable to _implement_
> EFI_WATCHDOG_TIMER_ARCH_PROTOCOL for a hardware watchdog that *cannot*
> cause a callback to a handler function.
> Because when the hardware watchdog times out, it triggers a hard
> system reset, without any software interaction.
>
> /
> Leif
>
> On Thu, Dec 07, 2017 at 02:54:08PM +0000, Gao, Liming wrote:
> > Leif:
> > I don't review the whole patch serial. Could you point your usage
> > case on watch dog timer protocol?
> >
> > Thanks
> > Liming
> > > -----Original Message-----
> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > Sent: Thursday, December 7, 2017 7:04 PM
> > > To: Gao, Liming <liming.gao@intel.com>
> > > Cc: Udit Kumar <udit.kumar@nxp.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
> > >
> > > Hi Liming,
> > >
> > > On Thu, Dec 07, 2017 at 07:11:38AM +0000, Gao, Liming wrote:
> > > > Leif:
> > > > I don't see the core driver uses
> > > > WatchdogTimer->RegisterHandler(). When it returns unsupported, it
> > > > means the additional handler can't be registered. DxeCore uses
> > > > WatchdogTimer->SetTimerPeriod(). This service is implemented in
> > > > your driver.
> > > >
> > > > Watchdog protocol is defined in PI spec. Spec describes that this
> > > > protocol provides the services required to implement the Boot
> > > > Service SetWatchdogTimer(). It provides a service to set the
> > > > amount of time to wait before firing the watchdog timer, and it
> > > > also provides a service to register a handler that is invoked when
> > > > the watchdog timer fires. This protocol can implement the watchdog
> > > > timer by using the event and timer Boot Services, or it can make
> > > > use of custom hardware. If no handler has been registered, or the
> > > > registered handler returns, then the system will be reset by
> > > > calling the Runtime Service ResetSystem(). So, this protocol is
> > > > required.
> > >
> > > I am not disputing that the protocol is not required. I am suggesting
> > > that this hardware watchdog _cannot_ be used to register a handler.
> > >
> > > If this hardware watchdog does not get updated in time, that causes an
> > > immediate hardware reset of the processor.
> > >
> > > Because of this, I believe EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is not the
> > > appropriate way to make use of it.
> > >
> > > Please let me know whether you agree.
> > >
> > > Regards,
> > >
> > > Leif
> > >
> > > > Thanks
> > > > Liming
> > > > >-----Original Message-----
> > > > >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > >Sent: Tuesday, December 05, 2017 7:06 PM
> > > > >To: Udit Kumar <udit.kumar@nxp.com>
> > > > >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > ><michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > > > ><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> > > > >devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > > > >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > > > >support for Watchdog driver
> > > > >
> > > > >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> > > > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > > > >implementation
> > > > >> > could return its status besides spec defined status.
> > > > >>
> > > > >> Thanks to help me , how core will treat this error
> > > > >> 1/ Wdt not available
> > > > >> 2/ ignoring this error
> > > > >> 3/ core is not registering handler
> > > > >> I guess 3 is valid,
> > > > >
> > > > >Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> > > > > //
> > > > > // Attempt to set the timeout
> > > > > //
> > > > > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> > > > > MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
> > > > >
> > > > > //
> > > > > // Check for errors
> > > > > //
> > > > > if (EFI_ERROR (Status)) {
> > > > > return EFI_DEVICE_ERROR;
> > > > > }
> > > > >
> > > > >The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
> > > > >
> > > > >> On side track, looks wdt is not used by core services then do we
> > > > >> really need this as part of arch protocol ?
> > > > >
> > > > >Yes, that was ultimately what I was implying with my question
> > > > >regarding whether this protocol is relevant for a watchdog that can
> > > > >only ever reset the system on timeout.
> > > > >
> > > > >The protocol looks to me to be designed to use a dedicated generic
> > > > >timer as backing for a software watchdog.
> > > > >
> > > > >Liming, Mike?
> > > > >
> > > > >If that is the case, then I agree this driver should probably not
> > > > >implement this protocol, but rather set up a timer event (or a
> > > > >dedicated timer) to stroke the watchdog.
> > > > >
> > > > >Regards,
> > > > >
> > > > >Leif
> > > > >
> > > > >> regards
> > > > >> Udit
> > > > >>
> > > > >> > -----Original Message-----
> > > > >> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > > > >> > Sent: Monday, December 04, 2017 8:53 PM
> > > > >> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> > > > >> > <michael.d.kinney@intel.com>
> > > > >> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > > >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> > > > >> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > > >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > > > >support
> > > > >> > for Watchdog driver
> > > > >> >
> > > > >> > Leif:
> > > > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > > > >implementation
> > > > >> > could return its status besides spec defined status.
> > > > >> >
> > > > >> > Thanks
> > > > >> > Liming
> > > > >> > > -----Original Message-----
> > > > >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > >> > > Sent: Monday, December 4, 2017 10:36 PM
> > > > >> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > > >> > > <liming.gao@intel.com>
> > > > >> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > > >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > > > >> > > udit.kumar@nxp.com; v.sethi@nxp.com
> > > > >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > > > >> > > support for Watchdog driver
> > > > >> > >
> > > > >> > > Mike, Liming, as MdePkg mainteiners - one question below:
> > > > >> > >
> > > > >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> > > > >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > >> > > > new file mode 100644
> > > > >> > > > index 0000000..a9c70ef
> > > > >> > > > --- /dev/null
> > > > >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > >> > > > @@ -0,0 +1,421 @@
> > > > >> > >
> > > > >> > > ...
> > > > >> > >
> > > > >> > > > +/**
> > > > >> > > > + This function registers the handler NotifyFunction so it is
> > > > >> > > > +called every time
> > > > >> > > > + the watchdog timer expires. It also passes the amount of time
> > > > >> > > > +since the last
> > > > >> > > > + handler call to the NotifyFunction.
> > > > >> > > > + If NotifyFunction is not NULL and a handler is not already
> > > > >> > > > +registered,
> > > > >> > > > + then the new handler is registered and EFI_SUCCESS is returned.
> > > > >> > > > + If NotifyFunction is NULL, and a handler is already registered,
> > > > >> > > > + then that handler is unregistered.
> > > > >> > > > + If an attempt is made to register a handler when a handler is
> > > > >> > > > +already registered,
> > > > >> > > > + then EFI_ALREADY_STARTED is returned.
> > > > >> > > > + If an attempt is made to unregister a handler when a handler is
> > > > >> > > > +not registered,
> > > > >> > > > + then EFI_INVALID_PARAMETER is returned.
> > > > >> > > > +
> > > > >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> > > > >> > > > + @param NotifyFunction The function to call when a timer interrupt
> > > > >fires.
> > > > >> > This
> > > > >> > > > + function executes at TPL_HIGH_LEVEL. The DXE Core
> > > > >will
> > > > >> > > > + register a handler for the timer interrupt, so it can know
> > > > >> > > > + how much time has passed. This information is used to
> > > > >> > > > + signal timer based events. NULL will unregister the
> > > > >handler.
> > > > >> > > > +
> > > > >> > > > + @retval EFI_SUCCESS The watchdog timer handler was
> > > > >registered.
> > > > >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> > > > >> > handler is already
> > > > >> > > > + registered.
> > > > >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a
> > > > >handler
> > > > >> > was not
> > > > >> > > > + previously registered.
> > > > >> > > > +
> > > > >> > > > +**/
> > > > >> > > > +STATIC
> > > > >> > > > +EFI_STATUS
> > > > >> > > > +EFIAPI
> > > > >> > > > +WdogRegisterHandler (
> > > > >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > > > >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > > > >> > > > + )
> > > > >> > > > +{
> > > > >> > > > + // ERROR: This function is not supported.
> > > > >> > > > + // The hardware watchdog will reset the board
> > > > >> > > > + return EFI_INVALID_PARAMETER;
> > > > >> > >
> > > > >> > > Michael, Liming - what's your take on this?
> > > > >> > >
> > > > >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
> > > > >pure-hw
> > > > >> > > watchdog such as this?
> > > > >> > >
> > > > >> > > If so, what would be a suitable return code here?
> > > > >> > > EFI_INVALID_PARAMETER does not look ideal.
> > > > >> > >
> > > > >> > > /
> > > > >> > > Leif
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi,
There is no clean way to register a handler with this watchdog controller.
Even if we do then there are chances that false notification will be sent to the module which has registered a handler.
We can go ahead with this implementation, i assume and i will share new revision of patch replacing EFI_INVALID_PARAMETER with EFI_UNSUPPORTED.
Please share your feedback.
Thanks,
Meenakshi
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Gao, Liming
> Sent: Sunday, December 10, 2017 7:01 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
> devel@lists.01.org; ard.biesheuvel@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> Add support for Watchdog driver
>
> Leif:
> I have no strong opinion. PI spec doesn't require WdogRegisterHandler
> must be supported. So, this implementation doesn't break spec. For this
> platform, if there is no register handler or no critical register handler, this
> Watchdog driver can be used.
>
> Thanks
> Liming
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leif Lindholm
> > Sent: Thursday, December 7, 2017 11:34 PM
> > To: Gao, Liming <liming.gao@intel.com>
> > Cc: ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Kinney, Michael D
> <michael.d.kinney@intel.com>
> > Subject: Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> Add support for Watchdog driver
> >
> > Liming,
> >
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> ww.mail-archive.com%2Fedk2-
> devel%40lists.01.org%2Fmsg32761.html&data=02%7C01%7Cmeenakshi.aggar
> wal%40nxp.com%7C3b6ad3d8cfdd4a766c4a08d53fd23a09%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C0%7C636485094541353596&sdata=kEb4x9jl1ng
> %2FlumodoxsB5i4RD3NmTUgX9GN9KcKtkI%3D&reserved=0
> > Search for WdogRegisterHandler.
> >
> > This topic is entirely unrelated to any _usage_ of watchdog timer
> > protocol.
> >
> > The topic is only whether it is reasonable to _implement_
> > EFI_WATCHDOG_TIMER_ARCH_PROTOCOL for a hardware watchdog that
> *cannot*
> > cause a callback to a handler function.
> > Because when the hardware watchdog times out, it triggers a hard
> > system reset, without any software interaction.
> >
> > /
> > Leif
> >
> > On Thu, Dec 07, 2017 at 02:54:08PM +0000, Gao, Liming wrote:
> > > Leif:
> > > I don't review the whole patch serial. Could you point your usage
> > > case on watch dog timer protocol?
> > >
> > > Thanks
> > > Liming
> > > > -----Original Message-----
> > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > Sent: Thursday, December 7, 2017 7:04 PM
> > > > To: Gao, Liming <liming.gao@intel.com>
> > > > Cc: Udit Kumar <udit.kumar@nxp.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> Add support for Watchdog driver
> > > >
> > > > Hi Liming,
> > > >
> > > > On Thu, Dec 07, 2017 at 07:11:38AM +0000, Gao, Liming wrote:
> > > > > Leif:
> > > > > I don't see the core driver uses
> > > > > WatchdogTimer->RegisterHandler(). When it returns unsupported, it
> > > > > means the additional handler can't be registered. DxeCore uses
> > > > > WatchdogTimer->SetTimerPeriod(). This service is implemented in
> > > > > your driver.
> > > > >
> > > > > Watchdog protocol is defined in PI spec. Spec describes that this
> > > > > protocol provides the services required to implement the Boot
> > > > > Service SetWatchdogTimer(). It provides a service to set the
> > > > > amount of time to wait before firing the watchdog timer, and it
> > > > > also provides a service to register a handler that is invoked when
> > > > > the watchdog timer fires. This protocol can implement the watchdog
> > > > > timer by using the event and timer Boot Services, or it can make
> > > > > use of custom hardware. If no handler has been registered, or the
> > > > > registered handler returns, then the system will be reset by
> > > > > calling the Runtime Service ResetSystem(). So, this protocol is
> > > > > required.
> > > >
> > > > I am not disputing that the protocol is not required. I am suggesting
> > > > that this hardware watchdog _cannot_ be used to register a handler.
> > > >
> > > > If this hardware watchdog does not get updated in time, that causes an
> > > > immediate hardware reset of the processor.
> > > >
> > > > Because of this, I believe EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is
> not the
> > > > appropriate way to make use of it.
> > > >
> > > > Please let me know whether you agree.
> > > >
> > > > Regards,
> > > >
> > > > Leif
> > > >
> > > > > Thanks
> > > > > Liming
> > > > > >-----Original Message-----
> > > > > >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > > >Sent: Tuesday, December 05, 2017 7:06 PM
> > > > > >To: Udit Kumar <udit.kumar@nxp.com>
> > > > > >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > > > ><michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > > > > ><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> > > > > >devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > > > > >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> Add
> > > > > >support for Watchdog driver
> > > > > >
> > > > > >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> > > > > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > > > > >implementation
> > > > > >> > could return its status besides spec defined status.
> > > > > >>
> > > > > >> Thanks to help me , how core will treat this error
> > > > > >> 1/ Wdt not available
> > > > > >> 2/ ignoring this error
> > > > > >> 3/ core is not registering handler
> > > > > >> I guess 3 is valid,
> > > > > >
> > > > > >Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> > > > > > //
> > > > > > // Attempt to set the timeout
> > > > > > //
> > > > > > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> > > > > > MultU64x32 (Timeout,
> WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
> > > > > >
> > > > > > //
> > > > > > // Check for errors
> > > > > > //
> > > > > > if (EFI_ERROR (Status)) {
> > > > > > return EFI_DEVICE_ERROR;
> > > > > > }
> > > > > >
> > > > > >The SetWatchdogTimer() call would always return
> EFI_DEVICE_ERROR.
> > > > > >
> > > > > >> On side track, looks wdt is not used by core services then do we
> > > > > >> really need this as part of arch protocol ?
> > > > > >
> > > > > >Yes, that was ultimately what I was implying with my question
> > > > > >regarding whether this protocol is relevant for a watchdog that can
> > > > > >only ever reset the system on timeout.
> > > > > >
> > > > > >The protocol looks to me to be designed to use a dedicated generic
> > > > > >timer as backing for a software watchdog.
> > > > > >
> > > > > >Liming, Mike?
> > > > > >
> > > > > >If that is the case, then I agree this driver should probably not
> > > > > >implement this protocol, but rather set up a timer event (or a
> > > > > >dedicated timer) to stroke the watchdog.
> > > > > >
> > > > > >Regards,
> > > > > >
> > > > > >Leif
> > > > > >
> > > > > >> regards
> > > > > >> Udit
> > > > > >>
> > > > > >> > -----Original Message-----
> > > > > >> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > > > > >> > Sent: Monday, December 04, 2017 8:53 PM
> > > > > >> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> > > > > >> > <michael.d.kinney@intel.com>
> > > > > >> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > > > >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> > > > > >> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > > > >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9]
> Platform/NXP : Add
> > > > > >support
> > > > > >> > for Watchdog driver
> > > > > >> >
> > > > > >> > Leif:
> > > > > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > > > > >implementation
> > > > > >> > could return its status besides spec defined status.
> > > > > >> >
> > > > > >> > Thanks
> > > > > >> > Liming
> > > > > >> > > -----Original Message-----
> > > > > >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > > >> > > Sent: Monday, December 4, 2017 10:36 PM
> > > > > >> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> Liming
> > > > > >> > > <liming.gao@intel.com>
> > > > > >> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > > > > >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > > > > >> > > udit.kumar@nxp.com; v.sethi@nxp.com
> > > > > >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9]
> Platform/NXP : Add
> > > > > >> > > support for Watchdog driver
> > > > > >> > >
> > > > > >> > > Mike, Liming, as MdePkg mainteiners - one question below:
> > > > > >> > >
> > > > > >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi
> Aggarwal wrote:
> > > > > >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > > >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > > >> > > > new file mode 100644
> > > > > >> > > > index 0000000..a9c70ef
> > > > > >> > > > --- /dev/null
> > > > > >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > > >> > > > @@ -0,0 +1,421 @@
> > > > > >> > >
> > > > > >> > > ...
> > > > > >> > >
> > > > > >> > > > +/**
> > > > > >> > > > + This function registers the handler NotifyFunction so it is
> > > > > >> > > > +called every time
> > > > > >> > > > + the watchdog timer expires. It also passes the amount of
> time
> > > > > >> > > > +since the last
> > > > > >> > > > + handler call to the NotifyFunction.
> > > > > >> > > > + If NotifyFunction is not NULL and a handler is not already
> > > > > >> > > > +registered,
> > > > > >> > > > + then the new handler is registered and EFI_SUCCESS is
> returned.
> > > > > >> > > > + If NotifyFunction is NULL, and a handler is already
> registered,
> > > > > >> > > > + then that handler is unregistered.
> > > > > >> > > > + If an attempt is made to register a handler when a handler
> is
> > > > > >> > > > +already registered,
> > > > > >> > > > + then EFI_ALREADY_STARTED is returned.
> > > > > >> > > > + If an attempt is made to unregister a handler when a
> handler is
> > > > > >> > > > +not registered,
> > > > > >> > > > + then EFI_INVALID_PARAMETER is returned.
> > > > > >> > > > +
> > > > > >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL
> instance.
> > > > > >> > > > + @param NotifyFunction The function to call when a timer
> interrupt
> > > > > >fires.
> > > > > >> > This
> > > > > >> > > > + function executes at TPL_HIGH_LEVEL. The
> DXE Core
> > > > > >will
> > > > > >> > > > + register a handler for the timer interrupt, so it
> can know
> > > > > >> > > > + how much time has passed. This information is
> used to
> > > > > >> > > > + signal timer based events. NULL will unregister
> the
> > > > > >handler.
> > > > > >> > > > +
> > > > > >> > > > + @retval EFI_SUCCESS The watchdog timer handler
> was
> > > > > >registered.
> > > > > >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not
> NULL, and a
> > > > > >> > handler is already
> > > > > >> > > > + registered.
> > > > > >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL,
> and a
> > > > > >handler
> > > > > >> > was not
> > > > > >> > > > + previously registered.
> > > > > >> > > > +
> > > > > >> > > > +**/
> > > > > >> > > > +STATIC
> > > > > >> > > > +EFI_STATUS
> > > > > >> > > > +EFIAPI
> > > > > >> > > > +WdogRegisterHandler (
> > > > > >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > > > > >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > > > > >> > > > + )
> > > > > >> > > > +{
> > > > > >> > > > + // ERROR: This function is not supported.
> > > > > >> > > > + // The hardware watchdog will reset the board
> > > > > >> > > > + return EFI_INVALID_PARAMETER;
> > > > > >> > >
> > > > > >> > > Michael, Liming - what's your take on this?
> > > > > >> > >
> > > > > >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use
> with a
> > > > > >pure-hw
> > > > > >> > > watchdog such as this?
> > > > > >> > >
> > > > > >> > > If so, what would be a suitable return code here?
> > > > > >> > > EFI_INVALID_PARAMETER does not look ideal.
> > > > > >> > >
> > > > > >> > > /
> > > > > >> > > Leif
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C3b6ad3d8cfd
> d4a766c4a08d53fd23a09%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636485094541353596&sdata=PQunSMPo4zejX997rje2fys4r93Wnrogz1rEi
> vpcrLY%3D&reserved=0
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C3b6ad3d8cfd
> d4a766c4a08d53fd23a09%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636485094541353596&sdata=PQunSMPo4zejX997rje2fys4r93Wnrogz1rEi
> vpcrLY%3D&reserved=0
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Liming,
> DxeCore uses WatchdogTimer->SetTimerPeriod(). This service is implemented in
> your driver.
Callers of SetTimerPeriod are ignoring error reported.
Is they assume this call will be perfect or they are ok in case some error on watchdog service.
Regards
Udit
> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Thursday, December 07, 2017 12:42 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Udit Kumar
> <udit.kumar@nxp.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support
> for Watchdog driver
>
> Leif:
> I don't see the core driver uses WatchdogTimer->RegisterHandler(). When it
> returns unsupported, it means the additional handler can't be registered.
> DxeCore uses WatchdogTimer->SetTimerPeriod(). This service is implemented in
> your driver.
>
> Watchdog protocol is defined in PI spec. Spec describes that this protocol
> provides the services required to implement the Boot Service
> SetWatchdogTimer(). It provides a service to set the amount of time to wait
> before firing the watchdog timer, and it also provides a service to register a
> handler that is invoked when the watchdog timer fires. This protocol can
> implement the watchdog timer by using the event and timer Boot Services, or it
> can make use of custom hardware. If no handler has been registered, or the
> registered handler returns, then the system will be reset by calling the Runtime
> Service ResetSystem(). So, this protocol is required.
>
> Thanks
> Liming
> >-----Original Message-----
> >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >Sent: Tuesday, December 05, 2017 7:06 PM
> >To: Udit Kumar <udit.kumar@nxp.com>
> >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> ><michael.d.kinney@intel.com>; Meenakshi Aggarwal
> ><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> >devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> >support for Watchdog driver
> >
> >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> >implementation
> >> > could return its status besides spec defined status.
> >>
> >> Thanks to help me , how core will treat this error 1/ Wdt not
> >> available 2/ ignoring this error 3/ core is not registering handler I
> >> guess 3 is valid,
> >
> >Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> > //
> > // Attempt to set the timeout
> > //
> > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> > MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
> >
> > //
> > // Check for errors
> > //
> > if (EFI_ERROR (Status)) {
> > return EFI_DEVICE_ERROR;
> > }
> >
> >The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
> >
> >> On side track, looks wdt is not used by core services then do we
> >> really need this as part of arch protocol ?
> >
> >Yes, that was ultimately what I was implying with my question regarding
> >whether this protocol is relevant for a watchdog that can only ever
> >reset the system on timeout.
> >
> >The protocol looks to me to be designed to use a dedicated generic
> >timer as backing for a software watchdog.
> >
> >Liming, Mike?
> >
> >If that is the case, then I agree this driver should probably not
> >implement this protocol, but rather set up a timer event (or a
> >dedicated timer) to stroke the watchdog.
> >
> >Regards,
> >
> >Leif
> >
> >> regards
> >> Udit
> >>
> >> > -----Original Message-----
> >> > From: Gao, Liming [mailto:liming.gao@intel.com]
> >> > Sent: Monday, December 04, 2017 8:53 PM
> >> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> >> > <michael.d.kinney@intel.com>
> >> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> >> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> >> > Add
> >support
> >> > for Watchdog driver
> >> >
> >> > Leif:
> >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> >implementation
> >> > could return its status besides spec defined status.
> >> >
> >> > Thanks
> >> > Liming
> >> > > -----Original Message-----
> >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> >> > > Sent: Monday, December 4, 2017 10:36 PM
> >> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> >> > > <liming.gao@intel.com>
> >> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> >> > > udit.kumar@nxp.com; v.sethi@nxp.com
> >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> >> > > Add support for Watchdog driver
> >> > >
> >> > > Mike, Liming, as MdePkg mainteiners - one question below:
> >> > >
> >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> >> > > > new file mode 100644
> >> > > > index 0000000..a9c70ef
> >> > > > --- /dev/null
> >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> >> > > > @@ -0,0 +1,421 @@
> >> > >
> >> > > ...
> >> > >
> >> > > > +/**
> >> > > > + This function registers the handler NotifyFunction so it is
> >> > > > +called every time
> >> > > > + the watchdog timer expires. It also passes the amount of
> >> > > > +time since the last
> >> > > > + handler call to the NotifyFunction.
> >> > > > + If NotifyFunction is not NULL and a handler is not already
> >> > > > +registered,
> >> > > > + then the new handler is registered and EFI_SUCCESS is returned.
> >> > > > + If NotifyFunction is NULL, and a handler is already
> >> > > > +registered,
> >> > > > + then that handler is unregistered.
> >> > > > + If an attempt is made to register a handler when a handler
> >> > > > +is already registered,
> >> > > > + then EFI_ALREADY_STARTED is returned.
> >> > > > + If an attempt is made to unregister a handler when a handler
> >> > > > +is not registered,
> >> > > > + then EFI_INVALID_PARAMETER is returned.
> >> > > > +
> >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> >> > > > + @param NotifyFunction The function to call when a timer interrupt
> >fires.
> >> > This
> >> > > > + function executes at
> >> > > > + TPL_HIGH_LEVEL. The DXE Core
> >will
> >> > > > + register a handler for the timer interrupt, so it can know
> >> > > > + how much time has passed. This information is used to
> >> > > > + signal timer based events. NULL
> >> > > > + will unregister the
> >handler.
> >> > > > +
> >> > > > + @retval EFI_SUCCESS The watchdog timer handler was
> >registered.
> >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> >> > handler is already
> >> > > > + registered.
> >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a
> >handler
> >> > was not
> >> > > > + previously registered.
> >> > > > +
> >> > > > +**/
> >> > > > +STATIC
> >> > > > +EFI_STATUS
> >> > > > +EFIAPI
> >> > > > +WdogRegisterHandler (
> >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> >> > > > + )
> >> > > > +{
> >> > > > + // ERROR: This function is not supported.
> >> > > > + // The hardware watchdog will reset the board
> >> > > > + return EFI_INVALID_PARAMETER;
> >> > >
> >> > > Michael, Liming - what's your take on this?
> >> > >
> >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
> >pure-hw
> >> > > watchdog such as this?
> >> > >
> >> > > If so, what would be a suitable return code here?
> >> > > EFI_INVALID_PARAMETER does not look ideal.
> >> > >
> >> > > /
> >> > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
CoreSetWatchdogTimer() allows the error status for watchdog service.
> -----Original Message-----
> From: Udit Kumar [mailto:udit.kumar@nxp.com]
> Sent: Thursday, December 7, 2017 7:15 PM
> To: Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
>
> Hi Liming,
>
> > DxeCore uses WatchdogTimer->SetTimerPeriod(). This service is implemented in
> > your driver.
>
> Callers of SetTimerPeriod are ignoring error reported.
> Is they assume this call will be perfect or they are ok in case some error on watchdog service.
>
>
> Regards
> Udit
>
> > -----Original Message-----
> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > Sent: Thursday, December 07, 2017 12:42 PM
> > To: Leif Lindholm <leif.lindholm@linaro.org>; Udit Kumar
> > <udit.kumar@nxp.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> > devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support
> > for Watchdog driver
> >
> > Leif:
> > I don't see the core driver uses WatchdogTimer->RegisterHandler(). When it
> > returns unsupported, it means the additional handler can't be registered.
> > DxeCore uses WatchdogTimer->SetTimerPeriod(). This service is implemented in
> > your driver.
> >
> > Watchdog protocol is defined in PI spec. Spec describes that this protocol
> > provides the services required to implement the Boot Service
> > SetWatchdogTimer(). It provides a service to set the amount of time to wait
> > before firing the watchdog timer, and it also provides a service to register a
> > handler that is invoked when the watchdog timer fires. This protocol can
> > implement the watchdog timer by using the event and timer Boot Services, or it
> > can make use of custom hardware. If no handler has been registered, or the
> > registered handler returns, then the system will be reset by calling the Runtime
> > Service ResetSystem(). So, this protocol is required.
> >
> > Thanks
> > Liming
> > >-----Original Message-----
> > >From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > >Sent: Tuesday, December 05, 2017 7:06 PM
> > >To: Udit Kumar <udit.kumar@nxp.com>
> > >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > ><michael.d.kinney@intel.com>; Meenakshi Aggarwal
> > ><meenakshi.aggarwal@nxp.com>; ard.biesheuvel@linaro.org; edk2-
> > >devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>
> > >Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > >support for Watchdog driver
> > >
> > >On Tue, Dec 05, 2017 at 05:07:00AM +0000, Udit Kumar wrote:
> > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > >implementation
> > >> > could return its status besides spec defined status.
> > >>
> > >> Thanks to help me , how core will treat this error 1/ Wdt not
> > >> available 2/ ignoring this error 3/ core is not registering handler I
> > >> guess 3 is valid,
> > >
> > >Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
> > > //
> > > // Attempt to set the timeout
> > > //
> > > Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
> > > MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));
> > >
> > > //
> > > // Check for errors
> > > //
> > > if (EFI_ERROR (Status)) {
> > > return EFI_DEVICE_ERROR;
> > > }
> > >
> > >The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.
> > >
> > >> On side track, looks wdt is not used by core services then do we
> > >> really need this as part of arch protocol ?
> > >
> > >Yes, that was ultimately what I was implying with my question regarding
> > >whether this protocol is relevant for a watchdog that can only ever
> > >reset the system on timeout.
> > >
> > >The protocol looks to me to be designed to use a dedicated generic
> > >timer as backing for a software watchdog.
> > >
> > >Liming, Mike?
> > >
> > >If that is the case, then I agree this driver should probably not
> > >implement this protocol, but rather set up a timer event (or a
> > >dedicated timer) to stroke the watchdog.
> > >
> > >Regards,
> > >
> > >Leif
> > >
> > >> regards
> > >> Udit
> > >>
> > >> > -----Original Message-----
> > >> > From: Gao, Liming [mailto:liming.gao@intel.com]
> > >> > Sent: Monday, December 04, 2017 8:53 PM
> > >> > To: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> > >> > <michael.d.kinney@intel.com>
> > >> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > >> > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> > >> > <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > >> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> > >> > Add
> > >support
> > >> > for Watchdog driver
> > >> >
> > >> > Leif:
> > >> > I suggest return EFI_UNSUPPORTED for this case. The protocol
> > >implementation
> > >> > could return its status besides spec defined status.
> > >> >
> > >> > Thanks
> > >> > Liming
> > >> > > -----Original Message-----
> > >> > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > >> > > Sent: Monday, December 4, 2017 10:36 PM
> > >> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > >> > > <liming.gao@intel.com>
> > >> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > >> > > ard.biesheuvel@linaro.org; edk2-devel@lists.01.org;
> > >> > > udit.kumar@nxp.com; v.sethi@nxp.com
> > >> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP :
> > >> > > Add support for Watchdog driver
> > >> > >
> > >> > > Mike, Liming, as MdePkg mainteiners - one question below:
> > >> > >
> > >> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> > >> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > >> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > >> > > > new file mode 100644
> > >> > > > index 0000000..a9c70ef
> > >> > > > --- /dev/null
> > >> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > >> > > > @@ -0,0 +1,421 @@
> > >> > >
> > >> > > ...
> > >> > >
> > >> > > > +/**
> > >> > > > + This function registers the handler NotifyFunction so it is
> > >> > > > +called every time
> > >> > > > + the watchdog timer expires. It also passes the amount of
> > >> > > > +time since the last
> > >> > > > + handler call to the NotifyFunction.
> > >> > > > + If NotifyFunction is not NULL and a handler is not already
> > >> > > > +registered,
> > >> > > > + then the new handler is registered and EFI_SUCCESS is returned.
> > >> > > > + If NotifyFunction is NULL, and a handler is already
> > >> > > > +registered,
> > >> > > > + then that handler is unregistered.
> > >> > > > + If an attempt is made to register a handler when a handler
> > >> > > > +is already registered,
> > >> > > > + then EFI_ALREADY_STARTED is returned.
> > >> > > > + If an attempt is made to unregister a handler when a handler
> > >> > > > +is not registered,
> > >> > > > + then EFI_INVALID_PARAMETER is returned.
> > >> > > > +
> > >> > > > + @param This The EFI_TIMER_ARCH_PROTOCOL instance.
> > >> > > > + @param NotifyFunction The function to call when a timer interrupt
> > >fires.
> > >> > This
> > >> > > > + function executes at
> > >> > > > + TPL_HIGH_LEVEL. The DXE Core
> > >will
> > >> > > > + register a handler for the timer interrupt, so it can know
> > >> > > > + how much time has passed. This information is used to
> > >> > > > + signal timer based events. NULL
> > >> > > > + will unregister the
> > >handler.
> > >> > > > +
> > >> > > > + @retval EFI_SUCCESS The watchdog timer handler was
> > >registered.
> > >> > > > + @retval EFI_ALREADY_STARTED NotifyFunction is not NULL, and a
> > >> > handler is already
> > >> > > > + registered.
> > >> > > > + @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a
> > >handler
> > >> > was not
> > >> > > > + previously registered.
> > >> > > > +
> > >> > > > +**/
> > >> > > > +STATIC
> > >> > > > +EFI_STATUS
> > >> > > > +EFIAPI
> > >> > > > +WdogRegisterHandler (
> > >> > > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This,
> > >> > > > + IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> > >> > > > + )
> > >> > > > +{
> > >> > > > + // ERROR: This function is not supported.
> > >> > > > + // The hardware watchdog will reset the board
> > >> > > > + return EFI_INVALID_PARAMETER;
> > >> > >
> > >> > > Michael, Liming - what's your take on this?
> > >> > >
> > >> > > Is EFI_WATCHDOG_TIMER_ARCH_PROTOCOL suitable for use with a
> > >pure-hw
> > >> > > watchdog such as this?
> > >> > >
> > >> > > If so, what would be a suitable return code here?
> > >> > > EFI_INVALID_PARAMETER does not look ideal.
> > >> > >
> > >> > > /
> > >> > > Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2026 Red Hat, Inc.