Re: [PATCH] BeagleBoardPkg: switch to use MdeModulePkg ResetSystemLib

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 3 July 2017 at 17:25, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> The BeagleBoard port used EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> for its reset handling. With the arrival
> MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>
> As part of this, change BeagleBoardPkg/Library/ResetSystemLib to be an
> implementation of ResetSystemLib instead of the previous
> EfiResetSystemLib.
>
> Wire all reset variants to ResetCold, except for ResetShutdown and
> EnterS3WithImmediateWake, which return immediately.
>
> Note: this ResetSystemLib never supported being called after
> ExitBootservices, and this shortcoming is not addressed here.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>
> This is only build tested for now.
> Once the edk2-platforms master branch goes live, I intend
> to move this platform there, and bring back in the
> BeagleBoard Black port. Which I do have a test platform
> handy for.
>
>  BeagleBoardPkg/BeagleBoardPkg.dsc                  |   4 +-
>  BeagleBoardPkg/BeagleBoardPkg.fdf                  |   2 +-
>  .../Library/ResetSystemLib/ResetSystemLib.c        | 180 ++++++++-------------
>  .../Library/ResetSystemLib/ResetSystemLib.inf      |  16 +-
>  4 files changed, 70 insertions(+), 132 deletions(-)
>
> diff --git a/BeagleBoardPkg/BeagleBoardPkg.dsc b/BeagleBoardPkg/BeagleBoardPkg.dsc
> index 90d6af444d..b22f814a28 100644
> --- a/BeagleBoardPkg/BeagleBoardPkg.dsc
> +++ b/BeagleBoardPkg/BeagleBoardPkg.dsc
> @@ -56,7 +56,7 @@
>    BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
>    BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>
> -  EfiResetSystemLib|BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.inf
> +  ResetSystemLib|BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.inf
>
>    PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
>    PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
> @@ -421,7 +421,7 @@
>  #      SerialPortLib|ArmPkg/Library/SemiHostingSerialPortLib/SemiHostingSerialPortLib.inf
>  #  }
>
> -  EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
>    EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
>
> diff --git a/BeagleBoardPkg/BeagleBoardPkg.fdf b/BeagleBoardPkg/BeagleBoardPkg.fdf
> index f140d9019c..c9c6afd714 100644
> --- a/BeagleBoardPkg/BeagleBoardPkg.fdf
> +++ b/BeagleBoardPkg/BeagleBoardPkg.fdf
> @@ -115,7 +115,7 @@ FvNameGuid         = d0dd3e90-343d-4cb3-8f69-772214989282
>    INF MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
>    INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
>
> -  INF EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  INF MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
>    INF EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
>
> diff --git a/BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.c b/BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.c
> index 6b7879b02b..7bc6c6c329 100644
> --- a/BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.c
> +++ b/BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.c
> @@ -2,6 +2,7 @@
>    Do a generic Cold Reset for OMAP3550 and BeagleBoard specific Warm reset
>
>    Copyright (c) 2008 - 2010, Apple Inc. 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
> @@ -16,150 +17,95 @@
>
>  #include <Uefi.h>
>
> -#include <Library/ArmLib.h>
> -#include <Library/CacheMaintenanceLib.h>
> -#include <Library/MemoryAllocationLib.h>
>  #include <Library/IoLib.h>
> -#include <Library/PcdLib.h>
> -#include <Library/DebugLib.h>
> -#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/ResetSystemLib.h>
>
>  #include <Omap3530/Omap3530.h>
>
> +/**
> +  This function causes a system-wide reset (cold reset), in which
> +  all circuitry within the system returns to its initial state. This type of
> +  reset is asynchronous to system operation and operates without regard to
> +  cycle boundaries.
>
> +  If this function returns, it means that the system does not support cold
> +  reset.
> +**/
>  VOID
> -ShutdownEfi (
> +EFIAPI
> +ResetCold (
>    VOID
>    )
>  {
> -  EFI_STATUS              Status;
> -  UINTN                   MemoryMapSize;
> -  EFI_MEMORY_DESCRIPTOR   *MemoryMap;
> -  UINTN                   MapKey;
> -  UINTN                   DescriptorSize;
> -  UINTN                   DescriptorVersion;
> -  UINTN                   Pages;
> -
> -  MemoryMap = NULL;
> -  MemoryMapSize = 0;
> -  do {
> -    Status = gBS->GetMemoryMap (
> -                    &MemoryMapSize,
> -                    MemoryMap,
> -                    &MapKey,
> -                    &DescriptorSize,
> -                    &DescriptorVersion
> -                    );
> -    if (Status == EFI_BUFFER_TOO_SMALL) {
> -
> -      Pages = EFI_SIZE_TO_PAGES (MemoryMapSize) + 1;
> -      MemoryMap = AllocatePages (Pages);
> -
> -      //
> -      // Get System MemoryMap
> -      //
> -      Status = gBS->GetMemoryMap (
> -                      &MemoryMapSize,
> -                      MemoryMap,
> -                      &MapKey,
> -                      &DescriptorSize,
> -                      &DescriptorVersion
> -                      );
> -      // Don't do anything between the GetMemoryMap() and ExitBootServices()
> -      if (!EFI_ERROR (Status)) {
> -        Status = gBS->ExitBootServices (gImageHandle, MapKey);
> -        if (EFI_ERROR (Status)) {
> -          FreePages (MemoryMap, Pages);
> -          MemoryMap = NULL;
> -          MemoryMapSize = 0;
> -        }
> -      }
> -    }
> -  } while (EFI_ERROR (Status));
> -
> -  //Clean and invalidate caches.
> -  WriteBackInvalidateDataCache();
> -  InvalidateInstructionCache();
> -
> -  //Turning off Caches and MMU
> -  ArmDisableDataCache ();
> -  ArmDisableInstructionCache ();
> -  ArmDisableMmu ();
> +  //Perform cold reset of the system.
> +  MmioOr32 (PRM_RSTCTRL, RST_DPLL3);
> +  while ((MmioRead32(PRM_RSTST) & GLOBAL_COLD_RST) != 0x1);
>  }
>
> -typedef
> +/**
> +  This function causes a system-wide initialization (warm reset), in which all
> +  processors are set to their initial state. Pending cycles are not corrupted.
> +
> +  If this function returns, it means that the system does not support warm
> +  reset.
> +**/
>  VOID
> -(EFIAPI *CALL_STUB)(
> +EFIAPI
> +ResetWarm (
>    VOID
> -);
> -
> +  )
> +{
> +  ResetCold ();
> +}
>
>  /**
> -  Resets the entire platform.
> -
> -  @param  ResetType             The type of reset to perform.
> -  @param  ResetStatus           The status code for the reset.
> -  @param  DataSize              The size, in bytes, of WatchdogData.
> -  @param  ResetData             For a ResetType of EfiResetCold, EfiResetWarm, or
> -                                EfiResetShutdown the data buffer starts with a Null-terminated
> -                                Unicode string, optionally followed by additional binary data.
> +  This function causes the system to enter a power state equivalent
> +  to the ACPI G2/S5 or G3 states.
>
> +  If this function returns, it means that the system does not support shut down
> +  reset.
>  **/
> -EFI_STATUS
> +VOID
>  EFIAPI
> -LibResetSystem (
> -  IN EFI_RESET_TYPE   ResetType,
> -  IN EFI_STATUS       ResetStatus,
> -  IN UINTN            DataSize,
> -  IN CHAR16           *ResetData OPTIONAL
> +ResetShutdown (
> +  VOID
>    )
>  {
> -  CALL_STUB   StartOfFv;
> -
> -  if (ResetData != NULL) {
> -    DEBUG((EFI_D_ERROR, "%s", ResetData));
> -  }
> -
> -  ShutdownEfi ();
> -
> -  switch (ResetType) {
> -  case EfiResetWarm:
> -    //Perform warm reset of the system by jumping to the begining of the FV
> -    StartOfFv = (CALL_STUB)(UINTN)PcdGet64 (PcdFvBaseAddress);
> -    StartOfFv ();
> -    break;
> -  case EfiResetCold:
> -  case EfiResetShutdown:
> -  default:
> -    //Perform cold reset of the system.
> -    MmioOr32 (PRM_RSTCTRL, RST_DPLL3);
> -    while ((MmioRead32(PRM_RSTST) & GLOBAL_COLD_RST) != 0x1);
> -    break;
> -  }
> -
> -  // If the reset didn't work, return an error.
> -  ASSERT (FALSE);
> -  return EFI_DEVICE_ERROR;
> +  // not implemented
>  }
>
> -
> -
>  /**
> -  Initialize any infrastructure required for LibResetSystem () to function.
> -
> -  @param  ImageHandle   The firmware allocated handle for the EFI image.
> -  @param  SystemTable   A pointer to the EFI System Table.
> -
> -  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
> +  This function causes the system to enter S3 and then wake up immediately.
>
> +  If this function returns, it means that the system does not support S3
> +  feature.
>  **/
> -EFI_STATUS
> +VOID
>  EFIAPI
> -LibInitializeResetSystem (
> -  IN EFI_HANDLE        ImageHandle,
> -  IN EFI_SYSTEM_TABLE  *SystemTable
> +EnterS3WithImmediateWake (
> +  VOID
>    )
>  {
> -  return EFI_SUCCESS;
> +  // not implemented
>  }
>
> +/**
> +  This function causes a systemwide reset. The exact type of the reset is
> +  defined by the EFI_GUID that follows the Null-terminated Unicode string passed
> +  into ResetData. If the platform does not recognize the EFI_GUID in ResetData
> +  the platform must pick a supported reset type to perform.The platform may
> +  optionally log the parameters from any non-normal reset that occurs.
> +
> +  @param[in]  DataSize   The size, in bytes, of ResetData.
> +  @param[in]  ResetData  The data buffer starts with a Null-terminated string,
> +                         followed by the EFI_GUID.
> +**/
> +VOID
> +EFIAPI
> +ResetPlatformSpecific (
> +  IN UINTN   DataSize,
> +  IN VOID    *ResetData
> +  )
> +{
> +  ResetCold ();
> +}
> diff --git a/BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.inf b/BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.inf
> index 64638cb62c..150c4bb900 100644
> --- a/BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.inf
> +++ b/BeagleBoardPkg/Library/ResetSystemLib/ResetSystemLib.inf
> @@ -2,6 +2,7 @@
>  # Reset System lib to make it easy to port new platforms
>  #
>  # Copyright (c) 2008, Apple Inc. 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
> @@ -19,31 +20,22 @@
>    FILE_GUID                      = 781371a2-3fdd-41d4-96a1-7b34cbc9e895
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = EfiResetSystemLib
> +  LIBRARY_CLASS                  = ResetSystemLib
>
>
>  [Sources.common]
>    ResetSystemLib.c
>
>  [Packages]
> -  MdePkg/MdePkg.dec
>    ArmPkg/ArmPkg.dec
> -  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
>    Omap35xxPkg/Omap35xxPkg.dec
>
>  [Pcd.common]
>    gArmTokenSpaceGuid.PcdCpuResetAddress
> -  gEmbeddedTokenSpaceGuid.PcdEmbeddedFdBaseAddress
>
>  [LibraryClasses]
> -  DebugLib
> -  ArmLib
> -  CacheMaintenanceLib
> -  MemoryAllocationLib
> -  UefiRuntimeServicesTableLib
> -  TimerLib
> -  UefiLib
> -  UefiBootServicesTableLib
>

Don't you need IoLib here?

Other than that:

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>  [Pcd]
>    gArmTokenSpaceGuid.PcdFvBaseAddress
> --
> 2.11.0
>