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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Jul 03, 2017 at 06:33:50PM +0100, Ard Biesheuvel wrote:
> 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?

Ah, yes. The MdePkg.dec gets the required -I option set, and
ResetSystemRuntimeDxe pulls in the actual library so the link step
doesn't fail.

But it should clearly be there.

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

Thanks.
Fix folded in.

Pushed as fb5a64de3a.

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