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

[edk2-devel] [PATCH v4 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

Changes between v3 and v4:

- Check Interface Identification Register for architecture revision before
  setting the high offset register value.
- Use @par for reference.
- Move setting of EBS flag from patch 2/3 to 3/3.
- Disable the watchdog in the case that EBS has been called and the timer
  period is non-zero.

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    | 11 +++-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 69 +++++++++++++-------
 2 files changed, 54 insertions(+), 26 deletions(-)

-- 
2.34.1



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

[edk2-devel] [PATCH v4 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    | 11 +++++++++-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 23 +++++++++++++++-----
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index 9bc3bf47047c..fb117832683f 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -1,9 +1,13 @@
 /** @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
 *
+*  @par Reference(s):
+*  - Generic Watchdog specification in Arm Base System Architecture 1.0C:
+*    https://developer.arm.com/documentation/den0094/c/
 **/
 
 #ifndef GENERIC_WATCHDOG_H_
@@ -14,12 +18,17 @@
 
 // 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)
+#define GENERIC_WDOG_IID_REG                 ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0xFCC)
 
 // Values of bit 0 of the Control/Status Register
 #define GENERIC_WDOG_ENABLED   1
 #define GENERIC_WDOG_DISABLED  0
 
+#define GENERIC_WDOG_IID_REV_SHIFT  16
+#define GENERIC_WDOG_IID_REV_MASK   0xF
+
 #endif // GENERIC_WATCHDOG_H_
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 66c6c37c08b0..20549aa91d94 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,23 @@ STATIC UINTN  mTimerFrequencyHz = 0;
    It is therefore stored here. 0 means the timer is not running. */
 STATIC UINT64  mNumTimerTicks = 0;
 
+STATIC UINT8  WatchdogRevision;
+
+#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);
+  if (WatchdogRevision == 1) {
+    MmioWrite32 (GENERIC_WDOG_OFFSET_REG_HIGH, (Value >> 32) & MAX_UINT16);
+  }
 }
 
 STATIC
@@ -211,17 +219,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 ();
   }
 
@@ -310,6 +318,7 @@ GenericWatchdogEntry (
 {
   EFI_STATUS  Status;
   EFI_HANDLE  Handle;
+  UINT32      WatchdogIId;
 
   Status = gBS->LocateProtocol (
                   &gHardwareInterrupt2ProtocolGuid,
@@ -367,7 +376,9 @@ GenericWatchdogEntry (
                   );
   ASSERT_EFI_ERROR (Status);
 
-  mNumTimerTicks = 0;
+  mNumTimerTicks   = 0;
+  WatchdogIId      = MmioRead32 (GENERIC_WDOG_IID_REG);
+  WatchdogRevision = (WatchdogIId >> GENERIC_WDOG_IID_REV_SHIFT) & GENERIC_WDOG_IID_REV_MASK;
   WatchdogDisable ();
 
   return EFI_SUCCESS;
-- 
2.34.1



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

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

[Sami Mujawar]

Hi Rebecca,

Thank you for this patch.

Please find my feedback inline marked as [SAMI].

Regards,

Sami Mujawar

On 19/01/2024, 15:46, "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 | 11 +++++++++-
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 23 +++++++++++++++-----
2 files changed, 27 insertions(+), 7 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index 9bc3bf47047c..fb117832683f 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -1,9 +1,13 @@
/** @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
*
+* @par Reference(s):
+* - Generic Watchdog specification in Arm Base System Architecture 1.0C:
+* https://developer.arm.com/documentation/den0094/c/ <https://developer.arm.com/documentation/den0094/c/>
**/


#ifndef GENERIC_WATCHDOG_H_
@@ -14,12 +18,17 @@


// 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)
+#define GENERIC_WDOG_IID_REG ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0xFCC)


// Values of bit 0 of the Control/Status Register
#define GENERIC_WDOG_ENABLED 1
#define GENERIC_WDOG_DISABLED 0


+#define GENERIC_WDOG_IID_REV_SHIFT 16
+#define GENERIC_WDOG_IID_REV_MASK 0xF
[SAMI] I think the above macros should be prefixed as GENERIC_WDOG_IID_ARCH_REV_ as this is the Generic Watchdog architecture revision and not the Component revision.

+
#endif // GENERIC_WATCHDOG_H_
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 66c6c37c08b0..20549aa91d94 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,23 @@ STATIC UINTN mTimerFrequencyHz = 0;
It is therefore stored here. 0 means the timer is not running. */
STATIC UINT64 mNumTimerTicks = 0;


+STATIC UINT8 WatchdogRevision;
[SAMI] Similarly this should be changed to WatchdogArchRevision.
+
+#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);
+ if (WatchdogRevision == 1) {
+ MmioWrite32 (GENERIC_WDOG_OFFSET_REG_HIGH, (Value >> 32) & MAX_UINT16);
+ }
}


STATIC
@@ -211,17 +219,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) {
[SAMI] The SBSA 3.1 spec https://developer.arm.com/documentation/den0029/a/?lang=en, Revision 3.1A, section 5.2, states 'The watchdog offset register is 32 bits wide.'
Considering that, I think the above check should be MAX_UINT32 for WatchdogArchRevision == 0. 
I think it may be good to define a static function GetMaxWatchdogOffsetRegisterValue() that does the arch revision check and returns either MAX_UINT32 or MAX_UINT48.
[/SAMI]	
/* 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 ();
}
	

@@ -310,6 +318,7 @@ GenericWatchdogEntry (
{
EFI_STATUS Status;
EFI_HANDLE Handle;
+ UINT32 WatchdogIId;
[SAMI] Minor, the above variable name should be WatchdogIid to comply with the coding standard.


Status = gBS->LocateProtocol (
&gHardwareInterrupt2ProtocolGuid,
@@ -367,7 +376,9 @@ GenericWatchdogEntry (
);
ASSERT_EFI_ERROR (Status);


- mNumTimerTicks = 0;
+ mNumTimerTicks = 0;
+ WatchdogIId = MmioRead32 (GENERIC_WDOG_IID_REG);
+ WatchdogRevision = (WatchdogIId >> GENERIC_WDOG_IID_REV_SHIFT) & GENERIC_WDOG_IID_REV_MASK;
WatchdogDisable ();


return EFI_SUCCESS;
-- 
2.34.1







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

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

[Rebecca Cran via groups.io]

On 1/23/2024 7:10 AM, Sami Mujawar wrote:

> @@ -310,6 +318,7 @@ GenericWatchdogEntry (
> {
> EFI_STATUS Status;
> EFI_HANDLE Handle;
> + UINT32 WatchdogIId;
> [SAMI] Minor, the above variable name should be WatchdogIid to comply with the coding standard.

I was wondering about the naming, since it's the "Watchdog Interface 
ID". Since the second "I" is for another word, should it still be 
lower-case?

-- 
Rebecca Cran



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

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

[Sami Mujawar]

Hi Rebecca,

On 24/01/2024, 19:36, "Rebecca Cran" <rebecca@os.amperecomputing.com <mailto:rebecca@os.amperecomputing.com>> wrote:


On 1/23/2024 7:10 AM, Sami Mujawar wrote:


> @@ -310,6 +318,7 @@ GenericWatchdogEntry (
> {
> EFI_STATUS Status;
> EFI_HANDLE Handle;
> + UINT32 WatchdogIId;
> [SAMI] Minor, the above variable name should be WatchdogIid to comply with the coding standard.


I was wondering about the naming, since it's the "Watchdog Interface 
ID". Since the second "I" is for another word, should it still be 
lower-case?
[SAMI] I agree. If it passes the CI checks (uncrustify/ecc), I think it should be ok.

Regards,

Sami Mujawar

-- 
Rebecca Cran







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

[edk2-devel] [PATCH v4 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 | 35 +++++++++-----------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 20549aa91d94..8dd247c44e8f 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;
 
 STATIC UINT8  WatchdogRevision;
 
@@ -95,7 +92,7 @@ WatchdogExitBootServicesEvent (
   )
 {
   WatchdogDisable ();
-  mNumTimerTicks = 0;
+  mTimerPeriod        = 0;
 }
 
 /* This function is called when the watchdog's first signal (WS0) goes high.
@@ -110,7 +107,6 @@ WatchdogInterruptHandler (
   )
 {
   STATIC CONST CHAR16  ResetString[] = L"The generic watchdog timer ran out.";
-  UINT64               TimerPeriod;
 
   WatchdogDisable ();
 
@@ -123,8 +119,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 (
@@ -204,22 +199,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
@@ -227,9 +227,9 @@ WatchdogSetTimerPeriod (
     WatchdogWriteOffsetRegister (MAX_UINT48);
     WatchdogEnable ();
     SystemCount = ArmGenericTimerGetSystemCount ();
-    WatchdogWriteCompareRegister (SystemCount + mNumTimerTicks);
+    WatchdogWriteCompareRegister (SystemCount + NumTimerTicks);
   } else {
-    WatchdogWriteOffsetRegister (mNumTimerTicks);
+    WatchdogWriteOffsetRegister (NumTimerTicks);
     WatchdogEnable ();
   }
 
@@ -264,7 +264,7 @@ WatchdogGetTimerPeriod (
     return EFI_INVALID_PARAMETER;
   }
 
-  *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
+  *TimerPeriod = mTimerPeriod;
 
   return EFI_SUCCESS;
 }
@@ -332,9 +332,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,
@@ -376,9 +373,9 @@ GenericWatchdogEntry (
                   );
   ASSERT_EFI_ERROR (Status);
 
-  mNumTimerTicks   = 0;
   WatchdogIId      = MmioRead32 (GENERIC_WDOG_IID_REG);
   WatchdogRevision = (WatchdogIId >> GENERIC_WDOG_IID_REV_SHIFT) & GENERIC_WDOG_IID_REV_MASK;
+  mTimerPeriod     = 0;
   WatchdogDisable ();
 
   return EFI_SUCCESS;
-- 
2.34.1



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

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

[Sami Mujawar]

Hi Rebecca,

Thank you for this patch.
These changes look good to me.

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

Regards,

Sami Mujawar

On 19/01/2024, 15:46, "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 | 35 +++++++++-----------
1 file changed, 16 insertions(+), 19 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 20549aa91d94..8dd247c44e8f 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;


STATIC UINT8 WatchdogRevision;


@@ -95,7 +92,7 @@ WatchdogExitBootServicesEvent (
)
{
WatchdogDisable ();
- mNumTimerTicks = 0;
+ mTimerPeriod = 0;
}


/* This function is called when the watchdog's first signal (WS0) goes high.
@@ -110,7 +107,6 @@ WatchdogInterruptHandler (
)
{
STATIC CONST CHAR16 ResetString[] = L"The generic watchdog timer ran out.";
- UINT64 TimerPeriod;


WatchdogDisable ();


@@ -123,8 +119,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 (
@@ -204,22 +199,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
@@ -227,9 +227,9 @@ WatchdogSetTimerPeriod (
WatchdogWriteOffsetRegister (MAX_UINT48);
WatchdogEnable ();
SystemCount = ArmGenericTimerGetSystemCount ();
- WatchdogWriteCompareRegister (SystemCount + mNumTimerTicks);
+ WatchdogWriteCompareRegister (SystemCount + NumTimerTicks);
} else {
- WatchdogWriteOffsetRegister (mNumTimerTicks);
+ WatchdogWriteOffsetRegister (NumTimerTicks);
WatchdogEnable ();
}


@@ -264,7 +264,7 @@ WatchdogGetTimerPeriod (
return EFI_INVALID_PARAMETER;
}


- *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
+ *TimerPeriod = mTimerPeriod;


return EFI_SUCCESS;
}
@@ -332,9 +332,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,
@@ -376,9 +373,9 @@ GenericWatchdogEntry (
);
ASSERT_EFI_ERROR (Status);


- mNumTimerTicks = 0;
WatchdogIId = MmioRead32 (GENERIC_WDOG_IID_REG);
WatchdogRevision = (WatchdogIId >> GENERIC_WDOG_IID_REV_SHIFT) & GENERIC_WDOG_IID_REV_MASK;
+ mTimerPeriod = 0;
WatchdogDisable ();


return EFI_SUCCESS;
--
2.34.1





IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


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

[edk2-devel] [PATCH v4 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 | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 8dd247c44e8f..189194a2b9ee 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -35,10 +35,14 @@ STATIC UINT64  mTimerPeriod = 0;
 
 STATIC UINT8  WatchdogRevision;
 
+/* 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
@@ -93,6 +97,7 @@ WatchdogExitBootServicesEvent (
 {
   WatchdogDisable ();
   mTimerPeriod        = 0;
+  mExitedBootServices = TRUE;
 }
 
 /* This function is called when the watchdog's first signal (WS0) goes high.
@@ -203,7 +208,15 @@ 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)) {
+    mTimerPeriod = 0;
+    WatchdogDisable ();
+    return EFI_DEVICE_ERROR;
+  }
+
+  // If TimerPeriod is 0 this is a request to stop the watchdog.
   if (TimerPeriod == 0) {
     mTimerPeriod = 0;
     WatchdogDisable ();
@@ -307,8 +320,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 (#114089): https://edk2.groups.io/g/devel/message/114089
Mute This Topic: https://groups.io/mt/103832320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Rebecca,

Thank you for this patch.
I have a minor suggestion marked inline as [SAMI].
Otherwise, this patch looks good to me.

With that fixed,
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 19/01/2024, 15:46, "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 | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 8dd247c44e8f..189194a2b9ee 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -35,10 +35,14 @@ STATIC UINT64 mTimerPeriod = 0;


STATIC UINT8 WatchdogRevision;


+/* 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
@@ -93,6 +97,7 @@ WatchdogExitBootServicesEvent (
{
WatchdogDisable ();
mTimerPeriod = 0;
+ mExitedBootServices = TRUE;
}


/* This function is called when the watchdog's first signal (WS0) goes high.
@@ -203,7 +208,15 @@ 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)) {
+ mTimerPeriod = 0;
+ WatchdogDisable ();
+ return EFI_DEVICE_ERROR;
[SAMI] Minor, the function documentation header needs updating to reflect the new error code being returned.
+ }
+
+ // If TimerPeriod is 0 this is a request to stop the watchdog.
if (TimerPeriod == 0) {
mTimerPeriod = 0;
WatchdogDisable ();
@@ -307,8 +320,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 (#114201): https://edk2.groups.io/g/devel/message/114201
Mute This Topic: https://groups.io/mt/103832320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-