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

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

2017-12-13 Thread Meenakshi Aggarwal

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

2017-12-10 Thread Gao, Liming

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

2017-12-07 Thread Udit Kumar

> 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

2017-12-07 Thread Leif Lindholm

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

2017-12-07 Thread Gao, Liming

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

2017-12-07 Thread Gao, Liming

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

2017-12-07 Thread Udit Kumar

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

2017-12-07 Thread Leif Lindholm

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

2017-12-06 Thread Meenakshi Aggarwal

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

2017-12-05 Thread Leif Lindholm

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

2017-12-04 Thread Udit Kumar

>   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

2017-12-04 Thread Leif Lindholm

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

2017-11-26 Thread Meenakshi Aggarwal

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
+

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

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

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

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

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

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

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

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

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

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

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

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

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

13 matches

Site Navigation

Mail list logo

Footer information