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

[Igor Grinberg]

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?

> +
> +	/* Align data offset to 4-byte boundrary */
> +	fit_size = fdt_totalsize(fit_header);
> +	fit_size = (fit_size + 3) & ~3;
> +
> +	/* Read in the splash data */
> +	location->offset = (location->offset + fit_size + splash_offset);
> +	res = splash_storage_read_raw(location, bmp_load_addr , splash_size);
> +	if (res < 0)
> +		return res;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_FIT */
> +
>  /**
>   * splash_source_load - load splash image from a supported location.
>   *
> @@ -277,5 +344,9 @@ int splash_source_load(struct splash_location *locations, uint size)
>  		return splash_load_raw(splash_location, bmp_load_addr);
>  	else if (splash_location->flags == SPLASH_STORAGE_FS)
>  		return splash_load_fs(splash_location, bmp_load_addr);
> +#ifdef CONFIG_FIT
> +	else if (splash_location->flags == SPLASH_STORAGE_FIT)
> +		return splash_load_fit(splash_location, bmp_load_addr);
> +#endif
>  	return -EINVAL;
>  }
> diff --git a/doc/README.splashprepare b/doc/README.splashprepare
> index 56c1bef..f1418de 100644
> --- a/doc/README.splashprepare
> +++ b/doc/README.splashprepare
> @@ -5,7 +5,7 @@ The splash_screen_prepare() function is a weak function defined in
>  common/splash.c. It is called as part of the splash screen display
>  sequence. It gives the board an opportunity to prepare the splash
>  image data before it is processed and sent to the frame buffer by
> -U-Boot.  Define your own version to use this feature.
> +U-Boot. Define your own version to use this feature.
>  
>  CONFIG_SPLASH_SOURCE
>  
> @@ -20,7 +20,12 @@ splashsource works as follows:
>  - If splashsource is undefined, use the first splash location as default.
>  - If splashsource is set to an unsupported value, do not load a splash screen.
>  
> -A splash source location can describe either storage with raw data, or storage
> -formatted with a file system. In case of a filesystem, the splash screen data is
> -loaded as a file. The name of the splash screen file can be controlled with the
> -environment variable "splashfile".
> +A splash source location can describe either storage with raw data, a storage
> +formatted with a file system or a FIT image. In case of a filesystem, the splash
> +screen data is loaded as a file. The name of the splash screen file can be
> +controlled with the environment variable "splashfile".
> +
> +To enable loading the splash image from a FIT image, CONFIG_FIT must be
> +enabled. Struct splash_location field 'name' should match the splash image
> +name within the FIT and the FIT should start at the 'offset' field address in
> +the specified storage.
> diff --git a/include/image.h b/include/image.h
> index f9ee564..2072a93 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -793,6 +793,8 @@ int bootz_setup(ulong image, ulong *start, ulong *end);
>  
>  /* image node */
>  #define FIT_DATA_PROP		"data"
> +#define FIT_DATA_OFFSET_PROP	"data-offset"
> +#define FIT_DATA_SIZE_PROP	"data-size"
>  #define FIT_TIMESTAMP_PROP	"timestamp"
>  #define FIT_DESC_PROP		"description"
>  #define FIT_ARCH_PROP		"arch"
> @@ -870,6 +872,8 @@ int fit_image_get_load(const void *fit, int noffset, ulong *load);
>  int fit_image_get_entry(const void *fit, int noffset, ulong *entry);
>  int fit_image_get_data(const void *fit, int noffset,
>  				const void **data, size_t *size);
> +int fit_image_get_data_offset(const void *fit, int noffset, int *data_offset);
> +int fit_image_get_data_size(const void *fit, int noffset, int *data_size);
>  
>  int fit_image_hash_get_algo(const void *fit, int noffset, char **algo);
>  int fit_image_hash_get_value(const void *fit, int noffset, uint8_t **value,
> diff --git a/include/splash.h b/include/splash.h
> index f0755ca..9c71581 100644
> --- a/include/splash.h
> +++ b/include/splash.h
> @@ -33,8 +33,9 @@ enum splash_storage {
>  };
>  
>  enum splash_flags {
> -	SPLASH_STORAGE_RAW,
> -	SPLASH_STORAGE_FS,
> +	SPLASH_STORAGE_RAW, /* Stored in raw memory */
> +	SPLASH_STORAGE_FS,  /* Stored within a file system */
> +	SPLASH_STORAGE_FIT, /* Stored inside a FIT image */
>  };
>  
>  struct splash_location {
> 

-- 
Regards,
Igor.