Re: [PATCH] Platforms/Marvell: switch to generic ResetSystemRuntimeDxe

[Marcin Wojtas <mw@semihalf.com>]

Hi,

2017-07-04 13:00 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> Switch to the generic implementation of the ResetSystem() runtime
> service call, which is preferred over the one in EmbeddedPkg. This
> involves selecting another DXE runtime driver, and morphing the
> existing EfiResetSystemLib implementation into a ResetSystemLib one
> (which is what the generic driver depends on for platform glue)
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platforms/Marvell/Armada/Armada.dsc.inc                       |   4 +-
>  Platforms/Marvell/Armada/Armada70x0.fdf                       |   2 +-
>  Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.c   | 110 ++++++++++++++------
>  Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.inf |   7 +-
>  4 files changed, 87 insertions(+), 36 deletions(-)
>
> diff --git a/Platforms/Marvell/Armada/Armada.dsc.inc b/Platforms/Marvell/Armada/Armada.dsc.inc
> index 475fc6d05379..32eaa2942564 100644
> --- a/Platforms/Marvell/Armada/Armada.dsc.inc
> +++ b/Platforms/Marvell/Armada/Armada.dsc.inc
> @@ -94,7 +94,7 @@
>    # Reset and Time libraries
>    EfiTimeBaseLib|OpenPlatformPkg/Library/EfiTimeBaseLib/EfiTimeBaseLib.inf
>    RealTimeClockLib|OpenPlatformPkg/Platforms/Marvell/Armada/Library/RealTimeClockLib/RealTimeClockLib.inf
> -  EfiResetSystemLib|OpenPlatformPkg/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.inf
> +  ResetSystemLib|OpenPlatformPkg/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.inf

Isn't it better to switch to generic PSCI:
EfiResetSystemLib|ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.inf
and remove MvResetSystemLib whatsoever?

Best regards,
Marcin

>
>    # Network support
>    NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
> @@ -416,7 +416,7 @@
>    MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf
>
>    EmbeddedPkg/EmbeddedMonotonicCounter/EmbeddedMonotonicCounter.inf
> -  EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
>    EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
>
> diff --git a/Platforms/Marvell/Armada/Armada70x0.fdf b/Platforms/Marvell/Armada/Armada70x0.fdf
> index 280b40be1d39..8091ea260261 100644
> --- a/Platforms/Marvell/Armada/Armada70x0.fdf
> +++ b/Platforms/Marvell/Armada/Armada70x0.fdf
> @@ -99,7 +99,7 @@ FvNameGuid         = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>    INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>    INF MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf
>    INF EmbeddedPkg/EmbeddedMonotonicCounter/EmbeddedMonotonicCounter.inf
> -  INF EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  INF MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
>    INF EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
>    INF OpenPlatformPkg/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
> diff --git a/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.c b/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.c
> index c0787aceac9c..f349c5c5387b 100644
> --- a/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.c
> +++ b/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.c
> @@ -66,51 +66,99 @@ LibResetSystemVirtualNotifyEvent (
>  }
>
>  /**
> -  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 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.
>  **/
> -EFI_STATUS
> +VOID
>  EFIAPI
> -LibResetSystem (
> -  IN EFI_RESET_TYPE   ResetType,
> -  IN EFI_STATUS       ResetStatus,
> -  IN UINTN            DataSize,
> -  IN VOID             *ResetData OPTIONAL
> +ResetCold (
> +  VOID
>    )
>  {
>    UINT32   Data;
>
> -  switch (ResetType) {
> -  case EfiResetCold:
> -  case EfiResetWarm:
> -    Data = MmioRead32 (mAddress);
> -    Data &= ~PcdGet32 (PcdResetRegMask);
> -    MmioWrite32 (mAddress, Data);
> -    break;
> -  case EfiResetShutdown:
> +  Data = MmioRead32 (mAddress);
> +  Data &= ~PcdGet32 (PcdResetRegMask);
> +  MmioWrite32 (mAddress, Data);
> +}
> +
> +/**
> +  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
> +ResetWarm (
> +  VOID
> +  )
> +{
> +  // Map a warm reset into a cold reset
> +  ResetCold ();
> +}
> +
> +/**
> +  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 shutdown reset.
> +**/
> +VOID
> +EFIAPI
> +ResetShutdown (
> +  VOID
> +  )
> +{
>    //
>    // Currently there is no support for power-off platform
>    //
> -    break;
> -  default:
> -    break;
> -  }
> +}
> +
> +/**
> +  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
> +}
>
> -  return EFI_DEVICE_ERROR;
> +/**
> +  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
> +  )
> +{
> +  // Map the platform specific reset as reboot
> +  ResetCold ();
>  }
>
> -EFI_STATUS
> +RETURN_STATUS
>  EFIAPI
>  LibInitializeResetSystem (
> -  IN EFI_HANDLE        ImageHandle,
> -  IN EFI_SYSTEM_TABLE  *SystemTable
> +  VOID
>    )
>  {
>    UINT64 Alignment;
> diff --git a/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.inf b/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.inf
> index 87fff5707909..0d5951d7e9a0 100644
> --- a/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.inf
> +++ b/Platforms/Marvell/Library/ResetSystemLib/MvResetSystemLib.inf
> @@ -1,4 +1,5 @@
>  # Copyright (C) 2016 Marvell International Ltd.
> +# Copyright (C) 2017 Linaro Ltd.
>  #
>  # Marvell BSD License Option
>  #
> @@ -32,16 +33,18 @@
>
>  [Defines]
>    INF_VERSION                    = 0x00010019
> -  BASE_NAME                      = Reset
> +  BASE_NAME                      = MvResetSystemLib
>    FILE_GUID                      = 9d1373c0-6fac-432c-88e7-818744dc45d9
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = EfiResetSystemLib
> +  LIBRARY_CLASS                  = ResetSystemLib
> +  CONSTRUCTOR                    = LibInitializeResetSystem
>
>  [Sources.common]
>    MvResetSystemLib.c
>
>  [Packages]
> +  MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    OpenPlatformPkg/Platforms/Marvell/Marvell.dec
>
> --
> 2.9.3
>