Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: fix FDT handling for RPi4

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Fri, 6 Mar 2020 at 13:46, Pete Batard <pete@akeo.ie> wrote:
>
> Please see one note at the end:
>
> On 2020.03.06 05:53, Andrei Warkentin wrote:
> > A rev-up of start4.elf VPU firmware meant that
> > the previous scheme of loading the DTB over top
> > of RPI_EFI.FD no longer works - the DT is now
> > loaded way before the armstub, so any overlap
> > means the DT is overridden.
> >
> > This change re-arranges a few items in the FD,
> > allowing the DTB to loaded directly after the
> > FD in physical memory.
> >
> > This moves UEFI image down by 0x10000, and reduces
> > the FD image size by 0x10000, leaving space for
> > a DTB to be loaded by config.txt at 0x1f0000.
> >
> > You need a matching "rev RPi4 TF-A for DTB fix" patch
> > to edk2-non-osi, as it requires a TF-A build with these
> > options:
> >
> > PRELOADED_BL33_BASE=0x20000 RPI3_PRELOADED_DTB_BASE=0x1f0000
> >
> > Note: the same problem still affects the Pi 3, and will be
> > fixed in a separate change.
> >
> > Signed-off-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> > ---
> >   Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf  |  2 ++
> >   Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c | 13 ++++++++++++-
> >   Platform/RaspberryPi/RPi4/RPi4.dsc                        | 10 +++++++---
> >   Platform/RaspberryPi/RPi4/RPi4.fdf                        | 20 +++++++-------------
> >   Platform/RaspberryPi/RPi4/Readme.md                       | 10 +++++-----
> >   Platform/RaspberryPi/RaspberryPi.dec                      | 15 ++++++++-------
> >   6 files changed, 41 insertions(+), 29 deletions(-)
> >
> > diff --git a/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf b/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf
> > index 77cdbe39..3aac6a98 100644
> > --- a/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf
> > +++ b/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf
> > @@ -44,6 +44,8 @@
> >   [FixedPcd]
> >     gArmTokenSpaceGuid.PcdFdBaseAddress
> >     gArmTokenSpaceGuid.PcdFvBaseAddress
> > +  gRaspberryPiTokenSpaceGuid.PcdFdtBaseAddress
> > +  gRaspberryPiTokenSpaceGuid.PcdFdtSize
> >     gArmPlatformTokenSpaceGuid.PcdCoreCount
> >     gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
> >     gArmTokenSpaceGuid.PcdArmPrimaryCore
> > diff --git a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> > index e795a885..dec8e09d 100644
> > --- a/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> > +++ b/Platform/RaspberryPi/Library/PlatformLib/RaspberryPiMem.c
> > @@ -94,7 +94,18 @@ ArmPlatformGetVirtualMemoryMap (
> >     VirtualMemoryInfo[Index].Type             = RPI_MEM_RUNTIME_REGION;
> >     VirtualMemoryInfo[Index++].Name           = L"FD Variables";
> >
> > -  if (BCM2711_SOC_REGISTERS == 0) {
> > +  if (BCM2711_SOC_REGISTERS != 0) {
> > +     //
> > +     // Only the Pi 4 firmware today expects the DTB to directly follow the
> > +     // FD instead of overlapping the FD.
> > +     //
> > +     VirtualMemoryTable[Index].PhysicalBase    = FixedPcdGet32 (PcdFdtBaseAddress);
> > +     VirtualMemoryTable[Index].VirtualBase     = VirtualMemoryTable[Index].PhysicalBase;
> > +     VirtualMemoryTable[Index].Length          = FixedPcdGet32 (PcdFdtSize);;
> > +     VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> > +     VirtualMemoryInfo[Index].Type             = RPI_MEM_RESERVED_REGION;
> > +     VirtualMemoryInfo[Index++].Name           = L"Flattened Device Tree";
> > +  } else {
> >        //
> >        // TF-A reserved RAM only exists for the Pi 3 TF-A.
> >        //
> > diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> > index da62dc5b..2e98c3e1 100644
> > --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> > +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> > @@ -279,7 +279,10 @@
> >     gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|1
> >     gEfiMdePkgTokenSpaceGuid.PcdPostCodePropertyMask|0
> >     gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|320
> > -  gRaspberryPiTokenSpaceGuid.PcdFdtBaseAddress|0x20000
> > +  #
> > +  # Follows right after the FD image.
> > +  #
> > +  gRaspberryPiTokenSpaceGuid.PcdFdtBaseAddress|0x001f0000
> >
> >     # DEBUG_ASSERT_ENABLED       0x01
> >     # DEBUG_PRINT_ENABLED        0x02
> > @@ -393,8 +396,9 @@
> >     # Size of the region used by UEFI in permanent memory (Reserved 64MB)
> >     gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000
> >     #
> > -  # This matches PcdFvBaseAddress, since everything less is ATF, and
> > -  # will be reserved away.
> > +  # 0x00000000 - 0x001F0000  FD (PcdFdBaseAddress, PcdFdSize)
> > +  # 0x001F0000 - 0x00200000 DTB (PcdFdtBaseAddress, PcdFdtSize)
> > +  # 0x00200000 - ...        RAM (PcdSystemMemoryBase, PcdSystemMemorySize)
> >     #
> >     gArmTokenSpaceGuid.PcdSystemMemoryBase|0x00200000
> >     gArmTokenSpaceGuid.PcdSystemMemorySize|0x3fe00000
> > diff --git a/Platform/RaspberryPi/RPi4/RPi4.fdf b/Platform/RaspberryPi/RPi4/RPi4.fdf
> > index c3832035..a59d3b60 100644
> > --- a/Platform/RaspberryPi/RPi4/RPi4.fdf
> > +++ b/Platform/RaspberryPi/RPi4/RPi4.fdf
> > @@ -25,11 +25,11 @@
> >
> >   [FD.RPI_EFI]
> >   BaseAddress   = 0x00000000|gArmTokenSpaceGuid.PcdFdBaseAddress
> > -Size          = 0x00200000|gArmTokenSpaceGuid.PcdFdSize
> > +Size          = 0x001f0000|gArmTokenSpaceGuid.PcdFdSize
> >   ErasePolarity = 1
> >
> >   BlockSize     = 0x00001000|gRaspberryPiTokenSpaceGuid.PcdFirmwareBlockSize
> > -NumBlocks     = 0x200
> > +NumBlocks     = 0x1f0
> >
> >   ################################################################################
> >   #
> > @@ -53,16 +53,10 @@ NumBlocks     = 0x200
> >   0x00000000|0x00020000
> >   FILE = $(TFA_BUILD_BL31)
> >
> > -#
> > -# DTB.
> > -#
> > -0x00020000|0x00010000
> > -DATA = { 0x00 }
> > -
> >   #
> >   # UEFI image
> >   #
> > -0x00030000|0x001b0000
> > +0x00020000|0x001b0000
> >   gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize
> >   FV = FVMAIN_COMPACT
> >
> > @@ -76,7 +70,7 @@ FV = FVMAIN_COMPACT
> >   #
> >
> >   # NV_VARIABLE_STORE
> > -0x001e0000|0x0000e000
> > +0x001d0000|0x0000e000
> >   gRaspberryPiTokenSpaceGuid.PcdNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> >
> >   DATA = {
> > @@ -119,11 +113,11 @@ DATA = {
> >   }
> >
> >   # NV_EVENT_LOG
> > -0x001ee000|0x00001000
> > +0x001de000|0x00001000
> >   gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogBase|gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogSize
> >
> >   # NV_FTW_WORKING header
> > -0x001ef000|0x00001000
> > +0x001df000|0x00001000
> >   gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwWorkingBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> >
> >   DATA = {
> > @@ -138,7 +132,7 @@ DATA = {
> >   }
> >
> >   # NV_FTW_WORKING data
> > -0x001f0000|0x00010000
> > +0x001e0000|0x00010000
> >   gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwSpareBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> >
> >   ################################################################################
> > diff --git a/Platform/RaspberryPi/RPi4/Readme.md b/Platform/RaspberryPi/RPi4/Readme.md
> > index 21c9fd4f..e2f0d698 100644
> > --- a/Platform/RaspberryPi/RPi4/Readme.md
> > +++ b/Platform/RaspberryPi/RPi4/Readme.md
> > @@ -49,8 +49,8 @@ Build instructions from the top level edk2-platforms Readme.md apply.
> >       ```
> >       Additionally, if you want to use PL011 instead of the miniUART, you can add the lines:
> >       ```
> > -    device_tree_address=0x20000
> > -    device_tree_end=0x30000
> > +    device_tree_address=0x1f0000
> > +    device_tree_end=0x200000
> >       device_tree=bcm2711-rpi-4-b.dtb
> >       dtoverlay=miniuart-bt
> >       ```
> > @@ -80,12 +80,12 @@ You can pass a custom Device Tree and overlays using the following:
> >   ```
> >   (...)
> >   disable_commandline_tags=2
> > -device_tree_address=0x20000
> > -device_tree_end=0x30000
> > +device_tree_address=0x1f0000
> > +device_tree_end=0x200000
> >   device_tree=bcm2711-rpi-4-b.dtb
> >   ```
> >
> > -Note: the address range **must** be `[0x20000:0x30000]`.
> > +Note: the address range **must** be `[0x1f0000:0x200000]`.
> >   `dtoverlay` and `dtparam` parameters are also supported **when** providing a Device Tree`.
> >
> >   ## Custom `bootargs`
> > diff --git a/Platform/RaspberryPi/RaspberryPi.dec b/Platform/RaspberryPi/RaspberryPi.dec
> > index 25058ccc..3ebb83d9 100644
> > --- a/Platform/RaspberryPi/RaspberryPi.dec
> > +++ b/Platform/RaspberryPi/RaspberryPi.dec
> > @@ -36,13 +36,14 @@
> >
> >   [PcdsFixedAtBuild.common]
> >     gRaspberryPiTokenSpaceGuid.PcdFdtBaseAddress|0x10000|UINT32|0x00000001
> > -  gRaspberryPiTokenSpaceGuid.PcdFirmwareBlockSize|0x0|UINT32|0x00000002
> > -  gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogBase|0x0|UINT32|0x00000003
> > -  gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogSize|0x0|UINT32|0x00000004
> > -  gRaspberryPiTokenSpaceGuid.PcdNvStorageVariableBase|0x0|UINT32|0x00000005
> > -  gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwSpareBase|0x0|UINT32|0x00000006
> > -  gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwWorkingBase|0x0|UINT32|0x00000007
> > -  gRaspberryPiTokenSpaceGuid.PcdExtendedMemoryBase|0x0|UINT32|0x00000008
> > +  gRaspberryPiTokenSpaceGuid.PcdFdtSize|0x10000|UINT32|0x00000002
> > +  gRaspberryPiTokenSpaceGuid.PcdFirmwareBlockSize|0x0|UINT32|0x00000003
> > +  gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogBase|0x0|UINT32|0x00000004
> > +  gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogSize|0x0|UINT32|0x00000005
> > +  gRaspberryPiTokenSpaceGuid.PcdNvStorageVariableBase|0x0|UINT32|0x00000006
> > +  gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwSpareBase|0x0|UINT32|0x00000007
> > +  gRaspberryPiTokenSpaceGuid.PcdNvStorageFtwWorkingBase|0x0|UINT32|0x00000008
> > +  gRaspberryPiTokenSpaceGuid.PcdExtendedMemoryBase|0x0|UINT32|0x00000009
>
> Since I got the same remark last time I went for something similar, I'm
> just going to point out that you only want to change/add the ids for the
> Pcds you are explicitly modifying or adding, and leave the rest as is
> even if it breaks sequentiality.
>
> In other words, you could just have inserted a:
>
> gRaspberryPiTokenSpaceGuid.PcdFdtSize|0x10000|UINT32|0x00000009
>
> either after gRaspberryPiTokenSpaceGuid.PcdFdtBaseAddress or preferably
> at the end, since I would assert that we care less about grouping the
> PCDs in a logical order here than we care about finding out the next
> free PCD id to use.
>
> I'm hoping that this can be fixed by a maintainer at integration, rather
> than require a v2, since that's the only comment I have on this patchset.
>
> With this:
>
> Reviewed-by: Pete Batard <pete@akeo.ie>
> Tested-by: Pete Batard <pete@akeo.ie>
>

Agreed

Pushed as 91ed4f904e16..828fcbf96a38

Thanks all.