The Arm and Aarch64 source files for ArmGicArchLib are copy/paste of each other with one minor difference to how they check for if the platform has GICv3.
I made the change in two patches so that the diff could be identified separately from the move commit.
The first patch makes both the Arm and Aarch64 versions identical and uses conditional compilation to handle Arm vs Aarch64.
[PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3
The second patch then removes the duplication in favour of a single source file.
[PATCH v2 2/2] ArmPkg/ArmGicArchLib: use common source file
Changes since v1 - rename new function from ArmGicArchLibHasGicv3 to GicSystemRegistersSupported - make new function STATIC - remove whitespace change - keep the version with the space before and after '|'
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 +}
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; }
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.
We usually isolate the code to a file that is processor specific, and use the INF file to point at the correct one.
Here is an example: https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/B... https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
[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
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
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.
Here is an example: https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/B...
[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
On Jul 31, 2015, at 9:37 AM, Ryan Harkin ryan.harkin@linaro.org wrote:
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.
The previous code was a bad porting job. it should have been:
[Sources] CommonCode.c
[Sources.ARM] Arm/ArmSpecific.c
[Sources.AARCH64] AArch64/AArch64Specific.c
Although, I could split the only GicSystemRegistersSupported function into it's own arch specific files to fit within that regime.
This is a good form as porting to a new processor architecture is quite easy. The process specific code is in a single file.
The #ifdefs, and possible including processor specific includes can get really unwieldy in the general edk2 case that supports 6 CPU architectures. This form also makes porting to new processor architectures a lot easier.
Seems over the top to me. So unless it's a hard requirement, I'd resist it.
Well the original issue was assumptions about what would happen in the code moving forward. What happens if some SOC comes along and uses a GIC for another CPU arch, or UEFI adds another CPU architecture?
I’m kind of 50/50 on this specific case. I would definitely reject if it was generic code. But for me if its 50/50 I vote against the code with #ifdefs.
Thanks,
Andrew Fish
We usually isolate the code to a file that is processor specific, and use the INF file to point at the correct one.
Here is an example: https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/B...
[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
edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
The .inf/source file approach is the way to be consistent with the rest of edk2. I think it would be messy for the rest of edk2 to use .inf binding and for Arm packages to use a combination of #ifdef and .inf techniques. As someone who has ported UEFI to a new processor architecture I appreciate the .inf approach. When you try to build the component for a new architecture and you're missing the source file you I get nice linker errors, a virtual 'todo' list for the port.
There's a right way to do this - refactor to minimize the difference to the smallest possible chunk of code and avoid the temptation to copy and paste the existing function / source file.
Eugene
-----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Andrew Fish Sent: Friday, July 31, 2015 10:54 AM To: Ryan Harkin ryan.harkin@linaro.org Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List linaro-uefi@lists.linaro.org; Leif Lindholm leif.lindholm@linaro.org; Ard Biesheuvel ard.biesheuvel@linaro.org Subject: Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check for GICv3
On Jul 31, 2015, at 9:37 AM, Ryan Harkin ryan.harkin@linaro.org wrote:
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.
The previous code was a bad porting job. it should have been:
[Sources] CommonCode.c
[Sources.ARM] Arm/ArmSpecific.c
[Sources.AARCH64] AArch64/AArch64Specific.c
Although, I could split the only GicSystemRegistersSupported function into it's own arch specific files to fit within that regime.
This is a good form as porting to a new processor architecture is quite easy. The process specific code is in a single file.
The #ifdefs, and possible including processor specific includes can get really unwieldy in the general edk2 case that supports 6 CPU architectures. This form also makes porting to new processor architectures a lot easier.
Seems over the top to me. So unless it's a hard requirement, I'd resist it.
Well the original issue was assumptions about what would happen in the code moving forward. What happens if some SOC comes along and uses a GIC for another CPU arch, or UEFI adds another CPU architecture?
I’m kind of 50/50 on this specific case. I would definitely reject if it was generic code. But for me if its 50/50 I vote against the code with #ifdefs.
Thanks,
Andrew Fish
We usually isolate the code to a file that is processor specific, and use the INF file to point at the correct one.
Here is an example: https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/B...
[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
edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 31 Jul 2015 21:21, "Cohen, Eugene" eugene@hp.com wrote:
The .inf/source file approach is the way to be consistent with the rest
of edk2. I think it would be messy for the rest of edk2 to use .inf binding and for Arm packages to use a combination of #ifdef and .inf techniques. As someone who has ported UEFI to a new processor architecture I appreciate the .inf approach. When you try to build the component for a new architecture and you're missing the source file you I get nice linker errors, a virtual 'todo' list for the port
I'm not in favour of over engineering code for the sake of it. A #error exactly at the line of code you need to examine is precise. Linker errors are never nice.
If someone wishes to expand this code beyond Arm, they will need to do more than create a simple source file to provide a helper function. The entire package will need reworking.
There's a right way to do this - refactor to minimize the difference to
the smallest possible chunk of code and avoid the temptation to copy and paste the existing function / source file.
That's exactly what my patch achieves.
Eugene
-----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
Andrew Fish
Sent: Friday, July 31, 2015 10:54 AM To: Ryan Harkin ryan.harkin@linaro.org Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List <
linaro-uefi@lists.linaro.org>; Leif Lindholm leif.lindholm@linaro.org; Ard Biesheuvel ard.biesheuvel@linaro.org
Subject: Re: [edk2] [PATCH v2 1/2] ArmPkg/ArmGicLibArchLib: common check
for GICv3
On Jul 31, 2015, at 9:37 AM, Ryan Harkin ryan.harkin@linaro.org wrote:
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.
The previous code was a bad porting job. it should have been:
[Sources] CommonCode.c
[Sources.ARM] Arm/ArmSpecific.c
[Sources.AARCH64] AArch64/AArch64Specific.c
Although, I could split the only GicSystemRegistersSupported function
into
it's own arch specific files to fit within that regime.
This is a good form as porting to a new processor architecture is quite
easy. The process specific code is in a single file.
The #ifdefs, and possible including processor specific includes can get
really unwieldy in the general edk2 case that supports 6 CPU architectures. This form also makes porting to new processor architectures a lot easier.
Seems over the top to me. So unless it's a hard requirement, I'd
resist it.
Well the original issue was assumptions about what would happen in the
code moving forward. What happens if some SOC comes along and uses a GIC for another CPU arch, or UEFI adds another CPU architecture?
I’m kind of 50/50 on this specific case. I would definitely reject if it
was generic code. But for me if its 50/50 I vote against the code with #ifdefs.
Thanks,
Andrew Fish
We usually isolate the code to a file that is processor specific, and
use
the INF file to point at the correct one.
Here is an example:
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/B...
[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
edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Now that the Arm and Aarch64 source files are identical and rely on conditional compilation to provide arch specific code, remove the duplicated files and use one common 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 --- ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 77 ---------------------- .../ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c | 0 ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 7 +- 3 files changed, 2 insertions(+), 82 deletions(-)
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c deleted file mode 100644 index 75ba156..0000000 --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c +++ /dev/null @@ -1,77 +0,0 @@ -/** @file -* -* Copyright (c) 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. -* -**/ - -#include <Library/ArmLib.h> -#include <Library/ArmGicLib.h> - -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 -ArmGicArchLibInitialize ( - VOID - ) -{ - UINT32 IccSre; - - // Ideally we would like to use the GICC IIDR Architecture version here, but - // this does not seem to be very reliable as the implementation could easily - // get it wrong. It is more reliable to check if the GICv3 System Register - // 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 (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 - // to fall back to the GICv2 MMIO interface. - // Note: We do not need to set ICC_SRE_EL2.Enable because the OS is started - // at the same exception level. - // It is the OS responsibility to set this bit. - IccSre = ArmGicV3GetControlSystemRegisterEnable (); - if (!(IccSre & ICC_SRE_EL2_SRE)) { - ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE); - IccSre = ArmGicV3GetControlSystemRegisterEnable (); - } - if (IccSre & ICC_SRE_EL2_SRE) { - mGicArchRevision = ARM_GIC_ARCH_REVISION_3; - } - } - return RETURN_SUCCESS; -} - -ARM_GIC_ARCH_REVISION -EFIAPI -ArmGicGetSupportedArchRevision ( - VOID - ) -{ - return mGicArchRevision; -} diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c similarity index 100% rename from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c rename to ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf index 7dbcb08..5c968e6 100644 --- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf +++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf @@ -20,11 +20,8 @@ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION CONSTRUCTOR = ArmGicArchLibInitialize
-[Sources.ARM] - Arm/ArmGicArchLib.c - -[Sources.AARCH64] - AArch64/ArmGicArchLib.c +[Sources.common] + ArmGicArchLib.c
[Packages] MdePkg/MdePkg.dec
On 31 July 2015 at 15:49, Ryan Harkin ryan.harkin@linaro.org wrote:
Now that the Arm and Aarch64 source files are identical and rely on conditional compilation to provide arch specific code, remove the duplicated files and use one common 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
ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 77
.../ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c | 0 ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 7 +- 3 files changed, 2 insertions(+), 82 deletions(-)
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c deleted file mode 100644 index 75ba156..0000000 --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c +++ /dev/null @@ -1,77 +0,0 @@ -/** @file -* -* Copyright (c) 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. -* -**/
-#include <Library/ArmLib.h> -#include <Library/ArmGicLib.h>
-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 -ArmGicArchLibInitialize (
- VOID
- )
-{
- UINT32 IccSre;
- // Ideally we would like to use the GICC IIDR Architecture version
here, but
- // this does not seem to be very reliable as the implementation could
easily
- // get it wrong. It is more reliable to check if the GICv3 System
Register
- // 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 (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
- // to fall back to the GICv2 MMIO interface.
- // Note: We do not need to set ICC_SRE_EL2.Enable because the OS is
started
- // at the same exception level.
- // It is the OS responsibility to set this bit.
- IccSre = ArmGicV3GetControlSystemRegisterEnable ();
- if (!(IccSre & ICC_SRE_EL2_SRE)) {
ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);
Ach! No, now I need a v3.
Sorry for the noise....
IccSre = ArmGicV3GetControlSystemRegisterEnable ();- }
- if (IccSre & ICC_SRE_EL2_SRE) {
mGicArchRevision = ARM_GIC_ARCH_REVISION_3;- }
- }
- return RETURN_SUCCESS;
-}
-ARM_GIC_ARCH_REVISION -EFIAPI -ArmGicGetSupportedArchRevision (
- VOID
- )
-{
- return mGicArchRevision;
-} diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c similarity index 100% rename from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c rename to ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf index 7dbcb08..5c968e6 100644 --- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf +++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf @@ -20,11 +20,8 @@ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION CONSTRUCTOR = ArmGicArchLibInitialize
-[Sources.ARM]
- Arm/ArmGicArchLib.c
-[Sources.AARCH64]
- AArch64/ArmGicArchLib.c
+[Sources.common]
- ArmGicArchLib.c
[Packages] MdePkg/MdePkg.dec -- 2.1.0
On 31 July 2015 at 15:50, Ryan Harkin ryan.harkin@linaro.org wrote:
On 31 July 2015 at 15:49, Ryan Harkin ryan.harkin@linaro.org wrote:
Now that the Arm and Aarch64 source files are identical and rely on conditional compilation to provide arch specific code, remove the duplicated files and use one common 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
ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 77
.../ArmGicArchLib/{AArch64 => }/ArmGicArchLib.c | 0 ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 7 +- 3 files changed, 2 insertions(+), 82 deletions(-)
diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c deleted file mode 100644 index 75ba156..0000000 --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c +++ /dev/null @@ -1,77 +0,0 @@ -/** @file -* -* Copyright (c) 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. -* -**/
-#include <Library/ArmLib.h> -#include <Library/ArmGicLib.h>
-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 -ArmGicArchLibInitialize (
- VOID
- )
-{
- UINT32 IccSre;
- // Ideally we would like to use the GICC IIDR Architecture version
here, but
- // this does not seem to be very reliable as the implementation could
easily
- // get it wrong. It is more reliable to check if the GICv3 System
Register
- // 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 (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
- // to fall back to the GICv2 MMIO interface.
- // Note: We do not need to set ICC_SRE_EL2.Enable because the OS is
started
- // at the same exception level.
- // It is the OS responsibility to set this bit.
- IccSre = ArmGicV3GetControlSystemRegisterEnable ();
- if (!(IccSre & ICC_SRE_EL2_SRE)) {
ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);Ach! No, now I need a v3.
Sorry for the noise....
False alarm, I'm mis-reading how git send-email -M munged the report and I thought I'd kept the version without the space before the '|'.
v2 is what I expected afterall.
IccSre = ArmGicV3GetControlSystemRegisterEnable ();- }
- if (IccSre & ICC_SRE_EL2_SRE) {
mGicArchRevision = ARM_GIC_ARCH_REVISION_3;- }
- }
- return RETURN_SUCCESS;
-}
-ARM_GIC_ARCH_REVISION -EFIAPI -ArmGicGetSupportedArchRevision (
- VOID
- )
-{
- return mGicArchRevision;
-} diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c similarity index 100% rename from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c rename to ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.c diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf index 7dbcb08..5c968e6 100644 --- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf +++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf @@ -20,11 +20,8 @@ LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION CONSTRUCTOR = ArmGicArchLibInitialize
-[Sources.ARM]
- Arm/ArmGicArchLib.c
-[Sources.AARCH64]
- AArch64/ArmGicArchLib.c
+[Sources.common]
- ArmGicArchLib.c
[Packages] MdePkg/MdePkg.dec -- 2.1.0
-
Andrew Fish -
Cohen, Eugene -
Ryan Harkin