[edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB
Pete Batard
pete at akeo.ie
Fri May 1 15:16:25 UTC 2020
On 2020.05.01 16:13, Ard Biesheuvel wrote:
> On 5/1/20 5:11 PM, Pete Batard wrote:
>> On 2020.05.01 14:19, Ard Biesheuvel wrote:
>>> On 4/30/20 11:16 PM, Andrei Warkentin wrote:
>>>> Initially, FdtDxe used an internal (embedded in UEFI) FDT,
>>>> because it was neither understood how to consume the
>>>> one loaded by the VideoCore firmware, nor understood just
>>>> how important it is to use the DTB provided by config.txt.
>>>>
>>>> Embedding the DT was a bad idea, because:
>>>> - Permanently stale
>>>> - No overlays
>>>>
>>>> Also, on devices like the Pi 4 you _have_ to have a DT
>>>> around for the start4 VPU firmware to pick up, otherwise
>>>> the board is left in an inconsistent state. So we're being
>>>> proscriptive now about DT use with config.txt, which means
>>>> this internal DT logic is deadc code.
>>>>
>>>> Further FdtDxe cleanups are possible and will be handled
>>>> separately, specifically:
>>>> - probably no need to use a separate allocation for patched DT
>>>> (optimize memory used)
>>>> - suspicious use of EfiBootServicesData (I filed
>>>> https://github.com/ARM-software/ebbr/issues/45 to sort out the real
>>>> requirements)
>>>>
>>>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2).
>>>>
>>>> Signed-off-by: Andrei Warkentin <andrey.warkentin at gmail.com>
>>>> ---
>>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206
>>>> ++++----------------
>>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 -
>>>> Platform/RaspberryPi/RPi3/RPi3.fdf | 11 --
>>>> Platform/RaspberryPi/RPi4/RPi4.fdf | 8 -
>>>> Platform/RaspberryPi/RaspberryPi.dec | 7 -
>>>> 5 files changed, 38 insertions(+), 198 deletions(-)
>>>>
>>>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>>>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>>>> index e7143f57..187b9566 100644
>>>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>>>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>>>> @@ -335,90 +335,6 @@ CleanSimpleFramebuffer (
>>>> return EFI_SUCCESS;
>>>> }
>>>> -#define MAX_CMDLINE_SIZE 512
>>>> -
>>>> -STATIC
>>>> -EFI_STATUS
>>>> -UpdateBootArgs (
>>>> - VOID
>>>> - )
>>>> -{
>>>> - INTN Node;
>>>> - INTN Retval;
>>>> - EFI_STATUS Status;
>>>> - CHAR8 *CommandLine;
>>>> -
>>>> - //
>>>> - // Locate the /chosen node
>>>> - //
>>>> - Node = fdt_path_offset (mFdtImage, "/chosen");
>>>> - if (Node < 0) {
>>>> - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n",
>>>> __FUNCTION__));
>>>> - return EFI_NOT_FOUND;
>>>> - }
>>>> -
>>>> - //
>>>> - // If /chosen/bootargs already exists, we want to add a space
>>>> character
>>>> - // before adding the firmware supplied arguments. However, the
>>>> RpiFirmware
>>>> - // protocol expects a 32-bit aligned buffer. So let's allocate 4
>>>> bytes of
>>>> - // slack, and skip the first 3 when passing this buffer into libfdt.
>>>> - //
>>>> - CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4;
>>>> - if (!CommandLine) {
>>>> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n",
>>>> __FUNCTION__));
>>>> - return EFI_OUT_OF_RESOURCES;
>>>> - }
>>>> -
>>>> - //
>>>> - // Get the command line from the firmware
>>>> - //
>>>> - Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE,
>>>> CommandLine + 4);
>>>> - if (EFI_ERROR (Status)) {
>>>> - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n",
>>>> __FUNCTION__));
>>>> - return Status;
>>>> - }
>>>> -
>>>> - if (AsciiStrLen (CommandLine + 4) == 0) {
>>>> - DEBUG ((DEBUG_INFO, "%a: empty command line received\n",
>>>> __FUNCTION__));
>>>> - return EFI_SUCCESS;
>>>> - }
>>>> -
>>>> - CommandLine[3] = ' ';
>>>> -
>>>> - Retval = fdt_appendprop_string (mFdtImage, Node, "bootargs",
>>>> &CommandLine[3]);
>>>> - if (Retval != 0) {
>>>> - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs
>>>> property (%d)\n",
>>>> - __FUNCTION__, Retval));
>>>> - }
>>>> -
>>>> - DEBUG_CODE_BEGIN ();
>>>> - CONST CHAR8 *Prop;
>>>> - INT32 Length;
>>>> - INT32 Index;
>>>> -
>>>> - Node = fdt_path_offset (mFdtImage, "/chosen");
>>>> - ASSERT (Node >= 0);
>>>> -
>>>> - Prop = fdt_getprop (mFdtImage, Node, "bootargs", &Length);
>>>> - ASSERT (Prop != NULL);
>>>> -
>>>> - DEBUG ((DEBUG_INFO, "Command line set from firmware (length
>>>> %d):\n'", Length));
>>>> -
>>>> - for (Index = 0; Index < Length; Index++, Prop++) {
>>>> - if (*Prop == '\0') {
>>>> - continue;
>>>> - }
>>>> - DEBUG ((DEBUG_INFO, "%c", *Prop));
>>>> - }
>>>> -
>>>> - DEBUG ((DEBUG_INFO, "'\n"));
>>>> - DEBUG_CODE_END ();
>>>> -
>>>> - FreePool (CommandLine - 4);
>>>> - return EFI_SUCCESS;
>>>> -}
>>>> -
>>>> -
>>>> /**
>>>> @param ImageHandle of the loaded driver
>>>> @param SystemTable Pointer to the System Table
>>>> @@ -435,13 +351,10 @@ FdtDxeInitialize (
>>>> IN EFI_SYSTEM_TABLE *SystemTable
>>>> )
>>>> {
>>>> + INT32 Retval;
>>>> EFI_STATUS Status;
>>>> - EFI_GUID *FdtGuid;
>>>> - VOID *FdtImage;
>>>> UINTN FdtSize;
>>>> - INT32 Retval;
>>>> - UINT32 BoardRevision;
>>>> - BOOLEAN Internal;
>>>> + VOID *FdtImage = NULL;
>>>> if (PcdGet32 (PcdOptDeviceTree) == 0) {
>>>> DEBUG ((DEBUG_INFO, "Device Tree disabled per user
>>>> configuration\n"));
>>>> @@ -450,77 +363,28 @@ FdtDxeInitialize (
>>>> Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid,
>>>> NULL,
>>>> (VOID**)&mFwProtocol);
>>>> - ASSERT_EFI_ERROR (Status);
>>>> + if (mFwProtocol == NULL) {
>>>> + /*
>>>> + * Impossible because of the depex...
>>>> + */
>>>> + ASSERT_EFI_ERROR (Status);
>>>> + return EFI_NOT_FOUND;
>>>> + }
>>>
>>> Do we need this change?
>>
>> Looking at what we are doing elsewhere, I thing we should go with:
>>
>> ASSERT_EFI_ERROR (Status);
>> if (EFI_ERROR (Status)) {
>> return Status;
>> }
>>
>> The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so we
>> definitely want to return an error code here if needed, and I guess
>> that, technically, we could consider that LocateProtocol () may
>> succeed and return a NULL value for mFwProtocol, but I doubt that's a
>> valid consideration.
>>
>
> But the DEPEX guarantees that the module will only be dispatched at a
> time when LocateProtocol() will succeed. This is a common pattern, so I
> wonder why it is being silently modified here.
>
Fair enough. Let's wait for Andrei to reply on this.
/Pete
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58512): https://edk2.groups.io/g/devel/message/58512
Mute This Topic: https://groups.io/mt/73385235/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list