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.
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];
+
/**
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);
-
*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.
+ #
+ gMarvellTokenSpaceGuid.PcdDramRemapSize|0x40000000|UINT32|0x50000004
+ gMarvellTokenSpaceGuid.PcdDramRemapTarget|0xC0000000|UINT32|0x50000003
+
[Protocols]
gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.)
/
Leif
> + #
> + gMarvellTokenSpaceGuid.PcdDramRemapSize|0x40000000|UINT32|0x50000004
> + gMarvellTokenSpaceGuid.PcdDramRemapTarget|0xC0000000|UINT32|0x50000003
> +
> [Protocols]
> gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
> gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
> --
> 2.7.4
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Leif,
2017-10-11 20:18 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> 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.
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.
However I have a question to that setting, please see below.
>>
>>> 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
>>> +
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;
?
>>> 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?
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.
Best regards,
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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 <ard.biesheuvel@linaro.org>:
> > 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.
>
> 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 <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
> >>> +
>
> 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 <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?
>
> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.