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

["Gao, Liming" <liming.gao@intel.com>]

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