From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web10.5573.1585744979723033032 for ; Wed, 01 Apr 2020 05:43:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=yDP405u8; spf=pass (domain: nuviainc.com, ip: 209.85.128.67, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f67.google.com with SMTP id c81so6411236wmd.4 for ; Wed, 01 Apr 2020 05:42:59 -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=ctwm6L117hbNJbja+EfmlXRQKsVd4tca7DwJ72o0iUI=; b=yDP405u8k5wR2H2DyhVRU3FfN7PfBLH2yB/7LpXj1nsFy0iQoLWGjn3ubwB6Mcm+TP Du4j+ta7vl05DLfuK/CQIsoehCWI/Qu7uiI3vwyghZUMEzvvrED0ZeFiYlkobV19ODmd rWBUs/oaBEBPBjbZgo/qAiRtS7gSOFYWsCbkxx/O9uHRMjf3KnaMzSSkjB5qNWvT3d2g gDGF4XOycnM51pq7xneDI9oET7E/gJM6U2PPsQWiWg9w2/AjS64tP0tICVoxcLChAYXj D5nXyrXsM73TWcx7FFwkCtQ2N69GQBSb/N6W1ttxMEOPaiWHrDUtHJFemKv5sGiBgGNu zqlw== 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=ctwm6L117hbNJbja+EfmlXRQKsVd4tca7DwJ72o0iUI=; b=Iy6Kds5E6ykbZTUjlbaOyCH0raUBGpXp717YF7Lr0GLiZvIotSSbhpwnOsm27NL4HX n84+3qfxriDHSfi6ttWUtHmfQCjXAU+6o71J3n3zge09l01UavfU9V8FrsN8YT3Q5b0A x98PIUrphTJp7ft/utAi2kUkaRCKXoohF37uxq8uUAyZcT9p7QyoK9gHQN4CmTNWrkU0 MwWjWygs7rAX2wno4kPR/CDBCbifLNJD0fsL0Q1UumxXtdaASzBU0XX6+77gBG2XU6vX KlWXrhqKucBHOvzRJHWQi0yhe5fM3AIXprg+esnUZm9v8wxWZlP6gc0tRJwHJUoQeqHO XH1Q== X-Gm-Message-State: AGi0PuZkpcsiSj8g3+Cw5pnlMaq7zKFcgpCOjYJvaFE9p/dU+iIFNzYe QzZrh0J+2H8wY+XJVh7PR/60Rg== X-Google-Smtp-Source: APiQypJvE92wdDlG8p3hkLOq3eDHvTAhHXgESfmx2PmCeKOpSO6/vhcXfp8rIenpBFQBpnnZhPi2xw== X-Received: by 2002:a1c:f407:: with SMTP id z7mr4030195wma.36.1585744978187; Wed, 01 Apr 2020 05:42:58 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id f9sm2853712wrs.36.2020.04.01.05.42.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Apr 2020 05:42:57 -0700 (PDT) Date: Wed, 1 Apr 2020 13:42:55 +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 Subject: Re: [PATCH v2 15/28] Silicon/NXP: Move RAM retrieval from SocLib Message-ID: <20200401124255.GW7468@vanye> References: <20200320143543.18615-1-pankaj.bansal@oss.nxp.com> <20200320143543.18615-16-pankaj.bansal@oss.nxp.com> MIME-Version: 1.0 In-Reply-To: <20200320143543.18615-16-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 Fri, Mar 20, 2020 at 20:05: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 > --- > Silicon/NXP/Include/DramInfo.h | 38 ----- > .../Library/MemoryInitPei/MemoryInitPeiLib.c | 137 ++++++++++++++---- > .../Library/MemoryInitPei/MemoryInitPeiLib.h | 25 ++++ > .../MemoryInitPei/MemoryInitPeiLib.inf | 7 +- > Silicon/NXP/Library/SocLib/Chassis.c | 67 --------- > Silicon/NXP/Library/SocLib/LS1043aSocLib.inf | 1 - > 6 files changed, 140 insertions(+), 135 deletions(-) > delete mode 100644 Silicon/NXP/Include/DramInfo.h > create mode 100644 Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.h > > 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.c b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c > index 3ea773678667..54d026ef1270 100644 > --- a/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c > +++ b/Silicon/NXP/Library/MemoryInitPei/MemoryInitPeiLib.c > @@ -17,8 +17,10 @@ > #include > #include > #include > +#include Please insert alpabetically sorted. > + > +#include "MemoryInitPeiLib.h" > > -#include > > VOID > BuildMemoryTypeInformationHob ( > @@ -68,10 +70,17 @@ MemoryPeim ( > ) > { > ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable; > + ARM_SMC_ARGS ArmSmcArgs; > + INT32 Index; > + UINTN DramSize; > + UINTN BaseAddress; > + UINTN Size; > + UINTN Top; > + DRAM_REGION_INFO DramRegions[MAX_DRAM_REGIONS]; > EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; > - EFI_PEI_HOB_POINTERS NextHob; > - BOOLEAN Found; > - DRAM_INFO DramInfo; > + UINTN FdBase; > + UINTN FdTop; > + BOOLEAN FoundSystemMem; > > // Get Virtual Memory Map from the Platform Library > ArmPlatformGetVirtualMemoryMap (&MemoryTable); > @@ -94,40 +103,112 @@ MemoryPeim ( > EFI_RESOURCE_ATTRIBUTE_TESTED > ); > > - if (GetDramBankInfo (&DramInfo)) { > - DEBUG ((DEBUG_ERROR, "Failed to get DRAM information, exiting...\n")); > - return EFI_UNSUPPORTED; > - } > - > - 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; > + FoundSystemMem = FALSE; > + ZeroMem (DramRegions, sizeof (DramRegions)); > + > + Index = -1; > + do { > + ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO; > + ArmSmcArgs.Arg1 = Index++; > + > + ArmCallSmc (&ArmSmcArgs); > + ASSERT (!(ArmSmcArgs.Arg0 && !Index)); This is being a bit too clever for its own good. We're verifying that if one of the inputs to the function we just called is zero (i.e. is the first time around the loop), the value returned in the struct must also be zero? Is this equivalent to if (Index == 0) { ASSERT (ArmSmcArgs.Arg0 == SMC_WHATEVER_OK); } ? If so, please use that form. Don't use !Index when you mean Index == 0. Regardless, a short explanation of the protocol and expected responses are needed unless the code can be made more self-explanatory. > + if (!Index) { > + DramSize = ArmSmcArgs.Arg1; > + } else { > + if (!ArmSmcArgs.Arg0) { > + BaseAddress = ArmSmcArgs.Arg1; > + Size = ArmSmcArgs.Arg2; > + ASSERT (BaseAddress && Size); > + > + DramRegions[Index - 1].BaseAddress = BaseAddress; > + DramRegions[Index - 1].Size = Size; There's a lot of "-1" going on in this function, which should be possible to avoid by incrementing Index at the end of the loop and initializing it to 0. The compiler *will* do a better job at optimizing code for you in all but the most exceptional cases, so try to avoid optimising C. > + DramSize -= Size; > + > + DEBUG ((DEBUG_INFO, "bank[%d]: start 0x%lx, size 0x%lx\n", > + Index, BaseAddress, Size)); > } > - NextHob.Raw = GET_NEXT_HOB (NextHob); > + } > + } while (DramSize && Index < MAX_DRAM_REGIONS); > + > + ASSERT (!DramSize); And this (human language) semantically says "throw an exception if we have RAM". I can sort of start to make educated guesses at this point, but I shouldn't have to. If making the ArmCallSmc with .Arg1 == 0 is a special request to report the total amount of RAM in the system, that shouldn't be part of the same loop as the calls extracting the size of individual regions. > + > + FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress); > + FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize); > + > + // Declare memory regios to system regions > + for (Index = MAX_DRAM_REGIONS - 1; Index >= 0; Index--) { I guess what confuses me about this loop is the combination of: - Iterating over the DRAM regions backwards (optimization?) - Not terminating the iteration once all of the Fd region has been covered (the opposite of optimization). Could something be done about that? / Leif > + if (!DramRegions[Index].Size) { > + continue; > } > > - if (!Found) { > - // Reserved the memory space occupied by the firmware volume > + 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 > + ); > + } > + // 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) { > + continue; > + } > + > + BaseAddress = DramRegions[Index].BaseAddress; > + Size = DramRegions[Index].Size; > + Top = DramRegions[Index].BaseAddress + DramRegions[Index].Size; > + > + if (FdBase >= BaseAddress && FdTop <= Top) { > + Size -= (UINTN)FixedPcdGet32 (PcdFdSize); > + } > + > + if (Size >= FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)) { > + FoundSystemMem = TRUE; > } > } > > + ASSERT (FoundSystemMem); > + > // Build Memory Allocation Hob > InitMmu (MemoryTable); > > 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.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 @@ [Defines] > [Sources] > MemoryInitPeiLib.c > > - > [Packages] > ArmPkg/ArmPkg.dec > ArmPlatformPkg/ArmPlatformPkg.dec > @@ -30,6 +29,7 @@ [Packages] > [LibraryClasses] > ArmMmuLib > ArmPlatformLib > + ArmSmcLib > DebugLib > HobLib > PcdLib > @@ -40,6 +40,11 @@ [Guids] > [FeaturePcd] > gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob > > +[FixedPcd] > + gArmTokenSpaceGuid.PcdFdBaseAddress > + gArmTokenSpaceGuid.PcdFdSize > + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize > + > [Pcd] > gArmTokenSpaceGuid.PcdSystemMemoryBase > gArmTokenSpaceGuid.PcdSystemMemorySize > 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; > -} > 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 @@ [Packages] > Silicon/NXP/NxpQoriqLs.dec > > [LibraryClasses] > - ArmSmcLib > BaseLib > DebugLib > IoAccessLib > -- > 2.17.1 >