[PATCH 0/5] Add HardwareInterrupt2 for ARM

[PATCH 0/5] Add HardwareInterrupt2 for ARM

[evan.lloyd]

From: EvanLloyd <evan.lloyd@arm.com>

This v3 series of patches corrects a problem detected on the ARM Juno
platform that is actually generic (at least to ARM GIC platforms).
The HardwareInterrupt protocol had no means of handling characteristics
like Edge/Level triggered and polarity.

A new HardwareInterrupt2 protocol (provided by Ard) is added, and code
changed to utilise the new capabilities.

The code is available for examination on Github at:
    https://github.com/EvanLloyd/tianocore/tree/376_irqtype_v3

v3 merely modifies instances of conditional assigment to comply with
maintainers preference for assignments within an if statement.

Note: Significant defects exist in the (original) Watchdog handling,
      and a new patch will follow.


Ard Biesheuvel (3):
  EmbeddedPkg: introduce HardwareInterrupt2 protocol
  ArmPkg/ArmGicDxe: expose HardwareInterrupt2 protocol
  ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type

Girish Pathak (1):
  ArmPkg:Provide GetTriggerType/SetTriggerType functions
*** BLURB HERE ***

Ard Biesheuvel (3):
  EmbeddedPkg: Introduce HardwareInterrupt2 protocol
  ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol
  ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type

Evan Lloyd (2):
  ArmPkg: Tidy GIC code before changes.
  ArmPkg: Tidy up GenericWatchdogDxe.c

 EmbeddedPkg/EmbeddedPkg.dec                              |   1 +
 ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                      |   3 +-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |   6 +-
 ArmPkg/Drivers/ArmGic/ArmGicDxe.h                        |  37 +++-
 ArmPkg/Include/Library/ArmGicLib.h                       |  39 ++--
 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h        | 182 ++++++++++++++++
 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c                  |  80 +++++--
 ArmPkg/Drivers/ArmGic/ArmGicLib.c                        |  72 +++++--
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c                | 196 ++++++++++++++++--
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c                | 219 +++++++++++++++++---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 151 +++++++-------
 11 files changed, 804 insertions(+), 182 deletions(-)
 create mode 100644 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

[PATCH 1/5] ArmPkg: Tidy GIC code before changes.

[evan.lloyd]

From: Evan Lloyd <evan.lloyd@arm.com>

This change is purely cosmetic, to tidy some code before change.
Mods involve:
    Reflow overlength comments.
    Split overlength code lines.
    Make protocol functions STATIC.
    Remove "Horor vacui" comments.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Signed-off-by: Girish Pathak <girish.pathak@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicDxe.h                      | 12 +--
 ArmPkg/Include/Library/ArmGicLib.h                     | 29 ++++---
 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c                | 36 +++++----
 ArmPkg/Drivers/ArmGic/ArmGicLib.c                      | 72 +++++++++++++----
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c              | 57 +++++++++-----
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c              | 81 +++++++++++++-------
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 12 +--
 7 files changed, 195 insertions(+), 104 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
index af33aa90b00c6775e10a831d63ed707394862362..1018f2004e75d879a72c2d6bf37b64051e720d12 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
+++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
@@ -28,9 +28,9 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 extern UINTN                        mGicNumInterrupts;
 extern HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers;
 
-//
+
 // Common API
-//
+
 EFI_STATUS
 InstallAndRegisterInterruptService (
   IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
@@ -46,18 +46,18 @@ RegisterInterruptSource (
   IN HARDWARE_INTERRUPT_HANDLER         Handler
   );
 
-//
+
 // GicV2 API
-//
+
 EFI_STATUS
 GicV2DxeInitialize (
   IN EFI_HANDLE         ImageHandle,
   IN EFI_SYSTEM_TABLE   *SystemTable
   );
 
-//
+
 // GicV3 API
-//
+
 EFI_STATUS
 GicV3DxeInitialize (
   IN EFI_HANDLE         ImageHandle,
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 4364f3ffef464596f64cf59881d703cf54cf0ddd..f7b546895d116f81c65a853fcdb067ec7601b2da 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -17,9 +17,9 @@
 
 #include <Library/ArmGicArchLib.h>
 
-//
+
 // GIC Distributor
-//
+
 #define ARM_GIC_ICDDCR          0x000 // Distributor Control Register
 #define ARM_GIC_ICDICTR         0x004 // Interrupt Controller Type Register
 #define ARM_GIC_ICDIIDR         0x008 // Implementer Identification Register
@@ -51,9 +51,9 @@
 #define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)
 #define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)
 
-//
+
 // GIC Redistributor
-//
+
 
 #define ARM_GICR_CTLR_FRAME_SIZE    SIZE_64KB
 #define ARM_GICR_SGI_PPI_FRAME_SIZE SIZE_64KB
@@ -65,9 +65,9 @@
 #define ARM_GICR_ISENABLER      0x0100  // Interrupt Set-Enable Registers
 #define ARM_GICR_ICENABLER      0x0180  // Interrupt Clear-Enable Registers
 
-//
+
 // GIC Cpu interface
-//
+
 #define ARM_GIC_ICCICR          0x00  // CPU Interface Control Register
 #define ARM_GIC_ICCPMR          0x04  // Interrupt Priority Mask Register
 #define ARM_GIC_ICCBPR          0x08  // Binary Point Register
@@ -104,9 +104,9 @@ ArmGicGetInterfaceIdentification (
   IN  INTN          GicInterruptInterfaceBase
   );
 
-//
+
 // GIC Secure interfaces
-//
+
 VOID
 EFIAPI
 ArmGicSetupNonSecure (
@@ -170,7 +170,8 @@ ArmGicSendSgiTo (
  * in the GICv3 the register value is only the InterruptId.
  *
  * @param GicInterruptInterfaceBase   Base Address of the GIC CPU Interface
- * @param InterruptId                 InterruptId read from the Interrupt Acknowledge Register
+ * @param InterruptId                 InterruptId read from the Interrupt
+ *                                    Acknowledge Register
  *
  * @retval value returned by the Interrupt Acknowledge Register
  *
@@ -220,12 +221,12 @@ ArmGicIsInterruptEnabled (
   IN UINTN                  Source
   );
 
-//
 // GIC revision 2 specific declarations
-//
 
-// Interrupts from 1020 to 1023 are considered as special interrupts (eg: spurious interrupts)
-#define ARM_GIC_IS_SPECIAL_INTERRUPTS(Interrupt) (((Interrupt) >= 1020) && ((Interrupt) <= 1023))
+// Interrupts from 1020 to 1023 are considered as special interrupts
+// (eg: spurious interrupts)
+#define ARM_GIC_IS_SPECIAL_INTERRUPTS(Interrupt) \
+          (((Interrupt) >= 1020) && ((Interrupt) <= 1023))
 
 VOID
 EFIAPI
@@ -260,9 +261,7 @@ ArmGicV2EndOfInterrupt (
   IN UINTN                  Source
   );
 
-//
 // GIC revision 3 specific declarations
-//
 
 #define ICC_SRE_EL2_SRE         (1 << 0)
 
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
index be77b8361c5af033fd2889cdb48902af867f321d..88cb455b75bb8e8cb22157643a392403ce93129d 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
@@ -28,14 +28,14 @@ ExitBootServicesEvent (
   IN VOID       *Context
   );
 
-//
+
 // Making this global saves a few bytes in image size
-//
+
 EFI_HANDLE  gHardwareInterruptHandle = NULL;
 
-//
+
 // Notifications
-//
+
 EFI_EVENT EfiExitBootServicesEvent      = (EFI_EVENT)NULL;
 
 // Maximum Number of Interrupts
@@ -96,7 +96,8 @@ InstallAndRegisterInterruptService (
   EFI_CPU_ARCH_PROTOCOL   *Cpu;
 
   // Initialize the array for the Interrupt Handlers
-  gRegisteredInterruptHandlers = (HARDWARE_INTERRUPT_HANDLER*)AllocateZeroPool (sizeof(HARDWARE_INTERRUPT_HANDLER) * mGicNumInterrupts);
+  gRegisteredInterruptHandlers = (HARDWARE_INTERRUPT_HANDLER*)
+    AllocateZeroPool (sizeof(HARDWARE_INTERRUPT_HANDLER) * mGicNumInterrupts);
   if (gRegisteredInterruptHandlers == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
@@ -110,32 +111,41 @@ InstallAndRegisterInterruptService (
     return Status;
   }
 
-  //
+
   // Get the CPU protocol that this driver requires.
-  //
+
   Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&Cpu);
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
-  //
+
   // Unregister the default exception handler.
-  //
+
   Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ, NULL);
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
-  //
+
   // Register to receive interrupts
-  //
-  Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ, InterruptHandler);
+  Status = Cpu->RegisterInterruptHandler (
+             Cpu,
+             ARM_ARCH_EXCEPTION_IRQ,
+             InterruptHandler
+             );
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
   // Register for an ExitBootServicesEvent
-  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY, ExitBootServicesEvent, NULL, &EfiExitBootServicesEvent);
+  Status = gBS->CreateEvent (
+             EVT_SIGNAL_EXIT_BOOT_SERVICES,
+             TPL_NOTIFY,
+             ExitBootServicesEvent,
+             NULL,
+             &EfiExitBootServicesEvent
+             );
 
   return Status;
 }
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index e658e9bff5d8107b3914bdf1e9e1e51a4e4d4cd7..852330b80db9726f860f6868f7e90d48756b6e58 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -55,13 +55,17 @@ GicGetCpuRedistributorBase (
   UINTN GicCpuRedistributorBase;
 
   MpId = ArmReadMpidr ();
-  // Define CPU affinity as Affinity0[0:8], Affinity1[9:15], Affinity2[16:23], Affinity3[24:32]
+  // Define CPU affinity as Affinity0[0:8], Affinity1[9:15],
+  //   Affinity2[16:23], Affinity3[24:32]
   // whereas Affinity3 is defined at [32:39] in MPIDR
-  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2)) | ((MpId & ARM_CORE_AFF3) >> 8);
+  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2))
+                | ((MpId & ARM_CORE_AFF3) >> 8);
 
   if (Revision == ARM_GIC_ARCH_REVISION_3) {
-    // 2 x 64KB frame: Redistributor control frame + SGI Control & Generation frame
-    GicRedistributorGranularity = ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_SGI_PPI_FRAME_SIZE;
+    // 2 x 64KB frame:
+    //   Redistributor control frame + SGI Control & Generation frame
+    GicRedistributorGranularity = ARM_GICR_CTLR_FRAME_SIZE
+                                  + ARM_GICR_SGI_PPI_FRAME_SIZE;
   } else {
     ASSERT_EFI_ERROR (EFI_UNSUPPORTED);
     return 0;
@@ -112,7 +116,10 @@ ArmGicSendSgiTo (
   IN  INTN          SgiId
   )
 {
-  MmioWrite32 (GicDistributorBase + ARM_GIC_ICDSGIR, ((TargetListFilter & 0x3) << 24) | ((CPUTargetList & 0xFF) << 16) | SgiId);
+  MmioWrite32 (
+    GicDistributorBase + ARM_GIC_ICDSGIR,
+    ((TargetListFilter & 0x3) << 24) | ((CPUTargetList & 0xFF) << 16) | SgiId
+    );
 }
 
 /*
@@ -123,7 +130,8 @@ ArmGicSendSgiTo (
  * in the GICv3 the register value is only the InterruptId.
  *
  * @param GicInterruptInterfaceBase   Base Address of the GIC CPU Interface
- * @param InterruptId                 InterruptId read from the Interrupt Acknowledge Register
+ * @param InterruptId                 InterruptId read from the Interrupt
+ *                                    Acknowledge Register
  *
  * @retval value returned by the Interrupt Acknowledge Register
  *
@@ -200,16 +208,28 @@ ArmGicEnableInterrupt (
       FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
       SourceIsSpi (Source)) {
     // Write set-enable register
-    MmioWrite32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset), 1 << RegShift);
+    MmioWrite32 (
+      GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset),
+      1 << RegShift
+      );
   } else {
-    GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);
+    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
+                                GicRedistributorBase,
+                                Revision
+                                );
     if (GicCpuRedistributorBase == 0) {
       ASSERT_EFI_ERROR (EFI_NOT_FOUND);
       return;
     }
 
     // Write set-enable register
-    MmioWrite32 (GicCpuRedistributorBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISENABLER + (4 * RegOffset), 1 << RegShift);
+    MmioWrite32 (
+      (GicCpuRedistributorBase
+        + ARM_GICR_CTLR_FRAME_SIZE
+        + ARM_GICR_ISENABLER
+        + (4 * RegOffset)),
+      1 << RegShift
+      );
   }
 }
 
@@ -235,15 +255,27 @@ ArmGicDisableInterrupt (
       FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
       SourceIsSpi (Source)) {
     // Write clear-enable register
-    MmioWrite32 (GicDistributorBase + ARM_GIC_ICDICER + (4 * RegOffset), 1 << RegShift);
+    MmioWrite32 (
+      GicDistributorBase + ARM_GIC_ICDICER + (4 * RegOffset),
+      1 << RegShift
+      );
   } else {
-    GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);
+    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
+      GicRedistributorBase,
+      Revision
+      );
     if (GicCpuRedistributorBase == 0) {
       return;
     }
 
     // Write clear-enable register
-    MmioWrite32 (GicCpuRedistributorBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ICENABLER + (4 * RegOffset), 1 << RegShift);
+    MmioWrite32 (
+      (GicCpuRedistributorBase
+        + ARM_GICR_CTLR_FRAME_SIZE
+        + ARM_GICR_ICENABLER
+        + (4 * RegOffset)),
+      1 << RegShift
+      );
   }
 }
 
@@ -269,15 +301,25 @@ ArmGicIsInterruptEnabled (
   if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
       FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
       SourceIsSpi (Source)) {
-    Interrupts = ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)) & (1 << RegShift)) != 0);
+    Interrupts = ((MmioRead32 (
+                     GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)
+                     )
+                  & (1 << RegShift)) != 0);
   } else {
-    GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);
+    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
+                                GicRedistributorBase,
+                                Revision
+                                );
     if (GicCpuRedistributorBase == 0) {
       return 0;
     }
 
     // Read set-enable register
-    Interrupts = MmioRead32 (GicCpuRedistributorBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISENABLER + (4 * RegOffset));
+    Interrupts = MmioRead32 (
+                   GicCpuRedistributorBase
+                   + ARM_GICR_CTLR_FRAME_SIZE
+                   + ARM_GICR_ISENABLER
+                   + (4 * RegOffset));
   }
 
   return ((Interrupts & (1 << RegShift)) != 0);
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index b9ecd5543a3e2e0b00fffbcf5543a60567bb5dde..50ec90207b515d849cbf64f0a4b0d639b3868e60 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -2,7 +2,7 @@
 
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
-Portions copyright (c) 2011-2016, ARM Ltd. All rights reserved.<BR>
+Portions copyright (c) 2011-2017, ARM Ltd. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -43,6 +43,7 @@ STATIC UINT32 mGicDistributorBase;
   @retval EFI_UNSUPPORTED   Source interrupt is not supported
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 GicV2EnableInterruptSource (
@@ -70,6 +71,7 @@ GicV2EnableInterruptSource (
   @retval EFI_UNSUPPORTED   Source interrupt is not supported
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 GicV2DisableInterruptSource (
@@ -98,6 +100,7 @@ GicV2DisableInterruptSource (
   @retval EFI_UNSUPPORTED   Source interrupt is not supported
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 GicV2GetInterruptSourceState (
@@ -127,6 +130,7 @@ GicV2GetInterruptSourceState (
   @retval EFI_UNSUPPORTED   Source interrupt is not supported
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 GicV2EndOfInterrupt (
@@ -147,13 +151,15 @@ GicV2EndOfInterrupt (
   EFI_CPU_INTERRUPT_HANDLER that is called when a processor interrupt occurs.
 
   @param  InterruptType    Defines the type of interrupt or exception that
-                           occurred on the processor.This parameter is processor architecture specific.
+                           occurred on the processor.This parameter is
+                           processor architecture specific.
   @param  SystemContext    A pointer to the processor context when
                            the interrupt occurred on the processor.
 
   @return None
 
 **/
+STATIC
 VOID
 EFIAPI
 GicV2IrqInterruptHandler (
@@ -166,7 +172,8 @@ GicV2IrqInterruptHandler (
 
   GicInterrupt = ArmGicV2AcknowledgeInterrupt (mGicInterruptInterfaceBase);
 
-  // Special Interrupts (ID1020-ID1023) have an Interrupt ID greater than the number of interrupt (ie: Spurious interrupt).
+  // Special Interrupts (ID1020-ID1023) have an Interrupt ID greater than the
+  // number of interrupt (ie: Spurious interrupt).
   if ((GicInterrupt & ARM_GIC_ICCIAR_ACKINTID) >= mGicNumInterrupts) {
     // The special interrupt do not need to be acknowledge
     return;
@@ -182,9 +189,9 @@ GicV2IrqInterruptHandler (
   }
 }
 
-//
+
 // The protocol instance produced by this driver
-//
+
 EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
   RegisterInterruptSource,
   GicV2EnableInterruptSource,
@@ -196,12 +203,13 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
 /**
   Shutdown our hardware
 
-  DXE Core will disable interrupts and turn off the timer and disable interrupts
-  after all the event handlers have run.
+  DXE Core will disable interrupts and turn off the timer and disable
+  interrupts after all the event handlers have run.
 
   @param[in]  Event   The Event that is being processed
   @param[in]  Context Event Context
 **/
+STATIC
 VOID
 EFIAPI
 GicV2ExitBootServicesEvent (
@@ -256,7 +264,8 @@ GicV2DxeInitialize (
   UINTN                   RegShift;
   UINT32                  CpuTarget;
 
-  // Make sure the Interrupt Controller Protocol is not already installed in the system.
+  // Make sure the Interrupt Controller Protocol is not already installed in
+  // the system.
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
 
   mGicInterruptInterfaceBase = PcdGet64 (PcdGicInterruptInterfaceBase);
@@ -276,25 +285,28 @@ GicV2DxeInitialize (
       );
   }
 
-  //
+
   // Targets the interrupts to the Primary Cpu
-  //
 
-  // Only Primary CPU will run this code. We can identify our GIC CPU ID by reading
-  // the GIC Distributor Target register. The 8 first GICD_ITARGETSRn are banked to each
-  // connected CPU. These 8 registers hold the CPU targets fields for interrupts 0-31.
-  // More Info in the GIC Specification about "Interrupt Processor Targets Registers"
-  //
-  // Read the first Interrupt Processor Targets Register (that corresponds to the 4
-  // first SGIs)
+  // Only Primary CPU will run this code. We can identify our GIC CPU ID by
+  // reading the GIC Distributor Target register. The 8 first GICD_ITARGETSRn
+  // are banked to each connected CPU. These 8 registers hold the CPU targets
+  // fields for interrupts 0-31. More Info in the GIC Specification about
+  // "Interrupt Processor Targets Registers"
+
+  // Read the first Interrupt Processor Targets Register (that corresponds to
+  // the 4 first SGIs)
   CpuTarget = MmioRead32 (mGicDistributorBase + ARM_GIC_ICDIPTR);
 
-  // The CPU target is a bit field mapping each CPU to a GIC CPU Interface. This value
-  // is 0 when we run on a uniprocessor platform.
+  // The CPU target is a bit field mapping each CPU to a GIC CPU Interface.
+  // This value is 0 when we run on a uniprocessor platform.
   if (CpuTarget != 0) {
     // The 8 first Interrupt Processor Targets Registers are read-only
     for (Index = 8; Index < (mGicNumInterrupts / 4); Index++) {
-      MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDIPTR + (Index * 4), CpuTarget);
+      MmioWrite32 (
+        mGicDistributorBase + ARM_GIC_ICDIPTR + (Index * 4),
+        CpuTarget
+        );
     }
   }
 
@@ -311,7 +323,10 @@ GicV2DxeInitialize (
   ArmGicEnableDistributor (mGicDistributorBase);
 
   Status = InstallAndRegisterInterruptService (
-          &gHardwareInterruptV2Protocol, GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
+             &gHardwareInterruptV2Protocol,
+             GicV2IrqInterruptHandler,
+             GicV2ExitBootServicesEvent
+             );
 
   return Status;
 }
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 8af97a93b1889b33978a7c7fb2a8417c83139142..69b2d8d794e151e25f06cbea079e2796d9793a43 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -33,6 +33,7 @@ STATIC UINTN mGicRedistributorsBase;
   @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 GicV3EnableInterruptSource (
@@ -60,6 +61,7 @@ GicV3EnableInterruptSource (
   @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 GicV3DisableInterruptSource (
@@ -88,6 +90,7 @@ GicV3DisableInterruptSource (
   @retval EFI_DEVICE_ERROR  InterruptState is not valid
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 GicV3GetInterruptSourceState (
@@ -101,7 +104,10 @@ GicV3GetInterruptSourceState (
     return EFI_UNSUPPORTED;
   }
 
-  *InterruptState = ArmGicIsInterruptEnabled (mGicDistributorBase, mGicRedistributorsBase, Source);
+  *InterruptState = ArmGicIsInterruptEnabled (
+                      mGicDistributorBase,
+                      mGicRedistributorsBase,
+                      Source);
 
   return EFI_SUCCESS;
 }
@@ -117,6 +123,7 @@ GicV3GetInterruptSourceState (
   @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
 
 **/
+STATIC
 EFI_STATUS
 EFIAPI
 GicV3EndOfInterrupt (
@@ -137,13 +144,15 @@ GicV3EndOfInterrupt (
   EFI_CPU_INTERRUPT_HANDLER that is called when a processor interrupt occurs.
 
   @param  InterruptType    Defines the type of interrupt or exception that
-                           occurred on the processor.This parameter is processor architecture specific.
+                           occurred on the processor. This parameter is
+                           processor architecture specific.
   @param  SystemContext    A pointer to the processor context when
                            the interrupt occurred on the processor.
 
   @return None
 
 **/
+STATIC
 VOID
 EFIAPI
 GicV3IrqInterruptHandler (
@@ -173,9 +182,9 @@ GicV3IrqInterruptHandler (
   }
 }
 
-//
+
 // The protocol instance produced by this driver
-//
+
 EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = {
   RegisterInterruptSource,
   GicV3EnableInterruptSource,
@@ -242,17 +251,16 @@ GicV3DxeInitialize (
   UINT64                  CpuTarget;
   UINT64                  MpId;
 
-  // Make sure the Interrupt Controller Protocol is not already installed in the system.
+  // Make sure the Interrupt Controller Protocol is not already installed in
+  // the system.
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
 
   mGicDistributorBase    = PcdGet64 (PcdGicDistributorBase);
   mGicRedistributorsBase = PcdGet64 (PcdGicRedistributorsBase);
   mGicNumInterrupts      = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
 
-  //
   // We will be driving this GIC in native v3 mode, i.e., with Affinity
   // Routing enabled. So ensure that the ARE bit is set.
-  //
   if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
     MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
   }
@@ -270,51 +278,65 @@ GicV3DxeInitialize (
       );
   }
 
-  //
   // Targets the interrupts to the Primary Cpu
-  //
 
   if (FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
-    // Only Primary CPU will run this code. We can identify our GIC CPU ID by reading
-    // the GIC Distributor Target register. The 8 first GICD_ITARGETSRn are banked to each
-    // connected CPU. These 8 registers hold the CPU targets fields for interrupts 0-31.
-    // More Info in the GIC Specification about "Interrupt Processor Targets Registers"
-    //
-    // Read the first Interrupt Processor Targets Register (that corresponds to the 4
-    // first SGIs)
+    // Only Primary CPU will run this code. We can identify our GIC CPU ID by
+    // reading the GIC Distributor Target register. The 8 first
+    // GICD_ITARGETSRn are banked to each connected CPU. These 8 registers
+    // hold the CPU targets fields for interrupts 0-31. More Info in the GIC
+    // Specification about "Interrupt Processor Targets Registers"
+
+    // Read the first Interrupt Processor Targets Register (that corresponds
+    // to the 4 first SGIs)
     CpuTarget = MmioRead32 (mGicDistributorBase + ARM_GIC_ICDIPTR);
 
-    // The CPU target is a bit field mapping each CPU to a GIC CPU Interface. This value
-    // is 0 when we run on a uniprocessor platform.
+    // The CPU target is a bit field mapping each CPU to a GIC CPU Interface.
+    // This value is 0 when we run on a uniprocessor platform.
     if (CpuTarget != 0) {
       // The 8 first Interrupt Processor Targets Registers are read-only
       for (Index = 8; Index < (mGicNumInterrupts / 4); Index++) {
-        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDIPTR + (Index * 4), CpuTarget);
+        MmioWrite32 (
+          mGicDistributorBase + ARM_GIC_ICDIPTR + (Index * 4),
+          CpuTarget
+          );
       }
     }
   } else {
     MpId = ArmReadMpidr ();
-    CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
+    CpuTarget = MpId &
+                (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
+
+    if ((MmioRead32 (
+           mGicDistributorBase + ARM_GIC_ICDDCR
+         ) & ARM_GIC_ICDDCR_DS) != 0) {
 
-    if ((MmioRead32 (mGicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_DS) != 0) {
-      //
       // If the Disable Security (DS) control bit is set, we are dealing with a
       // GIC that has only one security state. In this case, let's assume we are
       // executing in non-secure state (which is appropriate for DXE modules)
       // and that no other firmware has performed any configuration on the GIC.
       // This means we need to reconfigure all interrupts to non-secure Group 1
       // first.
-      //
-      MmioWrite32 (mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR, 0xffffffff);
+
+      MmioWrite32 (
+        mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR,
+        0xffffffff
+        );
 
       for (Index = 32; Index < mGicNumInterrupts; Index += 32) {
-        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 0xffffffff);
+        MmioWrite32 (
+          mGicDistributorBase + ARM_GIC_ICDISR + Index / 8,
+          0xffffffff
+          );
       }
     }
 
     // Route the SPIs to the primary CPU. SPIs start at the INTID 32
     for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {
-      MmioWrite32 (mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8), CpuTarget | ARM_GICD_IROUTER_IRM);
+      MmioWrite32 (
+        mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8),
+        CpuTarget | ARM_GICD_IROUTER_IRM
+        );
     }
   }
 
@@ -331,7 +353,10 @@ GicV3DxeInitialize (
   ArmGicEnableDistributor (mGicDistributorBase);
 
   Status = InstallAndRegisterInterruptService (
-          &gHardwareInterruptV3Protocol, GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
+             &gHardwareInterruptV3Protocol,
+             GicV3IrqInterruptHandler,
+             GicV3ExitBootServicesEvent
+             );
 
   return Status;
 }
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 54a1625a32137556b58fa93ddf7fbe4d0f22c786..6d102e25047253048ac555d6fb5de7223d78f381 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -193,18 +193,18 @@ WatchdogSetTimerPeriod (
   // Work out how many timer ticks will equate to TimerPeriod
   mNumTimerTicks = (mTimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND;
 
-  //
+
   // If the number of required ticks is greater than the max number the
   // watchdog's offset register (WOR) can hold, we need to manually compute and
   // set the compare register (WCV)
-  //
+
   if (mNumTimerTicks > MAX_UINT32) {
-    //
+
     // 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.
-    //
+
     Status = WatchdogWriteOffsetRegister (MAX_UINT32);
     if (EFI_ERROR (Status)) {
       return Status;
@@ -302,11 +302,11 @@ GenericWatchdogEntry (
   EFI_STATUS                      Status;
   EFI_HANDLE                      Handle;
 
-  //
+
   // Make sure the Watchdog Timer Architectural Protocol has not been installed
   // in the system yet.
   // This will avoid conflicts with the universal watchdog
-  //
+
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiWatchdogTimerArchProtocolGuid);
 
   mTimerFrequencyHz = ArmGenericTimerGetTimerFreq ();
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.

[Leif Lindholm]

On Mon, Sep 11, 2017 at 04:23:31PM +0100, evan.lloyd@arm.com wrote:
> From: Evan Lloyd <evan.lloyd@arm.com>
> 
> This change is purely cosmetic, to tidy some code before change.
> Mods involve:
>     Reflow overlength comments.
>     Split overlength code lines.
>     Make protocol functions STATIC.
>     Remove "Horor vacui" comments.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>

Not entirely vital, but custom is to order signed-off-by
chronologically, i.e., person submitting the patch would normally be
last. Pointing this out mainly because it is not consistent through
the set.

> ---
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.h                      | 12 +--
>  ArmPkg/Include/Library/ArmGicLib.h                     | 29 ++++---
>  ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c                | 36 +++++----
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c                      | 72 +++++++++++++----
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c              | 57 +++++++++-----
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c              | 81 +++++++++++++-------
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 12 +--
>  7 files changed, 195 insertions(+), 104 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> index af33aa90b00c6775e10a831d63ed707394862362..1018f2004e75d879a72c2d6bf37b64051e720d12 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> @@ -28,9 +28,9 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  extern UINTN                        mGicNumInterrupts;
>  extern HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers;
>  
> -//
> +
>  // Common API
> -//
> +

I am a little bit perplexed by the replacement of //-lines with blank
lines. Especially as it is neither consistent throughout the patch nor
aligned with existing single-line comments in the file.
Why not just delete them?

>  EFI_STATUS
>  InstallAndRegisterInterruptService (
>    IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
> @@ -46,18 +46,18 @@ RegisterInterruptSource (
>    IN HARDWARE_INTERRUPT_HANDLER         Handler
>    );
>  
> -//
> +
>  // GicV2 API
> -//
> +
>  EFI_STATUS
>  GicV2DxeInitialize (
>    IN EFI_HANDLE         ImageHandle,
>    IN EFI_SYSTEM_TABLE   *SystemTable
>    );
>  
> -//
> +
>  // GicV3 API
> -//
> +
>  EFI_STATUS
>  GicV3DxeInitialize (
>    IN EFI_HANDLE         ImageHandle,
> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
> index 4364f3ffef464596f64cf59881d703cf54cf0ddd..f7b546895d116f81c65a853fcdb067ec7601b2da 100644
> --- a/ArmPkg/Include/Library/ArmGicLib.h
> +++ b/ArmPkg/Include/Library/ArmGicLib.h
> @@ -17,9 +17,9 @@
>  
>  #include <Library/ArmGicArchLib.h>
>  
> -//
> +
>  // GIC Distributor
> -//
> +
>  #define ARM_GIC_ICDDCR          0x000 // Distributor Control Register
>  #define ARM_GIC_ICDICTR         0x004 // Interrupt Controller Type Register
>  #define ARM_GIC_ICDIIDR         0x008 // Implementer Identification Register
> @@ -51,9 +51,9 @@
>  #define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)
>  #define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)
>  
> -//
> +
>  // GIC Redistributor
> -//
> +
>  
>  #define ARM_GICR_CTLR_FRAME_SIZE    SIZE_64KB
>  #define ARM_GICR_SGI_PPI_FRAME_SIZE SIZE_64KB
> @@ -65,9 +65,9 @@
>  #define ARM_GICR_ISENABLER      0x0100  // Interrupt Set-Enable Registers
>  #define ARM_GICR_ICENABLER      0x0180  // Interrupt Clear-Enable Registers
>  
> -//
> +
>  // GIC Cpu interface
> -//
> +
>  #define ARM_GIC_ICCICR          0x00  // CPU Interface Control Register
>  #define ARM_GIC_ICCPMR          0x04  // Interrupt Priority Mask Register
>  #define ARM_GIC_ICCBPR          0x08  // Binary Point Register
> @@ -104,9 +104,9 @@ ArmGicGetInterfaceIdentification (
>    IN  INTN          GicInterruptInterfaceBase
>    );
>  
> -//
> +
>  // GIC Secure interfaces
> -//
> +
>  VOID
>  EFIAPI
>  ArmGicSetupNonSecure (
> @@ -170,7 +170,8 @@ ArmGicSendSgiTo (
>   * in the GICv3 the register value is only the InterruptId.
>   *
>   * @param GicInterruptInterfaceBase   Base Address of the GIC CPU Interface
> - * @param InterruptId                 InterruptId read from the Interrupt Acknowledge Register
> + * @param InterruptId                 InterruptId read from the Interrupt
> + *                                    Acknowledge Register
>   *
>   * @retval value returned by the Interrupt Acknowledge Register
>   *
> @@ -220,12 +221,12 @@ ArmGicIsInterruptEnabled (
>    IN UINTN                  Source
>    );
>  
> -//
>  // GIC revision 2 specific declarations
> -//
>  
> -// Interrupts from 1020 to 1023 are considered as special interrupts (eg: spurious interrupts)
> -#define ARM_GIC_IS_SPECIAL_INTERRUPTS(Interrupt) (((Interrupt) >= 1020) && ((Interrupt) <= 1023))
> +// Interrupts from 1020 to 1023 are considered as special interrupts
> +// (eg: spurious interrupts)
> +#define ARM_GIC_IS_SPECIAL_INTERRUPTS(Interrupt) \
> +          (((Interrupt) >= 1020) && ((Interrupt) <= 1023))
>  
>  VOID
>  EFIAPI
> @@ -260,9 +261,7 @@ ArmGicV2EndOfInterrupt (
>    IN UINTN                  Source
>    );
>  
> -//
>  // GIC revision 3 specific declarations
> -//
>  
>  #define ICC_SRE_EL2_SRE         (1 << 0)
>  
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> index be77b8361c5af033fd2889cdb48902af867f321d..88cb455b75bb8e8cb22157643a392403ce93129d 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> @@ -28,14 +28,14 @@ ExitBootServicesEvent (
>    IN VOID       *Context
>    );
>  
> -//
> +
>  // Making this global saves a few bytes in image size
> -//
> +
>  EFI_HANDLE  gHardwareInterruptHandle = NULL;
>  
> -//
> +
>  // Notifications
> -//
> +
>  EFI_EVENT EfiExitBootServicesEvent      = (EFI_EVENT)NULL;
>  
>  // Maximum Number of Interrupts
> @@ -96,7 +96,8 @@ InstallAndRegisterInterruptService (
>    EFI_CPU_ARCH_PROTOCOL   *Cpu;
>  
>    // Initialize the array for the Interrupt Handlers
> -  gRegisteredInterruptHandlers = (HARDWARE_INTERRUPT_HANDLER*)AllocateZeroPool (sizeof(HARDWARE_INTERRUPT_HANDLER) * mGicNumInterrupts);
> +  gRegisteredInterruptHandlers = (HARDWARE_INTERRUPT_HANDLER*)
> +    AllocateZeroPool (sizeof(HARDWARE_INTERRUPT_HANDLER) * mGicNumInterrupts);

I agree this makes the code more conformant with coding style.
But if taking the time to rewrite it, I would much rather do something
like:

  UINTN Size;

  Size = sizeof (HARDWARE_INTERRUPT_HANDLER) * mGicNumInterrupts;
  gRegisteredInterruptHandlers = AllocateZeroPool (Size);

Explicit casts of VOID * to the target pointer type just makes things
messier, especially when the target pointer type is 26 characters plus
4 characters of ( *).

>    if (gRegisteredInterruptHandlers == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> @@ -110,32 +111,41 @@ InstallAndRegisterInterruptService (
>      return Status;
>    }
>  
> -  //
> +
>    // Get the CPU protocol that this driver requires.
> -  //
> +
>    Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&Cpu);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
> -  //
> +
>    // Unregister the default exception handler.
> -  //
> +
>    Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ, NULL);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
> -  //
> +
>    // Register to receive interrupts
> -  //
> -  Status = Cpu->RegisterInterruptHandler (Cpu, ARM_ARCH_EXCEPTION_IRQ, InterruptHandler);
> +  Status = Cpu->RegisterInterruptHandler (
> +             Cpu,
> +             ARM_ARCH_EXCEPTION_IRQ,
> +             InterruptHandler
> +             );
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
>    // Register for an ExitBootServicesEvent
> -  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY, ExitBootServicesEvent, NULL, &EfiExitBootServicesEvent);
> +  Status = gBS->CreateEvent (
> +             EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +             TPL_NOTIFY,
> +             ExitBootServicesEvent,
> +             NULL,
> +             &EfiExitBootServicesEvent
> +             );
>  
>    return Status;
>  }
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index e658e9bff5d8107b3914bdf1e9e1e51a4e4d4cd7..852330b80db9726f860f6868f7e90d48756b6e58 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -55,13 +55,17 @@ GicGetCpuRedistributorBase (
>    UINTN GicCpuRedistributorBase;
>  
>    MpId = ArmReadMpidr ();
> -  // Define CPU affinity as Affinity0[0:8], Affinity1[9:15], Affinity2[16:23], Affinity3[24:32]
> +  // Define CPU affinity as Affinity0[0:8], Affinity1[9:15],
> +  //   Affinity2[16:23], Affinity3[24:32]

More conformant, but how about the alternative:

  // Define CPU affinity as
  //   Affinity0[0:8], Affinity1[9:15], Affinity2[16:23], Affinity3[24:32]

?

>    // whereas Affinity3 is defined at [32:39] in MPIDR
> -  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2)) | ((MpId & ARM_CORE_AFF3) >> 8);
> +  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2))
> +                | ((MpId & ARM_CORE_AFF3) >> 8);

I can't find an explicit rule on this, but my preference aligns with
what examples I can see in the Coding Style: moving that lone '|' to
the end of the preceding line:

  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2)) |
                ((MpId & ARM_CORE_AFF3) >> 8);

>    if (Revision == ARM_GIC_ARCH_REVISION_3) {
> -    // 2 x 64KB frame: Redistributor control frame + SGI Control & Generation frame
> -    GicRedistributorGranularity = ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_SGI_PPI_FRAME_SIZE;
> +    // 2 x 64KB frame:
> +    //   Redistributor control frame + SGI Control & Generation frame
> +    GicRedistributorGranularity = ARM_GICR_CTLR_FRAME_SIZE
> +                                  + ARM_GICR_SGI_PPI_FRAME_SIZE;
>    } else {
>      ASSERT_EFI_ERROR (EFI_UNSUPPORTED);
>      return 0;
> @@ -112,7 +116,10 @@ ArmGicSendSgiTo (
>    IN  INTN          SgiId
>    )
>  {
> -  MmioWrite32 (GicDistributorBase + ARM_GIC_ICDSGIR, ((TargetListFilter & 0x3) << 24) | ((CPUTargetList & 0xFF) << 16) | SgiId);
> +  MmioWrite32 (
> +    GicDistributorBase + ARM_GIC_ICDSGIR,
> +    ((TargetListFilter & 0x3) << 24) | ((CPUTargetList & 0xFF) << 16) | SgiId
> +    );
>  }
>  
>  /*
> @@ -123,7 +130,8 @@ ArmGicSendSgiTo (
>   * in the GICv3 the register value is only the InterruptId.
>   *
>   * @param GicInterruptInterfaceBase   Base Address of the GIC CPU Interface
> - * @param InterruptId                 InterruptId read from the Interrupt Acknowledge Register
> + * @param InterruptId                 InterruptId read from the Interrupt
> + *                                    Acknowledge Register
>   *
>   * @retval value returned by the Interrupt Acknowledge Register
>   *
> @@ -200,16 +208,28 @@ ArmGicEnableInterrupt (
>        FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
>        SourceIsSpi (Source)) {
>      // Write set-enable register
> -    MmioWrite32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset), 1 << RegShift);
> +    MmioWrite32 (
> +      GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset),
> +      1 << RegShift
> +      );
>    } else {
> -    GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);
> +    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
> +                                GicRedistributorBase,
> +                                Revision
> +                                );
>      if (GicCpuRedistributorBase == 0) {
>        ASSERT_EFI_ERROR (EFI_NOT_FOUND);
>        return;
>      }
>  
>      // Write set-enable register
> -    MmioWrite32 (GicCpuRedistributorBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISENABLER + (4 * RegOffset), 1 << RegShift);
> +    MmioWrite32 (
> +      (GicCpuRedistributorBase
> +        + ARM_GICR_CTLR_FRAME_SIZE
> +        + ARM_GICR_ISENABLER
> +        + (4 * RegOffset)),
> +      1 << RegShift
> +      );

Maybe rewrite as

#define SET_ENABLE_ADDRESS(base,offset) ((base) + ARM_GICR_CTLR_FRAME_SIZE + \
                                         ARM_GICR_ISENABLER + (4 * offset))

(further up)

then

    MmioWrite32 (
      SET_ENABLE_ADDRESS (GicCpuRedistributorBase, RegOffset),
      1 << RegShift
      );

?

>    }
>  }
>  
> @@ -235,15 +255,27 @@ ArmGicDisableInterrupt (
>        FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
>        SourceIsSpi (Source)) {
>      // Write clear-enable register
> -    MmioWrite32 (GicDistributorBase + ARM_GIC_ICDICER + (4 * RegOffset), 1 << RegShift);
> +    MmioWrite32 (
> +      GicDistributorBase + ARM_GIC_ICDICER + (4 * RegOffset),
> +      1 << RegShift
> +      );
>    } else {
> -    GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);
> +    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
> +      GicRedistributorBase,
> +      Revision
> +      );
>      if (GicCpuRedistributorBase == 0) {
>        return;
>      }
>  
>      // Write clear-enable register
> -    MmioWrite32 (GicCpuRedistributorBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ICENABLER + (4 * RegOffset), 1 << RegShift);
> +    MmioWrite32 (
> +      (GicCpuRedistributorBase
> +        + ARM_GICR_CTLR_FRAME_SIZE
> +        + ARM_GICR_ICENABLER
> +        + (4 * RegOffset)),
> +      1 << RegShift
> +      );


Like above, but with a CLEAR_ENABLE_ADDRESS macro?

>    }
>  }
>  
> @@ -269,15 +301,25 @@ ArmGicIsInterruptEnabled (
>    if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
>        FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
>        SourceIsSpi (Source)) {
> -    Interrupts = ((MmioRead32 (GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)) & (1 << RegShift)) != 0);
> +    Interrupts = ((MmioRead32 (
> +                     GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)
> +                     )
> +                  & (1 << RegShift)) != 0);
>    } else {
> -    GicCpuRedistributorBase = GicGetCpuRedistributorBase (GicRedistributorBase, Revision);
> +    GicCpuRedistributorBase = GicGetCpuRedistributorBase (
> +                                GicRedistributorBase,
> +                                Revision
> +                                );
>      if (GicCpuRedistributorBase == 0) {
>        return 0;
>      }
>  
>      // Read set-enable register
> -    Interrupts = MmioRead32 (GicCpuRedistributorBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISENABLER + (4 * RegOffset));
> +    Interrupts = MmioRead32 (
> +                   GicCpuRedistributorBase
> +                   + ARM_GICR_CTLR_FRAME_SIZE
> +                   + ARM_GICR_ISENABLER
> +                   + (4 * RegOffset));

SET_ENABLE_ADDRESS could be reused here.

No further comments below.

And this patch _is_ a big improvement. Both in keeping non-functional
changes separate from functional, and in correcting the line lengths.

/
    Leif

>    }
>  
>    return ((Interrupts & (1 << RegShift)) != 0);
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> index b9ecd5543a3e2e0b00fffbcf5543a60567bb5dde..50ec90207b515d849cbf64f0a4b0d639b3868e60 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> @@ -2,7 +2,7 @@
>  
>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
>  Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
> -Portions copyright (c) 2011-2016, ARM Ltd. All rights reserved.<BR>
> +Portions copyright (c) 2011-2017, ARM Ltd. All rights reserved.<BR>
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
> @@ -43,6 +43,7 @@ STATIC UINT32 mGicDistributorBase;
>    @retval EFI_UNSUPPORTED   Source interrupt is not supported
>  
>  **/
> +STATIC
>  EFI_STATUS
>  EFIAPI
>  GicV2EnableInterruptSource (
> @@ -70,6 +71,7 @@ GicV2EnableInterruptSource (
>    @retval EFI_UNSUPPORTED   Source interrupt is not supported
>  
>  **/
> +STATIC
>  EFI_STATUS
>  EFIAPI
>  GicV2DisableInterruptSource (
> @@ -98,6 +100,7 @@ GicV2DisableInterruptSource (
>    @retval EFI_UNSUPPORTED   Source interrupt is not supported
>  
>  **/
> +STATIC
>  EFI_STATUS
>  EFIAPI
>  GicV2GetInterruptSourceState (
> @@ -127,6 +130,7 @@ GicV2GetInterruptSourceState (
>    @retval EFI_UNSUPPORTED   Source interrupt is not supported
>  
>  **/
> +STATIC
>  EFI_STATUS
>  EFIAPI
>  GicV2EndOfInterrupt (
> @@ -147,13 +151,15 @@ GicV2EndOfInterrupt (
>    EFI_CPU_INTERRUPT_HANDLER that is called when a processor interrupt occurs.
>  
>    @param  InterruptType    Defines the type of interrupt or exception that
> -                           occurred on the processor.This parameter is processor architecture specific.
> +                           occurred on the processor.This parameter is
> +                           processor architecture specific.
>    @param  SystemContext    A pointer to the processor context when
>                             the interrupt occurred on the processor.
>  
>    @return None
>  
>  **/
> +STATIC
>  VOID
>  EFIAPI
>  GicV2IrqInterruptHandler (
> @@ -166,7 +172,8 @@ GicV2IrqInterruptHandler (
>  
>    GicInterrupt = ArmGicV2AcknowledgeInterrupt (mGicInterruptInterfaceBase);
>  
> -  // Special Interrupts (ID1020-ID1023) have an Interrupt ID greater than the number of interrupt (ie: Spurious interrupt).
> +  // Special Interrupts (ID1020-ID1023) have an Interrupt ID greater than the
> +  // number of interrupt (ie: Spurious interrupt).
>    if ((GicInterrupt & ARM_GIC_ICCIAR_ACKINTID) >= mGicNumInterrupts) {
>      // The special interrupt do not need to be acknowledge
>      return;
> @@ -182,9 +189,9 @@ GicV2IrqInterruptHandler (
>    }
>  }
>  
> -//
> +
>  // The protocol instance produced by this driver
> -//
> +
>  EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
>    RegisterInterruptSource,
>    GicV2EnableInterruptSource,
> @@ -196,12 +203,13 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
>  /**
>    Shutdown our hardware
>  
> -  DXE Core will disable interrupts and turn off the timer and disable interrupts
> -  after all the event handlers have run.
> +  DXE Core will disable interrupts and turn off the timer and disable
> +  interrupts after all the event handlers have run.
>  
>    @param[in]  Event   The Event that is being processed
>    @param[in]  Context Event Context
>  **/
> +STATIC
>  VOID
>  EFIAPI
>  GicV2ExitBootServicesEvent (
> @@ -256,7 +264,8 @@ GicV2DxeInitialize (
>    UINTN                   RegShift;
>    UINT32                  CpuTarget;
>  
> -  // Make sure the Interrupt Controller Protocol is not already installed in the system.
> +  // Make sure the Interrupt Controller Protocol is not already installed in
> +  // the system.
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
>  
>    mGicInterruptInterfaceBase = PcdGet64 (PcdGicInterruptInterfaceBase);
> @@ -276,25 +285,28 @@ GicV2DxeInitialize (
>        );
>    }
>  
> -  //
> +
>    // Targets the interrupts to the Primary Cpu
> -  //
>  
> -  // Only Primary CPU will run this code. We can identify our GIC CPU ID by reading
> -  // the GIC Distributor Target register. The 8 first GICD_ITARGETSRn are banked to each
> -  // connected CPU. These 8 registers hold the CPU targets fields for interrupts 0-31.
> -  // More Info in the GIC Specification about "Interrupt Processor Targets Registers"
> -  //
> -  // Read the first Interrupt Processor Targets Register (that corresponds to the 4
> -  // first SGIs)
> +  // Only Primary CPU will run this code. We can identify our GIC CPU ID by
> +  // reading the GIC Distributor Target register. The 8 first GICD_ITARGETSRn
> +  // are banked to each connected CPU. These 8 registers hold the CPU targets
> +  // fields for interrupts 0-31. More Info in the GIC Specification about
> +  // "Interrupt Processor Targets Registers"
> +
> +  // Read the first Interrupt Processor Targets Register (that corresponds to
> +  // the 4 first SGIs)
>    CpuTarget = MmioRead32 (mGicDistributorBase + ARM_GIC_ICDIPTR);
>  
> -  // The CPU target is a bit field mapping each CPU to a GIC CPU Interface. This value
> -  // is 0 when we run on a uniprocessor platform.
> +  // The CPU target is a bit field mapping each CPU to a GIC CPU Interface.
> +  // This value is 0 when we run on a uniprocessor platform.
>    if (CpuTarget != 0) {
>      // The 8 first Interrupt Processor Targets Registers are read-only
>      for (Index = 8; Index < (mGicNumInterrupts / 4); Index++) {
> -      MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDIPTR + (Index * 4), CpuTarget);
> +      MmioWrite32 (
> +        mGicDistributorBase + ARM_GIC_ICDIPTR + (Index * 4),
> +        CpuTarget
> +        );
>      }
>    }
>  
> @@ -311,7 +323,10 @@ GicV2DxeInitialize (
>    ArmGicEnableDistributor (mGicDistributorBase);
>  
>    Status = InstallAndRegisterInterruptService (
> -          &gHardwareInterruptV2Protocol, GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent);
> +             &gHardwareInterruptV2Protocol,
> +             GicV2IrqInterruptHandler,
> +             GicV2ExitBootServicesEvent
> +             );
>  
>    return Status;
>  }
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index 8af97a93b1889b33978a7c7fb2a8417c83139142..69b2d8d794e151e25f06cbea079e2796d9793a43 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD License
> @@ -33,6 +33,7 @@ STATIC UINTN mGicRedistributorsBase;
>    @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
>  
>  **/
> +STATIC
>  EFI_STATUS
>  EFIAPI
>  GicV3EnableInterruptSource (
> @@ -60,6 +61,7 @@ GicV3EnableInterruptSource (
>    @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
>  
>  **/
> +STATIC
>  EFI_STATUS
>  EFIAPI
>  GicV3DisableInterruptSource (
> @@ -88,6 +90,7 @@ GicV3DisableInterruptSource (
>    @retval EFI_DEVICE_ERROR  InterruptState is not valid
>  
>  **/
> +STATIC
>  EFI_STATUS
>  EFIAPI
>  GicV3GetInterruptSourceState (
> @@ -101,7 +104,10 @@ GicV3GetInterruptSourceState (
>      return EFI_UNSUPPORTED;
>    }
>  
> -  *InterruptState = ArmGicIsInterruptEnabled (mGicDistributorBase, mGicRedistributorsBase, Source);
> +  *InterruptState = ArmGicIsInterruptEnabled (
> +                      mGicDistributorBase,
> +                      mGicRedistributorsBase,
> +                      Source);
>  
>    return EFI_SUCCESS;
>  }
> @@ -117,6 +123,7 @@ GicV3GetInterruptSourceState (
>    @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
>  
>  **/
> +STATIC
>  EFI_STATUS
>  EFIAPI
>  GicV3EndOfInterrupt (
> @@ -137,13 +144,15 @@ GicV3EndOfInterrupt (
>    EFI_CPU_INTERRUPT_HANDLER that is called when a processor interrupt occurs.
>  
>    @param  InterruptType    Defines the type of interrupt or exception that
> -                           occurred on the processor.This parameter is processor architecture specific.
> +                           occurred on the processor. This parameter is
> +                           processor architecture specific.
>    @param  SystemContext    A pointer to the processor context when
>                             the interrupt occurred on the processor.
>  
>    @return None
>  
>  **/
> +STATIC
>  VOID
>  EFIAPI
>  GicV3IrqInterruptHandler (
> @@ -173,9 +182,9 @@ GicV3IrqInterruptHandler (
>    }
>  }
>  
> -//
> +
>  // The protocol instance produced by this driver
> -//
> +
>  EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = {
>    RegisterInterruptSource,
>    GicV3EnableInterruptSource,
> @@ -242,17 +251,16 @@ GicV3DxeInitialize (
>    UINT64                  CpuTarget;
>    UINT64                  MpId;
>  
> -  // Make sure the Interrupt Controller Protocol is not already installed in the system.
> +  // Make sure the Interrupt Controller Protocol is not already installed in
> +  // the system.
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
>  
>    mGicDistributorBase    = PcdGet64 (PcdGicDistributorBase);
>    mGicRedistributorsBase = PcdGet64 (PcdGicRedistributorsBase);
>    mGicNumInterrupts      = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
>  
> -  //
>    // We will be driving this GIC in native v3 mode, i.e., with Affinity
>    // Routing enabled. So ensure that the ARE bit is set.
> -  //
>    if (!FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
>      MmioOr32 (mGicDistributorBase + ARM_GIC_ICDDCR, ARM_GIC_ICDDCR_ARE);
>    }
> @@ -270,51 +278,65 @@ GicV3DxeInitialize (
>        );
>    }
>  
> -  //
>    // Targets the interrupts to the Primary Cpu
> -  //
>  
>    if (FeaturePcdGet (PcdArmGicV3WithV2Legacy)) {
> -    // Only Primary CPU will run this code. We can identify our GIC CPU ID by reading
> -    // the GIC Distributor Target register. The 8 first GICD_ITARGETSRn are banked to each
> -    // connected CPU. These 8 registers hold the CPU targets fields for interrupts 0-31.
> -    // More Info in the GIC Specification about "Interrupt Processor Targets Registers"
> -    //
> -    // Read the first Interrupt Processor Targets Register (that corresponds to the 4
> -    // first SGIs)
> +    // Only Primary CPU will run this code. We can identify our GIC CPU ID by
> +    // reading the GIC Distributor Target register. The 8 first
> +    // GICD_ITARGETSRn are banked to each connected CPU. These 8 registers
> +    // hold the CPU targets fields for interrupts 0-31. More Info in the GIC
> +    // Specification about "Interrupt Processor Targets Registers"
> +
> +    // Read the first Interrupt Processor Targets Register (that corresponds
> +    // to the 4 first SGIs)
>      CpuTarget = MmioRead32 (mGicDistributorBase + ARM_GIC_ICDIPTR);
>  
> -    // The CPU target is a bit field mapping each CPU to a GIC CPU Interface. This value
> -    // is 0 when we run on a uniprocessor platform.
> +    // The CPU target is a bit field mapping each CPU to a GIC CPU Interface.
> +    // This value is 0 when we run on a uniprocessor platform.
>      if (CpuTarget != 0) {
>        // The 8 first Interrupt Processor Targets Registers are read-only
>        for (Index = 8; Index < (mGicNumInterrupts / 4); Index++) {
> -        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDIPTR + (Index * 4), CpuTarget);
> +        MmioWrite32 (
> +          mGicDistributorBase + ARM_GIC_ICDIPTR + (Index * 4),
> +          CpuTarget
> +          );
>        }
>      }
>    } else {
>      MpId = ArmReadMpidr ();
> -    CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
> +    CpuTarget = MpId &
> +                (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
> +
> +    if ((MmioRead32 (
> +           mGicDistributorBase + ARM_GIC_ICDDCR
> +         ) & ARM_GIC_ICDDCR_DS) != 0) {
>  
> -    if ((MmioRead32 (mGicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_DS) != 0) {
> -      //
>        // If the Disable Security (DS) control bit is set, we are dealing with a
>        // GIC that has only one security state. In this case, let's assume we are
>        // executing in non-secure state (which is appropriate for DXE modules)
>        // and that no other firmware has performed any configuration on the GIC.
>        // This means we need to reconfigure all interrupts to non-secure Group 1
>        // first.
> -      //
> -      MmioWrite32 (mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR, 0xffffffff);
> +
> +      MmioWrite32 (
> +        mGicRedistributorsBase + ARM_GICR_CTLR_FRAME_SIZE + ARM_GIC_ICDISR,
> +        0xffffffff
> +        );
>  
>        for (Index = 32; Index < mGicNumInterrupts; Index += 32) {
> -        MmioWrite32 (mGicDistributorBase + ARM_GIC_ICDISR + Index / 8, 0xffffffff);
> +        MmioWrite32 (
> +          mGicDistributorBase + ARM_GIC_ICDISR + Index / 8,
> +          0xffffffff
> +          );
>        }
>      }
>  
>      // Route the SPIs to the primary CPU. SPIs start at the INTID 32
>      for (Index = 0; Index < (mGicNumInterrupts - 32); Index++) {
> -      MmioWrite32 (mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8), CpuTarget | ARM_GICD_IROUTER_IRM);
> +      MmioWrite32 (
> +        mGicDistributorBase + ARM_GICD_IROUTER + (Index * 8),
> +        CpuTarget | ARM_GICD_IROUTER_IRM
> +        );
>      }
>    }
>  
> @@ -331,7 +353,10 @@ GicV3DxeInitialize (
>    ArmGicEnableDistributor (mGicDistributorBase);
>  
>    Status = InstallAndRegisterInterruptService (
> -          &gHardwareInterruptV3Protocol, GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent);
> +             &gHardwareInterruptV3Protocol,
> +             GicV3IrqInterruptHandler,
> +             GicV3ExitBootServicesEvent
> +             );
>  
>    return Status;
>  }
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 54a1625a32137556b58fa93ddf7fbe4d0f22c786..6d102e25047253048ac555d6fb5de7223d78f381 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -193,18 +193,18 @@ WatchdogSetTimerPeriod (
>    // Work out how many timer ticks will equate to TimerPeriod
>    mNumTimerTicks = (mTimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND;
>  
> -  //
> +
>    // If the number of required ticks is greater than the max number the
>    // watchdog's offset register (WOR) can hold, we need to manually compute and
>    // set the compare register (WCV)
> -  //
> +
>    if (mNumTimerTicks > MAX_UINT32) {
> -    //
> +
>      // 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.
> -    //
> +
>      Status = WatchdogWriteOffsetRegister (MAX_UINT32);
>      if (EFI_ERROR (Status)) {
>        return Status;
> @@ -302,11 +302,11 @@ GenericWatchdogEntry (
>    EFI_STATUS                      Status;
>    EFI_HANDLE                      Handle;
>  
> -  //
> +
>    // Make sure the Watchdog Timer Architectural Protocol has not been installed
>    // in the system yet.
>    // This will avoid conflicts with the universal watchdog
> -  //
> +
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiWatchdogTimerArchProtocolGuid);
>  
>    mTimerFrequencyHz = ArmGenericTimerGetTimerFreq ();
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 

Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.

[Evan Lloyd]

Hi Leif.
I agree/accept all the comments, except:

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: 14 September 2017 17:41
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.
> 
> On Mon, Sep 11, 2017 at 04:23:31PM +0100, evan.lloyd@arm.com wrote:
> > From: Evan Lloyd <evan.lloyd@arm.com>
> >
...
> 
> >    // whereas Affinity3 is defined at [32:39] in MPIDR
> > -  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> > ARM_CORE_AFF2)) | ((MpId & ARM_CORE_AFF3) >> 8);
> > +  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> ARM_CORE_AFF2))
> > +                | ((MpId & ARM_CORE_AFF3) >> 8);
> 
> I can't find an explicit rule on this, but my preference aligns with what
> examples I can see in the Coding Style: moving that lone '|' to the end of the
> preceding line:
> 
>   CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> ARM_CORE_AFF2)) |
>                 ((MpId & ARM_CORE_AFF3) >> 8);

[[Evan Lloyd]] 5.2.1.6 and 5.7.2.4 have multiple examples of prefix style, 
  with 5.2.2.11 and 5.7.2.3 providing only 2 instances of a line suffix operator.
  I can change it if you insist, but it will be a clear instance of a 
  maintainer's personal prejudice overriding the coding standard. I strongly
  believe prefix format is much clearer, especially for compound conditions
  with nesting.

> 
> >    if (Revision == ARM_GIC_ARCH_REVISION_3) {
...
> >      // Write set-enable register
> > -    MmioWrite32 (GicCpuRedistributorBase +
> ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISENABLER + (4 * RegOffset), 1
> << RegShift);
> > +    MmioWrite32 (
> > +      (GicCpuRedistributorBase
> > +        + ARM_GICR_CTLR_FRAME_SIZE
> > +        + ARM_GICR_ISENABLER
> > +        + (4 * RegOffset)),
> > +      1 << RegShift
> > +      );
> 
> Maybe rewrite as
> 
> #define SET_ENABLE_ADDRESS(base,offset) ((base) +
> ARM_GICR_CTLR_FRAME_SIZE + \
>                                          ARM_GICR_ISENABLER + (4 * offset))
> 
> (further up)
> 
> then
> 
>     MmioWrite32 (
>       SET_ENABLE_ADDRESS (GicCpuRedistributorBase, RegOffset),
>       1 << RegShift
>       );
> 
> ?

[[Evan Lloyd]] Agree, but I called the macros ISENABLER_ADDRESS and ISENABLER_ADDRESS
(using the register names) because SET_ seemed to imply writing something in this context.

...

Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.

[Leif Lindholm]

On Thu, Sep 21, 2017 at 03:34:07PM +0000, Evan Lloyd wrote:
> Hi Leif.
> I agree/accept all the comments, except:
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: 14 September 2017 17:41
> > To: Evan Lloyd <Evan.Lloyd@arm.com>
> > Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> > Subject: Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.
> > 
> > On Mon, Sep 11, 2017 at 04:23:31PM +0100, evan.lloyd@arm.com wrote:
> > > From: Evan Lloyd <evan.lloyd@arm.com>
> > >
> ...
> > 
> > >    // whereas Affinity3 is defined at [32:39] in MPIDR
> > > -  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> > > ARM_CORE_AFF2)) | ((MpId & ARM_CORE_AFF3) >> 8);
> > > +  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> > ARM_CORE_AFF2))
> > > +                | ((MpId & ARM_CORE_AFF3) >> 8);
> > 
> > I can't find an explicit rule on this, but my preference aligns with what
> > examples I can see in the Coding Style: moving that lone '|' to the end of the
> > preceding line:
> > 
> >   CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> > ARM_CORE_AFF2)) |
> >                 ((MpId & ARM_CORE_AFF3) >> 8);
> 
> [[Evan Lloyd]] 5.2.1.6 and 5.7.2.4 have multiple examples of prefix style, 
>   with 5.2.2.11 and 5.7.2.3 providing only 2 instances of a line suffix operator.

The example of 5.2.1.6 already violates the rule of 5.2.2.11.
But the whole problem is that there is no explicit rule about
prefix/suffix, which leads to...

>   I can change it if you insist, but it will be a clear instance of a 
>   maintainer's personal prejudice overriding the coding standard.

If you can point me to a rule I have missed, then yes, you would be
absolutely correct. But I can only find uses, of both variants, in
examples explaining other rules.

If not, I am doing exactly what I am meant to be doing.

Since the maintainer is the one who a) has to review patches without
prior understanding of what the author was thinking and b) the one who
needs to review future modifications to that code, it would be highly
irresponsible to not ask for the code to be presented in the form most
clear to them, where this does not conflict with the coding style.

>   I strongly believe prefix format is much clearer, especially for
>   compound conditions with nesting.

Then why this prefix format not uniformly used in the patch?

Further down, there is
---
-    CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
+    CpuTarget = MpId &
+                (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
---

Now, a) I think that's very clear and b) any other way pushes the
length of the second line over 80 characters. But it does mean you are
not adhering to the rules you are arguing for.

> > 
> > >    if (Revision == ARM_GIC_ARCH_REVISION_3) {
> ...
> > >      // Write set-enable register
> > > -    MmioWrite32 (GicCpuRedistributorBase +
> > ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISENABLER + (4 * RegOffset), 1
> > << RegShift);
> > > +    MmioWrite32 (
> > > +      (GicCpuRedistributorBase
> > > +        + ARM_GICR_CTLR_FRAME_SIZE
> > > +        + ARM_GICR_ISENABLER
> > > +        + (4 * RegOffset)),
> > > +      1 << RegShift
> > > +      );
> > 
> > Maybe rewrite as
> > 
> > #define SET_ENABLE_ADDRESS(base,offset) ((base) +
> > ARM_GICR_CTLR_FRAME_SIZE + \
> >                                          ARM_GICR_ISENABLER + (4 * offset))
> > 
> > (further up)
> > 
> > then
> > 
> >     MmioWrite32 (
> >       SET_ENABLE_ADDRESS (GicCpuRedistributorBase, RegOffset),
> >       1 << RegShift
> >       );
> > 
> > ?
> 
> [[Evan Lloyd]] Agree, but I called the macros ISENABLER_ADDRESS and ISENABLER_ADDRESS
> (using the register names) because SET_ seemed to imply writing something in this context.

Yes, this is much better.

Another reason why I keep banging on about macros - they let you
add descriptive semantics that makes the code easier to understand for
someone unfamiliar with it.

Regards,

Leif

[PATCH 2/5] EmbeddedPkg: Introduce HardwareInterrupt2 protocol

[evan.lloyd]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

The existing HardwareInterrupt protocol lacks the means to configure
the level/edge and polarity properties of an interrupt. So introduce a
new protocol HardwareInterrupt2, and add some new members that allow
manipulation of those properties.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Girish Pathak <girish.pathak@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Tested-by: Girish Pathak <girish.pathak@arm.com>
---
 EmbeddedPkg/EmbeddedPkg.dec                       |   1 +
 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h | 182 ++++++++++++++++++++
 2 files changed, 183 insertions(+)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 0be102ad9c767d81a004c757f40a65063aab1d7a..151b1d503dee463d529a173d2555b6c9208100e5 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -69,6 +69,7 @@ [Guids.common]
 
 [Protocols.common]
   gHardwareInterruptProtocolGuid =  { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
+  gHardwareInterrupt2ProtocolGuid = { 0x32898322, 0x2da1, 0x474a, { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
   gEfiDebugSupportPeriodicCallbackProtocolGuid = { 0x9546e07c, 0x2cbb, 0x4c88, { 0x98, 0x6c, 0xcd, 0x34, 0x10, 0x86, 0xf0, 0x44 } }
   gEfiEblAddCommandProtocolGuid =   { 0xaeda2428, 0x9a22, 0x4637, { 0x9b, 0x21, 0x54, 0x5e, 0x28, 0xfb, 0xb8, 0x29 } }
   gEmbeddedDeviceGuid =   { 0xbf4b9d10, 0x13ec, 0x43dd, { 0x88, 0x80, 0xe9, 0xb, 0x71, 0x8f, 0x27, 0xde } }
diff --git a/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
new file mode 100644
index 0000000000000000000000000000000000000000..505052bd81afe7432e3122a2722ceab348777c29
--- /dev/null
+++ b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
@@ -0,0 +1,182 @@
+/** @file
+
+  Copyright (c) 2016-2017, Linaro Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __HARDWARE_INTERRUPT2_H__
+#define __HARDWARE_INTERRUPT2_H__
+
+#include <Protocol/HardwareInterrupt.h>
+
+// 22838932-1a2d-4a47-aaba-f3f7cf569470
+
+#define EFI_HARDWARE_INTERRUPT2_PROTOCOL_GUID \
+  { 0x32898322, 0x2d1a, 0x474a, \
+    { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
+
+typedef enum {
+  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_LOW,
+  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH,
+  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_FALLING,
+  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING,
+} EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE;
+
+typedef struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL \
+                 EFI_HARDWARE_INTERRUPT2_PROTOCOL;
+
+/**
+  Register Handler for the specified interrupt source.
+
+  @param This     Instance pointer for this protocol
+  @param Source   Hardware source of the interrupt
+  @param Handler  Callback for interrupt. NULL to unregister
+
+  @retval EFI_SUCCESS Source was updated to support Handler.
+  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_REGISTER) (
+  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+  IN HARDWARE_INTERRUPT_SOURCE Source,
+  IN HARDWARE_INTERRUPT_HANDLER Handler
+  );
+
+
+/**
+  Enable interrupt source Source.
+
+  @param This     Instance pointer for this protocol
+  @param Source   Hardware source of the interrupt
+
+  @retval EFI_SUCCESS       Source interrupt enabled.
+  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_ENABLE) (
+  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+  IN HARDWARE_INTERRUPT_SOURCE Source
+  );
+
+
+/**
+  Disable interrupt source Source.
+
+  @param This     Instance pointer for this protocol
+  @param Source   Hardware source of the interrupt
+
+  @retval EFI_SUCCESS       Source interrupt disabled.
+  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_DISABLE) (
+  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+  IN HARDWARE_INTERRUPT_SOURCE Source
+  );
+
+
+/**
+  Return current state of interrupt source Source.
+
+  @param This     Instance pointer for this protocol
+  @param Source   Hardware source of the interrupt
+  @param InterruptState  TRUE: source enabled, FALSE: source disabled.
+
+  @retval EFI_SUCCESS       InterruptState is valid
+  @retval EFI_DEVICE_ERROR  InterruptState is not valid
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_INTERRUPT_STATE) (
+  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+  IN HARDWARE_INTERRUPT_SOURCE Source,
+  IN BOOLEAN *InterruptState
+  );
+
+/**
+  Signal to the hardware that the End Of Intrrupt state
+  has been reached.
+
+  @param This     Instance pointer for this protocol
+  @param Source   Hardware source of the interrupt
+
+  @retval EFI_SUCCESS       Source interrupt EOI'ed.
+  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_END_OF_INTERRUPT) (
+  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+  IN HARDWARE_INTERRUPT_SOURCE Source
+  );
+
+/**
+  Return the configured trigger type for an interrupt source
+
+  @param This         Instance pointer for this protocol
+  @param Source       Hardware source of the interrupt
+  @param TriggerType  The configured trigger type
+
+  @retval EFI_SUCCESS       Operation successful
+  @retval EFI_DEVICE_ERROR  Information could not be returned
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE) (
+  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+  IN  HARDWARE_INTERRUPT_SOURCE Source,
+  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
+  );
+
+
+/**
+ Configure the trigger type for an interrupt source
+
+  @param This         Instance pointer for this protocol
+  @param Source       Hardware source of the interrupt
+  @param TriggerType  The trigger type to configure
+
+  @retval EFI_SUCCESS       Operation successful
+  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE) (
+  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
+  IN  HARDWARE_INTERRUPT_SOURCE Source,
+  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
+  );
+
+struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL {
+  HARDWARE_INTERRUPT2_REGISTER            RegisterInterruptSource;
+  HARDWARE_INTERRUPT2_ENABLE              EnableInterruptSource;
+  HARDWARE_INTERRUPT2_DISABLE             DisableInterruptSource;
+  HARDWARE_INTERRUPT2_INTERRUPT_STATE     GetInterruptSourceState;
+  HARDWARE_INTERRUPT2_END_OF_INTERRUPT    EndOfInterrupt;
+
+  // v2 members
+  HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE    GetTriggerType;
+  HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE    SetTriggerType;
+};
+
+extern EFI_GUID gHardwareInterrupt2ProtocolGuid;
+
+#endif
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

Re: [PATCH 2/5] EmbeddedPkg: Introduce HardwareInterrupt2 protocol

[Leif Lindholm]

On Mon, Sep 11, 2017 at 04:23:32PM +0100, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> The existing HardwareInterrupt protocol lacks the means to configure
> the level/edge and polarity properties of an interrupt. So introduce a
> new protocol HardwareInterrupt2, and add some new members that allow
> manipulation of those properties.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  EmbeddedPkg/EmbeddedPkg.dec                       |   1 +
>  EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h | 182 ++++++++++++++++++++
>  2 files changed, 183 insertions(+)
> 
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 0be102ad9c767d81a004c757f40a65063aab1d7a..151b1d503dee463d529a173d2555b6c9208100e5 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -69,6 +69,7 @@ [Guids.common]
>  
>  [Protocols.common]
>    gHardwareInterruptProtocolGuid =  { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
> +  gHardwareInterrupt2ProtocolGuid = { 0x32898322, 0x2da1, 0x474a, { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
>    gEfiDebugSupportPeriodicCallbackProtocolGuid = { 0x9546e07c, 0x2cbb, 0x4c88, { 0x98, 0x6c, 0xcd, 0x34, 0x10, 0x86, 0xf0, 0x44 } }
>    gEfiEblAddCommandProtocolGuid =   { 0xaeda2428, 0x9a22, 0x4637, { 0x9b, 0x21, 0x54, 0x5e, 0x28, 0xfb, 0xb8, 0x29 } }
>    gEmbeddedDeviceGuid =   { 0xbf4b9d10, 0x13ec, 0x43dd, { 0x88, 0x80, 0xe9, 0xb, 0x71, 0x8f, 0x27, 0xde } }
> diff --git a/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..505052bd81afe7432e3122a2722ceab348777c29
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> @@ -0,0 +1,182 @@
> +/** @file
> +
> +  Copyright (c) 2016-2017, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __HARDWARE_INTERRUPT2_H__
> +#define __HARDWARE_INTERRUPT2_H__
> +
> +#include <Protocol/HardwareInterrupt.h>
> +
> +// 22838932-1a2d-4a47-aaba-f3f7cf569470
> +
> +#define EFI_HARDWARE_INTERRUPT2_PROTOCOL_GUID \
> +  { 0x32898322, 0x2d1a, 0x474a, \
> +    { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
> +
> +typedef enum {
> +  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_LOW,
> +  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH,
> +  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_FALLING,
> +  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING,
> +} EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE;
> +
> +typedef struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL \
> +                 EFI_HARDWARE_INTERRUPT2_PROTOCOL;
> +
> +/**
> +  Register Handler for the specified interrupt source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +  @param Handler  Callback for interrupt. NULL to unregister
> +
> +  @retval EFI_SUCCESS Source was updated to support Handler.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_REGISTER) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source,
> +  IN HARDWARE_INTERRUPT_HANDLER Handler
> +  );
> +
> +
> +/**
> +  Enable interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt enabled.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_ENABLE) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source
> +  );
> +
> +
> +/**
> +  Disable interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt disabled.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_DISABLE) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source
> +  );
> +
> +
> +/**
> +  Return current state of interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +  @param InterruptState  TRUE: source enabled, FALSE: source disabled.
> +
> +  @retval EFI_SUCCESS       InterruptState is valid
> +  @retval EFI_DEVICE_ERROR  InterruptState is not valid
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_INTERRUPT_STATE) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source,
> +  IN BOOLEAN *InterruptState
> +  );
> +
> +/**
> +  Signal to the hardware that the End Of Intrrupt state
> +  has been reached.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt EOI'ed.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_END_OF_INTERRUPT) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source
> +  );
> +
> +/**
> +  Return the configured trigger type for an interrupt source
> +
> +  @param This         Instance pointer for this protocol
> +  @param Source       Hardware source of the interrupt
> +  @param TriggerType  The configured trigger type
> +
> +  @retval EFI_SUCCESS       Operation successful
> +  @retval EFI_DEVICE_ERROR  Information could not be returned
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE) (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE Source,
> +  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
> +  );
> +
> +
> +/**
> + Configure the trigger type for an interrupt source
> +
> +  @param This         Instance pointer for this protocol
> +  @param Source       Hardware source of the interrupt
> +  @param TriggerType  The trigger type to configure
> +
> +  @retval EFI_SUCCESS       Operation successful
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE) (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE Source,
> +  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
> +  );
> +
> +struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL {
> +  HARDWARE_INTERRUPT2_REGISTER            RegisterInterruptSource;
> +  HARDWARE_INTERRUPT2_ENABLE              EnableInterruptSource;
> +  HARDWARE_INTERRUPT2_DISABLE             DisableInterruptSource;
> +  HARDWARE_INTERRUPT2_INTERRUPT_STATE     GetInterruptSourceState;
> +  HARDWARE_INTERRUPT2_END_OF_INTERRUPT    EndOfInterrupt;
> +
> +  // v2 members
> +  HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE    GetTriggerType;
> +  HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE    SetTriggerType;
> +};
> +
> +extern EFI_GUID gHardwareInterrupt2ProtocolGuid;
> +
> +#endif
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 

Re: [PATCH 2/5] EmbeddedPkg: Introduce HardwareInterrupt2 protocol

[Alexei Fedorov]

In

"Signal to the hardware that the End Of Intrrupt state has been reached." Intrrupt  -> Interrupt

________________________________
From: edk2-devel <edk2-devel-bounces@lists.01.org> on behalf of Leif Lindholm <leif.lindholm@linaro.org>
Sent: 14 September 2017 17:42:24
To: Evan Lloyd
Cc: edk2-devel@lists.01.org; nd; Ard Biesheuvel
Subject: Re: [edk2] [PATCH 2/5] EmbeddedPkg: Introduce HardwareInterrupt2 protocol

On Mon, Sep 11, 2017 at 04:23:32PM +0100, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> The existing HardwareInterrupt protocol lacks the means to configure
> the level/edge and polarity properties of an interrupt. So introduce a
> new protocol HardwareInterrupt2, and add some new members that allow
> manipulation of those properties.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  EmbeddedPkg/EmbeddedPkg.dec                       |   1 +
>  EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h | 182 ++++++++++++++++++++
>  2 files changed, 183 insertions(+)
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 0be102ad9c767d81a004c757f40a65063aab1d7a..151b1d503dee463d529a173d2555b6c9208100e5 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -69,6 +69,7 @@ [Guids.common]
>
>  [Protocols.common]
>    gHardwareInterruptProtocolGuid =  { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
> +  gHardwareInterrupt2ProtocolGuid = { 0x32898322, 0x2da1, 0x474a, { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
>    gEfiDebugSupportPeriodicCallbackProtocolGuid = { 0x9546e07c, 0x2cbb, 0x4c88, { 0x98, 0x6c, 0xcd, 0x34, 0x10, 0x86, 0xf0, 0x44 } }
>    gEfiEblAddCommandProtocolGuid =   { 0xaeda2428, 0x9a22, 0x4637, { 0x9b, 0x21, 0x54, 0x5e, 0x28, 0xfb, 0xb8, 0x29 } }
>    gEmbeddedDeviceGuid =   { 0xbf4b9d10, 0x13ec, 0x43dd, { 0x88, 0x80, 0xe9, 0xb, 0x71, 0x8f, 0x27, 0xde } }
> diff --git a/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..505052bd81afe7432e3122a2722ceab348777c29
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h
> @@ -0,0 +1,182 @@
> +/** @file
> +
> +  Copyright (c) 2016-2017, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __HARDWARE_INTERRUPT2_H__
> +#define __HARDWARE_INTERRUPT2_H__
> +
> +#include <Protocol/HardwareInterrupt.h>
> +
> +// 22838932-1a2d-4a47-aaba-f3f7cf569470
> +
> +#define EFI_HARDWARE_INTERRUPT2_PROTOCOL_GUID \
> +  { 0x32898322, 0x2d1a, 0x474a, \
> +    { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
> +
> +typedef enum {
> +  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_LOW,
> +  EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH,
> +  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_FALLING,
> +  EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING,
> +} EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE;
> +
> +typedef struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL \
> +                 EFI_HARDWARE_INTERRUPT2_PROTOCOL;
> +
> +/**
> +  Register Handler for the specified interrupt source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +  @param Handler  Callback for interrupt. NULL to unregister
> +
> +  @retval EFI_SUCCESS Source was updated to support Handler.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_REGISTER) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source,
> +  IN HARDWARE_INTERRUPT_HANDLER Handler
> +  );
> +
> +
> +/**
> +  Enable interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt enabled.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_ENABLE) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source
> +  );
> +
> +
> +/**
> +  Disable interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt disabled.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_DISABLE) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source
> +  );
> +
> +
> +/**
> +  Return current state of interrupt source Source.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +  @param InterruptState  TRUE: source enabled, FALSE: source disabled.
> +
> +  @retval EFI_SUCCESS       InterruptState is valid
> +  @retval EFI_DEVICE_ERROR  InterruptState is not valid
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_INTERRUPT_STATE) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source,
> +  IN BOOLEAN *InterruptState
> +  );
> +
> +/**
> +  Signal to the hardware that the End Of Intrrupt state
> +  has been reached.
> +
> +  @param This     Instance pointer for this protocol
> +  @param Source   Hardware source of the interrupt
> +
> +  @retval EFI_SUCCESS       Source interrupt EOI'ed.
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_END_OF_INTERRUPT) (
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN HARDWARE_INTERRUPT_SOURCE Source
> +  );
> +
> +/**
> +  Return the configured trigger type for an interrupt source
> +
> +  @param This         Instance pointer for this protocol
> +  @param Source       Hardware source of the interrupt
> +  @param TriggerType  The configured trigger type
> +
> +  @retval EFI_SUCCESS       Operation successful
> +  @retval EFI_DEVICE_ERROR  Information could not be returned
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE) (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE Source,
> +  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType
> +  );
> +
> +
> +/**
> + Configure the trigger type for an interrupt source
> +
> +  @param This         Instance pointer for this protocol
> +  @param Source       Hardware source of the interrupt
> +  @param TriggerType  The trigger type to configure
> +
> +  @retval EFI_SUCCESS       Operation successful
> +  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE) (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE Source,
> +  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType
> +  );
> +
> +struct _EFI_HARDWARE_INTERRUPT2_PROTOCOL {
> +  HARDWARE_INTERRUPT2_REGISTER            RegisterInterruptSource;
> +  HARDWARE_INTERRUPT2_ENABLE              EnableInterruptSource;
> +  HARDWARE_INTERRUPT2_DISABLE             DisableInterruptSource;
> +  HARDWARE_INTERRUPT2_INTERRUPT_STATE     GetInterruptSourceState;
> +  HARDWARE_INTERRUPT2_END_OF_INTERRUPT    EndOfInterrupt;
> +
> +  // v2 members
> +  HARDWARE_INTERRUPT2_GET_TRIGGER_TYPE    GetTriggerType;
> +  HARDWARE_INTERRUPT2_SET_TRIGGER_TYPE    SetTriggerType;
> +};
> +
> +extern EFI_GUID gHardwareInterrupt2ProtocolGuid;
> +
> +#endif
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 3/5] ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol

[evan.lloyd]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

The existing HardwareInterrupt protocol lacked a means to configure the
level/edge properties of an interrupt.  The new HardwareInterrupt2
protocol introduced this capability.
This patch updates the GIC drivers to provide the new interfaces.
The changes comprise:
  Update to use HardwareInterrupt2 protocol
  Additions to register info in ArmGicLib.h
  Added new functionality (GetTriggerType and SetTriggerType)

The requirement for this change derives from a problem detected on ARM
Juno boards, but the change is of generic (ARM) relevance.

This commit is in response to review on the mailing list and, as
suggested there, rolls Girish's updates onto Ard's original example.

NOTE: At this point the GICv3 code is not tested.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Girish Pathak <girish.pathak@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Tested-by: Girish Pathak <girish.pathak@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicDxe.inf       |   3 +-
 ArmPkg/Drivers/ArmGic/ArmGicDxe.h         |  25 +++-
 ArmPkg/Include/Library/ArmGicLib.h        |  12 +-
 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c   |  44 ++++++-
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 139 +++++++++++++++++++-
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 138 ++++++++++++++++++-
 6 files changed, 354 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
index e554301c4b28022c805f69242cf6ee979d19abc2..d5921533fb68fa32c3e0705b05930700ee81da07 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
+++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
@@ -1,7 +1,7 @@
 #/** @file
 #
 #  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
-#  Copyright (c) 2012 - 2015, ARM Ltd. All rights reserved.<BR>
+#  Copyright (c) 2012 - 2017, ARM Ltd. All rights reserved.<BR>
 #
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD License
@@ -48,6 +48,7 @@ [LibraryClasses]
 
 [Protocols]
   gHardwareInterruptProtocolGuid
+  gHardwareInterrupt2ProtocolGuid
   gEfiCpuArchProtocolGuid
 
 [Pcd.common]
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
index 1018f2004e75d879a72c2d6bf37b64051e720d12..cefa4c2d4e4a05c54e51642db0f471e9a338afb6 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
+++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
@@ -1,6 +1,6 @@
 /*++
 
-Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -24,6 +24,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 #include <Protocol/Cpu.h>
 #include <Protocol/HardwareInterrupt.h>
+#include <Protocol/HardwareInterrupt2.h>
 
 extern UINTN                        mGicNumInterrupts;
 extern HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers;
@@ -34,6 +35,7 @@ extern HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers;
 EFI_STATUS
 InstallAndRegisterInterruptService (
   IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
+  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL  *Interrupt2Protocol,
   IN EFI_CPU_INTERRUPT_HANDLER          InterruptHandler,
   IN EFI_EVENT_NOTIFY                   ExitBootServicesEvent
   );
@@ -64,4 +66,25 @@ GicV3DxeInitialize (
   IN EFI_SYSTEM_TABLE   *SystemTable
   );
 
+
+// Shared code
+
+/**
+  Calculate GICD_ICFGRn base address and corresponding bit
+  field Int_config[1] of the GIC distributor register.
+
+  @param Source       Hardware source of the interrupt.
+  @param RegAddress   Corresponding GICD_ICFGRn base address.
+  @param Config1Bit   Bit number of F Int_config[1] bit in the register.
+
+  @retval EFI_SUCCESS       Source interrupt supported.
+  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
+**/
+EFI_STATUS
+GicGetDistributorIcfgBaseAndBit (
+  IN HARDWARE_INTERRUPT_SOURCE             Source,
+  OUT UINTN                               *RegAddress,
+  OUT UINTN                               *Config1Bit
+  );
+
 #endif
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index f7b546895d116f81c65a853fcdb067ec7601b2da..1c8d8cf6a7c39e2b5e2e36feb3e5433f29f488e2 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -51,10 +51,18 @@
 #define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)
 #define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)
 
+// GICD_ICDICFR bits
+#define ARM_GIC_ICDICFR_WIDTH            32   // ICDICFR is a 32 bit register
+#define ARM_GIC_ICDICFR_BYTES            (ARM_GIC_ICDICFR_WIDTH / 8)
+#define ARM_GIC_ICDICFR_F_WIDTH          2    // Each F field is 2 bits
+#define ARM_GIC_ICDICFR_F_STRIDE         16   // (32/2) F fields per register
+#define ARM_GIC_ICDICFR_F_CONFIG1_BIT    1    // Bit number within F field
+#define ARM_GIC_ICDICFR_LEVEL_TRIGGERED  0x0  // Level triggered interrupt
+#define ARM_GIC_ICDICFR_EDGE_TRIGGERED   0x1  // Edge triggered interrupt
+
 
 // GIC Redistributor
 
-
 #define ARM_GICR_CTLR_FRAME_SIZE    SIZE_64KB
 #define ARM_GICR_SGI_PPI_FRAME_SIZE SIZE_64KB
 
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
index 88cb455b75bb8e8cb22157643a392403ce93129d..13a7fae07b856025ab1c0eac97d07ec4c4df20a9 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
@@ -1,6 +1,6 @@
 /*++
 
-Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -43,6 +43,46 @@ UINTN mGicNumInterrupts                 = 0;
 
 HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers = NULL;
 
+
+/**
+  Calculate GICD_ICFGRn base address and corresponding bit
+  field Int_config[1] of the GIC distributor register.
+
+  @param Source       Hardware source of the interrupt.
+  @param RegAddress   Corresponding GICD_ICFGRn base address.
+  @param Config1Bit   Bit number of F Int_config[1] bit in the register.
+
+  @retval EFI_SUCCESS       Source interrupt supported.
+  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
+**/
+EFI_STATUS
+GicGetDistributorIcfgBaseAndBit (
+  IN HARDWARE_INTERRUPT_SOURCE             Source,
+  OUT UINTN                               *RegAddress,
+  OUT UINTN                               *Config1Bit
+  )
+{
+  UINTN                  RegIndex;
+  UINTN                  Field;
+
+  if (Source >= mGicNumInterrupts) {
+    ASSERT(Source < mGicNumInterrupts);
+    return EFI_UNSUPPORTED;
+  }
+
+  RegIndex = Source / ARM_GIC_ICDICFR_F_STRIDE;  // NOTE: truncation is significant
+  Field = Source % ARM_GIC_ICDICFR_F_STRIDE;
+  *RegAddress = PcdGet64 (PcdGicDistributorBase)
+                  + ARM_GIC_ICDICFR
+                  + (ARM_GIC_ICDICFR_BYTES * RegIndex);
+  *Config1Bit = ((Field * ARM_GIC_ICDICFR_F_WIDTH)
+                 + ARM_GIC_ICDICFR_F_CONFIG1_BIT);
+
+  return EFI_SUCCESS;
+}
+
+
+
 /**
   Register Handler for the specified interrupt source.
 
@@ -88,6 +128,7 @@ RegisterInterruptSource (
 EFI_STATUS
 InstallAndRegisterInterruptService (
   IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
+  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL  *Interrupt2Protocol,
   IN EFI_CPU_INTERRUPT_HANDLER          InterruptHandler,
   IN EFI_EVENT_NOTIFY                   ExitBootServicesEvent
   )
@@ -105,6 +146,7 @@ InstallAndRegisterInterruptService (
   Status = gBS->InstallMultipleProtocolInterfaces (
                   &gHardwareInterruptHandle,
                   &gHardwareInterruptProtocolGuid, InterruptProtocol,
+                  &gHardwareInterrupt2ProtocolGuid, Interrupt2Protocol,
                   NULL
                   );
   if (EFI_ERROR (Status)) {
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index 50ec90207b515d849cbf64f0a4b0d639b3868e60..41db2277132d47dda1a047b73eaf63323b5b9aaf 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -29,6 +29,7 @@ Abstract:
 #define ARM_GIC_DEFAULT_PRIORITY  0x80
 
 extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol;
+extern EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol;
 
 STATIC UINT32 mGicInterruptInterfaceBase;
 STATIC UINT32 mGicDistributorBase;
@@ -184,7 +185,7 @@ GicV2IrqInterruptHandler (
     // Call the registered interrupt handler.
     InterruptHandler (GicInterrupt, SystemContext);
   } else {
-    DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
+    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
     GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol, GicInterrupt);
   }
 }
@@ -200,6 +201,141 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
   GicV2EndOfInterrupt
 };
 
+/**
+  Get interrupt trigger type of an interrupt
+
+  @param This          Instance pointer for this protocol
+  @param Source        Hardware source of the interrupt.
+  @param TriggerType   Returns interrupt trigger type.
+
+  @retval EFI_SUCCESS       Source interrupt supported.
+  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+GicV2GetTriggerType (
+  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
+  IN  HARDWARE_INTERRUPT_SOURCE              Source,
+  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  *TriggerType
+  )
+{
+  UINTN                   RegAddress;
+  UINTN                   Config1Bit;
+  EFI_STATUS              Status;
+
+  Status = GicGetDistributorIcfgBaseAndBit (
+              Source,
+              &RegAddress,
+              &Config1Bit
+              );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (MmioBitFieldRead32 (RegAddress, Config1Bit, Config1Bit) == 0) {
+     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
+  } else {
+     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Set interrupt trigger type of an interrupt
+
+  @param This          Instance pointer for this protocol
+  @param Source        Hardware source of the interrupt.
+  @param TriggerType   Interrupt trigger type.
+
+  @retval EFI_SUCCESS       Source interrupt supported.
+  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+GicV2SetTriggerType (
+  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
+  IN  HARDWARE_INTERRUPT_SOURCE             Source,
+  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  TriggerType
+  )
+{
+  UINTN                   RegAddress;
+  UINTN                   Config1Bit;
+  UINT32                  Value;
+  EFI_STATUS              Status;
+  BOOLEAN                 SourceEnabled;
+
+  if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
+     && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
+          DEBUG ((DEBUG_ERROR, "Invalid interrupt trigger type: %d\n", \
+                  TriggerType));
+          ASSERT (FALSE);
+          return EFI_UNSUPPORTED;
+  }
+
+  Status = GicGetDistributorIcfgBaseAndBit (
+             Source,
+             &RegAddress,
+             &Config1Bit
+             );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = GicV2GetInterruptSourceState (
+             (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
+             Source,
+             &SourceEnabled
+             );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Value = (TriggerType == EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING)
+          ?  ARM_GIC_ICDICFR_EDGE_TRIGGERED
+          :  ARM_GIC_ICDICFR_LEVEL_TRIGGERED;
+
+  // Before changing the value, we must disable the interrupt,
+  // otherwise GIC behavior is UNPREDICTABLE.
+  if (SourceEnabled) {
+    GicV2DisableInterruptSource (
+      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
+      Source
+      );
+  }
+
+  MmioAndThenOr32 (
+    RegAddress,
+    ~(0x1 << Config1Bit),
+    Value << Config1Bit
+    );
+
+  // Restore interrupt state
+  if (SourceEnabled) {
+    GicV2EnableInterruptSource (
+      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
+      Source
+      );
+  }
+
+  return EFI_SUCCESS;
+}
+
+EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol = {
+  (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
+  (HARDWARE_INTERRUPT2_ENABLE)GicV2EnableInterruptSource,
+  (HARDWARE_INTERRUPT2_DISABLE)GicV2DisableInterruptSource,
+  (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV2GetInterruptSourceState,
+  (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV2EndOfInterrupt,
+  GicV2GetTriggerType,
+  GicV2SetTriggerType
+};
+
 /**
   Shutdown our hardware
 
@@ -324,6 +460,7 @@ GicV2DxeInitialize (
 
   Status = InstallAndRegisterInterruptService (
              &gHardwareInterruptV2Protocol,
+             &gHardwareInterrupt2V2Protocol,
              GicV2IrqInterruptHandler,
              GicV2ExitBootServicesEvent
              );
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 69b2d8d794e151e25f06cbea079e2796d9793a43..0c1d5b53119e8cad7ae67c57e9efaa51c1386be6 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -19,6 +19,7 @@
 #define ARM_GIC_DEFAULT_PRIORITY  0x80
 
 extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol;
+extern EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol;
 
 STATIC UINTN mGicDistributorBase;
 STATIC UINTN mGicRedistributorsBase;
@@ -177,7 +178,7 @@ GicV3IrqInterruptHandler (
     // Call the registered interrupt handler.
     InterruptHandler (GicInterrupt, SystemContext);
   } else {
-    DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
+    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
     GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, GicInterrupt);
   }
 }
@@ -193,6 +194,140 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = {
   GicV3EndOfInterrupt
 };
 
+/**
+  Get interrupt trigger type of an interrupt
+
+  @param This          Instance pointer for this protocol
+  @param Source        Hardware source of the interrupt.
+  @param TriggerType   Returns interrupt trigger type.
+
+  @retval EFI_SUCCESS       Source interrupt supported.
+  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+GicV3GetTriggerType (
+  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
+  IN  HARDWARE_INTERRUPT_SOURCE             Source,
+  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  *TriggerType
+  )
+{
+  UINTN                   RegAddress;
+  UINTN                   Config1Bit;
+  EFI_STATUS              Status;
+
+  Status = GicGetDistributorIcfgBaseAndBit (
+             Source,
+             &RegAddress,
+             &Config1Bit
+             );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (MmioBitFieldRead32 (RegAddress, Config1Bit, Config1Bit) == 0) {
+     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
+  } else {
+     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Set interrupt trigger type of an interrupt
+
+  @param This          Instance pointer for this protocol
+  @param Source        Hardware source of the interrupt.
+  @param TriggerType   Interrupt trigger type.
+
+  @retval EFI_SUCCESS       Source interrupt supported.
+  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+GicV3SetTriggerType (
+  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
+  IN  HARDWARE_INTERRUPT_SOURCE             Source,
+  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  TriggerType
+  )
+{
+  UINTN                   RegAddress;
+  UINTN                   Config1Bit;
+  UINT32                  Value;
+  EFI_STATUS              Status;
+  BOOLEAN                 SourceEnabled;
+
+  if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
+     && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {
+          DEBUG ((DEBUG_ERROR, "Invalid interrupt trigger type: %d\n", \
+                 TriggerType));
+          ASSERT (FALSE);
+          return EFI_UNSUPPORTED;
+  }
+
+  Status = GicGetDistributorIcfgBaseAndBit (
+             Source,
+             &RegAddress,
+             &Config1Bit
+             );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = GicV3GetInterruptSourceState (
+             (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
+             Source,
+             &SourceEnabled
+             );
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Value = (TriggerType == EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING)
+          ?  ARM_GIC_ICDICFR_EDGE_TRIGGERED
+          :  ARM_GIC_ICDICFR_LEVEL_TRIGGERED;
+
+  // Before changing the value, we must disable the interrupt,
+  // otherwise GIC behavior is UNPREDICTABLE.
+  if (SourceEnabled) {
+    GicV3DisableInterruptSource (
+      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
+      Source
+      );
+  }
+
+  MmioAndThenOr32 (
+    RegAddress,
+    ~(0x1 << Config1Bit),
+    Value << Config1Bit
+    );
+  // Restore interrupt state
+  if (SourceEnabled) {
+    GicV3EnableInterruptSource (
+      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
+      Source
+      );
+  }
+
+  return EFI_SUCCESS;
+}
+
+EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol = {
+  (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
+  (HARDWARE_INTERRUPT2_ENABLE)GicV3EnableInterruptSource,
+  (HARDWARE_INTERRUPT2_DISABLE)GicV3DisableInterruptSource,
+  (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV3GetInterruptSourceState,
+  (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV3EndOfInterrupt,
+  GicV3GetTriggerType,
+  GicV3SetTriggerType
+};
+
 /**
   Shutdown our hardware
 
@@ -354,6 +489,7 @@ GicV3DxeInitialize (
 
   Status = InstallAndRegisterInterruptService (
              &gHardwareInterruptV3Protocol,
+             &gHardwareInterrupt2V3Protocol,
              GicV3IrqInterruptHandler,
              GicV3ExitBootServicesEvent
              );
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

Re: [PATCH 3/5] ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol

[Leif Lindholm]

On Mon, Sep 11, 2017 at 04:23:33PM +0100, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> The existing HardwareInterrupt protocol lacked a means to configure the
> level/edge properties of an interrupt.  The new HardwareInterrupt2
> protocol introduced this capability.
> This patch updates the GIC drivers to provide the new interfaces.
> The changes comprise:
>   Update to use HardwareInterrupt2 protocol
>   Additions to register info in ArmGicLib.h
>   Added new functionality (GetTriggerType and SetTriggerType)
> 
> The requirement for this change derives from a problem detected on ARM
> Juno boards, but the change is of generic (ARM) relevance.
> 
> This commit is in response to review on the mailing list and, as
> suggested there, rolls Girish's updates onto Ard's original example.
> 
> NOTE: At this point the GICv3 code is not tested.

Is this still the case?
I don't see how the code can be merged while that holds true.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.inf       |   3 +-
>  ArmPkg/Drivers/ArmGic/ArmGicDxe.h         |  25 +++-
>  ArmPkg/Include/Library/ArmGicLib.h        |  12 +-
>  ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c   |  44 ++++++-
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 139 +++++++++++++++++++-
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 138 ++++++++++++++++++-
>  6 files changed, 354 insertions(+), 7 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> index e554301c4b28022c805f69242cf6ee979d19abc2..d5921533fb68fa32c3e0705b05930700ee81da07 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
> @@ -1,7 +1,7 @@
>  #/** @file
>  #
>  #  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> -#  Copyright (c) 2012 - 2015, ARM Ltd. All rights reserved.<BR>
> +#  Copyright (c) 2012 - 2017, ARM Ltd. All rights reserved.<BR>
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -48,6 +48,7 @@ [LibraryClasses]
>  
>  [Protocols]
>    gHardwareInterruptProtocolGuid
> +  gHardwareInterrupt2ProtocolGuid
>    gEfiCpuArchProtocolGuid
>  
>  [Pcd.common]
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> index 1018f2004e75d879a72c2d6bf37b64051e720d12..cefa4c2d4e4a05c54e51642db0f471e9a338afb6 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.h
> @@ -1,6 +1,6 @@
>  /*++
>  
> -Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
> @@ -24,6 +24,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  
>  #include <Protocol/Cpu.h>
>  #include <Protocol/HardwareInterrupt.h>
> +#include <Protocol/HardwareInterrupt2.h>
>  
>  extern UINTN                        mGicNumInterrupts;
>  extern HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers;
> @@ -34,6 +35,7 @@ extern HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers;
>  EFI_STATUS
>  InstallAndRegisterInterruptService (
>    IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL  *Interrupt2Protocol,
>    IN EFI_CPU_INTERRUPT_HANDLER          InterruptHandler,
>    IN EFI_EVENT_NOTIFY                   ExitBootServicesEvent
>    );
> @@ -64,4 +66,25 @@ GicV3DxeInitialize (
>    IN EFI_SYSTEM_TABLE   *SystemTable
>    );
>  
> +
> +// Shared code
> +
> +/**
> +  Calculate GICD_ICFGRn base address and corresponding bit
> +  field Int_config[1] of the GIC distributor register.
> +
> +  @param Source       Hardware source of the interrupt.
> +  @param RegAddress   Corresponding GICD_ICFGRn base address.
> +  @param Config1Bit   Bit number of F Int_config[1] bit in the register.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +EFI_STATUS
> +GicGetDistributorIcfgBaseAndBit (
> +  IN HARDWARE_INTERRUPT_SOURCE             Source,
> +  OUT UINTN                               *RegAddress,
> +  OUT UINTN                               *Config1Bit
> +  );
> +
>  #endif
> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
> index f7b546895d116f81c65a853fcdb067ec7601b2da..1c8d8cf6a7c39e2b5e2e36feb3e5433f29f488e2 100644
> --- a/ArmPkg/Include/Library/ArmGicLib.h
> +++ b/ArmPkg/Include/Library/ArmGicLib.h
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2017, ARM Limited. All rights reserved.
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD License
> @@ -51,10 +51,18 @@
>  #define ARM_GIC_ICDDCR_ARE      (1 << 4) // Affinity Routing Enable (ARE)
>  #define ARM_GIC_ICDDCR_DS       (1 << 6) // Disable Security (DS)
>  
> +// GICD_ICDICFR bits
> +#define ARM_GIC_ICDICFR_WIDTH            32   // ICDICFR is a 32 bit register
> +#define ARM_GIC_ICDICFR_BYTES            (ARM_GIC_ICDICFR_WIDTH / 8)
> +#define ARM_GIC_ICDICFR_F_WIDTH          2    // Each F field is 2 bits
> +#define ARM_GIC_ICDICFR_F_STRIDE         16   // (32/2) F fields per register
> +#define ARM_GIC_ICDICFR_F_CONFIG1_BIT    1    // Bit number within F field
> +#define ARM_GIC_ICDICFR_LEVEL_TRIGGERED  0x0  // Level triggered interrupt
> +#define ARM_GIC_ICDICFR_EDGE_TRIGGERED   0x1  // Edge triggered interrupt
> +
>  
>  // GIC Redistributor
>  
> -

Spurious whitespace change.

>  #define ARM_GICR_CTLR_FRAME_SIZE    SIZE_64KB
>  #define ARM_GICR_SGI_PPI_FRAME_SIZE SIZE_64KB
>  
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> index 88cb455b75bb8e8cb22157643a392403ce93129d..13a7fae07b856025ab1c0eac97d07ec4c4df20a9 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> @@ -1,6 +1,6 @@
>  /*++
>  
> -Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
> @@ -43,6 +43,46 @@ UINTN mGicNumInterrupts                 = 0;
>  
>  HARDWARE_INTERRUPT_HANDLER  *gRegisteredInterruptHandlers = NULL;
>  
> +
> +/**
> +  Calculate GICD_ICFGRn base address and corresponding bit
> +  field Int_config[1] of the GIC distributor register.
> +
> +  @param Source       Hardware source of the interrupt.
> +  @param RegAddress   Corresponding GICD_ICFGRn base address.
> +  @param Config1Bit   Bit number of F Int_config[1] bit in the register.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +EFI_STATUS
> +GicGetDistributorIcfgBaseAndBit (
> +  IN HARDWARE_INTERRUPT_SOURCE             Source,
> +  OUT UINTN                               *RegAddress,
> +  OUT UINTN                               *Config1Bit
> +  )
> +{
> +  UINTN                  RegIndex;
> +  UINTN                  Field;
> +
> +  if (Source >= mGicNumInterrupts) {
> +    ASSERT(Source < mGicNumInterrupts);
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  RegIndex = Source / ARM_GIC_ICDICFR_F_STRIDE;  // NOTE: truncation is significant
> +  Field = Source % ARM_GIC_ICDICFR_F_STRIDE;
> +  *RegAddress = PcdGet64 (PcdGicDistributorBase)
> +                  + ARM_GIC_ICDICFR
> +                  + (ARM_GIC_ICDICFR_BYTES * RegIndex);

Should these continuation lines really be indented?

> +  *Config1Bit = ((Field * ARM_GIC_ICDICFR_F_WIDTH)
> +                 + ARM_GIC_ICDICFR_F_CONFIG1_BIT);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +
> +
>  /**
>    Register Handler for the specified interrupt source.
>  
> @@ -88,6 +128,7 @@ RegisterInterruptSource (
>  EFI_STATUS
>  InstallAndRegisterInterruptService (
>    IN EFI_HARDWARE_INTERRUPT_PROTOCOL   *InterruptProtocol,
> +  IN EFI_HARDWARE_INTERRUPT2_PROTOCOL  *Interrupt2Protocol,
>    IN EFI_CPU_INTERRUPT_HANDLER          InterruptHandler,
>    IN EFI_EVENT_NOTIFY                   ExitBootServicesEvent
>    )
> @@ -105,6 +146,7 @@ InstallAndRegisterInterruptService (
>    Status = gBS->InstallMultipleProtocolInterfaces (
>                    &gHardwareInterruptHandle,
>                    &gHardwareInterruptProtocolGuid, InterruptProtocol,
> +                  &gHardwareInterrupt2ProtocolGuid, Interrupt2Protocol,
>                    NULL
>                    );
>    if (EFI_ERROR (Status)) {
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> index 50ec90207b515d849cbf64f0a4b0d639b3868e60..41db2277132d47dda1a047b73eaf63323b5b9aaf 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> @@ -29,6 +29,7 @@ Abstract:
>  #define ARM_GIC_DEFAULT_PRIORITY  0x80
>  
>  extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol;
> +extern EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol;
>  
>  STATIC UINT32 mGicInterruptInterfaceBase;
>  STATIC UINT32 mGicDistributorBase;
> @@ -184,7 +185,7 @@ GicV2IrqInterruptHandler (
>      // Call the registered interrupt handler.
>      InterruptHandler (GicInterrupt, SystemContext);
>    } else {
> -    DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
> +    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));

I don't see anything changing here other than changing the EFI_D_ERROR
to DEBUG_ERROR. Preferably leave such non-functional changes out of
functional patches, even if it leaves the file inconsistent.
If a DEBUG line has a functional modification, clearly it should be
updated to the new form at the same time.

>      GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol, GicInterrupt);
>    }
>  }
> @@ -200,6 +201,141 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = {
>    GicV2EndOfInterrupt
>  };
>  
> +/**
> +  Get interrupt trigger type of an interrupt
> +
> +  @param This          Instance pointer for this protocol
> +  @param Source        Hardware source of the interrupt.
> +  @param TriggerType   Returns interrupt trigger type.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV2GetTriggerType (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE              Source,
> +  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  *TriggerType
> +  )
> +{
> +  UINTN                   RegAddress;
> +  UINTN                   Config1Bit;
> +  EFI_STATUS              Status;
> +
> +  Status = GicGetDistributorIcfgBaseAndBit (
> +              Source,
> +              &RegAddress,
> +              &Config1Bit
> +              );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (MmioBitFieldRead32 (RegAddress, Config1Bit, Config1Bit) == 0) {

Why not

  if (MmioRead32 (RegAddress) & (1 << Config1Bit) == 0) {

?

> +     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
> +  } else {
> +     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Set interrupt trigger type of an interrupt
> +
> +  @param This          Instance pointer for this protocol
> +  @param Source        Hardware source of the interrupt.
> +  @param TriggerType   Interrupt trigger type.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV2SetTriggerType (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE             Source,
> +  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  TriggerType
> +  )
> +{
> +  UINTN                   RegAddress;
> +  UINTN                   Config1Bit;
> +  UINT32                  Value;
> +  EFI_STATUS              Status;
> +  BOOLEAN                 SourceEnabled;
> +
> +  if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> +     && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {

One more space indentation.
Could benefit from added parentheses:

  if ((TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING) &&
      (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH)) {

> +          DEBUG ((DEBUG_ERROR, "Invalid interrupt trigger type: %d\n", \
> +                  TriggerType));
> +          ASSERT (FALSE);
> +          return EFI_UNSUPPORTED;
> +  }
> +
> +  Status = GicGetDistributorIcfgBaseAndBit (
> +             Source,
> +             &RegAddress,
> +             &Config1Bit
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = GicV2GetInterruptSourceState (
> +             (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +             Source,
> +             &SourceEnabled
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Value = (TriggerType == EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING)
> +          ?  ARM_GIC_ICDICFR_EDGE_TRIGGERED
> +          :  ARM_GIC_ICDICFR_LEVEL_TRIGGERED;
> +
> +  // Before changing the value, we must disable the interrupt,
> +  // otherwise GIC behavior is UNPREDICTABLE.
> +  if (SourceEnabled) {
> +    GicV2DisableInterruptSource (
> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +      Source
> +      );
> +  }
> +
> +  MmioAndThenOr32 (
> +    RegAddress,
> +    ~(0x1 << Config1Bit),
> +    Value << Config1Bit
> +    );
> +
> +  // Restore interrupt state
> +  if (SourceEnabled) {
> +    GicV2EnableInterruptSource (
> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +      Source
> +      );
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol = {
> +  (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
> +  (HARDWARE_INTERRUPT2_ENABLE)GicV2EnableInterruptSource,
> +  (HARDWARE_INTERRUPT2_DISABLE)GicV2DisableInterruptSource,
> +  (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV2GetInterruptSourceState,
> +  (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV2EndOfInterrupt,
> +  GicV2GetTriggerType,
> +  GicV2SetTriggerType
> +};
> +
>  /**
>    Shutdown our hardware
>  
> @@ -324,6 +460,7 @@ GicV2DxeInitialize (
>  
>    Status = InstallAndRegisterInterruptService (
>               &gHardwareInterruptV2Protocol,
> +             &gHardwareInterrupt2V2Protocol,
>               GicV2IrqInterruptHandler,
>               GicV2ExitBootServicesEvent
>               );
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index 69b2d8d794e151e25f06cbea079e2796d9793a43..0c1d5b53119e8cad7ae67c57e9efaa51c1386be6 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -19,6 +19,7 @@
>  #define ARM_GIC_DEFAULT_PRIORITY  0x80
>  
>  extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol;
> +extern EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol;
>  
>  STATIC UINTN mGicDistributorBase;
>  STATIC UINTN mGicRedistributorsBase;
> @@ -177,7 +178,7 @@ GicV3IrqInterruptHandler (
>      // Call the registered interrupt handler.
>      InterruptHandler (GicInterrupt, SystemContext);
>    } else {
> -    DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
> +    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));

Non-functional change, please leave out.

>      GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, GicInterrupt);
>    }
>  }
> @@ -193,6 +194,140 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = {
>    GicV3EndOfInterrupt
>  };
>  
> +/**
> +  Get interrupt trigger type of an interrupt
> +
> +  @param This          Instance pointer for this protocol
> +  @param Source        Hardware source of the interrupt.
> +  @param TriggerType   Returns interrupt trigger type.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV3GetTriggerType (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE             Source,
> +  OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  *TriggerType
> +  )
> +{
> +  UINTN                   RegAddress;
> +  UINTN                   Config1Bit;
> +  EFI_STATUS              Status;
> +
> +  Status = GicGetDistributorIcfgBaseAndBit (
> +             Source,
> +             &RegAddress,
> +             &Config1Bit
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (MmioBitFieldRead32 (RegAddress, Config1Bit, Config1Bit) == 0) {

  if (MmioRead32 (RegAddress) & (1 << Config1Bit) == 0) {

?

> +     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH;
> +  } else {
> +     *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Set interrupt trigger type of an interrupt
> +
> +  @param This          Instance pointer for this protocol
> +  @param Source        Hardware source of the interrupt.
> +  @param TriggerType   Interrupt trigger type.
> +
> +  @retval EFI_SUCCESS       Source interrupt supported.
> +  @retval EFI_UNSUPPORTED   Source interrupt is not supported.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +GicV3SetTriggerType (
> +  IN  EFI_HARDWARE_INTERRUPT2_PROTOCOL      *This,
> +  IN  HARDWARE_INTERRUPT_SOURCE             Source,
> +  IN  EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE  TriggerType
> +  )
> +{
> +  UINTN                   RegAddress;
> +  UINTN                   Config1Bit;
> +  UINT32                  Value;
> +  EFI_STATUS              Status;
> +  BOOLEAN                 SourceEnabled;
> +
> +  if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> +     && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) {

Missing indentation space, and could do with added parentheses).

/
    Leif

> +          DEBUG ((DEBUG_ERROR, "Invalid interrupt trigger type: %d\n", \
> +                 TriggerType));
> +          ASSERT (FALSE);
> +          return EFI_UNSUPPORTED;
> +  }
> +
> +  Status = GicGetDistributorIcfgBaseAndBit (
> +             Source,
> +             &RegAddress,
> +             &Config1Bit
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = GicV3GetInterruptSourceState (
> +             (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +             Source,
> +             &SourceEnabled
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Value = (TriggerType == EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING)
> +          ?  ARM_GIC_ICDICFR_EDGE_TRIGGERED
> +          :  ARM_GIC_ICDICFR_LEVEL_TRIGGERED;
> +
> +  // Before changing the value, we must disable the interrupt,
> +  // otherwise GIC behavior is UNPREDICTABLE.
> +  if (SourceEnabled) {
> +    GicV3DisableInterruptSource (
> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +      Source
> +      );
> +  }
> +
> +  MmioAndThenOr32 (
> +    RegAddress,
> +    ~(0x1 << Config1Bit),
> +    Value << Config1Bit
> +    );
> +  // Restore interrupt state
> +  if (SourceEnabled) {
> +    GicV3EnableInterruptSource (
> +      (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This,
> +      Source
> +      );
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol = {
> +  (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource,
> +  (HARDWARE_INTERRUPT2_ENABLE)GicV3EnableInterruptSource,
> +  (HARDWARE_INTERRUPT2_DISABLE)GicV3DisableInterruptSource,
> +  (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV3GetInterruptSourceState,
> +  (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV3EndOfInterrupt,
> +  GicV3GetTriggerType,
> +  GicV3SetTriggerType
> +};
> +
>  /**
>    Shutdown our hardware
>  
> @@ -354,6 +489,7 @@ GicV3DxeInitialize (
>  
>    Status = InstallAndRegisterInterruptService (
>               &gHardwareInterruptV3Protocol,
> +             &gHardwareInterrupt2V3Protocol,
>               GicV3IrqInterruptHandler,
>               GicV3ExitBootServicesEvent
>               );
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 

[PATCH 4/5] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type

[evan.lloyd]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Utilise the new HardwareInterrupt2 protocol to adjust the
Edge/Level characteristics of the Watchdog interrupt.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Girish Pathak <girish.pathak@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Tested-by: Girish Pathak <girish.pathak@arm.com>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |  6 ++---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 28 ++++++++++++--------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
index fece14cc18315cd15510680c438288687b60c018..ba0403d7fdc3589803c643c27a44918e73afa97e 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
+#  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
 #
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD License
@@ -47,7 +47,7 @@ [Pcd.common]
 
 [Protocols]
   gEfiWatchdogTimerArchProtocolGuid
-  gHardwareInterruptProtocolGuid
+  gHardwareInterrupt2ProtocolGuid
 
 [Depex]
-  gHardwareInterruptProtocolGuid
+  gHardwareInterrupt2ProtocolGuid
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 6d102e25047253048ac555d6fb5de7223d78f381..69844db2e11f51907e6c8bff5c67d27ceb498150 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
+*  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD
@@ -24,8 +24,8 @@
 #include <Library/UefiLib.h>
 #include <Library/ArmGenericTimerCounterLib.h>
 
+#include <Protocol/HardwareInterrupt2.h>
 #include <Protocol/WatchdogTimer.h>
-#include <Protocol/HardwareInterrupt.h>
 
 #include "GenericWatchdog.h"
 
@@ -41,7 +41,7 @@ UINTN mTimerFrequencyHz = 0;
 // It is therefore stored here. 0 means the timer is not running.
 UINT64 mNumTimerTicks = 0;
 
-EFI_HARDWARE_INTERRUPT_PROTOCOL *mInterruptProtocol;
+EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
 
 EFI_STATUS
 WatchdogWriteOffsetRegister (
@@ -320,7 +320,7 @@ GenericWatchdogEntry (
   if (!EFI_ERROR (Status)) {
     // Install interrupt handler
     Status = gBS->LocateProtocol (
-                    &gHardwareInterruptProtocolGuid,
+                    &gHardwareInterrupt2ProtocolGuid,
                     NULL,
                     (VOID **)&mInterruptProtocol
                     );
@@ -331,13 +331,19 @@ GenericWatchdogEntry (
                                     WatchdogInterruptHandler
                                     );
       if (!EFI_ERROR (Status)) {
-        // Install the Timer Architectural Protocol onto a new handle
-        Handle = NULL;
-        Status = gBS->InstallMultipleProtocolInterfaces (
-                        &Handle,
-                        &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
-                        NULL
-                        );
+        Status = mInterruptProtocol->SetTriggerType (
+                                    mInterruptProtocol,
+                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+                                    EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
+        if (!EFI_ERROR (Status)) {
+          // Install the Timer Architectural Protocol onto a new handle
+          Handle = NULL;
+          Status = gBS->InstallMultipleProtocolInterfaces (
+                          &Handle,
+                          &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
+                          NULL
+                          );
+        }
       }
     }
   }
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

Re: [PATCH 4/5] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type

[Leif Lindholm]

On Mon, Sep 11, 2017 at 04:23:34PM +0100, evan.lloyd@arm.com wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Utilise the new HardwareInterrupt2 protocol to adjust the
> Edge/Level characteristics of the Watchdog interrupt.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Tested-by: Girish Pathak <girish.pathak@arm.com>

Looks plausible, but I would loke to see it tested by someone not
directly involved in development.
Armada70x0 also uses this, so adding Marcin on cc.

Beyond that, I'm told in Ryan's absence Arvind, Daniil and Thomas are
"platform owners" for Juno, so if one of you could have a spin, that'd
be great.

Pending another Tested-by:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> ---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |  6 ++---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 28 ++++++++++++--------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> index fece14cc18315cd15510680c438288687b60c018..ba0403d7fdc3589803c643c27a44918e73afa97e 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
> @@ -1,5 +1,5 @@
>  #
> -#  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
> +#  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -47,7 +47,7 @@ [Pcd.common]
>  
>  [Protocols]
>    gEfiWatchdogTimerArchProtocolGuid
> -  gHardwareInterruptProtocolGuid
> +  gHardwareInterrupt2ProtocolGuid
>  
>  [Depex]
> -  gHardwareInterruptProtocolGuid
> +  gHardwareInterrupt2ProtocolGuid
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 6d102e25047253048ac555d6fb5de7223d78f381..69844db2e11f51907e6c8bff5c67d27ceb498150 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
> +*  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD
> @@ -24,8 +24,8 @@
>  #include <Library/UefiLib.h>
>  #include <Library/ArmGenericTimerCounterLib.h>
>  
> +#include <Protocol/HardwareInterrupt2.h>
>  #include <Protocol/WatchdogTimer.h>
> -#include <Protocol/HardwareInterrupt.h>
>  
>  #include "GenericWatchdog.h"
>  
> @@ -41,7 +41,7 @@ UINTN mTimerFrequencyHz = 0;
>  // It is therefore stored here. 0 means the timer is not running.
>  UINT64 mNumTimerTicks = 0;
>  
> -EFI_HARDWARE_INTERRUPT_PROTOCOL *mInterruptProtocol;
> +EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
>  
>  EFI_STATUS
>  WatchdogWriteOffsetRegister (
> @@ -320,7 +320,7 @@ GenericWatchdogEntry (
>    if (!EFI_ERROR (Status)) {
>      // Install interrupt handler
>      Status = gBS->LocateProtocol (
> -                    &gHardwareInterruptProtocolGuid,
> +                    &gHardwareInterrupt2ProtocolGuid,
>                      NULL,
>                      (VOID **)&mInterruptProtocol
>                      );
> @@ -331,13 +331,19 @@ GenericWatchdogEntry (
>                                      WatchdogInterruptHandler
>                                      );
>        if (!EFI_ERROR (Status)) {
> -        // Install the Timer Architectural Protocol onto a new handle
> -        Handle = NULL;
> -        Status = gBS->InstallMultipleProtocolInterfaces (
> -                        &Handle,
> -                        &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
> -                        NULL
> -                        );
> +        Status = mInterruptProtocol->SetTriggerType (
> +                                    mInterruptProtocol,
> +                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> +                                    EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
> +        if (!EFI_ERROR (Status)) {
> +          // Install the Timer Architectural Protocol onto a new handle
> +          Handle = NULL;
> +          Status = gBS->InstallMultipleProtocolInterfaces (
> +                          &Handle,
> +                          &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
> +                          NULL
> +                          );
> +        }
>        }
>      }
>    }
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 

[PATCH 5/5] ArmPkg: Tidy up GenericWatchdogDxe.c

[evan.lloyd]

From: Evan Lloyd <evan.lloyd@arm.com>

This cosmetic change has no functional content.
It adjusts comment oddities, etc, noticed during previous work.
Specific changes are:
    Re-order #includes
    Use ns consistently (always "100ns" not sometimes "100 nS")
    Reflow overlength comments
    Change multiline comments to C style
    Adjust indent for overlength code lines.
    Replace explicit test and assert with ASSERT_EFI_ERROR.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 127 ++++++++++----------
 1 file changed, 61 insertions(+), 66 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 69844db2e11f51907e6c8bff5c67d27ceb498150..7c4c9ecd4e12d433e222d7d08adf20bda1ff9842 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -29,16 +29,16 @@
 
 #include "GenericWatchdog.h"
 
-// The number of 100ns periods (the unit of time passed to these functions)
-// in a second
+/* The number of 100ns periods (the unit of time passed to these functions)
+   in a second */
 #define TIME_UNITS_PER_SECOND 10000000
 
-// Tick frequency of the generic timer that is the basis of the generic watchdog
+// Tick frequency of the generic timer basis of the generic watchdog.
 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.
+/* 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. */
 UINT64 mNumTimerTicks = 0;
 
 EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
@@ -75,8 +75,7 @@ WatchdogDisable (
   return MmioWrite32 (GENERIC_WDOG_CONTROL_STATUS_REG, GENERIC_WDOG_DISABLED);
 }
 
-/**
-    On exiting boot services we must make sure the Watchdog Timer
+/** On exiting boot services we must make sure the Watchdog Timer
     is stopped.
 **/
 VOID
@@ -90,9 +89,8 @@ WatchdogExitBootServicesEvent (
   mNumTimerTicks = 0;
 }
 
-/*
-  This function is called when the watchdog's first signal (WS0) goes high.
-  It uses the ResetSystem Runtime Service to reset the board.
+/* This function is called when the watchdog's first signal (WS0) goes high.
+   It uses the ResetSystem Runtime Service to reset the board.
 */
 VOID
 EFIAPI
@@ -101,7 +99,7 @@ WatchdogInterruptHandler (
   IN  EFI_SYSTEM_CONTEXT          SystemContext
   )
 {
-  STATIC CONST CHAR16      ResetString[] = L"The generic watchdog timer ran out.";
+  STATIC CONST CHAR16 ResetString[]= L"The generic watchdog timer ran out.";
 
   WatchdogDisable ();
 
@@ -126,10 +124,10 @@ WatchdogInterruptHandler (
   then the new handler is registered and EFI_SUCCESS is returned.
   If NotifyFunction is NULL, and a handler is already registered,
   then that handler is unregistered.
-  If an attempt is made to register a handler when a handler is already registered,
-  then EFI_ALREADY_STARTED is returned.
-  If an attempt is made to unregister a handler when a handler is not registered,
-  then EFI_INVALID_PARAMETER is returned.
+  If an attempt is made to register a handler when a handler is already
+  registered, then EFI_ALREADY_STARTED is returned.
+  If an attempt is made to unregister a handler when a handler is not
+  registered, then EFI_INVALID_PARAMETER is returned.
 
   @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
   @param  NotifyFunction   The function to call when a timer interrupt fires.
@@ -139,11 +137,7 @@ WatchdogInterruptHandler (
                            information is used to signal timer based events.
                            NULL will unregister the handler.
 
-  @retval EFI_SUCCESS           The watchdog timer handler was registered.
-  @retval EFI_ALREADY_STARTED   NotifyFunction is not NULL, and a handler is already
-                                registered.
-  @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler was not
-                                previously registered.
+  @retval EFI_UNSUPPORTED       The code does not support NotifyFunction.
 
 **/
 EFI_STATUS
@@ -160,18 +154,18 @@ WatchdogRegisterHandler (
 
 /**
   This function sets the amount of time to wait before firing the watchdog
-  timer to TimerPeriod 100 nS units.  If TimerPeriod is 0, then the watchdog
+  timer to TimerPeriod 100ns units.  If TimerPeriod is 0, then the watchdog
   timer is disabled.
 
   @param  This             The EFI_WATCHDOG_TIMER_ARCH_PROTOCOL instance.
-  @param  TimerPeriod      The amount of time in 100 nS units to wait before the watchdog
-                           timer is fired. If TimerPeriod is zero, then the watchdog
-                           timer is disabled.
+  @param  TimerPeriod      The amount of time in 100ns units to wait before
+                           the watchdog timer is fired. If TimerPeriod is zero,
+                           then the watchdog timer is disabled.
 
-  @retval EFI_SUCCESS           The watchdog timer has been programmed to fire in Time
-                                100 nS units.
-  @retval EFI_DEVICE_ERROR      A watchdog timer could not be programmed due to a device
-                                error.
+  @retval EFI_SUCCESS           The watchdog timer has been programmed to fire
+                                in Time  100ns units.
+  @retval EFI_DEVICE_ERROR      A watchdog timer could not be programmed due
+                                to a device error.
 
 **/
 EFI_STATUS
@@ -184,7 +178,7 @@ WatchdogSetTimerPeriod (
   UINTN       SystemCount;
   EFI_STATUS  Status;
 
-  // if TimerPerdiod is 0, this is a request to stop the watchdog.
+  // if TimerPeriod is 0, this is a request to stop the watchdog.
   if (TimerPeriod == 0) {
     mNumTimerTicks = 0;
     return WatchdogDisable ();
@@ -193,17 +187,16 @@ WatchdogSetTimerPeriod (
   // Work out how many timer ticks will equate to TimerPeriod
   mNumTimerTicks = (mTimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND;
 
-
-  // If the number of required ticks is greater than the max number the
-  // watchdog's offset register (WOR) can hold, we need to manually compute and
-  // set the compare register (WCV)
+  /* 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) {
 
-    // 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.
+    /* 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. */
 
     Status = WatchdogWriteOffsetRegister (MAX_UINT32);
     if (EFI_ERROR (Status)) {
@@ -221,14 +214,14 @@ WatchdogSetTimerPeriod (
 }
 
 /**
-  This function retrieves the period of timer interrupts in 100 ns units,
+  This function retrieves the period of timer interrupts in 100ns units,
   returns that value in TimerPeriod, and returns EFI_SUCCESS.  If TimerPeriod
   is NULL, then EFI_INVALID_PARAMETER is returned.  If a TimerPeriod of 0 is
   returned, then the timer is currently disabled.
 
   @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
-  @param  TimerPeriod      A pointer to the timer period to retrieve in 100
-                           ns units. If 0 is returned, then the timer is
+  @param  TimerPeriod      A pointer to the timer period to retrieve in
+                           100ns units. If 0 is returned, then the timer is
                            currently disabled.
 
 
@@ -275,19 +268,19 @@ WatchdogGetTimerPeriod (
         this function will not have any chance of executing.
 
   @param SetTimerPeriod
-  Sets the period of the timer interrupt in 100 nS units.
+  Sets the period of the timer interrupt in 100ns units.
   This function is optional, and may return EFI_UNSUPPORTED.
   If this function is supported, then the timer period will
   be rounded up to the nearest supported timer period.
 
   @param GetTimerPeriod
-  Retrieves the period of the timer interrupt in 100 nS units.
+  Retrieves the period of the timer interrupt in 100ns units.
 
 **/
 EFI_WATCHDOG_TIMER_ARCH_PROTOCOL    gWatchdogTimer = {
-  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER) WatchdogRegisterHandler,
-  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD) WatchdogSetTimerPeriod,
-  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD) WatchdogGetTimerPeriod
+  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER)WatchdogRegisterHandler,
+  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD)WatchdogSetTimerPeriod,
+  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD)WatchdogGetTimerPeriod
 };
 
 EFI_EVENT                           EfiExitBootServicesEvent = (EFI_EVENT)NULL;
@@ -303,9 +296,9 @@ GenericWatchdogEntry (
   EFI_HANDLE                      Handle;
 
 
-  // Make sure the Watchdog Timer Architectural Protocol has not been installed
-  // in the system yet.
-  // This will avoid conflicts with the universal watchdog
+  /* Make sure the Watchdog Timer Architectural Protocol has not been installed
+     in the system yet.
+     This will avoid conflicts with the universal watchdog */
 
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiWatchdogTimerArchProtocolGuid);
 
@@ -314,8 +307,11 @@ GenericWatchdogEntry (
 
   // Register for an ExitBootServicesEvent
   Status = gBS->CreateEvent (
-                  EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY,
-                  WatchdogExitBootServicesEvent, NULL, &EfiExitBootServicesEvent
+                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
+                  TPL_NOTIFY,
+                  WatchdogExitBootServicesEvent,
+                  NULL,
+                  &EfiExitBootServicesEvent
                   );
   if (!EFI_ERROR (Status)) {
     // Install interrupt handler
@@ -326,32 +322,31 @@ GenericWatchdogEntry (
                     );
     if (!EFI_ERROR (Status)) {
       Status = mInterruptProtocol->RegisterInterruptSource (
-                                    mInterruptProtocol,
-                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
-                                    WatchdogInterruptHandler
-                                    );
+                 mInterruptProtocol,
+                 FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+                 WatchdogInterruptHandler
+                 );
       if (!EFI_ERROR (Status)) {
         Status = mInterruptProtocol->SetTriggerType (
-                                    mInterruptProtocol,
-                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
-                                    EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
+                   mInterruptProtocol,
+                   FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+                   EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
+                   );
         if (!EFI_ERROR (Status)) {
           // Install the Timer Architectural Protocol onto a new handle
           Handle = NULL;
           Status = gBS->InstallMultipleProtocolInterfaces (
-                          &Handle,
-                          &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
-                          NULL
-                          );
+                     &Handle,
+                     &gEfiWatchdogTimerArchProtocolGuid,
+                     &gWatchdogTimer,
+                     NULL
+                     );
         }
       }
     }
   }
 
-  if (EFI_ERROR (Status)) {
-    // The watchdog failed to initialize
-    ASSERT (FALSE);
-  }
+  ASSERT_EFI_ERROR (Status);
 
   mNumTimerTicks = 0;
   WatchdogDisable ();
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

Re: [PATCH 5/5] ArmPkg: Tidy up GenericWatchdogDxe.c

[Leif Lindholm]

On Mon, Sep 11, 2017 at 04:23:35PM +0100, evan.lloyd@arm.com wrote:
> From: Evan Lloyd <evan.lloyd@arm.com>
> 
> This cosmetic change has no functional content.
> It adjusts comment oddities, etc, noticed during previous work.
> Specific changes are:
>     Re-order #includes
>     Use ns consistently (always "100ns" not sometimes "100 nS")

Yes, in general I prefer fewer Siemens in my code.

>     Reflow overlength comments
>     Change multiline comments to C style
>     Adjust indent for overlength code lines.
>     Replace explicit test and assert with ASSERT_EFI_ERROR.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>

No objections to any of the changes, but I would be really happy if
this was reordered such that this comes before any patches with
functional changes.
That way git blame/praise output is more helpful.

/
    Leif

> ---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 127 ++++++++++----------
>  1 file changed, 61 insertions(+), 66 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 69844db2e11f51907e6c8bff5c67d27ceb498150..7c4c9ecd4e12d433e222d7d08adf20bda1ff9842 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -29,16 +29,16 @@
>  
>  #include "GenericWatchdog.h"
>  
> -// The number of 100ns periods (the unit of time passed to these functions)
> -// in a second
> +/* The number of 100ns periods (the unit of time passed to these functions)
> +   in a second */
>  #define TIME_UNITS_PER_SECOND 10000000
>  
> -// Tick frequency of the generic timer that is the basis of the generic watchdog
> +// Tick frequency of the generic timer basis of the generic watchdog.
>  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.
> +/* 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. */
>  UINT64 mNumTimerTicks = 0;
>  
>  EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
> @@ -75,8 +75,7 @@ WatchdogDisable (
>    return MmioWrite32 (GENERIC_WDOG_CONTROL_STATUS_REG, GENERIC_WDOG_DISABLED);
>  }
>  
> -/**
> -    On exiting boot services we must make sure the Watchdog Timer
> +/** On exiting boot services we must make sure the Watchdog Timer
>      is stopped.
>  **/
>  VOID
> @@ -90,9 +89,8 @@ WatchdogExitBootServicesEvent (
>    mNumTimerTicks = 0;
>  }
>  
> -/*
> -  This function is called when the watchdog's first signal (WS0) goes high.
> -  It uses the ResetSystem Runtime Service to reset the board.
> +/* This function is called when the watchdog's first signal (WS0) goes high.
> +   It uses the ResetSystem Runtime Service to reset the board.
>  */
>  VOID
>  EFIAPI
> @@ -101,7 +99,7 @@ WatchdogInterruptHandler (
>    IN  EFI_SYSTEM_CONTEXT          SystemContext
>    )
>  {
> -  STATIC CONST CHAR16      ResetString[] = L"The generic watchdog timer ran out.";
> +  STATIC CONST CHAR16 ResetString[]= L"The generic watchdog timer ran out.";
>  
>    WatchdogDisable ();
>  
> @@ -126,10 +124,10 @@ WatchdogInterruptHandler (
>    then the new handler is registered and EFI_SUCCESS is returned.
>    If NotifyFunction is NULL, and a handler is already registered,
>    then that handler is unregistered.
> -  If an attempt is made to register a handler when a handler is already registered,
> -  then EFI_ALREADY_STARTED is returned.
> -  If an attempt is made to unregister a handler when a handler is not registered,
> -  then EFI_INVALID_PARAMETER is returned.
> +  If an attempt is made to register a handler when a handler is already
> +  registered, then EFI_ALREADY_STARTED is returned.
> +  If an attempt is made to unregister a handler when a handler is not
> +  registered, then EFI_INVALID_PARAMETER is returned.
>  
>    @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
>    @param  NotifyFunction   The function to call when a timer interrupt fires.
> @@ -139,11 +137,7 @@ WatchdogInterruptHandler (
>                             information is used to signal timer based events.
>                             NULL will unregister the handler.
>  
> -  @retval EFI_SUCCESS           The watchdog timer handler was registered.
> -  @retval EFI_ALREADY_STARTED   NotifyFunction is not NULL, and a handler is already
> -                                registered.
> -  @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler was not
> -                                previously registered.
> +  @retval EFI_UNSUPPORTED       The code does not support NotifyFunction.
>  
>  **/
>  EFI_STATUS
> @@ -160,18 +154,18 @@ WatchdogRegisterHandler (
>  
>  /**
>    This function sets the amount of time to wait before firing the watchdog
> -  timer to TimerPeriod 100 nS units.  If TimerPeriod is 0, then the watchdog
> +  timer to TimerPeriod 100ns units.  If TimerPeriod is 0, then the watchdog
>    timer is disabled.
>  
>    @param  This             The EFI_WATCHDOG_TIMER_ARCH_PROTOCOL instance.
> -  @param  TimerPeriod      The amount of time in 100 nS units to wait before the watchdog
> -                           timer is fired. If TimerPeriod is zero, then the watchdog
> -                           timer is disabled.
> +  @param  TimerPeriod      The amount of time in 100ns units to wait before
> +                           the watchdog timer is fired. If TimerPeriod is zero,
> +                           then the watchdog timer is disabled.
>  
> -  @retval EFI_SUCCESS           The watchdog timer has been programmed to fire in Time
> -                                100 nS units.
> -  @retval EFI_DEVICE_ERROR      A watchdog timer could not be programmed due to a device
> -                                error.
> +  @retval EFI_SUCCESS           The watchdog timer has been programmed to fire
> +                                in Time  100ns units.
> +  @retval EFI_DEVICE_ERROR      A watchdog timer could not be programmed due
> +                                to a device error.
>  
>  **/
>  EFI_STATUS
> @@ -184,7 +178,7 @@ WatchdogSetTimerPeriod (
>    UINTN       SystemCount;
>    EFI_STATUS  Status;
>  
> -  // if TimerPerdiod is 0, this is a request to stop the watchdog.
> +  // if TimerPeriod is 0, this is a request to stop the watchdog.
>    if (TimerPeriod == 0) {
>      mNumTimerTicks = 0;
>      return WatchdogDisable ();
> @@ -193,17 +187,16 @@ WatchdogSetTimerPeriod (
>    // Work out how many timer ticks will equate to TimerPeriod
>    mNumTimerTicks = (mTimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND;
>  
> -
> -  // If the number of required ticks is greater than the max number the
> -  // watchdog's offset register (WOR) can hold, we need to manually compute and
> -  // set the compare register (WCV)
> +  /* 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) {
>  
> -    // 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.
> +    /* 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. */
>  
>      Status = WatchdogWriteOffsetRegister (MAX_UINT32);
>      if (EFI_ERROR (Status)) {
> @@ -221,14 +214,14 @@ WatchdogSetTimerPeriod (
>  }
>  
>  /**
> -  This function retrieves the period of timer interrupts in 100 ns units,
> +  This function retrieves the period of timer interrupts in 100ns units,
>    returns that value in TimerPeriod, and returns EFI_SUCCESS.  If TimerPeriod
>    is NULL, then EFI_INVALID_PARAMETER is returned.  If a TimerPeriod of 0 is
>    returned, then the timer is currently disabled.
>  
>    @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
> -  @param  TimerPeriod      A pointer to the timer period to retrieve in 100
> -                           ns units. If 0 is returned, then the timer is
> +  @param  TimerPeriod      A pointer to the timer period to retrieve in
> +                           100ns units. If 0 is returned, then the timer is
>                             currently disabled.
>  
>  
> @@ -275,19 +268,19 @@ WatchdogGetTimerPeriod (
>          this function will not have any chance of executing.
>  
>    @param SetTimerPeriod
> -  Sets the period of the timer interrupt in 100 nS units.
> +  Sets the period of the timer interrupt in 100ns units.
>    This function is optional, and may return EFI_UNSUPPORTED.
>    If this function is supported, then the timer period will
>    be rounded up to the nearest supported timer period.
>  
>    @param GetTimerPeriod
> -  Retrieves the period of the timer interrupt in 100 nS units.
> +  Retrieves the period of the timer interrupt in 100ns units.
>  
>  **/
>  EFI_WATCHDOG_TIMER_ARCH_PROTOCOL    gWatchdogTimer = {
> -  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER) WatchdogRegisterHandler,
> -  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD) WatchdogSetTimerPeriod,
> -  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD) WatchdogGetTimerPeriod
> +  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER)WatchdogRegisterHandler,
> +  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD)WatchdogSetTimerPeriod,
> +  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD)WatchdogGetTimerPeriod
>  };
>  
>  EFI_EVENT                           EfiExitBootServicesEvent = (EFI_EVENT)NULL;
> @@ -303,9 +296,9 @@ GenericWatchdogEntry (
>    EFI_HANDLE                      Handle;
>  
>  
> -  // Make sure the Watchdog Timer Architectural Protocol has not been installed
> -  // in the system yet.
> -  // This will avoid conflicts with the universal watchdog
> +  /* Make sure the Watchdog Timer Architectural Protocol has not been installed
> +     in the system yet.
> +     This will avoid conflicts with the universal watchdog */
>  
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiWatchdogTimerArchProtocolGuid);
>  
> @@ -314,8 +307,11 @@ GenericWatchdogEntry (
>  
>    // Register for an ExitBootServicesEvent
>    Status = gBS->CreateEvent (
> -                  EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY,
> -                  WatchdogExitBootServicesEvent, NULL, &EfiExitBootServicesEvent
> +                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +                  TPL_NOTIFY,
> +                  WatchdogExitBootServicesEvent,
> +                  NULL,
> +                  &EfiExitBootServicesEvent
>                    );
>    if (!EFI_ERROR (Status)) {
>      // Install interrupt handler
> @@ -326,32 +322,31 @@ GenericWatchdogEntry (
>                      );
>      if (!EFI_ERROR (Status)) {
>        Status = mInterruptProtocol->RegisterInterruptSource (
> -                                    mInterruptProtocol,
> -                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> -                                    WatchdogInterruptHandler
> -                                    );
> +                 mInterruptProtocol,
> +                 FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> +                 WatchdogInterruptHandler
> +                 );
>        if (!EFI_ERROR (Status)) {
>          Status = mInterruptProtocol->SetTriggerType (
> -                                    mInterruptProtocol,
> -                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> -                                    EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
> +                   mInterruptProtocol,
> +                   FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> +                   EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> +                   );
>          if (!EFI_ERROR (Status)) {
>            // Install the Timer Architectural Protocol onto a new handle
>            Handle = NULL;
>            Status = gBS->InstallMultipleProtocolInterfaces (
> -                          &Handle,
> -                          &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
> -                          NULL
> -                          );
> +                     &Handle,
> +                     &gEfiWatchdogTimerArchProtocolGuid,
> +                     &gWatchdogTimer,
> +                     NULL
> +                     );
>          }
>        }
>      }
>    }
>  
> -  if (EFI_ERROR (Status)) {
> -    // The watchdog failed to initialize
> -    ASSERT (FALSE);
> -  }
> +  ASSERT_EFI_ERROR (Status);
>  
>    mNumTimerTicks = 0;
>    WatchdogDisable ();
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 

[PATCH 0/5] HardwareInterrupt2 protocol

[evan.lloyd]

From: Evan Lloyd <evan.lloyd@arm.com>

This v2 series of patches corrects a problem detected on the ARM Juno
platform that is actually generic (at least to ARM GIC platforms).
The HardwareInterrupt protocol had no means of handling characteristics
like Edge/Level triggered and polarity.

A new HardwareInterrupt2 protocol is added, and code changed to utilise
the new capabilities.

The code is available for examination on Github at:
    https://github.com/EvanLloyd/tianocore/tree/376_irqtype_v2

Note: Significant defects exist in the (original) Watchdog handling,
      and a new patch will follow shortly.

Ard Biesheuvel (3):
  EmbeddedPkg: Introduce HardwareInterrupt2 protocol
  ArmPkg/ArmGicDxe: Expose HardwareInterrupt2 protocol
  ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type

Evan Lloyd (2):
  ArmPkg: Tidy GIC code before changes.
  ArmPkg: Tidy up GenericWatchdogDxe.c

 EmbeddedPkg/EmbeddedPkg.dec                              |   1 +
 ArmPkg/Drivers/ArmGic/ArmGicDxe.inf                      |   3 +-
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |   6 +-
 ArmPkg/Drivers/ArmGic/ArmGicDxe.h                        |  37 +++-
 ArmPkg/Include/Library/ArmGicLib.h                       |  39 ++--
 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h        | 182 +++++++++++++++++
 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c                  |  80 ++++++--
 ArmPkg/Drivers/ArmGic/ArmGicLib.c                        |  72 +++++--
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c                | 192 +++++++++++++++--
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c                | 215 +++++++++++++++++---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 151 +++++++-------
 11 files changed, 798 insertions(+), 180 deletions(-)
 create mode 100644 EmbeddedPkg/Include/Protocol/HardwareInterrupt2.h

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

[PATCH 4/5] ArmPkg/GenericWatchdogDxe: Set Watchdog interrupt type

[evan.lloyd]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Utilise the new HardwareInterrupt2 protocol to adjust the
Edge/Level characteristics of the Watchdog interrupt.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Girish Pathak <girish.pathak@arm.com>
Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
Tested-by: Girish Pathak <girish.pathak@arm.com>
---

Notes:
    v2:
    - corrected spelling [Leif]
    - changed include order [Leif]
    - fixed indentation [Leif]

 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf |  6 ++---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c   | 28 ++++++++++++--------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
index fece14cc18315cd15510680c438288687b60c018..ba0403d7fdc3589803c643c27a44918e73afa97e 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
+#  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
 #
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD License
@@ -47,7 +47,7 @@ [Pcd.common]
 
 [Protocols]
   gEfiWatchdogTimerArchProtocolGuid
-  gHardwareInterruptProtocolGuid
+  gHardwareInterrupt2ProtocolGuid
 
 [Depex]
-  gHardwareInterruptProtocolGuid
+  gHardwareInterrupt2ProtocolGuid
diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 6d102e25047253048ac555d6fb5de7223d78f381..69844db2e11f51907e6c8bff5c67d27ceb498150 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
+*  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD
@@ -24,8 +24,8 @@
 #include <Library/UefiLib.h>
 #include <Library/ArmGenericTimerCounterLib.h>
 
+#include <Protocol/HardwareInterrupt2.h>
 #include <Protocol/WatchdogTimer.h>
-#include <Protocol/HardwareInterrupt.h>
 
 #include "GenericWatchdog.h"
 
@@ -41,7 +41,7 @@ UINTN mTimerFrequencyHz = 0;
 // It is therefore stored here. 0 means the timer is not running.
 UINT64 mNumTimerTicks = 0;
 
-EFI_HARDWARE_INTERRUPT_PROTOCOL *mInterruptProtocol;
+EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
 
 EFI_STATUS
 WatchdogWriteOffsetRegister (
@@ -320,7 +320,7 @@ GenericWatchdogEntry (
   if (!EFI_ERROR (Status)) {
     // Install interrupt handler
     Status = gBS->LocateProtocol (
-                    &gHardwareInterruptProtocolGuid,
+                    &gHardwareInterrupt2ProtocolGuid,
                     NULL,
                     (VOID **)&mInterruptProtocol
                     );
@@ -331,13 +331,19 @@ GenericWatchdogEntry (
                                     WatchdogInterruptHandler
                                     );
       if (!EFI_ERROR (Status)) {
-        // Install the Timer Architectural Protocol onto a new handle
-        Handle = NULL;
-        Status = gBS->InstallMultipleProtocolInterfaces (
-                        &Handle,
-                        &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
-                        NULL
-                        );
+        Status = mInterruptProtocol->SetTriggerType (
+                                    mInterruptProtocol,
+                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
+                                    EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
+        if (!EFI_ERROR (Status)) {
+          // Install the Timer Architectural Protocol onto a new handle
+          Handle = NULL;
+          Status = gBS->InstallMultipleProtocolInterfaces (
+                          &Handle,
+                          &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
+                          NULL
+                          );
+        }
       }
     }
   }
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")