From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x234.google.com (mail-wm0-x234.google.com [IPv6:2a00:1450:400c:c09::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E81F821DFC89E for ; Thu, 14 Sep 2017 09:38:14 -0700 (PDT) Received: by mail-wm0-x234.google.com with SMTP id e71so2058225wmg.4 for ; Thu, 14 Sep 2017 09:41:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=SmOnKfHrdDPt31z7sWSfql+0UL4suDHTBHTuvHe50Bg=; b=RMi6N39AaPeTOBGpXEwyOtZIQ9VDDXqTmZ2GzDJcTYsY/5yANvvdF9o7rihX+S2i+u QgKK+NStDsrbrzLTjVcWLc2E5e0HsV3CEeOqaRCASJAnmfDbwUF5wsOXe2Vx+2rWU3mL 7vJ50Zn/Uclv/9cZPwqMkYvbGaDzfvZiFABLU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=SmOnKfHrdDPt31z7sWSfql+0UL4suDHTBHTuvHe50Bg=; b=agVRDlNM3odca0uxagush+zJM/Uu6mCoUPsU4QPHAHiLh9+51j51l/HCuSwwE4K3mO YjMAt+qJAmqrDtvYb5HgP/K1MChd6JdpIiRRcrKHf1kJPfpngD2kuTEBkuDQLV44bRlA JnO7Zb6ooz2JOwLm1skaYfo80eSpm0qJXinDGjfS1/sbdYglcJkX/D2BUVp5dYRXsl2T vd95juVslJg2Tt/8t+WPKvz2KoeEugTOVmCvggj+cw9R/GDWTqLdjQh7Jm+r/BNx/9H6 TL2AiJeGH/mAgcQvXSsKPMW54srveulFrL3XzOQ1J95KWI20pOyJFDNtOMNWF8Pv5VSb zH9w== X-Gm-Message-State: AHPjjUivMxrQXwHMQXj8sUTXBPxwnWQudnPMkhc4eplkA7P8KRfZ/H1x JJYdhGMi2PqCYxRw X-Google-Smtp-Source: AOwi7QDYmbOHis4yjKs+bteTvUCG0IU+yCOixJzNS7P+GASxOJE9jnDf4fOmsgAFWcnNniYV+f+RfA== X-Received: by 10.28.155.209 with SMTP id d200mr562279wme.138.1505407272377; Thu, 14 Sep 2017 09:41:12 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id b125sm382261wma.29.2017.09.14.09.41.10 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 14 Sep 2017 09:41:11 -0700 (PDT) Date: Thu, 14 Sep 2017 17:41:09 +0100 From: Leif Lindholm To: evan.lloyd@arm.com Cc: edk2-devel@lists.01.org, Ard Biesheuvel , Matteo Carlini , nd@arm.com Message-ID: <20170914164109.foiwrmrdbm7aftd6@bivouac.eciton.net> References: <20170911152335.72672-1-evan.lloyd@arm.com> <20170911152335.72672-2-evan.lloyd@arm.com> MIME-Version: 1.0 In-Reply-To: <20170911152335.72672-2-evan.lloyd@arm.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Sep 2017 16:38:15 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Sep 11, 2017 at 04:23:31PM +0100, evan.lloyd@arm.com wrote: > From: Evan Lloyd > > 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 > Signed-off-by: Girish Pathak 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 > > -// > + > // 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.
> Portions copyright (c) 2010, Apple Inc. All rights reserved.
> -Portions copyright (c) 2011-2016, ARM Ltd. All rights reserved.
> +Portions copyright (c) 2011-2017, ARM Ltd. All rights reserved.
> > 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") >