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

[edk2-devel] [PATCH 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: Fix the calculation of the timer period in GenericWatchdogDxe
  ArmPkg: Disable watchdog interaction after exiting boot services

 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h    |  4 +++-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 32 +++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 13 deletions(-)

-- 
2.34.1



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

[edk2-devel] [PATCH 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    |  4 +++-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 14 +++++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index 9bc3bf47047c..504bc4cacb33 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -1,5 +1,6 @@
 /** @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
@@ -14,7 +15,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..05df101d5f4b 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_UINT32);
 }
 
 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 (#113106): https://edk2.groups.io/g/devel/message/113106
Mute This Topic: https://groups.io/mt/103510103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/3] ArmPkg: Fix the calculation of the timer period in GenericWatchdogDxe

[Rebecca Cran via groups.io]

Fix the calculation of the timer period in GenericWatchdogDxe: we need
to multiply before dividing to keep the values as integers.

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

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 05df101d5f4b..8f02f38c64e3 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -119,7 +119,7 @@ WatchdogInterruptHandler (
   // the timer period plus 1.
   //
   if (mWatchdogNotify != NULL) {
-    TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
+    TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
     mWatchdogNotify (TimerPeriod + 1);
   }
 
@@ -260,7 +260,7 @@ WatchdogGetTimerPeriod (
     return EFI_INVALID_PARAMETER;
   }
 
-  *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
+  *TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
 
   return EFI_SUCCESS;
 }
-- 
2.34.1



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

[edk2-devel] [PATCH 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, 9 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 8f02f38c64e3..912106eb6ad2 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -36,10 +36,14 @@ STATIC UINTN  mTimerFrequencyHz = 0;
    It is therefore stored here. 0 means the timer is not running. */
 STATIC UINT64  mNumTimerTicks = 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
@@ -91,7 +95,8 @@ WatchdogExitBootServicesEvent (
   )
 {
   WatchdogDisable ();
-  mNumTimerTicks = 0;
+  mNumTimerTicks      = 0;
+  mExitedBootServices = TRUE;
 }
 
 /* This function is called when the watchdog's first signal (WS0) goes high.
@@ -202,8 +207,9 @@ WatchdogSetTimerPeriod (
 {
   UINTN  SystemCount;
 
-  // if TimerPeriod is 0, this is a request to stop the watchdog.
-  if (TimerPeriod == 0) {
+  // if TimerPeriod is 0 or we've exited boot services,
+  // this is a request to stop the watchdog.
+  if (TimerPeriod == 0 || mExitedBootServices) {
     mNumTimerTicks = 0;
     WatchdogDisable ();
     return EFI_SUCCESS;
@@ -303,8 +309,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 (#113107): https://edk2.groups.io/g/devel/message/113107
Mute This Topic: https://groups.io/mt/103510105/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] ArmPkg: Fix the calculation of the timer period in GenericWatchdogDxe

[Ard Biesheuvel]

Hi Rebecca,

On Wed, 3 Jan 2024 at 21:44, Rebecca Cran
<rebecca@os.amperecomputing.com> wrote:
>
> Fix the calculation of the timer period in GenericWatchdogDxe: we need
> to multiply before dividing to keep the values as integers.
>
> Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com>
> ---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 05df101d5f4b..8f02f38c64e3 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -119,7 +119,7 @@ WatchdogInterruptHandler (
>    // the timer period plus 1.
>    //
>    if (mWatchdogNotify != NULL) {
> -    TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
> +    TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);

Could we just store the timer period in a global mTimerPeriod, and get
rid of mNumTimerTicks entirely? AFAICT, that would get rid of these
calculations as well.

>      mWatchdogNotify (TimerPeriod + 1);
>    }
>
> @@ -260,7 +260,7 @@ WatchdogGetTimerPeriod (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
> +  *TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
>
>    return EFI_SUCCESS;
>  }
> --
> 2.34.1
>


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

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

[Sami Mujawar]

Hi Rebecca,

Thank you for this patch.

I have some minor suggestions marked inline as [SAMI].

Regards,

Sami Mujawar

On 03/01/2024, 20:44, "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 | 4 +++-
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 14 +++++++++-----
2 files changed, 12 insertions(+), 6 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index 9bc3bf47047c..504bc4cacb33 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -1,5 +1,6 @@
/** @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
[SAMI] Is it possible to add reference to the Generic Watchdog spec, please? i.e. https://developer.arm.com/documentation/den0094/

@@ -14,7 +15,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..05df101d5f4b 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_UINT32);
[SAMI] I think the writes to the higher order bits in the GENERIC_WDOG_OFFSET_REG_HIGH register can be masked with MAX_UINT16 instead of MAX_UINT32.
}


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 (#113158): https://edk2.groups.io/g/devel/message/113158
Mute This Topic: https://groups.io/mt/103510103/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sami Mujawar]

Hi Rebecca,

Thank you for this patch.

I have some minor suggestions marked inline as [SAMI].

Regards,

Sami Mujawar	

On 03/01/2024, 20:44, "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, 9 insertions(+), 5 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 8f02f38c64e3..912106eb6ad2 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -36,10 +36,14 @@ STATIC UINTN mTimerFrequencyHz = 0;
It is therefore stored here. 0 means the timer is not running. */
STATIC UINT64 mNumTimerTicks = 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
@@ -91,7 +95,8 @@ WatchdogExitBootServicesEvent (
)
{
WatchdogDisable ();
- mNumTimerTicks = 0;
+ mNumTimerTicks = 0;
+ mExitedBootServices = TRUE;
}


/* This function is called when the watchdog's first signal (WS0) goes high.
@@ -202,8 +207,9 @@ WatchdogSetTimerPeriod (
{
UINTN SystemCount;


- // if TimerPeriod is 0, this is a request to stop the watchdog.
- if (TimerPeriod == 0) {
+ // if TimerPeriod is 0 or we've exited boot services,
+ // this is a request to stop the watchdog.
+ if (TimerPeriod == 0 || mExitedBootServices) {
[SAMI] I think we need extra brackets here. Also, should EFI_DEVICE_ERROR be returned if ExitBootServices is signalled but TimerPeriod != 0?
 i.e. we stop the watchdog but return an error to the caller so that it knows to fix the broken code.
[/SAMI]
mNumTimerTicks = 0;
WatchdogDisable ();
return EFI_SUCCESS;
@@ -303,8 +309,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 (#113159): https://edk2.groups.io/g/devel/message/113159
Mute This Topic: https://groups.io/mt/103510105/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Rebecca Cran via groups.io]

Thanks, I've incorporated the changes into the v2 patch series.

-- 
Rebecca

On 1/4/2024 3:01 AM, Sami Mujawar wrote:
> Hi Rebecca,
> 
> Thank you for this patch.
> 
> I have some minor suggestions marked inline as [SAMI].
> 
> Regards,
> 
> Sami Mujawar	
> 
> On 03/01/2024, 20:44, "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, 9 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 8f02f38c64e3..912106eb6ad2 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -36,10 +36,14 @@ STATIC UINTN mTimerFrequencyHz = 0;
> It is therefore stored here. 0 means the timer is not running. */
> STATIC UINT64 mNumTimerTicks = 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
> @@ -91,7 +95,8 @@ WatchdogExitBootServicesEvent (
> )
> {
> WatchdogDisable ();
> - mNumTimerTicks = 0;
> + mNumTimerTicks = 0;
> + mExitedBootServices = TRUE;
> }
> 
> 
> /* This function is called when the watchdog's first signal (WS0) goes high.
> @@ -202,8 +207,9 @@ WatchdogSetTimerPeriod (
> {
> UINTN SystemCount;
> 
> 
> - // if TimerPeriod is 0, this is a request to stop the watchdog.
> - if (TimerPeriod == 0) {
> + // if TimerPeriod is 0 or we've exited boot services,
> + // this is a request to stop the watchdog.
> + if (TimerPeriod == 0 || mExitedBootServices) {
> [SAMI] I think we need extra brackets here. Also, should EFI_DEVICE_ERROR be returned if ExitBootServices is signalled but TimerPeriod != 0?
>   i.e. we stop the watchdog but return an error to the caller so that it knows to fix the broken code.
> [/SAMI]
> mNumTimerTicks = 0;
> WatchdogDisable ();
> return EFI_SUCCESS;
> @@ -303,8 +309,6 @@ STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL mWatchdogTimer = {
> WatchdogGetTimerPeriod
> };
> 
> 
> -STATIC EFI_EVENT mEfiExitBootServicesEvent;
> -
> EFI_STATUS
> EFIAPI
> GenericWatchdogEntry (



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

Re: [edk2-devel] [PATCH 2/3] ArmPkg: Fix the calculation of the timer period in GenericWatchdogDxe

[Rebecca Cran via groups.io]

Thanks, I've incorporated the changes into the v2 patch series.

-- 
Rebecca

On 1/3/2024 3:56 PM, Ard Biesheuvel wrote:
> Hi Rebecca,
> 
> On Wed, 3 Jan 2024 at 21:44, Rebecca Cran
> <rebecca@os.amperecomputing.com> wrote:
>>
>> Fix the calculation of the timer period in GenericWatchdogDxe: we need
>> to multiply before dividing to keep the values as integers.
>>
>> Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com>
>> ---
>>   ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
>> index 05df101d5f4b..8f02f38c64e3 100644
>> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
>> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
>> @@ -119,7 +119,7 @@ WatchdogInterruptHandler (
>>     // the timer period plus 1.
>>     //
>>     if (mWatchdogNotify != NULL) {
>> -    TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
>> +    TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
> 
> Could we just store the timer period in a global mTimerPeriod, and get
> rid of mNumTimerTicks entirely? AFAICT, that would get rid of these
> calculations as well.
> 
>>       mWatchdogNotify (TimerPeriod + 1);
>>     }
>>
>> @@ -260,7 +260,7 @@ WatchdogGetTimerPeriod (
>>       return EFI_INVALID_PARAMETER;
>>     }
>>
>> -  *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
>> +  *TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
>>
>>     return EFI_SUCCESS;
>>   }
>> --
>> 2.34.1
>>



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