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
