From: Leif Lindholm <leif.lindholm@linaro.org>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel@lists.01.org, Sami Mujawar <sami.mujawar@arm.com>,
Thomas Panakamattam Abraham <thomas.abraham@arm.com>,
Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>,
Udit Kumar <udit.kumar@nxp.com>,
Matteo Carlini <Matteo.Carlini@arm.com>,
Nariman Poushin <nariman.poushin@linaro.org>
Subject: Re: [PATCH v2 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode
Date: Thu, 20 Dec 2018 11:35:31 +0000 [thread overview]
Message-ID: <20181220113531.bzscssbuppyrzclz@bivouac.eciton.net> (raw)
In-Reply-To: <20181219204023.6317-3-ard.biesheuvel@linaro.org>
On Wed, Dec 19, 2018 at 09:40:21PM +0100, Ard Biesheuvel wrote:
> The SP805 watchdog driver doesn't implement the PI watchdog protocol
> fully, but always simply resets the system if the watchdog time runs
> out.
>
> However, the hardware does support the intended usage model, as long
> as the SP805 is wired up correctly. So let's implement interrupt based
> mode involving a handler that is registered by the DXE core and invoked
> when the watchdog runs out. In the interrupt handler, we invoke the
> notify function if one was registered, or call the ResetSystem()
> runtime service otherwise (as per the UEFI spec)
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPlatformPkg/ArmPlatformPkg.dec | 1 +
> ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf | 6 +-
> ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c | 100 +++++++++++++++-----
> 3 files changed, 80 insertions(+), 27 deletions(-)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 5f67e7415469..44c00bd0c133 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -70,6 +70,7 @@ [PcdsFixedAtBuild.common]
> ## SP805 Watchdog
> gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase|0x0|UINT32|0x00000023
> gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz|32000|UINT32|0x00000021
> + gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt|0|UINT32|0x0000002E
>
> ## PL011 UART
> gArmPlatformTokenSpaceGuid.PL011UartClkInHz|24000000|UINT32|0x0000001F
> diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> index c3971fb035d3..0e744deeca8d 100644
> --- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> +++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
> @@ -27,6 +27,7 @@ [Sources.common]
> [Packages]
> ArmPkg/ArmPkg.dec
> ArmPlatformPkg/ArmPlatformPkg.dec
> + EmbeddedPkg/EmbeddedPkg.dec
> MdePkg/MdePkg.dec
>
> [LibraryClasses]
> @@ -35,13 +36,16 @@ [LibraryClasses]
> IoLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
> + UefiRuntimeServicesTableLib
>
> [Pcd]
> gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase
> gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz
> + gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt
>
> [Protocols]
> + gHardwareInterruptProtocolGuid ## ALWAYS_CONSUMES
> gEfiWatchdogTimerArchProtocolGuid ## ALWAYS_PRODUCES
>
> [Depex]
> - TRUE
> + gHardwareInterruptProtocolGuid
> diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> index 12c2f0a1fe49..5bbb12af6019 100644
> --- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> +++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c
> @@ -21,12 +21,17 @@
> #include <Library/DebugLib.h>
> #include <Library/IoLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
>
> +#include <Protocol/HardwareInterrupt.h>
> #include <Protocol/WatchdogTimer.h>
>
> #include "SP805Watchdog.h"
>
> -STATIC EFI_EVENT mEfiExitBootServicesEvent;
> +STATIC EFI_EVENT mEfiExitBootServicesEvent;
> +STATIC EFI_HARDWARE_INTERRUPT_PROTOCOL *mInterrupt;
> +STATIC EFI_WATCHDOG_TIMER_NOTIFY mWatchdogNotify;
> +STATIC UINT32 mTimerPeriod;
>
> /**
> Make sure the SP805 registers are unlocked for writing.
> @@ -65,6 +70,33 @@ SP805Lock (
> }
> }
>
> +STATIC
> +VOID
> +EFIAPI
> +SP805InterruptHandler (
> + IN HARDWARE_INTERRUPT_SOURCE Source,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + )
> +{
> + SP805Unlock ();
> + MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears the irq
> + SP805Lock ();
> +
> + mInterrupt->EndOfInterrupt (mInterrupt, Source);
> +
> + //
> + // The notify function should be called with the elapsed number of ticks
> + // since the watchdog was armed, which should exceed the timer period.
> + // We don't actually know the elapsed number of ticks, so let's return
> + // the timer period plus 1.
> + //
> + if (mWatchdogNotify != NULL) {
> + mWatchdogNotify (mTimerPeriod + 1);
> + }
> +
> + gRT->ResetSystem (EfiResetCold, EFI_TIMEOUT, 0, NULL);
> +}
> +
> /**
> Stop the SP805 watchdog timer from counting down by disabling interrupts.
> **/
> @@ -149,9 +181,16 @@ SP805RegisterHandler (
> IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction
> )
> {
> - // ERROR: This function is not supported.
> - // The hardware watchdog will reset the board
> - return EFI_INVALID_PARAMETER;
> + if (mWatchdogNotify == NULL && NotifyFunction == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (mWatchdogNotify != NULL && NotifyFunction != NULL) {
> + return EFI_ALREADY_STARTED;
> + }
> +
> + mWatchdogNotify = NotifyFunction;
> + return EFI_SUCCESS;
> }
>
> /**
> @@ -202,19 +241,16 @@ SP805SetTimerPeriod (
> SP805Stop ();
> } else {
> // Calculate the Watchdog ticks required for a delay of (TimerTicks * 100) nanoseconds
> - // The SP805 will count down to ZERO once, generate an interrupt and
> - // then it will again reload the initial value and start again.
> - // On the second time when it reaches ZERO, it will actually reset the board.
> - // Therefore, we need to load half the required delay.
> + // The SP805 will count down to zero and generate an interrupt.
> //
> - // WatchdogTicks = ((TimerPeriod * 100 * SP805_CLOCK_FREQUENCY) / 1GHz) / 2 ;
> + // WatchdogTicks = ((TimerPeriod * 100 * SP805_CLOCK_FREQUENCY) / 1GHz);
> //
> // i.e.:
> //
> - // WatchdogTicks = (TimerPeriod * SP805_CLOCK_FREQUENCY) / 20 MHz ;
> + // WatchdogTicks = (TimerPeriod * SP805_CLOCK_FREQUENCY) / 10 MHz ;
>
> Ticks64bit = MultU64x32 (TimerPeriod, PcdGet32 (PcdSP805WatchdogClockFrequencyInHz));
> - Ticks64bit = DivU64x32 (Ticks64bit, 20000000);
> + Ticks64bit = DivU64x32 (Ticks64bit, 10 * 1000 * 1000);
>
> // The registers in the SP805 are only 32 bits
> if (Ticks64bit > MAX_UINT32) {
> @@ -233,9 +269,12 @@ SP805SetTimerPeriod (
> SP805Start ();
> }
>
> + mTimerPeriod = TimerPeriod;
> +
> EXIT:
> // Ensure the watchdog is locked before exiting.
> SP805Lock ();
> + ASSERT_EFI_ERROR (Status);
> return Status;
> }
>
> @@ -262,25 +301,11 @@ SP805GetTimerPeriod (
> OUT UINT64 *TimerPeriod
> )
> {
> - UINT64 ReturnValue;
> -
> if (TimerPeriod == NULL) {
> return EFI_INVALID_PARAMETER;
> }
>
> - // Check if the watchdog is stopped
> - if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_INTEN) == 0) {
> - // It is stopped, so return zero.
> - ReturnValue = 0;
> - } else {
> - // Convert the Watchdog ticks into TimerPeriod
> - // Ensure 64bit arithmetic throughout because the Watchdog ticks may already
> - // be at the maximum 32 bit value and we still need to multiply that by 600.
> - ReturnValue = MultU64x32 (MmioRead32 (SP805_WDOG_LOAD_REG), 600);
> - }
> -
> - *TimerPeriod = ReturnValue;
> -
> + *TimerPeriod = mTimerPeriod;
> return EFI_SUCCESS;
> }
>
> @@ -343,6 +368,11 @@ SP805Initialize (
> EFI_STATUS Status;
> EFI_HANDLE Handle;
>
> + // Find the interrupt controller protocol. ASSERT if not found.
> + Status = gBS->LocateProtocol (&gHardwareInterruptProtocolGuid, NULL,
> + (VOID **)&mInterrupt);
> + ASSERT_EFI_ERROR (Status);
> +
> // Unlock access to the SP805 registers
> SP805Unlock ();
>
> @@ -350,13 +380,31 @@ SP805Initialize (
> SP805Stop ();
>
> // Set the watchdog to reset the board when triggered
> + // This is a last resort in case the interrupt handler fails
> if ((MmioRead32 (SP805_WDOG_CONTROL_REG) & SP805_WDOG_CTRL_RESEN) == 0) {
> MmioOr32 (SP805_WDOG_CONTROL_REG, SP805_WDOG_CTRL_RESEN);
> }
>
> + // Clear any pending interrupts
> + MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears the irq
> +
> // Prohibit any rogue access to SP805 registers
> SP805Lock ();
>
> + if (PcdGet32 (PcdSP805WatchdogInterrupt) > 0) {
> + Status = mInterrupt->RegisterInterruptSource (mInterrupt,
> + PcdGet32 (PcdSP805WatchdogInterrupt),
> + SP805InterruptHandler);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: failed to register watchdog interrupt - %r\n",
> + __FUNCTION__, Status));
> + return Status;
> + }
> + } else {
> + DEBUG ((DEBUG_WARN, "%a: no interrupt specified, running in RESET mode only\n",
> + __FUNCTION__));
> + }
> +
> //
> // Make sure the Watchdog Timer Architectural Protocol has not been installed in the system yet.
> // This will avoid conflicts with the universal watchdog
> --
> 2.19.2
>
next prev parent reply other threads:[~2018-12-20 11:35 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-19 20:40 [PATCH v2 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup Ard Biesheuvel
2018-12-19 20:40 ` [PATCH v2 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup Ard Biesheuvel
2018-12-19 20:40 ` [PATCH v2 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode Ard Biesheuvel
2018-12-20 11:35 ` Leif Lindholm [this message]
2018-12-19 20:40 ` [PATCH v2 3/4] ArmPkg/GenericWatchdogDxe: clean up the code Ard Biesheuvel
2018-12-19 20:40 ` [PATCH v2 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method Ard Biesheuvel
2018-12-20 11:42 ` [PATCH v2 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181220113531.bzscssbuppyrzclz@bivouac.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox