From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web10.2995.1587633358525059594 for ; Thu, 23 Apr 2020 02:15:58 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=th7VBBXa; spf=pass (domain: nuviainc.com, ip: 209.85.128.66, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f66.google.com with SMTP id e26so5608177wmk.5 for ; Thu, 23 Apr 2020 02:15:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=3paCF2DnH/HC/67vXeMtWXZOR7Cd8kajrfeAQ2oc688=; b=th7VBBXa4RTX62UUfnfSTf6/vtzKAQcMxy/fUyISgHIW83s64Gtsa78hiw3PXbpacn 3BLHAE7mT+Pr99jBxkiLgKIhL5TRpwLAfpDiZ70Qws+DxS+3foibeSeJfXCtlWoSuXKQ xJeTC/Pqu7NDrRFLaTLVyQGFpP7fF2zDl0pRnbEa/+2esySRtX0aEbHZVqvLfOsUonoi khYaNoQADgBnzQqXMoDqut+qBdfVL0Icfrnyg+Ar1WXonxW1D4dXqPVtAoE5uYjCe1U0 0QewiUuJ9hxCrMxV8QXv/L6ORggigBwRiS50H9xOvC6FzeMUuhPfTdPRJL7fRO8tuX4w /PHA== 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=3paCF2DnH/HC/67vXeMtWXZOR7Cd8kajrfeAQ2oc688=; b=m+Xvy/+Xb2CPDsByQ9J3EkqgYnRcF4W4K6fQA5sCBVaYOAvhpaanptA4EiMUK4qbcm DeHBT87EjZCTWC5jFM+gcNS7u88dDmaq3/xE7IExRM16DF9PpxeX1qqG0kZdDcfx6kc1 3VusDnuNknJfVnhvX0WO2E/youfy9OZxi4XuuV9x0tjiB3EH5QHzzAC3DAFSKohawnm3 1DuP4SWu3dhulJhjj7Pl0eIoPFiySm4mCgwLQyPmJ3xy8ZLA/+LgdpofrqUeijZtz5P5 NncgiNK1sUciNO0r2QZUd5YfmZPrl6bK7V2D84mcYadz8oRDCPqqSGOxtgwYl1wDtJoh 2kgQ== X-Gm-Message-State: AGi0PuaeojvY2Tf53/rLNDiaLzxJHhwaVfidmaN2VJcHuyE/9ejQmZWv vsenDNaCanz9HZR2XGcuIVhUig== X-Google-Smtp-Source: APiQypIe53pJMc+i01H6u5nFXt2v+MeELgm+hvOAR1EWgzP8mCvpGG+QoRS8/Gfbn+W/aTTOQ3CRkw== X-Received: by 2002:a1c:23d4:: with SMTP id j203mr3233666wmj.49.1587633356985; Thu, 23 Apr 2020 02:15:56 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id p7sm2906575wrf.31.2020.04.23.02.15.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Apr 2020 02:15:56 -0700 (PDT) Date: Thu, 23 Apr 2020 10:15:54 +0100 From: "Leif Lindholm" To: Pankaj Bansal Cc: Meenakshi Aggarwal , Michael D Kinney , devel@edk2.groups.io, Varun Sethi , Samer El-Haj-Mahmoud , Jon Nettleton , Ard Biesheuvel Subject: Re: [PATCH edk2-platforms v3 12/24] Silicon/NXP: Move RAM retrieval from SocLib Message-ID: <20200423091554.GR14075@vanye> References: <20200415121342.9246-1-pankaj.bansal@oss.nxp.com> <20200415121342.9246-13-pankaj.bansal@oss.nxp.com> MIME-Version: 1.0 In-Reply-To: <20200415121342.9246-13-pankaj.bansal@oss.nxp.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Apr 15, 2020 at 17:43:30 +0530, Pankaj Bansal wrote: > From: Pankaj Bansal > > RAM retrieval using SMC commands is common to all Layerscape SOCs. > Therefore, move it to commom MemoryInit Pei Lib. > > Signed-off-by: Pankaj Bansal > --- > > Notes: > - sort headers alphabetically > - Moved DRAM region retrieval and Total DRAM size retrieval to separate > functions > - Fixed MemoryPeim function description > - Modified check on FoundSystemMem = TRUE to check the RAM region against > MemoryPeim function input arguments UefiMemoryBase and UefiMemorySize > - (!DramRegions[Index].Size) => (DramRegions[Index].Size == 0) > - (FoundSystemMem) => (FoundSystemMem == TRUE) > - Added explaination for starting for loop from the last DRAM region > > Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf | 7 +- > Silicon/NXP/Library/SocLib/LS1043aSocLib.inf | 1 - > Silicon/NXP/Include/DramInfo.h | 38 ---- > Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h | 25 +++ > Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c | 196 ++++++++++++++++---- > Silicon/NXP/Library/SocLib/Chassis.c | 67 ------- > 6 files changed, 188 insertions(+), 146 deletions(-) > > diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf > index a5bd39415def..ad2371115b17 100644 > --- a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf > +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.inf > @@ -18,7 +18,6 @@ > [Sources] > MemoryInitPeiLib.c > > - > [Packages] > ArmPkg/ArmPkg.dec > ArmPlatformPkg/ArmPlatformPkg.dec > @@ -30,6 +29,7 @@ > [LibraryClasses] > ArmMmuLib > ArmPlatformLib > + ArmSmcLib > DebugLib > HobLib > PcdLib > @@ -40,6 +40,11 @@ > [FeaturePcd] > gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob > > +[FixedPcd] > + gArmTokenSpaceGuid.PcdFdBaseAddress > + gArmTokenSpaceGuid.PcdFdSize > + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize > + > [Pcd] > gArmTokenSpaceGuid.PcdSystemMemoryBase > gArmTokenSpaceGuid.PcdSystemMemorySize > diff --git a/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf b/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf > index b7c7fc78cc8f..99d89498e0e2 100644 > --- a/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf > +++ b/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf > @@ -20,7 +20,6 @@ > Silicon/NXP/NxpQoriqLs.dec > > [LibraryClasses] > - ArmSmcLib > BaseLib > DebugLib > IoAccessLib > diff --git a/Silicon/NXP/Include/DramInfo.h b/Silicon/NXP/Include/DramInfo.h > deleted file mode 100644 > index a934aaeff1f5..000000000000 > --- a/Silicon/NXP/Include/DramInfo.h > +++ /dev/null > @@ -1,38 +0,0 @@ > -/** @file > -* Header defining the structure for Dram Information > -* > -* Copyright 2019 NXP > -* > -* SPDX-License-Identifier: BSD-2-Clause-Patent > -* > -**/ > - > -#ifndef DRAM_INFO_H_ > -#define DRAM_INFO_H_ > - > -#include > - > -#define SMC_DRAM_BANK_INFO (0xC200FF12) > - > -typedef struct { > - UINTN BaseAddress; > - UINTN Size; > -} DRAM_REGION_INFO; > - > -typedef struct { > - UINT32 NumOfDrams; > - UINT32 Reserved; > - DRAM_REGION_INFO DramRegion[3]; > -} DRAM_INFO; > - > -EFI_STATUS > -GetDramBankInfo ( > - IN OUT DRAM_INFO *DramInfo > - ); > - > -VOID > -UpdateDpaaDram ( > - IN OUT DRAM_INFO *DramInfo > - ); > - > -#endif /* DRAM_INFO_H_ */ > diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h > new file mode 100644 > index 000000000000..edbf0ceaf638 > --- /dev/null > +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h > @@ -0,0 +1,25 @@ > +/** @file > +* > +* Copyright 2020 NXP > +* > +* SPDX-License-Identifier: BSD-2-Clause-Patent > +* > +**/ > + > +#ifndef MEMORY_INIT_PEI_LIB_H_ > +#define MEMORY_INIT_PEI_LIB_H_ > + > +#include > + > +// Specifies the Maximum regions onto which DDR memory can be mapped in > +// a Platform > +#define MAX_DRAM_REGIONS 3 > +#define SMC_DRAM_BANK_INFO (0xC200FF12) > + > +typedef struct { > + UINTN BaseAddress; > + UINTN Size; > +} DRAM_REGION_INFO; > + > +#endif // MEMORY_INIT_PEI_LIB_H_ > + > diff --git a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c > index 3ea773678667..ea3e7d59532e 100644 > --- a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c > +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c > @@ -12,13 +12,15 @@ > > #include > #include > +#include > #include > #include > #include > #include > #include > > -#include > +#include "MemoryInitPeiLib.h" > + > > VOID > BuildMemoryTypeInformationHob ( > @@ -44,22 +46,79 @@ InitMmu ( > } > } > > -/*++ > +STATIC > +UINTN > +GetDramSize ( > + IN VOID > + ) > +{ > + ARM_SMC_ARGS ArmSmcArgs; > > -Routine Description: > + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO; > + ArmSmcArgs.Arg1 = -1; > > + ArmCallSmc (&ArmSmcArgs); > > + if (ArmSmcArgs.Arg0) { Breaking this up into helper functions has improved readability a lot, but this one could do with some more clarification. Can we have a #define with a descriptive name to explain better than ArmSmcArgs.Arg0 != 0 why we should return 0? > + return 0; > + } else { > + return ArmSmcArgs.Arg1; > + } > +} > > -Arguments: > +STATIC > +EFI_STATUS > +GetDramRegionsInfo ( > + OUT DRAM_REGION_INFO *DramRegions, > + IN UINT32 NumRegions > + ) > +{ > + ARM_SMC_ARGS ArmSmcArgs; > + UINT32 Index; > + UINTN RemainingDramSize; > + UINTN BaseAddress; > + UINTN Size; > > - FileHandle - Handle of the file being invoked. > - PeiServices - Describes the list of possible PEI Services. > + RemainingDramSize = GetDramSize (); > + DEBUG ((DEBUG_INFO, "DRAM Total Size 0x%lx \n", RemainingDramSize)); > > -Returns: > + // Ensure Total Dram Size is valid > + ASSERT (RemainingDramSize != 0); > > - Status - EFI_SUCCESS if the boot mode could be set > + for (Index = 0; (RemainingDramSize != 0) && (Index < NumRegions); Index++) { Do we *need* to check for RemainingDramSize in the loop condition? Would it not be better to process all regions and flag an error if RemainingDramSize == 0 is not reached? Hmm, right, so NumRegions is really just the size of our data structure - but how about something like: for (Index = 0; Index < NumRegions; Index++) { ... if (RemainingDramSize == 0) { return EFI_SUCCESS; } } DEBUG ((DEBUG_ERROR, ... return EFI_BUFFER_TOO_SMALL; ? > + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO; > + ArmSmcArgs.Arg1 = Index; > > ---*/ > + ArmCallSmc (&ArmSmcArgs); > + > + BaseAddress = ArmSmcArgs.Arg1; > + Size = ArmSmcArgs.Arg2; > + ASSERT (BaseAddress && Size); > + > + DramRegions[Index].BaseAddress = BaseAddress; > + DramRegions[Index].Size = Size; > + RemainingDramSize -= Size; > + > + DEBUG ((DEBUG_INFO, "DRAM Region[%d]: start 0x%lx, size 0x%lx\n", > + Index, BaseAddress, Size)); > + } > + > + // Ensure that all regions have been accounted for > + ASSERT (RemainingDramSize == 0); > + > + return EFI_SUCCESS; > +} > + > +/** > + Get the installed RAM information. > + Initialize MMU and Memory HOBs (Resource Descriptor HOBs) > + > + @param[in] UefiMemoryBase Base address of region used by UEFI in > + permanent memory > + @param[in] UefiMemorySize Size of the region used by UEFI in permanent memory > + > + @return EFI_SUCCESS Successfuly Initialize MMU and Memory HOBs. > +**/ > EFI_STATUS > EFIAPI > MemoryPeim ( > @@ -67,11 +126,16 @@ MemoryPeim ( > IN UINT64 UefiMemorySize > ) > { > - ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable; > - EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; > - EFI_PEI_HOB_POINTERS NextHob; > - BOOLEAN Found; > - DRAM_INFO DramInfo; > + ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable; > + INT32 Index; > + UINTN BaseAddress; > + UINTN Size; > + UINTN Top; > + DRAM_REGION_INFO DramRegions[MAX_DRAM_REGIONS]; > + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; > + UINTN FdBase; > + UINTN FdTop; > + BOOLEAN FoundSystemMem; > > // Get Virtual Memory Map from the Platform Library > ArmPlatformGetVirtualMemoryMap (&MemoryTable); > @@ -94,40 +158,94 @@ MemoryPeim ( > EFI_RESOURCE_ATTRIBUTE_TESTED > ); > > - if (GetDramBankInfo (&DramInfo)) { > - DEBUG ((DEBUG_ERROR, "Failed to get DRAM information, exiting...\n")); > - return EFI_UNSUPPORTED; > - } > + FoundSystemMem = FALSE; > + ZeroMem (DramRegions, sizeof (DramRegions)); > > - while (DramInfo.NumOfDrams--) { > - // > - // Check if the resource for the main system memory has been declared > - // > - Found = FALSE; > - NextHob.Raw = GetHobList (); > - while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, NextHob.Raw)) != NULL) { > - if ((NextHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) && > - (DramInfo.DramRegion[DramInfo.NumOfDrams].BaseAddress >= NextHob.ResourceDescriptor->PhysicalStart) && > - (NextHob.ResourceDescriptor->PhysicalStart + NextHob.ResourceDescriptor->ResourceLength <= > - DramInfo.DramRegion[DramInfo.NumOfDrams].BaseAddress + DramInfo.DramRegion[DramInfo.NumOfDrams].Size)) > - { > - Found = TRUE; > - break; > + GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions)); This call implicitly discards the return code. I don't think that's necessarily the wrong decision here, but it would be more clear (and prevent compilers and code analysis tools from objecting) if explicitly casting it away with (VOID). > + > + FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress); > + FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize); > + > + // Declare memory regios to system regions > + // The DRAM region info is sorted based on the RAM address is SOC memory map. > + // i.e. DramRegions[0] is at lower address, as compared to DramRegions[1]. > + // The goal to start from last region is to find the topmost RAM region that > + // can contain UEFI DXE region i.e. PcdSystemMemoryUefiRegionSize. > + // If UEFI were to allocate any reserved or runtime region, it would be > + // allocated from topmost RAM region. > + // This ensures that maximum amount of lower RAM (32 bit addresses) are left > + // for OS to allocate to devices that can only work with 32bit physical > + // addresses. E.g. legacy devices that need to DMA to 32bit addresses. Excellent, thanks! / Leif > + for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) { > + if (DramRegions[Index].Size == 0) { > + continue; > + } > + > + BaseAddress = DramRegions[Index].BaseAddress; > + Top = DramRegions[Index].BaseAddress + DramRegions[Index].Size; > + > + // EDK2 does not have the concept of boot firmware copied into DRAM. > + // To avoid the DXE core to overwrite this area we must create a memory > + // allocation HOB for the region, but this only works if we split off the > + // underlying resource descriptor as well. > + if (FdBase >= BaseAddress && FdTop <= Top) { > + // Update Size > + Size = FdBase - BaseAddress; > + if (Size) { > + BuildResourceDescriptorHob ( > + EFI_RESOURCE_SYSTEM_MEMORY, > + ResourceAttributes, > + BaseAddress, > + Size > + ); > } > - NextHob.Raw = GET_NEXT_HOB (NextHob); > - } > - > - if (!Found) { > - // Reserved the memory space occupied by the firmware volume > + // create the System Memory HOB for the firmware > BuildResourceDescriptorHob ( > EFI_RESOURCE_SYSTEM_MEMORY, > ResourceAttributes, > - DramInfo.DramRegion[DramInfo.NumOfDrams].BaseAddress, > - DramInfo.DramRegion[DramInfo.NumOfDrams].Size > + FdBase, > + PcdGet32 (PcdFdSize) > ); > + // Create the System Memory HOB for the remaining region (top of the FD)s > + Size = Top - FdTop; > + if (Size) { > + BuildResourceDescriptorHob ( > + EFI_RESOURCE_SYSTEM_MEMORY, > + ResourceAttributes, > + FdTop, > + Size > + ); > + }; > + // Mark the memory covering the Firmware Device as boot services data > + BuildMemoryAllocationHob (FixedPcdGet64 (PcdFdBaseAddress), > + FixedPcdGet32 (PcdFdSize), > + EfiBootServicesData); > + } else { > + BuildResourceDescriptorHob ( > + EFI_RESOURCE_SYSTEM_MEMORY, > + ResourceAttributes, > + DramRegions[Index].BaseAddress, > + DramRegions[Index].Size > + ); > + } > + > + if (FoundSystemMem == TRUE) { > + continue; > + } > + > + Size = DramRegions[Index].Size; > + > + if (FdBase >= BaseAddress && FdTop <= Top) { > + Size -= (UINTN)FixedPcdGet32 (PcdFdSize); > + } > + > + if ((UefiMemoryBase >= BaseAddress) && (Size >= UefiMemorySize)) { > + FoundSystemMem = TRUE; > } > } > > + ASSERT (FoundSystemMem == TRUE); > + > // Build Memory Allocation Hob > InitMmu (MemoryTable); > > diff --git a/Silicon/NXP/Library/SocLib/Chassis.c b/Silicon/NXP/Library/SocLib/Chassis.c > index 847331a63152..1ef99e8de25f 100644 > --- a/Silicon/NXP/Library/SocLib/Chassis.c > +++ b/Silicon/NXP/Library/SocLib/Chassis.c > @@ -22,7 +22,6 @@ > #include > #include > > -#include > #include "NxpChassis.h" > > UINT32 > @@ -75,69 +74,3 @@ SmmuInit ( > MmioWrite32 ((UINTN)SMMU_REG_NSCR0, Value); > } > > -UINTN > -GetDramSize ( > - IN VOID > - ) > -{ > - ARM_SMC_ARGS ArmSmcArgs; > - > - ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO; > - ArmSmcArgs.Arg1 = -1; > - > - ArmCallSmc (&ArmSmcArgs); > - > - if (ArmSmcArgs.Arg0) { > - return 0; > - } else { > - return ArmSmcArgs.Arg1; > - } > -} > - > -EFI_STATUS > -GetDramBankInfo ( > - IN OUT DRAM_INFO *DramInfo > - ) > -{ > - ARM_SMC_ARGS ArmSmcArgs; > - UINT32 I; > - UINTN DramSize; > - > - DramSize = GetDramSize (); > - DEBUG ((DEBUG_INFO, "DRAM Total Size 0x%lx \n", DramSize)); > - > - // Ensure DramSize has been set > - ASSERT (DramSize != 0); > - > - I = 0; > - > - do { > - ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO; > - ArmSmcArgs.Arg1 = I; > - > - ArmCallSmc (&ArmSmcArgs); > - if (ArmSmcArgs.Arg0) { > - if (I > 0) { > - break; > - } else { > - ASSERT (ArmSmcArgs.Arg0 == 0); > - } > - } > - > - DramInfo->DramRegion[I].BaseAddress = ArmSmcArgs.Arg1; > - DramInfo->DramRegion[I].Size = ArmSmcArgs.Arg2; > - > - DramSize -= DramInfo->DramRegion[I].Size; > - > - DEBUG ((DEBUG_INFO, "bank[%d]: start 0x%lx, size 0x%lx\n", > - I, DramInfo->DramRegion[I].BaseAddress, DramInfo->DramRegion[I].Size)); > - > - I++; > - } while (DramSize); > - > - DramInfo->NumOfDrams = I; > - > - DEBUG ((DEBUG_INFO, "Number Of DRAM in system %d \n", DramInfo->NumOfDrams)); > - > - return EFI_SUCCESS; > -} > -- > 2.17.1 >