From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::12d; helo=mail-it1-x12d.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x12d.google.com (mail-it1-x12d.google.com [IPv6:2607:f8b0:4864:20::12d]) (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 E40392119C8B2 for ; Wed, 12 Dec 2018 12:43:52 -0800 (PST) Received: by mail-it1-x12d.google.com with SMTP id h193so372023ita.5 for ; Wed, 12 Dec 2018 12:43:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=eQGT2mEfH3h5GBJU5/2/MqoB+BoL5nNOITDenTdodTQ=; b=SnRm8Yj6bVfG+REOBKUuK2faj3+IpbmRmabp/0qubx62dkoon/iEQV+ZpGg4TtH6jX Z1yy7/gkl6x6WIPt+3EtjTxQhf/YnipNbnbcl3y2i+y/ElA9bqqptU4m5voKGTNjzvTh EssybticJD0xd2VwPeXGlrKnxtoT6cdfae4+w= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=eQGT2mEfH3h5GBJU5/2/MqoB+BoL5nNOITDenTdodTQ=; b=jOXlGLrOvJbr5DyVrLND0Ku9NIwePN6VAPIKmGKS8x4fbRyplI0CPN++pyNXdsvSwf KLpWQSCxtwe2kEUT8uGR8zO3Fuel3FrXrARWTnIWRqKmXGgMyVHP1nP5tEP35Gswil2a N2c8cd0KjvzmrvKKvPwV+EDrH8am3y7ZHhMSc0IURoqKl1XQF5jV8fX+tNHnrPxhRakY Bde+Zqugot5CCq887EfXmRQ5yfTwIpb2mrxrrpta1k+zehFDVyfkqqIFzWoraqqAT4Hd ZchfoNQDA4Cb8zwpG7JBmFslRxZXl8XbyzKPeNdS+kxWl1UixJXdS+po9Qv1Pp9Ujfqv 9jNg== X-Gm-Message-State: AA+aEWa+OYIMEtxqINAkoNTvJ8ymgt8QRSutMYikwuvu0f2VKZM5K393 xDeBCjmHSdLxRFYaSE8RAeVKzxbdf+FHTiKf8ggGiw== X-Google-Smtp-Source: AFSGD/WCsp3yZGo2eGunqHPGId2QM8lA3OtMZ0ggeocjtv895qKSzFMMdIsD5iLDW4+V4BOvzxZ90lDZ6Ff3mBtpaVU= X-Received: by 2002:a02:183:: with SMTP id 3mr20910541jak.130.1544647431049; Wed, 12 Dec 2018 12:43:51 -0800 (PST) MIME-Version: 1.0 References: <20181210123853.4864-1-pete@akeo.ie> <20181210123853.4864-2-pete@akeo.ie> In-Reply-To: <20181210123853.4864-2-pete@akeo.ie> From: Ard Biesheuvel Date: Wed, 12 Dec 2018 21:43:39 +0100 Message-ID: To: Pete Batard Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH v2 edk2-platforms 01/20] Platform/Broadcom/RPi3: Add Reset and Memory Init libraries X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Dec 2018 20:43:53 -0000 Content-Type: text/plain; charset="UTF-8" On Mon, 10 Dec 2018 at 13:39, Pete Batard wrote: > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Pete Batard > --- > Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c | 183 ++++++++++++++++++++ > Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf | 51 ++++++ > Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c | 104 +++++++++++ > Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf | 46 +++++ > 4 files changed, 384 insertions(+) > > diff --git a/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c > new file mode 100644 > index 000000000000..81d810b5d428 > --- /dev/null > +++ b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c > @@ -0,0 +1,183 @@ > +/** @file > + * > + * Copyright (c) 2017-2018, Andrey Warkentin > + * Copyright (c) 2011-2015, ARM Limited. All rights reserved. > + * > + * This program and the accompanying materials > + * are licensed and made available under the terms and conditions of the BSD License > + * which accompanies this distribution. The full text of the license may be found at > + * http://opensource.org/licenses/bsd-license.php > + * > + * THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + * WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + * > + **/ > + > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +extern UINT64 mSystemMemoryEnd; > + > +VOID > +BuildMemoryTypeInformationHob ( > + VOID > + ); > + > +STATIC > +VOID > +InitMmu ( > + IN ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable > + ) > +{ > + > + VOID *TranslationTableBase; > + UINTN TranslationTableSize; You can drop these > + RETURN_STATUS Status; > + > + //Note: Because we called PeiServicesInstallPeiMemory() before to call InitMmu() the MMU Page Table resides in > + // DRAM (even at the top of DRAM as it is the first permanent memory allocation) > + Status = ArmConfigureMmu (MemoryTable, &TranslationTableBase, &TranslationTableSize); ... given that they are only passed here, and are actually OPTIONAL > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Error: Failed to enable MMU\n")); > + } > +} > + > +STATIC > +VOID > +AddAndRTSData(ARM_MEMORY_REGION_DESCRIPTOR *Desc) What does AddAnd mean? Can we improve the naming in the file? Also, please use a space before ( > +{ > + BuildResourceDescriptorHob ( > + EFI_RESOURCE_SYSTEM_MEMORY, > + EFI_RESOURCE_ATTRIBUTE_PRESENT | > + EFI_RESOURCE_ATTRIBUTE_INITIALIZED | > + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE | > + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE | > + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | > + EFI_RESOURCE_ATTRIBUTE_TESTED, > + Desc->PhysicalBase, > + Desc->Length > + ); > + > + BuildMemoryAllocationHob ( > + Desc->PhysicalBase, > + Desc->Length, > + EfiRuntimeServicesData > + ); > +} > + > +STATIC > +VOID > +AddAndReserved(ARM_MEMORY_REGION_DESCRIPTOR *Desc) > +{ > + BuildResourceDescriptorHob ( > + EFI_RESOURCE_SYSTEM_MEMORY, > + EFI_RESOURCE_ATTRIBUTE_PRESENT | > + EFI_RESOURCE_ATTRIBUTE_INITIALIZED | > + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE | > + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE | > + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | > + EFI_RESOURCE_ATTRIBUTE_TESTED, > + Desc->PhysicalBase, > + Desc->Length > + ); > + > + BuildMemoryAllocationHob ( > + Desc->PhysicalBase, > + Desc->Length, > + EfiReservedMemoryType > + ); > +} > + > +STATIC > +VOID > +AddAndMmio(ARM_MEMORY_REGION_DESCRIPTOR *Desc) > +{ > + BuildResourceDescriptorHob ( > + EFI_RESOURCE_SYSTEM_MEMORY, > + (EFI_RESOURCE_ATTRIBUTE_PRESENT | > + EFI_RESOURCE_ATTRIBUTE_INITIALIZED | > + EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE | > + EFI_RESOURCE_ATTRIBUTE_TESTED), > + Desc->PhysicalBase, > + Desc->Length > + ); > + > + BuildMemoryAllocationHob ( > + Desc->PhysicalBase, > + Desc->Length, > + EfiMemoryMappedIO > + ); > +} > + > +/*++ > + > +Routine Description: > + > + > + > +Arguments: > + > + FileHandle - Handle of the file being invoked. > + PeiServices - Describes the list of possible PEI Services. > + > +Returns: > + > + Status - EFI_SUCCESS if the boot mode could be set > + > +--*/ > +EFI_STATUS > +EFIAPI > +MemoryPeim ( > + IN EFI_PHYSICAL_ADDRESS UefiMemoryBase, > + IN UINT64 UefiMemorySize > + ) > +{ > + ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable; > + > + // Get Virtual Memory Map from the Platform Library > + ArmPlatformGetVirtualMemoryMap (&MemoryTable); > + > + // Ensure PcdSystemMemorySize has been set > + ASSERT (PcdGet64 (PcdSystemMemorySize) != 0); > + This function refers to array entries by index, which is a bit nasty. It would be better just to take the contents of ArmPlatformGetVirtualMemoryMap() and move them into this file. > + // FD without variable store > + AddAndReserved(&MemoryTable[0]); > + > + // Variable store. > + AddAndRTSData(&MemoryTable[1]); > + > + // Trusted Firmware region > + AddAndReserved(&MemoryTable[2]); > + > + // Usable memory. > + BuildResourceDescriptorHob ( > + EFI_RESOURCE_SYSTEM_MEMORY, > + EFI_RESOURCE_ATTRIBUTE_PRESENT | > + EFI_RESOURCE_ATTRIBUTE_INITIALIZED | > + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE | > + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE | > + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | > + EFI_RESOURCE_ATTRIBUTE_TESTED, > + MemoryTable[3].PhysicalBase, > + MemoryTable[3].Length > + ); > + > + AddAndReserved(&MemoryTable[4]); > + AddAndMmio(&MemoryTable[5]); > + Drop the last one. MMIO is not system memory, so it does not belong in the memory map (unless it requires a runtime mapping, but that is up to the DXE runtime driver) > + // Build Memory Allocation Hob > + InitMmu (MemoryTable); > + > + if (FeaturePcdGet (PcdPrePiProduceMemoryTypeInformationHob)) { > + // Optional feature that helps prevent EFI memory map fragmentation. > + BuildMemoryTypeInformationHob (); > + } > + > + return EFI_SUCCESS; > +} > diff --git a/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf > new file mode 100644 > index 000000000000..9f5204a210de > --- /dev/null > +++ b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf > @@ -0,0 +1,51 @@ > +#/** @file > +# > +# Copyright (c) 2016, Linaro, Ltd. All rights reserved. > +# Copyright (c) 2011-2014, ARM Ltd. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = MemoryInitPeiLib > + FILE_GUID = 4bbc9c10-a100-43fb-8311-332ba497d1b4 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = MemoryInitPeiLib|SEC PEIM > + > +[Sources] > + MemoryInitPeiLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + ArmPkg/ArmPkg.dec > + ArmPlatformPkg/ArmPlatformPkg.dec > + > +[LibraryClasses] > + DebugLib > + HobLib > + ArmMmuLib > + ArmPlatformLib > + > +[Guids] > + gEfiMemoryTypeInformationGuid > + > +[FeaturePcd] > + gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob > + > +[FixedPcd] > + gArmTokenSpaceGuid.PcdSystemMemoryBase > + gArmTokenSpaceGuid.PcdSystemMemorySize > + > +[Depex] > + TRUE > diff --git a/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c b/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c > new file mode 100644 > index 000000000000..1a3944b71d03 > --- /dev/null > +++ b/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c > @@ -0,0 +1,104 @@ > +/** @file > + * > + * Support ResetSystem Runtime call using PSCI calls. > + * Signals the gRaspberryPiEventResetGuid event group on reset. > + * > + * Copyright (c) 2018, Andrei Warkentin > + * Copyright (c) 2014, Linaro Ltd. All rights reserved. > + * Copyright (c) 2013-2015, ARM Ltd. All rights reserved. > + * Copyright (c) 2008-2009, Apple Inc. All rights reserved. > + * > + * This program and the accompanying materials > + * are licensed and made available under the terms and conditions of the BSD License > + * which accompanies this distribution. The full text of the license may be found at > + * http://opensource.org/licenses/bsd-license.php > + * > + * THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + * WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + * > + **/ > + > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +/** > + 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. > + > +**/ > +EFI_STATUS > +EFIAPI > +LibResetSystem ( > + IN EFI_RESET_TYPE ResetType, > + IN EFI_STATUS ResetStatus, > + IN UINTN DataSize, > + IN CHAR16 *ResetData OPTIONAL > + ) > +{ > + ARM_SMC_ARGS ArmSmcArgs; > + > + if (!EfiAtRuntime ()) { > + /* > + * Only if still in UEFI. > + */ > + EfiEventGroupSignal(&gRaspberryPiEventResetGuid); > + } Please drop this module entirely, and use the notification functionality provided by MdeModulePkg/Universal/ResetSystemRuntimeDxe (and use the generic PSCI library) > + > + switch (ResetType) { > + case EfiResetPlatformSpecific: > + // Map the platform specific reset as reboot > + case EfiResetWarm: > + // Map a warm reset into a cold reset > + case EfiResetCold: > + // Send a PSCI 0.2 SYSTEM_RESET command > + ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET; > + break; > + case EfiResetShutdown: > + // Send a PSCI 0.2 SYSTEM_OFF command > + ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF; > + break; > + default: > + ASSERT (FALSE); > + return EFI_UNSUPPORTED; > + } > + > + ArmCallSmc (&ArmSmcArgs); > + > + // We should never be here > + DEBUG ((DEBUG_ERROR, "%a: PSCI Reset failed\n", __FUNCTION__)); > + CpuDeadLoop (); > + return EFI_UNSUPPORTED; > +} > + > +/** > + 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. > + > +**/ > +EFI_STATUS > +EFIAPI > +LibInitializeResetSystem ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + return EFI_SUCCESS; > +} > diff --git a/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf b/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf > new file mode 100644 > index 000000000000..1c32a4b08162 > --- /dev/null > +++ b/Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf > @@ -0,0 +1,46 @@ > +#/** @file > +# > +# Reset System lib using PSCI hypervisor or secure monitor calls. > +# Signals the gRaspberryPiEventResetGuid event group on reset. > +# > +# Copyright (c) 2018, Andrei Warkentin > +# Copyright (c) 2014, Linaro Ltd. All rights reserved. > +# Copyright (c) 2014, ARM Ltd. All rights reserved. > +# Copyright (c) 2008, Apple Inc. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = ResetLib > + FILE_GUID = B9F59B69-A105-41C7-8F5A-2C60DD7FD7AB > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = EfiResetSystemLib > + > +[Sources] > + ResetLib.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + MdePkg/MdePkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + Platform/Broadcom/Bcm283x/RaspberryPiPkg.dec > + > +[LibraryClasses] > + DebugLib > + BaseLib > + ArmSmcLib > + UefiLib > + UefiRuntimeLib > + > +[Guids] > + gRaspberryPiEventResetGuid > -- > 2.17.0.windows.1 >