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::22d; helo=mail-it0-x22d.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22d.google.com (mail-it0-x22d.google.com [IPv6:2607:f8b0:4001:c0b::22d]) (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 92CAC21F38841 for ; Thu, 12 Oct 2017 03:54:58 -0700 (PDT) Received: by mail-it0-x22d.google.com with SMTP id n195so6391763itg.2 for ; Thu, 12 Oct 2017 03:58:28 -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=RbVeuC2qYN4BQpVJXhjNQpkpIDxJu2d5sxwCHPY624s=; b=2Rib/lczgSJaMw0+dC4v0TJ8upmvMUKwef2soH3Yy2HI0zC87Lv0v48WgNEPt1gF9u PDajrgXzjjBfUTtoyPU7iG37rX5xJVg54CVzBwJZvGFOJKxS697fhJZk7cIcYXEVq9C7 MBeQ/6gzm3OXTt2aVSJG703k3ohQH0kBhpbRFxx0scPG7JjNxkdPnlmOv9GDZ47NbllL S9zDBI27ClryHNRMDWb0ASNtQyHHIhBL/LI+fHGhwnQCmzcExB6WXNlXJXOYTEUl2biI NIH1fy0eS2gSp+ZpKq+Odpkyes/J/md56zMt16Ow3NidT7CKnmALruTEJoPEYULUoS4+ TgdQ== 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=RbVeuC2qYN4BQpVJXhjNQpkpIDxJu2d5sxwCHPY624s=; b=YSFvhyCf/4XXXHhA95XSghWYRItqjefaTuccHqbHDb9aTxnQnGn1qBMwU+x3WU9rdi 5kOEZmKNRXvEcPmDlc3q2Zwi3IOnxvh2DLmaWvy9mnzlk0Y3wolEv/Cm8AhOsB1OwoeM kFDe9Qk/TOk+VAQmBJbdH42+NkwSqUhFhpcVItVl6Atf2vEkbrgeW/TG876UCWbZBHKP OBnmHzIN63ZceBUYVpKUYkK1nQ5qT7KvXPjaY9tdu9tQGQG1molNPmJC6wEHt50I5fLB rF/MoGeXdQC4TCpx2isa9Clp32pSePn4QupyLZucpOpg+lHHHJjddhj7a9wcR9OzIhaZ LnLg== X-Gm-Message-State: AMCzsaVrqXv/lBHlTIZSM7AtJw8sasZAbBrqzhZzpq1W2g16I2oIhYN7 XI8JDdfdx/WJFwUCdkESEIJdefawKwF4fPszW9KzYw== X-Google-Smtp-Source: AOwi7QD1YfAjtqvuRe7AZZmKZATzo6Vh71pHBkYF+Y2KFQIqelXKxZFolRoanZ8ZZgfbCXdhi3AeEO+bi7LjwSQfWoM= X-Received: by 10.36.177.9 with SMTP id o9mr2722677itf.44.1507805908208; Thu, 12 Oct 2017 03:58:28 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.157.141 with HTTP; Thu, 12 Oct 2017 03:58:27 -0700 (PDT) In-Reply-To: <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> <20171012105050.2pvtqwajjgjuvjvq@bivouac.eciton.net> From: Marcin Wojtas Date: Thu, 12 Oct 2017 12:58:27 +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 10:54:58 -0000 Content-Type: text/plain; charset="UTF-8" 2017-10-12 12:50 GMT+02:00 Leif Lindholm : > > 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?) I guess nowhere, but please bear in mind, there are also custom modules (Armada-7040-DB has one) and on-board DRAMs - in such circumstances it's up to the designer. > > > > > > > > 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. > Ok, I'll try to come up with something readable. > > } 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. > Ok, thanks. Marcin