public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] BeagleBoardPkg: switch to use MdeModulePkg ResetSystemLib
@ 2017-07-03 16:25 Leif Lindholm
  2017-07-03 17:33 ` Ard Biesheuvel
  0 siblings, 1 reply; 3+ messages in thread
From: Leif Lindholm @ 2017-07-03 16:25 UTC (permalink / raw)
  To: edk2-devel; +Cc: ard.biesheuvel

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
 
 [Pcd]
   gArmTokenSpaceGuid.PcdFvBaseAddress
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] BeagleBoardPkg: switch to use MdeModulePkg ResetSystemLib
  2017-07-03 16:25 [PATCH] BeagleBoardPkg: switch to use MdeModulePkg ResetSystemLib Leif Lindholm
@ 2017-07-03 17:33 ` Ard Biesheuvel
  2017-07-03 21:32   ` Leif Lindholm
  0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2017-07-03 17:33 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: edk2-devel@lists.01.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
>


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] BeagleBoardPkg: switch to use MdeModulePkg ResetSystemLib
  2017-07-03 17:33 ` Ard Biesheuvel
@ 2017-07-03 21:32   ` Leif Lindholm
  0 siblings, 0 replies; 3+ messages in thread
From: Leif Lindholm @ 2017-07-03 21:32 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.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
> >


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-07-03 21:30 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-03 16:25 [PATCH] BeagleBoardPkg: switch to use MdeModulePkg ResetSystemLib Leif Lindholm
2017-07-03 17:33 ` Ard Biesheuvel
2017-07-03 21:32   ` Leif Lindholm

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox