[PATCH v1 0/1] Add SBSA watchdog rev 1 support to ARM Generic Watchdog driver

[PATCH v1 0/1] Add SBSA watchdog rev 1 support to ARM Generic Watchdog driver

[rajesh.ravi]

From: Rajesh Ravi <rajesh.ravi@broadcom.com>


Signed-off-by: Rajesh Ravi <rajesh.ravi@broadcom.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>

Add SBSA watchdog rev 1 support to ARM Generic Watchdog driver.

https://github.com/RajeshRavi-brcm/edk2/pull/new/sbsa_wd

Rajesh Ravi (1):
  ArmPkg/GenericWatchdogDxe: Add rev 1 support to ARM Generic Watchdog
    driver

 .../GenericWatchdogDxe/GenericWatchdog.h      |  8 ++-
 .../GenericWatchdogDxe/GenericWatchdogDxe.c   | 53 +++++++++++++++++--
 2 files changed, 56 insertions(+), 5 deletions(-)


-- 
2.17.1

[PATCH v1 1/1] ArmPkg/GenericWatchdogDxe: Add rev 1 support to ARM Generic Watchdog driver

[rajesh.ravi]

From: Rajesh Ravi <rajesh.ravi@broadcom.com>

Add SBSA watchdog rev 1 support to ARM Generic Watchdog driver.

Signed-off-by: Rajesh Ravi <rajesh.ravi@broadcom.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h    |  8 ++-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 53 ++++++++++++++++++--
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index c64bc5c4627d..ed74bcf95021 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -13,12 +13,18 @@
 
 // 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_IIDR_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_ARCH_REV_OFFSET 16
+#define GENERIC_WDOG_ARCH_REV_MASK   0xF
+#define SBSA_WDOG_WOR_WIDTH          48
+#define MAX_OFFSET_REG_VAL           (((UINT64)1 << 48) - 1)
 
 #endif  // __GENERIC_WATCHDOG_H__
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index f79cc9170f8a..7ef9b1ff08ab 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -35,16 +35,53 @@ STATIC UINTN mTimerFrequencyHz = 0;
    It is therefore stored here. 0 means the timer is not running. */
 STATIC UINT64 mNumTimerTicks = 0;
 
+STATIC UINT32 mWatchDogRev = 0;
+
+STATIC UINT64 mWatchdogMaxOffsetVal = MAX_UINT32;
+
 STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
 STATIC EFI_WATCHDOG_TIMER_NOTIFY        mWatchdogNotify;
 
+STATIC
+inline
+UINT32
+WatchdogReadRevisionRegister (
+  VOID
+  )
+{
+  return MmioRead32 (GENERIC_WDOG_IIDR_REG);
+}
+
+STATIC
+inline
+UINT32
+WatchdogGetRevision (
+  VOID
+  )
+{
+  UINT32 IidrRegVal;
+
+  IidrRegVal = WatchdogReadRevisionRegister();
+
+  return ((IidrRegVal >> GENERIC_WDOG_ARCH_REV_OFFSET) & GENERIC_WDOG_ARCH_REV_MASK);
+}
+
 STATIC
 VOID
 WatchdogWriteOffsetRegister (
-  UINT32  Value
+  UINT64  Value
   )
 {
-  MmioWrite32 (GENERIC_WDOG_OFFSET_REG, Value);
+  if(Value >> SBSA_WDOG_WOR_WIDTH) {
+    return;
+  }
+
+  MmioWrite32 (GENERIC_WDOG_OFFSET_REG_LOW, ((UINT32)Value & MAX_UINT32));
+  if (mWatchDogRev) {
+    MmioWrite32 (GENERIC_WDOG_OFFSET_REG_HIGH, (Value >> 32) & MAX_UINT32);
+  } else {
+    MmioWrite32 (GENERIC_WDOG_OFFSET_REG_HIGH, 0);
+  }
 }
 
 STATIC
@@ -207,12 +244,12 @@ 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 > mWatchdogMaxOffsetVal) {
     /* 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_OFFSET_REG_VAL);
     WatchdogEnable ();
     SystemCount = ArmGenericTimerGetSystemCount ();
     WatchdogWriteCompareRegister (SystemCount + mNumTimerTicks);
@@ -319,6 +356,14 @@ GenericWatchdogEntry (
   mTimerFrequencyHz = ArmGenericTimerGetTimerFreq ();
   ASSERT (mTimerFrequencyHz != 0);
 
+  mWatchDogRev = WatchdogGetRevision();
+
+  if (mWatchDogRev) {
+    mWatchdogMaxOffsetVal = MAX_OFFSET_REG_VAL;
+  } else {
+    mWatchdogMaxOffsetVal = MAX_UINT32;
+  }
+
   // Install interrupt handler
   Status = mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol,
                                  FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
-- 
2.17.1

Re: [edk2-devel] [PATCH v1 1/1] ArmPkg/GenericWatchdogDxe: Add rev 1 support to ARM Generic Watchdog driver

[Sami Mujawar]

Hi Rajesh,

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

Regards,

Sami Mujawar

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of rajesh.ravi@broadcom.com via groups.io
Sent: 08 October 2020 03:56 PM
To: devel@edk2.groups.io
Cc: Rajesh Ravi <rajesh.ravi@broadcom.com>; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>
Subject: [edk2-devel] [PATCH v1 1/1] ArmPkg/GenericWatchdogDxe: Add rev 1 support to ARM Generic Watchdog driver

From: Rajesh Ravi <rajesh.ravi@broadcom.com>

Add SBSA watchdog rev 1 support to ARM Generic Watchdog driver.

Signed-off-by: Rajesh Ravi <rajesh.ravi@broadcom.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h    |  8 ++-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 53 ++++++++++++++++++--
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
index c64bc5c4627d..ed74bcf95021 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
@@ -13,12 +13,18 @@

 // 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_IIDR_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_ARCH_REV_OFFSET 16
+#define GENERIC_WDOG_ARCH_REV_MASK   0xF
+#define SBSA_WDOG_WOR_WIDTH          48
+#define MAX_OFFSET_REG_VAL           (((UINT64)1 << 48) - 1)

 #endif  // __GENERIC_WATCHDOG_H__
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index f79cc9170f8a..7ef9b1ff08ab 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -35,16 +35,53 @@ STATIC UINTN mTimerFrequencyHz = 0;
    It is therefore stored here. 0 means the timer is not running. */
 STATIC UINT64 mNumTimerTicks = 0;

+STATIC UINT32 mWatchDogRev = 0;
+
+STATIC UINT64 mWatchdogMaxOffsetVal = MAX_UINT32;
+
 STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
 STATIC EFI_WATCHDOG_TIMER_NOTIFY        mWatchdogNotify;

+STATIC
+inline
+UINT32
+WatchdogReadRevisionRegister (
+  VOID
+  )

[SAMI] Please add doxygen style function headers. Same at other places.[/SAMI]

+{
+  return MmioRead32 (GENERIC_WDOG_IIDR_REG);
+}
+
+STATIC
+inline
+UINT32
+WatchdogGetRevision (
+  VOID
+  )
+{
+  UINT32 IidrRegVal;
+
+  IidrRegVal = WatchdogReadRevisionRegister();
+
+  return ((IidrRegVal >> GENERIC_WDOG_ARCH_REV_OFFSET) & GENERIC_WDOG_ARCH_REV_MASK);
+}
+
 STATIC
 VOID
 WatchdogWriteOffsetRegister (
-  UINT32  Value
+  UINT64  Value
   )
 {
-  MmioWrite32 (GENERIC_WDOG_OFFSET_REG, Value);
+  if(Value >> SBSA_WDOG_WOR_WIDTH) {

[SAMI] The condition in an if statement must be a Boolean expression. Same at other places. [/SAMI]

+    return;
+  }
+
+  MmioWrite32 (GENERIC_WDOG_OFFSET_REG_LOW, ((UINT32)Value & MAX_UINT32));
+  if (mWatchDogRev) {
+    MmioWrite32 (GENERIC_WDOG_OFFSET_REG_HIGH, (Value >> 32) & MAX_UINT32);
+  } else {
+    MmioWrite32 (GENERIC_WDOG_OFFSET_REG_HIGH, 0);

[SAMI] According to SBSA spec v3.1, the offsets 0x00C - 0x00F are reserved for Generic Watchdog version 0.  Writing zeros may have undesirable results on some platforms. [/SAMI]

+  }
 }

 STATIC
@@ -207,12 +244,12 @@ 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 > mWatchdogMaxOffsetVal) {
     /* 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_OFFSET_REG_VAL);
     WatchdogEnable ();
     SystemCount = ArmGenericTimerGetSystemCount ();
     WatchdogWriteCompareRegister (SystemCount + mNumTimerTicks);
@@ -319,6 +356,14 @@ GenericWatchdogEntry (
   mTimerFrequencyHz = ArmGenericTimerGetTimerFreq ();
   ASSERT (mTimerFrequencyHz != 0);

+  mWatchDogRev = WatchdogGetRevision();

[SAMI] Space needed between function name and opening bracket. [/SAMI]

+
+  if (mWatchDogRev) {
+    mWatchdogMaxOffsetVal = MAX_OFFSET_REG_VAL;
+  } else {
+    mWatchdogMaxOffsetVal = MAX_UINT32;
+  }
+
   // Install interrupt handler
   Status = mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol,
                                  FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
--
2.17.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.