[U-Boot] [PATCH 1/6] fdtdec: allow board to provide fdt for CONFIG_OF_SEPARATE

[Simon Glass]

Hi,

On 7 August 2017 at 07:54, Tom Rini <trini at konsulko.com> wrote:
> On Thu, Aug 03, 2017 at 12:48:30PM -0400, Rob Clark wrote:
>
>> Similar to CONFIG_OF_BOARD, but in this case the fdt is still built by
>> u-boot build.  This allows the board to patch the fdt, etc.
>>
>> In the specific case of dragonboard 410c, we pass the u-boot generated
>> fdt to the previous stage of bootloader (by embedding it in the
>> u-boot.img that is loaded by lk/aboot), which patches the fdt and passes
>> it back to u-boot.
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>>  include/fdtdec.h |  3 ++-
>>  lib/fdtdec.c     | 45 ++++++++++++++++++++++++++-------------------
>>  2 files changed, 28 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/fdtdec.h b/include/fdtdec.h
>> index 4a0947c626..b9acec735a 100644
>> --- a/include/fdtdec.h
>> +++ b/include/fdtdec.h
>> @@ -986,7 +986,8 @@ int fdtdec_setup(void);
>>
>>  /**
>>   * Board-specific FDT initialization. Returns the address to a device tree blob.
>> - * Called when CONFIG_OF_BOARD is defined.
>> + * Called when CONFIG_OF_BOARD is defined, or if CONFIG_OF_SEPARATE is defined
>> + * and the board implements it.
>>   */
>>  void *board_fdt_blob_setup(void);
>>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> index d2dbd0f122..07c458673c 100644
>> --- a/lib/fdtdec.c
>> +++ b/lib/fdtdec.c
>> @@ -1203,34 +1203,41 @@ int fdtdec_setup_memory_banksize(void)
>>  }
>>  #endif
>>
>> -int fdtdec_setup(void)
>> +#ifdef CONFIG_OF_SEPARATE
>> +/*
>> + * For CONFIG_OF_SEPARATE, the board may optionally implement this to
>> + * provide and/or fixup the fdt.
>> + */
>> +__weak void *board_fdt_blob_setup(void)
>>  {
>> -#if CONFIG_IS_ENABLED(OF_CONTROL)
>> -# ifdef CONFIG_OF_EMBED
>> -     /* Get a pointer to the FDT */
>> -     gd->fdt_blob = __dtb_dt_begin;
>> -# elif defined CONFIG_OF_SEPARATE
>> -#  ifdef CONFIG_SPL_BUILD
>> +     void *fdt_blob = NULL;
>> +#ifdef CONFIG_SPL_BUILD
>>       /* FDT is at end of BSS unless it is in a different memory region */
>>       if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS))
>> -             gd->fdt_blob = (ulong *)&_image_binary_end;
>> +             fdt_blob = (ulong *)&_image_binary_end;
>>       else
>> -             gd->fdt_blob = (ulong *)&__bss_end;
>> +             fdt_blob = (ulong *)&__bss_end;
>>
>> -#  elif defined CONFIG_FIT_EMBED
>> -     gd->fdt_blob = locate_dtb_in_fit(&_end);
>> +#elif defined CONFIG_FIT_EMBED
>> +     fdt_blob = locate_dtb_in_fit(&_end);
>>
>> -     if (gd->fdt_blob == NULL || gd->fdt_blob <= ((void *)&_end)) {
>> +     if (fdt_blob == NULL || fdt_blob <= ((void *)&_end))
>>               puts("Failed to find proper dtb in embedded FIT Image\n");
>> -             return -1;
>> -     }
>> -
>> -#  else
>> +#else
>>       /* FDT is at end of image */
>> -     gd->fdt_blob = (ulong *)&_end;
>> +     fdt_blob = (ulong *)&_end;
>>  #  endif
>> -# elif defined(CONFIG_OF_BOARD)
>> -     /* Allow the board to override the fdt address. */
>> +     return fdt_blob;
>> +}
>> +#endif
>> +
>> +int fdtdec_setup(void)
>> +{
>> +#if CONFIG_IS_ENABLED(OF_CONTROL)
>> +# ifdef CONFIG_OF_EMBED
>> +     /* Get a pointer to the FDT */
>> +     gd->fdt_blob = __dtb_dt_begin;
>> +# elif defined(CONFIG_OF_SEPARATE) || defined(CONFIG_OF_BOARD)
>>       gd->fdt_blob = board_fdt_blob_setup();
>>  # elif defined(CONFIG_OF_HOSTFILE)
>>       if (sandbox_read_fdt_from_file()) {
>
> Reviewed-by: Tom Rini <trini at konsulko.com>
>
> Simon, what do you think?  Thanks!

OK I see. I am not very keen on this, but at least I understand the problem.

I think the CONFIG_OF_BOARD thing should in fact be a separate option
from the others (i.e. it should be outside the 'choice'). Calling that
function can then be enabled independently of which option is used.
This will allow the fixup to be used regardless of the means of
obtaining an initial DT.

Then either we use CONFIG_OF_SEPARATE with the understanding that in
fact the DT might not be there, or (perhaps) create a new
CONFIG_OF_NONE to indicate that we don't have a DT (until
board_fdt_blob_setup() is called).

Regards,
Simon