On 29 July 2015 at 11:27, Ryan Harkin <[email protected]> wrote: > This list moves too fast for me to keep up with changes before they are > committed... :-( But still... >
Well, these have been on the list for a while, but I never cc'ed you the first time around. But thanks for taking a look anyway ... > On 26 July 2015 at 10:40, Ard Biesheuvel <[email protected]> wrote: >> >> Instead of inferring the GIC revision from the CPU id registers >> and the presence/availability of the system register interface >> upon each invocation, move the logic to a constructor and cache >> the result. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <[email protected]> >> Reviewed-by: Laszlo Ersek <[email protected]> >> >> --- >> ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 23 >> ++++++++++++++++---- >> ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 23 >> ++++++++++++++++---- >> ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 3 ++- >> 3 files changed, 40 insertions(+), 9 deletions(-) >> >> diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c >> b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c >> index 0e0fa3b9f33e..9853c7ba8566 100644 >> --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c >> +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c >> @@ -15,9 +15,11 @@ >> #include <Library/ArmLib.h> >> #include <Library/ArmGicLib.h> >> >> -ARM_GIC_ARCH_REVISION >> +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision; >> + >> +RETURN_STATUS >> EFIAPI >> -ArmGicGetSupportedArchRevision ( >> +ArmGicArchLibInitialize ( >> VOID >> ) >> { >> @@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision ( >> IccSre = ArmGicV3GetControlSystemRegisterEnable (); >> } >> if (IccSre & ICC_SRE_EL2_SRE) { >> - return ARM_GIC_ARCH_REVISION_3; >> + mGicArchRevision = ARM_GIC_ARCH_REVISION_3; >> + goto Done; > > > Any reason why you used a goto here instead of an else? > To avoid having two 'else' branches with the same 'mGicArchRevision = ARM_GIC_ARCH_REVISION_2' assignment. Perhaps just returning right there would have been even better. > >> } >> } >> >> - return ARM_GIC_ARCH_REVISION_2; >> + mGicArchRevision = ARM_GIC_ARCH_REVISION_2; >> + >> +Done: >> + return RETURN_SUCCESS; >> +} >> + >> +ARM_GIC_ARCH_REVISION >> +EFIAPI >> +ArmGicGetSupportedArchRevision ( >> + VOID >> + ) >> +{ >> + return mGicArchRevision; >> } >> diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c >> b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c >> index f256de704631..f8822a224580 100644 >> --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c >> +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c >> @@ -15,9 +15,11 @@ >> #include <Library/ArmLib.h> >> #include <Library/ArmGicLib.h> >> >> -ARM_GIC_ARCH_REVISION >> +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision; >> + >> +RETURN_STATUS >> EFIAPI >> -ArmGicGetSupportedArchRevision ( >> +ArmGicArchLibInitialize ( >> VOID >> ) >> { >> @@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision ( >> IccSre = ArmGicV3GetControlSystemRegisterEnable (); >> } >> if (IccSre & ICC_SRE_EL2_SRE) { >> - return ARM_GIC_ARCH_REVISION_3; >> + mGicArchRevision = ARM_GIC_ARCH_REVISION_3; >> + goto Done; > > > Ugh. More duplicated code. Not your fault, but just something to me > remember for the next round of code deletion... > Yes, but not quite. The ID register and the compare value are different. There is no abstraction yet for CPU feature checks that live in different registers on 32 vs 64 bit, and adding one just for this use case seems overkill imo -- Ard. >> >> } >> } >> >> - return ARM_GIC_ARCH_REVISION_2; >> + mGicArchRevision = ARM_GIC_ARCH_REVISION_2; >> + >> +Done: >> + return RETURN_SUCCESS; >> +} >> + >> +ARM_GIC_ARCH_REVISION >> +EFIAPI >> +ArmGicGetSupportedArchRevision ( >> + VOID >> + ) >> +{ >> + return mGicArchRevision; >> } >> diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf >> b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf >> index d71b2adc3027..7dbcb08f50d6 100644 >> --- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf >> +++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf >> @@ -17,7 +17,8 @@ [Defines] >> FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a >> MODULE_TYPE = BASE >> VERSION_STRING = 1.0 >> - LIBRARY_CLASS = ArmGicArchLib >> + LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER >> UEFI_APPLICATION >> + CONSTRUCTOR = ArmGicArchLibInitialize >> >> [Sources.ARM] >> Arm/ArmGicArchLib.c >> -- >> 1.9.1 >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
