From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::144; helo=mail-it1-x144.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x144.google.com (mail-it1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2214B2119590D for ; Tue, 18 Dec 2018 08:23:53 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id c9so5065354itj.1 for ; Tue, 18 Dec 2018 08:23:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kqiKpcm6USVh6rvPRu3dHztkfmc2vw3J26ltX07tgk0=; b=Va8KR0XspBJADPuA5lyOXojzRZ7TgCRoDIjF9PF75GANhRnhHlsVyTLttJCP5F9/SK 17d2jre9UANdotRnNpB8X63LGOMbb4S/1tA1/QrzAiZQaxxqEIpDNqDSOOUXXzOOKLte f7+52Kw6ytrPY2IGXMeWHDtskmFKO6O1MAtcs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kqiKpcm6USVh6rvPRu3dHztkfmc2vw3J26ltX07tgk0=; b=OWQMGPBYGslyf5Hp4BO1TjY7dAknqNu9M4Eb0sPu+1M2lghREMOJjvNaM4JNfHOLSH AXktEqwkybxXp9ROwQsk5AXHgWHWZqNzXncr5FI1EByeCTaBYMS2crFpZSGhUHtOh5W2 FOlQqRPweDCTOPGe4N2LjgVz7xnIvGzkRH4qq5bdjCPCaBNtE8On4rPc0R0RjTwX+lai /oOXoh+/MN6JZMK2Dwlv9MiUkcWDR989J59o8Z5w5ayMIbpYsOS82JgkV3WgYArbTtxR 14Og3VyUzoaA9L9aBES8Lpc3LWu//Q4tIIcAakkRyRpOFN2h/fbMDnyBc/rBcRCBLO8a c7xA== X-Gm-Message-State: AA+aEWYFRfMj7mddoiMM6Wm/psLZPMMsnJ3J5KLzu3MHnSr6X9yvxNwG sq/eaBh93r+hDGLhz2YHyxZ08kGPdmAGBqdK0g9vEg== X-Google-Smtp-Source: AFSGD/WfWHayX7OfRXkshzTxl5TrjSxyPXsfnn05PFhBCwe8ukCOUm2znvQy8PN78MStqTd/vX8gXmUapN28tLfdscY= X-Received: by 2002:a24:edc4:: with SMTP id r187mr3169799ith.158.1545150233049; Tue, 18 Dec 2018 08:23:53 -0800 (PST) MIME-Version: 1.0 References: <20181218131015.20062-1-ard.biesheuvel@linaro.org> <20181218131015.20062-3-ard.biesheuvel@linaro.org> In-Reply-To: From: Ard Biesheuvel Date: Tue, 18 Dec 2018 17:23:41 +0100 Message-ID: To: Udit Kumar Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Sami Mujawar , Thomas Panakamattam Abraham , Meenakshi Aggarwal , Matteo Carlini , Nariman Poushin Subject: Re: [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Dec 2018 16:23:54 -0000 Content-Type: text/plain; charset="UTF-8" On Tue, 18 Dec 2018 at 16:58, Udit Kumar wrote: > > > > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: Tuesday, December 18, 2018 6:40 PM > > To: edk2-devel@lists.01.org > > Cc: Ard Biesheuvel ; Leif Lindholm > > ; Sami Mujawar ; Thomas > > Panakamattam Abraham ; Meenakshi Aggarwal > > ; Udit Kumar ; Matteo > > Carlini ; Nariman Poushin > > > > Subject: [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt > > mode > > > > 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) > > Specs, says > If no handler has been registered, or the registered handler returns, then the system will be reset by calling the Runtime Service ResetSystem(). > My understanding is that we have to ResetSystem() always irrespective of notify function > Indeed. Thanks for spotting that. > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > > --- > > ArmPlatformPkg/ArmPlatformPkg.dec | 1 + > > ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c | 95 > > ++++++++++++++------ > > ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf | 6 +- > > 3 files changed, 75 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 @@ > > ## SP805 Watchdog > > > > gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase|0x0|UINT32|0x000000 > > 23 > > > > gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz|32000| > > UINT32|0x00000021 > > + > > + > > gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt|0|UINT32|0x0000 > > 00 > > + 2E > > > > ## PL011 UART > > > > gArmPlatformTokenSpaceGuid.PL011UartClkInHz|24000000|UINT32|0x000000 > > 1F > > diff --git a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c > > b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c > > index 12c2f0a1fe49..4f09acf1fa28 100644 > > --- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c > > +++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805Watchdog.c > > @@ -21,12 +21,17 @@ > > #include > > #include > > #include > > +#include > > > > +#include > > #include > > > > #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 > > + ) > > +{ > > + // > > + // 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); > > + } else { > > + gRT->ResetSystem (EfiResetCold, EFI_TIMEOUT, 0, NULL); > > + } > > Shouldn't we ResetSystem in all cases. > > > > + > > + SP805Unlock (); > > + MmioWrite32 (SP805_WDOG_INT_CLR_REG, 0); // write of any value clears > > + the irq SP805Lock (); > > + > > + mInterrupt->EndOfInterrupt (mInterrupt, Source); } > > + > > IMO, this routine should be > 1/ Handle IT > 2/ Do callback mWatchdogNotify > 3/ Reset the system. > > > /** > > 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,26 @@ 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 (); > > > > + 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; > > + } > > + > > // > > // Make sure the Watchdog Timer Architectural Protocol has not been installed > > in the system yet. > > // This will avoid conflicts with the universal watchdog diff --git > > a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf > > b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf > > index 99ecab477567..1373e267612f 100644 > > --- a/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf > > +++ b/ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf > > @@ -27,6 +27,7 @@ > > [Packages] > > ArmPlatformPkg/ArmPlatformPkg.dec > > ArmPkg/ArmPkg.dec > > + EmbeddedPkg/EmbeddedPkg.dec > > MdePkg/MdePkg.dec > > > > [LibraryClasses] > > @@ -35,13 +36,16 @@ > > IoLib > > UefiBootServicesTableLib > > UefiDriverEntryPoint > > + UefiRuntimeServicesTableLib > > > > [Pcd] > > gArmPlatformTokenSpaceGuid.PcdSP805WatchdogBase > > gArmPlatformTokenSpaceGuid.PcdSP805WatchdogClockFrequencyInHz > > + gArmPlatformTokenSpaceGuid.PcdSP805WatchdogInterrupt > > > > [Protocols] > > + gHardwareInterruptProtocolGuid ## ALWAYS_CONSUMES > > gEfiWatchdogTimerArchProtocolGuid ## ALWAYS_PRODUCES > > > > [Depex] > > - TRUE > > + gHardwareInterruptProtocolGuid > > -- > > 2.17.1 >