Re: [PATCH 3/4] Platforms/zx: Add boot manager lib and entries

[Jun Nie <jun.nie@linaro.org>]

On 2017年08月10日 22:41, Leif Lindholm wrote:
> On Wed, Aug 09, 2017 at 10:12:38PM +0800, Jun Nie wrote:
>> Add boot manager lib and entries, including Android and Grub.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>   .../Drivers/Zx296718EvbDxe/Zx296718EvbDxe.c        | 105 ++++++
>>   .../Drivers/Zx296718EvbDxe/Zx296718EvbDxe.inf      |  66 ++++
>>   .../Library/PlatformBootManagerLib/PlatformBm.c    | 404 +++++++++++++++++++++
>>   .../Library/PlatformBootManagerLib/PlatformBm.h    |  30 ++
>>   .../PlatformBootManagerLib.inf                     |  91 +++++
>>   Silicon/Sanchip/SanchipPkg.dec                     |  29 ++
>>   6 files changed, 725 insertions(+)
>>   create mode 100644 Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.c
>>   create mode 100644 Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.inf
>>   create mode 100644 Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.c
>>   create mode 100644 Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.h
>>   create mode 100644 Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>>   create mode 100644 Silicon/Sanchip/SanchipPkg.dec
>>
>> diff --git a/Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.c b/Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.c
>> new file mode 100644
>> index 0000000..47d02bf
>> --- /dev/null
>> +++ b/Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.c
>> @@ -0,0 +1,105 @@
>> +/** @file
>> +*
>> +*  Copyright (C) 2017 Sanechips Technology Co., Ltd.
>> +*  Copyright (c) 2017, Linaro Ltd.
>> +*
>> +*  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/BaseMemoryLib.h>
>> +#include <Library/BdsLib.h>
>> +#include <Library/CacheMaintenanceLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/DevicePathLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/PrintLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/UefiRuntimeServicesTableLib.h>
>> +
>> +#include <Protocol/AndroidBootImg.h>
>> +#include <Protocol/BlockIo.h>
> 
> Which aspect of BlockIo is used here?
> On the whole, several of the above includes look unused - could you
> prune them back a bit?

Sure.
> 
>> +
>> +#include <libfdt.h>
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +AndroidBootImgAppendKernelArgs (
>> +  IN CHAR16            *Args,
>> +  IN UINTN              Size
>> +  )
>> +{
>> +  UnicodeSPrint (
>> +    Args + StrLen (Args), Size - StrLen (Args), L" efi=noruntime");
> 
> I am not sure what your intent is here, but I am entirely convinced
> there is a better way to achieve it. Can you give some background,
> please?

RTC driver is lack of runtime service, so register access result kernel 
panic. After RTC driver support runtime service, I already delete this 
function  in later version.
> 
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +AndroidBootImgUpdateDtb (
>> +  IN  EFI_PHYSICAL_ADDRESS        OrigFdtBase,
>> +  OUT EFI_PHYSICAL_ADDRESS       *NewFdtBase
>> +  )
>> +{
>> +  UINTN             FdtSize, NumPages;
>> +  INTN              err;
>> +  EFI_STATUS        Status;
>> +
>> +  //
>> +  // Store the FDT as Runtime Service Data to prevent the Kernel from
>> +  // overwritting its data.
>> +  //
> 
> This should really not be necessary and has never been needed
> elsewhere. Have you seen this cause any change in behaviour in an
> actual system?

This is copied from Hisilicon code and I had thought it is for safety. I 
already delete this function in later version.
> 
>> +  FdtSize = fdt_totalsize ((VOID *)(UINTN)OrigFdtBase);
>> +  NumPages = EFI_SIZE_TO_PAGES (FdtSize) + 20;
>> +  Status = gBS->AllocatePages (
>> +                  AllocateAnyPages, EfiRuntimeServicesData,
>> +                  NumPages, NewFdtBase);
>> +  if (EFI_ERROR (Status)) {
>> +    return EFI_BUFFER_TOO_SMALL;
>> +  }
>> +
>> +  CopyMem (
>> +    (VOID*)(UINTN)*NewFdtBase,
>> +    (VOID*)(UINTN)OrigFdtBase,
>> +    FdtSize
>> +    );
>> +
>> +  fdt_pack ((VOID*)(UINTN)*NewFdtBase);
>> +  err = fdt_check_header ((VOID*)(UINTN)*NewFdtBase);
> 
> This does not feel like it belongs here. Check it when you first load
> it, by all means, but there is no need to keep checking it did not get
> destroyed by normal execution.

Yes, it can be removed after check logic is added in library.
> 
>> +  if (err != 0) {
>> +    DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (err:%d)\n", err));
>> +    gBS->FreePages (*NewFdtBase, NumPages);
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +ANDROID_BOOTIMG_PROTOCOL mAndroidBootImg = {
>> +  AndroidBootImgAppendKernelArgs,
>> +  AndroidBootImgUpdateDtb
>> +};
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +Zx296718EvbEntryPoint (
>> +  IN EFI_HANDLE         ImageHandle,
>> +  IN EFI_SYSTEM_TABLE   *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS           Status;
>> +
>> +  Status = gBS->InstallProtocolInterface (
>> +                  &ImageHandle,
>> +                  &gAndroidBootImgProtocolGuid,
>> +                  EFI_NATIVE_INTERFACE,
>> +                  &mAndroidBootImg
>> +                  );
>> +  return Status;
>> +}
>> diff --git a/Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.inf b/Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.inf
>> new file mode 100644
>> index 0000000..03982d1
>> --- /dev/null
>> +++ b/Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.inf
>> @@ -0,0 +1,66 @@
>> +#
>> +#  Copyright (C) 2017 Sanechips Technology Co., Ltd.
>> +#  Copyright (c) 2017, Linaro Ltd.
>> +#
>> +#  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
> 
> 0x00010019/1.25.
> 
>> +  BASE_NAME                      = Zx296718EvbDxe
>> +  FILE_GUID                      = db154b2a-031f-4f3c-9315-c4697ff33e6c
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  ENTRY_POINT                    = Zx296718EvbEntryPoint
>> +
>> +[Sources.common]
>> +  Zx296718EvbDxe.c
>> +
>> +[Packages]
>> +  ArmPkg/ArmPkg.dec
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
> 
> M (MdeM) before P (MdeP).
> 
>> +  Platform/Sanchip/Zx296718Evb/Zx296718Evb.dec
>> +
>> +[LibraryClasses]
>> +  BaseMemoryLib
>> +  BdsLib
>> +  CacheMaintenanceLib
>> +  DebugLib
>> +  DxeServicesLib
>> +  DxeServicesTableLib
>> +  FdtLib
>> +  IoLib
>> +  PcdLib
>> +  PrintLib
>> +  SerialPortLib
>> +  TimerLib
>> +  UefiBootServicesTableLib
>> +  UefiRuntimeServicesTableLib
>> +  UefiLib
> 
> L before R.
> 
>> +  UefiDriverEntryPoint
> 
> But D before L.
> 
> But also, how many of these LibraryClasses are actually used by this
> driver?
> 
>> +
>> +[Guids]
>> +  gEfiEndOfDxeEventGroupGuid
>> +  gEfiFileInfoGuid
>> +  gEfiGlobalVariableGuid
>> +  gFdtTableGuid
>> +  gFdtVariableGuid
> 
> How many of these Guids are actually used by this driver?
> 
>> +
>> +[Protocols]
>> +  gAndroidBootImgProtocolGuid
>> +  gEfiBlockIoProtocolGuid
>> +  gEfiDevicePathFromTextProtocolGuid
>> +  gEfiDevicePathToTextProtocolGuid
>> +  gEfiDevicePathProtocolGuid
>> +  gEfiSimpleFileSystemProtocolGuid
> 
> How many of these protocols are actually used by this driver?

Lots of code are removed from previous version. So most of them can be 
deleted now.

> 
>> +
>> +[Depex]
>> +  TRUE
>> diff --git a/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.c b/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.c
> 
> It looks to me like this implementation (perhaps with a few small
> modifications) should be reusable across multiple implementations.
> 
> That does not need to happen for this to be merged, but it would be
> good if you and Haojian could discuss between yourselves. (Also,
> perhaps we should all sit down and discuss this at Linaro Connect next
> month.)

It is good idea and I will collect your comments later when code is ready.
> 
>> new file mode 100644
>> index 0000000..41861e2
>> --- /dev/null
>> +++ b/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.c
>> @@ -0,0 +1,404 @@
>> +/** @file
>> +  Implementation for PlatformBootManagerLib library class interfaces.
>> +
>> +  Copyright (C) 2015-2016, Red Hat, Inc.
>> +  Copyright (c) 2014, ARM Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2016-2017, 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/DevicePathLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/UefiBootManagerLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Protocol/BlockIo.h>
>> +#include <Protocol/DevicePath.h>
>> +#include <Protocol/DevicePathFromText.h>
>> +#include <Protocol/DevicePathToText.h>
>> +#include <Protocol/LoadedImage.h>
>> +#include <Guid/EventGroup.h>
>> +#include <Guid/TtyTerm.h>
> 
> Can you verify all of the above are actually used by this file?

Sure.
> 
>> +
>> +#include "PlatformBm.h"
>> +
>> +#define DP_NODE_LEN(Type) { (UINT8)sizeof (Type), (UINT8)(sizeof (Type) >> 8) }
>> +
>> +#define GRUB_FILE_NAME       L"\\EFI\\BOOT\\GRUBAA64.EFI"
>> +
>> +
>> +#pragma pack (1)
>> +typedef struct {
>> +  VENDOR_DEVICE_PATH         SerialDxe;
>> +  UART_DEVICE_PATH           Uart;
>> +  VENDOR_DEFINED_DEVICE_PATH TermType;
>> +  EFI_DEVICE_PATH_PROTOCOL   End;
>> +} PLATFORM_SERIAL_CONSOLE;
>> +#pragma pack ()
>> +
>> +#define SERIAL_DXE_FILE_GUID { \
>> +          0xD3987D4B, 0x971A, 0x435F, \
>> +          { 0x8C, 0xAF, 0x49, 0x67, 0xEB, 0x62, 0x72, 0x41 } \
>> +          }
>> +
>> +STATIC PLATFORM_SERIAL_CONSOLE mSerialConsole = {
>> +  //
>> +  // VENDOR_DEVICE_PATH SerialDxe
>> +  //
>> +  {
>> +    { HARDWARE_DEVICE_PATH, HW_VENDOR_DP, DP_NODE_LEN (VENDOR_DEVICE_PATH) },
>> +    SERIAL_DXE_FILE_GUID
>> +  },
>> +
>> +  //
>> +  // UART_DEVICE_PATH Uart
>> +  //
>> +  {
>> +    { MESSAGING_DEVICE_PATH, MSG_UART_DP, DP_NODE_LEN (UART_DEVICE_PATH) },
>> +    0,                                      // Reserved
>> +    FixedPcdGet64 (PcdUartDefaultBaudRate), // BaudRate
>> +    FixedPcdGet8 (PcdUartDefaultDataBits),  // DataBits
>> +    FixedPcdGet8 (PcdUartDefaultParity),    // Parity
>> +    FixedPcdGet8 (PcdUartDefaultStopBits)   // StopBits
>> +  },
>> +
>> +  //
>> +  // VENDOR_DEFINED_DEVICE_PATH TermType
>> +  //
>> +  {
>> +    {
>> +      MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,
>> +      DP_NODE_LEN (VENDOR_DEFINED_DEVICE_PATH)
>> +    }
>> +    //
>> +    // Guid to be filled in dynamically
>> +    //
>> +  },
>> +
>> +  //
>> +  // EFI_DEVICE_PATH_PROTOCOL End
>> +  //
>> +  {
>> +    END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> +    DP_NODE_LEN (EFI_DEVICE_PATH_PROTOCOL)
>> +  }
>> +};
>> +
>> +/**
>> +  Check if the handle satisfies a particular condition.
>> +
>> +  @param[in] Handle      The handle to check.
>> +  @param[in] ReportText  A caller-allocated string passed in for reporting
>> +                         purposes. It must never be NULL.
>> +
>> +  @retval TRUE   The condition is satisfied.
>> +  @retval FALSE  Otherwise. This includes the case when the condition could not
>> +                 be fully evaluated due to an error.
>> +**/
>> +typedef
>> +BOOLEAN
>> +(EFIAPI *FILTER_FUNCTION) (
>> +  IN EFI_HANDLE   Handle,
>> +  IN CONST CHAR16 *ReportText
>> +  );
>> +
>> +
>> +/**
>> +  Process a handle.
>> +
>> +  @param[in] Handle      The handle to process.
>> +  @param[in] ReportText  A caller-allocated string passed in for reporting
>> +                         purposes. It must never be NULL.
>> +**/
>> +typedef
>> +VOID
>> +(EFIAPI *CALLBACK_FUNCTION)  (
>> +  IN EFI_HANDLE   Handle,
>> +  IN CONST CHAR16 *ReportText
>> +  );
>> +
>> +STATIC
>> +VOID
>> +PlatformRegisterFvBootOption (
>> +  EFI_GUID                         *FileGuid,
>> +  CHAR16                           *Description,
>> +  UINT32                           Attributes
>> +  )
>> +{
>> +  EFI_STATUS                        Status;
>> +  INTN                              OptionIndex;
>> +  EFI_BOOT_MANAGER_LOAD_OPTION      NewOption;
>> +  EFI_BOOT_MANAGER_LOAD_OPTION      *BootOptions;
>> +  UINTN                             BootOptionCount;
>> +  MEDIA_FW_VOL_FILEPATH_DEVICE_PATH FileNode;
>> +  EFI_LOADED_IMAGE_PROTOCOL         *LoadedImage;
>> +  EFI_DEVICE_PATH_PROTOCOL          *DevicePath;
>> +
>> +  Status = gBS->HandleProtocol (
>> +                  gImageHandle,
>> +                  &gEfiLoadedImageProtocolGuid,
>> +                  (VOID **) &LoadedImage
>> +                  );
>> +  ASSERT_EFI_ERROR (Status);
>> +
>> +  EfiInitializeFwVolDevicepathNode (&FileNode, FileGuid);
>> +  DevicePath = DevicePathFromHandle (LoadedImage->DeviceHandle);
>> +  ASSERT (DevicePath != NULL);
>> +  DevicePath = AppendDevicePathNode (
>> +                 DevicePath,
>> +                 (EFI_DEVICE_PATH_PROTOCOL *) &FileNode
>> +                 );
>> +  ASSERT (DevicePath != NULL);
>> +
>> +  Status = EfiBootManagerInitializeLoadOption (
>> +             &NewOption,
>> +             LoadOptionNumberUnassigned,
>> +             LoadOptionTypeBoot,
>> +             Attributes,
>> +             Description,
>> +             DevicePath,
>> +             NULL,
>> +             0
>> +             );
>> +  ASSERT_EFI_ERROR (Status);
>> +  FreePool (DevicePath);
>> +
>> +  BootOptions = EfiBootManagerGetLoadOptions (
>> +                  &BootOptionCount, LoadOptionTypeBoot
>> +                  );
>> +
>> +  OptionIndex = EfiBootManagerFindLoadOption (
>> +                  &NewOption, BootOptions, BootOptionCount
>> +                  );
>> +
>> +  if (OptionIndex == -1) {
>> +    Status = EfiBootManagerAddLoadOptionVariable (&NewOption, MAX_UINTN);
>> +    ASSERT_EFI_ERROR (Status);
>> +  }
>> +
>> +  EfiBootManagerFreeLoadOption (&NewOption);
>> +  EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
>> +}
>> +
>> +
>> +STATIC
>> +VOID
>> +PlatformRegisterBootGrub (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS                           Status;
>> +  CHAR16                              *BootPathStr;
>> +  EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL  *EfiDevicePathFromTextProtocol;
>> +  EFI_DEVICE_PATH                     *DevicePath;
>> +  EFI_DEVICE_PATH                     *FileDevicePath;
>> +  FILEPATH_DEVICE_PATH                *FilePath;
>> +  UINTN                                Size;
>> +  EFI_BOOT_MANAGER_LOAD_OPTION         NewOption;
>> +  EFI_BOOT_MANAGER_LOAD_OPTION        *BootOptions;
>> +  UINTN                                BootOptionCount;
>> +  INTN                                 OptionIndex;
>> +
>> +  //
>> +  // Get PcdAndroidBootDevicePath
>> +  //
>> +  BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
>> +  ASSERT (BootPathStr != NULL);
>> +  Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
>> +                                (VOID **)&EfiDevicePathFromTextProtocol);
>> +  ASSERT_EFI_ERROR(Status);
>> +  DevicePath = (EFI_DEVICE_PATH *)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
>> +  ASSERT (DevicePath != NULL);
>> +
>> +  Size = StrSize (GRUB_FILE_NAME);
>> +  FileDevicePath = AllocatePool (Size + SIZE_OF_FILEPATH_DEVICE_PATH + END_DEVICE_PATH_LENGTH);
>> +  if (FileDevicePath != NULL) {
>> +    FilePath = (FILEPATH_DEVICE_PATH *) FileDevicePath;
>> +    FilePath->Header.Type    = MEDIA_DEVICE_PATH;
>> +    FilePath->Header.SubType = MEDIA_FILEPATH_DP;
>> +    CopyMem (&FilePath->PathName, GRUB_FILE_NAME, Size);
>> +    SetDevicePathNodeLength (&FilePath->Header, Size + SIZE_OF_FILEPATH_DEVICE_PATH);
>> +    SetDevicePathEndNode (NextDevicePathNode (&FilePath->Header));
>> +
>> +    DevicePath = AppendDevicePath (DevicePath, FileDevicePath);
>> +    FreePool (FileDevicePath);
>> +  }
>> +  Status = EfiBootManagerInitializeLoadOption (
>> +             &NewOption,
>> +             LoadOptionNumberUnassigned,
>> +             LoadOptionTypeBoot,
>> +             LOAD_OPTION_ACTIVE,
>> +             L"Grub",
>> +             DevicePath,
>> +             NULL,
>> +             0
>> +             );
>> +  ASSERT_EFI_ERROR (Status);
>> +  FreePool (DevicePath);
>> +
>> +  BootOptions = EfiBootManagerGetLoadOptions (
>> +                  &BootOptionCount, LoadOptionTypeBoot
>> +                  );
>> +
>> +  OptionIndex = EfiBootManagerFindLoadOption (
>> +                  &NewOption, BootOptions, BootOptionCount
>> +                  );
>> +
>> +  if (OptionIndex == -1) {
>> +    Status = EfiBootManagerAddLoadOptionVariable (&NewOption, MAX_UINTN);
>> +    ASSERT_EFI_ERROR (Status);
>> +  }
>> +
>> +  EfiBootManagerFreeLoadOption (&NewOption);
>> +  EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
>> +
>> +}
>> +
>> +STATIC
>> +VOID
>> +PlatformRegisterOptionsAndKeys (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS                           Status;
>> +  EFI_INPUT_KEY                        Enter;
>> +  EFI_INPUT_KEY                        Esc;
>> +  EFI_BOOT_MANAGER_LOAD_OPTION         BootOption;
>> +
>> +  //
>> +  // Register Boot. OptionNumber is 1.
>> +  //
>> +  PlatformRegisterBootGrub ();
>> +
>> +  //
>> +  // Register Android Boot. OptionNumber is 2.
>> +  //
>> +  PlatformRegisterFvBootOption (
>> +    PcdGetPtr (PcdAndroidBootFile), L"Android Boot", LOAD_OPTION_ACTIVE
>> +    );
>> +
>> +  //
>> +  // Register ENTER as CONTINUE key
>> +  //
>> +  Enter.ScanCode    = SCAN_NULL;
>> +  Enter.UnicodeChar = CHAR_CARRIAGE_RETURN;
>> +  Status = EfiBootManagerRegisterContinueKeyOption (0, &Enter, NULL);
>> +  ASSERT_EFI_ERROR (Status);
>> +
>> +  //
>> +  // Map ESC to Boot Manager Menu
>> +  //
>> +  Esc.ScanCode    = SCAN_ESC;
>> +  Esc.UnicodeChar = CHAR_NULL;
>> +  Status = EfiBootManagerGetBootManagerMenu (&BootOption);
>> +  ASSERT_EFI_ERROR (Status);
>> +  Status = EfiBootManagerAddKeyOptionVariable (
>> +             NULL, (UINT16) BootOption.OptionNumber, 0, &Esc, NULL
>> +             );
>> +  ASSERT (Status == EFI_SUCCESS || Status == EFI_ALREADY_STARTED);
>> +}
>> +
>> +
>> +//
>> +// BDS Platform Functions
>> +//
>> +/**
>> +  Do the platform init, can be customized by OEM/IBV
>> +  Possible things that can be done in PlatformBootManagerBeforeConsole:
>> +  > Update console variable: 1. include hot-plug devices;
>> +  >                          2. Clear ConIn and add SOL for AMT
>> +  > Register new Driver#### or Boot####
>> +  > Register new Key####: e.g.: F12
>> +  > Signal ReadyToLock event
>> +  > Authentication action: 1. connect Auth devices;
>> +  >                        2. Identify auto logon user.
>> +**/
>> +VOID
>> +EFIAPI
>> +PlatformBootManagerBeforeConsole (
>> +  VOID
>> +  )
>> +{
>> +  //
>> +  // Signal EndOfDxe PI Event
>> +  //
>> +  EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
>> +
>> +  //
>> +  // Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
>> +  //
>> +  ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4);
>> +  CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid);
>> +
>> +  EfiBootManagerUpdateConsoleVariable (ConIn,
>> +    (EFI_DEVICE_PATH_PROTOCOL *)&mSerialConsole, NULL);
>> +  EfiBootManagerUpdateConsoleVariable (ConOut,
>> +    (EFI_DEVICE_PATH_PROTOCOL *)&mSerialConsole, NULL);
>> +  EfiBootManagerUpdateConsoleVariable (ErrOut,
>> +    (EFI_DEVICE_PATH_PROTOCOL *)&mSerialConsole, NULL);
>> +
>> +  //
>> +  // Register platform-specific boot options and keyboard shortcuts.
>> +  //
>> +  PlatformRegisterOptionsAndKeys ();
>> +}
>> +
>> +/**
>> +  Do the platform specific action after the console is ready
>> +  Possible things that can be done in PlatformBootManagerAfterConsole:
>> +  > Console post action:
>> +    > Dynamically switch output mode from 100x31 to 80x25 for certain senarino
>> +    > Signal console ready platform customized event
>> +  > Run diagnostics like memory testing
>> +  > Connect certain devices
>> +  > Dispatch aditional option roms
>> +  > Special boot
>> +**/
>> +VOID
>> +EFIAPI
>> +PlatformBootManagerAfterConsole (
>> +  VOID
>> +  )
>> +{
>> +  Print (L"Press ESCAPE for boot options ");
>> +
>> +  //
>> +  // Connect the rest of the devices.
>> +  //
>> +  EfiBootManagerConnectAll ();
>> +
>> +  //
>> +  // Enumerate all possible boot options.
>> +  //
>> +  EfiBootManagerRefreshAllBootOption ();
>> +
>> +  //
>> +  // Register UEFI Shell
>> +  //
>> +  PlatformRegisterFvBootOption (
>> +    PcdGetPtr (PcdShellFile), L"UEFI Shell", LOAD_OPTION_ACTIVE
>> +    );
>> +}
>> +
>> +/**
>> +  This function is called each second during the boot manager waits the
>> +  timeout.
>> +
>> +  @param TimeoutRemain  The remaining timeout.
>> +**/
>> +VOID
>> +EFIAPI
>> +PlatformBootManagerWaitCallback (
>> +  UINT16          TimeoutRemain
>> +  )
>> +{
>> +  Print (L".");
>> +}
>> diff --git a/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.h b/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.h
>> new file mode 100644
>> index 0000000..de52a9c
>> --- /dev/null
>> +++ b/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.h
>> @@ -0,0 +1,30 @@
>> +/** @file
>> +  Head file for BDS Platform specific code
>> +
>> +  Copyright (C) 2015-2016, Red Hat, Inc.
>> +  Copyright (c) 2004 - 2008, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2016-2017, 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.
>> +
>> +**/
>> +
>> +#ifndef _PLATFORM_BM_H_
>> +#define _PLATFORM_BM_H_
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/DevicePathLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/UefiRuntimeServicesTableLib.h>
> 
> If all this holds is a set of include files for a single source file,
> may they not just go directly into the .c file?

Sure.
> 
>> +
>> +#endif // _PLATFORM_BM_H_
>> diff --git a/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> new file mode 100644
>> index 0000000..b9c6aaa
>> --- /dev/null
>> +++ b/Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>> @@ -0,0 +1,91 @@
>> +## @file
>> +#  Implementation for PlatformBootManagerLib library class interfaces.
>> +#
>> +#  Copyright (C) 2015-2016, Red Hat, Inc.
>> +#  Copyright (c) 2014, ARM Ltd. All rights reserved.<BR>
>> +#  Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.<BR>
>> +#  Copyright (c) 2017, 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
> 
> 0x00010019/1.25.
> 
>> +  BASE_NAME                      = PlatformBootManagerLib
>> +  FILE_GUID                      = 9001ec12-8757-4638-b6d0-e0863dddddb0
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = PlatformBootManagerLib|DXE_DRIVER
>> +
>> +#
>> +# The following information is for reference only and not required by the build tools.
>> +#
>> +#  VALID_ARCHITECTURES           = ARM AARCH64
>> +#
>> +
>> +[Sources]
>> +  PlatformBm.c
>> +
>> +[Packages]
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +  IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  MdePkg/MdePkg.dec
>> +  Silicon/Sanchip/SanchipPkg.dec
> 
> Are all of these actually used?
> 
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +  BaseMemoryLib
>> +  DebugLib
>> +  DevicePathLib
>> +  DxeServicesLib
>> +  MemoryAllocationLib
>> +  PcdLib
>> +  PrintLib
>> +  UefiBootManagerLib
>> +  UefiBootServicesTableLib
>> +  UefiLib
> 
> Are all of these actually used?
> 
>> +
>> +[FeaturePcd]
>> +  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable
>> +  gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport
> 
> Are all of these actually used?
> 
>> +
>> +[FixedPcd]
>> +  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdLogoFile
>> +  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
>> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
>> +  gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType
> 
> Are all of these actually used?
> 
>> +
>> +[Pcd]
>> +  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
>> +  gSanchipTokenSpaceGuid.PcdAndroidBootFile
> 
> Are all of these actually used?
> 
>> +
>> +[Guids]
>> +  gEfiFileInfoGuid
>> +  gEfiFileSystemInfoGuid
>> +  gEfiFileSystemVolumeLabelInfoIdGuid
>> +  gEfiEndOfDxeEventGroupGuid
>> +  gEfiTtyTermGuid
> 
> Are all of these actually used?
> 
>> +
>> +[Protocols]
>> +  gEfiDevicePathFromTextProtocolGuid
>> +  gEfiDevicePathProtocolGuid
>> +  gEfiLoadedImageProtocolGuid
>> +  gEfiOEMBadgingProtocolGuid
>> +  gEfiPciRootBridgeIoProtocolGuid
>> +  gEfiSimpleFileSystemProtocolGuid
>> +  gEfiDevicePathToTextProtocolGuid
> 
> Are all of these actually used?

Will clean it after this library rewrite for sharing.
> 
>> +
>> +[Pcd]
>> +  gEmbeddedTokenSpaceGuid.PcdAndroidBootDevicePath
>> diff --git a/Silicon/Sanchip/SanchipPkg.dec b/Silicon/Sanchip/SanchipPkg.dec
>> new file mode 100644
>> index 0000000..92d439d
>> --- /dev/null
>> +++ b/Silicon/Sanchip/SanchipPkg.dec
>> @@ -0,0 +1,29 @@
>> +#
>> +#  Copyright (C) 2017 Sanechips Technology Co., Ltd.
>> +#  Copyright (c) 2017, Linaro Ltd.
>> +#
>> +#  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]
>> +  DEC_SPECIFICATION              = 0x00010005
> 
> 0x00010019/1.25.
> 
>> +  PACKAGE_NAME                   = SanchipPkg
>> +  PACKAGE_GUID                   = f991248f-9e21-4e4f-b344-eaad28e42ec0
>> +  PACKAGE_VERSION                = 0.1
>> +
>> +[Includes.common]
>> +  Include                        # Root include for the package
> 
> This directory does not exist, causing compliation to fail.

Test shows that this include entry is unnecessary.
> 
>> +
>> +[Guids.common]
>> +  gSanchipTokenSpaceGuid          =  { 0x9589ba86, 0x58e6, 0x426c, { 0xbb, 0x39, 0x21, 0xf5, 0x68, 0x78, 0xe4, 0x27 } }
>> +
>> +[PcdsFixedAtBuild.common]
>> +  gSanchipTokenSpaceGuid.PcdAndroidBootFile|{ 0x36, 0x8b, 0x73, 0x3a, 0xc5, 0xb9, 0x63, 0x47, 0xab, 0xbd, 0x6c, 0xbd, 0x4b, 0x25, 0xf9, 0xff }|VOID*|0x00000003
> 
> What?

PcdAndroidBootFile is FileGuid for AndroidBoot app file, as PcdShellFile 
Guid in other places. Do you want to hardcode the Guid in PlatformBm.c 
library code?
> 
>> +  gSanchipTokenSpaceGuid.PcdZxRtcClockBase|0|UINT64|0x00000002
>> +  gSanchipTokenSpaceGuid.PcdZxRtcClockFreq|24000|UINT64|0x00000001
>> -- 
>> 1.9.1
>>