> 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.lindh...@linaro.org] > Sent: Thursday, December 07, 2017 9:04 PM > To: Gao, Liming <liming....@intel.com> > Cc: Udit Kumar <udit.ku...@nxp.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; Meenakshi Aggarwal > <meenakshi.aggar...@nxp.com>; ard.biesheu...@linaro.org; edk2- > de...@lists.01.org; Varun Sethi <v.se...@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.lindh...@linaro.org] > > > Sent: Thursday, December 7, 2017 7:04 PM > > > To: Gao, Liming <liming....@intel.com> > > > Cc: Udit Kumar <udit.ku...@nxp.com>; Kinney, Michael D > > > <michael.d.kin...@intel.com>; Meenakshi Aggarwal > > > <meenakshi.aggar...@nxp.com>; ard.biesheu...@linaro.org; > > > edk2-devel@lists.01.org; Varun Sethi <v.se...@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.lindh...@linaro.org] > > > > >Sent: Tuesday, December 05, 2017 7:06 PM > > > > >To: Udit Kumar <udit.ku...@nxp.com> > > > > >Cc: Gao, Liming <liming....@intel.com>; Kinney, Michael D > > > > ><michael.d.kin...@intel.com>; Meenakshi Aggarwal > > > > ><meenakshi.aggar...@nxp.com>; ard.biesheu...@linaro.org; edk2- > > > > >de...@lists.01.org; Varun Sethi <v.se...@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....@intel.com] > > > > >> > Sent: Monday, December 04, 2017 8:53 PM > > > > >> > To: Leif Lindholm <leif.lindh...@linaro.org>; Kinney, Michael > > > > >> > D <michael.d.kin...@intel.com> > > > > >> > Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; > > > > >> > ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Udit > > > > >> > Kumar <udit.ku...@nxp.com>; Varun Sethi <v.se...@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.lindh...@linaro.org] > > > > >> > > Sent: Monday, December 4, 2017 10:36 PM > > > > >> > > To: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, > > > > >> > > Liming <liming....@intel.com> > > > > >> > > Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; > > > > >> > > ard.biesheu...@linaro.org; edk2-devel@lists.01.org; > > > > >> > > udit.ku...@nxp.com; v.se...@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
