* [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit @ 2016-10-17 8:54 Dennis Chen 2016-10-17 17:09 ` Ard Biesheuvel 0 siblings, 1 reply; 5+ messages in thread From: Dennis Chen @ 2016-10-17 8:54 UTC (permalink / raw) To: edk2-devel; +Cc: nd, Dennis Chen, Ard Biesheuvel, Leif Lindholm 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 <ard.biesheuvel@linaro.org> Cc: Leif Lindholm <leif.lindholm@linaro.org> Signed-off-by: Dennis Chen <dennis.chen@arm.com> --- 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit 2016-10-17 8:54 [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit Dennis Chen @ 2016-10-17 17:09 ` Ard Biesheuvel 2016-10-18 2:50 ` Dennis Chen 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2016-10-17 17:09 UTC (permalink / raw) To: Dennis Chen; +Cc: edk2-devel-01, nd, Leif Lindholm On 17 October 2016 at 09:54, Dennis Chen <dennis.chen@arm.com> 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 <ard.biesheuvel@linaro.org> > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit 2016-10-17 17:09 ` Ard Biesheuvel @ 2016-10-18 2:50 ` Dennis Chen 2016-10-18 8:40 ` Ard Biesheuvel 0 siblings, 1 reply; 5+ messages in thread From: Dennis Chen @ 2016-10-18 2:50 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel-01, nd, Leif Lindholm Hello Ard, On Mon, Oct 17, 2016 at 06:09:00PM +0100, Ard Biesheuvel wrote: > On 17 October 2016 at 09:54, Dennis Chen <dennis.chen@arm.com> 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 <ard.biesheuvel@linaro.org> > > Cc: Leif Lindholm <leif.lindholm@linaro.org> > > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > > 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? > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit 2016-10-18 2:50 ` Dennis Chen @ 2016-10-18 8:40 ` Ard Biesheuvel 2016-10-18 9:26 ` Dennis Chen 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2016-10-18 8:40 UTC (permalink / raw) To: Dennis Chen; +Cc: edk2-devel-01, nd, Leif Lindholm On 18 October 2016 at 03:50, Dennis Chen <dennis.chen@arm.com> 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 <dennis.chen@arm.com> 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 <ard.biesheuvel@linaro.org> >> > Cc: Leif Lindholm <leif.lindholm@linaro.org> >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> >> >> 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit 2016-10-18 8:40 ` Ard Biesheuvel @ 2016-10-18 9:26 ` Dennis Chen 0 siblings, 0 replies; 5+ messages in thread From: Dennis Chen @ 2016-10-18 9:26 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel-01, nd, Leif Lindholm Hello Ard, On Tue, Oct 18, 2016 at 09:40:42AM +0100, Ard Biesheuvel wrote: > On 18 October 2016 at 03:50, Dennis Chen <dennis.chen@arm.com> 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 <dennis.chen@arm.com> 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 <ard.biesheuvel@linaro.org> > >> > Cc: Leif Lindholm <leif.lindholm@linaro.org> > >> > Signed-off-by: Dennis Chen <dennis.chen@arm.com> > >> > >> 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. > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-10-18 9:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-17 8:54 [PATCH] ArmPkg: Fix the GIC base address variables as 64-bit Dennis Chen 2016-10-17 17:09 ` Ard Biesheuvel 2016-10-18 2:50 ` Dennis Chen 2016-10-18 8:40 ` Ard Biesheuvel 2016-10-18 9:26 ` Dennis Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox