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

[Tomas Melin]

Hi Simon, 

On 11/29/2016 11:40 PM, Simon Glass wrote:
> Hi Tomas,
> 
> On 25 November 2016 at 02:45, 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 
>> ---
>>  common/image-fit.c | 49 ++
>>  common/splash_source.c | 71 
>> ++
>>  include/image.h|  4 +++
>>  include/splash.h   |  5 ++--
>>  4 files changed, 127 insertions(+), 2 deletions(-)
> 
> Sorry, a few comments...
> 
> (is there a 2/2 patch - is that the one to sort the includes? If so it
> would be better to put that one first I think, so this one just adds
> the new includes in the right place)
Yes, the patch is here: http://patchwork.ozlabs.org/patch/699136/
I changed the state of that patch to superseded.

> 
> Please can you use patman/checkpatch to check the patch?
> 
> 4 errors, 2 warnings, 0 checks for
> 0001-splash-add-support-for-loading-splash-from-a-FIT-ima.patch:
> error: common/splash_source.c,317: "(foo*)" should be "(foo *)"
> error: common/splash_source.c,321: "(foo*)" should be "(foo *)"
> warning: common/splash_source.c,339: line over 80 characters
> error: common/splash_source.c,350: trailing whitespace
> error: common/splash_source.c,406: code indent should use tabs where possible
> warning: common/splash_source.c,406: please, no spaces at the start of a line
> 
> Also, can you add some docs to README.splashprepare ?

Sorry for not noticing this before posting. Fixing those issues and adding 
documentation for next round.

>>  }
>> +
> 
> Please drop this blank line.
> 
>> +
>> +   res = fit_image_get_data_size(fit_header, node_offset, _size);
>> +   if (res < 0) {
>> +   printf("Could not find 'data-size' property in FIT\n");
> 
> Should any of these be debug(), or do you really expect these to be
> useful to the user? If we can avoid printf() it does cut the code
> size.

You are right that these are more or less debug level information. Changing to 
use debug().

> 
>> +   return res;
>> +   }
>> +
>> +   /* 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;
>> +}
> 
> I suppose this is fine. But really I would prefer to use something like:
> 
>fit_image_load(, ..., , )
>memcpy(dst, src_addr, len);
> 
> but that would presumably require quite a few changes, right? At least
> it would be good to know what is missing.

The reason I kept it like this is that fit_image_load() seems to assume 
the FIT image is already loaded to memory. This loads only the splash image data
from storage (not all FIT data). Also, it looks as fit_image_load() doesn't 
support data
in FIT images with data-offset properties (?).

spl_load_simple_fit() does similar things, but cannot really be used as such 
for getting
the data needed here.

I'll send out a new version of this patch-series.


BR,
Tomas
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Simon Glass]

Hi Tomas,

On 25 November 2016 at 02:45, 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 
> ---
>  common/image-fit.c | 49 ++
>  common/splash_source.c | 71 
> ++
>  include/image.h|  4 +++
>  include/splash.h   |  5 ++--
>  4 files changed, 127 insertions(+), 2 deletions(-)

Sorry, a few comments...

(is there a 2/2 patch - is that the one to sort the includes? If so it
would be better to put that one first I think, so this one just adds
the new includes in the right place)

Please can you use patman/checkpatch to check the patch?

4 errors, 2 warnings, 0 checks for
0001-splash-add-support-for-loading-splash-from-a-FIT-ima.patch:
error: common/splash_source.c,317: "(foo*)" should be "(foo *)"
error: common/splash_source.c,321: "(foo*)" should be "(foo *)"
warning: common/splash_source.c,339: line over 80 characters
error: common/splash_source.c,350: trailing whitespace
error: common/splash_source.c,406: code indent should use tabs where possible
warning: common/splash_source.c,406: please, no spaces at the start of a line

Also, can you add some docs to README.splashprepare ?

>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 77dc011..6f4e9e6 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -777,6 +777,54 @@ int fit_image_get_data(const void *fit, int noffset,
>  }
>
>  /**
> + * Get 'data-offset' property from a given image node.
> + *
> + * @fit: pointer to the FIT image header
> + * @noffset: component image node offset
> + * @data_offset: holds the data-offset property
> + *
> + * returns:
> + * 0, on success
> + * -ENOENT if the property could not be found
> + */
> +int fit_image_get_data_offset(const void *fit, int noffset, int *data_offset)
> +{
> +   const fdt32_t *val;
> +
> +   val = fdt_getprop(fit, noffset, FIT_DATA_OFFSET_PROP, NULL);
> +   if (!val)
> +   return -ENOENT;
> +
> +   *data_offset = fdt32_to_cpu(*val);
> +
> +   return 0;
> +}
> +
> +/**
> + * Get 'data-size' property from a given image node.
> + *
> + * @fit: pointer to the FIT image header
> + * @noffset: component image node offset
> + * @data_size: holds the data-size property
> + *
> + * returns:
> + * 0, on success
> + * -ENOENT if the property could not be found
> + */
> +int fit_image_get_data_size(const void *fit, int noffset, int *data_size)
> +{
> +   const fdt32_t *val;
> +
> +   val = fdt_getprop(fit, noffset, FIT_DATA_SIZE_PROP, NULL);
> +   if (!val)
> +   return -ENOENT;
> +
> +   *data_size = fdt32_to_cpu(*val);
> +
> +   return 0;
> +}
> +
> +/**
>   * fit_image_hash_get_algo - get hash algorithm name
>   * @fit: pointer to the FIT format image header
>   * @noffset: hash node offset
> @@ -1825,3 +1873,4 @@ int boot_get_setup_fit(bootm_headers_t *images, uint8_t 
> arch,
>
> return ret;
>  }
> +

Please drop this blank line.

> diff --git a/common/splash_source.c b/common/splash_source.c
> index 72df2c1..9ca523d 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c
> @@ -16,6 +16,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -295,6 +297,71 @@ static struct splash_location *select_splash_location(
> return NULL;
>  }
>
> +#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) {
> +   printf("Could not find valid FIT-image\n");
> +   return -EINVAL;
> +   }
> +
> +   node_offset = fit_image_get_node(fit_header, location->name);
> +   if (node_offset < 0) {
> +   printf("Could not find splash image '%s' in FIT\n",
> +  location->name);
> +   return -ENOENT;
> +   }
> +
> +   res = fit_image_get_data_offset(fit_header,