On 31 July 2015 at 13:28, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
> On 31 July 2015 at 14:21, Ryan Harkin <ryan.har...@linaro.org> wrote: > > > > > > On 31 July 2015 at 13:13, Ard Biesheuvel <ard.biesheu...@linaro.org> > wrote: > >> > >> On 31 July 2015 at 14:06, Ryan Harkin <ryan.har...@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.har...@linaro.org> > >> > --- > >> > .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 26 > >> > +++++++++++++++------- > >> > ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 24 > >> > ++++++++++++++------ > >> > 2 files changed, 35 insertions(+), 15 deletions(-) > >> > > >> > diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c > >> > b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c > >> > index 9853c7b..b092e3a 100644 > >> > --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c > >> > +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c > >> > @@ -15,7 +15,22 @@ > >> > #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; > >> > + > >> > +RETURN_STATUS > >> > +EFIAPI > >> > +ArmGicArchLibHasGicv3 ( > >> > + 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 > >> > +} > >> > > >> > >> Officially, the bit indicates whether the GIC system register > >> interface is supported at the current exception level, and from that > >> we infer that the system most likely has a GICv3. > >> So if we don't have the SRE enabled, we try to drive it as a GICv2 > >> which only happens to work if this particular GICv3 has the GICv2 > >> compatibility mode implemented. (This is also mentioned in the > >> comments) > >> > >> So the function name is a bit misleading here, although I won't insist > >> that you change it. > > > > > > I'm happy to change it if you want to recommend a better name, this > seems a > > but unwieldy: > > > > HasGicv3 -> GicSystemRegistersSupported > > > > ... but is perhaps more correct. > > > > Well, the function and the caller are so close together that you > shouldn't be able to miss the comment. > I do recommend STATIC for the function though. > Sure, good point. I'll add it for v2. > > >> > >> > >> Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org> > > > > > > Thanks for the quick review turnaround :-) > > Too quick, as it seems. Spotted an inadvertent white space change below :-) > Drat, I would have got away with it if it wasn't for you meddling kids :-) > > >> > >> > >> > RETURN_STATUS > >> > EFIAPI > >> > @@ -31,7 +46,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 (ArmGicArchLibHasGicv3()) { > >> > // 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 > >> > @@ -41,18 +56,13 @@ ArmGicArchLibInitialize ( > >> > // It is the OS responsibility to set this bit. > >> > IccSre = ArmGicV3GetControlSystemRegisterEnable (); > >> > if (!(IccSre & ICC_SRE_EL2_SRE)) { > >> > - ArmGicV3SetControlSystemRegisterEnable (IccSre | > >> > ICC_SRE_EL2_SRE); > >> > + ArmGicV3SetControlSystemRegisterEnable (IccSre| > ICC_SRE_EL2_SRE); > > here ^^^ > I sort-of did that on purpose to make the two files identical. The space vanished when the original copy/paste/hack operation happened and it crept back in when I made the Arm and Aarch64 files identical. I know it's not important, but if I omit that whitespace change, then the two files won't actually be identical before moving to a single common source. Now, do you *really* want me to drop this hunk for v2?? > >> > IccSre = ArmGicV3GetControlSystemRegisterEnable (); > >> > } > >> > 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..b092e3a 100644 > >> > --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c > >> > +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c > >> > @@ -15,7 +15,22 @@ > >> > #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; > >> > + > >> > +RETURN_STATUS > >> > +EFIAPI > >> > +ArmGicArchLibHasGicv3 ( > >> > + 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 +46,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 (ArmGicArchLibHasGicv3()) { > >> > // 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 +61,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