On 04/06/16 18:15, Ard Biesheuvel wrote: > This implements a new DXE driver FdtClientDxe to produce the FDT client > protocol based on a device tree image supplied by the virt host. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> > --- > ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 236 ++++++++++++++++++++ > ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf | 48 ++++ > 2 files changed, 284 insertions(+)
Starting with the INF file: > diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > new file mode 100644 > index 000000000000..55a1e680ce7c > --- /dev/null > +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf > @@ -0,0 +1,48 @@ > +## @file > +# FDT client driver > +# > +# 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. > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = FdtClientDxe > + FILE_GUID = 9A871B00-1C16-4F61-8D2C-93B6654B5AD6 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = InitializeFdtClientDxe > + > +[Sources] > + FdtClientDxe.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + ArmVirtPkg/ArmVirtPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + > +[LibraryClasses] > + BaseLib > + PcdLib > + UefiDriverEntryPoint > + FdtLib > + HobLib > + UefiBootServicesTableLib (5) For new code, please strive to keep the Packages / LibraryClasses / etc sections in INF files sorted. First, it looks nice; second (more importantly), it helps you verify that you list all the library classes explicitly that you depend on. For example, DebugLib.h is included from the source code (correctly), but DebugLib is not present here (it should be). The linker succeeds because one of the library instances we depend on pulls in DebugLib anyway, but it's not nice. Concord can be reached if the C source includes Library/*.h headers until the compiler stops whining (and nothing more), and if the LibraryClasses section is consequently matched to the include list (+ UefiDriverEntryPoint). Similarly, PcdLib looks superfluous. Etc. > + > +[Protocols] > + gFdtClientProtocolGuid (6) Please add "## PRODUCES". > + > +[Guids] > + gFdtHobGuid > + > +[Depex] > + TRUE > C file: > diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > new file mode 100644 > index 000000000000..716194ef798a > --- /dev/null > +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c > @@ -0,0 +1,236 @@ > +/** @file > +* FDT client driver > +* > +* 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 <Library/BaseLib.h> > +#include <Library/DebugLib.h> > +#include <Library/UefiLib.h> (7) Can you remove BaseLib.h and UefiLib.h? Hmmm wait, BaseLib.h is needed for AsciiStrCmp() below. What about UefiLib.h though? > +#include <Library/UefiDriverEntryPoint.h> > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/HobLib.h> > +#include <libfdt.h> > + > +#include <Guid/Fdt.h> (8) <Guid/Fdt.h> should be unnecessary. > +#include <Guid/FdtHob.h> > + > +#include <Protocol/FdtClient.h> > + > +STATIC VOID *mDeviceTreeBase; > + > +STATIC > +EFI_STATUS > +GetNodeProperty ( > + IN FDT_CLIENT_PROTOCOL *This, > + IN INT32 Node, > + IN CONST CHAR8 *PropertyName, > + OUT CONST VOID **Prop, > + OUT UINTN *PropSize OPTIONAL > + ) > +{ > + INT32 Len; > + > + ASSERT (mDeviceTreeBase != NULL); > + > + *Prop = fdt_getprop (mDeviceTreeBase, Node, PropertyName, &Len); > + if (*Prop == NULL) { > + return EFI_NOT_FOUND; > + } > + > + if (PropSize != NULL) { > + *PropSize = Len; (9) I think we can present the property size directly as UINT32 in the protocol interface. > + } > + return EFI_SUCCESS; > +} > + > +STATIC > +EFI_STATUS > +SetNodeProperty ( > + IN FDT_CLIENT_PROTOCOL *This, > + IN INT32 Node, > + IN CONST CHAR8 *PropertyName, > + IN CONST VOID *Prop, > + IN UINTN PropSize > + ) > +{ > + INT32 Ret; > + > + ASSERT (mDeviceTreeBase != NULL); > + > + Ret = fdt_setprop (mDeviceTreeBase, Node, PropertyName, Prop, > (UINT32)PropSize); (10) Again, I think we can expose the property size as UINT32. > + if (Ret != 0) { > + return EFI_DEVICE_ERROR; > + } > + > + return EFI_SUCCESS; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +FindCompatibleNode ( > + IN FDT_CLIENT_PROTOCOL *This, > + IN CONST CHAR8 *CompatibleString, > + OUT INT32 *Node > + ) > +{ > + INT32 NextNode, PrevNode; > + CONST CHAR8 *Type, *Compatible; > + INT32 Len; > + > + ASSERT (mDeviceTreeBase != NULL); > + > + if (Node == NULL) { > + return EFI_INVALID_PARAMETER; > + } (11) I don't mind this, but then we should be consistent about it. In GetNodeProperty(), "Prop" is a mandatory output parameter, but you don't validate it. I think it's okay to drop the check here. > + > + for (PrevNode = 0;; PrevNode = NextNode) { > + NextNode = fdt_next_node (mDeviceTreeBase, PrevNode, NULL); > + if (NextNode < 0) { > + break; > + } > + > + Type = fdt_getprop (mDeviceTreeBase, NextNode, "compatible", &Len); > + if (Type == NULL) { > + continue; > + } > + > + // > + // A 'compatible' node may contain a sequence of NULL terminated (12) I realize this is being copied / reworked from VirtFdtDxe. Let's use the opportunity to clean up things -- please replace NULL with NUL. > + // compatible strings so check each one > + // > + for (Compatible = Type; Compatible < Type + Len && *Compatible; > + Compatible += 1 + AsciiStrLen (Compatible)) { > + if (AsciiStrCmp (CompatibleString, Compatible) == 0) { > + *Node = NextNode; > + return EFI_SUCCESS; > + } > + } > + } > + return EFI_NOT_FOUND; > +} > + > +STATIC > +EFI_STATUS > +EFIAPI > +FindCompatibleNodeProperty ( > + IN FDT_CLIENT_PROTOCOL *This, > + IN CONST CHAR8 *CompatibleString, > + IN CONST CHAR8 *PropertyName, > + OUT CONST VOID **Prop, > + OUT UINTN *PropSize OPTIONAL > + ) > +{ > + EFI_STATUS Status; > + INT32 Node; > + > + Status = FindCompatibleNode (This, CompatibleString, &Node); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + return GetNodeProperty (This, Node, PropertyName, Prop, PropSize); > +} (13) Are we going to use this interface frequently? Hm... grepping the tree in your "virt-fdt-refactor" branch, I can find four call sites. One is just below, another one in ArmVirtPsciResetSystemLib, and the last two in ArmVirtTimerFdtClientLib. I'm torn. This should be a helper function in a library. Meh, don't bother; keep it as-is. We can always rework this protocol if necessary. :) (14) How about renaming this function (and the protocol member) to GetCompatibleNodeProperty()? Just a suggestion, feel free to disagree. > + > +STATIC > +EFI_STATUS > +EFIAPI > +FindCompatibleNodeReg ( > + IN FDT_CLIENT_PROTOCOL *This, > + IN CONST CHAR8 *CompatibleString, > + OUT CONST VOID **Reg, > + OUT UINTN *RegElemSize, > + OUT UINTN *RegSize > + ) > +{ > + EFI_STATUS Status; > + > + // > + // Get the 'reg' property of this node. For now, we will assume > + // 8 byte quantities for base and size, respectively. > + // TODO use #cells root properties instead > + // > + Status = FindCompatibleNodeProperty (This, CompatibleString, "reg", Reg, > RegSize); (15) I haven't been verifying it consistently, but this line seems too long. > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + *RegElemSize = 8; (16) Can you verify that RegSize is divisible by 8? Log an error message and return an error code if it isn't. Also, due to points (9) and (10), you should be able to use the % operator for it (no need for a 64-bit wrapper function). > + > + return EFI_SUCCESS; > +} > + > +STATIC > +EFI_STATUS > +GetChosenNode ( > + IN FDT_CLIENT_PROTOCOL *This, > + OUT INT32 *Node > + ) > +{ > + INT32 NewNode; > + > + ASSERT (mDeviceTreeBase != NULL); > + > + NewNode = fdt_path_offset (mDeviceTreeBase, "/chosen"); > + if (NewNode < 0) { > + NewNode = fdt_add_subnode (mDeviceTreeBase, 0, "/chosen"); > + } > + > + if (NewNode < 0) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + return EFI_SUCCESS; > +} (17) Please rename this function (and the protocol member) to GetOrInsertChosenNode(). In general, it wouldn't be bad if you could document all the protocol member functions (according to the edk2 coding style, with leading comment blocks). I don't insist (the functions are quite small), but without such documentation, the function names become way more important, and should be very clear. (Also, in my experience, writing such function comments leads to "enlightenment" occasionally :) Opportunities for unification etc. I can't point at any candidates in this patch; it's just a generic remark.) > + > +STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = { > + GetNodeProperty, > + SetNodeProperty, > + FindCompatibleNode, > + FindCompatibleNodeProperty, > + FindCompatibleNodeReg, > + GetChosenNode, > +}; > + > +EFI_STATUS > +EFIAPI > +InitializeFdtClientDxe ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + VOID *Hob; > + VOID *DeviceTreeBase; > + > + Hob = GetFirstGuidHob(&gFdtHobGuid); > + if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) { > + return EFI_NOT_FOUND; > + } > + DeviceTreeBase = (VOID *)(UINTN)*(UINT64 *)GET_GUID_HOB_DATA (Hob); > + > + if (fdt_check_header (DeviceTreeBase) != 0) { > + DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n", __FUNCTION__, > DeviceTreeBase)); > + return EFI_NOT_FOUND; > + } (18) Okay, this is copied from InitializeVirtFdtDxe(). Please add a space after "GetFirstGuidHob". > + > + mDeviceTreeBase = DeviceTreeBase; > + > + DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase)); > + > + return gBS->InstallProtocolInterface ( > + &ImageHandle, > + &gFdtClientProtocolGuid, > + EFI_NATIVE_INTERFACE, > + &mFdtClientProtocol > + ); > +} (19) Please indent the arguments and the closing paren one or two colums relative to "InstallProtocolInterface". I'll seek to continue the review tomorrow. Cheers Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
