Re: [Linaro-uefi] [RFC PATCH 1/8] ArmVirtualizationPkg: move early UART discovery to PlatformPeim

On 12/10/14 11:59, Laszlo Ersek wrote:

On 12/08/14 19:22, Ard Biesheuvel wrote:

...

This is partially motivated by the desire to use PrePi in a virt environment, and in that configuration, ArmPlatformInitializeMemory()

Typo in commit message, should say ArmPlatformInitializeSystemMemory().

...

is never called. But actually, this is a more suitable place anyway.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org

---

.../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 aa4ced4582e8..463a6d6d3b9b 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)FixedPcdGet64 (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;

The discovery code that is being moved is kept intact.

The source module (ie. the module that the original code is embedded into during build) is "ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf", which has module type PEIM.

The target module is "ArmPlatformPkg/PlatformPei/PlatformPeim.inf", which is also of type PEIM.

These modules are both part of [FV.FVMAIN_COMPACT] in "ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.fdf", and their dispatch order is unspecified (neither is listed in APRIORI PEI).

I was wrong about the dispatch order; these two modules are serialized by the gEfiPeiMemoryDiscoveredPpiGuid [Depex] of "ArmPlatformPkg/ArmVirtualizationPkg/Library/PlatformPeiLib/PlatformPeiLib.inf".

That PPI GUID is installed by the PEI core sometime after the PeiServicesInstallPeiMemory() call in "ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf". (temporary ram migration etc.)

This patch is nonetheless correct; the HOB with gEarlyPL011BaseAddressGuid is needed in the DXE_CORE at the earliest (via "ArmPlatformPkg/ArmVirtualizationPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf').

So neither can depend on anything the other does, which means we're allowed to move code between them.

Reviewed-by: Laszlo Ersek lersek@redhat.com

So the R-b stands.

Laszlo