From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::22a; helo=mail-it0-x22a.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22a.google.com (mail-it0-x22a.google.com [IPv6:2607:f8b0:4001:c0b::22a]) (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 6355321F3C18C for ; Wed, 11 Oct 2017 22:44:24 -0700 (PDT) Received: by mail-it0-x22a.google.com with SMTP id p138so5870163itp.2 for ; Wed, 11 Oct 2017 22:47:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=t52Fbx5ItvHaSWh6h3IyvuPQz4QjIB+MFvpI57rVZpI=; b=wmEisWMccPna93ci00HGqFlyaaZQOcIdLQz9O7d9AGEih6vsvOHLF5aBCYIWjYzkWR gUwHUxBsa7mB0veaq4ICRuXKhB9PZVyJ72KnZR8lLCmXZ+iJ9cmyF8OrOqdTXdGXVzJG QtGI187EL4kFNo4JyVnKIejkxr9hoHVMXQ/0qWLBeaU1I5+OfuwF3B/nfgovuGRmINLZ uaKMiNPWVVNdpCzNg8KkAQbReK7OuH7HgGoOtvwRswcHz9Jg9V9MVCdg0x2xxej4ubkb BDew6L1FOH81vEw4KSVqzi50t4qQC3ld6vb5QBlhWi3G21NhBrakiLoAC1TcnkeqekYD bNTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=t52Fbx5ItvHaSWh6h3IyvuPQz4QjIB+MFvpI57rVZpI=; b=dr0d+z5jCDJs88n7pXMsiRSzba1KpKg6ujJeBpAVW9SLiLVY4fvzYxS/nHMnIiZpJO z4o49J/W27TcRqBWXlxacWNZqjxcZmvFJlpfAqty8Q6F6dnm0rvFTPFIzY1Qnt3Lh3TV rrZNtpty6cw/8V2kjGz8Q7gOo0VqZy4fWDEhUdtTwaIgotN3IxHX9bIKWwTAoE8kLTXM J1QU3EMCgiMbW7B/wcimtu93+bEION1yE3S9C9zYISK2vhoWooU6ERU8CFdo3J3tSkeH JcEFfO9951DgaHnSK7c5KoPq2rbwoL439s6w6NNbfC1aiWu1lO5Nxu/vNcnClDgsKgCy 9c2A== X-Gm-Message-State: AMCzsaXMWeyDYXNQ8hdRtDFYx1suHbnYLmFeVIHGVuAckBEEPqdBDVma WoTMTdK88EXmYRBGC5tkHJcId7/jRlDRVEo628a86scklgs= X-Google-Smtp-Source: AOwi7QDulIr85gaEPfanSg2LfQAKWJXUzwCUw9Zbm4GzPwB6raE5mmJ52TW6ehTJQAO3BBEilirIeez8c7NpLJQmFJc= X-Received: by 10.36.57.18 with SMTP id l18mr1748400ita.138.1507787273246; Wed, 11 Oct 2017 22:47:53 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.157.141 with HTTP; Wed, 11 Oct 2017 22:47:52 -0700 (PDT) In-Reply-To: <20171011175613.ozdhj5n7a6n247qi@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> From: Marcin Wojtas Date: Thu, 12 Oct 2017 07:47:52 +0200 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan 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 05:44:24 -0000 Content-Type: text/plain; charset="UTF-8" 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). > > 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); } else { DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for CS#%d\n", AreaLength, Cs)); } Is above ok for you? > >> + case 0x7: >> + *MemSize += 0x00800000; >> + break; >> + case 0x8: >> + *MemSize += 0x01000000; >> + break; >> + case 0x9: >> + *MemSize += 0x02000000; >> + break; >> + case 0xA: >> + *MemSize += 0x04000000; >> + break; >> + case 0xB: >> + *MemSize += 0x08000000; >> + break; >> + case 0xC: >> + *MemSize += 0x10000000; >> + break; >> + case 0xD: >> + *MemSize += 0x20000000; >> + break; >> + case 0xE: >> + *MemSize += 0x40000000; >> + break; >> + case 0xF: >> + *MemSize += 0x80000000; >> + break; >> + case 0x10: >> + *MemSize += 0x100000000; >> + break; >> + case 0x11: >> + *MemSize += 0x200000000; >> + break; >> + case 0x12: >> + *MemSize += 0x400000000; >> + break; >> + case 0x13: >> + *MemSize += 0x800000000; >> + break; >> + default: >> + DEBUG ((DEBUG_ERROR, "Invalid area length (0x%x) for CS#%d\n", AreaLengthMap, Cs)); > > Area length isn't really a helpful debug message. > "memory module size"? > Ok. Thanks, Marcin