Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
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-boun...@lists.01.org] On Behalf Of > Gao, Liming > Sent: Sunday, December 10, 2017 7:01 PM > To: Leif Lindholm <leif.lindh...@linaro.org> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; edk2- > de...@lists.01.org; ard.biesheu...@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-boun...@lists.01.org] On Behalf Of > Leif Lindholm > > Sent: Thursday, December 7, 2017 11:34 PM > > To: Gao, Liming <liming@intel.com> > > Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Kinney, Michael D > <michael.d.kin...@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=02%7C01%7Cmeenakshi.aggar > wal%40nxp.com%7C3b6ad3d8cfdd4a766c4a08d53fd23a09%7C686ea1d3bc2b4 > c6fa92cd99c5c301635%7C0%7C0%7C636485094541353596=kEb4x9jl1ng > %2FlumodoxsB5i4RD3NmTUgX9GN9KcKtkI%3D=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 +, 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- > 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 > > > > > > > > Hi Liming, > > > > > > > > On Thu, Dec 07, 2017 at 07:11:38AM +, 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 > >
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-boun...@lists.01.org] On Behalf Of Leif > Lindholm > Sent: Thursday, December 7, 2017 11:34 PM > To: Gao, Liming <liming@intel.com> > Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Kinney, Michael D > <michael.d.kin...@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 +, 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 +, 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> > > > >
Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
> 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=02%7C01%7Cudit.kumar%40nxp > .com%7Cb5a84bfc5cdc435a605e08d53d87ff95%7C686ea1d3bc2b4c6fa92cd99c > 5c301635%7C0%7C0%7C636482576674878205=w3k%2B7Aw6D78uaTty > GOh%2F8JUSiHVIdpCPkBudMth6m%2Fw%3D=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 +, 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 +, 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
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 +, 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 +, 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 +, 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 > > > &
Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
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 +, 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 +, 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
Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
CoreSetWatchdogTimer() allows the error status for watchdog service. > -Original Message- > From: Udit Kumar [mailto:udit.ku...@nxp.com] > Sent: Thursday, December 7, 2017 7:15 PM > To: Gao, Liming <liming@intel.com>; Leif Lindholm > <leif.lindh...@linaro.org> > Cc: 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, > > > 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 +, 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 > >
Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
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 +, 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 > &
Re: [edk2] [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 +, 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 +, 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 > &g
Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
Hi Liming, Mike, Please share your inputs on this. Thanks, Meenakshi > -Original Message- > From: Leif Lindholm [mailto:leif.lindh...@linaro.org] > Sent: Tuesday, December 05, 2017 4:36 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 +, 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 000..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 >
Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
On Tue, Dec 05, 2017 at 05:07:00AM +, 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 000..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 inter
Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
> 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, On side track, looks wdt is not used by core services then do we really need this as part of arch protocol ? 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 000..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
Re: [edk2] [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 000..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
[edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver
Installs watchdog timer arch protocol Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Meenakshi Aggarwal--- Platform/NXP/Drivers/WatchDog/WatchDog.c | 421 ++ Platform/NXP/Drivers/WatchDog/WatchDog.h | 37 +++ Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf | 47 +++ 3 files changed, 505 insertions(+) create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDog.c create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDog.h create mode 100644 Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c b/Platform/NXP/Drivers/WatchDog/WatchDog.c new file mode 100644 index 000..a9c70ef --- /dev/null +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c @@ -0,0 +1,421 @@ +/** WatchDog.c +* +* Based on Watchdog driver implemenation available in +* ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c +* +* Copyright (c) 2011-2013, ARM Limited. All rights reserved. +* Copyright 2017 NXP +* +* This program and the accompanying materials +* are licensed and made available under the terms and conditions of the BSD License +* which accompanies this distribution. The full text of the license may be found at +* http://opensource.org/licenses/bsd-license.php +* +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +* +**/ + +#include +#include +#include +#include +#include +#include +#include +#include + +#include "WatchDog.h" + +STATIC EFI_EVENT EfiExitBootServicesEvent; + +STATIC +UINT16 +EFIAPI +WdogRead ( + IN UINTN Address + ) +{ + if (FixedPcdGetBool (PcdWdogBigEndian)) { +return BeMmioRead16 (Address); + } else { +return MmioRead16(Address); + } +} + +STATIC +UINT16 +EFIAPI +WdogWrite ( + IN UINTN Address, + IN UINT16Value + ) +{ + if (FixedPcdGetBool (PcdWdogBigEndian)) { +return BeMmioWrite16 (Address, Value); + } else { +return MmioWrite16 (Address, Value); + } +} + +STATIC +UINT16 +EFIAPI +WdogAndThenOr ( + IN UINTN Address, + IN UINT16And, + IN UINT16Or + ) +{ + if (FixedPcdGetBool (PcdWdogBigEndian)) { +return BeMmioAndThenOr16 (Address, And, Or); + } else { +return MmioAndThenOr16 (Address, And, Or); + } +} + +STATIC +UINT16 +EFIAPI +WdogOr ( + IN UINTN Address, + IN UINT16Or + ) +{ + if (FixedPcdGetBool (PcdWdogBigEndian)) { +return BeMmioOr16 (Address, Or); + } else { +return MmioOr16 (Address, Or); + } +} + +STATIC +VOID +WdogPing ( + VOID + ) +{ + // + // To reload a timeout value to the counter the proper service sequence begins by + // writing 0x_ followed by 0x_ to the Watchdog Service Register (WDOG_WSR). + // This service sequence will reload the counter with the timeout value WT[7:0] of + // Watchdog Control Register (WDOG_WCR). + // + + WdogWrite (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WSR_OFFSET, + WDOG_SERVICE_SEQ1); + WdogWrite (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WSR_OFFSET, + WDOG_SERVICE_SEQ2); +} + +/** + Stop the Wdog watchdog timer from counting down. +**/ +STATIC +VOID +WdogStop ( + VOID + ) +{ + // Watchdog cannot be disabled by software once started. + // At best, we can keep reload counter with maximum value + + WdogAndThenOr (PcdGet64 (PcdWdog1BaseAddr) + WDOG_WCR_OFFSET, + (UINT16)(~WDOG_WCR_WT), + (WD_COUNT (WT_MAX_TIME) & WD_COUNT_MASK)); + WdogPing (); +} + +/** + Starts the Wdog counting down by feeding Service register with + desired pattern. + The count down will start from the value stored in the Load register, + not from the value where it was previously stopped. +**/ +STATIC +VOID +WdogStart ( + VOID + ) +{ + //Reload the timeout value + WdogPing (); +} + +/** +On exiting boot services we must make sure the Wdog Watchdog Timer +is stopped. +**/ +STATIC +VOID +EFIAPI +ExitBootServicesEvent ( + IN EFI_EVENT Event, + IN VOID *Context + ) +{ + WdogStop (); +} + +/** + 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 +