[U-Boot] [PATCH 1/1] efi_loader: pass address to efi_install_fdt()

[Heinrich Schuchardt]

On 12/3/19 10:31 AM, Alexander Graf wrote:
>
> On 03.12.19 11:26, Alexander Graf wrote:
>>
>> On 03.12.19 09:08, Heinrich Schuchardt wrote:
>>> On 12/3/19 7:37 AM, Alexander Graf wrote:
>>>>
>>>> On 03.12.19 08:27, Heinrich Schuchardt wrote:
>>>>> As part of moving the parsing of command line arguments to
>>>>> do_bootefi()
>>>>> call efi_install_fdt() with the address of the device tree instead
>>>>> of a
>>>>> string.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>>> ---
>>>>>   cmd/bootefi.c | 30 ++++++++++++++++--------------
>>>>>   1 file changed, 16 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>> index f613cce7e2..3cf190889e 100644
>>>>> --- a/cmd/bootefi.c
>>>>> +++ b/cmd/bootefi.c
>>>>> @@ -196,40 +196,37 @@ static void *get_config_table(const efi_guid_t
>>>>> *guid)
>>>>>   #endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
>>>>>
>>>>>   /**
>>>>> - * efi_install_fdt() - install fdt passed by a command argument
>>>>> + * efi_install_fdt() - install device tree
>>>>>    *
>>>>> - * If fdt_opt is available, the device tree located at that memory
>>>>> address will
>>>>> + * If fdt_addr is available, the device tree located at that memory
>>>>> address will
>>>>>    * will be installed as configuration table, otherwise the device
>>>>> tree located
>>>>>    * at the address indicated by environment variable fdtcontroladdr
>>>>> will be used.
>>>>>    *
>>>>> - * On architectures (x86) using ACPI tables device trees shall not be
>>>>> installed
>>>>> - * as configuration table.
>>>>> + * On architectures using ACPI tables device trees shall not be
>>>>> installed as
>>>>> + * configuration table.
>>>>>    *
>>>>> - * @fdt_opt:    pointer to argument
>>>>> + * @fdt_addr:    address of device tree
>>>>>    * Return:    status code
>>>>>    */
>>>>> -static efi_status_t efi_install_fdt(const char *fdt_opt)
>>>>> +static efi_status_t efi_install_fdt(uintptr_t fdt_addr)
>>>>>   {
>>>>>       /*
>>>>>        * The EBBR spec requires that we have either an FDT or an ACPI
>>>>> table
>>>>>        * but not both.
>>>>>        */
>>>>>   #if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
>>>>> -    if (fdt_opt) {
>>>>> +    if (fdt_addr) {
>>>>
>>>>
>>>> Why check for fdt_addr != 0 here? Since you do the parsing outside of
>>>> this function now, just make 0 a valid pointer and check for the
>>>> validity outside of this function.
>>>
>>> fdt_addr == 0 signals that U-Boot's internal device tree shall be used
>>> for the UEFI sub-system.
>>>
>>> Your suggested change would drop the capability to use the internal
>>> device tree. Why would you want to do so?
>>
>>
>> Mostly because I didn't look at the bigger picture, sorry :).
>> Looking at all the options, your patch is probably the best solution.
>> Sorry for the fuss.
>
>
> Actually, thinking about it again, can we make the "Please fall back to
> internal device tree" token explicit instead of the implicit 0? It might
> be enough to just do something as simple as
>
>    #define EFI_FDT_USE_INTERNAL 0
>
> and then use that in the if(!fdt_addr) line and in do_bootefi() instead.
> Then document it in the function header. That way we can make it an
> actual supported API that FIT image support can rely on.
>

Hello Alex,

some boards have CONFIG_OF_BOARD=y (device tree provided by board at
runtime) like qemu_arm64_defconfig and rpi_4_defconfig. I guess it would
be preferable to use the device tree pointed to by 'fdt_addr' in this
case instead of using the internal device tree indicated by
'fdtcontroladdr'.

According to doc/README.distro the mere existence of environment
variable fdt_addr indicates that a hardware supplied device tree is
available independent of CONFIG_OF_BOARD=y.

But some boards play it differently, e.g. include/configs/odroid.h. Here
fdt_addr is set after loading a device tree successfully.

After all I think if no device tree address is passed to do_bootefi()
and ACPI is not enabled we should first check if fdt_addr is defined and
only if it is not defined fall back to fdtcontroladdr.

Best regards

Heinrich