Re: [Xen-devel] [PATCH v1 04/21] ArmVirtualizationPkg: move early UART discovery to PlatformPeim

[Laszlo Ersek <lersek@xxxxxxxxxx>]

On 01/23/15 16:02, Ard Biesheuvel wrote:
> This is partially motivated by the desire to use PrePi in a virt
> environment, and in that configuration, ArmPlatformInitializeMemory()
> is never called. But actually, this is a more suitable place anyway.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Reviewed-by: Laszlo Ersek <lersek@xxxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
>  .../Library/ArmVirtualizationPlatformLib/Virt.c    | 46 +--------------------
>  .../Library/PlatformPeiLib/PlatformPeiLib.c        | 48 
> ++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 44 deletions(-)
> 
> diff --git 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c
>  
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c
> index 3e3074af72f1..17f268697583 100644
> --- 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c
> +++ 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c
> @@ -24,9 +24,6 @@
>  #include <Pi/PiBootMode.h>
>  #include <Uefi/UefiBaseType.h>
>  #include <Uefi/UefiMultiPhase.h>
> -#include <Pi/PiHob.h>
> -#include <Library/HobLib.h>
> -#include <Guid/EarlyPL011BaseAddress.h>
>  
>  /**
>    Return the current Boot Mode
> @@ -77,25 +74,13 @@ ArmPlatformInitializeSystemMemory (
>    INT32        Node, Prev;
>    UINT64       NewBase;
>    UINT64       NewSize;
> -  BOOLEAN      HaveMemory, HaveUART;
> -  UINT64       *HobData;
>    CONST CHAR8  *Type;
> -  CONST CHAR8  *Compatible;
> -  CONST CHAR8  *CompItem;
>    INT32        Len;
>    CONST UINT64 *RegProp;
> -  UINT64       UartBase;
>  
>    NewBase = 0;
>    NewSize = 0;
>  
> -  HaveMemory = FALSE;
> -  HaveUART   = FALSE;
> -
> -  HobData = BuildGuidHob (&gEarlyPL011BaseAddressGuid, sizeof *HobData);
> -  ASSERT (HobData != NULL);
> -  *HobData = 0;
> -
>    DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
>    ASSERT (DeviceTreeBase != NULL);
>  
> @@ -107,7 +92,7 @@ ArmPlatformInitializeSystemMemory (
>    //
>    // Look for a memory node
>    //
> -  for (Prev = 0; !(HaveMemory && HaveUART); Prev = Node) {
> +  for (Prev = 0;; Prev = Node) {
>      Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
>      if (Node < 0) {
>        break;
> @@ -140,34 +125,7 @@ ArmPlatformInitializeSystemMemory (
>          DEBUG ((EFI_D_ERROR, "%a: Failed to parse FDT memory node\n",
>                 __FUNCTION__));
>        }
> -      HaveMemory = TRUE;
> -      continue;
> -    }
> -
> -    //
> -    // Check for UART node
> -    //
> -    Compatible = fdt_getprop (DeviceTreeBase, Node, "compatible", &Len);
> -
> -    //
> -    // Iterate over the NULL-separated items in the compatible string
> -    //
> -    for (CompItem = Compatible; CompItem != NULL && CompItem < Compatible + 
> Len;
> -      CompItem += 1 + AsciiStrLen (CompItem)) {
> -
> -      if (AsciiStrCmp (CompItem, "arm,pl011") == 0) {
> -        RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> -        ASSERT (Len == 16);
> -
> -        UartBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
> -
> -        DEBUG ((EFI_D_INFO, "%a: PL011 UART @ 0x%lx\n", __FUNCTION__, 
> UartBase));
> -
> -        *HobData = UartBase;
> -
> -        HaveUART = TRUE;
> -        continue;
> -      }
> +      break;
>      }
>    }
>  
> diff --git 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> index af0d6e87da9f..58bc2b828dcd 100644
> --- 
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> +++ 
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.c
> @@ -21,6 +21,8 @@
>  #include <Library/PcdLib.h>
>  #include <libfdt.h>
>  
> +#include <Guid/EarlyPL011BaseAddress.h>
> +
>  EFI_STATUS
>  EFIAPI
>  PlatformPeim (
> @@ -30,6 +32,14 @@ PlatformPeim (
>    VOID               *Base;
>    VOID               *NewBase;
>    UINTN              FdtSize;
> +  UINT64             *UartHobData;
> +  INT32              Node, Prev;
> +  CONST CHAR8        *Compatible;
> +  CONST CHAR8        *CompItem;
> +  INT32              Len;
> +  CONST UINT64       *RegProp;
> +  UINT64             UartBase;
> +
>  
>    Base = (VOID*)(UINTN)FixedPcdGet64 (PcdDeviceTreeInitialBaseAddress);
>    ASSERT (fdt_check_header (Base) == 0);
> @@ -41,6 +51,44 @@ PlatformPeim (
>    CopyMem (NewBase, Base, FdtSize);
>    PcdSet64 (PcdDeviceTreeBaseAddress, (UINT64)(UINTN)NewBase);
>  
> +  UartHobData = BuildGuidHob (&gEarlyPL011BaseAddressGuid, sizeof 
> *UartHobData);
> +  ASSERT (UartHobData != NULL);
> +  *UartHobData = 0;
> +
> +  //
> +  // Look for a UART node
> +  //
> +  for (Prev = 0;; Prev = Node) {
> +    Node = fdt_next_node (Base, Prev, NULL);
> +    if (Node < 0) {
> +      break;
> +    }
> +
> +    //
> +    // Check for UART node
> +    //
> +    Compatible = fdt_getprop (Base, Node, "compatible", &Len);
> +
> +    //
> +    // Iterate over the NULL-separated items in the compatible string
> +    //
> +    for (CompItem = Compatible; CompItem != NULL && CompItem < Compatible + 
> Len;
> +      CompItem += 1 + AsciiStrLen (CompItem)) {
> +
> +      if (AsciiStrCmp (CompItem, "arm,pl011") == 0) {
> +        RegProp = fdt_getprop (Base, Node, "reg", &Len);
> +        ASSERT (Len == 16);
> +
> +        UartBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
> +
> +        DEBUG ((EFI_D_INFO, "%a: PL011 UART @ 0x%lx\n", __FUNCTION__, 
> UartBase));
> +
> +        *UartHobData = UartBase;
> +        break;
> +      }
> +    }
> +  }
> +
>    BuildFvHob (PcdGet64 (PcdFvBaseAddress), PcdGet32 (PcdFvSize));
>  
>    return EFI_SUCCESS;
> 

This patch is identical to the one in

http://lists.linaro.org/pipermail/linaro-uefi/2014-December/000603.html

(modulo the FixedPcdGet64 -> PcdGet64 changed previously), and that's
where it has my R-b from.

Which is okay, but you forgot to update the commit message the way I
requested at the top of

http://lists.linaro.org/pipermail/linaro-uefi/2014-December/000611.html

Thanks,
Laszlo

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel