Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver

[Udit Kumar <udit.kumar@nxp.com>]

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