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::230; helo=mail-wm0-x230.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x230.google.com (mail-wm0-x230.google.com [IPv6:2a00:1450:400c:c09::230]) (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 B471021F38836 for ; Thu, 12 Oct 2017 03:47:24 -0700 (PDT) Received: by mail-wm0-x230.google.com with SMTP id q132so12157130wmd.2 for ; Thu, 12 Oct 2017 03:50:54 -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=sD0T4VYFQxOcD1FNjZtho/pNhL+KGfGO2vls9ygUPuE=; b=Y4DS3goT9rTnboR4VfgpT0w0abAxrcrpexFzoAHxSwz07dgTlK0QcISzzXFCjhdLAt YCv6ZPU07OWZ7tX3jEcT3VjoMwAU5SBsT7WAa4HAPjJm3c3CRuNkHtMsN5i9IchG0W6m FIY+QLxq4CsmGBPa2YogcDulbN1QpqFera16o= 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=sD0T4VYFQxOcD1FNjZtho/pNhL+KGfGO2vls9ygUPuE=; b=m/N6xzcTpKSQDihxyTwpYl19l21K4DlHwmT1SHyHVrzLFX6lep7VN//1ZvtmSXE6SP W95wC1a58Hk3LDpsj6qeUkWG79QGLuEpvrzkx4HBRKPh/SS9UwaAdoRfxnCzwU2Uu2Ig 8omvVzSXk/6IlVOe6ncVCUazDh+yWpniR0i4azuiM9d8LOZMWx/Pi7foXhyCOoGP7PZS CB2Mtryw+3vFiYabsvAhGTsM3y7Vhd3hFAPfXCkVxFtwzoNtmJ30ZAPrTiliQyUO+h1f uJT+lewpuckr+1SBN48RwKCeibAb0E1mH6qVsLt5UAq4KxZXGGh60mGt8AtC93VOLxNt eXEg== X-Gm-Message-State: AMCzsaWqZGE0xU3ZklmCyd+Tl5vEzJF8QnE55ibWh3ZMkb+Aw23wiwR5 NOseLqm0atiPAZ4WGn8Yb+l+nw== X-Google-Smtp-Source: AOwi7QCnDp+4r++6Bp8CzgDPDJ7YlrjDmHkw2mtQQAXtedvb7vokUIvWwha22UAUWoUywCqBu3kynQ== X-Received: by 10.28.148.67 with SMTP id w64mr1390826wmd.132.1507805453422; Thu, 12 Oct 2017 03:50:53 -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 y23sm12146376wry.62.2017.10.12.03.50.52 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 12 Oct 2017 03:50:52 -0700 (PDT) Date: Thu, 12 Oct 2017 11:50:50 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan Message-ID: <20171012105050.2pvtqwajjgjuvjvq@bivouac.eciton.net> References: <1507736449-6073-1-git-send-email-mw@semihalf.com> <1507736449-6073-7-git-send-email-mw@semihalf.com> <20171011175613.ozdhj5n7a6n247qi@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms: PATCH 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: Thu, 12 Oct 2017 10:47:25 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Oct 12, 2017 at 07:47:52AM +0200, Marcin Wojtas wrote: > 2017-10-11 19:56 GMT+02:00 Leif Lindholm : > > On Wed, Oct 11, 2017 at 05:40:47PM +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 | 111 +++++++++++++++++++- > >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++ > >> 2 files changed, 159 insertions(+), 1 deletion(-) > >> > >> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > >> index 2cb2e15..22cbe47 100644 > >> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > >> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > >> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > >> #include > >> #include > >> #include > >> +#include > >> #include > >> > >> +#include "Armada70x0LibMem.h" > >> + > >> // The total number of descriptors, including the final "end-of-table" descriptor. > >> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16 > >> > >> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > >> > >> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS]; > >> > >> +// Obtain DRAM size basing on register values filled by early firmware. > >> +STATIC > >> +UINT64 > >> +DramSizeGet ( > > > > GetDramSize? > > > >> + UINT64 *MemSize > > > > IN, OUT? > > > > 2x OK. > > >> + ) > >> +{ > >> + UINT64 BaseAddr; > >> + UINT32 RegVal; > >> + UINT8 AreaLengthMap; > >> + UINT8 Cs; > >> + > >> + *MemSize = 0; > >> + > >> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) { > >> + > >> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)); > >> + > >> + /* Exit loop on first disabled DRAM CS */ > >> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) { > >> + break; > >> + } > >> + > >> + /* > >> + * Sanity check for base address of next DRAM block. > >> + * Only continuous space will be used. > >> + */ > >> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) << > >> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK); > > > > Please use macros, temporary variables, more parentheses or whatever > > to help make the above operation readable. > > > > Sure. > > >> + if (BaseAddr != *MemSize) { > >> + DEBUG ((DEBUG_ERROR, > >> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n", > >> + *MemSize)); > >> + return EFI_SUCCESS; > >> + } > >> + > >> + /* Decode area length for current CS from register value */ > >> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS); > >> + switch (AreaLengthMap) { > > > > Why Map? > > > > I stuck to mv-ddr convention, but I agree, 'AreaLength' will be better. > > >> + case 0x0: > >> + *MemSize += 0x18000000; > >> + break; > >> + case 0x1: > >> + *MemSize += 0x30000000; > >> + break; > >> + case 0x2: > >> + *MemSize += 0x60000000; > >> + break; > >> + case 0x3: > >> + *MemSize += 0xC0000000; > >> + break; > > > > The above ones look if not devoid of pattern, at least a bit > > unexpected - and 4-6 are comlpetely missing. Is there any > > documentation available for me to read to try to understand what is > > going on? > > Do you have a docs for armada 7k or 8k? Please check 0xf0020200 table, > bits 20:16. You are right, I missed '4' (6GB), but 5 and 6 are > reserved (no value possible). (Where do I find .75G, 1.5G, 3G and 6G DIMMs?) > > > > The below all look predictably formatted and possible to calculate > > rather than list one by one. > > > > (1 << ((AreaLengthMap + 16))) if a quick calculation serves me right. > > I think we can reduce the switch-case to this: > > if (AreaLength =< 0x4) { > *MemSize = 0x18000000 << AreaLength; > } else if (AreaLength >= 0x7 && AreaLength < 0x1B) > *MemSize = 1 << (AreaLengthMap + 16); I would prefer for the arithmetic to be hidden away in a macro. Both cases. Also, the tests themselves: IS_VALID_xxx_AREA(), IS_VALID_yyy_AREA(). Sorry, can't think of good real names - if the documentation has it, use that. > } else { > DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for > CS#%d\n", AreaLength, Cs)); > } > > Is above ok for you? Yeah, it's a big improvement over listing each individual option. / Leif