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

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

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