Re: [PATCH] ArmPlatformPkg: convert VExpress ResetSystemLib to ResetSystemLib

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

On Tue, Jul 04, 2017 at 12:35:34PM +0100, Ard Biesheuvel wrote:
> On 4 July 2017 at 12:29, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > Since we're in the process of migrating all of the VExpress platforms
> > to MdeModulePkg ResetSystemRuntimeDxe, convert VExpress ResetSystemLib
> > from EfiResetSystemLib interface to the ResetSystemLib one.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  .../Library/ResetSystemLib/ResetSystemLib.c        | 118 ++++++++++++---------
> >  .../Library/ResetSystemLib/ResetSystemLib.inf      |   4 +-
> >  2 files changed, 72 insertions(+), 50 deletions(-)
> >
> > diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
> > index bafb6f8093..45bb0ab848 100644
> > --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
> > +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.c
> > @@ -5,6 +5,7 @@
> >
> >    Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
> >    Copyright (c) 2013, ARM Ltd. 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,73 +17,92 @@
> >
> >  **/
> >
> > -#include <PiDxe.h>
> > +#include <Base.h>
> >
> > -#include <Library/BaseLib.h>
> >  #include <Library/DebugLib.h>
> > -#include <Library/EfiResetSystemLib.h>
> > -#include <Library/ArmPlatformSysConfigLib.h>
> > -
> > -#include <ArmPlatform.h>
> 
> Don't we need some of these for the definitions of
> ArmPlatformSysConfigSet and SYS_CFG_REBOOT?

Yes, I messed up the corresponding changes in OpenPlatformPkg for the
build test...

Thanks. Update coming.

> > +#include <Library/ResetSystemLib.h>
> >
> >  /**
> > -  Resets the entire platform.
> > +  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.
> >
> > -  @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.
> > +  If this function returns, it means that the system does not support cold
> > +  reset.
> > +**/
> > +VOID
> > +EFIAPI
> > +ResetCold (
> > +  VOID
> > +  )
> > +{
> > +  ArmPlatformSysConfigSet (SYS_CFG_REBOOT, 0);
> > +}
> >
> > +/**
> > +  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.
> >  **/
> > -EFI_STATUS
> > +VOID
> >  EFIAPI
> > -LibResetSystem (
> > -  IN EFI_RESET_TYPE   ResetType,
> > -  IN EFI_STATUS       ResetStatus,
> > -  IN UINTN            DataSize,
> > -  IN CHAR16           *ResetData OPTIONAL
> > +ResetWarm (
> > +  VOID
> >    )
> >  {
> > -  switch (ResetType) {
> > -  case EfiResetPlatformSpecific:
> > -    // Map the platform specific reset as reboot
> > -  case EfiResetWarm:
> > -    // Map a warm reset into a cold reset
> > -  case EfiResetCold:
> > -    // Send the REBOOT function to the platform microcontroller
> > -    ArmPlatformSysConfigSet (SYS_CFG_REBOOT, 0);
> > -
> > -    // We should never be here
> > -    while(1);
> > -  case EfiResetShutdown:
> > -    // Send the SHUTDOWN function to the platform microcontroller
> > -    ArmPlatformSysConfigSet (SYS_CFG_SHUTDOWN, 0);
> > -
> > -    // We should never be here
> > -    while(1);
> > -  }
> > -
> > -  ASSERT(FALSE);
> > -  return EFI_UNSUPPORTED;
> > +  ResetCold ();
> >  }
> >
> >  /**
> > -  Initialize any infrastructure required for LibResetSystem () to function.
> > +  This function causes the system to enter a power state equivalent
> > +  to the ACPI G2/S5 or G3 states.
> >
> > -  @param  ImageHandle   The firmware allocated handle for the EFI image.
> > -  @param  SystemTable   A pointer to the EFI System Table.
> > +  If this function returns, it means that the system does not support shut down reset.
> > +**/
> > +VOID
> > +EFIAPI
> > +ResetShutdown (
> > +  VOID
> > +  )
> > +{
> > +  ArmPlatformSysConfigSet (SYS_CFG_SHUTDOWN, 0);
> > +}
> >
> > -  @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.
> > +**/
> > +VOID
> > +EFIAPI
> > +EnterS3WithImmediateWake (
> > +  VOID
> > +  )
> > +{
> > +  // 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.
> >  **/
> > -EFI_STATUS
> > +VOID
> >  EFIAPI
> > -LibInitializeResetSystem (
> > -  IN EFI_HANDLE        ImageHandle,
> > -  IN EFI_SYSTEM_TABLE  *SystemTable
> > +ResetPlatformSpecific (
> > +  IN UINTN   DataSize,
> > +  IN VOID    *ResetData
> >    )
> >  {
> > -  return EFI_SUCCESS;
> > +  ResetCold ();
> >  }
> > diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.inf b/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.inf
> > index 8c17ea8cdb..bb87df50ef 100644
> > --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ResetSystemLib/ResetSystemLib.inf
> > +++ b/ArmPlatformPkg/ArmVExpressPkg/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,12 +20,13 @@
> >    FILE_GUID                      = 36885202-0854-4373-bfd2-95d229b44d44
> >    MODULE_TYPE                    = BASE
> >    VERSION_STRING                 = 1.0
> > -  LIBRARY_CLASS                  = EfiResetSystemLib
> > +  LIBRARY_CLASS                  = ResetSystemLib
> >
> >  [Sources.common]
> >    ResetSystemLib.c
> >
> >  [Packages]
> > +  MdeModulePkg/MdeModulePkg.dec
> >    MdePkg/MdePkg.dec
> >    EmbeddedPkg/EmbeddedPkg.dec
> 
> Do we still need this?

Unlikely.

> >    ArmPlatformPkg/ArmPlatformPkg.dec
> 
> ... and this, given my question above?

Probably.

/
    Leif