On 31 July 2015 at 17:16, Andrew Fish <afish@apple.com> wrote:

On Jul 31, 2015, at 7:49 AM, Ryan Harkin <ryan.harkin@linaro.org> wrote:

Make Arm and Aarch64 both use the same code, conditionally compiled, to
check if the platform has GICv3.

Both source files for Arm and Aarch64 are now identical and the next
step is to move to a common source file.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
.../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c  | 25 ++++++++++++++++------
ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c   | 25 ++++++++++++++++------
2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
index 9853c7b..36dbe41 100644
--- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
@@ -15,7 +15,23 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>

-STATIC ARM_GIC_ARCH_REVISION        mGicArchRevision;
+STATIC ARM_GIC_ARCH_REVISION        mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+
+STATIC
+RETURN_STATUS
+EFIAPI
+GicSystemRegistersSupported (
+  VOID
+  )
+{
+#if defined (MDE_CPU_ARM)
+  return (ArmReadIdPfr1 () & ARM_PFR1_GIC);
+#elif defined(MDE_CPU_AARCH64)
+  return (ArmReadIdPfr0 () & AARCH64_PFR0_GIC);
+#else
+ #error "Unknown chipset."
+#endif
+}


We usually don’t use processor specific #ifdef’s in code like this in the edk2.

That's probably why the code is filled with vast swathes of copy/paste/hack-one-line happening all over Arm*Pkg.

Although, I could split the only GicSystemRegistersSupported function into it's own arch specific files to fit within that regime.

Seems over the top to me.  So unless it's a hard requirement, I'd resist it.

 

We usually isolate the code to a file that is processor specific, and use the INF file to point at the correct one. 


[Sources]
  BasePeCoffLibInternals.h
  BasePeCoff.c

[Sources.IA32, Sources.X64, Sources.EBC]
  PeCoffLoaderEx.c

[Sources.IPF]
  Ipf/PeCoffLoaderEx.c

[Sources.ARM]
  Arm/PeCoffLoaderEx.c

[Sources.AARCH64]
  AArch64/PeCoffLoaderEx.c


This is a good example of where the arch specific sources are not copy/paste/hack, but split out for good reason.


Thanks,

Andrew Fish

RETURN_STATUS
EFIAPI
@@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
  // feature is implemented on the CPU. This is also convenient as our GICv3
  // driver requires SRE. If only Memory mapped access is available we try to
  // drive the GIC as a v2.
-  if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) {
+  if (GicSystemRegistersSupported()) {
    // Make sure System Register access is enabled (SRE). This depends on the
    // higher privilege level giving us permission, otherwise we will either
    // cause an exception here, or the write doesn't stick in which case we need
@@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
    }
    if (IccSre & ICC_SRE_EL2_SRE) {
      mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
-      goto Done;
    }
  }
-
-  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
-
-Done:
  return RETURN_SUCCESS;
}

diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
index f8822a2..75ba156 100644
--- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
+++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
@@ -15,7 +15,23 @@
#include <Library/ArmLib.h>
#include <Library/ArmGicLib.h>

-STATIC ARM_GIC_ARCH_REVISION        mGicArchRevision;
+STATIC ARM_GIC_ARCH_REVISION        mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
+
+STATIC
+RETURN_STATUS
+EFIAPI
+GicSystemRegistersSupported (
+  VOID
+  )
+{
+#if defined (MDE_CPU_ARM)
+  return (ArmReadIdPfr1 () & ARM_PFR1_GIC);
+#elif defined(MDE_CPU_AARCH64)
+  return (ArmReadIdPfr0 () & AARCH64_PFR0_GIC);
+#else
+ #error "Unknown chipset."
+#endif
+}

RETURN_STATUS
EFIAPI
@@ -31,7 +47,7 @@ ArmGicArchLibInitialize (
  // feature is implemented on the CPU. This is also convenient as our GICv3
  // driver requires SRE. If only Memory mapped access is available we try to
  // drive the GIC as a v2.
-  if (ArmReadIdPfr1 () & ARM_PFR1_GIC) {
+  if (GicSystemRegistersSupported()) {
    // Make sure System Register access is enabled (SRE). This depends on the
    // higher privilege level giving us permission, otherwise we will either
    // cause an exception here, or the write doesn't stick in which case we need
@@ -46,13 +62,8 @@ ArmGicArchLibInitialize (
    }
    if (IccSre & ICC_SRE_EL2_SRE) {
      mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
-      goto Done;
    }
  }
-
-  mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
-
-Done:
  return RETURN_SUCCESS;
}

--
2.1.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel