Hi Philippe, thanks for reviewing these.

On 2019.12.11 12:21, Philippe Mathieu-Daudé wrote:
On 12/11/19 12:25 PM, Pete Batard wrote:
From: Ard Biesheuvel <ard.biesheu...@linaro.org>

Having RAM and SoC register regions overlap is problematic for MMIO,
since, at the very least, we don't want these regions to be declared
as cacheable.

Signed-off-by: Pete Batard <p...@akeo.ie>
---
  Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c | 36 +++++++++++++-------
  1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
index cc761bea1307..781cf78b83d3 100644
--- a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
+++ b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
@@ -60,7 +60,7 @@ ArmPlatformGetVirtualMemoryMap (
  {
    UINTN                         Index = 0;
    UINTN                         GpuIndex;
-  INT64                         ExtendedMemorySize;
+  INT64                         SystemMemorySize;
    ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
    // Early output of the info we got from VideoCore can prove valuable.
@@ -120,21 +120,21 @@ ArmPlatformGetVirtualMemoryMap (
    VirtualMemoryInfo[Index].Type             = RPI_MEM_RESERVED_REGION;
    VirtualMemoryInfo[Index++].Name           = L"GPU Reserved";
-  // Compute the amount of extended RAM available on this platform
-  ExtendedMemorySize = SIZE_256MB;
-  ExtendedMemorySize <<= (mBoardRevision >> 20) & 0x07;
-  ExtendedMemorySize -= SIZE_1GB;
-  if (ExtendedMemorySize > 0) {
-    VirtualMemoryTable[Index].PhysicalBase  = FixedPcdGet64 (PcdExtendedMemoryBase); -    VirtualMemoryTable[Index].VirtualBase   = VirtualMemoryTable[Index].PhysicalBase;
-    VirtualMemoryTable[Index].Length        = ExtendedMemorySize;
-    VirtualMemoryTable[Index].Attributes    = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
-    VirtualMemoryInfo[Index].Type           = RPI_MEM_BASIC_REGION;
-    VirtualMemoryInfo[Index++].Name         = L"Extended System RAM";
-  }
+  // Compute the total RAM size available on this platform
+  SystemMemorySize = SIZE_256MB;
+  SystemMemorySize <<= (mBoardRevision >> 20) & 0x07;

TIL I learn this field is 3 bits (I thought it was 4).

"32 GB should be enough for everybody" (which is the max RAM you'll be able to declare in 3 bits with the current scheme).

Unless RAM becomes very very cheap, I doubt the Raspberry Pi foundation considers the current limit as much of a problem...


+
+  //
+  // Ensure that what we declare as System Memory doesn't overlap with the +  // Bcm2836 SoC registers. This can be achieved through a MIN () with the
+  // base address since SystemMemoryBase is 0 (we assert if it isn't).

Is the later comment "// On the Pi 3 the SoC registers may overlap VideoCore => fix this
" still accurate?

It is. We read the size of the GPU region (mVideoCoreSize) and the size of the System RAM from different queries. The thing is actually that VideoCore on the Pi 3 considers that its region should encompass the SoC registers, so it reports a size that does so, whereas we want to report them as separate regions (like they are reported on the Pi 4).

Regards,

/Pete

+  //
+  SystemMemorySize = MIN(SystemMemorySize, BCM2836_SOC_REGISTERS);
    // Extended SoC registers (PCIe, genet, ...)
    if (BCM2711_SOC_REGISTERS > 0) {
+    // Same overlap protection as above for the Bcm2711 SoC registers
+    SystemMemorySize                        = MIN(SystemMemorySize, BCM2711_SOC_REGISTERS);
      VirtualMemoryTable[Index].PhysicalBase  = BCM2711_SOC_REGISTERS;
      VirtualMemoryTable[Index].VirtualBase   = VirtualMemoryTable[Index].PhysicalBase;       VirtualMemoryTable[Index].Length        = BCM2711_SOC_REGISTER_LENGTH;
@@ -155,6 +155,16 @@ ArmPlatformGetVirtualMemoryMap (
    VirtualMemoryInfo[Index].Type             = RPI_MEM_RESERVED_REGION;
    VirtualMemoryInfo[Index++].Name           = L"SoC Reserved (283x)";

Sigh, the 2711/2838 naming is very confusing.

+  // If we have RAM above the 1 GB mark, declare it
+  if (SystemMemorySize - SIZE_1GB > 0) {
+    VirtualMemoryTable[Index].PhysicalBase  = FixedPcdGet64 (PcdExtendedMemoryBase); +    VirtualMemoryTable[Index].VirtualBase   = VirtualMemoryTable[Index].PhysicalBase; +    VirtualMemoryTable[Index].Length        = SystemMemorySize - SIZE_1GB; +    VirtualMemoryTable[Index].Attributes    = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+    VirtualMemoryInfo[Index].Type           = RPI_MEM_BASIC_REGION;
+    VirtualMemoryInfo[Index++].Name         = L"Extended System RAM";
+  }
+
    // End of Table
    VirtualMemoryTable[Index].PhysicalBase    = 0;
    VirtualMemoryTable[Index].VirtualBase     = 0;


Reviewed-by: Philippe Mathieu-Daude <phi...@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52121): https://edk2.groups.io/g/devel/message/52121
Mute This Topic: https://groups.io/mt/68147481/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to