From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: "Marcin Wojtas" <mw@semihalf.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"Nadav Haklai" <nadavh@marvell.com>,
"Neta Zur Hershkovits" <neta@marvell.com>,
"Kostya Porotchkin" <kostap@marvell.com>,
"Hua Jing" <jinghua@marvell.com>, "Jan Dąbroś" <jsd@semihalf.com>
Subject: Re: [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping
Date: Wed, 11 Oct 2017 19:18:12 +0100 [thread overview]
Message-ID: <CAKv+Gu_j=0ogqio_jOa2M-jRir-mCXnWs=Z9p0gjz3hRB_x8yQ@mail.gmail.com> (raw)
In-Reply-To: <20171011170815.fp2ni7r6eimjcgi6@bivouac.eciton.net>
On 11 October 2017 at 18:08, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Subject: from->for ?
>
> On Wed, Oct 11, 2017 at 05:40:45PM +0200, Marcin Wojtas wrote:
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> 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.
>
> 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.)
>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> 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
>> +
>> 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 <Base.h>
>> #include <Library/ArmPlatformLib.h>
>> #include <Library/DebugLib.h>
>> +#include <Library/HobLib.h>
>> #include <Library/MemoryAllocationLib.h>
>>
>> // 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?
>
>> +
>> /**
>> 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?
>
>> *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.
next prev parent reply other threads:[~2017-10-11 18:14 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
2017-10-11 15:40 ` [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG Marcin Wojtas
2017-10-11 16:47 ` Leif Lindholm
2017-10-11 18:15 ` Ard Biesheuvel
2017-10-12 4:39 ` Marcin Wojtas
2017-10-12 10:24 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 2/8] Marvell/Armada: Increase preallocated memory region size Marcin Wojtas
2017-10-11 16:56 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 3/8] Marvell/Armada: Remove custom reset library residues Marcin Wojtas
2017-10-11 16:56 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping Marcin Wojtas
2017-10-11 17:08 ` Leif Lindholm
2017-10-11 18:18 ` Ard Biesheuvel [this message]
2017-10-12 4:58 ` Marcin Wojtas
2017-10-12 10:29 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 5/8] Marvell/Armada: Add MemoryInitPeiLib that reserves secure region Marcin Wojtas
2017-10-11 17:11 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection Marcin Wojtas
2017-10-11 17:56 ` Leif Lindholm
2017-10-12 5:47 ` Marcin Wojtas
2017-10-12 10:50 ` Leif Lindholm
2017-10-12 10:58 ` Marcin Wojtas
2017-10-11 15:40 ` [platforms: PATCH 7/8] Marvell/Armada: Armada70x0Lib: Add support for 32-bit ARM Marcin Wojtas
2017-10-11 17:57 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 8/8] Marvell/Armada: Add 32-bit ARM support Marcin Wojtas
2017-10-11 17:58 ` Leif Lindholm
2017-10-11 18:20 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKv+Gu_j=0ogqio_jOa2M-jRir-mCXnWs=Z9p0gjz3hRB_x8yQ@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox