SP805 driver

SP805 driver

[Leif Lindholm]

Hi Sami, Thomas, (and others on cc)

NXP are upstreaming a set containing an implementation of
EFI_WATCHDOG_TIMER_ARCH_PROTOCOL using a hardware watchdog as backing.
This idea comes from the SP805
driver ArmPlatformPkg/Drivers/SP805WatchdogDxe, which does the same.

The problem is that this is a horrible idea. The point of
the EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is that it lets you schedule software
events when the watchdog timer expires. However, the SP805 driver does not
let you do this:

EFI_STATUS
EFIAPI
SP805RegisterHandler (
  IN CONST 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;
}

>From section 12.14 of the PI 1.6 spec:
"If no handler has been registered, or the registered handler returns, then
the system will be reset by calling the Runtime Service ResetSystem()."
Blatantly, any driver that does the above (and initializes hardware that
will reset the system without software control) will violate this.

Meanwhile, the only two ARM platforms that include this driver are TC2 and
FVP.

Now, NXP are not at fault for following examples given to them in the
reference ARM driver section - but can we please keep further people from
making the same mistake?
So my question to {Sami|Thomas} is - can we nuke this driver, and if so,
can you provide the patches to remove it from edk2 and the resulting ones
needed for edk2-platforms?

I have no issues with reintroducing a fixed SP805 driver in the future that
does not claim to conform to the above protocol.

Finally - both the D03 and D05 platform .dsc files, as well as the
Hisilicon ArmPlatformLib, contain references to it. (Ming/Heyi - please
provide a patch.)

/
    Leif

Re: SP805 driver

[Udit Kumar]

Thanks for note Leif,

I am trying to understand this behavior  of specification.

>"If no handler has been registered, or the registered handler returns, then the system will be reset by calling the Runtime Service ResetSystem()."
I believe,  this handler needs to be called by wdt driver after wdt is expired. Meaning , we want wdt to work without resetting the system.
Therefore a mask is needed with wdt, which will prevent to reset the system.   I see it working, at least for SP805, because of PMU mask bits.

Also ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c has this limitation.
I haven’t read spec of this wdt.  I hope there should a mask around.

Coming back to hardware, which does not have mask around wdt, how to implement this feature.


Regards
Udit


From: Leif Lindholm <leif.lindholm@linaro.org>
Sent: Monday, December 17, 2018 6:25 PM
To: Sami Mujawar <sami.mujawar@arm.com>; Thomas Panakamattam Abraham <thomas.abraham@arm.com>
Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Udit Kumar <udit.kumar@nxp.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel (edk2-devel@lists.01.org) <edk2-devel@lists.01.org>; Ming Huang <ming.huang@linaro.org>; Heyi Guo <heyi.guo@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>; Nariman Poushin <nariman.poushin@linaro.org>
Subject: SP805 driver

Hi Sami, Thomas, (and others on cc)

NXP are upstreaming a set containing an implementation of
EFI_WATCHDOG_TIMER_ARCH_PROTOCOL using a hardware watchdog as backing.
This idea comes from the SP805 driver ArmPlatformPkg/Drivers/SP805WatchdogDxe, which does the same.

The problem is that this is a horrible idea. The point of the EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is that it lets you schedule software events when the watchdog timer expires. However, the SP805 driver does not let you do this:

EFI_STATUS
EFIAPI
SP805RegisterHandler (
  IN CONST 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;
}

From section 12.14 of the PI 1.6 spec:
"If no handler has been registered, or the registered handler returns, then the system will be reset by calling the Runtime Service ResetSystem()."
Blatantly, any driver that does the above (and initializes hardware that will reset the system without software control) will violate this.

Meanwhile, the only two ARM platforms that include this driver are TC2 and FVP.

Now, NXP are not at fault for following examples given to them in the reference ARM driver section - but can we please keep further people from making the same mistake?
So my question to {Sami|Thomas} is - can we nuke this driver, and if so, can you provide the patches to remove it from edk2 and the resulting ones needed for edk2-platforms?

I have no issues with reintroducing a fixed SP805 driver in the future that does not claim to conform to the above protocol.

Finally - both the D03 and D05 platform .dsc files, as well as the Hisilicon ArmPlatformLib, contain references to it. (Ming/Heyi - please provide a patch.)

/
    Leif

Re: SP805 driver

[Leif Lindholm]

On Tue, Dec 18, 2018 at 05:20:59AM +0000, Udit Kumar wrote:
> Thanks for note Leif,
> 
> I am trying to understand this behavior  of specification.
> 
> >"If no handler has been registered, or the registered handler
> >returns, then the system will be reset by calling the Runtime
> >Service ResetSystem()."
> I believe,  this handler needs to be called by wdt driver after wdt
> is expired. Meaning , we want wdt to work without resetting the
> system.
> Therefore a mask is needed with wdt, which will prevent to reset the
> system.   I see it working, at least for SP805, because of PMU mask
> bits.

Yes, if the WDT can be configured to generate an interrupt instead of
a hardware reset, it can be used to implement this protocol.

> Also ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c has this limitation.
> I haven’t read spec of this wdt.  I hope there should a mask around.

No, the situation for the GenericWatchdogDxe is not as dire.

Correct: it does not permit registering software handlers (which
perhaps we should do something about, but is ... acceptable).

_But_, it still conforms to the above text; when the timer expires,
it resets the system by calling the ResetSystem() service. It does not
directly force a hardware reset.

The severe problem is not the lack of the ability to register the
software handler (which does remove much of the utility), but the
removal of reset control from the system firmware.

> Coming back to hardware, which does not have mask around wdt, how to
> implement this feature.

Simple - you can't.

You can absolutely implement exactly the functionality you have today,
with minimal changes to the protocol - it just should not be registered
as an implementation of EFI_WATCHDOG_TIMER_ARCH_PROTOCOL.

Then your platform code can invoke this custom protocol as needed.

Regards,

Leif

Re: SP805 driver

[Udit Kumar]

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Tuesday, December 18, 2018 2:56 PM


> 
> On Tue, Dec 18, 2018 at 05:20:59AM +0000, Udit Kumar wrote:
> > Thanks for note Leif,
> >
> > I am trying to understand this behavior  of specification.
> >
> > >"If no handler has been registered, or the registered handler
> > >returns, then the system will be reset by calling the Runtime Service
> > >ResetSystem()."
> > I believe,  this handler needs to be called by wdt driver after wdt is
> > expired. Meaning , we want wdt to work without resetting the system.
> > Therefore a mask is needed with wdt, which will prevent to reset the
> > system.   I see it working, at least for SP805, because of PMU mask
> > bits.
> 
> Yes, if the WDT can be configured to generate an interrupt instead of a
> hardware reset, it can be used to implement this protocol.

Here I am just thinking of one condition , some application started the wdt
and CPU got stuck somewhere in ISR routine, 
Now this wdt is expired. We will end up in hang system. 
I agree doing reset in graceful manner is always great, but In this case, resetting the 
as it is, will be more useful. 

Thought ? 
 
> > Also ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c has this
> limitation.
> > I haven’t read spec of this wdt.  I hope there should a mask around.
> 
> No, the situation for the GenericWatchdogDxe is not as dire.
> 
> Correct: it does not permit registering software handlers (which perhaps we
> should do something about, but is ... acceptable).
> 
> _But_, it still conforms to the above text; when the timer expires, it resets the
> system by calling the ResetSystem() service. It does not directly force a
> hardware reset.

Hmm, this does partly by calling  ResetSystem(), however this does not 
Allow to install handler in WatchdogRegisterHandler.

> The severe problem is not the lack of the ability to register the software handler
> (which does remove much of the utility), but the removal of reset control from
> the system firmware.
> 
> > Coming back to hardware, which does not have mask around wdt, how to
> > implement this feature.
> 
> Simple - you can't.
> 
> You can absolutely implement exactly the functionality you have today, with
> minimal changes to the protocol - it just should not be registered as an
> implementation of EFI_WATCHDOG_TIMER_ARCH_PROTOCOL.

I believe,  EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is must 
Are you suggesting to EFI_WATCHDOG_TIMER_ARCH_PROTOCOL from MdeModulePkg
and hook platform specific code with this.
Or simply register EFI_WATCHDOG_TIMER_ARCH_PROTOCOL with dummy functions.

> Then your platform code can invoke this custom protocol as needed.
 
> Regards,
> 
> Leif

Re: SP805 driver

[Leif Lindholm]

On Tue, Dec 18, 2018 at 12:30:34PM +0000, Udit Kumar wrote:
> > > >"If no handler has been registered, or the registered handler
> > > >returns, then the system will be reset by calling the Runtime Service
> > > >ResetSystem()."
> > > I believe,  this handler needs to be called by wdt driver after wdt is
> > > expired. Meaning , we want wdt to work without resetting the system.
> > > Therefore a mask is needed with wdt, which will prevent to reset the
> > > system.   I see it working, at least for SP805, because of PMU mask
> > > bits.
> > 
> > Yes, if the WDT can be configured to generate an interrupt instead of a
> > hardware reset, it can be used to implement this protocol.
> 
> Here I am just thinking of one condition , some application started the wdt
> and CPU got stuck somewhere in ISR routine, 
> Now this wdt is expired. We will end up in hang system. 
> I agree doing reset in graceful manner is always great, but In this case, resetting the 
> as it is, will be more useful. 
> 
> Thought ? 

The short answer is:
That would violate the specification, so what is the purpose of
debating the merit?

The longer answer is:
The comparison conflates software and hardware watchdogs. There is
nothing saying a system couldn't have both.

But giving someone a hardware watchdog when they explicitly ask for a
software one is not OK.

> > > Also ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c has this
> > limitation.
> > > I haven’t read spec of this wdt.  I hope there should a mask around.
> > 
> > No, the situation for the GenericWatchdogDxe is not as dire.
> > 
> > Correct: it does not permit registering software handlers (which perhaps we
> > should do something about, but is ... acceptable).
> > 
> > _But_, it still conforms to the above text; when the timer expires, it resets the
> > system by calling the ResetSystem() service. It does not directly force a
> > hardware reset.
> 
> Hmm, this does partly by calling  ResetSystem(), however this does not 
> Allow to install handler in WatchdogRegisterHandler.

As I said, it's acceptable - it's not ideal.

> > The severe problem is not the lack of the ability to register the software handler
> > (which does remove much of the utility), but the removal of reset control from
> > the system firmware.
> > 
> > > Coming back to hardware, which does not have mask around wdt, how to
> > > implement this feature.
> > 
> > Simple - you can't.
> > 
> > You can absolutely implement exactly the functionality you have today, with
> > minimal changes to the protocol - it just should not be registered as an
> > implementation of EFI_WATCHDOG_TIMER_ARCH_PROTOCOL.
> 
> I believe,  EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is must 
> Are you suggesting to EFI_WATCHDOG_TIMER_ARCH_PROTOCOL from MdeModulePkg
> and hook platform specific code with this.
> Or simply register EFI_WATCHDOG_TIMER_ARCH_PROTOCOL with dummy functions.

Are you referring to
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf?

That is certainly what most of the platforms in edk2-platforms use.

The EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is used by core code.

If you want to use your hardware watchdog as part of your platform
specific code, that is absolutely fine and probably a very good idea -
but it has nothing to do with this protocol. There is nothing forcing
you to use the platform-independent EFI_WATCHDOG_TIMER_ARCH_PROTOCOL
for this.

Regards,

Leif

Re: SP805 driver

[Udit Kumar]

> > > > Coming back to hardware, which does not have mask around wdt, how
> > > > to implement this feature.
> > >
> > > Simple - you can't.
> > >
> > > You can absolutely implement exactly the functionality you have
> > > today, with minimal changes to the protocol - it just should not be
> > > registered as an implementation of
> EFI_WATCHDOG_TIMER_ARCH_PROTOCOL.
> >
> > I believe,  EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is must Are you
> > suggesting to EFI_WATCHDOG_TIMER_ARCH_PROTOCOL from
> MdeModulePkg and
> > hook platform specific code with this.
> > Or simply register EFI_WATCHDOG_TIMER_ARCH_PROTOCOL with dummy
> functions.
> 
> Are you referring to
> MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf?

Yes !! 
 
> That is certainly what most of the platforms in edk2-platforms use.
> 
> The EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is used by core code.
> 
> If you want to use your hardware watchdog as part of your platform specific
> code, that is absolutely fine and probably a very good idea - but it has nothing to
> do with this protocol.  There is nothing forcing you to use the platform-
> independent EFI_WATCHDOG_TIMER_ARCH_PROTOCOL for this.

Yes, this was idea to use MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
and if needed, install a custom protocol for hardware wdt. And this to be used by platform specific code.

> Regards,
> 
> Leif

Re: SP805 driver

[Leif Lindholm]

On Tue, Dec 18, 2018 at 12:57:22PM +0000, Udit Kumar wrote:
> > Are you referring to
> > MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf?
> 
> Yes !! 

:)

> > That is certainly what most of the platforms in edk2-platforms use.
> > 
> > The EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is used by core code.
> > 
> > If you want to use your hardware watchdog as part of your platform specific
> > code, that is absolutely fine and probably a very good idea - but it has nothing to
> > do with this protocol.  There is nothing forcing you to use the platform-
> > independent EFI_WATCHDOG_TIMER_ARCH_PROTOCOL for this.
> 
> Yes, this was idea to use MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> and if needed, install a custom protocol for hardware wdt. And this to be used by platform specific code.

Ah, yes - exactly!
This would be the ideal solution.

Regards,

Leif

Re: SP805 driver

[Thomas Abraham]

Hi Leif,

On Mon, Dec 17, 2018 at 6:25 PM Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> Hi Sami, Thomas, (and others on cc)
>
> NXP are upstreaming a set containing an implementation of
> EFI_WATCHDOG_TIMER_ARCH_PROTOCOL using a hardware watchdog as backing.
> This idea comes from the SP805
> driver ArmPlatformPkg/Drivers/SP805WatchdogDxe, which does the same.
>
> The problem is that this is a horrible idea. The point of
> the EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is that it lets you schedule software
> events when the watchdog timer expires. However, the SP805 driver does not
> let you do this:
>
> EFI_STATUS
> EFIAPI
> SP805RegisterHandler (
>   IN CONST 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;
> }
>
> From section 12.14 of the PI 1.6 spec:
> "If no handler has been registered, or the registered handler returns, then
> the system will be reset by calling the Runtime Service ResetSystem()."
> Blatantly, any driver that does the above (and initializes hardware that
> will reset the system without software control) will violate this.
>
> Meanwhile, the only two ARM platforms that include this driver are TC2 and
> FVP.
>
> Now, NXP are not at fault for following examples given to them in the
> reference ARM driver section - but can we please keep further people from
> making the same mistake?
> So my question to {Sami|Thomas} is - can we nuke this driver, and if so,
> can you provide the patches to remove it from edk2 and the resulting ones
> needed for edk2-platforms?

Would it be okay to fix this driver by calling 'ResetSystem' instead
of removing this driver?

Thanks,
Thomas.

>
> I have no issues with reintroducing a fixed SP805 driver in the future that
> does not claim to conform to the above protocol.
>
> Finally - both the D03 and D05 platform .dsc files, as well as the
> Hisilicon ArmPlatformLib, contain references to it. (Ming/Heyi - please
> provide a patch.)
>
> /
>     Leif
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: SP805 driver

[Ming Huang]

On 12/17/2018 8:55 PM, Leif Lindholm wrote:
> Hi Sami, Thomas, (and others on cc)
> 
> NXP are upstreaming a set containing an implementation of 
> EFI_WATCHDOG_TIMER_ARCH_PROTOCOL using a hardware watchdog as backing.
> This idea comes from the SP805 driver ArmPlatformPkg/Drivers/SP805WatchdogDxe, which does the same.
> 
> The problem is that this is a horrible idea. The point of the EFI_WATCHDOG_TIMER_ARCH_PROTOCOL is that it lets you schedule software events when the watchdog timer expires. However, the SP805 driver does not let you do this:
> 
> EFI_STATUS
> EFIAPI
> SP805RegisterHandler (
>   IN CONST 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;
> }
> 
> From section 12.14 of the PI 1.6 spec:
> "If no handler has been registered, or the registered handler returns, then the system will be reset by calling the Runtime Service ResetSystem()."
> Blatantly, any driver that does the above (and initializes hardware that will reset the system without software control) will violate this.
> 
> Meanwhile, the only two ARM platforms that include this driver are TC2 and FVP.
> 
> Now, NXP are not at fault for following examples given to them in the reference ARM driver section - but can we please keep further people from making the same mistake?
> So my question to {Sami|Thomas} is - can we nuke this driver, and if so, can you provide the patches to remove it from edk2 and the resulting ones needed for edk2-platforms?
> 
> I have no issues with reintroducing a fixed SP805 driver in the future that does not claim to conform to the above protocol.
> 
> Finally - both the D03 and D05 platform .dsc files, as well as the Hisilicon ArmPlatformLib, contain references to it. (Ming/Heyi - please provide a patch.)

OK.

Thanks.

> 
> /
>     Leif