[edk2-devel] [PATCH v3 0/3] ArmPkg: GenericWatchdogDxe fixes and improvements

[edk2-devel] [PATCH v3 0/3] ArmPkg: GenericWatchdogDxe fixes and improvements

[Rebecca Cran via groups.io]

Fixes and improvements to GenericWatchdogDxe.

PR: https://github.com/tianocore/edk2/pull/5176

Rebecca Cran (3):
  ArmPkg: Update GenericWatchdogDxe to allow setting full 48-bit offset
  ArmPkg: Introduce global mTimerPeriod and remove calculation
  ArmPkg: Disable watchdog interaction after exiting boot services

 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h    |  6 ++-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 60 +++++++++++++++++-------------
 2 files changed, 40 insertions(+), 26 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113217): https://edk2.groups.io/g/devel/message/113217
Mute This Topic: https://groups.io/mt/103538117/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 1/3] ArmPkg: Update GenericWatchdogDxe to allow setting full 48-bit offset

[Rebecca Cran via groups.io]

The generic watchdog offset register is 48 bits wide, and can be set by
performing two 32-bit writes.

Add support for writing the high 16 bits of the offset register and
update the signature of the WatchdogWriteOffsetRegister function to take
a UINT64 value.

Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h    |  6 +++++-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 14 +++++++++-----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index 9bc3bf47047c..2a0634e7e9f1 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -1,9 +1,12 @@
 /** @file
 *
+*  Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
 *  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
+*  See Generic Watchdog specification in Arm Base System Architecture 1.0C:
+*  https://developer.arm.com/documentation/den0094/c/
 **/
 
 #ifndef GENERIC_WATCHDOG_H_
@@ -14,7 +17,8 @@
 
 // Control Frame:
 #define GENERIC_WDOG_CONTROL_STATUS_REG      ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x000)
-#define GENERIC_WDOG_OFFSET_REG              ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x008)
+#define GENERIC_WDOG_OFFSET_REG_LOW          ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x008)
+#define GENERIC_WDOG_OFFSET_REG_HIGH         ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x00C)
 #define GENERIC_WDOG_COMPARE_VALUE_REG_LOW   ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x010)
 #define GENERIC_WDOG_COMPARE_VALUE_REG_HIGH  ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x014)
 
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 66c6c37c08b0..f8c39458a53a 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -1,5 +1,6 @@
 /** @file
 *
+*  Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
 *  Copyright (c) 2013-2018, ARM Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -35,16 +36,19 @@ STATIC UINTN  mTimerFrequencyHz = 0;
    It is therefore stored here. 0 means the timer is not running. */
 STATIC UINT64  mNumTimerTicks = 0;
 
+#define MAX_UINT48  0xFFFFFFFFFFFFULL
+
 STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL  *mInterruptProtocol;
 STATIC EFI_WATCHDOG_TIMER_NOTIFY         mWatchdogNotify;
 
 STATIC
 VOID
 WatchdogWriteOffsetRegister (
-  UINT32  Value
+  UINT64  Value
   )
 {
-  MmioWrite32 (GENERIC_WDOG_OFFSET_REG, Value);
+  MmioWrite32 (GENERIC_WDOG_OFFSET_REG_LOW, Value & MAX_UINT32);
+  MmioWrite32 (GENERIC_WDOG_OFFSET_REG_HIGH, (Value >> 32) & MAX_UINT16);
 }
 
 STATIC
@@ -211,17 +215,17 @@ WatchdogSetTimerPeriod (
   /* 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_UINT32) {
+  if (mNumTimerTicks > 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
        trigger an interrupt, set the offset to max. */
-    WatchdogWriteOffsetRegister (MAX_UINT32);
+    WatchdogWriteOffsetRegister (MAX_UINT48);
     WatchdogEnable ();
     SystemCount = ArmGenericTimerGetSystemCount ();
     WatchdogWriteCompareRegister (SystemCount + mNumTimerTicks);
   } else {
-    WatchdogWriteOffsetRegister ((UINT32)mNumTimerTicks);
+    WatchdogWriteOffsetRegister (mNumTimerTicks);
     WatchdogEnable ();
   }
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113215): https://edk2.groups.io/g/devel/message/113215
Mute This Topic: https://groups.io/mt/103538115/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/3] ArmPkg: Update GenericWatchdogDxe to allow setting full 48-bit offset

[Sami Mujawar]

Hi Rebecca,

Thank you for the updated patch.

Please find my feedback inline marked [SAMI].

Regards,

Sami Mujawar

On 05/01/2024, 05:15, "Rebecca Cran" <rebecca@os.amperecomputing.com <mailto:rebecca@os.amperecomputing.com>> wrote:


The generic watchdog offset register is 48 bits wide, and can be set by
performing two 32-bit writes.


Add support for writing the high 16 bits of the offset register and
update the signature of the WatchdogWriteOffsetRegister function to take
a UINT64 value.


Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com <mailto:rebecca@os.amperecomputing.com>>
---
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h | 6 +++++-
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 14 +++++++++-----
2 files changed, 14 insertions(+), 6 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index 9bc3bf47047c..2a0634e7e9f1 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -1,9 +1,12 @@
/** @file
*
+* Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
* Copyright (c) 2013-2017, ARM Limited. All rights reserved.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*
+* See Generic Watchdog specification in Arm Base System Architecture 1.0C:
+* https://developer.arm.com/documentation/den0094/c/ <https://developer.arm.com/documentation/den0094/c/>
[SAMI] Minor. I think this should be changed to use the @par Reference(s):
e.g.
* @par Reference(s):
*  - Generic Watchdog specification in Arm Base System Architecture 1.0C:
*  https://developer.arm.com/documentation/den0094/c/
[/SAMI]
**/


#ifndef GENERIC_WATCHDOG_H_
@@ -14,7 +17,8 @@


// Control Frame:
#define GENERIC_WDOG_CONTROL_STATUS_REG ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x000)
-#define GENERIC_WDOG_OFFSET_REG ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x008)
+#define GENERIC_WDOG_OFFSET_REG_LOW ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x008)
+#define GENERIC_WDOG_OFFSET_REG_HIGH ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x00C)
#define GENERIC_WDOG_COMPARE_VALUE_REG_LOW ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x010)
#define GENERIC_WDOG_COMPARE_VALUE_REG_HIGH ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x014)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 66c6c37c08b0..f8c39458a53a 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -1,5 +1,6 @@
/** @file
*
+* Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
* Copyright (c) 2013-2018, ARM Limited. All rights reserved.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -35,16 +36,19 @@ STATIC UINTN mTimerFrequencyHz = 0;
It is therefore stored here. 0 means the timer is not running. */
STATIC UINT64 mNumTimerTicks = 0;


+#define MAX_UINT48 0xFFFFFFFFFFFFULL
+
STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
STATIC EFI_WATCHDOG_TIMER_NOTIFY mWatchdogNotify;


STATIC
VOID
WatchdogWriteOffsetRegister (
- UINT32 Value
+ UINT64 Value
)
{
- MmioWrite32 (GENERIC_WDOG_OFFSET_REG, Value);
+ MmioWrite32 (GENERIC_WDOG_OFFSET_REG_LOW, Value & MAX_UINT32);
+ MmioWrite32 (GENERIC_WDOG_OFFSET_REG_HIGH, (Value >> 32) & MAX_UINT16);
[SAMI] Apologies for not catching this in my earlier review, but I think you need to read W_IIDR and only write to the GENERIC_WDOG_OFFSET_REG_HIGH register if the watchdog revision is == 1. Otherwise, this may cause undesirable results on systems that implement watchdog timer revision 0.
}


STATIC
@@ -211,17 +215,17 @@ WatchdogSetTimerPeriod (
/* 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_UINT32) {
+ if (mNumTimerTicks > 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
trigger an interrupt, set the offset to max. */
- WatchdogWriteOffsetRegister (MAX_UINT32);
+ WatchdogWriteOffsetRegister (MAX_UINT48);
WatchdogEnable ();
SystemCount = ArmGenericTimerGetSystemCount ();
WatchdogWriteCompareRegister (SystemCount + mNumTimerTicks);
} else {
- WatchdogWriteOffsetRegister ((UINT32)mNumTimerTicks);
+ WatchdogWriteOffsetRegister (mNumTimerTicks);
WatchdogEnable ();
}


-- 
2.34.1







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113272): https://edk2.groups.io/g/devel/message/113272
Mute This Topic: https://groups.io/mt/103538115/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Rebecca Cran via groups.io]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Rebecca Cran via groups.io]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 3/3] ArmPkg: Disable watchdog interaction after exiting boot services

[Rebecca Cran via groups.io]

Update GenericWatchdogDxe to disable watchdog interaction after exiting
boot services. Also, move the mEfiExitBootServicesEvent event to the top
of the file with the other static variables.

Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 78cee62a19d6..ddf131660f9d 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -33,10 +33,14 @@
    It is therefore stored here. 0 means the timer is not running. */
 STATIC UINT64  mTimerPeriod = 0;
 
+/* disables watchdog interaction after Exit Boot Services */
+STATIC BOOLEAN  mExitedBootServices = FALSE;
+
 #define MAX_UINT48  0xFFFFFFFFFFFFULL
 
 STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL  *mInterruptProtocol;
 STATIC EFI_WATCHDOG_TIMER_NOTIFY         mWatchdogNotify;
+STATIC EFI_EVENT                         mEfiExitBootServicesEvent;
 
 STATIC
 VOID
@@ -200,7 +204,13 @@ WatchdogSetTimerPeriod (
   UINT64  TimerFrequencyHz;
   UINT64  NumTimerTicks;
 
-  // if TimerPeriod is 0, this is a request to stop the watchdog.
+  // If we've exited Boot Services but TimerPeriod isn't zero, this
+  // indicates that the caller is doing something wrong.
+  if (mExitedBootServices && (TimerPeriod != 0)) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  // If TimerPeriod is 0 this is a request to stop the watchdog.
   if (TimerPeriod == 0) {
     mTimerPeriod = 0;
     WatchdogDisable ();
@@ -304,8 +314,6 @@ STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL  mWatchdogTimer = {
   WatchdogGetTimerPeriod
 };
 
-STATIC EFI_EVENT  mEfiExitBootServicesEvent;
-
 EFI_STATUS
 EFIAPI
 GenericWatchdogEntry (
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113218): https://edk2.groups.io/g/devel/message/113218
Mute This Topic: https://groups.io/mt/103538118/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 3/3] ArmPkg: Disable watchdog interaction after exiting boot services

[Sami Mujawar]

Hi Rebecca,

Thank you for this patch. 
Please see my feedback inline marked [SAMI].

Regards,

Sami Mujawar

On 05/01/2024, 05:15, "Rebecca Cran" <rebecca@os.amperecomputing.com <mailto:rebecca@os.amperecomputing.com>> wrote:


Update GenericWatchdogDxe to disable watchdog interaction after exiting
boot services. Also, move the mEfiExitBootServicesEvent event to the top
of the file with the other static variables.


Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com <mailto:rebecca@os.amperecomputing.com>>
---
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 78cee62a19d6..ddf131660f9d 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -33,10 +33,14 @@
It is therefore stored here. 0 means the timer is not running. */
STATIC UINT64 mTimerPeriod = 0;


+/* disables watchdog interaction after Exit Boot Services */
+STATIC BOOLEAN mExitedBootServices = FALSE;
+
#define MAX_UINT48 0xFFFFFFFFFFFFULL


STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
STATIC EFI_WATCHDOG_TIMER_NOTIFY mWatchdogNotify;
+STATIC EFI_EVENT mEfiExitBootServicesEvent;


STATIC
VOID
@@ -200,7 +204,13 @@ WatchdogSetTimerPeriod (
UINT64 TimerFrequencyHz;
UINT64 NumTimerTicks;


- // if TimerPeriod is 0, this is a request to stop the watchdog.
+ // If we've exited Boot Services but TimerPeriod isn't zero, this
+ // indicates that the caller is doing something wrong.
+ if (mExitedBootServices && (TimerPeriod != 0)) {
[SAMI] Thanks for updating the code to return the error code. 
However, I see you are not stopping the watchdog timer. Is this because you expect the watchdog period to expire and reset the system? 
Also, did you see an issue that motivated this patch, or this was just a case of hardening the code?
Can you provide more information, please?
[/SAMI]
+ return EFI_DEVICE_ERROR;
+ }
+
+ // If TimerPeriod is 0 this is a request to stop the watchdog.
if (TimerPeriod == 0) {
mTimerPeriod = 0;
WatchdogDisable ();
@@ -304,8 +314,6 @@ STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL mWatchdogTimer = {
WatchdogGetTimerPeriod
};


-STATIC EFI_EVENT mEfiExitBootServicesEvent;
-
EFI_STATUS
EFIAPI
GenericWatchdogEntry (
-- 
2.34.1







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113274): https://edk2.groups.io/g/devel/message/113274
Mute This Topic: https://groups.io/mt/103538118/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 3/3] ArmPkg: Disable watchdog interaction after exiting boot services

[Rebecca Cran via groups.io]

On 1/5/2024 4:12 AM, Sami Mujawar wrote:

> - // if TimerPeriod is 0, this is a request to stop the watchdog.
> + // If we've exited Boot Services but TimerPeriod isn't zero, this
> + // indicates that the caller is doing something wrong.
> + if (mExitedBootServices && (TimerPeriod != 0)) {
> [SAMI] Thanks for updating the code to return the error code.
> However, I see you are not stopping the watchdog timer. Is this because you expect the watchdog period to expire and reset the system?

I removed the code to stop the watchdog timer because it's an error 
condition. However, I've updated it so it does also get stopped in this 
case too.

> Also, did you see an issue that motivated this patch, or this was just a case of hardening the code?
> Can you provide more information, please?
> [/SAMI]

This was issues found and improvements made by various people in the 
last couple of years that we're now upstreaming to contribute 
improvements and reduce our diffs against upstream.

-- 
Rebecca Cran


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114018): https://edk2.groups.io/g/devel/message/114018
Mute This Topic: https://groups.io/mt/103538118/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/3] ArmPkg: GenericWatchdogDxe fixes and improvements

[Ard Biesheuvel]

On Fri, 5 Jan 2024 at 06:15, Rebecca Cran
<rebecca@os.amperecomputing.com> wrote:
>
> Fixes and improvements to GenericWatchdogDxe.
>

What is the difference between v2 and v3?

> PR: https://github.com/tianocore/edk2/pull/5176
>
> Rebecca Cran (3):
>   ArmPkg: Update GenericWatchdogDxe to allow setting full 48-bit offset
>   ArmPkg: Introduce global mTimerPeriod and remove calculation
>   ArmPkg: Disable watchdog interaction after exiting boot services
>
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h    |  6 ++-
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 60 +++++++++++++++++-------------
>  2 files changed, 40 insertions(+), 26 deletions(-)
>
> --
> 2.34.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113221): https://edk2.groups.io/g/devel/message/113221
Mute This Topic: https://groups.io/mt/103538117/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/3] ArmPkg: GenericWatchdogDxe fixes and improvements

[Rebecca Cran via groups.io]

On 1/5/2024 1:26 AM, Ard Biesheuvel via groups.io wrote:
> On Fri, 5 Jan 2024 at 06:15, Rebecca Cran
> <rebecca@os.amperecomputing.com> wrote:
>>
>> Fixes and improvements to GenericWatchdogDxe.
>>
> 
> What is the difference between v2 and v3?

Sorry, I should have said that. I forgot to build v2 and it had a bug in 
the exit boot services handler where I'd forgotten to update a line.

-- 
Rebecca Cran


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113298): https://edk2.groups.io/g/devel/message/113298
Mute This Topic: https://groups.io/mt/103538117/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-