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

2024-01-05 Thread 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" mailto:rebe...@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 mailto:rebe...@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.
* 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/ 

[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.
* 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 0xULL
+
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-01-04 Thread 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 
---
 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.
 *  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.
 *  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  0xULL
+
 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-