Re: [edk2-platforms][PATCH 4/8] Platform/RPi: Read more variables from VideoCore during early init

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Thu, Nov 14, 2019 at 04:07:36PM +0000, Pete Batard wrote:
> Besides the base memory size, we can read the GPU/VideoCore base as
> well as the model during early init, which we'll need for improving
> the memory mapping.
> 
> This patch adds the retrieval of these variables, as well as some
> early debug display of their values (which can be useful) and also
> removes unused variables such as mGPUMemoryBase and mGPUMemoryLength.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h              |  1 +
>  Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S | 75 ++++++++++++++++++--
>  Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c            | 14 +++-
>  3 files changed, 84 insertions(+), 6 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h b/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
> index d3b6f117cfdf..584786a61dfd 100644
> --- a/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
> +++ b/Platform/RaspberryPi/Include/IndustryStandard/RpiMbox.h
> @@ -34,6 +34,7 @@
>  #define RPI_MBOX_GET_MAC_ADDRESS                              0x00010003
>  #define RPI_MBOX_GET_BOARD_SERIAL                             0x00010004
>  #define RPI_MBOX_GET_ARM_MEMSIZE                              0x00010005
> +#define RPI_MBOX_GET_VC_MEMSIZE                               0x00010006
>  
>  #define RPI_MBOX_SET_POWER_STATE                              0x00028001
>  
> diff --git a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> index 36af208d12d8..49a132c722f1 100644
> --- a/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> +++ b/Platform/RaspberryPi/Library/PlatformLib/AArch64/RaspberryPiHelper.S
> @@ -1,5 +1,6 @@
>  /** @file
>   *
> + *  Copyright (c) 2019, Pete Batard <pete@akeo.ie>
>   *  Copyright (c) 2016, Linaro Limited. All rights reserved.
>   *  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
>   *
> @@ -52,24 +53,90 @@ ASM_FUNC (ArmPlatformPeiBootAction)

Before this change, x1 is initialized once, used immediately, and later
corrupted. After this change it becomes effectively a variable, used
several times throughout the function.

This is all fine, but I think it deserves adding a comment to the
assignment along the lines of
  // x1 holds the value of PcdDmaDeviceOffset throughout the function

/
    Leif

>      ldr     wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
>      dmb     ld
>  
> +    ldr     w0, .Lmembase
> +    adr     x2, mSystemMemoryBase
> +    str     x0, [x2]
> +
>      ldr     w0, .Lmemsize
>      sub     x0, x0, #1
> -    adr     x1, mSystemMemoryEnd
> -    str     x0, [x1]
> +    adr     x2, mSystemMemoryEnd
> +    str     x0, [x2]
> +
> +    adr     x0, .Lvcinfo_buffer
> +    orr     x0, x0, #RPI_MBOX_VC_CHANNEL
> +    add     x0, x0, x1
> +
> +    poll    BCM2836_MBOX_STATUS_FULL
> +    str     w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
> +    dmb     sy
> +    poll    BCM2836_MBOX_STATUS_EMPTY
> +    dmb     sy
> +    ldr     wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
> +    dmb     ld
> +
> +    ldr     w0, .Lvcbase
> +    adr     x2, mVideoCoreBase
> +    str     x0, [x2]
> +
> +    ldr     w0, .Lvcsize
> +    adr     x2, mVideoCoreSize
> +    str     x0, [x2]
> +
> +    adr     x0, .Lrevinfo_buffer
> +    orr     x0, x0, #RPI_MBOX_VC_CHANNEL
> +    add     x0, x0, x1
> +
> +    poll    BCM2836_MBOX_STATUS_FULL
> +    str     w0, [x4, #BCM2836_MBOX_WRITE_OFFSET]
> +    dmb     sy
> +    poll    BCM2836_MBOX_STATUS_EMPTY
> +    dmb     sy
> +    ldr     wzr, [x4, #BCM2836_MBOX_READ_OFFSET]
> +    dmb     ld
> +
> +    ldr     w0, .Lrevision
> +    adr     x2, mBoardRevision
> +    str     w0, [x2]
> +
>      ret
>  
>      .align  4
>  .Lmeminfo_buffer:
> -    .long   .Lbuffer_size
> +    .long   .Lmeminfo_size
>      .long   0x0
>      .long   RPI_MBOX_GET_ARM_MEMSIZE
>      .long   8                           // buf size
>      .long   0                           // input len
> +.Lmembase:
>      .long   0                           // mem base
>  .Lmemsize:
>      .long   0                           // mem size
>      .long   0                           // end tag
> -    .set    .Lbuffer_size, . - .Lmeminfo_buffer
> +    .set    .Lmeminfo_size, . - .Lmeminfo_buffer
> +
> +.Lvcinfo_buffer:
> +    .long   .Lvcinfo_size
> +    .long   0x0
> +    .long   RPI_MBOX_GET_VC_MEMSIZE
> +    .long   8                           // buf size
> +    .long   0                           // input len
> +.Lvcbase:
> +    .long   0                           // videocore base
> +.Lvcsize:
> +    .long   0                           // videocore size
> +    .long   0                           // end tag
> +    .set    .Lvcinfo_size, . - .Lvcinfo_buffer
> +
> +.Lrevinfo_buffer:
> +    .long   .Lrevinfo_size
> +    .long   0x0
> +    .long   RPI_MBOX_GET_BOARD_REVISION
> +    .long   4                           // buf size
> +    .long   0                           // input len
> +.Lrevision:
> +    .long   0                           // revision
> +    .long   0                           // end tag
> +    .set    .Lrevinfo_size, . - .Lrevinfo_buffer
>  
>  //UINTN
>  //ArmPlatformGetPrimaryCoreMpId (
> diff --git a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> index 97d5af5260c6..2bfd3f020a6e 100644
> --- a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> +++ b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> @@ -12,9 +12,11 @@
>  #include <IndustryStandard/Bcm2836.h>
>  #include <Library/PcdLib.h>
>  
> +UINT64 mSystemMemoryBase;
>  extern UINT64 mSystemMemoryEnd;
> -extern UINT64 mGPUMemoryBase;
> -extern UINT64 mGPUMemoryLength;
> +UINT64 mVideoCoreBase;
> +UINT64 mVideoCoreSize;
> +UINT32 mBoardRevision;
>  
>  #define VariablesSize (FixedPcdGet32(PcdFlashNvStorageVariableSize) +   \
>                         FixedPcdGet32(PcdFlashNvStorageFtwWorkingSize) + \
> @@ -87,6 +89,14 @@ ArmPlatformGetVirtualMemoryMap (
>    IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
>    )
>  {
> +  // Early output of the info we got from VideoCore can prove valuable.
> +  DEBUG ((DEBUG_INFO, "Board Rev: 0x%lX\n", mBoardRevision));
> +  DEBUG ((DEBUG_INFO, "Base RAM : 0x%ll08X (Size 0x%ll08X)\n", mSystemMemoryBase, mSystemMemoryEnd + 1));
> +  DEBUG ((DEBUG_INFO, "VideoCore: 0x%ll08X (Size 0x%ll08X)\n", mVideoCoreBase, mVideoCoreSize));
> +
> +  ASSERT (mSystemMemoryBase == 0);
> +  ASSERT (VirtualMemoryMap != NULL);
> +
>    RaspberryPiMemoryRegionDescriptor[3].Length = mSystemMemoryEnd + 1 -
>      FixedPcdGet64 (PcdSystemMemoryBase);
>  
> -- 
> 2.21.0.windows.1
>