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

Hi Tomas,

On 25 November 2016 at 02:45, Tomas Melin tomas.melin@vaisala.com 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@vaisala.com

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 <sata.h> #include <bmp_layout.h> #include <fs.h> +#include <fdt_support.h> +#include <image.h>

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, node_offset, &splash_offset);
  

•   if (res < 0) {
  

•           printf("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) {
  

•           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.

•           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(, ..., &data, &len) 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.

+#endif /* CONFIG_FIT */

/**

• splash_source_load - load splash image from a supported location.

@@ -331,5 +398,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/include/image.h b/include/image.h index 2b1296c..94df589 100644 --- a/include/image.h +++ b/include/image.h @@ -870,6 +870,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" @@ -948,6 +950,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 136eac7..228aff4 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 {

2.1.4

Regards, Simon