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")
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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")
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
...
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.