Re: [edk2] SP805 driver
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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SP805 driver
On Tue, Dec 18, 2018 at 12:57:22PM +, 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SP805 driver
> > > > 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SP805 driver
On Tue, Dec 18, 2018 at 12:30:34PM +, 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SP805 driver
> -Original Message- > From: Leif Lindholm > Sent: Tuesday, December 18, 2018 2:56 PM > > On Tue, Dec 18, 2018 at 05:20:59AM +, 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SP805 driver
On Tue, Dec 18, 2018 at 05:20:59AM +, 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SP805 driver
Hi Leif,
On Mon, Dec 17, 2018 at 6:25 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_NOTIFYNotifyFunction
> )
> {
> // 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SP805 driver
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
Sent: Monday, December 17, 2018 6:25 PM
To: Sami Mujawar ; Thomas Panakamattam Abraham
Cc: Meenakshi Aggarwal ; Udit Kumar
; Ard Biesheuvel ; edk2-devel
(edk2-devel@lists.01.org) ; Ming Huang
; Heyi Guo ; Matteo Carlini
; Nariman Poushin
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_NOTIFYNotifyFunction
)
{
// 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] 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_NOTIFYNotifyFunction
)
{
// 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
