From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web09.15533.1583513304046657960 for ; Fri, 06 Mar 2020 08:48:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=IB9bRVMC; spf=pass (domain: linaro.org, ip: 209.85.221.67, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f67.google.com with SMTP id z15so3177742wrl.1 for ; Fri, 06 Mar 2020 08:48:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=GWvcGzpUvOZG2xfCo/+UyGxxAgTbdK8R251Srm6jT5A=; b=IB9bRVMCj/5oM4RZwOuY4DuXC0IuMHbHdevVzToc5i+IP691QCw5PkN96BlA/oQQht 2uUGekZ0RArKLGizy6fmWeyGJ8c1c6lStQYTq/JNKULD3f9f6h7VFQauFYnxYhctpF8B Z8iSdBDvodSBl6Ykky2uLf0vGbKbR84Ez283vierva2NgXTg2LdaSQjCgDdgAqHMLBtr POKoZaYJshYa+bUk5qqJfOeF5uYJVoVTJ9IB57FY67qqHDge1VYhiL0TTfQE4F4CI8bL ZNZx/5Y/WSv5dPunC+GKexsIu6D8Oim/SeDME6VmitWebbNGHUaJEBjRUI13k+qQ3lbj M1Dg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=GWvcGzpUvOZG2xfCo/+UyGxxAgTbdK8R251Srm6jT5A=; b=fFuXk7v93EZwIWWEJOdiSDB3gBreDcpFoiPoI8TlVIk/STXBQAdheRlmy1v0sG4XNF Jc8xDo2C3MFuf+ezlr1GBBBW5cONlobvlgQYmtWXaiAfpolJi8Adn7QoDu9AgOI7lveq T1tdVNect+KX4fv7mw7JcpAw7H0Icn0qYEGuR+ozh6baGYTH4fCA5iN26FiCb4InrZ8h tGGA0JA5EsQR0IFMdXdYoi2er4P3r1E4HsvWpPmt5uVc9E9tUDFEIvk5MjYKeO3nQ4rl LA0WbQzRIGpdGuXpmlbc9o6TNCDkK5PayR0Ss1bCSFuYcYTredSpX9BjteYMHlc0LLA4 NDjA== X-Gm-Message-State: ANhLgQ1Mm6JD5tbuE4K5te7l+JE2O2pSs/AgozTAoCT0XdwzhjW6wUE+ 3YzhcWNcwgjbKOqJ2DZq5msKt2qSb06RrzSanKHHVg== X-Google-Smtp-Source: ADFU+vu0q+NxgPpPOgRMN2yr0DNVCsP6nXvazouCPj/arTFMoqNdmV2fVdsQW4yzgM6kWHUX59Pmhjw3khe2UAudx0U= X-Received: by 2002:adf:e742:: with SMTP id c2mr4870720wrn.262.1583513302417; Fri, 06 Mar 2020 08:48:22 -0800 (PST) MIME-Version: 1.0 References: <20200306055323.39073-1-andrey.warkentin@gmail.com> <326fc408-43a8-3a10-ea42-c4a38b7b3381@akeo.ie> In-Reply-To: <326fc408-43a8-3a10-ea42-c4a38b7b3381@akeo.ie> From: "Ard Biesheuvel" Date: Fri, 6 Mar 2020 17:48:11 +0100 Message-ID: Subject: Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: fix FDT handling for RPi4 To: Pete Batard Cc: Andrei Warkentin , edk2-devel-groups-io , Leif Lindholm , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Content-Type: text/plain; charset="UTF-8" On Fri, 6 Mar 2020 at 13:46, Pete Batard 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 > > --- > > 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 > Tested-by: Pete Batard > Agreed Pushed as 91ed4f904e16..828fcbf96a38 Thanks all.