On Mon, Sep 11, 2017 at 04:23:31PM +0100, evan.ll...@arm.com wrote:
> From: Evan Lloyd <evan.ll...@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.ll...@arm.com>
> Signed-off-by: Girish Pathak <girish.pat...@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
