[U-Boot] [PATCH 05/17] SPL: FIT: allow loading multiple images

[Simon Glass]

Hi Andre,

On 26 March 2017 at 19:19, André Przywara <andre.przywara at arm.com> wrote:
> On 08/03/17 21:00, Simon Glass wrote:
>> Hi Andre,
>>
>> On 28 February 2017 at 19:25, Andre Przywara <andre.przywara at arm.com> wrote:
>>> So far we were not using the FIT image format to its full potential:
>>> The SPL FIT loader was just loading the first image from the /images
>>> node plus one of the listed DTBs.
>>> Now with the refactored loader code it's easy to load an arbitrary
>>> number of images in addition to the two mentioned above.
>>> As described in the FIT image source file format description, iterate
>>> over all images listed at the "loadables" property in the configuration
>>> node and load every image at its desired location.
>>> This allows to load any kind of images:
>>> - firmware images to execute before U-Boot proper (for instance
>>>   ARM Trusted Firmware (ATF))
>>> - firmware images for management processors (SCP, arisc, ...)
>>> - firmware images for devices like WiFi controllers
>>> - bit files for FPGAs
>>> - additional configuration data
>>> - kernels and/or ramdisks
>>> The actual usage of this feature would be platform and/or board specific.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>>> ---
>>>  common/spl/spl_fit.c | 32 ++++++++++++++++++++++++++++----
>>>  1 file changed, 28 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>> index ad5ba15..5583e09 100644
>>> --- a/common/spl/spl_fit.c
>>> +++ b/common/spl/spl_fit.c
>>> @@ -178,10 +178,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>>>         if (image_info) {
>>>                 image_info->load_addr = load_addr;
>>>                 image_info->size = length;
>>> -               if (entry == -1UL)
>>> -                       image_info->entry_point = load_addr;
>>> -               else
>>> -                       image_info->entry_point = entry;
>>> +               image_info->entry_point = entry;
>>
>> Need to update function comment to indicate that it can put -1 in here.
>>
>>>         }
>>>
>>>         return 0;
>>> @@ -196,6 +193,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>>         struct spl_image_info image_info;
>>>         int node, images;
>>>         int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>>> +       int index = 0;
>>>
>>>         /*
>>>          * Figure out where the external images start. This is the base for the
>>> @@ -240,6 +238,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>>         if (node < 0) {
>>>                 debug("could not find firmware image, trying loadables...\n");
>>>                 node = spl_fit_get_image_node(fit, images, "loadables", 0);
>>> +               /*
>>> +                * If we pick the U-Boot image from "loadables", start at
>>> +                * the second image when later loading additional images.
>>> +                */
>>> +               index = 1;
>>>         }
>>>         if (node < 0) {
>>>                 debug("%s: Cannot find u-boot image node: %d\n",
>>> @@ -265,5 +268,26 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>>         image_info.load_addr = spl_image->load_addr + spl_image->size;
>>>         spl_load_fit_image(info, sector, fit, base_offset, node, &image_info);
>>>
>>> +       /* Now check if there are more images for us to load */
>>> +       for (; ; index++) {
>>> +               node = spl_fit_get_image_node(fit, images, "loadables", index);
>>> +               if (node < 0)
>>> +                       break;
>>> +
>>> +               spl_load_fit_image(info, sector, fit, base_offset, node,
>>> +                                  &image_info);
>>
>> Error check?
>>
>>> +
>>> +               /*
>>> +                * If the "firmware" image did not provide an entry point,
>>> +                * use the first valid entry point from the loadables.
>>> +                */
>>> +               if (spl_image->entry_point == -1UL &&
>>> +                   image_info.entry_point != -1UL)
>>> +                       spl_image->entry_point = image_info.entry_point;
>>> +       }
>>> +
>>> +       if (spl_image->entry_point == -1UL || spl_image->entry_point == 0)
>>
>> Why does 0 mean there is no entry point? I suppose that is safe, but
>> would anyone use this?
>
> So this is due to U-Boot's own Makefile, which sets
> CONFIG_SYS_UBOOT_START to 0 if it isn't already defined. This then gets
> passed on to mkimage -f auto as the -e argument.
> So today's FIT images have 0 as an entry point - a least on sunxi.  As I
> wanted to retain compatibility with that, I added this check.
> Maybe sunxi (and other platforms?) should explicitly define the entry
> point to avoid it being set to 0, or we should default to the load
> address as the fallback in absence of such an explicit definition.
>
> In any case I wanted to keep existing u-boot.img files booting, so I
> added this safe guard here to do "the right thing (tm)".

I very much doubt that 0 is needed as a default, so it should be OK to
break that. I'm almost certain there is no test to verify it.
Sometimes there are wierd things in the code that we should try to
drop.

>
> I added a comment there to point this out.
>
> Cheers,
> Andre.

Regards,
Simon