From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22e.google.com (mail-wm0-x22e.google.com [IPv6:2a00:1450:400c:c09::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E5ACE21CF3B92 for ; Mon, 3 Jul 2017 14:30:43 -0700 (PDT) Received: by mail-wm0-x22e.google.com with SMTP id f67so66575200wmh.1 for ; Mon, 03 Jul 2017 14:32:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=wBDqKeFDYZ4pop4/BT/Z8nSf0U1RLSgHH6RvoeMJ5f0=; b=PWPfV9JjLbexXzpsduPV/oZdkFlsvUgZYTA1glU3iDFO7nigAKzX1Hm2A9ef4Dv6tF eHA8UbJ1QWe+Z3WYGM9FeKJ88JCb/0Ewefuc6Nk78iO4MEpTkEG57vdGqyfdV4yJZwqm DKLIvA1eyywT+NNQzyJoU0AaCto7GUUjAMjwk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=wBDqKeFDYZ4pop4/BT/Z8nSf0U1RLSgHH6RvoeMJ5f0=; b=ASGjzktWOSJgXG3FHPEqYT2spzTFEWCsnIT6sB0kDYUK1yZghZxtCUT1vTLAcxSzUb 1qouIwj7/62GBAEPRp41xTTOULUXD+aztekes/MnJRHatnPuqIBwaQYGERe8KOlMzWD5 DWxQj4c4AqTtTFWYwsHDitFX9+ajqDgRjHkA6/wFzVi1KGf6NUdkNOaHA7EXw5m0/nRO dM0zEDLx3inqug2RsjC6V61qOFSqf6sIA7l452+2X7jdR5wA+Y4V1fXJwBdIK9ENJDtC 0IMK082gO8kg2FMXZsIL+LBewn9RHptxI9pQW0Jq43Cd15W06eynezS3IxcNchsGvYIA PCSg== X-Gm-Message-State: AKS2vOwbAlyqf+odu4Jbb7C3kI7PROOP6uGR/5cTvMUb+xJR6TQuGreR B5/lHqg0pePs9d30L4SyTw== X-Received: by 10.28.175.17 with SMTP id y17mr15582571wme.100.1499117539822; Mon, 03 Jul 2017 14:32:19 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id i131sm22042258wmf.0.2017.07.03.14.32.18 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 03 Jul 2017 14:32:19 -0700 (PDT) Date: Mon, 3 Jul 2017 22:32:17 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" Message-ID: <20170703213217.GL26676@bivouac.eciton.net> References: <20170703162531.27349-1-leif.lindholm@linaro.org> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH] BeagleBoardPkg: switch to use MdeModulePkg ResetSystemLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jul 2017 21:30:44 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Jul 03, 2017 at 06:33:50PM +0100, Ard Biesheuvel wrote: > On 3 July 2017 at 17:25, Leif Lindholm 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 > > --- > > > > 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.
> > + Copyright (c) 2017, Linaro Ltd. All rights reserved.
> > > > 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 > > > > -#include > > -#include > > -#include > > #include > > -#include > > -#include > > -#include > > +#include > > > > #include > > > > +/** > > + 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.
> > +# Copyright (c) 2017, Linaro Ltd. All rights reserved.
> > # > > # 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 Thanks. Fix folded in. Pushed as fb5a64de3a. > > [Pcd] > > gArmTokenSpaceGuid.PcdFvBaseAddress > > -- > > 2.11.0 > >