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....@intel.com] > Sent: Thursday, December 07, 2017 12:42 PM > To: Leif Lindholm <leif.lindh...@linaro.org>; Udit Kumar > <udit.ku...@nxp.com> > Cc: 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 > > 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.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
