[U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Simon Glass]

Hi Igor,

On 11 December 2016 at 10:37, Igor Grinberg <grinberg at compulab.co.il> wrote:
> Hi Tomas, Simon,
>
> Sorry, to break in that late...
> I have a quick question below.
>
> On 12/05/16 09:36, Tomas Melin wrote:
>> Enable support for loading a splash image from within a FIT image.
>> The image is assumed to be generated with mkimage -E flag to hold
>> the data external to the FIT.
>>
>> Signed-off-by: Tomas Melin <tomas.melin at vaisala.com>
>
> [...]
>
>> diff --git a/common/splash_source.c b/common/splash_source.c
>> index 70d724f..94b46d3 100644
>> --- a/common/splash_source.c
>> +++ b/common/splash_source.c
>
> [...]
>
>> +#ifdef CONFIG_FIT
>> +static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>> +{
>> +     int res;
>> +     int node_offset;
>> +     int splash_offset;
>> +     int splash_size;
>> +     struct image_header *img_header;
>> +     const u32 *fit_header;
>> +     u32 fit_size;
>> +     const size_t header_size = sizeof(struct image_header);
>> +
>> +     /* Read in image header */
>> +     res = splash_storage_read_raw(location, bmp_load_addr, header_size);
>> +     if (res < 0)
>> +             return res;
>> +
>> +     img_header = (struct image_header *)bmp_load_addr;
>> +     fit_size = fdt_totalsize(img_header);
>> +
>> +     /* Read in entire FIT */
>> +     fit_header = (const u32 *)(bmp_load_addr + header_size);
>> +     res = splash_storage_read_raw(location, (u32)fit_header, fit_size);
>> +     if (res < 0)
>> +             return res;
>> +
>> +     res = fit_check_format(fit_header);
>> +     if (!res) {
>> +             debug("Could not find valid FIT image\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     node_offset = fit_image_get_node(fit_header, location->name);
>> +     if (node_offset < 0) {
>> +             debug("Could not find splash image '%s' in FIT\n",
>> +                   location->name);
>> +             return -ENOENT;
>> +     }
>> +
>
> I think two above debug() are very legitimate - no need to shout if no FIT image
> or no splash in it...
>
>> +     res = fit_image_get_data_offset(fit_header, node_offset,
>> +                                     &splash_offset);
>> +     if (res < 0) {
>> +             debug("Could not find 'data-offset' property in FIT\n");
>> +             return res;
>> +     }
>> +
>> +     res = fit_image_get_data_size(fit_header, node_offset, &splash_size);
>> +     if (res < 0) {
>> +             debug("Could not find 'data-size' property in FIT\n");
>> +             return res;
>> +     }
>
> Now regarding these two, I'm not sure.
> Since we have found a valid FIT and also a node with a correct splash name,
> probably the intent is that we show the splash, right?
> But in the two above checks, we find inconsistencies that do not allow us to
> show the splash - meaning the FIT is not actually good (am I right here?).
> So may be we should report it to the 'user' and allow correcting the FIT?
> Otherwise, it is impossible to debug the image w/o a debug version of U-Boot...
> Do I make sense, or do I miss something?

Yes that makes some sense, but the problem is that then you are
including error messages always which would never happen in a working
system (i.e. it just bloats the code).

So long as the error is reported (even if it is not a very specific
error), people can add DEBUG and track it down.

Regards,
Simon