[edk2,04/21] ArmVirtPkg/FdtClientDxe: implement new driver

Message ID 1459959319-19293-5-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel April 6, 2016, 4:15 p.m.
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.biesheuvel@linaro.org>

---
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 236 ++++++++++++++++++++
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  48 ++++
 2 files changed, 284 insertions(+)

-- 
2.5.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek April 6, 2016, 8:25 p.m. | #1
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.biesheuvel@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
Ard Biesheuvel April 7, 2016, 7:32 a.m. | #2
On 6 April 2016 at 22:25, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.biesheuvel@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


I don't think minimising the set of #includes should be a goal in
itself, especially since you may end up relying on transitive
includes. A source file should include the header files for all
functionality it pulls in from other objects.
I know we still end up relying on transitive includes for things like
types, and EDK2 with its AutoGen.? may supply some things implicitly
as well. But in general, I don't think removing header #includes
because you can is a good thing.

Other than that, I agree that this should be cleaned up

> (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".

>


OK

>> +

>> +[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?

>


It can be removed

>> +#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.

>


OK

>> +#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.

>


Indeed. Will change that.

>> +  }

>> +  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.

>


Ack

>> +  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.

>


OK

>> +

>> +  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.

>


OK

>> +    // 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. :)

>


I was torn myself between putting the DT base address in a protocol
and nothing else, and providing some access functions around it. I
have tried to find a sweet spot where we don't duplicate too much code
in the caller, and expose an abstract interface which does not allow
the DT to be arbitrarily mangled.

> (14) How about renaming this function (and the protocol member) to

> GetCompatibleNodeProperty()? Just a suggestion, feel free to disagree.

>


I used Find vs Get since the former suggests that you look for
something that may be anywhere, or not present at all, whereas Get
suggests 'just get me the node, even if you have to create it'

But as I said, I am mostly interested in the protocol dependency that
this provides, and the actual interface could look wildly different
for all I care.

>> +

>> +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.

>


OK

>> +  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).

>


OK

>> +

>> +  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.)

>


Happy to do that, but I deliberately omitted it for v1 considering the
high likelihood that we ultimately end up with something completely
different.

>> +

>> +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".

>


OK

>> +

>> +  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".

>


OK

> I'll seek to continue the review tomorrow.

>


Thanks

We haven't discussed the general goal of this series, but I assume
you're on board with that, given that you are reviewing in detail
already.

Thanks again,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 7, 2016, 7:57 a.m. | #3
On 04/07/16 09:32, Ard Biesheuvel wrote:
> On 6 April 2016 at 22:25, Laszlo Ersek <lersek@redhat.com> wrote:


>> (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

> 

> I don't think minimising the set of #includes should be a goal in

> itself, especially since you may end up relying on transitive

> includes. A source file should include the header files for all

> functionality it pulls in from other objects.

> I know we still end up relying on transitive includes for things like

> types, and EDK2 with its AutoGen.? may supply some things implicitly

> as well. But in general, I don't think removing header #includes

> because you can is a good thing.


Okay.

> Other than that, I agree that this should be cleaned up


[snip]

>>> +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. :)

>>

> 

> I was torn myself between putting the DT base address in a protocol

> and nothing else,


Yup, it crossed my mind.

> and providing some access functions around it. I

> have tried to find a sweet spot where we don't duplicate too much code

> in the caller, and expose an abstract interface which does not allow

> the DT to be arbitrarily mangled.


I think there are certainly arguments in favor of thinking about a
protocol like a shared library (dynamic linking). If we consider it a
service / singleton protocol, and simply want to avoid linking the code
statically into a bunch of independent EFI binaries, that's a good
enough reason for protocol-izing it. I don't think it's necessary for a
protocol to have several instances, and for each instance to have a
bunch of private data to it.

> 

>> (14) How about renaming this function (and the protocol member) to

>> GetCompatibleNodeProperty()? Just a suggestion, feel free to disagree.

>>

> 

> I used Find vs Get since the former suggests that you look for

> something that may be anywhere, or not present at all, whereas Get

> suggests 'just get me the node, even if you have to create it'


I think GetNodeProperty() doesn't comply with that already.

> But as I said, I am mostly interested in the protocol dependency that

> this provides, and the actual interface could look wildly different

> for all I care.


I think the interface looks good thus far, I'd just like it to be a bit
more intuitive. That's why I suggested docs, or name(s) that I felt
would be improvement.

[snip]

>>> +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.)

>>

> 

> Happy to do that, but I deliberately omitted it for v1 considering the

> high likelihood that we ultimately end up with something completely

> different.


Good point. It does make sense to delay the docs until we get to the end
of the series and the interface proves good (for what we need it).

[snip]

> We haven't discussed the general goal of this series, but I assume

> you're on board with that, given that you are reviewing in detail

> already.


Yes, I read the blurb. On one hand, we can have one iteration over the
FDT (+), a bunch of PCDs (which are not flexible enough for exposing
repeating data, BTW) (-), a driver that is tied to everything (-), and
an APRIORI section (-).

On the other hand, each interested driver might have to iterate over the
FDT (-) albeit through the protocol, the data fished out of the DTB
could be arbitrary (+), the FDT accesses would be separated from each
other functionally (+), and DepExes could replace the APRIORI section (+).

I think the conversion is valuable. I do expect some snags (I skimmed a
bit ahead in the series) with PCDs that we'll have to set anyway,
because core drivers depend on them. Plugin libraries can hopefully help
with that (or else we might be able to prove the necessary ordering by
different means). I'll comment on this in more detail when I look at the
affected patches.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 7, 2016, 8:27 a.m. | #4
On 7 April 2016 at 09:57, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/07/16 09:32, Ard Biesheuvel wrote:

>> On 6 April 2016 at 22:25, Laszlo Ersek <lersek@redhat.com> wrote:

>

>>> (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

>>

>> I don't think minimising the set of #includes should be a goal in

>> itself, especially since you may end up relying on transitive

>> includes. A source file should include the header files for all

>> functionality it pulls in from other objects.

>> I know we still end up relying on transitive includes for things like

>> types, and EDK2 with its AutoGen.? may supply some things implicitly

>> as well. But in general, I don't think removing header #includes

>> because you can is a good thing.

>

> Okay.

>

>> Other than that, I agree that this should be cleaned up

>

> [snip]

>

>>>> +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. :)

>>>

>>

>> I was torn myself between putting the DT base address in a protocol

>> and nothing else,

>

> Yup, it crossed my mind.

>

>> and providing some access functions around it. I

>> have tried to find a sweet spot where we don't duplicate too much code

>> in the caller, and expose an abstract interface which does not allow

>> the DT to be arbitrarily mangled.

>

> I think there are certainly arguments in favor of thinking about a

> protocol like a shared library (dynamic linking). If we consider it a

> service / singleton protocol, and simply want to avoid linking the code

> statically into a bunch of independent EFI binaries, that's a good

> enough reason for protocol-izing it. I don't think it's necessary for a

> protocol to have several instances, and for each instance to have a

> bunch of private data to it.

>


Agreed. Let's be pragmatic here: all we want to reuse PCD based
drivers that produce architectural protocols, and that are normally
fixed for a platform but now need to be discovered from DT, which
imposed some kind of ordering. I am happy with almost anything that
achieves that in a more maintainable way than VirtFdtDxe, even if it
does not rhyme :-)

>>

>>> (14) How about renaming this function (and the protocol member) to

>>> GetCompatibleNodeProperty()? Just a suggestion, feel free to disagree.

>>>

>>

>> I used Find vs Get since the former suggests that you look for

>> something that may be anywhere, or not present at all, whereas Get

>> suggests 'just get me the node, even if you have to create it'

>

> I think GetNodeProperty() doesn't comply with that already.

>


True.

>> But as I said, I am mostly interested in the protocol dependency that

>> this provides, and the actual interface could look wildly different

>> for all I care.

>

> I think the interface looks good thus far, I'd just like it to be a bit

> more intuitive. That's why I suggested docs, or name(s) that I felt

> would be improvement.

>

> [snip]

>

>>>> +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.)

>>>

>>

>> Happy to do that, but I deliberately omitted it for v1 considering the

>> high likelihood that we ultimately end up with something completely

>> different.

>

> Good point. It does make sense to delay the docs until we get to the end

> of the series and the interface proves good (for what we need it).

>

> [snip]

>

>> We haven't discussed the general goal of this series, but I assume

>> you're on board with that, given that you are reviewing in detail

>> already.

>

> Yes, I read the blurb. On one hand, we can have one iteration over the

> FDT (+), a bunch of PCDs (which are not flexible enough for exposing

> repeating data, BTW) (-), a driver that is tied to everything (-), and

> an APRIORI section (-).

>

> On the other hand, each interested driver might have to iterate over the

> FDT (-) albeit through the protocol, the data fished out of the DTB

> could be arbitrary (+), the FDT accesses would be separated from each

> other functionally (+), and DepExes could replace the APRIORI section (+).

>

> I think the conversion is valuable. I do expect some snags (I skimmed a

> bit ahead in the series) with PCDs that we'll have to set anyway,

> because core drivers depend on them. Plugin libraries can hopefully help

> with that (or else we might be able to prove the necessary ordering by

> different means). I'll comment on this in more detail when I look at the

> affected patches.

>


Yes, please. And since only virtio/mmio and xenio/mmio remain in
VirtFdtDxe, I could actually handle that as well in a v2, and get rid
of VirtFdtDxe completely.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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>
+#include <Library/UefiDriverEntryPoint.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/HobLib.h>
+#include <libfdt.h>
+
+#include <Guid/Fdt.h>
+#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;
+  }
+  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);
+  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;
+  }
+
+  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
+    // 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);
+}
+
+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);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  *RegElemSize = 8;
+
+  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;
+}
+
+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;
+  }
+
+  mDeviceTreeBase = DeviceTreeBase;
+
+  DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase));
+
+  return gBS->InstallProtocolInterface (
+       &ImageHandle,
+       &gFdtClientProtocolGuid,
+       EFI_NATIVE_INTERFACE,
+       &mFdtClientProtocol
+       );
+}
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
+
+[Protocols]
+  gFdtClientProtocolGuid
+
+[Guids]
+  gFdtHobGuid
+
+[Depex]
+  TRUE