Re: [edk2-devel] [PATCH v3 2/3] ArmPkg: Introduce global mTimerPeriod and remove calculation

["Ard Biesheuvel" <ardb@kernel.org>]

On Fri, 5 Jan 2024 at 06:15, Rebecca Cran
<rebecca@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 <rebecca@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-