On 04/08/16 11:44, Ard Biesheuvel wrote: > This implements a library ArmVirtTimerFdtClientLib which is intended to > be incorporated into TimerDxe via NULL library class resolution. This > allows us to make TimerDxe depend on the FDT client protocol, and > discover the timer interrupts from the device tree directly rather than > relying on VirtFdtDxe to set the dynamic PCDs.
I agree that the four PCDs massaged here are only consumed by "ArmPkg/Drivers/TimerDxe". > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> > --- > ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c | > 86 ++++++++++++++++++++ > ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf | > 45 ++++++++++ > 2 files changed, 131 insertions(+) > > diff --git > a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c > b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c > new file mode 100644 > index 000000000000..08e24fd82be5 > --- /dev/null > +++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c > @@ -0,0 +1,86 @@ > +/** @file > + FDT client library for ARM's TimerDxe > + > + Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR> > + > + 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 <Uefi.h> > + > +#include <Library/BaseLib.h> > +#include <Library/DebugLib.h> > +#include <Library/PcdLib.h> > +#include <Library/UefiBootServicesTableLib.h> Please add UefiBootServicesTableLib to [LibraryClasses]. > + > +#include <Protocol/FdtClient.h> > + > +typedef struct { > + UINT32 Type; > + UINT32 Number; > + UINT32 Flags; > +} INTERRUPT_PROPERTY; > + > +RETURN_STATUS > +EFIAPI > +ArmVirtTimerFdtClientLibConstructor ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + FDT_CLIENT_PROTOCOL *FdtClient; > + CONST INTERRUPT_PROPERTY *InterruptProp; Ah, so it will point directly into the DTB. This is an opportunity to document the source then: please bracket the INTERRUPT_PROPERTY typedef with: #pragma pack (1) ... #pragma pack () > + UINT32 PropSize; > + INT32 SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum; > + > + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, > + (VOID **)&FdtClient); > + ASSERT_EFI_ERROR (Status); This is right, but I'll have a note at the end. > + > + Status = FdtClient->FindCompatibleNodeProperty (FdtClient, > "arm,armv7-timer", > + "interrupts", (CONST VOID **)&InterruptProp, > + &PropSize); > + if (Status == EFI_NOT_FOUND) { > + Status = FdtClient->FindCompatibleNodeProperty (FdtClient, > + "arm,armv8-timer", "interrupts", > + (CONST VOID **)&InterruptProp, > + &PropSize); > + } > + > + if (EFI_ERROR (Status)) { > + return Status; > + } This will again trigger the library construct assertion, but that's okay; I guess we'd be busted without a timer anyway. > + > + // > + // - interrupts : Interrupt list for secure, non-secure, virtual and > + // hypervisor timers, in that order. > + // > + ASSERT (PropSize == 36 || PropSize == 48); > + > + SecIntrNum = SwapBytes32 (InterruptProp[0].Number) > + + (InterruptProp[0].Type ? 16 : 0); > + IntrNum = SwapBytes32 (InterruptProp[1].Number) > + + (InterruptProp[1].Type ? 16 : 0); > + VirtIntrNum = SwapBytes32 (InterruptProp[2].Number) > + + (InterruptProp[2].Type ? 16 : 0); > + HypIntrNum = PropSize < 48 ? 0 : SwapBytes32 (InterruptProp[3].Number) > + + (InterruptProp[3].Type ? 16 : 0); > + > + DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n", > + SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum)); > + > + PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum); > + PcdSet32 (PcdArmArchTimerIntrNum, IntrNum); > + PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum); > + PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum); > + > + return EFI_SUCCESS; > +} > + > diff --git > a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf > b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf > new file mode 100644 > index 000000000000..e54c401b305e > --- /dev/null > +++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf > @@ -0,0 +1,45 @@ > +#/** @file > +# FDT client library for ARM's TimerDxe > +# > +# Copyright (c) 2016, Linaro Ltd. 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. > +# > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = ArmVirtTimerFdtClientLib > + FILE_GUID = 77EA80CA-2EFB-4AB4-8567-230D968F6B37 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = ArmVirtTimerFdtClientLib|DXE_DRIVER > + CONSTRUCTOR = ArmVirtTimerFdtClientLibConstructor > + > +[Sources] > + ArmVirtTimerFdtClientLib.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + ArmVirtPkg/ArmVirtPkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + BaseLib > + DebugLib > + PcdLib > + > +[Protocols] > + gFdtClientProtocolGuid ## CONSUMES > + > +[Pcd] > + gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum > + gArmTokenSpaceGuid.PcdArmArchTimerIntrNum > + gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum > + gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum > Since the stated goal of this library (class & instance alike) is to be linked into TimerDxe, and to imbue the latter with a dependency on the FDT client protocol, please move the DepEx hunk from the next patch into this one. (The DepEx also belongs together with the ASSERT_EFI_ERROR() above.) With the above addressed: Reviewed-by: Laszlo Ersek <ler...@redhat.com> Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
