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

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@arm.com>
---
 common/spl/spl_fit.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

Hi Andre,

On 28 February 2017 at 19:25, Andre Przywara <andre.przywara@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@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?

> +               spl_image->entry_point = spl_image->load_addr;
> +
>         return 0;
>  }
> --
> 2.8.2
>

Regards,
Simon

On Wednesday 01 March 2017 07:55 AM, Andre Przywara 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@arm.com>


Reviewed-by: Lokesh Vutla <lokeshvuta@ti.com>

Thanks and regards,
Lokesh

On 08/03/17 21:00, Simon Glass wrote:
> Hi Andre,
> 
> On 28 February 2017 at 19:25, Andre Przywara <andre.przywara@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@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 added a comment there to point this out.

Cheers,
Andre.

>> +               spl_image->entry_point = spl_image->load_addr;
>> +
>>         return 0;
>>  }
>> --
>> 2.8.2
>>
> 
> Regards,
> Simon
>

Hi Andre,

On 26 March 2017 at 19:19, André Przywara <andre.przywara@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@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@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