From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web09.11948.1583498777143061983 for ; Fri, 06 Mar 2020 04:46:17 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=IKfX0j2m; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.66, mailfrom: pete@akeo.ie) Received: by mail-wm1-f66.google.com with SMTP id i9so2252875wml.4 for ; Fri, 06 Mar 2020 04:46:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=OAA9fWCBzuUfIy3vWSRJ1ZSgdLblLTHNkNmHnewkEBs=; b=IKfX0j2mXkAOQJD9PVbSiiqbXHXUlO4yR4RFxMEh5B0dYB6RL16DOlS3Zb7jO15ni5 d+kSQAMnkV9S2GfVCFseefYap//skDbKsdTHvCSbTo4mrI48Xn+3lmW0Agu5RLI8S37/ Zdqv2KdF1Rcx0iw3mAVGuj3NO+S9dG6Seh6l+vNKXZY76Nq9tqKefFLWI2wTIElFEi9T /7ka1XlRF8JxXCW+GIONdJ8YMYlNAh0mRxWw8e789fnFykGBk5ibEpMEcbZ0NafXH3UN fULq9vbN6YryS6Gs8x2SBTQVuxvnGOKeQr9h7hgBtYomMMnwaEgoFQEcPgjQkYXJMvvU bT5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=OAA9fWCBzuUfIy3vWSRJ1ZSgdLblLTHNkNmHnewkEBs=; b=aLsFsHirCGmqwt2oFHxdzD8qG7M4Vt8csRaKE1+45qjCVYXx62h8DiI5y9IU8uLRmb G9wmdI4UMG8b/Fmne/3Jy+ds9R3IqGmLdpHydV7rC9VWp43TFjEoETdxmcYE9pSPaEvm PipSVxlRDbwXo4GZKy4uRpoDAolBtjknk//n1AjgwyspGC/8/w8q4IiROix5NH6zOthB hL9bbB6kGst+IAuwsLIVLwM3Zz6prNI6sa6z1ERutR5sHMkzBvo8HfdF56ORLt+/IPUp JyuejYvs92tHTRSJuiKQl4DwQ0FeN2OA0JiTZnKDFS2M41xvhsqL5JgokLzd0cs1/5uH tuPw== X-Gm-Message-State: ANhLgQ0RgKzHD90KNpMK0c1/fDr+1o2ISc1Mh+qgwSZF4+7zbWg73HNI bkkSSBK0mr3m3DuQnUHB22hZDA== X-Google-Smtp-Source: ADFU+vsKUMTH0VIbT2gw8vw4xAFhLMg+UAiWgv5cBH9l92Wmi+KVzeA0yQLldsrSr732e6nQaEd1Zg== X-Received: by 2002:a1c:9a88:: with SMTP id c130mr4019592wme.73.1583498775621; Fri, 06 Mar 2020 04:46:15 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.49.209]) by smtp.googlemail.com with ESMTPSA id u17sm24518469wrq.74.2020.03.06.04.46.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 06 Mar 2020 04:46:14 -0800 (PST) Subject: Re: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: fix FDT handling for RPi4 To: Andrei Warkentin , devel@edk2.groups.io Cc: ard.biesheuvel@linaro.org, leif@nuviainc.com, philmd@redhat.com References: <20200306055323.39073-1-andrey.warkentin@gmail.com> From: "Pete Batard" Message-ID: <326fc408-43a8-3a10-ea42-c4a38b7b3381@akeo.ie> Date: Fri, 6 Mar 2020 12:46:13 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200306055323.39073-1-andrey.warkentin@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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 > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] > gRaspberryPiTokenSpaceGuid.PcdCpuClock|0|UINT32|0x0000000d