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::233; helo=mail-wm0-x233.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (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 4A66D21F3882A for ; Thu, 12 Oct 2017 03:26:33 -0700 (PDT) Received: by mail-wm0-x233.google.com with SMTP id f4so12034325wme.0 for ; Thu, 12 Oct 2017 03:30:03 -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=N1UeLz8cdGT33BpNyQrw5OgLJcpk9tWcnaYou2QxrrM=; b=Tokits0cEddsDkBs3M88oESQT4iFRNYUmIkid5rAO6cLS4PrZoGGvQh4xQKBMmRi35 OCiGG7udfFf9Yr3NhWODI+a1Qjn1SUcrAS+XNQU0wW+bVlrsQuWyrNi4Z7FSExfHX9jS iohWgTrbKKfAbJC2ONcoBmvcpAB5kDi0FJRvc= 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=N1UeLz8cdGT33BpNyQrw5OgLJcpk9tWcnaYou2QxrrM=; b=nemPBX4fyxttTeRfNCpxja2oRy3iX1kqFRnTQAPnkr6c+JqaWWwPrs2JiDA8qaAOHQ /NeUH5SHgN35T1IiiJoLqGhdZ0QHgnIj4VGGxz+XHtb54XEmsYeYwxz+066znKu1RB9W xDft1Z2BdEyox0j4O0ZayR8Tr01ZYggddbmu1KbHd27dd8z2ml44h3L9il13l63O3ZbH BSk2XOMDCHH3LJ6lbhxd2aC2ynlWud0M01bis1fZ4232MUAKDSm4o/Fn1qZv+4mKwXle QT1dK2cYMZRaXtnod27+AMlkY1LrPJH6oOC7/cQ4J0JdTW1uz4X6sLBYbBEH6SpeOGBz ofZg== X-Gm-Message-State: AMCzsaU8CBC4h2arOaSVuw6Jp6Qjy1eA5wLiVp2b541i4Sw7tiZw+S5U +T1XeBkMpJ8F9KcIH/Q2/uZyHA== X-Google-Smtp-Source: AOwi7QBjH3ILOc4hJXu+9jbCnHcoh3DSGFQZrkl90eSE7tr7cHWDdBZzBbIFk0fx0mYlZthd2+5kJw== X-Received: by 10.28.58.81 with SMTP id h78mr1583740wma.10.1507804201805; Thu, 12 Oct 2017 03:30:01 -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 p13sm13843510wrg.28.2017.10.12.03.30.00 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 12 Oct 2017 03:30:00 -0700 (PDT) Date: Thu, 12 Oct 2017 11:29:59 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: Ard Biesheuvel , "edk2-devel@lists.01.org" , Nadav Haklai , Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , Jan =?utf-8?B?RMSFYnJvxZs=?= Message-ID: <20171012102959.isozglddgunxmhc6@bivouac.eciton.net> References: <1507736449-6073-1-git-send-email-mw@semihalf.com> <1507736449-6073-5-git-send-email-mw@semihalf.com> <20171011170815.fp2ni7r6eimjcgi6@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping 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:26:33 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Oct 12, 2017 at 06:58:37AM +0200, Marcin Wojtas wrote: > Hi Leif, > > 2017-10-11 20:18 GMT+02:00 Ard Biesheuvel : > > On 11 October 2017 at 18:08, Leif Lindholm wrote: > >> Subject: from->for ? > >> > >> On Wed, Oct 11, 2017 at 05:40:45PM +0200, Marcin Wojtas wrote: > >>> From: Ard Biesheuvel > >>> > >>> The Armada 70x0/80x0 DRAM controller allows a single window of DRAM > >>> to be remapped to another location in the physical address space. This > >>> allows us to free up some memory in the 32-bit addressable region for > >>> peripheral MMIO and PCI MMIO32 and CONFIG spaces. This patch adjusts > >>> memory blocks to the configuration done in ATF. > >> > >> The ARM Trusted Firmware developers tend to frown at that > >> abbreviation. Please use ARM-TF in official context. > > Ok. > > >> > >> Which configuration is this? Presumably, if this was changed in > >> ARM-TF, we would need to change it this end too, so does not hurt to > >> be explicit. (It uses a FixedPcd, so clearly it is not dynamically > >> detected, which the commit message can be read as.) > > I checked and I can modify the code, to obtain from registers an > information about source/target and whether Dram remap is enabled at > all. This way we would be immune to any further changes in ARM-TF. That sounds like an excellent improvement. > However I have a question to that setting, please see below. > > >> > >>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>> Signed-off-by: Ard Biesheuvel > >>> Signed-off-by: Marcin Wojtas > >>> --- > >>> Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S | 15 +++++ > >>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf | 3 + > >>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 60 ++++++++++++++++---- > >>> Platform/Marvell/Marvell.dec | 13 +++++ > >>> 4 files changed, 81 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S > >>> index 72f8cfc..c5be1a9 100644 > >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S > >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S > >>> @@ -17,6 +17,21 @@ > >>> > >>> ASM_FUNC(ArmPlatformPeiBootAction) > >>> mov x29, xzr > >>> + > >>> + .if FixedPcdGet64 (PcdSystemMemoryBase) != 0 > >>> + .err PcdSystemMemoryBase should be 0x0 on this platform! > >>> + .endif > >>> + > >>> + .if FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget) > >>> + // > >>> + // Use the low range for UEFI itself. The remaining memory will be mapped > >>> + // and added to the GCD map later. > >>> + // > >>> + adr x0, mSystemMemoryEnd > >>> + MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1) > >>> + str x1, [x0] > >>> + .endif > >>> + > > The original commit, due lack of dram size detection, increased total > memory to 4GB, which required above modification with limiting > mSystemMemoryEnd to dram target (0xc0000000). Now PcdSystemMemorySize > is left at somewhat lowest reasonable value (1GB), so above code is in > fact never executed. Are you ok that > I remove it and leave mSystemMemoryEnd to be set as-is in PrePi.c: > > UINT64 mSystemMemoryEnd = FixedPcdGet64(PcdSystemMemoryBase) + > FixedPcdGet64(PcdSystemMemorySize) - 1; > > ? Unless Ard tells me we're missing some aspect, that sounds reasonable. > >>> ret > >>> > >>> //UINTN > >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf > >>> index 2e198c3..838a670 100644 > >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf > >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf > >>> @@ -67,5 +67,8 @@ > >>> gArmTokenSpaceGuid.PcdArmPrimaryCoreMask > >>> gArmTokenSpaceGuid.PcdArmPrimaryCore > >>> > >>> + gMarvellTokenSpaceGuid.PcdDramRemapSize > >>> + gMarvellTokenSpaceGuid.PcdDramRemapTarget > >>> + > >>> [Ppis] > >>> gArmMpCoreInfoPpiGuid > >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > >>> index 74c9956..2cb2e15 100644 > >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c > >>> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > >>> #include > >>> #include > >>> #include > >>> +#include > >>> #include > >>> > >>> // The total number of descriptors, including the final "end-of-table" descriptor. > >>> @@ -44,6 +45,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > >>> #define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK > >>> #define DDR_ATTRIBUTES_UNCACHED ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED > >>> > >>> +STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS]; > >> > >> mVirtualMemoryTable? > > Ok. > > >> > >>> + > >>> /** > >>> Return the Virtual Memory Map of your platform > >>> > >>> @@ -59,20 +62,41 @@ ArmPlatformGetVirtualMemoryMap ( > >>> IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap > >>> ) > >>> { > >>> - ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable; > >>> UINTN Index = 0; > >>> + UINT64 MemSize; > >>> + UINT64 MemLowSize; > >>> + UINT64 MemHighStart; > >>> + UINT64 MemHighSize; > >>> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; > >>> > >>> ASSERT (VirtualMemoryMap != NULL); > >>> > >>> - VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR*)AllocatePages(EFI_SIZE_TO_PAGES (sizeof(ARM_MEMORY_REGION_DESCRIPTOR) * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS)); > >>> - if (VirtualMemoryTable == NULL) { > >>> - return; > >>> - } > >>> + MemSize = FixedPcdGet64 (PcdSystemMemorySize); > >>> + MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize); > >>> + MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) + > >>> + FixedPcdGet32 (PcdDramRemapSize); > >>> + MemHighSize = MemSize - MemLowSize; > >>> + > >>> + ResourceAttributes = ( > >>> + EFI_RESOURCE_ATTRIBUTE_PRESENT | > >>> + EFI_RESOURCE_ATTRIBUTE_INITIALIZED | > >>> + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE | > >>> + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE | > >>> + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | > >>> + EFI_RESOURCE_ATTRIBUTE_TESTED > >>> + ); > >>> + > >>> + BuildResourceDescriptorHob ( > >>> + EFI_RESOURCE_SYSTEM_MEMORY, > >>> + ResourceAttributes, > >>> + FixedPcdGet64 (PcdSystemMemoryBase), > >>> + MemLowSize > >>> + ); > >>> > >>> // DDR > >>> - VirtualMemoryTable[Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase); > >>> - VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdSystemMemoryBase); > >>> - VirtualMemoryTable[Index].Length = PcdGet64 (PcdSystemMemorySize); > >>> + VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdSystemMemoryBase); > >>> + VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdSystemMemoryBase); > >>> + VirtualMemoryTable[Index].Length = MemLowSize; > >>> VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED; > >>> > >>> // Configuration space 0xF000_0000 - 0xFFFF_FFFF > >>> @@ -81,13 +105,29 @@ ArmPlatformGetVirtualMemoryMap ( > >>> VirtualMemoryTable[Index].Length = 0x10000000; > >>> VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > >>> > >>> + if (MemSize > MemLowSize) { > >>> + // > >>> + // If we have more than MemLowSize worth of DRAM, the remainder will be > >>> + // mapped at the top of the remapped window. > >>> + // > >>> + VirtualMemoryTable[++Index].PhysicalBase = MemHighStart; > >>> + VirtualMemoryTable[Index].VirtualBase = MemHighStart; > >>> + VirtualMemoryTable[Index].Length = MemHighSize; > >>> + VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED; > >>> + > >>> + BuildResourceDescriptorHob ( > >>> + EFI_RESOURCE_SYSTEM_MEMORY, > >>> + ResourceAttributes, > >>> + MemHighStart, > >>> + MemHighSize > >>> + ); > >>> + } > >>> + > >>> // End of Table > >>> VirtualMemoryTable[++Index].PhysicalBase = 0; > >>> VirtualMemoryTable[Index].VirtualBase = 0; > >>> VirtualMemoryTable[Index].Length = 0; > >>> VirtualMemoryTable[Index].Attributes = 0; > >>> > >>> - ASSERT((Index + 1) <= MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS); > >>> - > >> > >> Why delete this assert? > > Indeed, it looks like unnecessarily removed. I will restore it, > > >> > >>> *VirtualMemoryMap = VirtualMemoryTable; > >>> } > >>> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec > >>> index 434d6cb..db1c7fa 100644 > >>> --- a/Platform/Marvell/Marvell.dec > >>> +++ b/Platform/Marvell/Marvell.dec > >>> @@ -194,6 +194,19 @@ > >>> #TRNG > >>> gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053 > >>> > >>> + # > >>> + # DRAM remapping controls. > >>> + # On the 70x0/80x0 SOCs, the DRAM is mapped at 0x0, and could be up to > >>> + # 16 GB in size. To allow for 32-bit addressable MMIO peripherals or PCI > >>> + # windows, a single window of up to 4 GB in size can be remapped elsewhere. > >>> + # So let's define a 1 GB window at 0xC000000 by default: this is the minimum > >>> + # alignment that Linux can map optimally (i.e., it's section shift is 30 bits) > >>> + # and gives us an additional 768 MB (on top of the 256 MB platform MMIO window > >>> + # at 0xF0000000) for the PCI MMIO32 and CONFIG spaces. > >> > >> If DRAM is mapped at 0, and can be up to 16GB in size, why do we need > >> to remap some of it below 4GB? > >> I guess what you're actually doing is mapping it _away_. Where to? > >> > >> (And a short-short version of this in the commit message should take > >> care of that item.) > >> > > > > Indeed. Physical RAM starts at 0x0, and to avoid wasting all 32-bit > > addressable memory space on RAM, the hardware allows some of it to be > > remapped elsewhere. So by remapping 1 GB, we make 1 GB of address > > space available for MMIO, PCI config space and PCI MMIO32 space. > > If we agree to detect remap size and target from registers, those PCDs > (and comment) will be removed. I will modify the commit log only. Sounds good to me. / Leif