1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v3 0/3] ArmPkg: GenericWatchdogDxe fixes and improvements
  4. View patch

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

Rebecca Cran via groups.io posted 3 patches 1 year, 11 months ago
There is a newer version of this series
[edk2-devel] [PATCH v3 2/3] ArmPkg: Introduce global mTimerPeriod and remove calculation
Posted by Rebecca Cran via groups.io 1 year, 11 months ago 
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;
 }
 
 /* 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 (#113216): https://edk2.groups.io/g/devel/message/113216
Mute This Topic: https://groups.io/mt/103538116/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 2/3] ArmPkg: Introduce global mTimerPeriod and remove calculation
Posted by Sami Mujawar 1 year, 11 months ago 
Hi Rebecca,

I have a minor suggestion marked inline as [SAMI], otherwise this patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 05/01/2024, 05:15, "Rebecca Cran" <rebecca@os.amperecomputing.com <mailto: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 <mailto: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;
}


/* 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;
[SAMI] I think we do not need to initialise mTimerPeriod to 0 here as it would already be 0, right?
WatchdogDisable ();


return EFI_SUCCESS;
-- 
2.34.1







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113273): https://edk2.groups.io/g/devel/message/113273
Mute This Topic: https://groups.io/mt/103538116/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 2/3] ArmPkg: Introduce global mTimerPeriod and remove calculation
Posted by Ard Biesheuvel 1 year, 11 months ago 
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 2/3] ArmPkg: Introduce global mTimerPeriod and remove calculation
Posted by Rebecca Cran via groups.io 1 year, 11 months ago 
On 1/5/2024 1:26 AM, Ard Biesheuvel wrote:

>> @@ -91,7 +88,8 @@ WatchdogExitBootServicesEvent (
>>     )
>>   {
>>     WatchdogDisable ();
>> -  mNumTimerTicks = 0;
>> +  mTimerPeriod        = 0;
>> +  mExitedBootServices = TRUE;
> 
> Where is this declared/defined?

Oh, it's defined in the 3rd patch - which obviously breaks bisect. I'll 
fix it.

-- 
Rebecca Cran



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114019): https://edk2.groups.io/g/devel/message/114019
Mute This Topic: https://groups.io/mt/103538116/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.