On 05/29/15 14:33, Ard Biesheuvel wrote: > Before splitting off ArmGicArchLib and moving it out of > ArmPkg/Drivers/ArmGic into ArmPkg/Library, make sure that the > GIC specific declarations it depends on are not hidden away in > local headers "GicV2/GicV2Lib.h" and "GicV3/GicV3Lib.h". > > So merge them with <Library/ArmGicLib.h>. This is entirely > appropriate, since this is not a header that declares a public > interface into ArmGicLib, but defines implementation internals.
The code movements in this patch seem okay to me, but I'm unsure about your claim "Library/ArmGicLib.h [...] is not a header that declares a public interface into ArmGicLib". For example, take the function ArmGicSetupNonSecure(). This function is declared in "ArmPkg/Include/Library/ArmGicLib.h", which one could say is the library class itself (and your claim is the opposite). The function is called in "ArmPlatformPkg/Sec/Sec.c" (the library client). The funcion is implemented in "ArmPkg/Drivers/ArmGic/ArmGicSecLib.c", which is built as part of "ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf". That INF file has LIBRARY_CLASS = ArmGicLib. This kinda looks to counter your claim. ... Hm... on the other hand. Functions in a library class header must be implemented by all library instances. And, we have another ArmGicLib instance under ArmPkg/Drivers/ArmGic/ArmGicLib.inf that does not build "ArmGicSecLib.c", therefore it will lack an implementation for ArmGicSetupNonSecure(). In addition, [LibraryClasses.common] in "ArmPkg/ArmPkg.dec" does not list the header file. So I guess you're right, "ArmPkg/Include/Library/ArmGicLib.h" is already a "dumping ground" for declarations of internal-use functions. Reviewed-by: Laszlo Ersek <ler...@redhat.com> Laszlo > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> > --- > ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c | 2 - > ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c | 2 - > ArmPkg/Drivers/ArmGic/ArmGicLib.c | 3 - > ArmPkg/Drivers/ArmGic/ArmGicSecLib.c | 2 - > ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 3 +- > ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h | 54 ------- > ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 3 +- > ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h | 68 -------- > ArmPkg/Include/Library/ArmGicLib.h | 94 ++++++++++++ > 9 files changed, 98 insertions(+), 133 deletions(-) > > diff --git a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c > b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c > index 88fa4621e613..0e0fa3b9f33e 100644 > --- a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c > +++ b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c > @@ -15,8 +15,6 @@ > #include <Library/ArmLib.h> > #include <Library/ArmGicLib.h> > > -#include "GicV3/ArmGicV3Lib.h" > - > ARM_GIC_ARCH_REVISION > EFIAPI > ArmGicGetSupportedArchRevision ( > diff --git a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c > b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c > index 9ef56efeaa7b..f256de704631 100644 > --- a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c > +++ b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c > @@ -15,8 +15,6 @@ > #include <Library/ArmLib.h> > #include <Library/ArmGicLib.h> > > -#include "GicV3/ArmGicV3Lib.h" > - > ARM_GIC_ARCH_REVISION > EFIAPI > ArmGicGetSupportedArchRevision ( > diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c > b/ArmPkg/Drivers/ArmGic/ArmGicLib.c > index 48708e3812b4..248e896c4b94 100644 > --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c > +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c > @@ -19,9 +19,6 @@ > #include <Library/IoLib.h> > #include <Library/PcdLib.h> > > -#include "GicV2/ArmGicV2Lib.h" > -#include "GicV3/ArmGicV3Lib.h" > - > /** > * Return the base address of the GIC redistributor for the current CPU > * > diff --git a/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c > b/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c > index 1fdd4d73bd7d..d64806d2f16b 100644 > --- a/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c > +++ b/ArmPkg/Drivers/ArmGic/ArmGicSecLib.c > @@ -17,8 +17,6 @@ > #include <Library/IoLib.h> > #include <Library/ArmGicLib.h> > > -#include "GicV2/ArmGicV2Lib.h" > - > /* > * This function configures the interrupts set by the mask to be secure. > * > diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > index 743c534e04d9..e649ac1bc6af 100644 > --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > @@ -22,8 +22,9 @@ Abstract: > > --*/ > > +#include <Library/ArmGicLib.h> > + > #include "ArmGicDxe.h" > -#include "GicV2/ArmGicV2Lib.h" > > #define ARM_GIC_DEFAULT_PRIORITY 0x80 > > diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h > b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h > deleted file mode 100644 > index 6803467346a3..000000000000 > --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.h > +++ /dev/null > @@ -1,54 +0,0 @@ > -/** @file > -* > -* Copyright (c) 2013-2014, 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 > -* which accompanies this distribution. The full text of the license may be > found at > -* http://opensource.org/licenses/bsd-license.php > -* > -* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > -* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > -* > -**/ > - > -#ifndef _ARM_GIC_V2_H_ > -#define _ARM_GIC_V2_H_ > - > -// 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 > -ArmGicV2SetupNonSecure ( > - IN UINTN MpId, > - IN INTN GicDistributorBase, > - IN INTN GicInterruptInterfaceBase > - ); > - > -VOID > -EFIAPI > -ArmGicV2EnableInterruptInterface ( > - IN INTN GicInterruptInterfaceBase > - ); > - > -VOID > -EFIAPI > -ArmGicV2DisableInterruptInterface ( > - IN INTN GicInterruptInterfaceBase > - ); > - > -UINTN > -EFIAPI > -ArmGicV2AcknowledgeInterrupt ( > - IN UINTN GicInterruptInterfaceBase > - ); > - > -VOID > -EFIAPI > -ArmGicV2EndOfInterrupt ( > - IN UINTN GicInterruptInterfaceBase, > - IN UINTN Source > - ); > - > -#endif > diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > index 73cac8774056..4afa3d5a09c2 100644 > --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > @@ -12,8 +12,9 @@ > * > **/ > > +#include <Library/ArmGicLib.h> > + > #include "ArmGicDxe.h" > -#include "GicV3/ArmGicV3Lib.h" > > #define ARM_GIC_DEFAULT_PRIORITY 0x80 > > diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h > b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h > deleted file mode 100644 > index 794e8788a60b..000000000000 > --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Lib.h > +++ /dev/null > @@ -1,68 +0,0 @@ > -/** @file > -* > -* Copyright (c) 2014-2015, 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 which accompanies this > -* distribution. The full text of the license may be found at > -* http://opensource.org/licenses/bsd-license.php > -* > -* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > -* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > -* > -**/ > - > -#ifndef _ARM_GIC_V3_H_ > -#define _ARM_GIC_V3_H_ > - > -#define ICC_SRE_EL2_SRE (1 << 0) > - > -#define ARM_GICD_IROUTER_IRM BIT31 > - > -UINT32 > -EFIAPI > -ArmGicV3GetControlSystemRegisterEnable ( > - VOID > - ); > - > -VOID > -EFIAPI > -ArmGicV3SetControlSystemRegisterEnable ( > - IN UINT32 ControlSystemRegisterEnable > - ); > - > -VOID > -EFIAPI > -ArmGicV3EnableInterruptInterface ( > - VOID > - ); > - > -VOID > -EFIAPI > -ArmGicV3DisableInterruptInterface ( > - VOID > - ); > - > -UINTN > -EFIAPI > -ArmGicV3AcknowledgeInterrupt ( > - VOID > - ); > - > -VOID > -EFIAPI > -ArmGicV3EndOfInterrupt ( > - IN UINTN Source > - ); > - > -VOID > -ArmGicV3SetBinaryPointer ( > - IN UINTN BinaryPoint > - ); > - > -VOID > -ArmGicV3SetPriorityMask ( > - IN UINTN Priority > - ); > - > -#endif > diff --git a/ArmPkg/Include/Library/ArmGicLib.h > b/ArmPkg/Include/Library/ArmGicLib.h > index e2a4818c4c0c..e3db9c0d250f 100644 > --- a/ArmPkg/Include/Library/ArmGicLib.h > +++ b/ArmPkg/Include/Library/ArmGicLib.h > @@ -231,4 +231,98 @@ 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)) > + > +VOID > +EFIAPI > +ArmGicV2SetupNonSecure ( > + IN UINTN MpId, > + IN INTN GicDistributorBase, > + IN INTN GicInterruptInterfaceBase > + ); > + > +VOID > +EFIAPI > +ArmGicV2EnableInterruptInterface ( > + IN INTN GicInterruptInterfaceBase > + ); > + > +VOID > +EFIAPI > +ArmGicV2DisableInterruptInterface ( > + IN INTN GicInterruptInterfaceBase > + ); > + > +UINTN > +EFIAPI > +ArmGicV2AcknowledgeInterrupt ( > + IN UINTN GicInterruptInterfaceBase > + ); > + > +VOID > +EFIAPI > +ArmGicV2EndOfInterrupt ( > + IN UINTN GicInterruptInterfaceBase, > + IN UINTN Source > + ); > + > +// > +// GIC revision 3 specific declarations > +// > + > +#define ICC_SRE_EL2_SRE (1 << 0) > + > +#define ARM_GICD_IROUTER_IRM BIT31 > + > +UINT32 > +EFIAPI > +ArmGicV3GetControlSystemRegisterEnable ( > + VOID > + ); > + > +VOID > +EFIAPI > +ArmGicV3SetControlSystemRegisterEnable ( > + IN UINT32 ControlSystemRegisterEnable > + ); > + > +VOID > +EFIAPI > +ArmGicV3EnableInterruptInterface ( > + VOID > + ); > + > +VOID > +EFIAPI > +ArmGicV3DisableInterruptInterface ( > + VOID > + ); > + > +UINTN > +EFIAPI > +ArmGicV3AcknowledgeInterrupt ( > + VOID > + ); > + > +VOID > +EFIAPI > +ArmGicV3EndOfInterrupt ( > + IN UINTN Source > + ); > + > +VOID > +ArmGicV3SetBinaryPointer ( > + IN UINTN BinaryPoint > + ); > + > +VOID > +ArmGicV3SetPriorityMask ( > + IN UINTN Priority > + ); > + > #endif > ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
