[edk2] [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit
Since ACPI spec defines the GIC base addresses (CPU interface, Distributor and Redistributor*GICv3 only*) as 64-bit, so we should define these corresponding base address variables as 64-bit instead of 32-bit. This patch redefines them according to the ACPI spec. Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Ard Biesheuvel Cc: Leif Lindholm Signed-off-by: Dennis Chen --- ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c index b9ecd55..c7c5af1 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -30,8 +30,8 @@ Abstract: extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol; -STATIC UINT32 mGicInterruptInterfaceBase; -STATIC UINT32 mGicDistributorBase; +STATIC UINT64 mGicInterruptInterfaceBase; +STATIC UINT64 mGicDistributorBase; /** Enable interrupt source Source. -- 2.7.4 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit
On 17 October 2016 at 09:54, Dennis Chen wrote: > Since ACPI spec defines the GIC base addresses (CPU interface, > Distributor and Redistributor*GICv3 only*) as 64-bit, so we should > define these corresponding base address variables as 64-bit instead of > 32-bit. This patch redefines them according to the ACPI spec. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Cc: Ard Biesheuvel > Cc: Leif Lindholm > Signed-off-by: Dennis Chen After a closer look, I noticed the following: ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicInterruptInterfaceBase ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicInterruptInterfaceBase ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicDistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicInterruptInterfaceBase ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicInterruptInterfaceBase ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicInterruptInterfaceBase, ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicInterruptInterfaceBase, ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicInterruptInterfaceBase, ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicDistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicRedistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicDistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicRedistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicDistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicRedistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicInterruptInterfaceBase ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicInterruptInterfaceBase ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicInterruptInterfaceBase ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicInterruptInterfaceBase ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicInterruptInterfaceBase, so I think we need to clean up the use of these values more widely than we have done up till now Leif: I was wondering if EFI_PHYSICAL_ADDRESS would be more appropriate for MMIO base addresses? ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit
Hello Ard, On Mon, Oct 17, 2016 at 06:09:00PM +0100, Ard Biesheuvel wrote: > On 17 October 2016 at 09:54, Dennis Chen wrote: > > Since ACPI spec defines the GIC base addresses (CPU interface, > > Distributor and Redistributor*GICv3 only*) as 64-bit, so we should > > define these corresponding base address variables as 64-bit instead of > > 32-bit. This patch redefines them according to the ACPI spec. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Cc: Ard Biesheuvel > > Cc: Leif Lindholm > > Signed-off-by: Dennis Chen > > After a closer look, I noticed the following: > > ArmPkg/Include/Library/ArmGicLib.h: IN INTN > GicInterruptInterfaceBase > ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN INTN > GicInterruptInterfaceBase > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicDistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN INTN > GicInterruptInterfaceBase > ArmPkg/Include/Library/ArmGicLib.h: IN INTN > GicInterruptInterfaceBase > ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase > ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase > ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase > ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicInterruptInterfaceBase, > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicInterruptInterfaceBase, > ArmPkg/Include/Library/ArmGicLib.h: IN INTN > GicInterruptInterfaceBase, > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicDistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicRedistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicDistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicRedistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicDistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicRedistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, > ArmPkg/Include/Library/ArmGicLib.h: IN INTN > GicInterruptInterfaceBase > ArmPkg/Include/Library/ArmGicLib.h: IN INTN > GicInterruptInterfaceBase > ArmPkg/Include/Library/ArmGicLib.h: IN INTN > GicInterruptInterfaceBase > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicInterruptInterfaceBase > ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > GicInterruptInterfaceBase, > > so I think we need to clean up the use of these values more widely > than we have done up till now > I am not very sure if we still need to support UEFI on 32-bit ARM platform, as Leif mentioned if we use INTN or UINTN that will be more generic to embrace both 32 &64-bit platform, at least in form of. Currently we are only focused on 64-bit platform, let's wait for Leif's comment then I can re-work my patch to adapt it after we have reached a wider agreement. Thanks, Dennis > > Leif: I was wondering if EFI_PHYSICAL_ADDRESS would be more > appropriate for MMIO base addresses? > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit
On 18 October 2016 at 03:50, Dennis Chen wrote: > Hello Ard, > > On Mon, Oct 17, 2016 at 06:09:00PM +0100, Ard Biesheuvel wrote: >> On 17 October 2016 at 09:54, Dennis Chen wrote: >> > Since ACPI spec defines the GIC base addresses (CPU interface, >> > Distributor and Redistributor*GICv3 only*) as 64-bit, so we should >> > define these corresponding base address variables as 64-bit instead of >> > 32-bit. This patch redefines them according to the ACPI spec. >> > >> > Contributed-under: TianoCore Contribution Agreement 1.0 >> > Cc: Ard Biesheuvel >> > Cc: Leif Lindholm >> > Signed-off-by: Dennis Chen >> >> After a closer look, I noticed the following: >> >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN >> GicInterruptInterfaceBase >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN >> GicInterruptInterfaceBase >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicDistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN >> GicInterruptInterfaceBase >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN >> GicInterruptInterfaceBase >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicInterruptInterfaceBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicInterruptInterfaceBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN >> GicInterruptInterfaceBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicDistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicRedistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicDistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicRedistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicDistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicRedistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN >> GicInterruptInterfaceBase >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN >> GicInterruptInterfaceBase >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN >> GicInterruptInterfaceBase >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicInterruptInterfaceBase >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN >> GicInterruptInterfaceBase, >> >> so I think we need to clean up the use of these values more widely >> than we have done up till now >> > I am not very sure if we still need to support UEFI on 32-bit ARM platform, > as Leif mentioned > if we use INTN or UINTN that will be more generic to embrace both 32 &64-bit > platform, at least > in form of. Currently we are only focused on 64-bit platform, let's wait for > Leif's comment then > I can re-work my patch to adapt it after we have reached a wider agreement. > Hi Dennis, My primary concern is the sloppiness regarding INTN/UINTN, so it seem a major cleanup is due. I'd prefer using UINT64 everywhere: we can still assert that an UINT64 *value* does not exceed MAX_UINTN if we want to -- Ard. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit
Hello Ard, On Tue, Oct 18, 2016 at 09:40:42AM +0100, Ard Biesheuvel wrote: > On 18 October 2016 at 03:50, Dennis Chen wrote: > > Hello Ard, > > > > On Mon, Oct 17, 2016 at 06:09:00PM +0100, Ard Biesheuvel wrote: > >> On 17 October 2016 at 09:54, Dennis Chen wrote: > >> > Since ACPI spec defines the GIC base addresses (CPU interface, > >> > Distributor and Redistributor*GICv3 only*) as 64-bit, so we should > >> > define these corresponding base address variables as 64-bit instead of > >> > 32-bit. This patch redefines them according to the ACPI spec. > >> > > >> > Contributed-under: TianoCore Contribution Agreement 1.0 > >> > Cc: Ard Biesheuvel > >> > Cc: Leif Lindholm > >> > Signed-off-by: Dennis Chen > >> > >> After a closer look, I noticed the following: > >> > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN > >> GicInterruptInterfaceBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN > >> GicInterruptInterfaceBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN GicDistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN > >> GicInterruptInterfaceBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN > >> GicInterruptInterfaceBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicInterruptInterfaceBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicInterruptInterfaceBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN > >> GicInterruptInterfaceBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicDistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicRedistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicDistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicRedistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicDistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicRedistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN GicDistributorBase, > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN > >> GicInterruptInterfaceBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN > >> GicInterruptInterfaceBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN INTN > >> GicInterruptInterfaceBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicInterruptInterfaceBase > >> ArmPkg/Include/Library/ArmGicLib.h: IN UINTN > >> GicInterruptInterfaceBase, > >> > >> so I think we need to clean up the use of these values more widely > >> than we have done up till now > >> > > I am not very sure if we still need to support UEFI on 32-bit ARM platform, > > as Leif mentioned > > if we use INTN or UINTN that will be more generic to embrace both 32 > > &64-bit platform, at least > > in form of. Currently we are only focused on 64-bit platform, let's wait > > for Leif's comment then > > I can re-work my patch to adapt it after we have reached a wider agreement. > > > > Hi Dennis, > > My primary concern is the sloppiness regarding INTN/UINTN, so it seem > a major cleanup is due. I'd prefer using UINT64 everywhere: we can > still assert that an UINT64 *value* does not exceed MAX_UINTN if we > want to > Indeed, INTN/UINTN usage is very inconsistent in that header file. I suppose you have considered the 32-bit case when you say you prefer using UNIT64 everywhere, thus I have no objection for that since I always work on 64-bit area. I will hold on this patch for a while and resend the revised patch if no more comments inputted. Thanks, Dennis > > -- > Ard. > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
