From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::241; helo=mail-wm0-x241.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) (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 3E48B21CEB15C for ; Wed, 25 Oct 2017 05:22:41 -0700 (PDT) Received: by mail-wm0-x241.google.com with SMTP id p75so1539505wmg.3 for ; Wed, 25 Oct 2017 05:26:26 -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=scUT6T060RzNZ7jOUxKEhzvggwKo8CpXl05lfx0eUHg=; b=VCk7CC9kMC0w/OdxrOnEGZkhwQuSgs6xYCAJDwBdCdhRoIGwHxkmiIW1StyrkzfMdu TZ4x5Py/yQXfpRJewtzVN45/Nwl3VhAGvO1OnBVTSJCwalGkA226Ezd2YmD+1U8rx56q HITpsYQiNeQkU3Tt5CIyoVwlKhGip9XuOcAao= 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=scUT6T060RzNZ7jOUxKEhzvggwKo8CpXl05lfx0eUHg=; b=c3uTeJmuhPHapUd647YuxiWolpe4oBx4gDgAaaVjIgdUmK7lhA4nYaJ2YiGTFrYl8G /bC7XJar54migbF2Hj+SHo/ba5JrQLt49RoNyJ3rq6BDgZf8gPsZhtHkuoQO6r/hpmqV qJfMMLzaBaKEFMADYxiq0/pG9pSNAb3NEyyGXSmDhneJ2E2/AGbL9/jGqp913qmdZj99 QIU/zAcKVmDlq/Gpaboh7ZZqPwG+vY2wDOuv3aleLl6uLVTPZMeI7LiD81I6OJtC2JkE Ew7ibfOSXObjGc6bR5XeeXkLD6k7b0qlNcy0yTcYCyJpAjpkOX6Ro4NLPvcMDJpQcCqz xjgw== X-Gm-Message-State: AMCzsaUOmsOTpRoXKCifu/e7zCwRBwmmrTM35AgsGiv6g69V9mo3naMN Tg8Uu8/Xw2o9DQmwR5tLYb0gMg== X-Google-Smtp-Source: ABhQp+SpIjBOsQldWSBHbk7gWQBlvgVzw+AFjxbXhEE4oQnD9wFNncOuJqk4V9UUEI3MVLcUCFoOxg== X-Received: by 10.28.28.201 with SMTP id c192mr1797093wmc.71.1508934384681; Wed, 25 Oct 2017 05:26:24 -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 o197sm8714484wmg.3.2017.10.25.05.26.23 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 25 Oct 2017 05:26:23 -0700 (PDT) Date: Wed, 25 Oct 2017 13:26:21 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, nadavh@marvell.com, neta@marvell.com, kostap@marvell.com, jinghua@marvell.com, jsd@semihalf.com Message-ID: <20171025122621.yaiqol2t3ofkq5zm@bivouac.eciton.net> References: <1508913930-30886-1-git-send-email-mw@semihalf.com> <1508913930-30886-7-git-send-email-mw@semihalf.com> MIME-Version: 1.0 In-Reply-To: <1508913930-30886-7-git-send-email-mw@semihalf.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms: PATCH v2 6/8] Marvell/Armada: Enable dynamic DRAM size detection 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: Wed, 25 Oct 2017 12:22:41 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Oct 25, 2017 at 08:45:28AM +0200, Marcin Wojtas wrote: > Instead of using hardcoded value in PcdSystemMemorySize PCD, > obtain DRAM size directly from SoC registers, which are filled > by firmware during early initialization stage. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marcin Wojtas > --- > Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 62 +++++++++++++++++++- > Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 25 ++++++++ > 2 files changed, 86 insertions(+), 1 deletion(-) > > diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > index 978e4d3..bf0ebcf 100644 > --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > @@ -50,6 +50,60 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > STATIC ARM_MEMORY_REGION_DESCRIPTOR mVirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS]; > > +// Obtain DRAM size basing on register values filled by early firmware. > +STATIC > +UINT64 > +GetDramSize ( > + IN OUT UINT64 *MemSize > + ) > +{ > + UINT64 BaseAddr; > + UINT8 RegionCode; > + UINT8 Cs; > + > + *MemSize = 0; > + > + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) { > + > + /* Exit loop on first disabled DRAM CS */ > + if (!DRAM_CS_ENABLED (Cs)) { > + break; > + } > + > + /* > + * Sanity check for base address of next DRAM block. > + * Only continuous space will be used. > + */ > + BaseAddr = GET_DRAM_REGION_BASE (Cs); > + if (BaseAddr != *MemSize) { > + DEBUG ((DEBUG_ERROR, > + "%a: DRAM blocks are not contiguous, limit size to 0x%llx\n", > + __FUNCTION__, > + *MemSize)); > + return EFI_SUCCESS; > + } > + > + /* Decode area length for current CS from register value */ > + RegionCode = GET_DRAM_REGION_SIZE_CODE (Cs); > + if (DRAM_REGION_SIZE_CODE_INVALID (RegionCode)) { > + DEBUG ((DEBUG_ERROR, > + "%a: Invalid memory region code (0x%x) for CS#%d\n", > + __FUNCTION__, > + RegionCode, > + Cs)); > + return EFI_INVALID_PARAMETER; > + } > + > + if (RegionCode <= 0x4) { It would be even nicer this one could be abstracted away in another macro (now everything else is this clean, it does sort of stand out). > + *MemSize += GET_DRAM_REGION_SIZE_ODD (RegionCode); > + } else { > + *MemSize += GET_DRAM_REGION_SIZE_EVEN (RegionCode); > + } I guess we should also check for the reserved ones. I'm also a fan of the "common case first" approach to if-statements. So, could we squash these two together?: if (DRAM_REGION_SIZE_EVEN (RegionCode)) { *MemSize += GET_DRAM_REGION_SIZE_EVEN (RegionCode); } else if (DRAM_REGION_SIZE_ODD (RegionCode)) { *MemSize += GET_DRAM_REGION_SIZE_ODD (RegionCode); } else { DEBUG ((DEBUG_ERROR, "%a: Invalid memory region code (0x%x) for CS#%d\n", __FUNCTION__, RegionCode, Cs)); return EFI_INVALID_PARAMETER; } Where #define DRAM_REGION_SIZE_ODD(x) ((x) <= 4) #define DRAM_REGION_SIZE_EVEN(x) (((x) >= 7) && ((x) <= 26)) (I use decimal numbers, because so does the documentation.) / Leif > + } > + > + return EFI_SUCCESS; > +} > + > /** > Return the Virtual Memory Map of your platform > > @@ -72,12 +126,18 @@ ArmPlatformGetVirtualMemoryMap ( > UINT64 MemHighSize; > UINT64 ConfigSpaceBaseAddr; > EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; > + EFI_STATUS Status; > > ASSERT (VirtualMemoryMap != NULL); > > ConfigSpaceBaseAddr = FixedPcdGet64 (PcdConfigSpaceBaseAddress); > > - MemSize = FixedPcdGet64 (PcdSystemMemorySize); > + // Obtain total memory size from the hardware. > + Status = GetDramSize (&MemSize); > + if (EFI_ERROR (Status)) { > + MemSize = FixedPcdGet64 (PcdSystemMemorySize); > + DEBUG ((DEBUG_ERROR, "Limit total memory size to %d MB\n", MemSize / 1024 / 1024)); > + } > > if (DRAM_REMAP_ENABLED) { > MemLowSize = MIN (DRAM_REMAP_TARGET, MemSize); > diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h > index 8101cf3..dd218d9 100644 > --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h > +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h > @@ -46,3 +46,28 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > (MmioRead32 (CCU_MC_RCR_REG) & REMAP_SIZE_MASK) + SIZE_1MB > #define DRAM_REMAP_TARGET \ > (MmioRead32 (CCU_MC_RTBR_REG) << TARGET_BASE_OFFS) > + > +#define DRAM_CH0_MMAP_LOW_REG(cs) (0xf0020200 + (cs) * 0x8) > +#define DRAM_CS_VALID_ENABLED_MASK 0x1 > +#define DRAM_AREA_LENGTH_OFFS 16 > +#define DRAM_AREA_LENGTH_MASK (0x1f << DRAM_AREA_LENGTH_OFFS) > +#define DRAM_START_ADDRESS_L_OFFS 23 > +#define DRAM_START_ADDRESS_L_MASK (0x1ff << DRAM_START_ADDRESS_L_OFFS) > +#define DRAM_CH0_MMAP_HIGH_REG(cs) (0xf0020204 + (cs) * 0x8) > +#define DRAM_START_ADDR_HTOL_OFFS 32 > + > +#define DRAM_MAX_CS_NUM 8 > + > +#define DRAM_CS_ENABLED(Cs) \ > + (MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)) & DRAM_CS_VALID_ENABLED_MASK) > +#define GET_DRAM_REGION_BASE(Cs) \ > + ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG ((Cs))) << \ > + DRAM_START_ADDR_HTOL_OFFS) | \ > + (MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)) & DRAM_START_ADDRESS_L_MASK); > +#define GET_DRAM_REGION_SIZE_CODE(Cs) \ > + (MmioRead32 (DRAM_CH0_MMAP_LOW_REG ((Cs))) & \ > + DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS > +#define DRAM_REGION_SIZE_CODE_INVALID(C) \ > + (((C) > 0x4 && (C) < 0x7) || (C) > 0x1b) > +#define GET_DRAM_REGION_SIZE_ODD(C) ((UINT64)0x18000000 << (C)) > +#define GET_DRAM_REGION_SIZE_EVEN(C) ((UINT64)1 << ((C) + 16)) > -- > 2.7.4 >