| Message ID | 1480067157-37955-1-git-send-email-tomas.melin@vaisala.com |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
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
Hi Simon, On 11/29/2016 11:40 PM, Simon Glass wrote: > 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) 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, &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. 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(, ..., &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. 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
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; } + 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"); + 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; +} +#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 {
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(-)