On Fri, 5 Jan 2024 at 06:15, Rebecca Cran
<rebe...@os.amperecomputing.com> wrote:
>
> The calculation of the timer period was broken. Introduce a global
> mTimerPeriod so the calculation can be removed. Since mTimerFrequencyHz
> is only used in one place, remove the global and make it a local
> variable. Do the same with mNumTimerTicks.
>
> Signed-off-by: Rebecca Cran <rebe...@os.amperecomputing.com>
> ---
> ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 36
> ++++++++++++++----------------
> 1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index f8c39458a53a..78cee62a19d6 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -28,13 +28,10 @@
> in a second */
> #define TIME_UNITS_PER_SECOND 10000000
>
> -// Tick frequency of the generic timer basis of the generic watchdog.
> -STATIC UINTN mTimerFrequencyHz = 0;
> -
> /* In cases where the compare register was set manually, information about
> how long the watchdog was asked to wait cannot be retrieved from hardware.
> It is therefore stored here. 0 means the timer is not running. */
> -STATIC UINT64 mNumTimerTicks = 0;
> +STATIC UINT64 mTimerPeriod = 0;
>
> #define MAX_UINT48 0xFFFFFFFFFFFFULL
>
> @@ -91,7 +88,8 @@ WatchdogExitBootServicesEvent (
> )
> {
> WatchdogDisable ();
> - mNumTimerTicks = 0;
> + mTimerPeriod = 0;
> + mExitedBootServices = TRUE;
Where is this declared/defined?
> }
>
> /* This function is called when the watchdog's first signal (WS0) goes high.
> @@ -106,7 +104,6 @@ WatchdogInterruptHandler (
> )
> {
> STATIC CONST CHAR16 ResetString[] = L"The generic watchdog timer ran
> out.";
> - UINT64 TimerPeriod;
>
> WatchdogDisable ();
>
> @@ -119,8 +116,7 @@ WatchdogInterruptHandler (
> // the timer period plus 1.
> //
> if (mWatchdogNotify != NULL) {
> - TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) *
> mNumTimerTicks);
> - mWatchdogNotify (TimerPeriod + 1);
> + mWatchdogNotify (mTimerPeriod + 1);
> }
>
> gRT->ResetSystem (
> @@ -200,22 +196,27 @@ WatchdogSetTimerPeriod (
> IN UINT64 TimerPeriod // In 100ns units
> )
> {
> - UINTN SystemCount;
> + UINTN SystemCount;
> + UINT64 TimerFrequencyHz;
> + UINT64 NumTimerTicks;
>
> // if TimerPeriod is 0, this is a request to stop the watchdog.
> if (TimerPeriod == 0) {
> - mNumTimerTicks = 0;
> + mTimerPeriod = 0;
> WatchdogDisable ();
> return EFI_SUCCESS;
> }
>
> // Work out how many timer ticks will equate to TimerPeriod
> - mNumTimerTicks = (mTimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND;
> + TimerFrequencyHz = ArmGenericTimerGetTimerFreq ();
> + ASSERT (TimerFrequencyHz != 0);
> + mTimerPeriod = TimerPeriod;
> + NumTimerTicks = (TimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND;
>
> /* If the number of required ticks is greater than the max the watchdog's
> offset register (WOR) can hold, we need to manually compute and set
> the compare register (WCV) */
> - if (mNumTimerTicks > MAX_UINT48) {
> + if (NumTimerTicks > MAX_UINT48) {
> /* We need to enable the watchdog *before* writing to the compare
> register,
> because enabling the watchdog causes an "explicit refresh", which
> clobbers the compare register (WCV). In order to make sure this
> doesn't
> @@ -223,9 +224,9 @@ WatchdogSetTimerPeriod (
> WatchdogWriteOffsetRegister (MAX_UINT48);
> WatchdogEnable ();
> SystemCount = ArmGenericTimerGetSystemCount ();
> - WatchdogWriteCompareRegister (SystemCount + mNumTimerTicks);
> + WatchdogWriteCompareRegister (SystemCount + NumTimerTicks);
> } else {
> - WatchdogWriteOffsetRegister (mNumTimerTicks);
> + WatchdogWriteOffsetRegister (NumTimerTicks);
> WatchdogEnable ();
> }
>
> @@ -260,7 +261,7 @@ WatchdogGetTimerPeriod (
> return EFI_INVALID_PARAMETER;
> }
>
> - *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) *
> mNumTimerTicks);
> + *TimerPeriod = mTimerPeriod;
>
> return EFI_SUCCESS;
> }
> @@ -327,9 +328,6 @@ GenericWatchdogEntry (
> This will avoid conflicts with the universal watchdog */
> ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL,
> &gEfiWatchdogTimerArchProtocolGuid);
>
> - mTimerFrequencyHz = ArmGenericTimerGetTimerFreq ();
> - ASSERT (mTimerFrequencyHz != 0);
> -
> // Install interrupt handler
> Status = mInterruptProtocol->RegisterInterruptSource (
> mInterruptProtocol,
> @@ -371,7 +369,7 @@ GenericWatchdogEntry (
> );
> ASSERT_EFI_ERROR (Status);
>
> - mNumTimerTicks = 0;
> + mTimerPeriod = 0;
> WatchdogDisable ();
>
> return EFI_SUCCESS;
> --
> 2.34.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113222): https://edk2.groups.io/g/devel/message/113222
Mute This Topic: https://groups.io/mt/103538116/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
